Message-ID: <d7cc2c82-db99-c9a0-dc09-474b6a213865@gmail.com>
Date:   Fri, 5 Apr 2019 22:08:25 +0200
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Dan Murphy <dmurphy@...com>, linux-leds@...r.kernel.org
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        pavel@....cz, robh@...nel.org,
        Baolin Wang <baolin.wang@...aro.org>,
        Daniel Mack <daniel@...que.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Oleh Kravchenko <oleg@....org.ua>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Simon Shields <simon@...eageos.org>
Subject: Re: [PATCH v3 05/25] leds: core: Add support for composing LED class
 device names

Hi Dan,

Thank you for the review.

On 4/5/19 1:45 PM, Dan Murphy wrote:
> Jacek
> 
> On 3/31/19 12:54 PM, Jacek Anaszewski wrote:
>> Add generic support for composing LED class device name basing on
>> fwnode_handle data. The function composes device name according to
>> either a new <color:function> pattern or the legacy
>> <devicename:color:function> pattern. The decision on using the
>> particular pattern is made basing on whether fwnode contains new
>> "function" and "color" properties, or the legacy "label" proeprty.
>>
>> Backward compatibility with in-driver hard-coded LED class device
>> names is assured thanks to the default_label and led_hw_name properties
>> of newly introduced struct led_init_data.
>>
>> In case none of the aforementioned properties was found, then, for OF
>> nodes, the node name is adopted for LED class device name.
>>
>> At the occassion of amending the Documentation/leds/leds-class.txt
>> unify spelling: colour -> color.
>>
>> Alongside these changes added is a new tool - tools/leds/get_led_device_info.sh.
>> The tool allows retrieving details of a LED class device's parent device,
>> which proves that getting rid of a devicename section from LED name pattern
>> is justified since this information is already available in sysfs.
>>
>> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@...il.com>
>> Cc: Baolin Wang <baolin.wang@...aro.org>
>> Cc: Pavel Machek <pavel@....cz>
>> Cc: Dan Murphy <dmurphy@...com>
>> Cc: Daniel Mack <daniel@...que.org>
>> Cc: Linus Walleij <linus.walleij@...aro.org>
>> Cc: Oleh Kravchenko <oleg@....org.ua>
>> Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>
>> Cc: Simon Shields <simon@...eageos.org>
>> ---
>>   Documentation/leds/leds-class.txt | 27 +++++++++--
>>   drivers/leds/led-class.c          | 29 ++++++++++--
>>   drivers/leds/led-core.c           | 96 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/leds.h              | 43 ++++++++++++++++++
>>   tools/leds/get_led_device_info.sh | 81 +++++++++++++++++++++++++++++++++
>>   5 files changed, 270 insertions(+), 6 deletions(-)
>>   create mode 100755 tools/leds/get_led_device_info.sh
>>
>> diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt
>> index 8b39cc6b03ee..11e19c3c2e4d 100644
>> --- a/Documentation/leds/leds-class.txt
>> +++ b/Documentation/leds/leds-class.txt
>> @@ -43,14 +43,35 @@ LED Device Naming
>>   
>>   Is currently of the form:
>>   
>> -"devicename:colour:function"
>> -
>> -There have been calls for LED properties such as colour to be exported as
>> +"color:function"
>> +
>> +There might be still LED class drivers around using "devicename:color:function"
>> +naming pattern, but the "devicename" section is now deprecated since it used
>> +to convey information that was already available in the sysfs, like product
>> +name. There is a tool (tools/leds/get_led_device_info.sh) available for
>> +retrieving that information per a LED class device.
>> +
>> +Associations with other devices, like network ones, should be defined
>> +via LED trigger mechanism. This approach is applied by some of wireless
>> +network drivers that create triggers dynamically and incorporate phy
>> +name into the trigger name. On the other hand input subsystem offers LED - input
>> +bridge (drivers/input/input-leds.c) for exposing keyboard LEDs as LED class
>> +devices. The get_led_device_info.sh script has support for retrieving related
>> +input device node name. Should it support discovery of associations between
>> +LEDs and other subsystems, please don't hesitate to submit a relevant patch.
>> +
>> +There have been calls for LED properties such as color to be exported as
>>   individual led class attributes. As a solution which doesn't incur as much
>>   overhead, I suggest these become part of the device name. The naming scheme
>>   above leaves scope for further attributes should they be needed. If sections
>>   of the name don't apply, just leave that section blank.
>>   
>> +Please also keep in mind that LED subsystem has a protection against LED name
>> +conflict. It adds numerical suffix (e.g. "_1", "_2", "_3" etc.) to the requested
>> +LED class device name in case it is already in use. In order to prevent LED core
>> +from assigning these suffixes in an arbitrary order the led-enumerator fwnode
>> +property can be used for differentiation of LEDs that share common function
>> +and/or color. In this case enumerators will be prepended with "-" character.
>>   
>>   Brightness setting API
>>   ======================
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index 2f09156b0c63..bfd46a9bba63 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -26,6 +26,18 @@
>>   
>>   static struct class *leds_class;
>>   
>> +const char *led_colors[LED_COLOR_ID_COUNT] = {
>> +	[LED_COLOR_ID_WHITE] = "white",
>> +	[LED_COLOR_ID_RED] = "red",
>> +	[LED_COLOR_ID_GREEN] = "green",
>> +	[LED_COLOR_ID_BLUE] = "blue",
>> +	[LED_COLOR_ID_AMBER] = "amber",
>> +	[LED_COLOR_ID_VIOLET] = "violet",
>> +	[LED_COLOR_ID_YELLOW] = "yellow",
>> +	[LED_COLOR_ID_IR] = "ir",
>> +};
>> +EXPORT_SYMBOL_GPL(led_colors);
>> +
> 
> Why is this exported when it is only used here?
> 
> I can re-use this array for the multi color framework so I don't oppose it being exported.

I did that specifically for that purpose :-)

> Reviewed-by: Dan Murphy <dmurphy@...com>

Thanks!

-- 
Best regards,
Jacek Anaszewski

Powered by blists - more mailing lists