Hi Uwe,
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Sent: Monday, April 17, 2023 11:42 PM
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Cc: linux-pwm@xxxxxxxxxxxxxxx; Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>;
> Lee Jones <lee@xxxxxxxxxx>; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>;
> William Breathitt Gray <william.gray@xxxxxxxxxx>; Daniel Lezcano
> <daniel.lezcano@xxxxxxxxxx>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@xxxxxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx; Thierry Reding
> <thierry.reding@xxxxxxxxx>; kernel@xxxxxxxxxxxxxx
> Subject: Re: [PATCH v15 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver
>
> Hello Biju,
>
> On Mon, Apr 17, 2023 at 11:06:59AM +0000, Biju Das wrote:
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v15 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM
> > > driver
> > >
> > > Hello,
> > >
> > > On Fri, Apr 14, 2023 at 09:53:09AM +0000, Biju Das wrote:
> > > > > On Thu, Mar 30, 2023 at 12:16:32PM +0100, Biju Das wrote:
> > > > > > + val = RZ_MTU3_TCR_CKEG_RISING | prescale;
> > > > > > + if (priv->map->channel == pwm->hwpwm) {
> > > > > > + rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
> > > > > > + RZ_MTU3_TCR_CCLR_TGRA | val);
> > > > >
> > > > > If the sibling PWM on the same channel is on, you're overwriting
> > > > > its prescale value here, are you not?
> > > >
> > > > Yes, you are correct. Will cache prescale and add the below code
> > > > in rz_mtu3_pwm_config(). Is it ok?
> > > >
> > > > + * Prescalar is shared by multiple channels, so prescale can
> > > > + * NOT be modified when there are multiple channels in use
> with
> > > > + * different settings.
> > > > + */
> > > > + if (prescale != rz_mtu3_pwm->prescale[ch] && rz_mtu3_pwm-
> > > >user_count[ch] > 1)
> > > > + return -EBUSY;
> > >
> > > If the other PWM is off, you can (and should) change the prescale value.
> > > Also if the current prescale value is less than the one you want to
> > > set, you can handle that.
> > >
> >
> > You mean like below??
> >
> > static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > const struct pwm_state *state) {
> > struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
> > struct rz_mtu3_pwm_channel *priv;
> > u64 period_cycles;
> > u64 duty_cycles;
> > u8 prescale;
> > u16 pv, dc;
> > u8 val;
> > u32 ch;
> >
> > priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
> > ch = priv - rz_mtu3_pwm->channel_data;
> >
> > period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate,
> > NSEC_PER_SEC);
> > prescale = rz_mtu3_pwm_calculate_prescale(rz_mtu3_pwm,
> > period_cycles);
> >
> > /*
> > * Prescalar is shared by multiple channels, so prescale can
> > * NOT be modified when there are multiple channels in use with
> > * different settings. Modify prescalar if other PWM is off or current
> > * prescale value is less than the one we want to set.
> > */
> > if (rz_mtu3_pwm->enable_count[ch] > 1 &&
> > rz_mtu3_pwm->prescale[ch] > prescale)
> > return -EBUSY;
> >
> > pv = rz_mtu3_pwm_calculate_pv_or_dc(period_cycles, prescale);
>
> Here it starts to get wrong. If rz_mtu3_pwm->enable_count[ch] > 1 you have
> to use rz_mtu3_pwm->prescale[ch] instead of prescale.
Thanks for correcting me. Will test with these changes and send v16 for PWM alone
as core driver patch is already in Linux-next[1]
[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20230417&id=bbd4013b44e6c21b997c1fa18ee635a9f3b1c4cc
Cheers,
Biju
