> -----Original Message----- > From: linux-pwm-owner@xxxxxxxxxxxxxxx <linux-pwm- > owner@xxxxxxxxxxxxxxx> On Behalf Of Andy Shevchenko > Sent: Monday, August 24, 2020 3:53 PM > To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@xxxxxxxxx> > Cc: thierry.reding@xxxxxxxxx; u.kleine-koenig@xxxxxxxxxxxxxx; > robh+dt@xxxxxxxxxx; linux-pwm@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; Wan Mohamad, Wan Ahmad Zainie > <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>; Raja Subramanian, Lakshmi > Bai <lakshmi.bai.raja.subramanian@xxxxxxxxx> > Subject: Re: [PATCH v4 1/2] pwm: Add PWM driver for Intel Keem Bay > > On Sat, Aug 22, 2020 at 10:30:45PM +0800, > vineetha.g.jaya.kumaran@xxxxxxxxx wrote: > > From: "Lai, Poey Seng" <poey.seng.lai@xxxxxxxxx> > > > > Side note, please use my @linux.intel.com address in the Cc list. > > > Enable PWM support for the Intel Keem Bay SoC. > > ... > > > + * Authors: Lai Poey Seng <poey.seng.lai@xxxxxxxxx> > > + * Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@xxxxxxxxx> > > Seems you missed Co-developed-by in the tag block of commit message. > OK, I will add this in. > ... > > > +#include <linux/bitfield.h> > > +#include <linux/clk.h> > > +#include <linux/io.h> > > +#include <linux/module.h> > > > +#include <linux/of.h> > > There is no user of this header. But mod_devicetable.h is missing. > OK, will remove this, and add in mod_devicetable.h > > +#include <linux/platform_device.h> > > +#include <linux/pwm.h> > > +#include <linux/regmap.h> > > ... > > > + /* > > + * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register > contain > > + * the high time of the waveform, while the last 16 bits contain > > + * the low time of the waveform, in terms of clock cycles. > > + * > > + * high time = clock rate * duty cycle / NSEC_PER_SEC > > + * low time = clock rate * (period - duty cycle) / NSEC_PER_SEC > > + * > > + * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz: > > > + * high time = (500MHz * 30us) / 1000000000 = 0x3A98 > > 10^9 divisor now is redundant. It's school physics about units. > > > + * low time = (500MHz * 20us) / 1000000000 = 0x2710 > > Ditto. > Thank you for pointing this out - will fix this. > > + * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710 > > + */ > > Note, in the code you are operating different units. > > ... > > > + div = clk_rate * state->duty_cycle; > > + div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC); > > For example, here you have clock rate in Hz, duty cycle in ns, that's why you > need to use NSEC_PER_SEC divisor here. > > ... > > > + div = clk_rate * (state->period - state->duty_cycle); > > + div = DIV_ROUND_CLOSEST_ULL(div, NSEC_PER_SEC); > > Same here. > > (Just to be clear, code is okay!) > > ... > > > + priv->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(priv->clk)) > > + dev_err_probe(dev, PTR_ERR(priv->clk), > > + "Failed to get clock\n"); > > First of all, it can be one line, second, it misses something... > Missed the return - sorry, will fix it. Thanks for the feedback, Andy. Will make the changes and resubmit. > -- > With Best Regards, > Andy Shevchenko >
