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

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