Message-ID: <4zdh44geiodumnqsfmwbxcfhishs7xeg5qsb6o3zb3nog7yfu6@wvqwqwltlip5>
Date: Mon, 24 Nov 2025 17:51:47 +0100
From: Uwe Kleine-König <ukleinek@...nel.org>
To: "Rafael V. Volkmer" <rafael.v.volkmer@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v6 4/6] pwm: tiehrpwm: implement .get_state callback

Hello Rafael,

On Sun, Nov 23, 2025 at 08:31:51PM -0300, Rafael V. Volkmer wrote:
> Implement ehrpwm_get_state() so that consumers can query the current
> hardware configuration via pwm_get_state().

Note this is wrong as pwm_get_state() only yields the last request but
not the actual hardware setting.

> The callback reconstructs period and duty_cycle in nanoseconds from
> TBPRD, CMPA/B and the TBCTL prescaler fields. It also inspects AQCSFRC
> to treat software-forced outputs as disabled, and decodes the CAx/CDx
> action-qualifier bits in AQCTL(A/B) to report the current polarity.
> 
> To make polarity decoding reliable, the configuration path now programs
> both up- and down-count compare actions (CAx/CDx) consistently for each
> polarity.
> 
> This lets consumers query the effective hardware state instead of just
> the last requested settings.
> 
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@...il.com>
> ---
>  drivers/pwm/pwm-tiehrpwm.c | 132 +++++++++++++++++++++++++++++++++----
>  1 file changed, 121 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 9f1be35912d3..cc525626b2f9 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -114,6 +114,9 @@
>  
>  #define NUM_PWM_CHANNEL		2	/* EHRPWM channels */
>  
> +#define PWM_CHA			0
> +#define PWM_CHB			1

Please don't introduce more defines with a PWM_ prefix that look more
generic than they are.

