> -----Original Message-----
> From: Shevchenko, Andriy <andriy.shevchenko@xxxxxxxxx>
> Sent: Friday, August 14, 2020 3:24 AM
> 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 v3 1/2] pwm: Add PWM driver for Intel Keem Bay
>
> On Fri, Aug 14, 2020 at 12:04:05AM +0800,
> vineetha.g.jaya.kumaran@xxxxxxxxx wrote:
> > From: "Lai, Poey Seng" <poey.seng.lai@xxxxxxxxx>
> >
> > Enable PWM support for the Intel Keem Bay SoC.
>
> ...
>
> > +static inline void keembay_pwm_update_bits(struct keembay_pwm
> *priv, u32 mask,
> > + u32 val, u32 offset)
> > +{
> > + u32 buff, tmp;
>
> > + void __iomem *address;
>
> No need to have this. Just use calc in place.
>
OK, will remove this.
> > + address = priv->base + offset;
>
> > + buff = readl(address);
>
> > + tmp = buff & ~mask;
> > + tmp |= FIELD_PREP(mask, val);
>
> One line and one variable less:
>
> buff = (buff & ~mask) | FIELD_PREP(...);
>
> But shouldn't be u32_replace_bits() instead?
>
Thanks, I will try to switch to this instead.
> > + writel(tmp, address);
> > +}
>
> ...
>
> > + * high time = clock rate * duty cycle / NSEC_PER_SEC
> > + * low time = clock rate * (period - duty cycle) / NSEC_PER_SEC
>
> > + * e.g. For period 50000ns, duty cycle 30000ns, and clock rate 500MHz:
> > + * high time = (500000000 * 30000) / 1000000000 = 0x3A98
> > + * low time = (500000000 * 20000) / 1000000000 = 0x2710
>
> Please, replace all multipliers to physical units
> ... 50us ... 30us ...
> ... 500MHz * 30us = 0x3a98
> ...and so on.
>
Noted, will fix this in V4.
> > + * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710
>
> ...
>
> > + priv->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(priv->clk)) {
>
> > + if (PTR_ERR(priv->clk) != -EPROBE_DEFER)
> > + dev_err(dev, "Failed to get clock: %pe", priv->clk);
> > +
> > + return PTR_ERR(priv->clk);
>
> return dev_err_probe(...);
>
> > + }
>
> ...
>
> > + ret = pwmchip_add(&priv->chip);
> > + if (ret < 0) {
>
> ' < 0' makes any sense?
>
OK, will change this to just use 'if (ret)' instead.
> > + dev_err(dev, "Failed to add PWM chip: %pe\n",
> ERR_PTR(ret));
> > + return ret;
> > + }
>
> --
> With Best Regards,
> Andy Shevchenko
>
Thanks for the comments, Andy - will make the changes and resubmit for V4.
