linux-kernel - Re: [PATCH v2] pwm: add support for Intel Low Power Subsystem PWM

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: <20140318152855.GA7237@mithrandir>
Date:	Tue, 18 Mar 2014 16:28:56 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Chew Chiau Ee <chiau.ee.chew@...el.com>
Cc:	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Chew Kean Ho <kean.ho.chew@...el.com>,
	Chang Rebecca Swee Fun <rebecca.swee.fun.chang@...el.com>,
	linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] pwm: add support for Intel Low Power Subsystem PWM

On Fri, Feb 28, 2014 at 10:50:57PM +0800, Chew Chiau Ee wrote:
> From: Mika Westerberg <mika.westerberg@...ux.intel.com>
> 
> Add support for Intel Low Power I/O subsystem PWM controllers found on
> Intel BayTrail SoC.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> Signed-off-by: Chew, Kean Ho <kean.ho.chew@...el.com>
> Signed-off-by: Chang, Rebecca Swee Fun <rebecca.swee.fun.chang@...el.com>
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@...el.com>
> ---
>  drivers/pwm/Kconfig    |   10 +++
>  drivers/pwm/Makefile   |    1 +
>  drivers/pwm/pwm-lpss.c |  179 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 190 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/pwm/pwm-lpss.c

Sorry for taking so long to get back to you on this. Things have been
somewhat crazy lately.

This generally looks very good, but I found two issues that escaped last
time. See below.

> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
[...]
> +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   int duty_ns, int period_ns)
> +{
> +	struct pwm_lpss_chip *lpwm = to_lpwm(chip);
> +	u8 on_time_div;
> +	unsigned long c = clk_get_rate(lpwm->clk);
> +	unsigned long long base_unit, freq = NSECS_PER_SEC;
> +	u32 ctrl;
> +
> +	do_div(freq, period_ns);
> +
> +	/* The equation is: base_unit = ((freq / c) * 65536) + correction */
> +	base_unit = freq * 65536;
> +	do_div(base_unit, c);

clk_get_rate() can return 0, so perhaps it would be safer to check for
the validity of c before dividing by it. This will probably never
happen for your driver or platform, but people may look at your driver
for inspiration at some point, so it should still be handling this
correctly.

> +MODULE_DESCRIPTION("PWM driver for Intel LPSS");
> +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@...ux.intel.com>");
> +MODULE_LICENSE("GPLv2");

I think this needs to be "GPL v2".

Thierry

Content of type "application/pgp-signature" skipped