Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to get the interrupt

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Mon, Dec 20, 2021 at 10:18 AM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Sat, Dec 18, 2021 at 5:59 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote:
> > platform_get_resource(pdev, IORESOURCE_IRQ, ..) relies on static
> > allocation of IRQ resources in DT core code, this causes an issue
> > when using hierarchical interrupt domains using "interrupts" property
> > in the node as this bypasses the hierarchical setup and messes up the
> > irq chaining.
>
> Thanks for your patch!
>
> > In preparation for removal of static setup of IRQ resource from DT core
> > code use platform_get_irq_optional() for DT users only.
>
> Why only for DT users?
> Plenty of driver code shared by Renesas ARM (DT-based) on SuperH
> (non-DT) SoCs already uses platform_get_irq_optional(), so I expect
> that to work for both.
>
For the non DT users the IRQ resource is passed as a range [0] and not
a single interrupt so I went with this approach. Is there a way I'm
missing where we could still use platform_get_irq_xyz() variants for
such cases?

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> > @@ -830,20 +830,41 @@ static void sh_mobile_i2c_release_dma(struct sh_mobile_i2c_data *pd)
> >
> >  static int sh_mobile_i2c_hook_irqs(struct platform_device *dev, struct sh_mobile_i2c_data *pd)
> >  {
> > -       struct resource *res;
> > -       resource_size_t n;
> > +       struct device_node *np = dev_of_node(&dev->dev);
> >         int k = 0, ret;
> >
> > -       while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> > -               for (n = res->start; n <= res->end; n++) {
> > -                       ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> > -                                         0, dev_name(&dev->dev), pd);
> > +       if (!np) {
> > +               struct resource *res;
> > +               resource_size_t n;
> > +
> > +               while ((res = platform_get_resource(dev, IORESOURCE_IRQ, k))) {
> > +                       for (n = res->start; n <= res->end; n++) {
> > +                               ret = devm_request_irq(&dev->dev, n, sh_mobile_i2c_isr,
> > +                                                      0, dev_name(&dev->dev), pd);
> > +                               if (ret) {
> > +                                       dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> > +                                       return ret;
> > +                               }
> > +                       }
> > +                       k++;
> > +               }
> > +       } else {
> > +               int irq;
> > +
> > +               do {
> > +                       irq = platform_get_irq_optional(dev, k);
>
> Check for irq == -ENXIO first, to simplify the checks below?
>
OK.

> > +                       if (irq <= 0 && irq != -ENXIO)
> > +                               return irq ? irq : -ENXIO;
>
> Can irq == 0 really happen?
>
> All SuperH users of the "i2c-sh_mobile" platform device use an
> evt2irq() value that is non-zero.
>
> I might have missed something, but it seems the only user of IRQ 0 on
> SuperH is smsc911x Ethernet in arch/sh/boards/board-apsh4a3a.c and
> arch/sh/boards/board-apsh4ad0a.c, which use evt2irq(0x200).
>
I'll keep that in mind if the Ethernet driver falls in the convection
patch changes.

> These should have been seeing the "0 is an invalid IRQ number"
> warning splat since it was introduced in commit a85a6c86c25be2d2
> ("driver core: platform: Clarify that IRQ 0 is invalid"). Or not:
> the rare users may not have upgraded their kernels beyond v5.8 yet...
>
Might be users have not updated their kernels.

[0] https://elixir.bootlin.com/linux/v5.16-rc6/source/arch/sh/kernel/cpu/sh4a/setup-sh7724.c#L454

Cheers,
Prabhakar
> > +                       if (irq == -ENXIO)
> > +                               break;
> > +                       ret = devm_request_irq(&dev->dev, irq, sh_mobile_i2c_isr,
> > +                                              0, dev_name(&dev->dev), pd);
> >                         if (ret) {
> > -                               dev_err(&dev->dev, "cannot request IRQ %pa\n", &n);
> > +                               dev_err(&dev->dev, "cannot request IRQ %d\n", irq);
> >                                 return ret;
> >                         }
> > -               }
> > -               k++;
> > +                       k++;
> > +               } while (irq);
> >         }
> >
> >         return k > 0 ? 0 : -ENOENT;
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds