linux-kernel - RE: [PATCH v4] pwm: rzg2l-gpt: Implementation of the waveform callbacks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <TY3PR01MB11346A3DA30AF85296A927D1D868DA@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Fri, 16 Jan 2026 09:34:02 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Uwe Kleine-König <ukleinek@...nel.org>, biju.das.au
	<biju.das.au@...il.com>
CC: "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Geert
 Uytterhoeven <geert+renesas@...der.be>, Prabhakar Mahadev Lad
	<prabhakar.mahadev-lad.rj@...renesas.com>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH v4] pwm: rzg2l-gpt: Implementation of the waveform
 callbacks

Hi Uwe,

Thanks for the feedback.

> -----Original Message-----
> From: Uwe Kleine-König <ukleinek@...nel.org>
> Sent: 15 January 2026 09:53
> Subject: Re: [PATCH v4] pwm: rzg2l-gpt: Implementation of the waveform callbacks
> 
> 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;

wfhw->gtccr = 0;

> > +	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.

Ok, I will initialize the value to 0 explicitly as it returns without initializing
.gtccr.

> 
> > -	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

Yes.

> 
> 	if (duty_ticks > RZG2L_MAX_TICKS)
> 		duty_ticks = RZG2L_MAX_TICKS;
> 
> here, too?

Agreed. Will replace the check for duty_ticks from
period_ticks-> RZG2L_MAX_TICKS.

> 
> > -	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

OK, will fix the comment. I am planning to incorporate the above comments as
v5 of the series [1]. I hope it is ok for you.

[1] https://lore.kernel.org/linux-renesas-soc/20251208152133.269316-1-biju.das.jz@bp.renesas.com/

Cheers,
Biju