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

[Biju Das]

Hi Uwe/Cosmin,

Thanks for the feedback.

> -----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
> 
> Hello Cosmin,
> 
> 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
  * channel or an active sibling channel's period_ticks.
  */
if (duty_ticks > period_ticks)
	duty_ticks = period_ticks;

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


> >
> > 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.
> 
> Indeed, the tohw and fromhw callbacks must have no side effects.
> There isn't a set_waveform call after each round_waveform.

Ok.


Cheers,
Biju