Re: [PATCH v4 04/11] leds: led-class: Add generic [devm_]led_get()

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Thu, Jan 19, 2023 at 4:16 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 1/19/23 15:04, Andy Shevchenko wrote:
> > On Thu, Jan 19, 2023 at 3:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> >> +/*
> >> + * This is used to tell led_get() device which led_classdev to return for
> >> + * a specific consumer device-name, function pair on non devicetree platforms.
> >> + * Note all strings must be set.
> >> + */
> >> +struct led_lookup_data {
> >> +       struct list_head list;
> >> +       const char *led_name;
> >> +       const char *consumer_dev_name;
> >> +       const char *consumer_function;
> >> +};
> >
> > I'm wondering if it would be better to have something like
> >
> > struct led_function_map {
> >        const char *name;
> >        const char *function;
> > };
> >
> > struct led_lookup_data {
> >        struct list_head list;
> >        const char *dev_name;
> >        const struct led_function_map map[];
> > };
>
> Thank you for the review.
>
> Since led_lookup_data now is variable sized, AFAIK this means that
> the led_lookup_data now can no longer be embedded in another struct and
> instead it must always be dynamically allocated, including adding error
> checking + rollback for said allocation.

I'm not sure what you are talking about here. GPIO lookup table
defined in the same way and doesn't strictly require heap allocation.
For the embedding into another structure, it can be as the last entry AFAIU.

> If you look at the only current consumer of this:
>
> [PATCH v4 09/11] platform/x86: int3472/discrete: Create a LED class device for the privacy LED
>
> then the code there would become more complicated.

> > as you may have more than one LED per the device and it would be a
> > more compressed list in this case. I'm basically referring to the GPIO
> > lookup table.
>
> Right, but having more then 1 GPIO per device is quite common while
> I expect having more then 1 (or maybe 2) LEDs per device to be rare while
> at the same time the suggested change makes things slightly more
> complicated for consumers of the API which know before hand how much
> lookup entries they will need (typically 1).
>
> So all in all I believe staying with the current implementation is better
> but if there is a strong preference to switch to the structure you suggest
> then I have no objection against that.

I have no strong opinion, I just want to have fewer variants of the
lookup tables.
Anyway, reset framework has something similar to yours. Question: can you
rename fields to be something like dev_id, con_id, etc as it's done in the most
of the lookup data types?

-- 
With Best Regards,
Andy Shevchenko