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.
> + 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?
> + 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.
> + * 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?
> + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret));
> + return ret;
> + }
--
With Best Regards,
Andy Shevchenko
