Re: [PATCH v2] thermal: rcar_thermal: Use platform_get_irq_optional() to get the interrupt

[Lad, Prabhakar]

Hi Andy,

Thank you for the review.

On Sat, Dec 25, 2021 at 11:41 AM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
>
>
> On Friday, December 24, 2021, 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.
>>
>> 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@xxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxx/#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
>
>