Thanks Bartosz for looking into it!
On Wed, 2024-11-27 at 11:04 +0100, Bartosz Golaszewski wrote:
@@ -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?
Ok, I needed to take a long look at this driver.
IIUC what happens now:
In omap_gpio_irq_shutdown() and omap_gpio_free() you now enable the clock (really just bumping its enable count) before entering omap_clear_gpio_debounce() so that its internal call do clk_disable() only decreases the reference count without actually disabling it and the clock is really disabled one level up the stack.
If that's correct, isn't it unnecessarily convoluted? omap_clear_gpio_debounce() is only called from these two functions so why not just move the clk_disable() out of it and into them?
Seems that I didn't notice the elephant in the room, so let me rework it ;-)