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

[Thierry Reding <thierry.reding@gmail.com>]

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

On Fri, Feb 28, 2014 at 10:50:57PM +0800, Chew Chiau Ee wrote:
> From: Mika Westerberg <mika.westerberg@linux.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@linux.intel.com>
> Signed-off-by: Chew, Kean Ho <kean.ho.chew@intel.com>
> Signed-off-by: Chang, Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com>
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.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@linux.intel.com>");
> +MODULE_LICENSE("GPLv2");

I think this needs to be "GPL v2".

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]