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

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

[Lad Prabhakar]

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

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Hi,

Dropping usage of platform_get_resource() was agreed based on
the discussion [0].

[0] https://patchwork.kernel.org/project/linux-renesas-soc/
patch/20211209001056.29774-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar
---
 drivers/thermal/rcar_thermal.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index b49f04daaf47..e4c7bc1bf7ef 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)
+		int irq;
+
+		irq = platform_get_irq_optional(pdev, i);
+		if (irq <= 0 && irq != -ENXIO) {
+			ret = irq ? irq : -ENXIO;
+			goto error_unregister;
+		}
+		if (irq == -ENXIO)
 			continue;
+
 		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

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

[Daniel Lezcano]

On 18/12/2021 15:41, Lad Prabhakar 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().
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Hi,
> 
> Dropping usage of platform_get_resource() was agreed based on
> the discussion [0].
> 
> [0] https://patchwork.kernel.org/project/linux-renesas-soc/
> patch/20211209001056.29774-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
> 
> Cheers,
> Prabhakar
> ---
>  drivers/thermal/rcar_thermal.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index b49f04daaf47..e4c7bc1bf7ef 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)
> +		int irq;
> +
> +		irq = platform_get_irq_optional(pdev, i);
> +		if (irq <= 0 && irq != -ENXIO) {
> +			ret = irq ? irq : -ENXIO;
> +			goto error_unregister;
> +		}
> +		if (irq == -ENXIO)
>  			continue;

Why not invert the conditions?

		if (irq == -ENXIO)
			continue;

		if (irq <= 0) {
			ret = irq ? irq : -ENXIO;
			goto out_unregister;
		}


> +
>  		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 ");
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Geert Uytterhoeven]

On Mon, Dec 20, 2021 at 1:29 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 18/12/2021 15:41, Lad Prabhakar 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().
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > Hi,
> >
> > Dropping usage of platform_get_resource() was agreed based on
> > the discussion [0].
> >
> > [0] https://patchwork.kernel.org/project/linux-renesas-soc/
> > patch/20211209001056.29774-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
> >
> > Cheers,
> > Prabhakar
> > ---
> >  drivers/thermal/rcar_thermal.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> > index b49f04daaf47..e4c7bc1bf7ef 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)
> > +             int irq;
> > +
> > +             irq = platform_get_irq_optional(pdev, i);
> > +             if (irq <= 0 && irq != -ENXIO) {
> > +                     ret = irq ? irq : -ENXIO;
> > +                     goto error_unregister;
> > +             }
> > +             if (irq == -ENXIO)
> >                       continue;
>
> Why not invert the conditions?
>
>                 if (irq == -ENXIO)
>                         continue;

And this can be break.

>
>                 if (irq <= 0) {
>                         ret = irq ? irq : -ENXIO;

irq == 0 cannot happen.

>                         goto out_unregister;
>                 }

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

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

[Daniel Lezcano]

On 20/12/2021 14:48, Geert Uytterhoeven wrote:
> On Mon, Dec 20, 2021 at 1:29 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 18/12/2021 15:41, Lad Prabhakar 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().
>>>
>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>> ---
>>> Hi,
>>>
>>> Dropping usage of platform_get_resource() was agreed based on
>>> the discussion [0].
>>>
>>> [0] https://patchwork.kernel.org/project/linux-renesas-soc/
>>> patch/20211209001056.29774-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
>>>
>>> Cheers,
>>> Prabhakar
>>> ---
>>>  drivers/thermal/rcar_thermal.c | 15 +++++++++++----
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
>>> index b49f04daaf47..e4c7bc1bf7ef 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)
>>> +             int irq;
>>> +
>>> +             irq = platform_get_irq_optional(pdev, i);
>>> +             if (irq <= 0 && irq != -ENXIO) {
>>> +                     ret = irq ? irq : -ENXIO;
>>> +                     goto error_unregister;
>>> +             }
>>> +             if (irq == -ENXIO)
>>>                       continue;
>>
>> Why not invert the conditions?
>>
>>                 if (irq == -ENXIO)
>>                         continue;
> 
> And this can be break.
> 
>>
>>                 if (irq <= 0) {
>>                         ret = irq ? irq : -ENXIO;
> 
> irq == 0 cannot happen.
> 
>>                         goto out_unregister;
>>                 }

Sorry, I don't get the two comments. May be I missed something but it
seems for me the results are the same with the inverted conditions or not.

if (irq <= 0 && irq != -ENXIO)
	goto out;

if (irq == -ENXIO)
	continue;

Can be changed to:

if (irq != -ENXIO)
	if (irq <= 0)
		goto out;

if (irq == -ENXIO)
	continue;

Can be changed to:


if (irq == -ENXIO)
	continue;

if (irq != -ENXIO)
	if (irq <= 0)
		goto out;

The second condition is always true because the first condition is the
opposite of the second condition, if the second condition block is
reached, that means irq != -ENXIO, so we can remove the second condition
and that results into:

