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

[Mikko Perttunen <mperttunen@nvidia.com>]

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