Message-ID: <042a2183-3f04-088c-1861-656de870337d@gmail.com>
Date:   Mon, 20 Dec 2021 14:53:27 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [PATCH 2/3] i2c: sh_mobile: Use platform_get_irq_optional() to
 get the interrupt
Content-Language: en-US
To:     Geert Uytterhoeven <geert@linux-m68k.org>,
        Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Nicolas Saenz Julienne <nsaenz@kernel.org>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Ray Jui <rjui@broadcom.com>,
        Scott Branden <sbranden@broadcom.com>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
        Chris Brandt <chris.brandt@renesas.com>,
        Wolfram Sang <wsa+renesas@sang-engineering.com>,
        Linux I2C <linux-i2c@vger.kernel.org>,
        linux-rpi-kernel <linux-rpi-kernel@lists.infradead.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
        Prabhakar <prabhakar.csengg@gmail.com>,
        Linux-sh list <linux-sh@vger.kernel.org>
References: <20211218165258.16716-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20211218165258.16716-3-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <CAMuHMdUg3=q7gyaVHP0XcYUOo3PQUUv8Hc8wp5faVQ+bTBpg4A@mail.gmail.com>
From:   Sergei Shtylyov <sergei.shtylyov@gmail.com>
Organization: Brain-dead Software
In-Reply-To: <CAMuHMdUg3=q7gyaVHP0XcYUOo3PQUUv8Hc8wp5faVQ+bTBpg4A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 20.12.2021 13:17, Geert Uytterhoeven 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.
> 
>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
>> --- 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?
> 
>> +                       if (irq <= 0 && irq != -ENXIO)
>> +                               return irq ? irq : -ENXIO;
> 
> Can irq == 0 really happen?

    Doesn't matter much in this case -- devm_request_irq() happily takes IRQ0. :-)

> 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).
> 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:

    Warning or no warning, 0 is still returned. :-/
    My attempt to put an end to this has stuck waiting a review from the IRQ 
people...

> the rare users may not have upgraded their kernels beyond v5.8 yet...
> 
>> +                       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@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