if (irq == -ENXIO)
	continue;

if (irq <= 0)
	goto out;


Did I miss your point ?



> 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
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Geert Uytterhoeven]

Hi Daniel,

On Mon, Dec 20, 2021 at 3:19 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 20/12/2021 14:48, Geert Uytterhoeven wrote:
> > On Mon, Dec 20, 2021 at 1:29 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >> On 18/12/2021 15:41, Lad Prabhakar 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().
> >>>
> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> >>> --- 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)
> >>> +             int irq;
> >>> +
> >>> +             irq = platform_get_irq_optional(pdev, i);
> >>> +             if (irq <= 0 && irq != -ENXIO) {
> >>> +                     ret = irq ? irq : -ENXIO;
> >>> +                     goto error_unregister;
> >>> +             }
> >>> +             if (irq == -ENXIO)
> >>>                       continue;
> >>
> >> Why not invert the conditions?
> >>
> >>                 if (irq == -ENXIO)
> >>                         continue;
> >
> > And this can be break.
> >
> >>
> >>                 if (irq <= 0) {
> >>                         ret = irq ? irq : -ENXIO;
> >
> > irq == 0 cannot happen.
> >
> >>                         goto out_unregister;
> >>                 }
>
> Sorry, I don't get the two comments. May be I missed something but it
> seems for me the results are the same with the inverted conditions or not.
>
> if (irq <= 0 && irq != -ENXIO)
>         goto out;
>
> if (irq == -ENXIO)
>         continue;
>
> Can be changed to:
>
> if (irq != -ENXIO)
>         if (irq <= 0)
>                 goto out;
>
> if (irq == -ENXIO)
>         continue;
>
> Can be changed to:
>
>
> if (irq == -ENXIO)
>         continue;
>
> if (irq != -ENXIO)
>         if (irq <= 0)
>                 goto out;
>
> The second condition is always true because the first condition is the
> opposite of the second condition, if the second condition block is
> reached, that means irq != -ENXIO, so we can remove the second condition
> and that results into:
>
> if (irq == -ENXIO)
>         continue;
>
> if (irq <= 0)
>         goto out;
>
>
> Did I miss your point ?

I think so, as I don't see your point, neither ;-)

I meant (a) there is no need to continue the loop when there are no
more interrupts present, and (b) irq == 0 cannot happen, so the cod
can be simplified to:

    if (irq == -ENXIO)
            break;
    if (irq < 0) {
            ret = irq;
            goto out_unregister;
    }

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

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

[Daniel Lezcano]

On 20/12/2021 15:26, Geert Uytterhoeven wrote:

[ ... ]

>>
>> if (irq == -ENXIO)
>>         continue;
>>
>> if (irq <= 0)
>>         goto out;
>>
>>
>> Did I miss your point ?
> 
> I think so, as I don't see your point, neither ;-)
> 
> I meant (a) there is no need to continue the loop when there are no
> more interrupts present, and (b) irq == 0 cannot happen, so the cod
> can be simplified to:
> 
>     if (irq == -ENXIO)
>             break;
>     if (irq < 0) {
>             ret = irq;
>             goto out_unregister;
>     }
> 

Makes sense for me now, thanks :)




-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Andy Shevchenko]

On Mon, Dec 20, 2021 at 8:08 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, Dec 20, 2021 at 3:19 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> > On 20/12/2021 14:48, Geert Uytterhoeven wrote:
> > > On Mon, Dec 20, 2021 at 1:29 PM Daniel Lezcano
> > > <daniel.lezcano@linaro.org> wrote:
> > >> On 18/12/2021 15:41, Lad Prabhakar wrote:


> > >>> +             irq = platform_get_irq_optional(pdev, i);
> > >>> +             if (irq <= 0 && irq != -ENXIO) {
> > >>> +                     ret = irq ? irq : -ENXIO;
> > >>> +                     goto error_unregister;
> > >>> +             }
> > >>> +             if (irq == -ENXIO)
> > >>>                       continue;
> > >>
> > >> Why not invert the conditions?
> > >>
> > >>                 if (irq == -ENXIO)
> > >>                         continue;
> > >
> > > And this can be break.
> > >
> > >>
> > >>                 if (irq <= 0) {
> > >>                         ret = irq ? irq : -ENXIO;
> > >
> > > irq == 0 cannot happen.

Even if it's so, it adds a burden on my shoulders in the future.

> > >>                         goto out_unregister;
> > >>                 }

> I think so, as I don't see your point, neither ;-)
>
> I meant (a) there is no need to continue the loop when there are no
> more interrupts present, and (b) irq == 0 cannot happen, so the cod
> can be simplified to:
>
>     if (irq == -ENXIO)
>             break;

This should be a better check to include 0 as no IRQ case. It will
allow the platform_get_irq_optional() API to be aligned with other
_optional() APIs.

>     if (irq < 0) {
>             ret = irq;
>             goto out_unregister;
>     }

-- 
With Best Regards,
Andy Shevchenko