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

["Uwe Kleine-König" <ukleinek@kernel.org>]

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

Hello Biju,

On Fri, Apr 24, 2026 at 09:01:51AM +0000, Biju Das wrote:
> > -----Original Message-----
> > From: Uwe Kleine-König <ukleinek@kernel.org>
> > Sent: 21 April 2026 09:41
> > Subject: Re: [PATCH v5 4/9] pwm: rzg2l-gpt: Convert to waveform callbacks
> > 
> > On Mon, Apr 20, 2026 at 05:55:07PM +0000, Cosmin-Gabriel Tanislav wrote:
> > > > @@ -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?
> > 
> > Indeed this is irritating. I assume I missed that and take the blame for the wrong suggestions.
> > Depending on how hardware copes with such a configuration it might be ok to keep the code as is, but a
> > comment would be justified in this case.
> 
> Please confirm
> 
>  /*
>   * duty_ticks were clampled to match either period_ticks of this

I think it's "clamped"

>   * channel or an active sibling channel's period_ticks.
>   */
> if (duty_ticks > period_ticks)
> 	duty_ticks = period_ticks;

I think the comment is not needed in this case. (I only wanted a comment
if the comparison against RZG2L_MAX_TICKS was kept.)

> > > >  	/*
> > > >  	 * 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().
> 
> Also, if we move this to rzg2l_gpt_write_waveform() there is a rounding
> error possible as we need to use hardware register to calculate
> rzg2l_gpt->period_ticks[ch].
> 
> Can you please confirm, is it ok for you?

I don't understand that. Why is there a rounding error possible? The
relevant thing here is that a call to the two rounding callbacks is not
supposed to change internal state of the driver or even the hardware.

Best regards
Uwe

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