leds: pwm: Allow automatic labels for DT based devices
PATCH 1/1 leds: pwm: Allow automatic labels for DT based devices
From: Alexander Dahl <[email protected]>
If LEDs are configured through device tree and the property 'label' is omitted, the label is supposed to be generated from the properties 'function' and 'color' if present. While this works fine for e.g. the 'leds-gpio' driver, it did not for 'leds-pwm'.
The reason is, you get this label naming magic only if you add a LED device through 'devm_led_classdev_register_ext()' and pass a pointer to the current device tree node. The approach to fix this was adopted from the 'leds-gpio' driver.
For the following node from dts the LED appeared as 'led5' in sysfs before and as 'red:debug' after this change.
pwm_leds {
compatible = "pwm-leds";
led5 {
function = LED_FUNCTION_DEBUG;
color = <LED_COLOR_ID_RED>;
pwms = <&pwm0 2 10000000 0>;
max-brightness = <127>;
linux,default-trigger = "heartbeat";
panic-indicator;
};
};---
Notes:
v1: based on v5.9-rc2, backport on v5.4.59 also works
drivers/leds/leds-pwm.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/leds/leds-pwm.c +8 -1
@@ -65,6 +65,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv, struct led_pwm *led, struct fwnode_handle *fwnode) { struct led_pwm_data *led_data = &priv->leds[priv->num_leds]; +struct led_init_data init_data = {};int ret; led_data->active_low = led->active_low;
@@ -90,7 +91,13 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,pwm_init_state (led_data->pwm, &led_data->pwmstate); - ret =devm_led_classdev_register (dev, &led_data->cdev); +if (fwnode) { + init_data.fwnode = fwnode; + ret =devm_led_classdev_register_ext (dev, &led_data->cdev, + &init_data); + }else { + ret =devm_led_classdev_register (dev, &led_data->cdev); + }if (ret) {dev_err (dev,"failed to register PWM led for %s: %d\n" , led->name, ret);
Signature · 1 line
2.27.0
Hi Alexander,
Quoted 56 lines · Alexander Dahl
From: Alexander Dahl <[email protected]>
If LEDs are configured through device tree and the property 'label' is omitted, the label is supposed to be generated from the properties 'function' and 'color' if present. While this works fine for e.g. the 'leds-gpio' driver, it did not for 'leds-pwm'.
The reason is, you get this label naming magic only if you add a LED device through '
devm_led_classdev_register_ext()' and pass a pointer to the current device tree node. The approach to fix this was adopted from the 'leds-gpio' driver.For the following node from dts the LED appeared as 'led5' in sysfs before and as 'red:debug' after this change.
pwm_leds { compatible = "pwm-leds"; led5 { function = LED_FUNCTION_DEBUG; color = <LED_COLOR_ID_RED>; pwms = <&pwm0 2 10000000 0>; max-brightness = <127>; linux,default-trigger = "heartbeat"; panic-indicator; }; };Signed-off-by Alexander Dahl <[email protected]>---
Notes:
v1: based on v5.9-rc2, backport on v5.4.59 also worksdrivers/leds/leds-pwm.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)drivers/leds/leds-pwm.c +1
-1@@ -65,6 +65,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv, struct led_pwm *led, struct fwnode_handle *fwnode) { struct led_pwm_data *led_data = &priv->leds[priv->num_leds]; +struct led_init_data init_data = {};int ret; led_data->active_low = led->active_low;@@ -90,7 +91,13 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,pwm_init_state (led_data->pwm, &led_data->pwmstate); - ret =devm_led_classdev_register (dev, &led_data->cdev);
drivers/leds/leds-pwm.c static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv, · line 103
Quoted 10 lines
drivers/leds/leds-pwm.c +7
-0+if (fwnode) { + init_data.fwnode = fwnode; + ret =devm_led_classdev_register_ext (dev, &led_data->cdev, + &init_data); + }else { + ret =devm_led_classdev_register (dev, &led_data->cdev); + }if (ret) {dev_err (dev,"failed to register PWM led for %s: %d\n" , led->name, ret);
This part looks good, but corresponding update of Documentation/devicetree/bindings/leds/leds-pwm.txt is needed as well. It would be good to switch to yaml by this occassion.
Signature · 2 lines
Best regards, Jacek Anaszewski
Hello Jacek,
Am Donnerstag, 27. August 2020, 23:28:45 CEST schrieb Jacek Anaszewski:
Quoted 74 lines · Alexander Dahl
Quoted 73 lines
From: Alexander Dahl <[email protected]>
If LEDs are configured through device tree and the property 'label' is omitted, the label is supposed to be generated from the properties 'function' and 'color' if present. While this works fine for e.g. the 'leds-gpio' driver, it did not for 'leds-pwm'.
The reason is, you get this label naming magic only if you add a LED device through '
devm_led_classdev_register_ext()' and pass a pointer to the current device tree node. The approach to fix this was adopted from the 'leds-gpio' driver.For the following node from dts the LED appeared as 'led5' in sysfs before and as 'red:debug' after this change.
pwm_leds { compatible = "pwm-leds"; led5 { function = LED_FUNCTION_DEBUG; color = <LED_COLOR_ID_RED>; pwms = <&pwm0 2 10000000 0>; max-brightness = <127>; linux,default-trigger = "heartbeat"; panic-indicator; }; };Signed-off-by Alexander Dahl <[email protected]>---
Notes:
v1: based on v5.9-rc2, backport on v5.4.59 also worksdrivers/leds/leds-pwm.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)drivers/leds/leds-pwm.c +0
-0@@ -65,6 +65,7 @@ static int led_pwm_add(struct device *dev, struct
led_pwm_priv*priv,>
structled_pwm*led, structfwnode_handle*fwnode){ struct led_pwm_data *led_data = &priv->leds[priv->num_leds];drivers/leds/leds-pwm.c +1
-0+struct led_init_data init_data = {};int ret; led_data->active_low = led->active_low;@@ -90,7 +91,13 @@ static int led_pwm_add(struct device *dev, struct
led_pwm_priv*priv,>
pwm_init_state(led_data->pwm, &led_data->pwmstate);drivers/leds/leds-pwm.c +7
-1- ret =devm_led_classdev_register (dev, &led_data->cdev); +if (fwnode) { + init_data.fwnode = fwnode; + ret =devm_led_classdev_register_ext (dev, &led_data->cdev, + &init_data); + }else { + ret =devm_led_classdev_register (dev, &led_data->cdev); + }
Quoted 8 lines
Quoted 5 lines
if (ret) {dev_err(dev, "failed to register PWM led for %s: %d\n",
led->name, ret);This part looks good, but corresponding update of
Documentation/devicetree/bindings/leds/leds-pwm.txtis needed as well.
I'm not sure, what needs updating. The properties 'function' and 'color' are already documented in Documentation/devicetree/bindings/leds/common.yaml … the only thing I can think of here is updating the examples? That would be nice, as would be updating to yaml, but I don't see the strong relation, yet.
Quoted 1 line
It would be good to switch to yaml by this occassion.
Is there some guidance on that in general?
Greets
Alex
Hi Alexander,
Quoted 86 lines · Alexander Dahl
Hello Jacek,
Am Donnerstag, 27. August 2020, 23:28:45 CEST schrieb Jacek Anaszewski:
Quoted 74 lines · Alexander Dahl
Quoted 73 lines
From: Alexander Dahl <[email protected]>
If LEDs are configured through device tree and the property 'label' is omitted, the label is supposed to be generated from the properties 'function' and 'color' if present. While this works fine for e.g. the 'leds-gpio' driver, it did not for 'leds-pwm'.
The reason is, you get this label naming magic only if you add a LED device through '
devm_led_classdev_register_ext()' and pass a pointer to the current device tree node. The approach to fix this was adopted from the 'leds-gpio' driver.For the following node from dts the LED appeared as 'led5' in sysfs before and as 'red:debug' after this change.
pwm_leds { compatible = "pwm-leds"; led5 { function = LED_FUNCTION_DEBUG; color = <LED_COLOR_ID_RED>; pwms = <&pwm0 2 10000000 0>; max-brightness = <127>; linux,default-trigger = "heartbeat"; panic-indicator; }; };Signed-off-by Alexander Dahl <[email protected]>---
Notes:
v1: based on v5.9-rc2, backport on v5.4.59 also worksdrivers/leds/leds-pwm.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)drivers/leds/leds-pwm.c +0
-0@@ -65,6 +65,7 @@ static int led_pwm_add(struct device *dev, struct
led_pwm_priv*priv,>
structled_pwm*led, structfwnode_handle*fwnode){ struct led_pwm_data *led_data = &priv->leds[priv->num_leds];drivers/leds/leds-pwm.c +1
-0+struct led_init_data init_data = {};int ret; led_data->active_low = led->active_low;@@ -90,7 +91,13 @@ static int led_pwm_add(struct device *dev, struct
led_pwm_priv*priv,>
pwm_init_state(led_data->pwm, &led_data->pwmstate);drivers/leds/leds-pwm.c +7
-1- ret =devm_led_classdev_register (dev, &led_data->cdev); +if (fwnode) { + init_data.fwnode = fwnode; + ret =devm_led_classdev_register_ext (dev, &led_data->cdev, + &init_data); + }else { + ret =devm_led_classdev_register (dev, &led_data->cdev); + }Quoted 8 lines
Quoted 5 lines
if (ret) {dev_err(dev, "failed to register PWM led for %s: %d\n",
led->name, ret);This part looks good, but corresponding update of
Documentation/devicetree/bindings/leds/leds-pwm.txtis needed as well.
Quoted 4 lines
I'm not sure, what needs updating. The properties 'function' and 'color' are already documented in
Documentation/devicetree/bindings/leds/common.yaml… the only thing I can think of here is updating the examples? That would be nice, as would be updating to yaml, but I don't see the strong relation, yet.
It is necessary to tell the user that given driver is capable of utilizing a property. I thought of something like in commit [0].
Quoted 3 lines
Quoted 1 line
It would be good to switch to yaml by this occassion.
Is there some guidance on that in general?
I am not aware of, but surely sooner or later all bindings will need to be unified. Touching the file is always a good opportunity to address that. It's up to you, though.
Signature · 2 lines
Best regards, Jacek Anaszewski
Hello Jacek,
Am Freitag, 28. August 2020, 22:43:02 CEST schrieb Jacek Anaszewski:
Quoted 13 lines · Alexander Dahl
Quoted 9 lines
Am Donnerstag, 27. August 2020, 23:28:45 CEST schrieb Jacek Anaszewski:
Quoted 2 lines
This part looks good, but corresponding update of
Documentation/devicetree/bindings/leds/leds-pwm.txtis needed as well.I'm not sure, what needs updating. The properties 'function' and 'color' are already documented in
Documentation/devicetree/bindings/leds/common.yaml… the only thing I can think of here is updating the examples? That would be nice, as would be updating to yaml, but I don't see the strong relation, yet.It is necessary to tell the user that given driver is capable of utilizing a property. I thought of something like in commit [0].
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
It would be good to switch to yaml by this occassion.
Is there some guidance on that in general?
I am not aware of, but surely sooner or later all bindings will need to be unified. Touching the file is always a good opportunity to address that. It's up to you, though.
This update from txt to yaml is a manual task and after reading [1] and some other examples, I tried to come up with something. I pushed the WIP to my GitHub tree and will run the checks recommended by [1] later in the evening. If that goes well, I'll send a v2 series.
Quoted 4 lines
[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Do
cumentation/devicetree/bindings/leds/leds-lm3692x.txt?id=4dcbc8f8c59f4b618d651f5ba884ee5bf562c8de
Well okay, that was for the old format, but I see what you mean.
Greets
Alex
[1] https://www.kernel.org/doc/html/latest/devicetree/writing-schema.html