Re: [PATCH v3 3/3] pwm: ehrpwm: ensure clock and runtime PM are enabled if hardware is active

[Uwe Kleine-König]

Hello Rafael,

On Fri, Feb 07, 2025 at 06:34:24PM -0300, Rafael V. Volkmer wrote:
> During probe, if the hardware is already active, it is not guaranteed
> that the clock is enabled. To address this, ehrpwm_pwm_probe() now
> checks whether the PWM is enabled and ensures that the necessary
> resources are initialized.
> 
> Changes:
> - Call ehrpwm_get_state() during probe to check if the PWM is active.
> - If the PWM is enabled, call clk_prepare_enable() to ensure the clock
> is active.
> - If the clock is successfully enabled, call pm_runtime_get_sync() to
> manage power state.
> - Handle failure cases by properly disabling and unpreparing the clock.

This is too detailed, just drop the changes list.

> This ensures that the driver correctly handles cases where the hardware
> is already in use at the time of initialization, preventing potential
> failures due to uninitialized resources.
> 
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@xxxxxxxxx>
> ---
>  drivers/pwm/pwm-tiehrpwm.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 52527136c507..30beaf7d1721 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -633,8 +633,10 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
>  	struct ehrpwm_pwm_chip *pc;
> +	struct pwm_state state;
>  	struct pwm_chip *chip;
>  	struct clk *clk;
> +	bool tbclk_enabled;
>  	int ret;
>  
>  	chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
> @@ -676,6 +678,18 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ehrpwm_get_state(chip, &chip->pwms[0], &state);

ehrpwm_get_state() does some things that are not needed here given that
you only evaluate state.enabled. I suggest to just read the one (or
two?) registers you need to determine if the PWM is on.

> +	if (state.enabled == true) {
> +		ret = clk_prepare_enable(pc->tbclk);

pc->tbclk is already prepared, so clk_enable() should be enough. After
all this should match what ehrpwm_pwm_enable() does.

> +		if (ret) {
> +			dev_err_probe(&pdev->dev, ret, "clk_prepare_enable() failed");
> +			goto err_pwmchip_remove;
> +		}
> +
> +		tbclk_enabled = true;
> +	}
> +
>  	ret = pwmchip_add(chip);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> @@ -685,10 +699,22 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, chip);
>  	pm_runtime_enable(&pdev->dev);
>  
> +	if (state.enabled == true) {
> +		ret = pm_runtime_get_sync(&pdev->dev);
> +		if (ret < 0) {
> +			dev_err_probe(&pdev->dev, ret, "pm_runtime_get_sync() failed");
> +			clk_disable_unprepare(pc->tbclk);
> +			goto err_pwmchip_remove;
> +		}

It feels a bit strange to do this here. I think technically it's fine
here, but doing pm_runtime_get_sync() before pwmchip_add() would make
that a bit clearer.

> +	}
> +
>  	return 0;
>  
> +err_pwmchip_remove:
> +	pwmchip_remove(chip);
>  err_clk_unprepare:
> -	clk_unprepare(pc->tbclk);
> +	if (tbclk_enabled)
> +		clk_unprepare(pc->tbclk);

I think this is wrong an keeping the unconditional clk_unprepare() is
right. Might be easier to convert the driver to use
devm_clk_get_enabled().

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature