Messages in this thread | | | Date | Tue, 18 Mar 2014 16:28:56 +0100 | From | Thierry Reding <> | 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@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 [unhandled content-type:application/pgp-signature] | |