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

[Andy Shevchenko]

On Wed, Feb 22, 2023 at 8:40 PM Asmaa Mnebhi <asmaa@xxxxxxxxxx> 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