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:   Wed, 22 Feb 2023 18:40:02 +0000
From:   Asmaa Mnebhi <asmaa@...dia.com>
To:     Andy Shevchenko <andy.shevchenko@...il.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

> +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;
}

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.

--
With Best Regards,
Andy Shevchenko