RE: [PATCH v4 1/2] gpio: gpio-mlxbf3: Add gpio driver support

[Asmaa Mnebhi]

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