Re: [PATCH v6 19/19] auxdisplay: ht16k33: Add LED support

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hoi Robin,

On Mon, Oct 4, 2021 at 10:26 AM Robin van der Gracht <robin@protonic.nl> wrote:
> On 2021-10-01 17:51, Geert Uytterhoeven wrote:
> > On Thu, Sep 30, 2021 at 12:57 PM Robin van der Gracht <robin@protonic.nl>
> > wrote:
> >> On 2021-09-14 16:38, Geert Uytterhoeven wrote:
> >> > Instantiate a single LED based on the "led" subnode in DT.
> >> > This allows the user to control display brightness and blinking (backed
> >> > by hardware support) through the LED class API and triggers, and exposes
> >> > the display color.  The LED will be named
> >> > "auxdisplay:<color>:<function>".
> >> >
> >> > When running in dot-matrix mode and if no "led" subnode is found, the
> >> > driver falls back to the traditional backlight mode, to preserve
> >> > backwards compatibility.
> >> >
> >> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> >> > +
> >> > +     err = devm_led_classdev_register_ext(dev, led, &init_data);
> >> > +     if (err)
> >> > +             dev_err(dev, "Failed to register LED\n");
> >>
> >> You might want to call ht16k33_brightness_set(priv, brightness) here to get
> >> a
> >> know value into the display setup register (0x80).
> >>
> >> Right now if I enable hardware blinking and (soft)reboot my board it keeps
> >> on
> >> blinking even after a re-probe.
> >
> > I don't have that issue.
> > Aha, ht16k33_seg_probe() calls ht16k33_brightness_set(), but
> > ht16k33_fbdev_probe() doesn't.  The latter should do that, too,
> > when not using backwards compatibility mode.
>
> Ack. I have hardware which uses the ht16k33 in dot matrix mode and I tested
> both the backlight and led setup. I ran into this with the fbdev + led setup.
>
> I noticed ht16k33_bl_update_status() is called in ht16k33_fbdev_probe()
> before the fbdev device is registered. Which is fine right now, but in theory
> the fbdev blank state can influence the backlight setting (nitpick since
> the fbdev device is unblanked by default).
>
> The point: Maybe ht16k33_brightness_set() (or ht16k33_bl_update_status() for
> backlight device) should be called in one central place (i.e at the end of
> the
> main probe function).

That would mean the main function need to know more about which mode
is being used, so I think it's better to leave it to the individual display
sub-drivers.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds