Hi Uwe,
Thanks for the feedback.
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx>
> Sent: Friday, October 4, 2024 1:47 PM
> Subject: Re: [PATCH v21 3/4] pwm: Add support for RZ/G2L GPT
>
> Hello Biju,
>
> On Fri, Oct 04, 2024 at 09:53:08AM +0000, Biju Das wrote:
> > On Wed, Sep 25, Uwe Kleine-König wrote:
> > > On Thu, Aug 08, 2024 at 02:16:19PM +0100, Biju Das wrote:
> > > > +static int rzg2l_gpt_request(struct pwm_chip *chip, struct
> > > > +pwm_device
> > > > +*pwm) {
> > > > + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > > > + u32 ch = RZG2L_GET_CH(pwm->hwpwm);
> > > > +
> > > > + mutex_lock(&rzg2l_gpt->lock);
> > > > + rzg2l_gpt->user_count[ch]++;
> > > > + mutex_unlock(&rzg2l_gpt->lock);
> > >
> > > Please consider using guard(mutex)(&rzg2l_gpt->lock);
> >
> > Agreed. expect rzg2l_gpt_apply() as it will cause deadlock as
> > rzg2l_gpt_enable acquires same lock.
>
> Note there is scoped_guard() if you don't want to hold the lock for the whole function but only for a
> block. Regarding rzg2l_gpt_apply() calling
> rzg2l_gpt_enable(): It might make sense to shift the semantic of
> rzg2l_gpt_enable() to expect the caller to hold the lock already. This way you won't release the lock
> just to allow a called function to retake it. This is usually also safer, consider someone manages to
> grab the lock in between.
OK, will remove the lock from rzg2l_gpt_enable().
>
> > > > + * clearing the flag will avoid errors during unbind.
> > > > + */
> > > > + if (enabled && rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm])
> > > > + rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] = false;
> > >
> > > Hmm, not 100% sure, but I think if rzg2l_gpt_config() below fails, cleaning this flag is wrong.
> > >
> > > Does rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] == true
> > > imply enabled == true? If so, the if condition can be simplified to
> > > just the right hand side of the &&. Then even an unconditional
> > > assignment works, because
> > >
> > > rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] = false;
> > >
> > > is a nop if the flag is already false.
> >
> > I am planning to drop "bootloader_enabled_channels" based on your
> > comment in probe() which simplifies the driver.
>
> If by saying "drop" you mean that you remove bootloader_enabled_channels completely from the driver,
> that is the wrong conclusion.
"bootloader_enabled_channels" is added mainly for avoiding PM unbalance for bind() followed by
Unbind(). By adding devm_clock_enabled() to make clk_get_rate() well-defined,
the clock will be always on and there is no need for PM run time calls.
Am I miss anything here??
Cheers,
Biju
