Hi Uwe,
> -----Original Message-----
> From: Biju Das
> Sent: Friday, October 4, 2024 2:48 PM
> Subject: RE: [PATCH v21 3/4] pwm: Add support for RZ/G2L GPT
>
> 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??
Just to add,
previously,
a) For bootloader enabled channel case: Clock is ON till linux takes control in .apply().
b)For bootloader disabled case: Clock is OFF and turned on during enable().
Now, after introducing devm_clock_enabled():
a) For bootloader enabled channel case: Clock is ON and will stay ON till unbind/remove().
b) For bootloader disabled case: Clock is on during probe and will stay ON till unbind/remove().
Cheers,
Biju
