When probing if default LED state is on then default brightness will be
applied instead of max brightness.
Signed-off-by: George Stark <gnstark@salutedevices.com>
---
drivers/leds/leds-pwm.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 7961dca0db2f..514fc8ca3e80 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -65,7 +65,8 @@ static int led_pwm_set(struct led_classdev *led_cdev,
__attribute__((nonnull))
static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
- struct led_pwm *led, struct fwnode_handle *fwnode)
+ struct led_pwm *led, struct fwnode_handle *fwnode,
+ unsigned int default_brightness)
{
struct led_pwm_data *led_data = &priv->leds[priv->num_leds];
struct led_init_data init_data = { .fwnode = fwnode };
@@ -104,7 +105,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
/* set brightness */
switch (led->default_state) {
case LEDS_DEFSTATE_ON:
- led_data->cdev.brightness = led->max_brightness;
+ led_data->cdev.brightness = default_brightness;
break;
case LEDS_DEFSTATE_KEEP:
{
@@ -141,6 +142,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
{
struct led_pwm led;
+ unsigned int default_brightness;
int ret;
device_for_each_child_node_scoped(dev, fwnode) {
@@ -160,7 +162,12 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
led.default_state = led_init_default_state_get(fwnode);
- ret = led_pwm_add(dev, priv, &led, fwnode);
+ ret = fwnode_property_read_u32(fwnode, "default-brightness",
+ &default_brightness);
+ if (ret < 0 || default_brightness > led.max_brightness)
+ default_brightness = led.max_brightness;
+
+ ret = led_pwm_add(dev, priv, &led, fwnode, default_brightness);
if (ret)
return ret;
}
--
2.25.1
On Tue, 15 Oct 2024, George Stark wrote: > When probing if default LED state is on then default brightness will be > applied instead of max brightness. > > Signed-off-by: George Stark <gnstark@salutedevices.com> > --- > drivers/leds/leds-pwm.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c > index 7961dca0db2f..514fc8ca3e80 100644 > --- a/drivers/leds/leds-pwm.c > +++ b/drivers/leds/leds-pwm.c > @@ -65,7 +65,8 @@ static int led_pwm_set(struct led_classdev *led_cdev, > > __attribute__((nonnull)) > static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv, > - struct led_pwm *led, struct fwnode_handle *fwnode) > + struct led_pwm *led, struct fwnode_handle *fwnode, > + unsigned int default_brightness) > { > struct led_pwm_data *led_data = &priv->leds[priv->num_leds]; > struct led_init_data init_data = { .fwnode = fwnode }; > @@ -104,7 +105,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv, > /* set brightness */ > switch (led->default_state) { > case LEDS_DEFSTATE_ON: > - led_data->cdev.brightness = led->max_brightness; > + led_data->cdev.brightness = default_brightness; > break; > case LEDS_DEFSTATE_KEEP: > { > @@ -141,6 +142,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv, > static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv) > { > struct led_pwm led; > + unsigned int default_brightness; > int ret; > > device_for_each_child_node_scoped(dev, fwnode) { > @@ -160,7 +162,12 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv) > > led.default_state = led_init_default_state_get(fwnode); > > - ret = led_pwm_add(dev, priv, &led, fwnode); > + ret = fwnode_property_read_u32(fwnode, "default-brightness", > + &default_brightness); > + if (ret < 0 || default_brightness > led.max_brightness) > + default_brightness = led.max_brightness; > + > + ret = led_pwm_add(dev, priv, &led, fwnode, default_brightness); This creates a lot more hopping around than is necessary. Since led_pwm_add() already has access to the fwnode, why not look up the property in there instead, thus massively simplifying things. > if (ret) > return ret; > } > -- > 2.25.1 > -- Lee Jones [李琼斯]
Hello Lee Thanks for the review! On 10/31/24 17:32, Lee Jones wrote: > On Tue, 15 Oct 2024, George Stark wrote: > >> When probing if default LED state is on then default brightness will be >> applied instead of max brightness. >> >> Signed-off-by: George Stark <gnstark@salutedevices.com> >> --- >> drivers/leds/leds-pwm.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c >> index 7961dca0db2f..514fc8ca3e80 100644 >> --- a/drivers/leds/leds-pwm.c >> +++ b/drivers/leds/leds-pwm.c >> @@ -65,7 +65,8 @@ static int led_pwm_set(struct led_classdev *led_cdev, >> >> __attribute__((nonnull)) >> static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv, >> - struct led_pwm *led, struct fwnode_handle *fwnode) >> + struct led_pwm *led, struct fwnode_handle *fwnode, >> + unsigned int default_brightness) >> { >> struct led_pwm_data *led_data = &priv->leds[priv->num_leds]; >> struct led_init_data init_data = { .fwnode = fwnode }; >> @@ -104,7 +105,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv, >> /* set brightness */ >> switch (led->default_state) { >> case LEDS_DEFSTATE_ON: >> - led_data->cdev.brightness = led->max_brightness; >> + led_data->cdev.brightness = default_brightness; >> break; >> case LEDS_DEFSTATE_KEEP: >> { >> @@ -141,6 +142,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv, >> static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv) >> { >> struct led_pwm led; >> + unsigned int default_brightness; >> int ret; >> >> device_for_each_child_node_scoped(dev, fwnode) { >> @@ -160,7 +162,12 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv) >> >> led.default_state = led_init_default_state_get(fwnode); >> >> - ret = led_pwm_add(dev, priv, &led, fwnode); >> + ret = fwnode_property_read_u32(fwnode, "default-brightness", >> + &default_brightness); >> + if (ret < 0 || default_brightness > led.max_brightness) >> + default_brightness = led.max_brightness; >> + >> + ret = led_pwm_add(dev, priv, &led, fwnode, default_brightness); > > This creates a lot more hopping around than is necessary. > > Since led_pwm_add() already has access to the fwnode, why not look up > the property in there instead, thus massively simplifying things. I looked up the new property here to be near to led_init_default_state_get (both props are from the same group) and led_pwm_add is big enough already. And you're absolutely right that the patch can be optimized. Please take a look at the v2 > > >> if (ret) >> return ret; >> } >> -- >> 2.25.1 >> > -- Best regards George
© 2016 - 2024 Red Hat, Inc.