Re: [PATCH v2 9/9] gpio: ep93xx: replace bools with idx for IRQ ports

From: Alexander Sverdlin <alexander.sverdlin@gmail.com>
To: Nikita Shubin <nikita.shubin@maquefel.me>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 9/9] gpio: ep93xx: replace bools with idx for IRQ ports
Date: Wed, 27 Jan 2021 12:34:23 +0100	[thread overview]
Message-ID: <409a17a00b4797b56ce6d47441c6477d34734309.camel@gmail.com> (raw)
In-Reply-To: <20210127104617.1173-10-nikita.shubin@maquefel.me>

Hello Nikita!

On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> Replace boolean values used for determining if gpiochip is IRQ
> capable
> or not with index.

What's the purpose of this patch?
It neither makes the code more effective nor does it make
the code more readable IMO. There are also some changes in the
code which were not mentioned in the commit message.

> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  drivers/gpio/gpio-ep93xx.c | 47 ++++++++++++++++++++----------------
> --
>  1 file changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 2aee13b5067d..f75a33b79504 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -44,6 +44,8 @@ struct ep93xx_gpio {
>         struct irq_chip         ic[EP93XX_GPIO_IRQ_CHIPS_NUM];
>  };
>  
> +#define EP93XX_GPIO_A_PORT_INDEX 0
> +#define EP93XX_GPIO_B_PORT_INDEX 1
>  /*
>   * F Port index in GPIOCHIP'S array is 5
>   * but we use index 2 for stored values and offsets
> @@ -291,38 +293,36 @@ static struct irq_chip ep93xx_gpio_irq_chip = {
>   * gpiolib interface for EP93xx on-chip GPIOs
>  
> *********************************************************************
> ****/
>  struct ep93xx_gpio_bank {
> +       u8              idx;
>         const char      *label;
>         int             data;
>         int             dir;
>         int             base;
> -       bool            has_irq;
> -       bool            has_hierarchical_irq;
>         unsigned int    irq_base;
>  };
>  
> -#define EP93XX_GPIO_BANK(_label, _data, _dir, _base, _has_irq,
> _has_hier, _irq_base) \
> +#define EP93XX_GPIO_BANK(_index, _label, _data, _dir, _base,
> _irq_base) \
>         {                                                       \
> +               .idx            = _index,                       \
>                 .label          = _label,                       \
>                 .data           = _data,                        \
>                 .dir            = _dir,                         \
>                 .base           = _base,                        \
> -               .has_irq        = _has_irq,                     \
> -               .has_hierarchical_irq = _has_hier,              \
>                 .irq_base       = _irq_base,                    \
>         }
>  
>  static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = {
>         /* Bank A has 8 IRQs */
> -       EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false,
> EP93XX_GPIO_A_IRQ_BASE),
> +       EP93XX_GPIO_BANK(0, "A", 0x00, 0x10, 0,
> EP93XX_GPIO_A_IRQ_BASE),
>         /* Bank B has 8 IRQs */
> -       EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false,
> EP93XX_GPIO_B_IRQ_BASE),
> -       EP93XX_GPIO_BANK("C", 0x08, 0x18, 40, false, false, 0),
> -       EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 24, false, false, 0),
> -       EP93XX_GPIO_BANK("E", 0x20, 0x24, 32, false, false, 0),
> +       EP93XX_GPIO_BANK(1, "B", 0x04, 0x14, 8,
> EP93XX_GPIO_B_IRQ_BASE),
> +       EP93XX_GPIO_BANK(2, "C", 0x08, 0x18, 40, 0),
> +       EP93XX_GPIO_BANK(3, "D", 0x0c, 0x1c, 24, 0),
> +       EP93XX_GPIO_BANK(4, "E", 0x20, 0x24, 32, 0),
>         /* Bank F has 8 IRQs */
> -       EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true,
> EP93XX_GPIO_F_IRQ_BASE),
> -       EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0),
> -       EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0),
> +       EP93XX_GPIO_BANK(5, "F", 0x30, 0x34, 16,
> EP93XX_GPIO_F_IRQ_BASE),
> +       EP93XX_GPIO_BANK(6, "G", 0x38, 0x3c, 48, 0),
> +       EP93XX_GPIO_BANK(7, "H", 0x40, 0x44, 56, 0),
>  };
>  
>  static int ep93xx_gpio_set_config(struct gpio_chip *gc, unsigned
> offset,
> @@ -356,10 +356,11 @@ static void ep93xx_init_irq_chips(struct
> ep93xx_gpio *epg)
>  }
>  
>  static int ep93xx_gpio_add_ab_irq_chip(struct platform_device *pdev,
> -                                       struct gpio_irq_chip *girq,
> +                                       struct gpio_chip *gc,
>                                         unsigned int irq_base)
>  {
>         struct device *dev = &pdev->dev;
> +       struct gpio_irq_chip *girq = &gc->irq;
>         int ab_parent_irq = platform_get_irq(pdev, 0);
>  
>         girq->parent_handler = ep93xx_gpio_ab_irq_handler;
> @@ -378,12 +379,13 @@ static int ep93xx_gpio_add_ab_irq_chip(struct
> platform_device *pdev,
>  }
>  
>  static int ep93xx_gpio_add_f_irq_chip(struct platform_device *pdev,
> -                                       struct gpio_irq_chip *girq,
> +                                       struct gpio_chip *gc,
>                                         unsigned int irq_base)
>  {
>         int gpio_irq;
>         int i;
>         struct device *dev = &pdev->dev;
> +       struct gpio_irq_chip *girq = &gc->irq;
>  
>         /*
>          * FIXME: convert this to use hierarchical IRQ support!
> @@ -397,10 +399,10 @@ static int ep93xx_gpio_add_f_irq_chip(struct
> platform_device *pdev,
>         if (!girq->parents)
>                 return -ENOMEM;
>         /* Pick resources 1..8 for these IRQs */
> -       for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {
> +       for (i = 0; i < girq->num_parents; i++) {
>                 girq->parents[i] = platform_get_irq(pdev, i + 1);
>                 gpio_irq = irq_base + i;
> -               irq_set_chip_data(gpio_irq, &epg->gc[5]);
> +               irq_set_chip_data(gpio_irq, gc);
>                 irq_set_chip_and_handler(gpio_irq,
>                                          &ep93xx_gpio_irq_chip,
>                                          handle_level_irq);
> @@ -433,21 +435,22 @@ static int ep93xx_gpio_add_bank(struct
> gpio_chip *gc,
>         gc->base = bank->base;
>  
>         girq = &gc->irq;
> -       if (bank->has_irq || bank->has_hierarchical_irq) {
> +       if (bank->irq_base != 0) {
>                 gc->set_config = ep93xx_gpio_set_config;
>                 port = ep93xx_gpio_port(epg, gc);
>                 girq->chip = &epg->ic[port];
>         }
>  
> -       if (bank->has_irq) {
> -               err = ep93xx_gpio_add_ab_irq_chip(pdev, girq, bank-
> >irq_base);
> +       if (bank->idx == EP93XX_GPIO_A_PORT_INDEX ||
> +               bank->idx == EP93XX_GPIO_B_PORT_INDEX) {
> +               err = ep93xx_gpio_add_ab_irq_chip(pdev, gc, bank-
> >irq_base);
>                 if (err)
>                         return err;
>         }
>  
>         /* Only bank F has especially funky IRQ handling */
> -       if (bank->has_hierarchical_irq) {
> -               err = ep93xx_gpio_add_f_irq_chip(pdev, girq, bank-
> >irq_base);
> +       if (bank->idx == EP93XX_GPIO_F_PORT_INDEX) {
> +               err = ep93xx_gpio_add_f_irq_chip(pdev, gc, bank-
> >irq_base);
>                 if (err)
>                         return err;
>         }

-- 
Alexander Sverdlin.