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

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

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

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@nvidia.com>
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  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)?

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.

> +
>  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.

>  {
> -	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

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