On Fri, Feb 28, 2014 at 10:50:57PM +0800, Chew Chiau Ee wrote: > From: Mika Westerberg <[email protected]> > > Add support for Intel Low Power I/O subsystem PWM controllers found on > Intel BayTrail SoC. > > Signed-off-by: Mika Westerberg <[email protected]> > Signed-off-by: Chew, Kean Ho <[email protected]> > Signed-off-by: Chang, Rebecca Swee Fun <[email protected]> > Signed-off-by: Chew, Chiau Ee <[email protected]> > --- > 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 <[email protected]>");
> +MODULE_LICENSE("GPLv2");
I think this needs to be "GPL v2".
Thierry
pgpowIBd4MJFS.pgp
Description: PGP signature
