Re: [PATCH v2 3/3] pwm: rockchip: Do not start PWMs not already running

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3085 bytes --]

On Sat, Dec 19, 2020 at 03:44:10PM -0500, Simon South wrote:
> Currently the Rockchip PWM driver enables the signal ("bus") clock for
> every PWM device it finds during probing, then disables it for any device
> that was not already enabled (such as by a bootloader) when the kernel
> started.
> 
> Instead of starting PWMs unnecessarily, check first to see whether a device

"starting PWM" here means enabling their clocks, right? I wouldn't
expect that this has any effect on the output, am I right?

> has already been enabled and if not, do not enable its signal clock.
> 
> Signed-off-by: Simon South <simon@simonsouth.net>
> ---
>  drivers/pwm/pwm-rockchip.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index f286a498b82c..b9faef3e9954 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -327,19 +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 bus 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);
> -		clk_disable_unprepare(pc->clk);
> -		return ret;
> -	}

Just for my understanding: That you moved clk_prepare_enable(pc->pclk)
further down is not strictly necessary for your change, right?

> -
>  	platform_set_drvdata(pdev, pc);
>  
>  	pc->data = id->data;
> @@ -353,12 +340,23 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>  		pc->chip.of_pwm_n_cells = 3;
>  	}
>  
> +	ret = clk_prepare_enable(pc->pclk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret);
> +		return ret;
> +	}
> +
>  	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
>  	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);
> +
> +	ret = enabled ? clk_prepare_enable(pc->clk) : clk_prepare(pc->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
> +		clk_disable_unprepare(pc->pclk);
> +		return ret;
> +	}

I'm not a big fan of this ?: construct. I'd prefer

	ret = clk_prepare(pc->clk);
	if (ret)
		...

	/* Keep the PWM clk enabled ... */
	enabled = ...
	if (enabled) {
		ret = clk_enable(pc->clk);
		if (ret)
			...
	}

even though it is less compact. A small benefit is that the error
message can be more accurate. (You wrote "Can't prepare bus clk" while
the problem might well be in the enable part, but mentioning "enable"
might also be misleading for the enabled = false case.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]