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

[Robin Murphy <robin.murphy@arm.com>]

On 2020-12-19 20:44, 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
> 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;
> -	}
> -
>   	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);

Since you're touching it, I guess it might be a good idea to update this 
message to say "PWM clk" for clarity.

I suspect there might also have been some historical confounding in the 
fact that when there is only one clock (pclk_pwm), it's merely a gate 
whose parent is pclk_bus, which serves all the peripherals in the 
usefully-named PD_BUS domain...

Robin.

> +		clk_disable_unprepare(pc->pclk);
> +		return ret;
> +	}
>   
>   	clk_disable(pc->pclk);
>   
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel