linux-kernel - Re: [PATCH 1/2] lp855x_bl: use 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 for Android: free password hash cracker in your pocket

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

Message-ID: <20121019082107.GA20659@avionic-0098.mockup.avionic-design.de>
Date:	Fri, 19 Oct 2012 10:21:07 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	"Kim, Milo" <Milo.Kim@...com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Richard Purdie <rpurdie@...ys.net>,
	Bryan Wu <bryan.wu@...onical.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] lp855x_bl: use generic PWM functions

On Fri, Oct 19, 2012 at 08:11:50AM +0000, Kim, Milo wrote:
>  The LP855x family devices support the PWM input for the backlight control.
>  Period of the PWM is configurable in the platform side.
>  Platform specific functions are unnecessary anymore because
>  generic PWM functions are used inside the driver.
> 
>  (PWM input mode)
>  To set the brightness, new lp855x_pwm_ctrl() is used.
>  If a PWM device is not allocated, devm_pwm_get() is called.
>  The PWM consumer name is from the chip name such as 'lp8550' and 'lp8556'.
>  To get the brightness value, no additional handling is required.
>  Just the value of 'props.brightness' is returned.
> 
>  If the PWM driver is not ready while initializing the LP855x driver, it's OK.
>  The PWM device can be retrieved later, when the brightness value is changed.
> 
>  Documentation is updated with an example.
> 
> Signed-off-by: Milo(Woogyom) Kim <milo.kim@...com>

Generally this looks good. Obviously you'll need to update any users of
this driver as well. It might make sense to include those changes in
this patch to avoid interim build failures.

Other than that I have just one smaller comment below.

> @@ -121,6 +123,25 @@ static int lp855x_init_registers(struct lp855x *lp)
>  	return ret;
>  }
>  
> +static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
> +{
> +	unsigned int period = lp->pdata->period_ns;
> +	unsigned int duty = br * period / max_br;
> +	struct pwm_device *pwm;
> +
> +	/* request pwm device with the consumer name */
> +	if (!lp->pwm) {
> +		pwm = devm_pwm_get(lp->dev, lp->chipname);
> +		if (IS_ERR(pwm))
> +			return;
> +
> +		lp->pwm = pwm;
> +	}
> +
> +	pwm_config(lp->pwm, duty, period);
> +	duty == 0 ? pwm_disable(lp->pwm) : pwm_enable(lp->pwm);

This is really ugly and should be written explicitly:

	if (duty == 0)
		pwm_disable(lp->pwm);
	else
		pwm_enable(lp->pwm);

Thierry

Content of type "application/pgp-signature" skipped