[PATCH 0/2] leds: pwm: Add default-brightness property

[PATCH 0/2] leds: pwm: Add default-brightness property

[George Stark]

led-pwm driver supports default-state DT property and if that state is on then
the driver during initialization turns on the LED setting maximum brightness.
Sometimes it's desirable to use lower initial brightness.
This patch series adds support for DT property default-brightness.

Patch series is prepared for linux-next

Things to discuss:
If such a property is acceptable it could be moved to leds/common.yaml due to
several drivers support multiple brightness levels and could support the property
too.

George Stark (2):
  dt-bindings: leds: pwm: Add default-brightness property
  leds: pwm: Add optional DT property default-brightness

 .../devicetree/bindings/leds/leds-pwm.yaml          |  6 ++++++
 drivers/leds/leds-pwm.c                             | 13 ++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

--
2.25.1

[PATCH 1/2] dt-bindings: leds: pwm: Add default-brightness property

[George Stark]

Optional default-brightness property specifies brightness value to be
used if default LED state is on.

Signed-off-by: George Stark <gnstark@salutedevices.com>
---
 Documentation/devicetree/bindings/leds/leds-pwm.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.yaml b/Documentation/devicetree/bindings/leds/leds-pwm.yaml
index 113b7c218303..61b97e8bc36d 100644
--- a/Documentation/devicetree/bindings/leds/leds-pwm.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-pwm.yaml
@@ -34,6 +34,12 @@ patternProperties:
           Maximum brightness possible for the LED
         $ref: /schemas/types.yaml#/definitions/uint32
 
+      default-brightness:
+        description:
+          Brightness to be set if LED's default state is on. Used only during
+          initialization. If the option is not set then max brightness is used.
+        $ref: /schemas/types.yaml#/definitions/uint32
+
     required:
       - pwms
       - max-brightness
-- 
2.25.1

[PATCH 2/2] leds: pwm: Add optional DT property default-brightness

[George Stark]

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

Re: [PATCH 1/2] dt-bindings: leds: pwm: Add default-brightness property

[Rob Herring (Arm)]

On Tue, 15 Oct 2024 18:14:09 +0300, George Stark wrote:
> Optional default-brightness property specifies brightness value to be
> used if default LED state is on.
> 
> Signed-off-by: George Stark <gnstark@salutedevices.com>
> ---
>  Documentation/devicetree/bindings/leds/leds-pwm.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 2/2] leds: pwm: Add optional DT property default-brightness

[Lee Jones]

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 [李琼斯]

Re: [PATCH 2/2] leds: pwm: Add optional DT property default-brightness

[George Stark]

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