| Message ID | 20190310182836.20841-1-jacek.anaszewski@gmail.com |
|---|---|
| Headers | show |
| Series | Add generic support for composing LED class device name | expand |
Thanks for patches :) 10.03.19 20:28, Jacek Anaszewski пише: > Replace of_led_classdev_register() with led_classdev_register_ext(), which > accepts easily extendable struct led_init_data, instead of the fixed > struct device_node argument. The latter can be now passed in a fwnode > property of the struct led_init_data. > > The modification is driven by the need for passing another argument > to the LED class initialization function - a LED class device name. > Currently it is conveyed in the "name" char pointer property of > struct led_classdev, which is semantically and functionally incorrect > since it is needed only on LED class device registration, but persists > in system memory throughout the whole LED class device lifetime. > > Whereas in case of the name strings coming from DT "label" property or > statically initialized ones the cost is only the pointer size, it grows > to LED_MAX_NAME_SIZE (64) with the advent of a new LED class device naming > scheme, where color and function properties come from separate board > firmware properties and the name needs to be constructed in a new buffer. > > The change will not break any existing clients since the patch alters > also existing led_classdev{_flash}_register() macro wrappers, that pass > NULL in place of init_data, which leads to using legacy name > initialization path basing on the struct led_classdev's "name" property. > > Two existing users of devm_of_led_classdev_registers() are modified > to use devm_led_classdev_register(), which will not impact their > operation since they in fact didn't need to pass struct device_node on > registration from the beginning. > > Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Tested-by: Baolin Wang <baolin.wang@linaro.org> > Acked-by: Pavel Machek <pavel@ucw.cz> > Acked-by: Dan Murphy <dmurphy@ti.com> > Cc: Daniel Mack <daniel@zonque.org> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Oleh Kravchenko <oleg@kaa.org.ua> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Simon Shields <simon@lineageos.org> > --- > drivers/leds/led-class-flash.c | 9 +++++---- > drivers/leds/led-class.c | 34 ++++++++++++++++++++++------------ > drivers/leds/leds-cr0014114.c | 3 +-- > drivers/leds/leds-gpio.c | 2 +- > include/linux/led-class-flash.h | 15 ++++++++++----- > include/linux/leds.h | 37 +++++++++++++++++++++++++++---------- > 6 files changed, 66 insertions(+), 34 deletions(-) > > diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c > index cf398275a53c..8d1c1177bb9a 100644 > --- a/drivers/leds/led-class-flash.c > +++ b/drivers/leds/led-class-flash.c > @@ -285,8 +285,9 @@ static void led_flash_init_sysfs_groups(struct led_classdev_flash *fled_cdev) > led_cdev->groups = flash_groups; > } > > -int led_classdev_flash_register(struct device *parent, > - struct led_classdev_flash *fled_cdev) > +int led_classdev_flash_register_ext(struct device *parent, > + struct led_classdev_flash *fled_cdev, > + struct led_init_data *init_data) > { > struct led_classdev *led_cdev; > const struct led_flash_ops *ops; > @@ -312,13 +313,13 @@ int led_classdev_flash_register(struct device *parent, > } > > /* Register led class device */ > - ret = led_classdev_register(parent, led_cdev); > + ret = led_classdev_register_ext(parent, led_cdev, init_data); > if (ret < 0) > return ret; > > return 0; > } > -EXPORT_SYMBOL_GPL(led_classdev_flash_register); > +EXPORT_SYMBOL_GPL(led_classdev_flash_register_ext); > > void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev) > { > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 3c7e3487b373..a8337e0e02ea 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -17,6 +17,7 @@ > #include <linux/leds.h> > #include <linux/list.h> > #include <linux/module.h> > +#include <linux/of.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > #include <linux/timer.h> > @@ -244,18 +245,25 @@ static int led_classdev_next_name(const char *init_name, char *name, > } > > /** > - * of_led_classdev_register - register a new object of led_classdev class. > + * led_classdev_register_ext - register a new object of led_classdev class > + * with init data. > * > * @parent: parent of LED device > * @led_cdev: the led_classdev structure for this device. > - * @np: DT node describing this LED > + * @init_data: LED class device initialization data > */ > -int of_led_classdev_register(struct device *parent, struct device_node *np, > - struct led_classdev *led_cdev) > +int led_classdev_register_ext(struct device *parent, > + struct led_classdev *led_cdev, > + struct led_init_data *init_data) > { > char name[LED_MAX_NAME_SIZE]; > int ret; > > + if (init_data && init_data->name) > + led_cdev->name = init_data->name; > + else > + dev_info(parent, "init_data not available"); > + > ret = led_classdev_next_name(led_cdev->name, name, sizeof(name)); > if (ret < 0) > return ret; > @@ -268,7 +276,8 @@ int of_led_classdev_register(struct device *parent, struct device_node *np, > mutex_unlock(&led_cdev->led_access); > return PTR_ERR(led_cdev->dev); > } > - led_cdev->dev->of_node = np; > + if (init_data && init_data->fwnode) > + led_cdev->dev->of_node = to_of_node(init_data->fwnode); > > if (ret) > dev_warn(parent, "Led %s renamed to %s due to name collision", > @@ -313,7 +322,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np, > > return 0; > } > -EXPORT_SYMBOL_GPL(of_led_classdev_register); > +EXPORT_SYMBOL_GPL(led_classdev_register_ext); > > /** > * led_classdev_unregister - unregisters a object of led_properties class. > @@ -358,14 +367,15 @@ static void devm_led_classdev_release(struct device *dev, void *res) > } > > /** > - * devm_of_led_classdev_register - resource managed led_classdev_register() > + * devm_led_classdev_register_ext - resource managed led_classdev_register_ext() > * > * @parent: parent of LED device > * @led_cdev: the led_classdev structure for this device. > + * @init_data: LED class device initialization data > */ > -int devm_of_led_classdev_register(struct device *parent, > - struct device_node *np, > - struct led_classdev *led_cdev) > +int devm_led_classdev_register_ext(struct device *parent, > + struct led_classdev *led_cdev, > + struct led_init_data *init_data) > { > struct led_classdev **dr; > int rc; > @@ -374,7 +384,7 @@ int devm_of_led_classdev_register(struct device *parent, > if (!dr) > return -ENOMEM; > > - rc = of_led_classdev_register(parent, np, led_cdev); > + rc = led_classdev_register_ext(parent, led_cdev, init_data); > if (rc) { > devres_free(dr); > return rc; > @@ -385,7 +395,7 @@ int devm_of_led_classdev_register(struct device *parent, > > return 0; > } > -EXPORT_SYMBOL_GPL(devm_of_led_classdev_register); > +EXPORT_SYMBOL_GPL(devm_led_classdev_register_ext); > > static int devm_led_classdev_match(struct device *dev, void *res, void *data) > { > diff --git a/drivers/leds/leds-cr0014114.c b/drivers/leds/leds-cr0014114.c > index 0e4262462cb9..1c82152764d2 100644 > --- a/drivers/leds/leds-cr0014114.c > +++ b/drivers/leds/leds-cr0014114.c > @@ -207,8 +207,7 @@ static int cr0014114_probe_dt(struct cr0014114 *priv) > led->ldev.max_brightness = CR_MAX_BRIGHTNESS; > led->ldev.brightness_set_blocking = cr0014114_set_sync; > > - ret = devm_of_led_classdev_register(priv->dev, np, > - &led->ldev); > + ret = devm_led_classdev_register(priv->dev, &led->ldev); > if (ret) { > dev_err(priv->dev, > "failed to register LED device %s, err %d", > diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c > index 998f2ff6914d..b26cf78993d1 100644 > --- a/drivers/leds/leds-gpio.c > +++ b/drivers/leds/leds-gpio.c > @@ -112,7 +112,7 @@ static int create_gpio_led(const struct gpio_led *template, > if (ret < 0) > return ret; > > - return devm_of_led_classdev_register(parent, np, &led_dat->cdev); > + return devm_led_classdev_register(parent, &led_dat->cdev); > } > > struct gpio_leds_priv { > diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h > index 700efaa9e115..ded8d9fa6149 100644 > --- a/include/linux/led-class-flash.h > +++ b/include/linux/led-class-flash.h > @@ -90,15 +90,20 @@ static inline struct led_classdev_flash *lcdev_to_flcdev( > } > > /** > - * led_classdev_flash_register - register a new object of led_classdev class > - * with support for flash LEDs > - * @parent: the flash LED to register > + * led_classdev_flash_register_ext - register a new object of LED class with > + * init data and with support for flash LEDs > + * @parent: LED flash controller device this flash LED is driven by > * @fled_cdev: the led_classdev_flash structure for this device > + * @init_data: the LED class flash device initialization data > * > * Returns: 0 on success or negative error value on failure > */ > -extern int led_classdev_flash_register(struct device *parent, > - struct led_classdev_flash *fled_cdev); > +extern int led_classdev_flash_register_ext(struct device *parent, > + struct led_classdev_flash *fled_cdev, > + struct led_init_data *init_data); > + > +#define led_classdev_flash_register(parent, fled_cdev) \ > + led_classdev_flash_register_ext(parent, fled_cdev, NULL) > > /** > * led_classdev_flash_unregister - unregisters an object of led_classdev class > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 5263f87e1d2c..bffb4315fd66 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -20,6 +20,7 @@ > #include <linux/spinlock.h> > #include <linux/timer.h> > #include <linux/workqueue.h> > +#include <uapi/linux/uleds.h> > > struct device; > struct led_pattern; > @@ -34,6 +35,13 @@ enum led_brightness { > LED_FULL = 255, > }; > > +struct led_init_data { > + /* Device fwnode handle */ > + struct fwnode_handle *fwnode; > + /* Requested LED class device name */ > + char name[LED_MAX_NAME_SIZE]; > +}; > + > struct led_classdev { > const char *name; > enum led_brightness brightness; > @@ -129,16 +137,25 @@ struct led_classdev { > struct mutex led_access; > }; > > -extern int of_led_classdev_register(struct device *parent, > - struct device_node *np, > - struct led_classdev *led_cdev); > -#define led_classdev_register(parent, led_cdev) \ > - of_led_classdev_register(parent, NULL, led_cdev) > -extern int devm_of_led_classdev_register(struct device *parent, > - struct device_node *np, > - struct led_classdev *led_cdev); > -#define devm_led_classdev_register(parent, led_cdev) \ > - devm_of_led_classdev_register(parent, NULL, led_cdev) > +/** > + * led_classdev_register_ext - register a new object of LED class with > + * init data > + * @parent: LED controller device this LED is driven by > + * @led_cdev: the led_classdev structure for this device > + * @init_data: the LED class device initialization data > + * > + * Returns: 0 on success or negative error value on failure > + */ > +extern int led_classdev_register_ext(struct device *parent, > + struct led_classdev *led_cdev, > + struct led_init_data *init_data); > +#define led_classdev_register(parent, led_cdev) \ > + led_classdev_register_ext(parent, led_cdev, NULL) > +extern int devm_led_classdev_register_ext(struct device *parent, > + struct led_classdev *led_cdev, > + struct led_init_data *init_data); > +#define devm_led_classdev_register(parent, led_cdev) \ > + devm_led_classdev_register_ext(parent, led_cdev, NULL) > extern void led_classdev_unregister(struct led_classdev *led_cdev); > extern void devm_led_classdev_unregister(struct device *parent, > struct led_classdev *led_cdev); >
On 3/10/19 1:28 PM, Jacek Anaszewski wrote: > Switch to using generic LED support for composing LED class > device name. > > While at it, avoid iterating through available child of nodes > in favor of obtaining single expected child node using single > call to of_get_next_available_child(). > > Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Cc: Dan Murphy <dmurphy@ti.com> > --- > drivers/leds/leds-lp8860.c | 38 +++++++++++++++++++------------------- > 1 file changed, 19 insertions(+), 19 deletions(-) > > diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c > index 39c72a908f3b..7c12ccdc1f2f 100644 > --- a/drivers/leds/leds-lp8860.c > +++ b/drivers/leds/leds-lp8860.c > @@ -22,7 +22,6 @@ > #include <linux/of_gpio.h> > #include <linux/gpio/consumer.h> > #include <linux/slab.h> > -#include <uapi/linux/uleds.h> > > #define LP8860_DISP_CL1_BRT_MSB 0x00 > #define LP8860_DISP_CL1_BRT_LSB 0x01 > @@ -87,6 +86,8 @@ > > #define LP8860_CLEAR_FAULTS 0x01 > > +#define LP8860_NAME "lp8860" > + > /** > * struct lp8860_led - > * @lock - Lock for reading/writing the device > @@ -96,7 +97,6 @@ > * @eeprom_regmap - EEPROM register map > * @enable_gpio - VDDIO/EN gpio to enable communication interface > * @regulator - LED supply regulator pointer > - * @label - LED label > */ > struct lp8860_led { > struct mutex lock; > @@ -106,7 +106,6 @@ struct lp8860_led { > struct regmap *eeprom_regmap; > struct gpio_desc *enable_gpio; > struct regulator *regulator; > - char label[LED_MAX_NAME_SIZE]; > }; > > struct lp8860_eeprom_reg { > @@ -387,25 +386,26 @@ static int lp8860_probe(struct i2c_client *client, > struct lp8860_led *led; > struct device_node *np = client->dev.of_node; > struct device_node *child_node; > - const char *name; > + struct led_init_data init_data; > > led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL); > if (!led) > return -ENOMEM; > > - for_each_available_child_of_node(np, child_node) { > - led->led_dev.default_trigger = of_get_property(child_node, > - "linux,default-trigger", > - NULL); > - > - ret = of_property_read_string(child_node, "label", &name); > - if (!ret) > - snprintf(led->label, sizeof(led->label), "%s:%s", > - id->name, name); > - else > - snprintf(led->label, sizeof(led->label), > - "%s::display_cluster", id->name); > - } > + child_node = of_get_next_available_child(np, NULL); Again this device has multiple outputs that can be banked or separated. I would prefer to leave the iteration and just change the name generation. Dan > + if (!child_node) > + return -EINVAL; > + > + init_data.fwnode = of_fwnode_handle(child_node), > + > + led->led_dev.default_trigger = of_get_property(child_node, > + "linux,default-trigger", > + NULL); > + > + ret = led_compose_name(init_data.fwnode, LP8860_NAME, > + ":display_cluster", init_data.name); > + if (ret) > + return ret; > > led->enable_gpio = devm_gpiod_get_optional(&client->dev, > "enable", GPIOD_OUT_LOW); > @@ -420,7 +420,6 @@ static int lp8860_probe(struct i2c_client *client, > led->regulator = NULL; > > led->client = client; > - led->led_dev.name = led->label; > led->led_dev.brightness_set_blocking = lp8860_brightness_set; > > mutex_init(&led->lock); > @@ -447,7 +446,8 @@ static int lp8860_probe(struct i2c_client *client, > if (ret) > return ret; > > - ret = devm_led_classdev_register(&client->dev, &led->led_dev); > + ret = devm_led_classdev_register_ext(&client->dev, &led->led_dev, > + &init_data); > if (ret) { > dev_err(&client->dev, "led register err: %d\n", ret); > return ret; >
On 3/10/19 1:28 PM, Jacek Anaszewski wrote: > Switch to using generic LED support for composing LED class > device name. > > Since the same device strings would be used in two places, > then add macros LM36922_NAME and LM36922_NAME for use in > lm3692x_probe_dt(() and lm3692x_id array. > > Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > Acked-by: Pavel Machek <pavel@ucw.cz> > Cc: Dan Murphy <dmurphy@ti.com> > --- > drivers/leds/leds-lm3692x.c | 39 ++++++++++++++++++++------------------- > 1 file changed, 20 insertions(+), 19 deletions(-) > > diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c > index 4f413a7c5f05..9dfc0f28a9c8 100644 > --- a/drivers/leds/leds-lm3692x.c > +++ b/drivers/leds/leds-lm3692x.c > @@ -13,7 +13,6 @@ > #include <linux/regmap.h> > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > -#include <uapi/linux/uleds.h> > > #define LM36922_MODEL 0 > #define LM36923_MODEL 1 > @@ -95,6 +94,9 @@ > #define LM3692X_FAULT_FLAG_SHRT BIT(3) > #define LM3692X_FAULT_FLAG_OPEN BIT(4) > > +#define LM36922_NAME "lm36922" > +#define LM36923_NAME "lm36923" > + > /** > * struct lm3692x_led - > * @lock - Lock for reading/writing the device > @@ -103,7 +105,6 @@ > * @regmap - Devices register map > * @enable_gpio - VDDIO/EN gpio to enable communication interface > * @regulator - LED supply regulator pointer > - * @label - LED label > * @led_enable - LED sync to be enabled > * @model_id - Current device model ID enumerated > */ > @@ -114,7 +115,6 @@ struct lm3692x_led { > struct regmap *regmap; > struct gpio_desc *enable_gpio; > struct regulator *regulator; > - char label[LED_MAX_NAME_SIZE]; > int led_enable; > int model_id; > }; > @@ -325,7 +325,8 @@ static int lm3692x_init(struct lm3692x_led *led) > static int lm3692x_probe_dt(struct lm3692x_led *led) > { > struct fwnode_handle *child = NULL; > - const char *name; > + struct led_init_data init_data; > + char *model_name; > int ret; > > led->enable_gpio = devm_gpiod_get_optional(&led->client->dev, > @@ -346,17 +347,20 @@ static int lm3692x_probe_dt(struct lm3692x_led *led) > dev_err(&led->client->dev, "No LED Child node\n"); > return -ENODEV; > } > + init_data.fwnode = child; > > - fwnode_property_read_string(child, "linux,default-trigger", > - &led->led_dev.default_trigger); > + if (led->model_id == LM36922_MODEL) > + model_name = LM36922_NAME; > + else > + model_name = LM36923_NAME; > Why do we need this change? led->client->name should suffice here and then we do not have to #define the name at all. > - ret = fwnode_property_read_string(child, "label", &name); > + ret = led_compose_name(child, model_name, ":backlight_cluster", > + init_data.name); > if (ret) > - snprintf(led->label, sizeof(led->label), > - "%s::", led->client->name); > - else > - snprintf(led->label, sizeof(led->label), > - "%s:%s", led->client->name, name); > + return ret; > + > + fwnode_property_read_string(child, "linux,default-trigger", > + &led->led_dev.default_trigger); > > ret = fwnode_property_read_u32(child, "reg", &led->led_enable); > if (ret) { > @@ -364,16 +368,13 @@ static int lm3692x_probe_dt(struct lm3692x_led *led) > return ret; > } > > - led->led_dev.name = led->label; > - > - ret = devm_led_classdev_register(&led->client->dev, &led->led_dev); > + ret = devm_led_classdev_register_ext(&led->client->dev, &led->led_dev, > + &init_data); > if (ret) { > dev_err(&led->client->dev, "led register err: %d\n", ret); > return ret; > } > > - led->led_dev.dev->of_node = to_of_node(child); > - Seems like an additional change here not documented. If it is a bug fix or an enhancement probably need a separate patch. I do know why this is removed just need tracability on the log. Dan > return 0; > } > > @@ -439,8 +440,8 @@ static int lm3692x_remove(struct i2c_client *client) > } > > static const struct i2c_device_id lm3692x_id[] = { > - { "lm36922", LM36922_MODEL }, > - { "lm36923", LM36923_MODEL }, > + { LM36922_NAME, LM36922_MODEL }, > + { LM36923_NAME, LM36923_MODEL }, > { } > }; > MODULE_DEVICE_TABLE(i2c, lm3692x_id); >
Dan, On 3/11/19 1:28 PM, Dan Murphy wrote: > On 3/10/19 1:28 PM, Jacek Anaszewski wrote: >> Switch to using generic LED support for composing LED class >> device name. >> >> While at it, avoid iterating through available child of nodes >> in favor of obtaining single expected child node using single >> call to of_get_next_available_child(). >> >> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> Cc: Dan Murphy <dmurphy@ti.com> >> --- >> drivers/leds/leds-lp8860.c | 38 +++++++++++++++++++------------------- >> 1 file changed, 19 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c >> index 39c72a908f3b..7c12ccdc1f2f 100644 >> --- a/drivers/leds/leds-lp8860.c >> +++ b/drivers/leds/leds-lp8860.c >> @@ -22,7 +22,6 @@ >> #include <linux/of_gpio.h> >> #include <linux/gpio/consumer.h> >> #include <linux/slab.h> >> -#include <uapi/linux/uleds.h> >> >> #define LP8860_DISP_CL1_BRT_MSB 0x00 >> #define LP8860_DISP_CL1_BRT_LSB 0x01 >> @@ -87,6 +86,8 @@ >> >> #define LP8860_CLEAR_FAULTS 0x01 >> >> +#define LP8860_NAME "lp8860" >> + >> /** >> * struct lp8860_led - >> * @lock - Lock for reading/writing the device >> @@ -96,7 +97,6 @@ >> * @eeprom_regmap - EEPROM register map >> * @enable_gpio - VDDIO/EN gpio to enable communication interface >> * @regulator - LED supply regulator pointer >> - * @label - LED label >> */ >> struct lp8860_led { >> struct mutex lock; >> @@ -106,7 +106,6 @@ struct lp8860_led { >> struct regmap *eeprom_regmap; >> struct gpio_desc *enable_gpio; >> struct regulator *regulator; >> - char label[LED_MAX_NAME_SIZE]; >> }; >> >> struct lp8860_eeprom_reg { >> @@ -387,25 +386,26 @@ static int lp8860_probe(struct i2c_client *client, >> struct lp8860_led *led; >> struct device_node *np = client->dev.of_node; >> struct device_node *child_node; >> - const char *name; >> + struct led_init_data init_data; >> >> led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL); >> if (!led) >> return -ENOMEM; >> >> - for_each_available_child_of_node(np, child_node) { >> - led->led_dev.default_trigger = of_get_property(child_node, >> - "linux,default-trigger", >> - NULL); >> - >> - ret = of_property_read_string(child_node, "label", &name); >> - if (!ret) >> - snprintf(led->label, sizeof(led->label), "%s:%s", >> - id->name, name); >> - else >> - snprintf(led->label, sizeof(led->label), >> - "%s::display_cluster", id->name); >> - } >> + child_node = of_get_next_available_child(np, NULL); > > Again this device has multiple outputs that can be banked or separated. > > I would prefer to leave the iteration and just change the name generation. In [0] you agreed on removing that. This code is simply broken since there is only one instance of struct lp8860_led allocated. >> + if (!child_node) >> + return -EINVAL; >> + >> + init_data.fwnode = of_fwnode_handle(child_node), >> + >> + led->led_dev.default_trigger = of_get_property(child_node, >> + "linux,default-trigger", >> + NULL); >> + >> + ret = led_compose_name(init_data.fwnode, LP8860_NAME, >> + ":display_cluster", init_data.name); >> + if (ret) >> + return ret; >> >> led->enable_gpio = devm_gpiod_get_optional(&client->dev, >> "enable", GPIOD_OUT_LOW); >> @@ -420,7 +420,6 @@ static int lp8860_probe(struct i2c_client *client, >> led->regulator = NULL; >> >> led->client = client; >> - led->led_dev.name = led->label; >> led->led_dev.brightness_set_blocking = lp8860_brightness_set; >> >> mutex_init(&led->lock); >> @@ -447,7 +446,8 @@ static int lp8860_probe(struct i2c_client *client, >> if (ret) >> return ret; >> >> - ret = devm_led_classdev_register(&client->dev, &led->led_dev); >> + ret = devm_led_classdev_register_ext(&client->dev, &led->led_dev, >> + &init_data); >> if (ret) { >> dev_err(&client->dev, "led register err: %d\n", ret); >> return ret; >> > > [0] https://lkml.org/lkml/2018/11/12/2231
Dan, On 3/11/19 1:38 PM, Dan Murphy wrote: > On 3/10/19 1:28 PM, Jacek Anaszewski wrote: >> Switch to using generic LED support for composing LED class >> device name. >> >> Since the same device strings would be used in two places, >> then add macros LM36922_NAME and LM36922_NAME for use in >> lm3692x_probe_dt(() and lm3692x_id array. >> >> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> Acked-by: Pavel Machek <pavel@ucw.cz> >> Cc: Dan Murphy <dmurphy@ti.com> >> --- >> drivers/leds/leds-lm3692x.c | 39 ++++++++++++++++++++------------------- >> 1 file changed, 20 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c >> index 4f413a7c5f05..9dfc0f28a9c8 100644 >> --- a/drivers/leds/leds-lm3692x.c >> +++ b/drivers/leds/leds-lm3692x.c >> @@ -13,7 +13,6 @@ >> #include <linux/regmap.h> >> #include <linux/regulator/consumer.h> >> #include <linux/slab.h> >> -#include <uapi/linux/uleds.h> >> >> #define LM36922_MODEL 0 >> #define LM36923_MODEL 1 >> @@ -95,6 +94,9 @@ >> #define LM3692X_FAULT_FLAG_SHRT BIT(3) >> #define LM3692X_FAULT_FLAG_OPEN BIT(4) >> >> +#define LM36922_NAME "lm36922" >> +#define LM36923_NAME "lm36923" >> + >> /** >> * struct lm3692x_led - >> * @lock - Lock for reading/writing the device >> @@ -103,7 +105,6 @@ >> * @regmap - Devices register map >> * @enable_gpio - VDDIO/EN gpio to enable communication interface >> * @regulator - LED supply regulator pointer >> - * @label - LED label >> * @led_enable - LED sync to be enabled >> * @model_id - Current device model ID enumerated >> */ >> @@ -114,7 +115,6 @@ struct lm3692x_led { >> struct regmap *regmap; >> struct gpio_desc *enable_gpio; >> struct regulator *regulator; >> - char label[LED_MAX_NAME_SIZE]; >> int led_enable; >> int model_id; >> }; >> @@ -325,7 +325,8 @@ static int lm3692x_init(struct lm3692x_led *led) >> static int lm3692x_probe_dt(struct lm3692x_led *led) >> { >> struct fwnode_handle *child = NULL; >> - const char *name; >> + struct led_init_data init_data; >> + char *model_name; >> int ret; >> >> led->enable_gpio = devm_gpiod_get_optional(&led->client->dev, >> @@ -346,17 +347,20 @@ static int lm3692x_probe_dt(struct lm3692x_led *led) >> dev_err(&led->client->dev, "No LED Child node\n"); >> return -ENODEV; >> } >> + init_data.fwnode = child; >> >> - fwnode_property_read_string(child, "linux,default-trigger", >> - &led->led_dev.default_trigger); >> + if (led->model_id == LM36922_MODEL) >> + model_name = LM36922_NAME; >> + else >> + model_name = LM36923_NAME; >> > > > Why do we need this change? > > led->client->name should suffice here and then we do not have to #define the name at all. Indeed, at that time I was not aware that I2C subsystem creates client name using product name from DT compatible property. > >> - ret = fwnode_property_read_string(child, "label", &name); >> + ret = led_compose_name(child, model_name, ":backlight_cluster", >> + init_data.name); >> if (ret) >> - snprintf(led->label, sizeof(led->label), >> - "%s::", led->client->name); >> - else >> - snprintf(led->label, sizeof(led->label), >> - "%s:%s", led->client->name, name); >> + return ret; >> + >> + fwnode_property_read_string(child, "linux,default-trigger", >> + &led->led_dev.default_trigger); >> >> ret = fwnode_property_read_u32(child, "reg", &led->led_enable); >> if (ret) { >> @@ -364,16 +368,13 @@ static int lm3692x_probe_dt(struct lm3692x_led *led) >> return ret; >> } >> >> - led->led_dev.name = led->label; >> - >> - ret = devm_led_classdev_register(&led->client->dev, &led->led_dev); >> + ret = devm_led_classdev_register_ext(&led->client->dev, &led->led_dev, >> + &init_data); >> if (ret) { >> dev_err(&led->client->dev, "led register err: %d\n", ret); >> return ret; >> } >> >> - led->led_dev.dev->of_node = to_of_node(child); >> - > > Seems like an additional change here not documented. > If it is a bug fix or an enhancement probably need a separate patch. > > I do know why this is removed just need tracability on the log. OK, I will add the comment. >> return 0; >> } >> >> @@ -439,8 +440,8 @@ static int lm3692x_remove(struct i2c_client *client) >> } >> >> static const struct i2c_device_id lm3692x_id[] = { >> - { "lm36922", LM36922_MODEL }, >> - { "lm36923", LM36923_MODEL }, >> + { LM36922_NAME, LM36922_MODEL }, >> + { LM36923_NAME, LM36923_MODEL }, >> { } >> }; >> MODULE_DEVICE_TABLE(i2c, lm3692x_id); >> > >
Jacek On 3/11/19 12:24 PM, Jacek Anaszewski wrote: > Dan, > > On 3/11/19 1:28 PM, Dan Murphy wrote: >> On 3/10/19 1:28 PM, Jacek Anaszewski wrote: >>> Switch to using generic LED support for composing LED class >>> device name. >>> >>> While at it, avoid iterating through available child of nodes >>> in favor of obtaining single expected child node using single >>> call to of_get_next_available_child(). >>> >>> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> >>> Cc: Dan Murphy <dmurphy@ti.com> >>> --- >>> drivers/leds/leds-lp8860.c | 38 +++++++++++++++++++------------------- >>> 1 file changed, 19 insertions(+), 19 deletions(-) >>> >>> diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c >>> index 39c72a908f3b..7c12ccdc1f2f 100644 >>> --- a/drivers/leds/leds-lp8860.c >>> +++ b/drivers/leds/leds-lp8860.c >>> @@ -22,7 +22,6 @@ >>> #include <linux/of_gpio.h> >>> #include <linux/gpio/consumer.h> >>> #include <linux/slab.h> >>> -#include <uapi/linux/uleds.h> >>> #define LP8860_DISP_CL1_BRT_MSB 0x00 >>> #define LP8860_DISP_CL1_BRT_LSB 0x01 >>> @@ -87,6 +86,8 @@ >>> #define LP8860_CLEAR_FAULTS 0x01 >>> +#define LP8860_NAME "lp8860" >>> + >>> /** >>> * struct lp8860_led - >>> * @lock - Lock for reading/writing the device >>> @@ -96,7 +97,6 @@ >>> * @eeprom_regmap - EEPROM register map >>> * @enable_gpio - VDDIO/EN gpio to enable communication interface >>> * @regulator - LED supply regulator pointer >>> - * @label - LED label >>> */ >>> struct lp8860_led { >>> struct mutex lock; >>> @@ -106,7 +106,6 @@ struct lp8860_led { >>> struct regmap *eeprom_regmap; >>> struct gpio_desc *enable_gpio; >>> struct regulator *regulator; >>> - char label[LED_MAX_NAME_SIZE]; >>> }; >>> struct lp8860_eeprom_reg { >>> @@ -387,25 +386,26 @@ static int lp8860_probe(struct i2c_client *client, >>> struct lp8860_led *led; >>> struct device_node *np = client->dev.of_node; >>> struct device_node *child_node; >>> - const char *name; >>> + struct led_init_data init_data; >>> led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL); >>> if (!led) >>> return -ENOMEM; >>> - for_each_available_child_of_node(np, child_node) { >>> - led->led_dev.default_trigger = of_get_property(child_node, >>> - "linux,default-trigger", >>> - NULL); >>> - >>> - ret = of_property_read_string(child_node, "label", &name); >>> - if (!ret) >>> - snprintf(led->label, sizeof(led->label), "%s:%s", >>> - id->name, name); >>> - else >>> - snprintf(led->label, sizeof(led->label), >>> - "%s::display_cluster", id->name); >>> - } >>> + child_node = of_get_next_available_child(np, NULL); >> >> Again this device has multiple outputs that can be banked or separated. >> >> I would prefer to leave the iteration and just change the name generation. > > In [0] you agreed on removing that. This code is simply broken since > there is only one instance of struct lp8860_led allocated. > Thanks for the reminder on my ack Dan >>> + if (!child_node) >>> + return -EINVAL; >>> + >>> + init_data.fwnode = of_fwnode_handle(child_node), >>> + >>> + led->led_dev.default_trigger = of_get_property(child_node, >>> + "linux,default-trigger", >>> + NULL); >>> + >>> + ret = led_compose_name(init_data.fwnode, LP8860_NAME, >>> + ":display_cluster", init_data.name); >>> + if (ret) >>> + return ret; >>> led->enable_gpio = devm_gpiod_get_optional(&client->dev, >>> "enable", GPIOD_OUT_LOW); >>> @@ -420,7 +420,6 @@ static int lp8860_probe(struct i2c_client *client, >>> led->regulator = NULL; >>> led->client = client; >>> - led->led_dev.name = led->label; >>> led->led_dev.brightness_set_blocking = lp8860_brightness_set; >>> mutex_init(&led->lock); >>> @@ -447,7 +446,8 @@ static int lp8860_probe(struct i2c_client *client, >>> if (ret) >>> return ret; >>> - ret = devm_led_classdev_register(&client->dev, &led->led_dev); >>> + ret = devm_led_classdev_register_ext(&client->dev, &led->led_dev, >>> + &init_data); >>> if (ret) { >>> dev_err(&client->dev, "led register err: %d\n", ret); >>> return ret; >>> >> >> > > [0] https://lkml.org/lkml/2018/11/12/2231 >
Two auto corrections: On 3/10/19 7: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 s/triggr/trigger/ > +network drivers that create triggers dynamically and incorporate phy > +name into its name. On the other hand input subsystem offers LED - input s/its name/the trigger name/ > +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.
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. maybe if $1 = "?" then print usage Dan > +if [ $# -ne 1 ]; then > + echo "get_led_devicename.sh LED_CDEV_PATH" > + 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 >
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 >> > >
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. 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 >>> >> >> >
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 >>>> >>> >>> >> > >
On Sun, Mar 10, 2019 at 07:28:11PM +0100, Jacek Anaszewski wrote: > Changes from v1: > > - improved led_parse_properties() to parse label property at first > and return immediately after parsing succeeds > - added tool get_led_device_info.sh for retrieving LED class device's > parent device related information > - extended LED naming section of Documentation/leds/leds-class.txt > - adjusted the list of LED_FUNCTION definitions according to the v1 review > remarks > - added standard LED_COLOR_NAME definitions > - removed functions.h and moved both LED_FUNCTION and LED_COLOR_NAME > definitions to include/dt-bindings/common.h > - rebased leds-as3645a changes on top of the patch switching to fwnode > property API > - updated DT bindings to use new LED_COLOR_NAME definitions > - improved common LED bindings to not use address unit for sub-nodes > without reg property > > Generally I still insist on deprecating label property and devicename > section of LED name. The tool I added for obtaining LED device name > proves availability of the related information in other places in > the sysfs. Other discussed use cases are covered in the updated > Documentation/leds/leds-class.txt. > > Beside that, I tried also to create macros for automatic composition > of "-N" suffixed LED functions, so that it would not be necessary > to pollute common.h with plenty repetitions of the same function, > differing only with the postfix. Unfortunately, the preprocessor > of the dtc compiler seems not to accept string concatenetation. > I.e. it is not possible to to the following assighment: > > function = "hdd""-1" > > If anyone knows how to obviate this shortocoming please let me know. Raise the question on devicetree-compiler list. I've done my share of dtc patches, but the parsing side is generally not an area I touch. Rob
On 3/28/19 1:19 AM, Rob Herring wrote: > On Sun, Mar 10, 2019 at 07:28:11PM +0100, Jacek Anaszewski wrote: >> Changes from v1: >> >> - improved led_parse_properties() to parse label property at first >> and return immediately after parsing succeeds >> - added tool get_led_device_info.sh for retrieving LED class device's >> parent device related information >> - extended LED naming section of Documentation/leds/leds-class.txt >> - adjusted the list of LED_FUNCTION definitions according to the v1 review >> remarks >> - added standard LED_COLOR_NAME definitions >> - removed functions.h and moved both LED_FUNCTION and LED_COLOR_NAME >> definitions to include/dt-bindings/common.h >> - rebased leds-as3645a changes on top of the patch switching to fwnode >> property API >> - updated DT bindings to use new LED_COLOR_NAME definitions >> - improved common LED bindings to not use address unit for sub-nodes >> without reg property >> >> Generally I still insist on deprecating label property and devicename >> section of LED name. The tool I added for obtaining LED device name >> proves availability of the related information in other places in >> the sysfs. Other discussed use cases are covered in the updated >> Documentation/leds/leds-class.txt. >> >> Beside that, I tried also to create macros for automatic composition >> of "-N" suffixed LED functions, so that it would not be necessary >> to pollute common.h with plenty repetitions of the same function, >> differing only with the postfix. Unfortunately, the preprocessor >> of the dtc compiler seems not to accept string concatenetation. >> I.e. it is not possible to to the following assighment: >> >> function = "hdd""-1" >> >> If anyone knows how to obviate this shortocoming please let me know. > > Raise the question on devicetree-compiler list. I've done my share of > dtc patches, but the parsing side is generally not an area I touch. I'll probably come up with additional property function-enumerator.