linux-kernel - Re: [PATCH] leds-lm3530: replace pwm platform functions with generic pwm functions

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: <20120820050452.GA16729@avionic-0098.adnet.avionic-design.de>
Date:	Mon, 20 Aug 2012 07:04:52 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	"Kim, Milo" <Milo.Kim@...com>
Cc:	Bryan Wu <bryan.wu@...onical.com>, "arnd@...db.de" <arnd@...db.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] leds-lm3530: replace pwm platform functions with generic
 pwm functions

On Mon, Aug 20, 2012 at 04:02:05AM +0000, Kim, Milo wrote:
> use generic pwm functions for changing the duty rather than the platform data

pwm -> PWM, duty -> duty-cycle

There are more occurrences of "pwm"scattered through the file, please
fix those as well.

> @@ -153,6 +158,64 @@ static int lm3530_get_mode_from_str(const char *str)
>  	return -1;
>  }
>  
> +#ifdef CONFIG_PWM
> +static int lm3530_pwm_request(struct lm3530_data *drvdata)
> +{
> +	int pwm_id;
> +
> +	/* if the pwm device exists, skip requesting the device */
> +	if (drvdata->pwm)
> +		return 0;
> +
> +	pwm_id = drvdata->pdata ? drvdata->pdata->pwm_id : 0;
> +
> +	drvdata->pwm = pwm_request(pwm_id, "lm3530-pwm");

Please don't use pwm_id and pwm_request() for new code. You should be
using pwm_get() along with a corresponding PWM_LOOKUP() entry.

> +	drvdata->period_ns = drvdata->pdata ? drvdata->pdata->period_ns : 0;
> +
> +	return IS_ERR(drvdata->pwm) ? PTR_ERR(drvdata->pwm) : 0;

This is "PTR_RET(drvdata->pwm)".

> +}
> +
> +static void lm3530_pwm_set(struct lm3530_data *drvdata,
> +			int brightness, int max_brightness)
> +{
> +	struct pwm_device *pwm = drvdata->pwm;
> +
> +	if (pwm) {

Maybe this check should be:

	if (pwm && drvdata->period_ns > 0)

> @@ -363,9 +418,15 @@ static ssize_t lm3530_mode_set(struct device *dev, struct device_attribute
>  
>  	drvdata->mode = mode;
>  
> -	/* set pwm to low if unnecessary */
> -	if (mode != LM3530_BL_MODE_PWM && pwm->pwm_set_intensity)
> -		pwm->pwm_set_intensity(0, max_brightness);
> +	/* if new mode is pwm, then request a pwm device.
> +	   otherwise, set pwm to low */
> +	if (mode == LM3530_BL_MODE_PWM) {
> +		err = lm3530_pwm_request(drvdata);
> +		if (err)
> +			return err;
> +	} else {
> +		lm3530_pwm_set(drvdata, 0, max_brightness);
> +	}

Don't you need to free the PWM if mode is set to !LM3530_BL_MODE_PWM?

Thierry

Content of type "application/pgp-signature" skipped