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

[Biju Das]

Hello Uwe,

Thanks for the feedback.

> -----Original Message-----
> From: Uwe Kleine-König <ukleinek@xxxxxxxxxx>
> Sent: 17 May 2026 17:58
> Subject: Re: [PATCH v5 4/9] pwm: rzg2l-gpt: Convert to waveform callbacks
> 
> Hello Biju,
> 
> On Fri, Apr 24, 2026 at 09:01:51AM +0000, Biju Das wrote:
> > > -----Original Message-----
> > > From: Uwe Kleine-König <ukleinek@xxxxxxxxxx>
> > > 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.)

OK will drop the comment.

> 
> > > > >  	/*
> > > > >  	 * 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.

OK, will move assigning rzg2l_gpt->period_ticks[ch] = period_ticks;
to rzg2l_gpt_write_waveform().

Cheers,
Biju