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

Hoi Robin,

On Mon, Oct 4, 2021 at 10:26 AM Robin van der Gracht [off-list ref] wrote:

On 2021-10-01 17:51, Geert Uytterhoeven wrote:

quoted

On Thu, Sep 30, 2021 at 12:57 PM Robin van der Gracht [off-list ref]
wrote:

quoted

On 2021-09-14 16:38, Geert Uytterhoeven wrote:

quoted

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>

quoted

quoted

quoted

+
+     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