Re: [PATCH v3] gpio-mlxbf2: only get IRQ for device instances 0 and 3

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Jul 22, 2025 at 04:52:02PM +0200, Bartosz Golaszewski wrote:
> On Mon, Jul 21, 2025 at 6:22 PM David Thompson <davthompson@nvidia.com> wrote:
> >
> > The gpio-mlxbf2 driver interfaces with four GPIO controllers,
> > device instances 0-3. There are two IRQ resources shared between
> > the four controllers, and they are found in the ACPI table for
> > device instances 0 and 3.  The driver should not attempt to get
> > an IRQ resource when probing device instance 1 or 2, otherwise
> > the following error is logged:
> >   mlxbf2_gpio MLNXBF22:01: error -ENXIO: IRQ index 0 not found

...

> > -       irq = platform_get_irq(pdev, 0);

So, why not simply change this to the _optional() call?

> > -       if (irq >= 0) {
> > -               girq = &gs->gc.irq;
> > -               gpio_irq_chip_set_chip(girq, &mlxbf2_gpio_irq_chip);
> > -               girq->handler = handle_simple_irq;
> > -               girq->default_type = IRQ_TYPE_NONE;
> > -               /* This will let us handle the parent IRQ in the driver */
> > -               girq->num_parents = 0;
> > -               girq->parents = NULL;
> > -               girq->parent_handler = NULL;
> > -
> > -               /*
> > -                * Directly request the irq here instead of passing
> > -                * a flow-handler because the irq is shared.
> > -                */
> > -               ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler,
> > -                                      IRQF_SHARED, name, gs);
> > -               if (ret) {
> > -                       dev_err(dev, "failed to request IRQ");
> > -                       return ret;
> > +       colon_ptr = strchr(dev_name(dev), ':');
> > +       if (!colon_ptr) {
> > +               dev_err(dev, "invalid device name format\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = kstrtol(++colon_ptr, 16, &num);
> > +       if (ret) {
> > +               dev_err(dev, "invalid device instance\n");
> > +               return ret;
> > +       }
> > +
> 
> That is *really* fragile. Andy, Mika: does this look remotely correct
> to you? I don't know much about ACPI systems.

I totally agree with you. This is an ugly hack and here is formal NAK from me.
The ACPI tables that doesn't provide an IRQ resources (in any of its possible
type) can be simply ignored by not requesting that IRQ. The message above
AFAICT is harmless. Above I proposed the better fix.

> > +       if (!num || num == 3) {
> > +               irq = platform_get_irq(pdev, 0);
> > +               if (irq >= 0) {
> > +                       girq = &gs->gc.irq;
> > +                       gpio_irq_chip_set_chip(girq, &mlxbf2_gpio_irq_chip);
> > +                       girq->handler = handle_simple_irq;
> > +                       girq->default_type = IRQ_TYPE_NONE;
> > +                       /* This will let us handle the parent IRQ in the driver */
> > +                       girq->num_parents = 0;
> > +                       girq->parents = NULL;
> > +                       girq->parent_handler = NULL;
> > +
> > +                       /*
> > +                        * Directly request the irq here instead of passing
> > +                        * a flow-handler because the irq is shared.
> > +                        */
> > +                       ret = devm_request_irq(dev, irq, mlxbf2_gpio_irq_handler,
> > +                                              IRQF_SHARED, name, gs);
> > +                       if (ret) {
> > +                               dev_err(dev, "failed to request IRQ");
> > +                               return ret;
> > +                       }
> >                 }
> >         }

-- 
With Best Regards,
Andy Shevchenko