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 18:28:10 +0100 |
| |
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); >> 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.
> 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
| |