Re: [PATCH v2 4/7] pwm: tegra: Parametrize enable register offset

[Thierry Reding]

On Mon, Mar 30, 2026 at 11:24:09AM +0900, Mikko Perttunen wrote:
> On 2026-03-26 10:47 +0100, Thierry Reding wrote:
> > On Wed, Mar 25, 2026 at 07:17:02PM +0900, Mikko Perttunen wrote:
> > > On Tegra264, the PWM enablement bit is not located at the base address
> > > of the PWM controller. Hence, introduce an enablement offset field in
> > > the tegra_pwm_soc structure to describe the offset of the register.
> > > 
> > > Co-developed-by: Yi-Wei Wang <yiweiw@xxxxxxxxxx>
> > > Signed-off-by: Yi-Wei Wang <yiweiw@xxxxxxxxxx>
> > > Signed-off-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> > > ---
> > >  drivers/pwm/pwm-tegra.c | 17 ++++++++++++-----
> > >  1 file changed, 12 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > > index cf54f75d92a5..22d709986e8c 100644
> > > --- a/drivers/pwm/pwm-tegra.c
> > > +++ b/drivers/pwm/pwm-tegra.c
> > > @@ -61,6 +61,7 @@
> > >  
> > >  struct tegra_pwm_soc {
> > >  	unsigned int num_channels;
> > > +	unsigned int enable_reg;
> > >  };
> > >  
> > >  struct tegra_pwm_chip {
> > > @@ -197,8 +198,9 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  		err = pm_runtime_resume_and_get(pwmchip_parent(chip));
> > >  		if (err)
> > >  			return err;
> > > -	} else
> > > +	} else if (pc->soc->enable_reg == PWM_CSR_0) {
> > >  		val |= PWM_ENABLE;
> > > +	}
> > 
> > This looks incomplete for the Tegra264 case where
> > 
> > 	pc->soc->enable_reg == PWM_CSR_1
> > 
> > >  
> > >  	pwm_writel(pwm, PWM_CSR_0, val);
> > 
> > I think we need another write for PWM_CSR_1 here to properly toggle the
> > PWM_ENABLE bit on Tegra264.
> > 
> > Or am I missing something?
> 
> This check is here just so we don't change the value of PWM_ENABLE when
> writing the CSR_0 register. The function doesn't write to CSR_1 so
> nothing needs to be done on Tegra264.
> 
> I agree it's not the clearest, but it'll get cleaned up when adding
> support for configurable depth, as at that point we will need to write
> both registers on Tegra264.

Ah... nevermind. I realize now that we're not touching PWM_CSR_1 at all
in tegra_pwm_config(), so there's no need to explicitly set PWM_ENABLE.

If moving to the new APIs, that would need to change, but for the legacy
PWM callbacks this is probably fine.

Thierry

Attachment: signature.asc
Description: PGP signature