[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
<TY3PR01MB11346BFFE8362B9B5619AC0658622A@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Tue, 5 Aug 2025 14:24:30 +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] pwm: rzg2l-gpt: Implementation of the waveform callbacks
Hello Uwe,
> -----Original Message-----
> From: Uwe Kleine-König <ukleinek@...nel.org>
> Sent: 05 August 2025 14:28
> Subject: Re: [PATCH] pwm: rzg2l-gpt: Implementation of the waveform callbacks
>
> Hello Biju,
>
> On Tue, Aug 05, 2025 at 07:50:23AM +0100, Biju wrote:
> > From: Biju Das <biju.das.jz@...renesas.com>
> >
> > Convert the rzg2l-gpt driver to use the new callbacks for hardware
> > programming.
>
> You picked a great project!
I started working on adding PWM support for new SoC RZ/G3E.
Plan is to add new SoC with wf callback support.
>
> > +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;
> > 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,
> > + };
> > +
> > + if (rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm))
> > + rzg2l_gpt_disable(rzg2l_gpt, pwm);
>
> This is wrong. the .round_waveform_tohw() callback isn't supposed to modify hardware. It's only
> supposed to tell the caller about the current possibilities of the corresponding device.
OK, will move it to write_waveform().
>
> > + 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;
> > /*
>
> Otherwise it looks good from a quick glance.
Thanks for the quick feedback.
Cheers,
Biju
Powered by blists - more mailing lists