Re: [PATCH 2/3] gpio: bcm-kona: Make sure GPIO bits are unlocked when requesting IRQ

[Markus Mayer]

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>