[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHp75Vc+iSt2XvpOYzwZnzX7Qg013e-E27CPjPaO-QmtferWVg@mail.gmail.com>
Date: Thu, 23 Feb 2023 13:06:15 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Asmaa Mnebhi <asmaa@...dia.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
On Wed, Feb 22, 2023 at 8:40 PM Asmaa Mnebhi <asmaa@...dia.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
Powered by blists - more mailing lists