On Mon, May 22, 2023 at 07:43:28AM +0000, Biju Das wrote:
> Hi Uwe,
>
> > -----Original Message-----
> > From: Biju Das
> > Sent: Tuesday, May 16, 2023 9:15 AM
> > To: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > Cc: Thierry Reding <thierry.reding@xxxxxxxxx>; linux-
> > pwm@xxxxxxxxxxxxxxx; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>;
> > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>; linux-
> > renesas-soc@xxxxxxxxxxxxxxx
> > Subject: RE: [PATCH v16] pwm: Add Renesas RZ/G2L MTU3a PWM driver
> >
> > Hi Uwe,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v16] pwm: Add Renesas RZ/G2L MTU3a PWM driver
> > >
> > > Hello Biju,
> > >
> > > here now comes my promised review. Took a bit longer than anticipated,
> > > sorry for that.
> >
> > I know you are busy with "Convert to platform remove callback returning
> > void".
> >
> > >
> > > On Tue, Apr 18, 2023 at 11:20:37AM +0100, Biju Das wrote:
> > > > +static u8 rz_mtu3_pwm_calculate_prescale(struct rz_mtu3_pwm_chip
> > > *rz_mtu3,
> > > > + u64 period_cycles)
> > > > +{
> > > > + u32 prescaled_period_cycles;
> > > > + u8 prescale;
> > > > +
> > > > + prescaled_period_cycles = period_cycles >> 16;
> > > > + if (prescaled_period_cycles >= 16)
> > > > + prescale = 3;
> > > > + else
> > > > + prescale = (fls(prescaled_period_cycles) + 1) / 2;
> > > > +
> > > > + return prescale;
> > >
> > > That value is supposed to be written to RZ_MTU3_TCR_TPCS, right? This
> > > is a 3bit register field and in .get_state() you handle values up to
> > > 7. I wonder why the max here is 3 only.
> >
> > I thought, for the initial basic driver, support bit values {0, 1, 2, 3}
> > as It is same for all MTU channels and later plan to support the
> > complicated external and internal clocks as it different for each
> > channels.
>
> Shall I add this in limitations section, so that limitation is clear to everyone?? And then send V17,
> as it is only the open point.
I'd put it in a comment in rz_mtu3_pwm_calculate_prescale, for the
general overview it's not that important I guess.
> I am planning to change non-error check version
> "pm_runtime_get_sync" to error check version "pm_runtime_resume_and_get()" as you suggested.
I didn't check the details here, but note that in .remove()
pm_runtime_resume_and_get() is probably wrong. See for example
commit 9496fffcb28f39e0352779a0199b6e61861c9221.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
