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

[Mikko Perttunen]

On Monday, May 18, 2026 2:35 AM Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Mar 31, 2026 at 11:12:16AM +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 358c81cea05b..b925ef914411 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;
> > +	}
> >  
> >  	pwm_writel(pwm, PWM_CSR_0, val);
> >  
> 
> The patch is a bit artificial because we don't have a driver yet where
> `pc->soc->enable_reg == PWM_CSR_0` doesn't hold. But it looks strange to
> me that there is no enable bit set for the pc->soc->enable_reg !=
> PWM_CSR_0 case. So I tend to want these changes in squashed into another
> patch such that the combined patch handles the enabling completely.

Patches 3-5 are all preparatory for patch 6 which finally is enabling
Tegra264. Would you like all of these patches squashed?

FWIW, the reason there is no pc->soc->enable_reg != PWM_CSR_0 case is
that this function only writes PWM_CSR_0. So if it's anything else we
don't need to take care to preserve the enable bit. It's a bit janky
but I wasn't immediately able to find anything nicer without a larger
refactoring.

> 
> > @@ -213,6 +215,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  
> >  static int tegra_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> > +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> >  	int rc = 0;
> >  	u32 val;
> >  
> > @@ -220,20 +223,22 @@ static int tegra_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  	if (rc)
> >  		return rc;
> >  
> > -	val = pwm_readl(pwm, PWM_CSR_0);
> > +
> 
> A single empty line is enough.

Sorry, will fix.

Thanks
Mikko

> 
> > +	val = pwm_readl(pwm, pc->soc->enable_reg);
> >  	val |= PWM_ENABLE;
> > -	pwm_writel(pwm, PWM_CSR_0, val);
> > +	pwm_writel(pwm, pc->soc->enable_reg, val);
> >  
> >  	return 0;
> >  }
> >  
> >  static void tegra_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> > +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> >  	u32 val;
> >  
> > -	val = pwm_readl(pwm, PWM_CSR_0);
> > +	val = pwm_readl(pwm, pc->soc->enable_reg);
> >  	val &= ~PWM_ENABLE;
> > -	pwm_writel(pwm, PWM_CSR_0, val);
> > +	pwm_writel(pwm, pc->soc->enable_reg, val);
> >  
> >  	pm_runtime_put_sync(pwmchip_parent(chip));
> >  }
> > @@ -398,10 +403,12 @@ static int __maybe_unused tegra_pwm_runtime_resume(struct device *dev)
> >  
> >  static const struct tegra_pwm_soc tegra20_pwm_soc = {
> >  	.num_channels = 4,
> > +	.enable_reg = PWM_CSR_0,
> >  };
> >  
> >  static const struct tegra_pwm_soc tegra186_pwm_soc = {
> >  	.num_channels = 1,
> > +	.enable_reg = PWM_CSR_0,
> >  };
> >  
> >  static const struct of_device_id tegra_pwm_of_match[] = {
> > 
> > -- 
> > 2.53.0
> >