Messages in this thread | | | Subject | Re: [PATCH 02/25] leds: core: Add support for composing LED class device names | From | Jacek Anaszewski <> | Date | Tue, 12 Mar 2019 19:19:33 +0100 |
| |
On 3/12/19 6:46 PM, Dan Murphy wrote: > 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@gmail.com> >>>> Cc: Baolin Wang <baolin.wang@linaro.org> >>>> Cc: Daniel Mack <daniel@zonque.org> >>>> Cc: Dan Murphy <dmurphy@ti.com> >>>> Cc: Linus Walleij <linus.walleij@linaro.org> >>>> Cc: Oleh Kravchenko <oleg@kaa.org.ua> >>>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> >>>> --- >>>> 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.
Adding to the struct led_classdev anything needed only in led_classdev_register() would be waste of memory, but we can add required properties to the new struct led_init_data and then call led_compose_name() inside led_classdev_register().
I will change it in the v3.
> 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.
struct led_init_data already has fwnode property in this set.
> >>>> 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"
Ack.
>>> 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 >>>> >>> >>> >> > >
-- Best regards, Jacek Anaszewski
| |