RE: [PATCH v5 4/9] pwm: rzg2l-gpt: Convert to waveform callbacks

[Cosmin-Gabriel Tanislav]

> -----Original Message-----
> From: Biju
> Sent: Monday, April 20, 2026 1:43 PM
> To: Uwe Kleine-König <ukleinek@xxxxxxxxxx>
> Cc: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; linux-pwm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> Geert Uytterhoeven <geert+renesas@xxxxxxxxx>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@xxxxxxxxxxxxxx>; Biju Das <biju.das.au@xxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx
> Subject: [PATCH v5 4/9] pwm: rzg2l-gpt: Convert to waveform callbacks
> 
> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 
> Migrate the rzg2l-gpt driver from the legacy .get_state/.apply ops to the
> new waveform callback interface.
> 
> Introduce struct rzg2l_gpt_waveform to represent a hardware waveform
> configuration holding the period register value (gtpr), compare/capture
> register value (gtccr), and prescaler (prescale).
> 
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> v5:
>  * Updated commit description.
>  * Updated rzg2l_gpt_round_waveform_tohw() to initialize gtccr when the
>    period of the second channel is smaller.
>  * Replaced period_ticks with RZG2L_MAX_TICKS for the duty_ticks maximum
>    value check in rzg2l_gpt_round_waveform_tohw().
> v4 from [1]
> [1] https://lore.kernel.org/all/20251208152133.269316-3-biju.das.jz@xxxxxxxxxxxxxx/
> ---
>  drivers/pwm/pwm-rzg2l-gpt.c | 197 ++++++++++++++++++++++--------------
>  1 file changed, 121 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
> index 659044fa3d2f..9e7a897a0b4d 100644
> --- a/drivers/pwm/pwm-rzg2l-gpt.c
> +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> @@ -100,6 +100,13 @@ struct rzg2l_gpt_chip {
>  	DECLARE_BITMAP(poeg_gpt_link, RZG2L_MAX_POEG_GROUPS * RZG2L_MAX_HW_CHANNELS);
>  };
> 
> +/* This represents a hardware configuration for one channel */
> +struct rzg2l_gpt_waveform {
> +	u32 gtpr;
> +	u32 gtccr;
> +	u8 prescale;
> +};
> +
>  static inline struct rzg2l_gpt_chip *to_rzg2l_gpt_chip(struct pwm_chip *chip)
>  {
>  	return pwmchip_get_drvdata(chip);
> @@ -166,7 +173,8 @@ static void rzg2l_gpt_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  	rzg2l_gpt->channel_request_count[ch]--;
>  }
> 
> -static bool rzg2l_gpt_is_ch_enabled(struct rzg2l_gpt_chip *rzg2l_gpt, u8 hwpwm)
> +static bool rzg2l_gpt_is_ch_enabled(struct rzg2l_gpt_chip *rzg2l_gpt, u8 hwpwm,
> +				    u32 *gtcr)
>  {
>  	u8 ch = RZG2L_GET_CH(hwpwm);
>  	u32 val;
> @@ -175,6 +183,9 @@ static bool rzg2l_gpt_is_ch_enabled(struct rzg2l_gpt_chip *rzg2l_gpt, u8 hwpwm)
>  	if (!(val & RZG2L_GTCR_CST))
>  		return false;
> 
> +	if (gtcr)
> +		*gtcr = val;
> +
>  	val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTIOR(ch));
> 
>  	return val & RZG2L_GTIOR_OxE(rzg2l_gpt_subchannel(hwpwm));
> @@ -233,54 +244,38 @@ static u64 rzg2l_gpt_calculate_period_or_duty(struct rzg2l_gpt_chip *rzg2l_gpt,
>  	return DIV64_U64_ROUND_UP(tmp, rzg2l_gpt->rate_khz);
>  }
> 
> -static int rzg2l_gpt_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> -			       struct pwm_state *state)
> -{
> -	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> -
> -	state->enabled = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm);
> -	if (state->enabled) {
> -		u32 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> -		u32 ch = RZG2L_GET_CH(pwm->hwpwm);
> -		u8 prescale;
> -		u32 val;
> -
> -		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR(ch));
> -		prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
> -
> -		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR(ch));
> -		state->period = rzg2l_gpt_calculate_period_or_duty(rzg2l_gpt, val, prescale);
> -
> -		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch));
> -		state->duty_cycle = rzg2l_gpt_calculate_period_or_duty(rzg2l_gpt, val, prescale);
> -		if (state->duty_cycle > state->period)
> -			state->duty_cycle = state->period;
> -	}
> -
> -	state->polarity = PWM_POLARITY_NORMAL;
> -
> -	return 0;
> -}
> -
>  static u32 rzg2l_gpt_calculate_pv_or_dc(u64 period_or_duty_cycle, u8 prescale)
>  {
>  	return min_t(u64, DIV_ROUND_DOWN_ULL(period_or_duty_cycle, 1 << (2 * prescale)),
>  		     U32_MAX);
>  }
> 
> -/* Caller holds the lock while calling rzg2l_gpt_config() */
> -static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> -			    const struct pwm_state *state)
> +static int rzg2l_gpt_round_waveform_tohw(struct pwm_chip *chip,
> +					 struct pwm_device *pwm,
> +					 const struct pwm_waveform *wf,
> +					 void *_wfhw)
> +
>  {
>  	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> -	u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> +	struct rzg2l_gpt_waveform *wfhw = _wfhw;
> +	bool is_small_second_period = false;
>  	u8 ch = RZG2L_GET_CH(pwm->hwpwm);
>  	u64 period_ticks, duty_ticks;
>  	unsigned long pv, dc;
> -	u8 prescale;
> +
> +	guard(mutex)(&rzg2l_gpt->lock);
> +	if (wf->period_length_ns == 0) {
> +		*wfhw = (struct rzg2l_gpt_waveform){
> +			.gtpr = 0,
> +			.gtccr = 0,
> +			.prescale = 0,
> +		};
> +
> +		return 0;
> +	}
> 
>  	/* Limit period/duty cycle to max value supported by the HW */
> -	period_ticks = mul_u64_u64_div_u64(state->period, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> +	period_ticks = mul_u64_u64_div_u64(wf->period_length_ns, rzg2l_gpt->rate_khz, USEC_PER_SEC);
>  	if (period_ticks > RZG2L_MAX_TICKS)
>  		period_ticks = RZG2L_MAX_TICKS;
>  	/*
> @@ -291,21 +286,26 @@ static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (rzg2l_gpt->channel_request_count[ch] > 1) {
>  		u8 sibling_ch = rzg2l_gpt_sibling(pwm->hwpwm);
> 
> -		if (rzg2l_gpt_is_ch_enabled(rzg2l_gpt, sibling_ch)) {
> +		if (rzg2l_gpt_is_ch_enabled(rzg2l_gpt, sibling_ch, NULL)) {
>  			if (period_ticks < rzg2l_gpt->period_ticks[ch])
> -				return -EBUSY;
> +				is_small_second_period = true;
> 
>  			period_ticks = rzg2l_gpt->period_ticks[ch];
>  		}
>  	}
> 
> -	prescale = rzg2l_gpt_calculate_prescale(period_ticks);
> -	pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, prescale);
> +	wfhw->prescale = rzg2l_gpt_calculate_prescale(period_ticks);
> +	pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, wfhw->prescale);
> +	wfhw->gtpr = pv;
> +	wfhw->gtccr = 0;
> +	if (is_small_second_period)
> +		return 1;
> 
> -	duty_ticks = mul_u64_u64_div_u64(state->duty_cycle, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> -	if (duty_ticks > period_ticks)
> -		duty_ticks = period_ticks;
> -	dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, prescale);
> +	duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> +	if (duty_ticks > RZG2L_MAX_TICKS)
> +		duty_ticks = RZG2L_MAX_TICKS;

I know this change from > period_ticks to > RZG2L_MAX_TICKS has been
suggested by you, Uwe, but is this correct if period_ticks was set to a
smaller value in the earlier sibling channel condition?

In that case I think it might be possible for duty_ticks to end up
larger than period_ticks which wouldn't work correctly with the
> RZG2L_MAX_TICKS check, but would have been clamped fine using the
other check.

What do you think?

> +	dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, wfhw->prescale);
> +	wfhw->gtccr = dc;

Is there any reason why the results of rzg2l_gpt_calculate_pv_or_dc()
cannot be assigned to wfhw->gtpr and wfhw->gtccr directly?

That would get rid of the pv and dc variables as we do not need them
for anything else anymore, as far as I can tell.

> 
>  	/*
>  	 * GPT counter is shared by multiple channels, we cache the period ticks
> @@ -314,6 +314,61 @@ static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 */
>  	rzg2l_gpt->period_ticks[ch] = period_ticks;
> 

This should be part of rzg2l_gpt_write_waveform().

Otherwise, if pwm_round_waveform_might_sleep() is called without 
pwm_set_waveform_might_sleep() being called immediately after with the
rounded waveform, the software state will become out of sync with the
hardware state.

Uwe, what's your take on this?

> +	return 0;
> +}
> +