Re: [PATCH v4 3/7] pwm: tegra: Modify read/write accessors for multi-register channel

[Mikko Perttunen]

On Monday, May 18, 2026 2:30 AM Uwe Kleine-König wrote:
> On Tue, Mar 31, 2026 at 11:12:15AM +0900, Mikko Perttunen wrote:
> > On Tegra264, each PWM instance has two registers (per channel, of which
> > there is one). Update the pwm_readl/pwm_writel helper functions to
> > take channel (as struct pwm_device *) and offset separately.
> > 
> > Reviewed-by: Thierry Reding <treding@xxxxxxxxxx>
> > Signed-off-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> > ---
> >  drivers/pwm/pwm-tegra.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > index 8a330169d531..358c81cea05b 100644
> > --- a/drivers/pwm/pwm-tegra.c
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -57,6 +57,8 @@
> >  #define PWM_SCALE_WIDTH	13
> >  #define PWM_SCALE_SHIFT	0
> >  
> > +#define PWM_CSR_0	0
> 
> Is this a register offset (of the for now single per channel register)?

It is, yes.

> 
> One thing that bothers me about this driver is that the defines are not
> prefixed by the driver name. `PWM_SCALE_WIDTH` looks more generic than
> it is.

I can add a patch to prefix everything with tegra_/TEGRA_ for the next
version.

> 
> > +
> >  struct tegra_pwm_soc {
> >  	unsigned int num_channels;
> >  };
> > @@ -78,14 +80,18 @@ static inline struct tegra_pwm_chip *to_tegra_pwm_chip(struct pwm_chip *chip)
> >  	return pwmchip_get_drvdata(chip);
> >  }
> >  
> > -static inline u32 pwm_readl(struct tegra_pwm_chip *pc, unsigned int offset)
> > +static inline u32 pwm_readl(struct pwm_device *dev, unsigned int offset)
> 
> s/dev/pwm/ to match the variable naming in the rest of the driver.

Will fix.

> 
> >  {
> > -	return readl(pc->regs + (offset << 4));
> > +	struct tegra_pwm_chip *chip = to_tegra_pwm_chip(dev->chip);
> > +
> > +	return readl(chip->regs + (dev->hwpwm * 16) + offset);
> 
> Best regards
> Uwe

Thanks
Mikko