mbox series

[v3,0/2] leds: pwm: Make automatic labels work

Message ID 20200907043459.2961-1-post@lespocky.de
Headers show
Series leds: pwm: Make automatic labels work | expand

Message

Alexander Dahl Sept. 7, 2020, 4:34 a.m. UTC 
Hei hei,

for leds-gpio you can use the properties 'function' and 'color' in the
devicetree node and omit 'label', the label is constructed
automatically.  This is a common feature supposed to be working for all
LED drivers.  However it did not yet work for the 'leds-pwm' driver.
This series fixes the driver and takes the opportunity to update the
dt-bindings accordingly.

v3:
- rebased on v5.9-rc4
- changed license of .yaml file to recommended one
- added Acked-by

v2:
- rebased on v5.9-rc3
- added the dt-bindings update patch

v1:
- based on v5.9-rc2
- backport on v5.4.59 tested and working

Greets
Alex

Alexander Dahl (2):
  leds: pwm: Allow automatic labels for DT based devices
  dt-bindings: leds: Convert pwm to yaml

 .../devicetree/bindings/leds/leds-pwm.txt     | 50 -----------
 .../devicetree/bindings/leds/leds-pwm.yaml    | 85 +++++++++++++++++++
 drivers/leds/leds-pwm.c                       |  9 +-
 3 files changed, 93 insertions(+), 51 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.txt
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pwm.yaml

Comments

Pavel Machek Sept. 9, 2020, 9:07 a.m. UTC | #1 
Hi!

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

Can you always use _ext version, even with null fwnode? If not, can
you fix the core to accept that? Having that conditional in driver is
ugly.

Best regards,
									Pavel
Alexander Dahl Sept. 9, 2020, 8:29 p.m. UTC | #2 
Hei hei,

On Wed, Sep 09, 2020 at 11:07:36AM +0200, Pavel Machek wrote:
> Hi!
> 
> >  	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);
> > +	}
> 
> Can you always use _ext version, even with null fwnode? 

I did not try on real hardware, but from reading the code I would say
the following would happen: led_classdev_register_ext() calls
led_compose_name(parent, init_data, composed_name) which itself calls
led_parse_fwnode_props(dev, fwnode, &props); that returns early due to
fwnode==NULL without changing props, thus this stays as initialized
with {}, so led_compose_name() would return -EINVAL which would let
led_classdev_register_ext() fail, too.

> If not, can you fix the core to accept that? Having that conditional
> in driver is ugly.

It is ugly, although the approach is inspired by the leds-gpio driver.
I'll see if I can come up with a change to led-core, but I'm also open
for suggestions. ;-)

fyi: Peter Ujfalusi answered and would give his Ack to the changed
dual license for the yaml file.  You can expect that for v4.

Stay tuned
Alex
Jacek Anaszewski Sept. 9, 2020, 8:47 p.m. UTC | #3 
Hi Alexander,

On 9/9/20 10:29 PM, Alexander Dahl wrote:
> Hei hei,
> 
> On Wed, Sep 09, 2020 at 11:07:36AM +0200, Pavel Machek wrote:
>> Hi!
>>
>>>   	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);
>>> +	}
>>
>> Can you always use _ext version, even with null fwnode?
> 
> I did not try on real hardware, but from reading the code I would say
> the following would happen: led_classdev_register_ext() calls
> led_compose_name(parent, init_data, composed_name) which itself calls
> led_parse_fwnode_props(dev, fwnode, &props); that returns early due to
> fwnode==NULL without changing props, thus this stays as initialized
> with {}, so led_compose_name() would return -EINVAL which would let
> led_classdev_register_ext() fail, too.
> 
>> If not, can you fix the core to accept that? Having that conditional
>> in driver is ugly.
> 
> It is ugly, although the approach is inspired by the leds-gpio driver.
> I'll see if I can come up with a change to led-core, but I'm also open
> for suggestions. ;-)

devm_led_classdev_register() calls devm_led_classdev_register_ext()
with NULL passed in place of init_data, so you could do something like
below to achieve the same without touching LED core:

struct led_init_data init_data_impl = { .fwnode = fwnode };
struct led_init_data *init_data = NULL;

if (fwnode)
	init_data = &init_data_impl;

devm_led_classdev_register_ext(dev, &led_data->cdev, init_data);

> fyi: Peter Ujfalusi answered and would give his Ack to the changed
> dual license for the yaml file.  You can expect that for v4.
> 
> Stay tuned
> Alex
>
Pavel Machek Sept. 9, 2020, 8:58 p.m. UTC | #4 
Hi!

> >>>  	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);
> >>>+	}
> >>
> >>Can you always use _ext version, even with null fwnode?
> >
> >I did not try on real hardware, but from reading the code I would say
> >the following would happen: led_classdev_register_ext() calls
> >led_compose_name(parent, init_data, composed_name) which itself calls
> >led_parse_fwnode_props(dev, fwnode, &props); that returns early due to
> >fwnode==NULL without changing props, thus this stays as initialized
> >with {}, so led_compose_name() would return -EINVAL which would let
> >led_classdev_register_ext() fail, too.
> >
> >>If not, can you fix the core to accept that? Having that conditional
> >>in driver is ugly.
> >
> >It is ugly, although the approach is inspired by the leds-gpio driver.
> >I'll see if I can come up with a change to led-core, but I'm also open
> >for suggestions. ;-)
> 
> devm_led_classdev_register() calls devm_led_classdev_register_ext()
> with NULL passed in place of init_data, so you could do something like
> below to achieve the same without touching LED core:
> 
> struct led_init_data init_data_impl = { .fwnode = fwnode };
> struct led_init_data *init_data = NULL;
> 
> if (fwnode)
> 	init_data = &init_data_impl;
> 
> devm_led_classdev_register_ext(dev, &led_data->cdev, init_data);

Umm.. This is not too great, either. Maybe I'd really prefer the
change to the LED core.

> >fyi: Peter Ujfalusi answered and would give his Ack to the changed
> >dual license for the yaml file.  You can expect that for v4.

Good :-).

Best regards,
									pavel