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.
> > > + * 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.
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
