Re: [PATCH v2 18/18] auxdisplay: ht16k33: Add segment display LED support

[Geert Uytterhoeven]

Hi Marek,

On Mon, Jun 28, 2021 at 12:15 PM Marek Behun <marek.behun@xxxxxx> wrote:
> On Mon, 28 Jun 2021 11:21:04 +0200
> Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > On Fri, Jun 25, 2021 at 10:39 PM Marek Behun <marek.behun@xxxxxx> wrote:
> > > On Fri, 25 Jun 2021 14:59:02 +0200
> > > Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > >
> > > > Instantiate a single LED for a segment display.  This allows the user to
> > > > control display brightness and blinking through the LED class API and
> > > > triggers, and exposes the display color.
> > > > The LED will be named "auxdisplay:<color>:backlight".
> > >
> > > What if there are multiple "auxdisplay"s ?
> >
> > I understand the LED core will just add a suffix on a name collision.
> >
> > > Doesn't this subsystem have IDs? So that you can use auxdisplayN for
> > > device name, for example?
> >
> > Auxdisplay does not have IDs, as there is no subsystem to register
> > with.  It's just a collection of drivers for auxiliary displays with
> > no common API.  Some drivers use fbdev, others use a chardev, or an
> > attribute file in sysfs.
> >
> > BTW, I just followed Pavel's advice in naming.
>
> Very well.
>
> > > > +     of_property_read_u32(node, "color", &color);
> > > > +     seg->led.name = devm_kasprintf(dev, GFP_KERNEL,
> > > > +                     "auxdisplay:%s:" LED_FUNCTION_BACKLIGHT,
> > > > +                     color < LED_COLOR_ID_MAX ? led_colors[color] : "");
> > >
> > > If you use devm_led_classdev_register_ext and pass struct
> > > led_init_data, LED core will generate name of the LED itself.
> >
> > Will that make any difference, except for adding more code?
>
> You are hardcoding the backlight function. Using the _ext() registering
> function will make it so that the function and color are parsed from
> fwnode by LED core. I understand that the function will always be
> "backlight" in this case, but this should be specified in the
> device-tree anyway, so why not use it?
>
> > Looking at the implementation, I still have to use devm_kasprintf()
> > to combine color and function for led_init_data.default_label?
>
> AFAIK you don't have to fill in default_label. (If the needed OF
> properties are not present so that default_label is tried, it means the
> device-tree does not correctly specify the device. In that case I don't
> think it is a problem if the default_label is not present and LED
> core will use the OF node name as the LED name.)
>
> The code could look like this
>
>   struct led_init_data init_data = {};
>
>   init_data.fwnode = of_fwnode_handle(node);
>   init_data.devicename = "auxdisplay";
>   init_data.devname_mandatory = true;
>
>   ...register_ext();
>
> But if you still don't want to do this then ignore me :)

No, thanks a lot!

Your comments made me realize I should put the LED properties in an
"led" subnode, and defer all parsing to the LED core.
This also allows the user to use the more powerful LED mode even in
dot-matrix mode, while falling back to the existing backlight mode if
no "led" subnode is found, and thus preserving backwards compatibility.

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