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. ... > +#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. > +#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. > + * 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... -- With Best Regards, Andy Shevchenko
