RE: [PATCH v3 1/2] pwm: Add PWM driver for Intel Keem Bay

-----Original Message-----
From: Shevchenko, Andriy <redacted>
Sent: Friday, August 14, 2020 3:24 AM
To: G Jaya Kumaran, Vineetha <redacted>
Cc: thierry.reding@gmail.com; u.kleine-koenig@pengutronix.de;
robh+dt@kernel.org; linux-pwm@vger.kernel.org;
devicetree@vger.kernel.org; Wan Mohamad, Wan Ahmad Zainie
[off-list ref]; Raja Subramanian, Lakshmi
Bai [off-list ref]
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@intel.com wrote:

quoted

From: "Lai, Poey Seng" <redacted>

Enable PWM support for the Intel Keem Bay SoC.

...

quoted

+static inline void keembay_pwm_update_bits(struct keembay_pwm

*priv, u32 mask,

quoted

+					   u32 val, u32 offset)
+{
+	u32 buff, tmp;

quoted

+	void __iomem *address;

No need to have this. Just use calc in place.

OK, will remove this.

quoted

+	address = priv->base + offset;

quoted

+	buff = readl(address);

quoted

+	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.

quoted

+	writel(tmp, address);
+}

...

quoted

+	 * high time = clock rate * duty cycle / NSEC_PER_SEC
+	 * low time =  clock rate * (period - duty cycle) / NSEC_PER_SEC

quoted

+	 * 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.

quoted

+	 * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710

...

quoted

+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk)) {

quoted

+		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(...);

quoted

+	}

...

quoted

+	ret = pwmchip_add(&priv->chip);
+	if (ret < 0) {

' < 0' makes any sense?

OK, will change this to just use 'if (ret)' instead.

quoted

+		dev_err(dev, "Failed to add PWM chip: %pe\n",

ERR_PTR(ret));

quoted

+		return ret;
+	}

--
With Best Regards,
Andy Shevchenko

Thanks for the comments, Andy - will make the changes and resubmit for V4.