| Message ID | 20211218144136.6663-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | thermal: rcar_thermal: Use platform_get_irq_optional() to get the interrupt | expand |
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 "); >
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
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 >
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
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 :)
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; > }
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 ");
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(-)