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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200722140443.6cagitx3dozgjazh@duo.ucw.cz>
Date:   Wed, 22 Jul 2020 16:04:43 +0200
From:   Pavel Machek <pavel@....cz>
To:     Denis Osterland-Heim <denis.osterland@...hl.com>
Cc:     "dmurphy@...com" <dmurphy@...com>,
        "jacek.anaszewski@...il.com" <jacek.anaszewski@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v6 1/2] leds: pwm: add support for default-state device
 property


> 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@...hl.com>
> Acked-by: Jacek Anaszewski <jacek.anaszewski@...il.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

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)