Hi Uwe,
> -----Original Message-----
> From: Biju Das
> Sent: Saturday, October 5, 2024 11:51 AM
> Subject: RE: [PATCH v21 3/4] pwm: Add support for RZ/G2L GPT
>
> 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().
Do you think clock should be always on to make clk_get_rate() well-defined??
In our system, we don't have OPP for peripherals, and no one can change the clock rate of PWM IP.
Currently, I use PM_runtime calls to turn on the clk and get the clock rate and turn the clk off again.
It is guaranteed in our system, there is no way clock rate can be changed for PWM IP,
Next time when you turn the clock, still the rate is guaranteed to be same.
If you are OK, I can send next version with below changes. Please let me know.
- /*
- * Probe() sets bootloader_enabled_channels. In such case,
- * 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;
-
if (!state->enabled) {
if (enabled) {
rzg2l_gpt_disable(rzg2l_gpt, pwm);
@@ -386,11 +372,18 @@ static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return ret;
}
- mutex_lock(&rzg2l_gpt->lock);
+ guard(mutex)(&rzg2l_gpt->lock);
ret = rzg2l_gpt_config(chip, pwm, state);
- mutex_unlock(&rzg2l_gpt->lock);
- if (!ret && !enabled)
- ret = rzg2l_gpt_enable(rzg2l_gpt, pwm);
+ if (!ret) {
+ if (enabled)
+ /*
+ * .probe() sets bootloader_enabled_channels. In such case,
+ * clearing the flag will avoid errors during unbind.
+ */
+ rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] = false;
+ else
+ rzg2l_gpt_enable(rzg2l_gpt, pwm);
+ }
Cheers,
Biju
