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

[Thierry Reding <thierry.reding@kernel.org>]

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

On Mon, Sep 21, 2026 at 04:26:03PM +0200, Uwe Kleine-König wrote:
> 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@nvidia.com>
> > > Tested-by: Ola Chr. Vaage <ola.christoffer.vage@scoutdi.com>
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > > ---
> > >  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.

Okay, fair enough.

Thierry

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