Hoi Robin,
On Thu, Sep 30, 2021 at 12:57 PM Robin van der Gracht <robin@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > Reviewed-by: Marek Behún <kabel@xxxxxxxxxx>
> > ---
> > v6:
> > - Add Reviewed-by,
> > - Reorder operations in ht16k33_led_probe() to ease future conversion
> > to device properties,
> > --- a/drivers/auxdisplay/ht16k33.c
> > +++ b/drivers/auxdisplay/ht16k33.c
> > @@ -425,6 +477,35 @@ static void ht16k33_seg14_update(struct work_struct
> > *work)
> > i2c_smbus_write_i2c_block_data(priv->client, 0, ARRAY_SIZE(buf), buf);
> > }
> >
> > +static int ht16k33_led_probe(struct device *dev, struct led_classdev *led,
> > + unsigned int brightness)
> > +{
> > + struct led_init_data init_data = {};
> > + struct device_node *node;
> > + int err;
> > +
> > + /* The LED is optional */
> > + node = of_get_child_by_name(dev->of_node, "led");
> > + if (!node)
> > + return 0;
> > +
> > + init_data.fwnode = of_fwnode_handle(node);
> > + init_data.devicename = "auxdisplay";
> > + init_data.devname_mandatory = true;
> > +
> > + led->brightness_set_blocking = ht16k33_brightness_set_blocking;
> > + led->blink_set = ht16k33_blink_set;
> > + led->flags = LED_CORE_SUSPENDRESUME;
> > + led->brightness = brightness;
> > + led->max_brightness = MAX_BRIGHTNESS;
>
> What do you think about adding a default trigger and making it 'backlight'?
>
> led->default_trigger = "blacklight";
>
> Or as an alternative, suggesting linux,default-trigger = "backlight" in the
> docs? Since the led class won't respond to blank events by just making it's
> function LED_FUNCTION_BACKLIGHT.
>
> led {
> function = LED_FUNCTION_BACKLIGHT;
> color = <LED_COLOR_ID_GREEN>;
> linux,default-trigger = "backlight";
> };
The latter makes perfect sense to me. Will do.
> I noticed blanking is broken. The backlight device (or LED device with
> backlight trigger) doens't get notified when the framebuffer is blanked since
> the driver doesn't implement fb_blank.
>
> Right now:
>
> echo 1 > /sys/class/graphics/fb0/blank
> |
> sh: write error: Invalid argument
>
> Due to:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/video/fbdev/core/fbmem.c?h=v5.15-rc3#n1078
That's a pre-existing problem, righ? ;-)
> Something like this fixes it.
>
> diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> index 89ee5b4b3dfc..0883d5252c81 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -346,6 +346,15 @@ static int ht16k33_mmap(struct fb_info *info, struct
> vm_area_struct *vma)
> return vm_map_pages_zero(vma, &pages, 1);
> }
>
> +/*
> + * Blank events will be passed to the backlight device (or the LED device if
> + * it's trigger is 'backlight') when we return 0 here.
> + */
> +static int ht16k33_blank(int blank, struct fb_info *info)
> +{
> + return 0;
> +}
> +
> static const struct fb_ops ht16k33_fb_ops = {
> .owner = THIS_MODULE,
> .fb_read = fb_sys_read,
> @@ -354,6 +363,7 @@ static const struct fb_ops ht16k33_fb_ops = {
> .fb_copyarea = sys_copyarea,
> .fb_imageblit = sys_imageblit,
> .fb_mmap = ht16k33_mmap,
> + .fb_blank = ht16k33_blank,
> };
>
> /*
>
> Feel free to include (something like) this in the patch stack.
Thanks, will do.
> > +
> > + 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.
> > @@ -575,7 +660,7 @@ static int ht16k33_seg_probe(struct device *dev, struct
> > ht16k33_priv *priv,
> > struct ht16k33_seg *seg = &priv->seg;
> > int err;
> >
> > - err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
> > + err = ht16k33_brightness_set(priv, brightness);
>
> This looks like a bugfix for patch 17, maybe move this change there?
Indeed. Bad rebase. Will move.
Thanks a lot for your comments!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
