Re: [PATCH v4] pwm: rzg2l-gpt: Implementation of the waveform callbacks

[Uwe Kleine-König]

Hello Biju,

On Fri, Nov 28, 2025 at 10:34:29AM +0000, Biju wrote:
> -/* 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;
>  	/*
> @@ -278,21 +273,25 @@ 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(rzg2l_gpt, period_ticks);
> -	pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, prescale);
> +	wfhw->prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_ticks);
> +	pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, wfhw->prescale);
> +	wfhw->gtpr = pv;
> +	if (is_small_second_period)
> +		return 1;

Why don't you need to set .gtccr in this case? I think it's wrong, but
even if 0 is ok, please initialize the value explicitly.

> -	duty_ticks = mul_u64_u64_div_u64(state->duty_cycle, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> +	duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns, rzg2l_gpt->rate_khz, USEC_PER_SEC);
>  	if (duty_ticks > period_ticks)
>  		duty_ticks = period_ticks;

Orthogonal to this patch: The if condition can only become true if

	if (period_ticks > RZG2L_MAX_TICKS)
		period_ticks = RZG2L_MAX_TICKS;

triggered above, right? So maybe it's more natural to do

	if (duty_ticks > RZG2L_MAX_TICKS)
		duty_ticks = RZG2L_MAX_TICKS;

here, too?

> -	dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, prescale);
> +	dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, wfhw->prescale);
> +	wfhw->gtccr = dc;
>  
>  	/*
>  	 * GPT counter is shared by multiple channels, we cache the period ticks

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature