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.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
