| Message ID | 20200907043459.2961-2-post@lespocky.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] leds: pwm: Allow automatic labels for DT based devices | expand |
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
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
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
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);
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> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> --- Notes: v2 -> v3: * added Acked-by drivers/leds/leds-pwm.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)