On Tue, Nov 26, 2024 at 9:59 PM Sverdlin, Alexander alexander.sverdlin@siemens.com wrote:
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?
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?
Bartosz