diff mbox series

[v3,6/7] pwm: rockchip: Enable PWM clock of probed device only if running

Message ID cb5a28d5fde04c09e9aace6ce619e445f895e0a8.1608735481.git.simon@simonsouth.net (mailing list archive)
State New
Headers show
Series pwm: rockchip: Eliminate potential race condition when probing | expand

Commit Message

Simon South Dec. 23, 2020, 4:01 p.m. UTC
Currently rockchip_pwm_probe() enables the PWM clock of every device it
matches, then disables the clock unless the device itself appears to have
been enabled (by a bootloader, presumably) before the kernel started.

Simplify this by enabling (rather, keeping enabled) the PWM clock of only
devices that are already running, as enabling the clock for any other
device during probing is unnecessary.

Signed-off-by: Simon South <simon@simonsouth.net>
---
 drivers/pwm/pwm-rockchip.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

Comments

Uwe Kleine-König Jan. 13, 2021, 7:50 a.m. UTC | #1
On Wed, Dec 23, 2020 at 11:01:08AM -0500, Simon South wrote:
> Currently rockchip_pwm_probe() enables the PWM clock of every device it
> matches, then disables the clock unless the device itself appears to have
> been enabled (by a bootloader, presumably) before the kernel started.
> 
> Simplify this by enabling (rather, keeping enabled) the PWM clock of only
> devices that are already running, as enabling the clock for any other
> device during probing is unnecessary.
> 
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index 80f5e69d9b8a..02da7370db70 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -327,12 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ret = clk_prepare_enable(pc->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't prepare enable PWM clk: %d\n", ret);
> -		return ret;
> -	}
> -

This undoes stuff that you introduced in patch 1. Don't you reintroduce
the problem that was fixed by this patch?

>  	ret = clk_prepare_enable(pc->pclk);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
> @@ -357,8 +351,19 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  	enable_conf = pc->data->enable_conf;
>  	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
>  	enabled = ((ctrl & enable_conf) == enable_conf);
> -	if (!enabled)
> -		clk_disable(pc->clk);
> +	if (enabled) {
> +		ret = clk_prepare_enable(pc->clk);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Can't enable PWM clk: %d\n", ret);
> +			return ret;
> +		}
> +	} else {
> +		ret = clk_prepare(pc->clk);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Can't prepare PWM clk: %d\n", ret);
> +			return ret;
> +		}
> +	}

I don't see the advantage of this patch. In my eyes the code
complication doesn't justify the gain (i.e. prevent the PWM clock being
enabled for a few instructions).

Best regards
Uwe
Simon South Jan. 14, 2021, 3:22 p.m. UTC | #2
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> I don't see the advantage of this patch. In my eyes the code
> complication doesn't justify the gain (i.e. prevent the PWM clock
> being enabled for a few instructions).

I was starting to feel the same way after I sent out this series, so
let's just drop this change.

Thanks for the feedback, Uwe. I'll get a v4 out shortly.
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 80f5e69d9b8a..02da7370db70 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -327,12 +327,6 @@  static int rockchip_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = clk_prepare_enable(pc->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "Can't prepare enable PWM clk: %d\n", ret);
-		return ret;
-	}
-
 	ret = clk_prepare_enable(pc->pclk);
 	if (ret) {
 		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
@@ -357,8 +351,19 @@  static int rockchip_pwm_probe(struct platform_device *pdev)
 	enable_conf = pc->data->enable_conf;
 	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
 	enabled = ((ctrl & enable_conf) == enable_conf);
-	if (!enabled)
-		clk_disable(pc->clk);
+	if (enabled) {
+		ret = clk_prepare_enable(pc->clk);
+		if (ret) {
+			dev_err(&pdev->dev, "Can't enable PWM clk: %d\n", ret);
+			return ret;
+		}
+	} else {
+		ret = clk_prepare(pc->clk);
+		if (ret) {
+			dev_err(&pdev->dev, "Can't prepare PWM clk: %d\n", ret);
+			return ret;
+		}
+	}
 
 	clk_disable(pc->pclk);