Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201224112203.7174-1-nikita.shubin@maquefel.me> <20201224112203.7174-2-nikita.shubin@maquefel.me>
In-Reply-To: <20201224112203.7174-2-nikita.shubin@maquefel.me>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Sat, 26 Dec 2020 19:52:00 +0200
Message-ID: <CAHp75VctYjASuKeEqOQDH1k5XKSYVGMFsf+_cOCx72JtiMnBSw@mail.gmail.com>
Subject: Re: [RFC PATCH 1/3] gpio: ep93xx: convert to multi irqchips
To:     Nikita Shubin <nikita.shubin@maquefel.me>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Dec 24, 2020 at 1:23 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> Since gpiolib requires having separate irqchips for each gpiochip, we
> need to add some we definetly need a separate one for F port, and we

definitely

> could combine gpiochip A and B into one - but this will break namespace
> and logick.
>
> So despite 3 irqchips is a bit beefy we need a separate irqchip for each

is a -> being a

> interrupt capable port.
>
> - added separate irqchip for each iterrupt capable gpiochip

interrupt

> - dropped ep93xx_gpio_port (it wasn't working correct for port F anyway)
> - moved irq registers into separate struct ep93xx_irq_chip, togather

irq -> IRQ (everywhere)

together

>   with regs current state
> - reworked irq handle for ab gpiochips (through bit not tottaly sure this
>   is a correct thing to do)

ab -> AB ?

In the parentheses something like "I'm not totally sure that this is a
correct thing to do, though".

> - dropped has_irq and has_hierarchical_irq and added a simple index
>   which we rely on when adding irq's to gpiochip's

IRQs to GPIO chips

(It would be nice if you can spell check and proofread  commit
messages and comments in the code.

...

> +struct ep93xx_irq_chip {
> +       void __iomem    *int_type1;
> +       void __iomem    *int_type2;
> +       void __iomem    *eoi;
> +       void __iomem    *en;
> +       void __iomem    *debounce;
> +       void __iomem    *status;

This is a bit... overcomplicated.
Can we rather use regmap API?

> +       u8              gpio_int_unmasked;
> +       u8              gpio_int_enabled;
> +       u8              gpio_int_type1;
> +       u8              gpio_int_type2;
> +       u8              gpio_int_debounce;
> +       struct  irq_chip chip;
> +};

...

>  /* Port ordering is: A B F */
> +static const char *irq_chip_names[3]           = {"gpio-irq-a",
> +                                               "gpio-irq-b",
> +                                               "gpio-irq-f"};

Can you use better pattern, ie.
static const char * const foo[] = {
  ...
};

(there are two things: splitting per lines and additional const)?

...

> +       ab_parent_irq = platform_get_irq(pdev, 0);

Error check, please?
Also, if it's an optional resource, use platform_get_irq_optional().

> +       err = devm_request_irq(dev, ab_parent_irq,
> +                       ep93xx_ab_irq_handler,
> +                       IRQF_SHARED, eic->chip.name, gc);

> +

Redundant blank line.

> +       if (err) {
> +               dev_err(dev, "error requesting IRQ : %d\n", ab_parent_irq);
> +               return err;
> +       }

...

> +       girq->num_parents = 1;
> +       girq->parents = devm_kcalloc(dev, 1,
> +                               sizeof(*girq->parents),
> +                               GFP_KERNEL);

Can be squeezed to less amount of LOCs. Also consider to use
girq->num_parents as a parameter to devm_kcalloc().

> +       if (!girq->parents)
> +               return -ENOMEM;

...

> +       girq->handler = handle_level_irq;

Don't we want to mark them as bad by using handle_bad_irq() as default handler?

...

> +       /*
> +        * FIXME: convert this to use hierarchical IRQ support!
> +        * this requires fixing the root irqchip to be hierarchial.

hierarchical

> +        */

...

> +       girq->num_parents = 8;
> +       girq->parents = devm_kcalloc(dev, 8,
> +                                    sizeof(*girq->parents),
> +                                    GFP_KERNEL);

As per above.

> +

Redundant blank line.

> +       if (!girq->parents)
> +               return -ENOMEM;

...

> +       /* Pick resources 1..8 for these IRQs */
> +       for (i = 1; i <= 8; i++)
> +               girq->parents[i - 1] = platform_get_irq(pdev, i);

I would rather like to see i + 1 as a parameter which is much easier
to read and understand.

> +       for (i = 0; i < 8; i++) {

Also in both cases replace 8 by ARRAY_SIZE() or predefined constant.

> +               gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
> +               irq_set_chip_data(gpio_irq, gc);
> +               irq_set_chip_and_handler(gpio_irq,
> +                                       girq->chip,
> +                                       handle_level_irq);
> +               irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST);
> +       }

Okay, I see that this is in the original code. Consider them as
suggestions for additional changes.

And briefly looking into the rest of the code the recommendation is to
split this and perhaps other patches to smaller logical pieces.

Also, try to organize your series in groups each of them respectively
represents the following
1) fixes (can be backported, usually contain Fixes tag to the culprit commit);
2) preparatory refactoring patches / cleanups;
3) new features.


-- 
With Best Regards,
Andy Shevchenko