Re: [PATCH 02/25] leds: core: Add support for composing LED class device names

[Dan Murphy]

On 3/12/19 12:28 PM, Jacek Anaszewski wrote:
> Hi Dan,
> 
> On 3/12/19 6:15 PM, Dan Murphy wrote:
>> On 3/10/19 1:28 PM, Jacek Anaszewski wrote:
>>> Add public led_compose_name() API 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.
>>>
>>> Backwards compatibility with in-driver hard-coded LED class device
>>> names is assured thanks to the default_desc argument.
>>>
>>> In case none of the aforementioned properties was found, then, for OF
>>> nodes, the node name is adopted for LED class device name.
>>>
>>> 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@xxxxxxxxx>
>>> Cc: Baolin Wang <baolin.wang@xxxxxxxxxx>
>>> Cc: Daniel Mack <daniel@xxxxxxxxxx>
>>> Cc: Dan Murphy <dmurphy@xxxxxx>
>>> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
>>> Cc: Oleh Kravchenko <oleg@xxxxxxxxxx>
>>> Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>>> ---
>>> Â Documentation/leds/leds-class.txt | 20 +++++++++-
>>> Â drivers/leds/led-core.cÂÂÂÂÂÂÂÂÂÂ | 82 +++++++++++++++++++++++++++++++++++++++
>>> Â include/linux/leds.hÂÂÂÂÂÂÂÂÂÂÂÂÂ | 31 +++++++++++++++
>>> Â tools/leds/get_led_device_info.sh | 81 ++++++++++++++++++++++++++++++++++++++
>>> Â 4 files changed, 213 insertions(+), 1 deletion(-)
>>> Â 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..866fe87063d4 100644
>>> --- a/Documentation/leds/leds-class.txt
>>> +++ b/Documentation/leds/leds-class.txt
>>> @@ -43,7 +43,22 @@ LED Device Naming
>>> Â Â Is currently of the form:
>>> Â -"devicename:colour:function"
>>> +"colour:function"
>>> +
>>> +There might be still LED class drivers around using "devicename:colour: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 triggr mechanism. This approach is applied by some of wireless
>>> +network drivers that create triggers dynamically and incorporate phy
>>> +name into its 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 colour to be exported as
>>> Â individual led class attributes. As a solution which doesn't incur as much
>>> @@ -51,6 +66,9 @@ 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.
>>> Â Â Brightness setting API
>>> Â ======================
>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>> index ede4fa0ac2cc..bad92250d1d5 100644
>>> --- a/drivers/leds/led-core.c
>>> +++ b/drivers/leds/led-core.c
>>> @@ -16,6 +16,8 @@
>>> Â #include <linux/list.h>
>>> Â #include <linux/module.h>
>>> Â #include <linux/mutex.h>
>>> +#include <linux/of.h>
>>> +#include <linux/property.h>
>>> Â #include <linux/rwsem.h>
>>> Â #include "leds.h"
>>> Â @@ -327,3 +329,83 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
>>> ÂÂÂÂÂ led_cdev->flags &= ~LED_SYSFS_DISABLE;
>>> Â }
>>> Â EXPORT_SYMBOL_GPL(led_sysfs_enable);
>>> +
>>> +static void led_parse_properties(struct fwnode_handle *fwnode,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct led_properties *props)
>>> +{
>>> +ÂÂÂ int ret;
>>> +
>>> +ÂÂÂ if (!fwnode)
>>> +ÂÂÂÂÂÂÂ return;
>>> +
>>> +ÂÂÂ if (fwnode_property_present(fwnode, "label")) {
>>> +ÂÂÂÂÂÂÂ ret = fwnode_property_read_string(fwnode, "label", &props->label);
>>> +ÂÂÂÂÂÂÂ if (ret)
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pr_err("Error parsing \'label\' property (%d)\n", ret);
>>> +ÂÂÂÂÂÂÂ return;
>>> +ÂÂÂ }
>>> +
>>> +ÂÂÂ if (fwnode_property_present(fwnode, "function")) {
>>> +ÂÂÂÂÂÂÂ ret = fwnode_property_read_string(fwnode, "function", &props->function);
>>> +ÂÂÂÂÂÂÂ if (ret)
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pr_err("Error parsing \'function\' property (%d)\n", ret);
>>> +ÂÂÂ } else {
>>> +ÂÂÂÂÂÂÂ pr_info("\'function\' property not found\n");
>>> +ÂÂÂ }
>>> +
>>> +ÂÂÂ if (fwnode_property_present(fwnode, "color")) {
>>> +ÂÂÂÂÂÂÂ ret = fwnode_property_read_string(fwnode, "color", &props->color);
>>> +ÂÂÂÂÂÂÂ if (ret)
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pr_info("Error parsing \'color\' property (%d)\n", ret);
>>> +ÂÂÂ } else {
>>> +ÂÂÂÂÂÂÂ pr_info("\'color\' property not found\n");
>>> +ÂÂÂ }
>>> +}
>>> +
>>> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂ const char *default_desc, char *led_classdev_name)
>>> +{
>>> +ÂÂÂ struct led_properties props = {};
>>> +
>>> +ÂÂÂ if (!led_classdev_name)
>>> +ÂÂÂÂÂÂÂ return -EINVAL;
>>> +
>>> +ÂÂÂ led_parse_properties(fwnode, &props);
>>> +
>>> +ÂÂÂ if (props.label) {
>>> +ÂÂÂÂÂÂÂ /*
>>> +ÂÂÂÂÂÂÂÂ * Presence of 'label' DT property implies legacy LED name,
>>> +ÂÂÂÂÂÂÂÂ * formatted as <devicename:color:function>, with possible
>>> +ÂÂÂÂÂÂÂÂ * section omission if doesn't apply to given device.
>>> +ÂÂÂÂÂÂÂÂ *
>>> +ÂÂÂÂÂÂÂÂ * If no led_hw_name has been passed, then it indicates that
>>> +ÂÂÂÂÂÂÂÂ * DT label should be used as-is for LED class device name.
>>> +ÂÂÂÂÂÂÂÂ * Otherwise the label is prepended with led_hw_name to compose
>>> +ÂÂÂÂÂÂÂÂ * the final LED class device name.
>>> +ÂÂÂÂÂÂÂÂ */
>>> +ÂÂÂÂÂÂÂ if (!led_hw_name) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ strncpy(led_classdev_name, props.label,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ LED_MAX_NAME_SIZE);
>>> +ÂÂÂÂÂÂÂ } else {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ led_hw_name, props.label);
>>> +ÂÂÂÂÂÂÂ }
>>> +ÂÂÂ } else if (props.function || props.color) {
>>> +ÂÂÂÂÂÂÂ snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂ props.color ?: "", props.function ?: "");
>>> +ÂÂÂ } else if (default_desc) {
>>> +ÂÂÂÂÂÂÂ if (!led_hw_name) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pr_err("Legacy LED naming requires devicename segment");
>>> +ÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
>>> +ÂÂÂÂÂÂÂ }
>>> +ÂÂÂÂÂÂÂ snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂ led_hw_name, default_desc);
>>> +ÂÂÂ } else if (is_of_node(fwnode)) {
>>> +ÂÂÂÂÂÂÂ strncpy(led_classdev_name, to_of_node(fwnode)->name,
>>> +ÂÂÂÂÂÂÂÂÂÂÂ LED_MAX_NAME_SIZE);
>>> +ÂÂÂ } else
>>> +ÂÂÂÂÂÂÂ return -EINVAL;
>>> +
>>> +ÂÂÂ return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(led_compose_name);

