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

[Jacek Anaszewski]

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);
> > 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