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@xxxxxxxxx>
> instead of
> Reply-To: <20200309082218.13263-1-Denis.Osterland@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
>> ---
>> 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