LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] leds: pwm: add support for default-state device property
@ 2020-03-10 12:47 Denis Osterland-Heim
  2020-03-10 15:19 ` Denis Osterland-Heim
  2020-03-11 21:33 ` Pavel Machek
  0 siblings, 2 replies; 8+ messages in thread
From: Denis Osterland-Heim @ 2020-03-10 12:47 UTC (permalink / raw)
  To: dmurphy, pavel, mark.rutland, jacek.anaszewski, robh+dt
  Cc: linux-kernel, Denis Osterland-Heim, linux-leds, devicetree

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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] leds: pwm: add support for default-state device property
  2020-03-10 12:47 [PATCH v2] leds: pwm: add support for default-state device property Denis Osterland-Heim
@ 2020-03-10 15:19 ` Denis Osterland-Heim
  2020-03-10 21:15   ` Jacek Anaszewski
  2020-03-11 21:33 ` Pavel Machek
  1 sibling, 1 reply; 8+ messages in thread
From: Denis Osterland-Heim @ 2020-03-10 15:19 UTC (permalink / raw)
  To: dmurphy, pavel, mark.rutland, jacek.anaszewski, robh+dt
  Cc: linux-kernel, linux-leds, devicetree

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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] leds: pwm: add support for default-state device property
  2020-03-10 15:19 ` Denis Osterland-Heim
@ 2020-03-10 21:15   ` Jacek Anaszewski
  2020-03-11  6:45     ` Denis Osterland-Heim
  0 siblings, 1 reply; 8+ messages in thread
From: Jacek Anaszewski @ 2020-03-10 21:15 UTC (permalink / raw)
  To: Denis Osterland-Heim, dmurphy, pavel, mark.rutland, robh+dt
  Cc: linux-kernel, linux-leds, devicetree

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] leds: pwm: add support for default-state device property
  2020-03-10 21:15   ` Jacek Anaszewski
@ 2020-03-11  6:45     ` Denis Osterland-Heim
  2020-03-11 22:33       ` Jacek Anaszewski
  0 siblings, 1 reply; 8+ messages in thread
From: Denis Osterland-Heim @ 2020-03-11  6:45 UTC (permalink / raw)
  To: dmurphy, pavel, mark.rutland, jacek.anaszewski, robh+dt
  Cc: linux-kernel, linux-leds, devicetree

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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] leds: pwm: add support for default-state device property
  2020-03-10 12:47 [PATCH v2] leds: pwm: add support for default-state device property Denis Osterland-Heim
  2020-03-10 15:19 ` Denis Osterland-Heim
@ 2020-03-11 21:33 ` Pavel Machek
  2020-03-12  7:35   ` Denis Osterland-Heim
  1 sibling, 1 reply; 8+ messages in thread
From: Pavel Machek @ 2020-03-11 21:33 UTC (permalink / raw)
  To: 20200309082218.13263-1-Denis.Osterland
  Cc: dmurphy, mark.rutland, jacek.anaszewski, robh+dt, linux-kernel,
	Denis Osterland-Heim, linux-leds, devicetree

[-- 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 --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] leds: pwm: add support for default-state device property
  2020-03-11  6:45     ` Denis Osterland-Heim
@ 2020-03-11 22:33       ` Jacek Anaszewski
  2020-03-12  7:25         ` Denis Osterland-Heim
  0 siblings, 1 reply; 8+ messages in thread
From: Jacek Anaszewski @ 2020-03-11 22:33 UTC (permalink / raw)
  To: Denis Osterland-Heim, dmurphy, pavel, mark.rutland, robh+dt
  Cc: linux-kernel, linux-leds, devicetree

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] leds: pwm: add support for default-state device property
  2020-03-11 22:33       ` Jacek Anaszewski
@ 2020-03-12  7:25         ` Denis Osterland-Heim
  0 siblings, 0 replies; 8+ messages in thread
From: Denis Osterland-Heim @ 2020-03-12  7:25 UTC (permalink / raw)
  To: dmurphy, pavel, mark.rutland, jacek.anaszewski, robh+dt
  Cc: linux-kernel, linux-leds, devicetree

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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] leds: pwm: add support for default-state device property
  2020-03-11 21:33 ` Pavel Machek
@ 2020-03-12  7:35   ` Denis Osterland-Heim
  0 siblings, 0 replies; 8+ messages in thread
From: Denis Osterland-Heim @ 2020-03-12  7:35 UTC (permalink / raw)
  To: pavel
  Cc: devicetree, dmurphy, mark.rutland, linux-kernel,
	jacek.anaszewski, robh+dt, linux-leds

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/

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-03-12  7:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 12:47 [PATCH v2] leds: pwm: add support for default-state device property Denis Osterland-Heim
2020-03-10 15:19 ` Denis Osterland-Heim
2020-03-10 21:15   ` Jacek Anaszewski
2020-03-11  6:45     ` Denis Osterland-Heim
2020-03-11 22:33       ` Jacek Anaszewski
2020-03-12  7:25         ` Denis Osterland-Heim
2020-03-11 21:33 ` Pavel Machek
2020-03-12  7:35   ` Denis Osterland-Heim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).