| Message ID | 20250207213424.1117-1-rafael.v.volkmer@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3,1/3] pwm: tiehrpwm: replace manual bit definitions with bitfield.h macros | expand |
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@gmail.com> > --- > 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
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); + + if (state.enabled == true) { + ret = clk_prepare_enable(pc->tbclk); + 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; + } + } + return 0; +err_pwmchip_remove: + pwmchip_remove(chip); err_clk_unprepare: - clk_unprepare(pc->tbclk); + if (tbclk_enabled) + clk_unprepare(pc->tbclk); return ret; }
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 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@gmail.com> --- drivers/pwm/pwm-tiehrpwm.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)