Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 23 Feb 2023 13:06:15 +0200 | 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@nvidia.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
| |