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: 2591 bytes --]

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.

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

Indeed, the tohw and fromhw callbacks must have no side effects.
There isn't a set_waveform call after each round_waveform.

Best regards
Uwe

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