On Thu, 30 Jan 2025 at 09:10, Artur Weber <aweber.kernel@xxxxxxxxx> wrote: > diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c > index 77bd4ec93a231472d7bc40db9d5db12d20bb1611..eeaa921df6f072129dbdf1c73d6da2bd7c1fe716 100644 > --- a/drivers/gpio/gpio-bcm-kona.c > +++ b/drivers/gpio/gpio-bcm-kona.c > @@ -69,6 +69,22 @@ struct bcm_kona_gpio { > struct bcm_kona_gpio_bank { > int id; > int irq; > + /* > + * Used to keep track of lock/unlock operations for each GPIO in the > + * bank. > + * > + * All GPIOs are locked by default (see bcm_kona_gpio_reset), and the > + * unlock count for all GPIOs is 0 by default. Each unlock increments > + * the counter, and each lock decrements the counter. > + * > + * The lock function only locks the GPIO once its unlock counter is > + * down to 0. This is necessary because the GPIO is unlocked in two > + * places in this driver: once for requested GPIOs, and once for > + * requested IRQs. Since it is possible for a GPIO to be requested > + * as both a GPIO and an IRQ, we need to ensure that we don't lock it > + * too early. > + */ > + u8 gpio_unlock_count[GPIO_PER_BANK]; > /* Used in the interrupt handler */ > struct bcm_kona_gpio *kona_gpio; > }; > @@ -87,14 +103,25 @@ static void bcm_kona_gpio_lock_gpio(struct bcm_kona_gpio *kona_gpio, > unsigned long flags; > int bank_id = GPIO_BANK(gpio); > int bit = GPIO_BIT(gpio); > + struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id]; > > - raw_spin_lock_irqsave(&kona_gpio->lock, flags); > + if (bank->gpio_unlock_count[bit] == 0) { > + dev_err(kona_gpio->gpio_chip.parent, > + "Unbalanced locks for GPIO %u\n", gpio); > + return; > + } > > - val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id)); > - val |= BIT(bit); > - bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val); > + bank->gpio_unlock_count[bit] -= 1; Not a big deal or a show-stopper, but this could be bank->gpio_unlock_count[bit]--; or, better yet, --bank->gpio_unlock_count[bit]; And a bit further down... > - raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > + if (bank->gpio_unlock_count[bit] == 0) { > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > + > + val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id)); > + val |= BIT(bit); > + bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val); > + > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > + } > } > > static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio, > @@ -104,14 +131,20 @@ static void bcm_kona_gpio_unlock_gpio(struct bcm_kona_gpio *kona_gpio, > unsigned long flags; > int bank_id = GPIO_BANK(gpio); > int bit = GPIO_BIT(gpio); > + struct bcm_kona_gpio_bank *bank = &kona_gpio->banks[bank_id]; > > - raw_spin_lock_irqsave(&kona_gpio->lock, flags); > + if (bank->gpio_unlock_count[bit] == 0) { > + raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > - val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id)); > - val &= ~BIT(bit); > - bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val); > + val = readl(kona_gpio->reg_base + GPIO_PWD_STATUS(bank_id)); > + val &= ~BIT(bit); > + bcm_kona_gpio_write_lock_regs(kona_gpio->reg_base, bank_id, val); > > - raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > + > + raw_spin_unlock_irqrestore(&kona_gpio->lock, flags); > + } > + > + bank->gpio_unlock_count[bit] += 1; ...this could be ++bank->gpio_unlock_count[bit]; > } > > static int bcm_kona_gpio_get_dir(struct gpio_chip *chip, unsigned gpio) > @@ -362,6 +395,7 @@ static void bcm_kona_gpio_irq_mask(struct irq_data *d) > > kona_gpio = irq_data_get_irq_chip_data(d); > reg_base = kona_gpio->reg_base; > + > raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > val = readl(reg_base + GPIO_INT_MASK(bank_id)); > @@ -384,6 +418,7 @@ static void bcm_kona_gpio_irq_unmask(struct irq_data *d) > > kona_gpio = irq_data_get_irq_chip_data(d); > reg_base = kona_gpio->reg_base; > + > raw_spin_lock_irqsave(&kona_gpio->lock, flags); > > val = readl(reg_base + GPIO_INT_MSKCLR(bank_id)); > @@ -479,15 +514,25 @@ static void bcm_kona_gpio_irq_handler(struct irq_desc *desc) > static int bcm_kona_gpio_irq_reqres(struct irq_data *d) > { > struct bcm_kona_gpio *kona_gpio = irq_data_get_irq_chip_data(d); > + unsigned int gpio = d->hwirq; > > - return gpiochip_reqres_irq(&kona_gpio->gpio_chip, d->hwirq); > + /* > + * We need to unlock the GPIO before any other operations are performed > + * on the relevant GPIO configuration registers > + */ > + bcm_kona_gpio_unlock_gpio(kona_gpio, gpio); > + > + return gpiochip_reqres_irq(&kona_gpio->gpio_chip, gpio); > } > > static void bcm_kona_gpio_irq_relres(struct irq_data *d) > { > struct bcm_kona_gpio *kona_gpio = irq_data_get_irq_chip_data(d); > + unsigned int gpio = d->hwirq; Since you added a comment to bcm_kona_gpio_irq_reqres(), would it make sense to add one here too? Just another nitpick and not a big deal either way. > + bcm_kona_gpio_lock_gpio(kona_gpio, gpio); > > - gpiochip_relres_irq(&kona_gpio->gpio_chip, d->hwirq); > + gpiochip_relres_irq(&kona_gpio->gpio_chip, gpio); > } > > static struct irq_chip bcm_gpio_irq_chip = { > > -- > 2.48.1 > Since I am okay either way with regards to my three nitpicks: Reviewed-by: Markus Mayer <mmayer@xxxxxxxxxxxx>