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

["Lad, Prabhakar" <prabhakar.csengg@gmail.com>]

Hi Niklas,

On Thu, Jan 6, 2022 at 3:29 PM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> Hi Lad,
>
> On 2022-01-05 19:25:25 +0000, Lad, Prabhakar wrote:
> > Hi Niklas,
> >
> > Thank you for the review.
> >
> > On Wed, Jan 5, 2022 at 7:13 PM Niklas Söderlund
> > <niklas.soderlund@ragnatech.se> wrote:
> > >
> > > Hi Lad,
> > >
> > > Thanks for your work.
> > >
> > > On 2022-01-04 14:52:11 +0000, 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>
> > > > ---
> > > > v2-v3:
> > > > * Fixed review comment pointed by Andy
> > > >
> > > > v1->v2
> > > > * Simplified checking error code
> > > > * Break loop earlier if no interrupts are seen
> > > >
> > > > v1: https://lkml.org/lkml/2021/12/18/163
> > > > ---
> > > >  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..e480f7290ccf 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 < 0 && irq != -ENXIO) {
> > > > +                     ret = irq;
> > > > +                     goto error_unregister;
> > > > +             }
> > > > +             if (!irq || irq == -ENXIO)
> > > > +                     break;
> > >
> > > This do not look correct and differs form v1.
> > >
> > > In the old code if we can't get an IRQ the loop is continued. This is
> > > used to detect if interrupts are supported or not on the platform.  This
> > > change will fail on all systems that don't describes interrupts in DT
> > > while the driver can function without interrupts.
> > >
> > There are no non-DT users for this driver. Do you see this driver
> > being used in a non-DT environment in near future?
>
> No, maybe I was unclear sorry about that. What I intended to say was
> that this change will break platforms that that make use of this driver
> but do not describe interrupts in its DT description. As with this
> change not describing interrupts is consider an error.
>
> For example checkout thermal@ffc48000 in arch/arm/boot/dts/r8a7779.dtsi.
>
If the interrupts are missing in DT (for example in [1])
platform_get_irq_optional() will return -ENXIO with this patch this
error code is handled gracefully i.e. it doesn't return error and
breaks instead keeping the orignal behavior of the driver.

[1] arch/arm/boot/dts/r8a7779.dtsi

Cheers,
Prabhakar