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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

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:
>>
>> Add a generic [devm_]led_get() method which can be used on both devicetree
>> and non devicetree platforms to get a LED classdev associated with
>> a specific function on a specific device, e.g. the privacy LED associated
>> with a specific camera sensor.
>>
>> Note unlike of_led_get() this takes a string describing the function
>> rather then an index. This is done because e.g. camera sensors might
> 
> than
> 
>> have a privacy LED, or a flash LED, or both and using an index
>> approach leaves it unclear what the function of index 0 is if there is
>> only 1 LED.
>>
>> This uses a lookup-table mechanism for non devicetree platforms.
>> This allows the platform code to map specific LED class_dev-s to a specific
>> device,function combinations this way.
>>
>> For devicetree platforms getting the LED by function-name could be made
>> to work using the standard devicetree pattern of adding a -names string
>> array to map names to the indexes.
> 
> ...
> 
>> +/*
>> + * 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.

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.

Regards,

Hans