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
