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

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

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

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.

> +
> +		/*
> +		 * 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).

Thierry

> +			.polarity = PWM_POLARITY_NORMAL,
> +			.enabled = true,
> +		};
> +
> +	} else {
> +		*state = (struct pwm_state){
> +			.enabled = false,
> +		};
> +	}
> +
> +	pm_runtime_put(pwmchip_parent(chip));
> +
> +	return 0;
> +}
> +
>  static const struct pwm_ops tegra_pwm_ops = {
>  	.apply = tegra_pwm_apply,
> +	.get_state = tegra_pwm_get_state,
>  };
>  
>  static int tegra_pwm_probe(struct platform_device *pdev)
> -- 
> 2.47.3
> 

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