linux-kernel - Re: [PATCH V4] PWM: Add SPEAr PWM chip driver support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121024055137.GC9787@avionic-0098.mockup.avionic-design.de>
Date:	Wed, 24 Oct 2012 07:51:37 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Shiraz Hashim <shiraz.hashim@...com>
Cc:	linux-kernel@...r.kernel.org, spear-devel@...t.st.com,
	Viresh Kumar <viresh.kumar@...aro.org>
Subject: Re: [PATCH V4] PWM: Add SPEAr PWM chip driver support

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

Content of type "application/pgp-signature" skipped