Re: [PATCH v6 1/2] leds: pwm: add support for default-state device property

[Denis Osterland-Heim <denis.osterland@diehl.com>]

Hi Pavel,

Am Mittwoch, den 22.07.2020, 16:04 +0200 schrieb Pavel Machek:
> > 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 sets something different.
> > 
> > Signed-off-by: Denis Osterland-Heim <Denis.Osterland@diehl.com>
> > Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > +#define LEDS_PWM_DEFSTATE_OFF	0
> > +#define LEDS_PWM_DEFSTATE_ON	1
> > +#define LEDS_PWM_DEFSTATE_KEEP	2
> 
> Turn this into enum; no need for prefix as this is private to the driver.
> 
> >  struct led_pwm {
> >  	const char	*name;
> >  	const char	*default_trigger;
> >  	u8		active_low;
> > +	u8		default_state;
> >  	unsigned int	max_brightness;
> >  };
> >  
> > @@ -88,7 +93,30 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
> >  
> >  	led_data->cdev.brightness_set_blocking = led_pwm_set;
> >  
> > -	pwm_init_state(led_data->pwm, &led_data->pwmstate);
> > +	/* init PWM state */
> > +	if (led->default_state == LEDS_PWM_DEFSTATE_KEEP) {
> > +		pwm_get_state(led_data->pwm, &led_data->pwmstate);
> > +		if (!led_data->pwmstate.period) {
> > +			led->default_state = LEDS_PWM_DEFSTATE_OFF;
> > +			dev_warn(dev,
> > +				"failed to read period for %s, default to off",
> > +				led->name);
> > +		}
> > +	}
> > +	if (led->default_state != LEDS_PWM_DEFSTATE_KEEP)
> > +		pwm_init_state(led_data->pwm, &led_data->pwmstate);
> > +
> > +	/* set brightness */
> > +	if (led->default_state == LEDS_PWM_DEFSTATE_ON)
> > +		led_data->cdev.brightness = led->max_brightness;
> > +	else if (led->default_state == LEDS_PWM_DEFSTATE_KEEP) {
> > +		uint64_t brightness;
> > +
> > +		brightness = led->max_brightness;
> > +		brightness *= led_data->pwmstate.duty_cycle;
> > +		do_div(brightness, led_data->pwmstate.period);
> > +		led_data->cdev.brightness = brightness;
> > +	}
> 
> Try to clean this up... switch() might help. Maybe two of them.
looks possible, lets see if it improves readability

> 
> > @@ -134,6 +166,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_PWM_DEFSTATE_KEEP;
> > +			else if (!strcmp(state, "on"))
> > +				led.default_state = LEDS_PWM_DEFSTATE_ON;
> > +			else
> > +				led.default_state = LEDS_PWM_DEFSTATE_OFF;
> > +		}
> 
> Actually... Move the enum to core, and add helper for this. We don't
> want to see this duplicated.
I think I should put the refactoring into a separate patch.

This code duplicates leds-gpio, which uses the LEDS_GPIO_DEFSTATE_* defines,
defined in include/linux/leds.h.
The I idea was to let gpio its defines and add dt compatible defines for leds-pwm.
But it would be useful to have a common function, yes.

It looks like a enum leds_default_state with the define for leds-gpio changed
to the new enum in linux/leds.h and a new function declaration leds_defstate_get()
in leds/leds.h, with the implementation led-core.c. And leds-gpio and leds-pwm
will then include leds/leds.h to use leds_defstate_get().

> 
> > 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/
> 
> Get rid of this.
my force is not strong enough...

Regards, Denis

> 									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/