>  struct ehrpwm_context {
>  	u16 tbctl;
>  	u16 tbprd;
> @@ -267,37 +270,37 @@ static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	/* Update clock prescaler values */
>  	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_PRESCALE_MASK, tb_divval);
>  
> -	if (pwm->hwpwm == 1) {
> +	if (pwm->hwpwm == PWM_CHB) {
>  		/* Channel 1 configured with compare B register */
>  		cmp_reg = CMPB;
>  
>  		aqctl_reg = AQCTLB;
> -		aqctl_mask = AQCTL_CBU_MASK;
> +		aqctl_mask = AQCTL_CBU_MASK | AQCTL_CBD_MASK;
>  
>  		if (polarity == PWM_POLARITY_INVERSED)
> -			aqctl_val = AQCTL_CHB_UP_POLINVERSE;
> +			aqctl_val = AQCTL_CHB_UP_POLINVERSE | AQCTL_CHB_DN_POLINVERSE;
>  		else
> -			aqctl_val = AQCTL_CHB_UP_POLNORMAL;
> +			aqctl_val = AQCTL_CHB_UP_POLNORMAL | AQCTL_CHB_DN_POLNORMAL;
>  
> -		/* if duty_cycle is big, don't toggle on CBU */
> +		/* if duty_cycle is big, don't toggle on compare events */
>  		if (duty_cycles > period_cycles)
> -			aqctl_val &= ~AQCTL_CBU_MASK;
> +			aqctl_val &= ~(AQCTL_CBU_MASK | AQCTL_CBD_MASK);
>  
>  	} else {
>  		/* Channel 0 configured with compare A register */
>  		cmp_reg = CMPA;
>  
>  		aqctl_reg = AQCTLA;
> -		aqctl_mask = AQCTL_CAU_MASK;
> +		aqctl_mask = AQCTL_CAU_MASK | AQCTL_CAD_MASK;
>  
>  		if (polarity == PWM_POLARITY_INVERSED)
> -			aqctl_val = AQCTL_CHA_UP_POLINVERSE;
> +			aqctl_val = AQCTL_CHA_UP_POLINVERSE | AQCTL_CHA_DN_POLINVERSE;
>  		else
> -			aqctl_val = AQCTL_CHA_UP_POLNORMAL;
> +			aqctl_val = AQCTL_CHA_UP_POLNORMAL | AQCTL_CHA_DN_POLNORMAL;
>  
> -		/* if duty_cycle is big, don't toggle on CAU */
> +		/* if duty_cycle is big, don't toggle on compare events */
>  		if (duty_cycles > period_cycles)
> -			aqctl_val &= ~AQCTL_CAU_MASK;
> +			aqctl_val &= ~(AQCTL_CAU_MASK | AQCTL_CAD_MASK);
>  	}
>  
>  	aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK;

Can you please split out these changes? These if branches are nearly
identical, I wonder if there is a way to make them smaller without
making it considerably less understandable.

> @@ -426,9 +429,116 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return err;
>  }
>  
> +static int ehrpwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    struct pwm_state *state)
> +{
> +	int ret = 0;
> +
> +	struct ehrpwm_pwm_chip *pc = NULL;
> +
> +	/* Registers */
> +	u16 aqcsfrc_reg, aqctl_reg, tbprd_reg, tbctl_reg;
> +
> +	/* Bits */
> +	u8 csf_bits, clkdiv_bits, hspclkdiv_bits;
> +
> +	/* Values */
> +	u64 period_ticks, duty_ticks, ps_div;
> +
> +	/* Actions */
> +	u8 up_action, down_action;
> +
> +	pc = to_ehrpwm_pwm_chip(chip);
> +
> +	/*
> +	 * The 'hwpwm' field identifies which hardware output channel (e.g.,
> +	 * 0 for channel A and 1 for channel B) of the eHRPWM module is in use.
> +	 */
> +	if (pwm->hwpwm == PWM_CHA) {
> +		aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
> +		csf_bits = FIELD_GET(AQCSFRC_CSFA_MASK, aqcsfrc_reg);
> +		aqctl_reg = readw(pc->mmio_base + AQCTLA);
> +	} else {
> +		aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
> +		csf_bits = FIELD_GET(AQCSFRC_CSFB_MASK, aqcsfrc_reg);
> +		aqctl_reg = readw(pc->mmio_base + AQCTLB);
> +	}
> +
> +	if (csf_bits || !aqctl_reg) {
> +		state->enabled = false;
> +		return 0;
> +	}
> +
> +	state->enabled = true;
> +
> +	tbprd_reg = readw(pc->mmio_base + TBPRD);
> +	period_ticks = (u64)tbprd_reg + 1u;
> +
> +	tbctl_reg = readw(pc->mmio_base + TBCTL);
> +	hspclkdiv_bits = FIELD_GET(TBCTL_HSPCLKDIV_MASK, tbctl_reg);
> +	clkdiv_bits = FIELD_GET(TBCTL_CLKDIV_MASK, tbctl_reg);
> +
> +	ps_div = (1 << clkdiv_bits) * (hspclkdiv_bits ? (hspclkdiv_bits * 2) : 1);
> +
> +	/*
> +	 * period (in ns) = (period_ticks * preescaler * 1e9) / clk_rate
> +	 * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
> +	 */
> +	state->period = DIV_ROUND_UP_ULL(period_ticks * ps_div * NSEC_PER_SEC, pc->clk_rate);
> +
> +	if (pwm->hwpwm == PWM_CHA)
> +		duty_ticks = readw(pc->mmio_base + CMPA);
> +	else
> +		duty_ticks = readw(pc->mmio_base + CMPB);
> +
> +	/*
> +	 * duty_cycle (in ns) = (duty_ticks * preescaler * 1e9) / clk_rate
> +	 * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
> +	 */
> +	state->duty_cycle = DIV_ROUND_UP_ULL(duty_ticks * ps_div * NSEC_PER_SEC, pc->clk_rate);
> +
> +	/*
> +	 * The 'hwpwm' field identifies which hardware output channel (e.g.,
> +	 * 0 for channel A and 1 for channel B) of the eHRPWM module is in use.
> +	 */
> +	if (pwm->hwpwm == PWM_CHA) {
> +		aqctl_reg = readw(pc->mmio_base + AQCTLA);
> +		up_action = FIELD_GET(AQCTL_CAU_MASK, aqctl_reg);
> +		down_action = FIELD_GET(AQCTL_CAD_MASK, aqctl_reg);
> +	} else {
> +		aqctl_reg = readw(pc->mmio_base + AQCTLB);
> +		up_action = FIELD_GET(AQCTL_CBU_MASK, aqctl_reg);
> +		down_action = FIELD_GET(AQCTL_CBD_MASK, aqctl_reg);
> +	}
> +
> +	/*
> +	 * Evaluate the actions to determine the PWM polarity:
> +	 *  - In this driver, NORMAL polarity is programmed as:
> +	 *      ZRO = FRCHIGH and CAx/CDx = FRCLOW.
> +	 *  - INVERSED polarity is programmed as:
> +	 *      ZRO = FRCLOW and CAx/CDx = FRCHIGH.
> +	 *
> +	 * So if both up/down compare actions are FRCLOW we treat it as
> +	 * NORMAL, and if both are FRCHIGH we treat it as INVERSED.
> +	 */
> +	if (up_action == AQCTL_FRCLOW && down_action == AQCTL_FRCLOW) {
> +		state->polarity = PWM_POLARITY_NORMAL;
> +	} else if (up_action == AQCTL_FRCHIGH && down_action == AQCTL_FRCHIGH) {
> +		state->polarity = PWM_POLARITY_INVERSED;
> +	} else {
> +		state->polarity = PWM_POLARITY_NORMAL;
> +		dev_dbg(pwmchip_parent(chip),
> +			"ehrpwm: unknown polarity bits (0x%x/0x%x), defaulting to NORMAL\n",
> +			up_action, down_action);
> +	}
> +
> +	return ret;
> +}
> +
>  static const struct pwm_ops ehrpwm_pwm_ops = {
>  	.free = ehrpwm_pwm_free,
>  	.apply = ehrpwm_pwm_apply,
> +	.get_state = ehrpwm_get_state,
>  };

Without having looked in detail: 

A superior improvement over implementing .get_state() is implementing the
new callbacks round_waveform_tohw, round_waveform_fromhw, read_waveform
and write_waveform. And bonus points if you also implement offset
support.

(A "no" here is completely fine, just let me know if you like the idea
and want to work on it. Otherwise I'll look in more detail here.)

At one point in time I had that on my todo list but dropped the ball
after the fixes that went into 6.18-rc1.

>  static const struct of_device_id ehrpwm_of_match[] = {

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists