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

[Pavel Machek <pavel@ucw.cz>]

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


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

> @@ -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.

> 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.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]