[PATCH] leds: pwm: Allow automatic labels for DT based devices

[PATCH] leds: pwm: Allow automatic labels for DT based devices

[Alexander Dahl]

From: Alexander Dahl <post@lespocky.de>

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 <post@lespocky.de>
---

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

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index ef7b91bd2064..a27a1d75a3e9 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -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);
-- 
2.27.0

Re: [PATCH] leds: pwm: Allow automatic labels for DT based devices

[Jacek Anaszewski]

Hi Alexander,

On 8/26/20 11:37 AM, Alexander Dahl wrote:
> From: Alexander Dahl <post@lespocky.de>
> 
> 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 <post@lespocky.de>
> ---
> 
> 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(-)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index ef7b91bd2064..a27a1d75a3e9 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -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);
> 

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.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: pwm: Allow automatic labels for DT based devices

[Alexander Dahl]

Hello Jacek,

Am Donnerstag, 27. August 2020, 23:28:45 CEST schrieb Jacek Anaszewski:
> On 8/26/20 11:37 AM, Alexander Dahl wrote:
> > From: Alexander Dahl <post@lespocky.de>
> > 
> > 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 <post@lespocky.de>
> > ---
> > 
> > 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(-)
> > 
> > diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> > index ef7b91bd2064..a27a1d75a3e9 100644
> > --- a/drivers/leds/leds-pwm.c
> > +++ b/drivers/leds/leds-pwm.c
> > @@ -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);
> 
> This part looks good, but corresponding update of
> Documentation/devicetree/bindings/leds/leds-pwm.txt is 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 would be good to switch to yaml by this occassion.

Is there some guidance on that in general?

Greets
Alex

Re: [PATCH] leds: pwm: Allow automatic labels for DT based devices

[Jacek Anaszewski]

Hi Alexander,

On 8/28/20 9:00 AM, Alexander Dahl wrote:
> Hello Jacek,
> 
> Am Donnerstag, 27. August 2020, 23:28:45 CEST schrieb Jacek Anaszewski:
>> On 8/26/20 11:37 AM, Alexander Dahl wrote:
>>> From: Alexander Dahl <post@lespocky.de>
>>>
>>> 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 <post@lespocky.de>
>>> ---
>>>
>>> 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(-)
>>>
>>> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
>>> index ef7b91bd2064..a27a1d75a3e9 100644
>>> --- a/drivers/leds/leds-pwm.c
>>> +++ b/drivers/leds/leds-pwm.c
>>> @@ -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);
>>
>> This part looks good, but corresponding update of
>> Documentation/devicetree/bindings/leds/leds-pwm.txt is 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].

>> 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.

[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/leds/leds-lm3692x.txt?id=4dcbc8f8c59f4b618d651f5ba884ee5bf562c8de

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: pwm: Allow automatic labels for DT based devices

[Alexander Dahl]

Hello Jacek,

Am Freitag, 28. August 2020, 22:43:02 CEST schrieb Jacek Anaszewski:
> On 8/28/20 9:00 AM, Alexander Dahl wrote:
> > Am Donnerstag, 27. August 2020, 23:28:45 CEST schrieb Jacek Anaszewski:
> >> This part looks good, but corresponding update of
> >> Documentation/devicetree/bindings/leds/leds-pwm.txt is 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].
> 
> >> 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.

> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Do
> cumentation/devicetree/bindings/leds/leds-lm3692x.txt?id=4dcbc8f8c59f4b618d6
> 51f5ba884ee5bf562c8de

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