Hi Bartosz!
On Tue, 2024-11-26 at 21:37 +0100, Bartosz Golaszewski wrote:
> > @@ -647,6 +656,13 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
> > unsigned long flags;
> > unsigned offset = d->hwirq;
> >
> > + /*
> > + * Enable the clock here so that the nested clk_disable() in the
> > + * following omap_clear_gpio_debounce() is lockless
> > + */
> > + if (bank->dbck_flag)
> > + clk_enable(bank->dbck);
> > +
>
> But this looks like a functional change. You effectively bump the
> clock enable count but don't add a corresponding clk_disable() in the
> affected path. Is the clock ever actually disabled then?
>
> Am I not getting something?
Actually I though I enable and disable them in the very same function, so for the
first enable above...
>
> > raw_spin_lock_irqsave(&bank->lock, flags);
> > bank->irq_usage &= ~(BIT(offset));
> > omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
> > @@ -656,6 +672,9 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
> > omap_clear_gpio_debounce(bank, offset);
> > omap_disable_gpio_module(bank, offset);
> > raw_spin_unlock_irqrestore(&bank->lock, flags);
> > +
> > + if (bank->dbck_flag)
> > + clk_disable(bank->dbck);
^^^^^^^^^^^^^^^^^^^^^^^^
this would be the corresponding disable.
> > }
> >
> > static void omap_gpio_irq_bus_lock(struct irq_data *data)
> > @@ -827,6 +846,13 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
> > struct gpio_bank *bank = gpiochip_get_data(chip);
> > unsigned long flags;
> >
> > + /*
> > + * Enable the clock here so that the nested clk_disable() in the
> > + * following omap_clear_gpio_debounce() is lockless
> > + */
> > + if (bank->dbck_flag)
> > + clk_enable(bank->dbck);
^^^^^^^^^^^^^^^^^^^^^^
And for this second enable....
> > +
> > raw_spin_lock_irqsave(&bank->lock, flags);
> > bank->mod_usage &= ~(BIT(offset));
> > if (!LINE_USED(bank->irq_usage, offset)) {
> > @@ -836,6 +862,9 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
> > omap_disable_gpio_module(bank, offset);
> > raw_spin_unlock_irqrestore(&bank->lock, flags);
> >
> > + if (bank->dbck_flag)
> > + clk_disable(bank->dbck);
^^^^^^^^^^^^^^^^^^^^^^^
... this would be the corresponding 2nd disable.
> > +
> > pm_runtime_put(chip->parent);
> > }
> >
Aren't they paired from your PoV?
--
Alexander Sverdlin
Siemens AG
www.siemens.com
