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

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Robin van der Gracht <robin@protonic.nl>
Cc: "Miguel Ojeda" <ojeda@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Paul Burton" <paulburton@kernel.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Pavel Machek" <pavel@ucw.cz>, "Marek Behun" <marek.behun@nic.cz>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	linux-leds <linux-leds@vger.kernel.org>,
	"open list:BROADCOM NVRAM DRIVER" <linux-mips@vger.kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Marek Behún" <kabel@kernel.org>
Subject: Re: [PATCH v6 19/19] auxdisplay: ht16k33: Add LED support
Date: Tue, 12 Oct 2021 17:08:23 +0200	[thread overview]
Message-ID: <CAMuHMdUrSpvVKPmi3EXvyKnDrq48Z5jvRY-a1kEoGSt2kS8J_Q@mail.gmail.com> (raw)
In-Reply-To: <bc1632943ecbb7e244b87c285501f706@protonic.nl>

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