Re: [PATCH v15 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2782 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]