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

["Lad, Prabhakar" <prabhakar.csengg@gmail.com>]

Hi Geert,

On Mon, Dec 20, 2021 at 12:54 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, Dec 20, 2021 at 12:56 PM Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
> > On Mon, Dec 20, 2021 at 10:18 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Sat, Dec 18, 2021 at 5:59 PM Lad Prabhakar
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com> 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?
>
> Oh, I didn't realize it used a single resource with a range.
> Is this common, i.e. would it make sense to add support for this to
> platform_get_irq_optional()?
>
No this isn't common even non dt users should ideally be passing a
single IRQ resource. There are very few such platforms which do this
so I don't see any point in adding this support to
platform_get_irq_optional() unless the IRQ maintainers think otherwise.

> > > > --- a/drivers/i2c/busses/i2c-sh_mobile.c
> > > > +++ b/drivers/i2c/busses/i2c-sh_mobile.c
>
> > > > +                       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.
>
> The Ethernet driver was converted 6 years ago, cfr. commit
> 965b2aa78fbcb831 ("net/smsc911x: fix irq resource allocation failure").
>
Thanks for the pointer.

Cheers,
Prabhakar

> > [0] https://elixir.bootlin.com/linux/v5.16-rc6/source/arch/sh/kernel/cpu/sh4a/setup-sh7724.c#L454
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> 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