On Mon, Oct 22, 2012 at 04:36:41PM +0530, Shiraz Hashim wrote:
[...]
> +struct spear_pwm_chip {
> + void __iomem *mmio_base;
> + struct clk *clk;
> + struct pwm_chip chip;
My editor shows a tab between pwm_chip and chip. This should really be a
space.
> + ret = pwmchip_add(&pc->chip);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(pc->clk);
> + if (ret < 0)
> + return pwmchip_remove(&pc->chip);
I think in order to fix the potential race condition that Viresh
mentioned we should move the clk_prepare_enable() before the
pwmchip_add(), but don't forget to disable and unprepare the clock if
pwmchip_add() fails.
Actually, can't we make it a clk_prepare() only at this point and move
the clk_enable() and clk_disable() into the if block below? In case the
compatible value is not "st,spear1340-pwm" we don't need the clock
enabled.
> +
> + if (of_device_is_compatible(np, "st,spear1340-pwm")) {
> + /*
> + * Following enables PWM chip, channels would still be
> + * enabled individually through their control register
> + */
> + val = readl_relaxed(pc->mmio_base + PWMMCR);
> + val |= PWMMCR_PWM_ENABLE;
> + writel_relaxed(val, pc->mmio_base + PWMMCR);
> +
Oh, and a spurious newline here... =)
> + }
> +
> + /* only disable the clk and leave it prepared */
> + clk_disable(pc->clk);
This can go into the if block to match the clk_enable().
Thierry
pgpaMIyao45I9.pgp
Description: PGP signature
