Re: [PATCH v1 4/6] pwm: tegra: Simplify using devm_reset_control_get_exclusive_deasserted()

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2702 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]