linux-kernel - Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 23 Feb 2023 13:06:15 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Asmaa Mnebhi <asmaa@...dia.com>
Cc:     "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "bgolaszewski@...libre.com" <bgolaszewski@...libre.com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

On Wed, Feb 22, 2023 at 8:40 PM Asmaa Mnebhi <asmaa@...dia.com> wrote:

First of all, please, please, fix your email client!
It's so hard to distinguish my own words from yours.

> > +static const struct irq_chip gpio_mlxbf3_irqchip = {
> > +       .name = "MLNXBF33",
> > +       .irq_set_type = mlxbf3_gpio_irq_set_type,
> > +       .irq_enable = mlxbf3_gpio_irq_enable,
> > +       .irq_disable = mlxbf3_gpio_irq_disable, };
>
> Seems missing two things (dunno if bgpio_init() helps with that):
> - IMMUTABLE flag
> - actual calls to enable and disable IRQs for internal GPIO library usage
> See other drivers how it's done. There are even plenty of patches to enable this thing separately.
>
> I saw that in other drivers only irq_enable and irq_disable are defined. Example in gpio-davinci.c:
> static struct irq_chip gpio_irqchip = {
> .name           = "GPIO",
> .irq_enable     = gpio_irq_enable,
> .irq_disable    = gpio_irq_disable,
> .irq_set_type   = gpio_irq_type,
> .flags          = IRQCHIP_SET_TYPE_MASKED,
> };
>
> Which I think is ok due to the following logic:
>
> gpiochip_add_irqchip calls
> gpiochip_set_irq_hooks which contains the following logic:
>
> if (irqchip->irq_disable) {
>                  gc->irq.irq_disable = irqchip->irq_disable;
>                  irqchip->irq_disable = gpiochip_irq_disable;
> } else {
>                  gc->irq.irq_mask = irqchip->irq_mask;
>                  irqchip->irq_mask = gpiochip_irq_mask;
> }
> if (irqchip->irq_enable) {
>                  gc->irq.irq_enable = irqchip->irq_enable;
>                  irqchip->irq_enable = gpiochip_irq_enable;
> } else {
>                  gc->irq.irq_unmask = irqchip->irq_unmask;
>                  irqchip->irq_unmask = gpiochip_irq_unmask;
> }

The chips have another flag there:

        .flags          = IRQCHIP_IMMUTABLE,
       GPIOCHIP_IRQ_RESOURCE_HELPERS,

> So it doesn’t seem like we need to define irq_mask/unmask if we have irq_enable/disable?
>
> > +       npins = MLXBF3_GPIO_MAX_PINS_PER_BLOCK;
> > +       device_property_read_u32(dev, "npins", &npins);
>
> I don't see DT bindings for this property (being added in this series). Is it already established one?
>
> Ah that’s my bad. The property should be called "ngpios" like in the DT documentation. Will fix.

And why do you need it? What's a corner case that the GPIO library
doesn't handle yet?

-- 
With Best Regards,
Andy Shevchenko