Hi Daniel,
On Mon, Dec 20, 2021 at 3:19 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
> On 20/12/2021 14:48, Geert Uytterhoeven wrote:
> > On Mon, Dec 20, 2021 at 1:29 PM Daniel Lezcano
> > <daniel.lezcano@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> >>> --- 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@xxxxxxxxxxxxxx
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
