| Message ID | 1f12ae7f9479aa910fcaf066fae82c7b7cb89073.1784030076.git.ukleinek@kernel.org |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | pwm: tegra: Cleanups and .get_state() | expand |
On Di, 2026-07-14 at 14:02 +0200, Uwe Kleine-König wrote: > This function ensures the reset is already deasserted at probe time and > asserted at unbind. So the remove function and the error paths in the > probe function can be simplified accordingly. > > Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org> > --- > drivers/pwm/pwm-tegra.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c > index e99e1c5b18c3..d7f4baa4cd9b 100644 > --- a/drivers/pwm/pwm-tegra.c > +++ b/drivers/pwm/pwm-tegra.c > @@ -108,7 +108,6 @@ struct tegra_pwm_soc { > > struct tegra_pwm_chip { > struct clk *clk; > - struct reset_control*rst; > > unsigned long clk_rate; > unsigned long min_period_ns; > @@ -319,6 +318,7 @@ static int tegra_pwm_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct pwm_chip *chip; > struct tegra_pwm_chip *pc; > + struct reset_control*rst; You could use this opportunity to add a space between reset_control and *rst. > const struct tegra_pwm_soc *soc; > int ret; > > @@ -391,20 +391,17 @@ static int tegra_pwm_probe(struct platform_device *pdev) > pc->min_period_ns = > (NSEC_PER_SEC / (pc->clk_rate / TEGRA_PWM_DEPTH)) + 1; > > - pc->rst = devm_reset_control_get_exclusive(dev, "pwm"); > - if (IS_ERR(pc->rst)) { > - ret = dev_err_probe(dev, PTR_ERR(pc->rst), "Failed to get reset control\n"); > + rst = devm_reset_control_get_exclusive_deasserted(dev, "pwm"); > + if (IS_ERR(rst)) { > + ret = dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset control\n"); > goto put_pm; > } > > - reset_control_deassert(pc->rst); > - > chip->ops = &tegra_pwm_ops; > > ret = pwmchip_add(chip); > if (ret < 0) { > dev_err_probe(dev, ret, "Adding pwmchip failed\n"); > - reset_control_assert(pc->rst); With this change, pm_runtime_put_sync_suspend() and pm_runtime_force_suspend() are called before the reset control is asserted again in the error case. Is this safe? regards Philipp
On Tue, Jul 14, 2026 at 02:12:38PM +0200, Philipp Zabel wrote: > On Di, 2026-07-14 at 14:02 +0200, Uwe Kleine-König wrote: > > This function ensures the reset is already deasserted at probe time and > > asserted at unbind. So the remove function and the error paths in the > > probe function can be simplified accordingly. > > > > Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org> > > --- > > drivers/pwm/pwm-tegra.c | 14 ++++---------- > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c > > index e99e1c5b18c3..d7f4baa4cd9b 100644 > > --- a/drivers/pwm/pwm-tegra.c > > +++ b/drivers/pwm/pwm-tegra.c > > @@ -108,7 +108,6 @@ struct tegra_pwm_soc { > > > > struct tegra_pwm_chip { > > struct clk *clk; > > - struct reset_control*rst; > > > > unsigned long clk_rate; > > unsigned long min_period_ns; > > @@ -319,6 +318,7 @@ static int tegra_pwm_probe(struct platform_device *pdev) > > struct device *dev = &pdev->dev; > > struct pwm_chip *chip; > > struct tegra_pwm_chip *pc; > > + struct reset_control*rst; > > You could use this opportunity to add a space between reset_control and > *rst. Oh indeed. I thought I called checkpatch, but there are two more warnings in this series that I'm not aware of, so it seems I didn't check before sending :-o > > const struct tegra_pwm_soc *soc; > > int ret; > > > > @@ -391,20 +391,17 @@ static int tegra_pwm_probe(struct platform_device *pdev) > > pc->min_period_ns = > > (NSEC_PER_SEC / (pc->clk_rate / TEGRA_PWM_DEPTH)) + 1; > > > > - pc->rst = devm_reset_control_get_exclusive(dev, "pwm"); > > - if (IS_ERR(pc->rst)) { > > - ret = dev_err_probe(dev, PTR_ERR(pc->rst), "Failed to get reset control\n"); > > + rst = devm_reset_control_get_exclusive_deasserted(dev, "pwm"); > > + if (IS_ERR(rst)) { > > + ret = dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset control\n"); > > goto put_pm; > > } > > > > - reset_control_deassert(pc->rst); > > - > > chip->ops = &tegra_pwm_ops; > > > > ret = pwmchip_add(chip); > > if (ret < 0) { > > dev_err_probe(dev, ret, "Adding pwmchip failed\n"); > > - reset_control_assert(pc->rst); > > With this change, pm_runtime_put_sync_suspend() and > pm_runtime_force_suspend() are called before the reset control is > asserted again in the error case. Is this safe? Ah, this is true for pm_runtime_put_sync_suspend(). Too bad this isn't easily fixable :-\ I would expect this not to be a problem, but given that I don't have the hardware, I guess being conservative here is needed. Thanks for catching this. Uwe
On Tuesday, July 14, 2026 11:07 PM Uwe Kleine-König wrote: > On Tue, Jul 14, 2026 at 02:12:38PM +0200, Philipp Zabel wrote: > > On Di, 2026-07-14 at 14:02 +0200, Uwe Kleine-König wrote: > > > This function ensures the reset is already deasserted at probe time and > > > asserted at unbind. So the remove function and the error paths in the > > > probe function can be simplified accordingly. > > > > > > Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org> > > > --- > > > drivers/pwm/pwm-tegra.c | 14 ++++---------- > > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c > > > index e99e1c5b18c3..d7f4baa4cd9b 100644 > > > --- a/drivers/pwm/pwm-tegra.c > > > +++ b/drivers/pwm/pwm-tegra.c > > > @@ -108,7 +108,6 @@ struct tegra_pwm_soc { > > > > > > struct tegra_pwm_chip { > > > struct clk *clk; > > > - struct reset_control*rst; > > > > > > unsigned long clk_rate; > > > unsigned long min_period_ns; > > > @@ -319,6 +318,7 @@ static int tegra_pwm_probe(struct platform_device *pdev) > > > struct device *dev = &pdev->dev; > > > struct pwm_chip *chip; > > > struct tegra_pwm_chip *pc; > > > + struct reset_control*rst; > > > > You could use this opportunity to add a space between reset_control and > > *rst. > > Oh indeed. I thought I called checkpatch, but there are two more > warnings in this series that I'm not aware of, so it seems I didn't > check before sending :-o > > > > const struct tegra_pwm_soc *soc; > > > int ret; > > > > > > @@ -391,20 +391,17 @@ static int tegra_pwm_probe(struct platform_device *pdev) > > > pc->min_period_ns = > > > (NSEC_PER_SEC / (pc->clk_rate / TEGRA_PWM_DEPTH)) + 1; > > > > > > - pc->rst = devm_reset_control_get_exclusive(dev, "pwm"); > > > - if (IS_ERR(pc->rst)) { > > > - ret = dev_err_probe(dev, PTR_ERR(pc->rst), "Failed to get reset control\n"); > > > + rst = devm_reset_control_get_exclusive_deasserted(dev, "pwm"); > > > + if (IS_ERR(rst)) { > > > + ret = dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset control\n"); > > > goto put_pm; > > > } > > > > > > - reset_control_deassert(pc->rst); > > > - > > > chip->ops = &tegra_pwm_ops; > > > > > > ret = pwmchip_add(chip); > > > if (ret < 0) { > > > dev_err_probe(dev, ret, "Adding pwmchip failed\n"); > > > - reset_control_assert(pc->rst); > > > > With this change, pm_runtime_put_sync_suspend() and > > pm_runtime_force_suspend() are called before the reset control is > > asserted again in the error case. Is this safe? > > Ah, this is true for pm_runtime_put_sync_suspend(). Too bad this isn't > easily fixable :-\ I would expect this not to be a problem, but given > that I don't have the hardware, I guess being conservative here is > needed. > > Thanks for catching this. > > Uwe I agree that it's unlikely anything bad would happen in practice if asserting the reset after disabling the clock, but the proper sequence indeed to my understanding is the other way around. Mikko
diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c index e99e1c5b18c3..d7f4baa4cd9b 100644 --- a/drivers/pwm/pwm-tegra.c +++ b/drivers/pwm/pwm-tegra.c @@ -108,7 +108,6 @@ struct tegra_pwm_soc { struct tegra_pwm_chip { struct clk *clk; - struct reset_control*rst; unsigned long clk_rate; unsigned long min_period_ns; @@ -319,6 +318,7 @@ static int tegra_pwm_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct pwm_chip *chip; struct tegra_pwm_chip *pc; + struct reset_control*rst; const struct tegra_pwm_soc *soc; int ret; @@ -391,20 +391,17 @@ static int tegra_pwm_probe(struct platform_device *pdev) pc->min_period_ns = (NSEC_PER_SEC / (pc->clk_rate / TEGRA_PWM_DEPTH)) + 1; - pc->rst = devm_reset_control_get_exclusive(dev, "pwm"); - if (IS_ERR(pc->rst)) { - ret = dev_err_probe(dev, PTR_ERR(pc->rst), "Failed to get reset control\n"); + rst = devm_reset_control_get_exclusive_deasserted(dev, "pwm"); + if (IS_ERR(rst)) { + ret = dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset control\n"); goto put_pm; } - reset_control_deassert(pc->rst); - chip->ops = &tegra_pwm_ops; ret = pwmchip_add(chip); if (ret < 0) { dev_err_probe(dev, ret, "Adding pwmchip failed\n"); - reset_control_assert(pc->rst); goto put_pm; } @@ -419,11 +416,8 @@ static int tegra_pwm_probe(struct platform_device *pdev) static void tegra_pwm_remove(struct platform_device *pdev) { struct pwm_chip *chip = platform_get_drvdata(pdev); - struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip); pwmchip_remove(chip); - - reset_control_assert(pc->rst); } static int __maybe_unused tegra_pwm_runtime_suspend(struct device *dev)
This function ensures the reset is already deasserted at probe time and asserted at unbind. So the remove function and the error paths in the probe function can be simplified accordingly. Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org> --- drivers/pwm/pwm-tegra.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)