I was thinking a bit more about this.
Why do we want to export this function to have the drivers call it?
Can't we just set the required parameters in the led_class struct?

struct led_properties can be extended to set the needed strings in the LED driver.
The pointer to this struct can be set in the LED driver for the class

Then we can just call compose_name during class registration.

If we add the fwnode to the struct this may help in the multi-color registration as we could pick off
the parent node and look for the specific labels/handles.


>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>> index bffb4315fd66..c2936fc989d4 100644
>>> --- a/include/linux/leds.h
>>> +++ b/include/linux/leds.h
>>> @@ -252,6 +252,31 @@ extern void led_sysfs_disable(struct led_classdev *led_cdev);
>>> Â extern void led_sysfs_enable(struct led_classdev *led_cdev);
>>> Â Â /**
>>> + * led_compose_name - compose LED class device name
>>> + * @child: child fwnode_handle describing a LED,
>>> + *ÂÂÂÂÂÂ or a group of synchronized LEDs.
>>> + * @led_hw_name: name of the LED controller, used when falling back to legacy
>>> + *ÂÂÂÂÂÂÂÂ LED naming; it should be set to NULL in new LED class drivers
>>> + * @default_desc: default <color:function> tuple, for backwards compatibility
>>> + *ÂÂÂÂÂÂÂÂÂ with in-driver hard-coded LED names used as a fallback when
>>> + *ÂÂÂÂÂÂÂÂÂ "label" DT property is absent; it should be set to NULL
>>> + *ÂÂÂÂÂÂÂÂÂ in new LED class drivers
>>> + * @led_classdev_name: composed LED class device name
>>> + *
>>> + * Create LED class device name basing on the configuration provided by the
>>> + * board firmware. The name can have a legacy form <devicename:color:function>,
>>> + * or a new form <color:function>. The latter is chosen if "label" property is
>>> + * absent and at least one of "color" or "function" is present in the fwnode,
>>> + * leaving the section blank if the related property is absent. In case none
>>> + * of the aforementioned properties is found, then, for OF nodes, the node name
>>> + * is adopted for LED class device name.
>>> + *
>>> + * Returns: 0 on success or negative error value on failure
>>> + */
>>> +extern int led_compose_name(struct fwnode_handle *child, const char *led_hw_name,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ const char *default_desc, char *led_classdev_name);
>>> +
>>> +/**
>>> ÂÂ * led_sysfs_is_disabled - check if LED sysfs interface is disabled
>>> ÂÂ * @led_cdev: the LED to query
>>> ÂÂ *
>>> @@ -428,6 +453,12 @@ struct led_platform_data {
>>> ÂÂÂÂÂ struct led_infoÂÂÂ *leds;
>>> Â };
>>> Â +struct led_properties {
>>> +ÂÂÂ const char *color;
>>> +ÂÂÂ const char *function;
>>> +ÂÂÂ const char *label;
>>> +};
>>> +
>>> Â struct gpio_desc;
>>> Â typedef int (*gpio_blink_set_t)(struct gpio_desc *desc, int state,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long *delay_on,
>>> diff --git a/tools/leds/get_led_device_info.sh b/tools/leds/get_led_device_info.sh
>>> new file mode 100755
>>> index 000000000000..4671aa690e4a
>>> --- /dev/null
>>> +++ b/tools/leds/get_led_device_info.sh
>>> @@ -0,0 +1,81 @@
>>> +#!/bin/sh
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +
>>
>> Is there a way to give usage or help here? It's not entirely clear what the argument to pass in is.
> 
> It is in the first statement of this script - see below.
> It is customary to print help when unexpected arguments or a number
> thereof is given.
> 
> I can print help also when "--help" is passed.
> 

OK.  Maybe doing --help or --? would be to much.  Maybe a bit better help message I could not tell that
was an error message.

maybe

echo "usage: get_led_device_info.sh LED_CDEV_PATH"

>> maybe if $1 = "?" then print usage
>>
>> Dan
>>
>>
>>> +if [ $# -ne 1 ]; then
>>> +ÂÂÂ echo "get_led_devicename.sh LED_CDEV_PATH"
> 
> s/get_led_devicename/get_led_device_info/
> 
> It is a leftover from earlier stage of development.
> 
>>> +ÂÂÂ exit 1
>>> +fi
>>> +
>>> +led_cdev_path=`echo $1 | sed s'/\/$//'`
>>> +
>>> +ls "$led_cdev_path/brightness" > /dev/null 2>&1
>>> +if [ $? -ne 0 ]; then
>>> +ÂÂÂ echo "Device \"$led_cdev_path\" does not exist."
>>> +ÂÂÂ exit 1
>>> +fi
>>> +
>>> +bus=`readlink $led_cdev_path/device/subsystem | sed s'/.*\///'`
>>> +usb_subdev=`readlink $led_cdev_path | grep usb | sed s'/\(.*usb[0-9]*\/[0-9]*-[0-9]*\)\/.*/\1/'`
>>> +ls "$led_cdev_path/device/of_node/compatible" > /dev/null 2>&1
>>> +of_node_missing=$?
>>> +
>>> +if [ "$bus" = "input" ]; then
>>> +ÂÂÂ input_node=`readlink $led_cdev_path/device | sed s'/.*\///'`
>>> +ÂÂÂ if [ ! -z $usb_subdev ]; then
>>> +ÂÂÂÂÂÂÂ bus="usb"
>>> +ÂÂÂ fi
>>> +fi
>>> +
>>> +if [ "$bus" = "usb" ]; then
>>> +ÂÂÂ usb_interface=`readlink $led_cdev_path | sed s'/.*\(usb[0-9]*\)/\1/' | cut -d \/ -f 3`
>>> +ÂÂÂ driver=`readlink $usb_interface/driver | sed s'/.*\///'`
>>> +ÂÂÂ cd $led_cdev_path/../$usb_subdev
>>> +ÂÂÂ idVendor=`cat idVendor`
>>> +ÂÂÂ idProduct=`cat idProduct`
>>> +ÂÂÂ manufacturer=`cat manufacturer`
>>> +ÂÂÂ product=`cat product`
>>> +elif [ "$bus" = "input" ]; then
>>> +ÂÂÂ cd $led_cdev_path
>>> +ÂÂÂ product=`cat device/name`
>>> +ÂÂÂ driver=`cat device/device/driver/description`
>>> +elif [ $of_node_missing -eq 0 ]; then
>>> +ÂÂÂ cd $led_cdev_path
>>> +ÂÂÂ compatible=`cat device/of_node/compatible`
>>> +ÂÂÂ if [ "$compatible" = "gpio-leds" ]; then
>>> +ÂÂÂÂÂÂÂ driver="leds-gpio"
>>> +ÂÂÂ elif [ "$compatible" = "pwm-leds" ]; then
>>> +ÂÂÂÂÂÂÂ driver="leds-pwm"
>>> +ÂÂÂ else
>>> +ÂÂÂÂÂÂÂ manufacturer=`echo $compatible | cut -d, -f1`
>>> +ÂÂÂÂÂÂÂ product=`echo $compatible | cut -d, -f2`
>>> +ÂÂÂ fi
>>> +else
>>> +ÂÂÂ echo "Unknown device type."
>>> +ÂÂÂ exit 1
>>> +fi
>>> +
>>> +printf "bus:\t\t\t$bus\n"
>>> +
>>> +if [ ! -z "$idVendor" ]; then
>>> +ÂÂÂ printf "idVendor:\t\t$idVendor\n"
>>> +fi
>>> +
>>> +if [ ! -z "$idProduct" ]; then
>>> +ÂÂÂ printf "idProduct:\t\t$idProduct\n"
>>> +fi
>>> +
>>> +if [ ! -z "$manufacturer" ]; then
>>> +ÂÂÂ printf "manufacturer:\t\t$manufacturer\n"
>>> +fi
>>> +
>>> +if [ ! -z "$product" ]; then
>>> +ÂÂÂ printf "product:\t\t$product\n"
>>> +fi
>>> +
>>> +if [ ! -z "$driver" ]; then
>>> +ÂÂÂ printf "driver:\t\t\t$driver\n"
>>> +fi
>>> +
>>> +if [ ! -z "$input_node" ]; then
>>> +ÂÂÂ printf "associated input node:\t$input_node\n"
>>> +fi
>>>
>>
>>
> 


-- 
------------------
Dan Murphy