MIME-Version: 1.0
References: <20211224194649.22328-1-prabhakar.mahadev-lad.rj@bp.renesas.com> <CAHp75Vd+84qyTzPc=Tkhz2cxkVv9Y+ME64XO6btXq=RmtWUxZw@mail.gmail.com>
In-Reply-To: <CAHp75Vd+84qyTzPc=Tkhz2cxkVv9Y+ME64XO6btXq=RmtWUxZw@mail.gmail.com>
From:   "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date:   Sat, 25 Dec 2021 12:17:16 +0000
Message-ID: <CA+V-a8vC9=sXr+HqMCt808yuNcjobQPJw45JimdQg8qc0G1uPA@mail.gmail.com>
Subject: Re: [PATCH v2] thermal: rcar_thermal: Use platform_get_irq_optional()
 to get the interrupt
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        =?UTF-8?Q?Niklas_S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>,
        Rob Herring <robh+dt@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-renesas-soc@vger.kernel.org" 
        <linux-renesas-soc@vger.kernel.org>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Andy,

Thank you for the review.

On Sat, Dec 25, 2021 at 11:41 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
>
>
> On Friday, December 24, 2021, 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.
>>
>> In preparation for removal of static setup of IRQ resource from DT core
>> code use platform_get_irq_optional().
>>
>> This patch break's the for loop if there are no more interrupts instead
>> of continuing as done in previous code.
>>
>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>> ---
>> v1->v2
>> * Simplified checking error code
>> * Break loop earlier if no interrupts are seen
>> ---
>>  drivers/thermal/rcar_thermal.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
>> index b49f04daaf47..ff9c55999ddf 100644
>> --- a/drivers/thermal/rcar_thermal.c
>> +++ b/drivers/thermal/rcar_thermal.c
>> @@ -445,7 +445,7 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>>         struct rcar_thermal_common *common;
>>         struct rcar_thermal_priv *priv;
>>         struct device *dev = &pdev->dev;
>> -       struct resource *res, *irq;
>> +       struct resource *res;
>>         const struct rcar_thermal_chip *chip = of_device_get_match_data(dev);
>>         int mres = 0;
>>         int i;
>> @@ -467,9 +467,16 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>>         pm_runtime_get_sync(dev);
>>
>>         for (i = 0; i < chip->nirqs; i++) {
>> -               irq = platform_get_resource(pdev, IORESOURCE_IRQ, i);
>> -               if (!irq)
>> -                       continue;
>> +               int irq;
>> +
>> +               irq = platform_get_irq_optional(pdev, i);
>> +               if (irq == -ENXIO)
>> +                       break;
>> +               if (irq < 0) {
>> +                       ret = irq;
>> +                       goto error_unregister;
>> +               }
>
>
> In all your patches which introduce optional IRQ please change the logic to the opposite:
>
>   if (irq > 0)
>     ...we got one...
>   if (irq == -EPROBE_DEFER)
>     ...return it, if it is ever possible...
>
> With this you adding me a work.
>
The maintainer has requested me to do so [0].

[0] https://patchwork.kernel.org/project/linux-pm/patch/20211218144136.6663-1-prabhakar.mahadev-lad.rj@bp.renesas.com/#24669399

Cheers,
Prabhakar
>
>>
>> +
>>                 if (!common->base) {
>>                         /*
>>                          * platform has IRQ support.
>> @@ -487,7 +494,7 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>>                         idle = 0; /* polling delay is not needed */
>>                 }
>>
>> -               ret = devm_request_irq(dev, irq->start, rcar_thermal_irq,
>> +               ret = devm_request_irq(dev, irq, rcar_thermal_irq,
>>                                        IRQF_SHARED, dev_name(dev), common);
>>                 if (ret) {
>>                         dev_err(dev, "irq request failed\n ");
>> --
>> 2.17.1
>>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>