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

From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2021-10-01 15:51:33
Also in: linux-leds, linux-mips, lkml

Hoi Robin,

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

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>
Reviewed-by: Marek Behún <kabel@kernel.org>
---
v6:
  - Add Reviewed-by,
  - Reorder operations in ht16k33_led_probe() to ease future conversion
    to device properties,

quoted

--- 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? ;-)

quoted hunk ↗ jump to hunk

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.

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.

quoted

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help