Date:   Mon, 28 Dec 2020 18:05:07 +0300
From:   nikita.shubin@...uefel.me
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/3] gpio: ep93xx: convert to multi irqchips

26.12.2020, 20:52, "Andy Shevchenko" <andy.shevchenko@...il.com>:
> On Thu, Dec 24, 2020 at 1:23 PM Nikita Shubin <nikita.shubin@...uefel.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

Thank you, Andy, i ll rework my RFC patch according to your advices and resubmit.

Powered by blists - more mailing lists