[PATCH v2] leds: pwm: add support for default-state device property

[PATCH v2] leds: pwm: add support for default-state device property

[Denis Osterland-Heim]

This patch adds support for "default-state" devicetree property, which
allows to defer pwm init to first use of led.

This allows to configure the PWM early in bootloader to let the LED
blink until an application in Linux userspace set something different.

Signed-off-by: Denis Osterland-Heim <Denis.Osterland@diehl.com>
---
v1->v2:
  - use default-state = "keep", as suggested by Jacek Anaszewski
  - calc initial brightness with PWM state from device

 .../devicetree/bindings/leds/leds-pwm.txt     |  2 ++
 drivers/leds/leds-pwm.c                       | 33 +++++++++++++++++--
 include/linux/leds_pwm.h                      |  1 +
 3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.txt b/Documentation/devicetree/bindings/leds/leds-pwm.txt
index 6c6583c35f2f..d0f489680594 100644
--- a/Documentation/devicetree/bindings/leds/leds-pwm.txt
+++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
@@ -19,6 +19,8 @@ LED sub-node properties:
   see Documentation/devicetree/bindings/leds/common.txt
 - linux,default-trigger :  (optional)
   see Documentation/devicetree/bindings/leds/common.txt
+- default-state : (optional)
+  see Documentation/devicetree/bindings/leds/common.yaml
 
 Example:
 
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 8b6965a563e9..92726c2e43ba 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -75,7 +75,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	led_data->active_low = led->active_low;
 	led_data->cdev.name = led->name;
 	led_data->cdev.default_trigger = led->default_trigger;
-	led_data->cdev.brightness = LED_OFF;
+	ret = led->default_state == LEDS_GPIO_DEFSTATE_ON;
+	led_data->cdev.brightness = ret ? led->max_brightness : LED_OFF;
 	led_data->cdev.max_brightness = led->max_brightness;
 	led_data->cdev.flags = LED_CORE_SUSPENDRESUME;
 
@@ -97,7 +98,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	 * FIXME: pwm_apply_args() should be removed when switching to the
 	 * atomic PWM API.
 	 */
-	pwm_apply_args(led_data->pwm);
+	if (led->default_state != LEDS_GPIO_DEFSTATE_KEEP)
+		pwm_apply_args(led_data->pwm);
 
 	pwm_get_args(led_data->pwm, &pargs);
 
@@ -105,10 +107,23 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	if (!led_data->period && (led->pwm_period_ns > 0))
 		led_data->period = led->pwm_period_ns;
 
