linux-kernel - Re: [PATCH v4 2/4] pwm: tiehrpwm: add get_state function to retrieve PWM channel state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <iguu6qhympqyoyxxqwwlyceg4qk2pwqodwfvwh2q45z5hqkqdv@zinbt6go5rqn>
Date: Tue, 13 May 2025 11:27:23 +0200
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 v4 2/4] pwm: tiehrpwm: add get_state function to retrieve
 PWM channel state

Hello Rafael,

On Sat, Apr 19, 2025 at 04:55:55PM -0300, Rafael V. Volkmer wrote:
> The ehrpwm driver was missing a get_state function, which is required
> to properly retrieve the current state of the PWM channel. Add the
> ehrpwm_get_state() function, allowing users to query the enabled state,
> period, duty cycle, and polarity of the PWM output.
> 
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@...il.com>
> ---
>  drivers/pwm/pwm-tiehrpwm.c | 97 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 1ead1aa91a1a..cde331a73696 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -68,7 +68,9 @@
>  #define AQCTL_ZRO_MASK	GENMASK(1, 0)
>  #define AQCTL_PRD_MASK	GENMASK(3, 2)
>  #define AQCTL_CAU_MASK	GENMASK(5, 4)
> +#define AQCTL_CAD_MASK	GENMASK(7, 6)
>  #define AQCTL_CBU_MASK	GENMASK(9, 8)
> +#define AQCTL_CBD_MASK	GENMASK(11, 10)
>  
>  /* common action codes (2‑bit) */
>  #define AQCTL_FRCLOW	1
> @@ -470,9 +472,104 @@ 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, cmpa_reg;
> +
> +	/* Bits */
> +	u8 csf_bits;
> +
> +	/* Values */
> +	u64 period_cycles, duty_cycles;
> +
> +	/* 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 == 0) {
> +		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)
> +		state->enabled = false;
> +	else if (aqctl_reg)
> +		state->enabled = true;
> +	else
> +		state->enabled = false;

if (csf_bits || !aqctl_reg) {
	state->enabled = false;
	return 0;
}

?
	
> +
> +	tbprd_reg = readw(pc->mmio_base + TBPRD);
> +	period_cycles = (u64)tbprd_reg + 1u;
> +
> +	/*
> +	 * period (in ns) = (period_cycles * 1e9) / clk_rate
> +	 * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
> +	 */
> +	state->period = DIV_ROUND_UP_ULL(period_cycles * NSEC_PER_SEC, pc->clk_rate);
> +
> +	cmpa_reg = readw(pc->mmio_base + CMPA);
> +	duty_cycles = cmpa_reg;

duty_cycles = readw(pc->mmio_base + CMPA);

and drop the otherwise unused cmpa_reg.

I would expect that you need to read CMPB for hwpwm == 1?

I see the naming is consistent with ehrpwm_pwm_config, but I'd prefer
period_ticks and duty_ticks over period_cycles and duty_cycles. Then
it's clear that the unit is clock ticks and not ns.

> +	/*
> +	 * duty_cycle (in ns) = (duty_cycles * 1e9) / clk_rate
> +	 * Using DIV_ROUND_UP_ULL to avoid floating-point operations.
> +	 */
> +	state->duty_cycle = DIV_ROUND_UP_ULL(duty_cycles * 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 == 0) {
> +		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);
> +	}

When you send a new revision, please start a new thread.

Best regards
Uwe

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