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.
OK will add a comment in rz_mtu3_pwm_calculate_prescale.
Yes, for initial basic driver support this is not important.
>
> > 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.
We are not calling pm_runtime_resume_and_get() in remove. Probably it should be ok.
Cheers,
Biju