+	if (led->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
+		uint64_t brightness;
+		struct pwm_device *pwm = led_data->pwm;
+		struct pwm_state state;
+
+		pwm->chip->ops->get_state(pwm->chip, pwm, &state);
+		brightness = led->max_brightness * state.duty_cycle;
+		do_div(brightness, state.period);
+		led_data->cdev.brightness = (enum led_brightness)brightness;
+	}
+
 	ret = devm_led_classdev_register(dev, &led_data->cdev);
 	if (ret == 0) {
 		priv->num_leds++;
-		led_pwm_set(&led_data->cdev, led_data->cdev.brightness);
+		if (led->default_state != LEDS_GPIO_DEFSTATE_KEEP)
+			led_pwm_set(&led_data->cdev,
+					led_data->cdev.brightness);
 	} else {
 		dev_err(dev, "failed to register PWM led for %s: %d\n",
 			led->name, ret);
@@ -126,6 +141,8 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
 	memset(&led, 0, sizeof(led));
 
 	device_for_each_child_node(dev, fwnode) {
+		const char *state = NULL;
+
 		ret = fwnode_property_read_string(fwnode, "label", &led.name);
 		if (ret && is_of_node(fwnode))
 			led.name = to_of_node(fwnode)->name;
@@ -143,6 +160,16 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
 		fwnode_property_read_u32(fwnode, "max-brightness",
 					 &led.max_brightness);
 
+		if (!fwnode_property_read_string(fwnode, "default-state",
+						 &state)) {
+			if (!strcmp(state, "keep"))
+				led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
+			else if (!strcmp(state, "on"))
+				led.default_state = LEDS_GPIO_DEFSTATE_ON;
+			else
+				led.default_state = LEDS_GPIO_DEFSTATE_OFF;
+		}
+
 		ret = led_pwm_add(dev, priv, &led, fwnode);
 		if (ret) {
 			fwnode_handle_put(fwnode);
diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
index 93d101d28943..c9ef9012913d 100644
--- a/include/linux/leds_pwm.h
+++ b/include/linux/leds_pwm.h
@@ -10,6 +10,7 @@ struct led_pwm {
 	const char	*default_trigger;
 	unsigned	pwm_id __deprecated;
 	u8 		active_low;
+	u8		default_state;
 	unsigned 	max_brightness;
 	unsigned	pwm_period_ns;
 };
-- 
2.25.1



Diehl Connectivity Solutions GmbH
Geschäftsführung: Horst Leonberger
Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
Nürnberg: HRB 32315
___________________________________________________________________________________________________

Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht.
Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
- Informationen zum Datenschutz, insbesondere zu Ihren Rechten, erhalten Sie unter https://www.diehl.com/group/de/transparenz-und-informationspflichten/

The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited. 
- For general information on data protection and your respective rights please visit https://www.diehl.com/group/en/transparency-and-information-obligations/

Re: [PATCH v2] leds: pwm: add support for default-state device property

[Denis Osterland-Heim]

Hi,

should be
In-Reply-To: <20200309082218.13263-1-Denis.Osterland@diehl.com>
instead of
Reply-To: <20200309082218.13263-1-Denis.Osterland@diehl.com>

Sorry

Am Dienstag, den 10.03.2020, 13:47 +0100 schrieb Denis Osterland-Heim:
> This patch adds support for "default-state" devicetree property, which
> allows to defer pwm init to first use of led.
> 
> This allows to configure the PWM early in bootloader to let the LED
> blink until an application in Linux userspace set something different.
> 
> Signed-off-by: Denis Osterland-Heim <Denis.Osterland@diehl.com>
> ---
> v1->v2:
>   - use default-state = "keep", as suggested by Jacek Anaszewski
>   - calc initial brightness with PWM state from device
> 
>  .../devicetree/bindings/leds/leds-pwm.txt     |  2 ++
>  drivers/leds/leds-pwm.c                       | 33 +++++++++++++++++--
>  include/linux/leds_pwm.h                      |  1 +
>  3 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.txt b/Documentation/devicetree/bindings/leds/leds-pwm.txt
> index 6c6583c35f2f..d0f489680594 100644
> --- a/Documentation/devicetree/bindings/leds/leds-pwm.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
> @@ -19,6 +19,8 @@ LED sub-node properties:
>    see Documentation/devicetree/bindings/leds/common.txt
>  - linux,default-trigger :  (optional)
>    see Documentation/devicetree/bindings/leds/common.txt
> +- default-state : (optional)
> +  see Documentation/devicetree/bindings/leds/common.yaml
>  
>  Example:
>  
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index 8b6965a563e9..92726c2e43ba 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -75,7 +75,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>  	led_data->active_low = led->active_low;
>  	led_data->cdev.name = led->name;
>  	led_data->cdev.default_trigger = led->default_trigger;
> -	led_data->cdev.brightness = LED_OFF;
> +	ret = led->default_state == LEDS_GPIO_DEFSTATE_ON;
> +	led_data->cdev.brightness = ret ? led->max_brightness : LED_OFF;
>  	led_data->cdev.max_brightness = led->max_brightness;
>  	led_data->cdev.flags = LED_CORE_SUSPENDRESUME;
>  
> @@ -97,7 +98,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>  	 * FIXME: pwm_apply_args() should be removed when switching to the
>  	 * atomic PWM API.
>  	 */
> -	pwm_apply_args(led_data->pwm);
> +	if (led->default_state != LEDS_GPIO_DEFSTATE_KEEP)
> +		pwm_apply_args(led_data->pwm);
>  
>  	pwm_get_args(led_data->pwm, &pargs);
>  
> @@ -105,10 +107,23 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>  	if (!led_data->period && (led->pwm_period_ns > 0))
>  		led_data->period = led->pwm_period_ns;
>  
> +	if (led->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
> +		uint64_t brightness;
> +		struct pwm_device *pwm = led_data->pwm;
> +		struct pwm_state state;
> +
> +		pwm->chip->ops->get_state(pwm->chip, pwm, &state);
> +		brightness = led->max_brightness * state.duty_cycle;
> +		do_div(brightness, state.period);
> +		led_data->cdev.brightness = (enum led_brightness)brightness;
> +	}
> +
>  	ret = devm_led_classdev_register(dev, &led_data->cdev);
>  	if (ret == 0) {
>  		priv->num_leds++;
> -		led_pwm_set(&led_data->cdev, led_data->cdev.brightness);
> +		if (led->default_state != LEDS_GPIO_DEFSTATE_KEEP)
> +			led_pwm_set(&led_data->cdev,
> +					led_data->cdev.brightness);
>  	} else {
>  		dev_err(dev, "failed to register PWM led for %s: %d\n",
>  			led->name, ret);
> @@ -126,6 +141,8 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
>  	memset(&led, 0, sizeof(led));
>  
>  	device_for_each_child_node(dev, fwnode) {
> +		const char *state = NULL;
> +
>  		ret = fwnode_property_read_string(fwnode, "label", &led.name);
>  		if (ret && is_of_node(fwnode))
>  			led.name = to_of_node(fwnode)->name;
> @@ -143,6 +160,16 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
>  		fwnode_property_read_u32(fwnode, "max-brightness",
>  					 &led.max_brightness);
>  
> +		if (!fwnode_property_read_string(fwnode, "default-state",
> +						 &state)) {
> +			if (!strcmp(state, "keep"))
> +				led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
> +			else if (!strcmp(state, "on"))
> +				led.default_state = LEDS_GPIO_DEFSTATE_ON;
> +			else
> +				led.default_state = LEDS_GPIO_DEFSTATE_OFF;
> +		}
> +
>  		ret = led_pwm_add(dev, priv, &led, fwnode);
>  		if (ret) {
>  			fwnode_handle_put(fwnode);
> diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
> index 93d101d28943..c9ef9012913d 100644
> --- a/include/linux/leds_pwm.h
> +++ b/include/linux/leds_pwm.h
> @@ -10,6 +10,7 @@ struct led_pwm {
>  	const char	*default_trigger;
>  	unsigned	pwm_id __deprecated;
>  	u8 		active_low;
> +	u8		default_state;
>  	unsigned 	max_brightness;
>  	unsigned	pwm_period_ns;
>  };


Diehl Connectivity Solutions GmbH
Geschäftsführung: Horst Leonberger
Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
Nürnberg: HRB 32315
___________________________________________________________________________________________________

Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht.
Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
- Informationen zum Datenschutz, insbesondere zu Ihren Rechten, erhalten Sie unter https://www.diehl.com/group/de/transparenz-und-informationspflichten/

The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited. 
- For general information on data protection and your respective rights please visit https://www.diehl.com/group/en/transparency-and-information-obligations/

Re: [PATCH v2] leds: pwm: add support for default-state device property

[Jacek Anaszewski]

Hi Denis,

Thank you for the update. Please find my remarks below.

On 3/10/20 4:19 PM, Denis Osterland-Heim wrote:
> Hi,
> 
> should be
> In-Reply-To: <20200309082218.13263-1-Denis.Osterland@diehl.com>
> instead of
> Reply-To: <20200309082218.13263-1-Denis.Osterland@diehl.com>
> 
> Sorry
> 
> Am Dienstag, den 10.03.2020, 13:47 +0100 schrieb Denis Osterland-Heim:
>> This patch adds support for "default-state" devicetree property, which
>> allows to defer pwm init to first use of led.
>>
>> This allows to configure the PWM early in bootloader to let the LED
>> blink until an application in Linux userspace set something different.
>>
>> Signed-off-by: Denis Osterland-Heim <Denis.Osterland@diehl.com>
>> ---
>> v1->v2:
>>   - use default-state = "keep", as suggested by Jacek Anaszewski
>>   - calc initial brightness with PWM state from device
>>
>>  .../devicetree/bindings/leds/leds-pwm.txt     |  2 ++
>>  drivers/leds/leds-pwm.c                       | 33 +++++++++++++++++--
>>  include/linux/leds_pwm.h                      |  1 +
>>  3 files changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.txt b/Documentation/devicetree/bindings/leds/leds-pwm.txt
>> index 6c6583c35f2f..d0f489680594 100644
>> --- a/Documentation/devicetree/bindings/leds/leds-pwm.txt
>> +++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
>> @@ -19,6 +19,8 @@ LED sub-node properties:
>>    see Documentation/devicetree/bindings/leds/common.txt
>>  - linux,default-trigger :  (optional)
>>    see Documentation/devicetree/bindings/leds/common.txt
>> +- default-state : (optional)
>> +  see Documentation/devicetree/bindings/leds/common.yaml
>>  
>>  Example:
>>  
>> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
>> index 8b6965a563e9..92726c2e43ba 100644
>> --- a/drivers/leds/leds-pwm.c
>> +++ b/drivers/leds/leds-pwm.c
>> @@ -75,7 +75,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>>  	led_data->active_low = led->active_low;
>>  	led_data->cdev.name = led->name;
>>  	led_data->cdev.default_trigger = led->default_trigger;
>> -	led_data->cdev.brightness = LED_OFF;
>> +	ret = led->default_state == LEDS_GPIO_DEFSTATE_ON;

ret is for return value and it should not be used for anything
else just because it is at hand. Also LEDS_GPIO* definitions have
nothing to do with pwm leds. This is legacy because default-state
property was primarily specific to leds-gpio bindings and only
later was made common.

Please introduce corresponding LEDS_PWM definitions, but in leds-pwm.c.

>> +	led_data->cdev.brightness = ret ? led->max_brightness : LED_OFF;

Instead of above two changes I'd add below:

if (led->default_state == LEDS_PWM_DEFSTATE_ON) {
	led_data->cdev.brightness = led->max_brightness;
} else if (led->default_state == LEDS_PWM_DEFSTATE_KEEP)) {
	// here put what you're adding below, but please use
	// pwm_get_state() instead of accessing ops directly
}

LED_OFF case is covered by kzalloc() in led_pwm_probe().

>>  	led_data->cdev.max_brightness = led->max_brightness;
>>  	led_data->cdev.flags = LED_CORE_SUSPENDRESUME;
>>  
>> @@ -97,7 +98,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>>  	 * FIXME: pwm_apply_args() should be removed when switching to the
>>  	 * atomic PWM API.
>>  	 */
>> -	pwm_apply_args(led_data->pwm);
>> +	if (led->default_state != LEDS_GPIO_DEFSTATE_KEEP)
>> +		pwm_apply_args(led_data->pwm);
>>  
>>  	pwm_get_args(led_data->pwm, &pargs);
>>  
>> @@ -105,10 +107,23 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>>  	if (!led_data->period && (led->pwm_period_ns > 0))
>>  		led_data->period = led->pwm_period_ns;
>>  
>> +	if (led->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
>> +		uint64_t brightness;
>> +		struct pwm_device *pwm = led_data->pwm;
>> +		struct pwm_state state;
>> +
>> +		pwm->chip->ops->get_state(pwm->chip, pwm, &state);
>> +		brightness = led->max_brightness * state.duty_cycle;
>> +		do_div(brightness, state.period);
>> +		led_data->cdev.brightness = (enum led_brightness)brightness;
>> +	}
>> +
>>  	ret = devm_led_classdev_register(dev, &led_data->cdev);
>>  	if (ret == 0) {
>>  		priv->num_leds++;
>> -		led_pwm_set(&led_data->cdev, led_data->cdev.brightness);
>> +		if (led->default_state != LEDS_GPIO_DEFSTATE_KEEP)
>> +			led_pwm_set(&led_data->cdev,
>> +					led_data->cdev.brightness);
>>  	} else {
>>  		dev_err(dev, "failed to register PWM led for %s: %d\n",
>>  			led->name, ret);
>> @@ -126,6 +141,8 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
>>  	memset(&led, 0, sizeof(led));
>>  
>>  	device_for_each_child_node(dev, fwnode) {
>> +		const char *state = NULL;
>> +
>>  		ret = fwnode_property_read_string(fwnode, "label", &led.name);
>>  		if (ret && is_of_node(fwnode))
>>  			led.name = to_of_node(fwnode)->name;
>> @@ -143,6 +160,16 @@ static int led_pwm_create_fwnode(struct device *dev, struct led_pwm_priv *priv)
>>  		fwnode_property_read_u32(fwnode, "max-brightness",
>>  					 &led.max_brightness);
>>  
>> +		if (!fwnode_property_read_string(fwnode, "default-state",
>> +						 &state)) {
>> +			if (!strcmp(state, "keep"))
>> +				led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
>> +			else if (!strcmp(state, "on"))
>> +				led.default_state = LEDS_GPIO_DEFSTATE_ON;
>> +			else
>> +				led.default_state = LEDS_GPIO_DEFSTATE_OFF;
>> +		}
>> +
>>  		ret = led_pwm_add(dev, priv, &led, fwnode);
>>  		if (ret) {
>>  			fwnode_handle_put(fwnode);
>> diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
>> index 93d101d28943..c9ef9012913d 100644
>> --- a/include/linux/leds_pwm.h
>> +++ b/include/linux/leds_pwm.h
>> @@ -10,6 +10,7 @@ struct led_pwm {
>>  	const char	*default_trigger;
>>  	unsigned	pwm_id __deprecated;
>>  	u8 		active_low;
>> +	u8		default_state;
>>  	unsigned 	max_brightness;
>>  	unsigned	pwm_period_ns;
>>  };
> 
> 
> Diehl Connectivity Solutions GmbH
> Geschäftsführung: Horst Leonberger
> Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
> Nürnberg: HRB 32315
> ___________________________________________________________________________________________________
> 
> Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
> Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht.
> Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
> - Informationen zum Datenschutz, insbesondere zu Ihren Rechten, erhalten Sie unter https://www.diehl.com/group/de/transparenz-und-informationspflichten/
> 
> The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
> mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited. 
> - For general information on data protection and your respective rights please visit https://www.diehl.com/group/en/transparency-and-information-obligations/
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2] leds: pwm: add support for default-state device property

[Denis Osterland-Heim]

Hi Jacek,

Am Dienstag, den 10.03.2020, 22:15 +0100 schrieb Jacek Anaszewski:
> Hi Denis,
> 
> Thank you for the update. Please find my remarks below.
> 
> On 3/10/20 4:19 PM, Denis Osterland-Heim wrote:
> > Hi,
> > 
...
> > > --- a/drivers/leds/leds-pwm.c
> > > +++ b/drivers/leds/leds-pwm.c
> > > @@ -75,7 +75,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
> > >  	led_data->active_low = led->active_low;
> > >  	led_data->cdev.name = led->name;
> > >  	led_data->cdev.default_trigger = led->default_trigger;
> > > -	led_data->cdev.brightness = LED_OFF;
> > > +	ret = led->default_state == LEDS_GPIO_DEFSTATE_ON;
> 
> ret is for return value and it should not be used for anything
> else just because it is at hand. Also LEDS_GPIO* definitions have
> nothing to do with pwm leds. This is legacy because default-state
> property was primarily specific to leds-gpio bindings and only
> later was made common.
> 
> Please introduce corresponding LEDS_PWM definitions, but in leds-pwm.c.
will change

> 
> > > +	led_data->cdev.brightness = ret ? led->max_brightness : LED_OFF;
> 
> Instead of above two changes I'd add below:
> 
> if (led->default_state == LEDS_PWM_DEFSTATE_ON) {
> 	led_data->cdev.brightness = led->max_brightness;
> } else if (led->default_state == LEDS_PWM_DEFSTATE_KEEP)) {
> 	// here put what you're adding below, but please use
> 	// pwm_get_state() instead of accessing ops directly
> }
> 
> LED_OFF case is covered by kzalloc() in led_pwm_probe().
Looks very elegant, I will apply this.
pwm_get_state() is not sufficient here because it only returns the values from structure,
which were not read read from registers at pwm_device_request().
Something like pwm_get_state_uncached() would be required, which I have not found yet.

I looked at the atomic PWM API (5ec803edcb703fe379836f13560b79dfac79b01d),
which claims to provide a smooth handover from bootloader to kernel.
So it seems it would be better to fix the FIXME first, in a first patch.

Regards Denis

> 
> > >  	led_data->cdev.max_brightness = led->max_brightness;
> > >  	led_data->cdev.flags = LED_CORE_SUSPENDRESUME;
> > >  
...



Diehl Connectivity Solutions GmbH
Geschäftsführung: Horst Leonberger
Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
Nürnberg: HRB 32315
___________________________________________________________________________________________________

Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht.
Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
- Informationen zum Datenschutz, insbesondere zu Ihren Rechten, erhalten Sie unter https://www.diehl.com/group/de/transparenz-und-informationspflichten/

The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited. 
- For general information on data protection and your respective rights please visit https://www.diehl.com/group/en/transparency-and-information-obligations/

Re: [PATCH v2] leds: pwm: add support for default-state device property

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1687 bytes --]

Hi!

> This patch adds support for "default-state" devicetree property, which
> allows to defer pwm init to first use of led.
> 
> This allows to configure the PWM early in bootloader to let the LED
> blink until an application in Linux userspace set something different.

"sets".

> Signed-off-by: Denis Osterland-Heim <Denis.Osterland@diehl.com>

Looks good, I'll probably just apply it.

> index 6c6583c35f2f..d0f489680594 100644
> --- a/Documentation/devicetree/bindings/leds/leds-pwm.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
> @@ -19,6 +19,8 @@ LED sub-node properties:
>    see Documentation/devicetree/bindings/leds/common.txt
>  - linux,default-trigger :  (optional)
>    see Documentation/devicetree/bindings/leds/common.txt
> +- default-state : (optional)
> +  see Documentation/devicetree/bindings/leds/common.yaml
>  

Should other references be updated to common.yaml (as a separate patch)?

> diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
> index 93d101d28943..c9ef9012913d 100644
> --- a/include/linux/leds_pwm.h
> +++ b/include/linux/leds_pwm.h
> @@ -10,6 +10,7 @@ struct led_pwm {
>  	const char	*default_trigger;
>  	unsigned	pwm_id __deprecated;
>  	u8 		active_low;
> +	u8		default_state;
>  	unsigned 	max_brightness;
>  	unsigned	pwm_period_ns;
>  };

leds-pwm.c but leds_pwm.h. Hmm. This really should be leds-pwm.h.

Actually, leds-pwm.c is only user of leds_pwm.h, so that one should
just disappear...

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2] leds: pwm: add support for default-state device property

[Jacek Anaszewski]

Denis,

On 3/11/20 7:45 AM, Denis Osterland-Heim wrote:
> Hi Jacek,
> 
> Am Dienstag, den 10.03.2020, 22:15 +0100 schrieb Jacek Anaszewski:
>> Hi Denis,
>>
>> Thank you for the update. Please find my remarks below.
>>
>> On 3/10/20 4:19 PM, Denis Osterland-Heim wrote:
>>> Hi,
>>>
> ...
>>>> --- a/drivers/leds/leds-pwm.c
>>>> +++ b/drivers/leds/leds-pwm.c
>>>> @@ -75,7 +75,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>>>>  	led_data->active_low = led->active_low;
>>>>  	led_data->cdev.name = led->name;
>>>>  	led_data->cdev.default_trigger = led->default_trigger;
>>>> -	led_data->cdev.brightness = LED_OFF;
>>>> +	ret = led->default_state == LEDS_GPIO_DEFSTATE_ON;
>>
>> ret is for return value and it should not be used for anything
>> else just because it is at hand. Also LEDS_GPIO* definitions have
>> nothing to do with pwm leds. This is legacy because default-state
>> property was primarily specific to leds-gpio bindings and only
>> later was made common.
>>
>> Please introduce corresponding LEDS_PWM definitions, but in leds-pwm.c.
> will change
> 
>>
>>>> +	led_data->cdev.brightness = ret ? led->max_brightness : LED_OFF;
>>
>> Instead of above two changes I'd add below:
>>
>> if (led->default_state == LEDS_PWM_DEFSTATE_ON) {
>> 	led_data->cdev.brightness = led->max_brightness;
>> } else if (led->default_state == LEDS_PWM_DEFSTATE_KEEP)) {
>> 	// here put what you're adding below, but please use
>> 	// pwm_get_state() instead of accessing ops directly
>> }
>>
>> LED_OFF case is covered by kzalloc() in led_pwm_probe().
> Looks very elegant, I will apply this.
> pwm_get_state() is not sufficient here because it only returns the values from structure,
> which were not read read from registers at pwm_device_request().
> Something like pwm_get_state_uncached() would be required, which I have not found yet.
> 
> I looked at the atomic PWM API (5ec803edcb703fe379836f13560b79dfac79b01d),
> which claims to provide a smooth handover from bootloader to kernel.
> So it seems it would be better to fix the FIXME first, in a first patch.

I missed that it's been recently done. You've got to rebase onto Pavel's
for-next branch [0].

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/log/?h=for-next

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2] leds: pwm: add support for default-state device property

[Denis Osterland-Heim]

Hi Jacek,

thanks for the hint.
Perfekt! One thing less to do :D

Regards Denis

Am Mittwoch, den 11.03.2020, 23:33 +0100 schrieb Jacek Anaszewski:
> Denis,
> 
> On 3/11/20 7:45 AM, Denis Osterland-Heim wrote:
> > Hi Jacek,
> > 
> > Am Dienstag, den 10.03.2020, 22:15 +0100 schrieb Jacek Anaszewski:
> > > Hi Denis,
> > > 
> > > Thank you for the update. Please find my remarks below.
> > > 
> > > On 3/10/20 4:19 PM, Denis Osterland-Heim wrote:
> > > > Hi,
> > > > 
> > 
> > ...
> > > > > --- a/drivers/leds/leds-pwm.c
> > > > > +++ b/drivers/leds/leds-pwm.c
> > > > > @@ -75,7 +75,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
> > > > >  	led_data->active_low = led->active_low;
> > > > >  	led_data->cdev.name = led->name;
> > > > >  	led_data->cdev.default_trigger = led->default_trigger;
> > > > > -	led_data->cdev.brightness = LED_OFF;
> > > > > +	ret = led->default_state == LEDS_GPIO_DEFSTATE_ON;
> > > 
> > > ret is for return value and it should not be used for anything
> > > else just because it is at hand. Also LEDS_GPIO* definitions have
> > > nothing to do with pwm leds. This is legacy because default-state
> > > property was primarily specific to leds-gpio bindings and only
> > > later was made common.
> > > 
> > > Please introduce corresponding LEDS_PWM definitions, but in leds-pwm.c.
> > 
> > will change
> > 
> > > 
> > > > > +	led_data->cdev.brightness = ret ? led->max_brightness : LED_OFF;
> > > 
> > > Instead of above two changes I'd add below:
> > > 
> > > if (led->default_state == LEDS_PWM_DEFSTATE_ON) {
> > > 	led_data->cdev.brightness = led->max_brightness;
> > > } else if (led->default_state == LEDS_PWM_DEFSTATE_KEEP)) {
> > > 	// here put what you're adding below, but please use
> > > 	// pwm_get_state() instead of accessing ops directly
> > > }
> > > 
> > > LED_OFF case is covered by kzalloc() in led_pwm_probe().
> > 
> > Looks very elegant, I will apply this.
> > pwm_get_state() is not sufficient here because it only returns the values from structure,
> > which were not read read from registers at pwm_device_request().
> > Something like pwm_get_state_uncached() would be required, which I have not found yet.
> > 
> > I looked at the atomic PWM API (5ec803edcb703fe379836f13560b79dfac79b01d),
> > which claims to provide a smooth handover from bootloader to kernel.
> > So it seems it would be better to fix the FIXME first, in a first patch.
> 
> I missed that it's been recently done. You've got to rebase onto Pavel's
> for-next branch [0].
> 
> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/log/?h=for-next
> 


Diehl Connectivity Solutions GmbH
Geschäftsführung: Horst Leonberger
Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
Nürnberg: HRB 32315
___________________________________________________________________________________________________

Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht.
Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
- Informationen zum Datenschutz, insbesondere zu Ihren Rechten, erhalten Sie unter https://www.diehl.com/group/de/transparenz-und-informationspflichten/

The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited. 
- For general information on data protection and your respective rights please visit https://www.diehl.com/group/en/transparency-and-information-obligations/

Re: [PATCH v2] leds: pwm: add support for default-state device property

[Denis Osterland-Heim]

Hi Pavel,

Am Mittwoch, den 11.03.2020, 22:33 +0100 schrieb Pavel Machek:
> Hi!
> 
> > This patch adds support for "default-state" devicetree property, which
> > allows to defer pwm init to first use of led.
> > 
> > This allows to configure the PWM early in bootloader to let the LED
> > blink until an application in Linux userspace set something different.
> 
> "sets".
done

> 
> > Signed-off-by: Denis Osterland-Heim <Denis.Osterland@diehl.com>
> 
> Looks good, I'll probably just apply it.
I will rebase on your next branch, so that it uses atomic PWM API, soon.

> 
> > index 6c6583c35f2f..d0f489680594 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-pwm.txt
> > +++ b/Documentation/devicetree/bindings/leds/leds-pwm.txt
> > @@ -19,6 +19,8 @@ LED sub-node properties:
> >    see Documentation/devicetree/bindings/leds/common.txt
> >  - linux,default-trigger :  (optional)
> >    see Documentation/devicetree/bindings/leds/common.txt
> > +- default-state : (optional)
> > +  see Documentation/devicetree/bindings/leds/common.yaml
> >  
> 
> Should other references be updated to common.yaml (as a separate patch)?
well, the whole txt file should be converted to yaml...
currently common.txt exists and points to common.yaml, so no urgent need

> 
> > diff --git a/include/linux/leds_pwm.h b/include/linux/leds_pwm.h
> > index 93d101d28943..c9ef9012913d 100644
> > --- a/include/linux/leds_pwm.h
> > +++ b/include/linux/leds_pwm.h
> > @@ -10,6 +10,7 @@ struct led_pwm {
> >  	const char	*default_trigger;
> >  	unsigned	pwm_id __deprecated;
> >  	u8 		active_low;
> > +	u8		default_state;
> >  	unsigned 	max_brightness;
> >  	unsigned	pwm_period_ns;
> >  };
> 
> leds-pwm.c but leds_pwm.h. Hmm. This really should be leds-pwm.h.
> 
> Actually, leds-pwm.c is only user of leds_pwm.h, so that one should
> just disappear...
I can move it in a second patch, if you want

Regards Denis
> 
> Best regards,
> 									Pavel
> 
> +----------------------------------------------------------------------+
> > Z1 SecureMail Gateway Processing Info                                |
> 
> +----------------------------------------------------------------------+
> > - The message was signed by                                          |
> >   [No Info available]                                                |
> >   Signature not verifiable                                           |
> >   - Message content not verifiable                                   |
> >   - Certificate not verifiable                                       |
> 
> +----------------------------------------------------------------------+


Diehl Connectivity Solutions GmbH
Geschäftsführung: Horst Leonberger
Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
Nürnberg: HRB 32315
___________________________________________________________________________________________________

Der Inhalt der vorstehenden E-Mail ist nicht rechtlich bindend. Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen.
Informieren Sie uns bitte, wenn Sie diese E-Mail faelschlicherweise erhalten haben. Bitte loeschen Sie in diesem Fall die Nachricht.
Jede unerlaubte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt.
- Informationen zum Datenschutz, insbesondere zu Ihren Rechten, erhalten Sie unter https://www.diehl.com/group/de/transparenz-und-informationspflichten/

The contents of the above mentioned e-mail is not legally binding. This e-mail contains confidential and/or legally protected information. Please inform us if you have received this e-mail by
mistake and delete it in such a case. Each unauthorized reproduction, disclosure, alteration, distribution and/or publication of this e-mail is strictly prohibited. 
- For general information on data protection and your respective rights please visit https://www.diehl.com/group/en/transparency-and-information-obligations/