Re: [PATCH v2 3/3] pwm: tegra: Implement .get_state()

[Uwe Kleine-König]

Hello Thierry,

On Mon, Sep 21, 2026 at 12:18:16PM +0200, Thierry Reding wrote:
> On Fri, Sep 18, 2026 at 04:33:47PM +0200, Uwe Kleine-König wrote:
> > The registers of the PWM IP are readable. Use that to implement the
> > .get_state() callback.
> > 
> > Reviewed-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> > Tested-by: Ola Chr. Vaage <ola.christoffer.vage@xxxxxxxxxxx>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx>
> > ---
> >  drivers/pwm/pwm-tegra.c | 48 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > index b461d3877f43..0520d025c776 100644
> > --- a/drivers/pwm/pwm-tegra.c
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -310,8 +310,56 @@ static int tegra_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	return err;
> >  }
> >  
> > +static int tegra_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			       struct pwm_state *state)
> > +{
> > +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> > +	int rc;
> > +	u32 val;
> > +
> > +	rc = pm_runtime_resume_and_get(pwmchip_parent(chip));
> > +	if (rc)
> > +		return rc;
> > +
> > +	val = tegra_pwm_readl(pwm, pc->soc->enable_reg);
> > +	if (val & TEGRA_PWM_ENABLE) {
> > +		u32 scale, pwm0;
> > +
> > +		if (pc->soc->enable_reg != TEGRA_PWM_CSR_0)
> > +			val = tegra_pwm_readl(pwm, TEGRA_PWM_CSR_0);
> > +
> > +		scale = (val >> TEGRA_PWM_SCALE_SHIFT) & ((1 << pc->soc->scale_width) - 1);
> > +		pwm0 = (val >> TEGRA_PWM_DUTY_SHIFT) & (2 * TEGRA_PWM_DEPTH - 1);
> > +
> > +		if (pwm0 > TEGRA_PWM_DEPTH)
> > +			pwm0 = TEGRA_PWM_DEPTH;
> 
> It feels like this has too many assumptions built-in. That's mostly a
> predefined issue, but I think if we want to get accurate hardware read-
> out, we need to address this.
> 
> According to the register documentation, the PWM depth is 16 bits wide
> (on generations where it can be programmed). The value defaults to 255
> (which is n - 1 encoded, hence TEGRA_PWM_DEPTH), but it can technically
> be reprogrammed to any 16-bit value, as far as I can tell.
> 
> So I think for this to be correct we'd need to read out the actual value
> before overwriting with TEGRA_PWM_CSR_0 contents above. At that point I
> think we'd need to either adjust the mask to be (2 * depth) - 1, or
> maybe better yet, avoid masking it out arbitrarily based on the depth
> and instead cap it at depth so we never exceed the 1:1 ratio for duty
> cycle vs. period.

As long as .apply() also hardcodes TEGRA_PWM_DEPTH, it's IMO fine that
.get_state() does so, too.

> > +		/*
> > +		 * scale + 1 is at most 1 << 17, TEGRA_PWM_DEPTH is 256, so the
> > +		 * multiplication for .period doesn't overflow a u64. With
> > +		 * pwm0 ≤ TEGRA_PWM_DEPTH, .duty_cycle is also fine.
> > +		 */
> > +		*state = (struct pwm_state){
> > +			.period = DIV64_U64_ROUND_UP((u64)(scale + 1) * TEGRA_PWM_DEPTH * NSEC_PER_SEC, pc->clk_rate),
> > +			.duty_cycle = DIV64_U64_ROUND_UP((u64)(scale + 1) * pwm0 * NSEC_PER_SEC, pc->clk_rate),
> 
> Maybe as part of the above this can be broken down a bit to make it
> easier to read? Something like:
> 
> 	u64 duty_cycle = (u64)(scale + 1) * pwm0 * NSEC_PER_SEC;
> 	u64 period = (u64)(scale + 1) * depth * NSEC_PER_SEC;
> 
> 	...
> 
> 	*state = (struct pwm_state) {
> 		.period = DIV64_U64_ROUND_UP(period, pc->clk_rate),
> 		.duty_cycle = DIV64_U64_ROUND_UP(duty_cycle, pc->clk_rate),
> 		...
> 	};
> 
> is a bit easier on the eye and would make checkpatch happy (or happier).

Fine for me, will address in the next revision.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature