Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Jernej =?utf-8?B?xaBrcmFiZWM=?= <jernej.skrabec@gmail.com>
To:     p.zabel@pengutronix.de, Ezequiel Garcia <ezequiel@collabora.com>
Cc:     mchehab@kernel.org, gregkh@linuxfoundation.org,
        hverkuil-cisco@xs4all.nl, emil.velikov@collabora.com,
        linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] media: hantro: Fix check for single irq
Date:   Fri, 06 Aug 2021 06:44:35 +0200
Message-ID: <8101406.vZ8PxZ7URt@jernej-laptop>
In-Reply-To: <6761bb11f4554e9f9cbe468b5ff8f851c57515ef.camel@collabora.com>
References: <20210805190416.332563-1-jernej.skrabec@gmail.com> <6761bb11f4554e9f9cbe468b5ff8f851c57515ef.camel@collabora.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

Dne petek, 06. avgust 2021 ob 00:03:36 CEST je Ezequiel Garcia napisal(a):
> Hi Jernej,
> 
> On Thu, 2021-08-05 at 21:04 +0200, Jernej Skrabec wrote:
> > Some cores use only one interrupt and in such case interrupt name in DT
> > is not needed. Driver supposedly accounted that, but due to the wrong
> > field check it never worked. Fix that.
> > 
> > Fixes: 18d6c8b7b4c9 ("media: hantro: add fallback handling for single
> > irq/clk") Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> >  drivers/staging/media/hantro/hantro_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c
> > b/drivers/staging/media/hantro/hantro_drv.c index
> > 8a2edd67f2c6..20e508158871 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -919,7 +919,7 @@ static int hantro_probe(struct platform_device *pdev)
> >                 if (!vpu->variant->irqs[i].handler)
> >                         continue;
> >  
> > -               if (vpu->variant->num_clocks > 1) {
> > +               if (vpu->variant->num_irqs > 1) {
> 
> Oops, thanks for spotting this.
> 
> How about this instead?

No, original solution is more robust. With solution below, you're assuming 
that irq order in driver array is same as in DT. That doesn't matter if there 
is only one name or if names match. However, if there is a typo, either in DT 
node or in driver, driver will still happily assign clock based on index and 
that might not be correct one. Even if it works out, you can easily miss that 
you have a typo. Driver doesn't tell you which irq is used, if it is 
successfully acquired.

Best regards,
Jernej

> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c
> b/drivers/staging/media/hantro/hantro_drv.c index
> 31d8449ca1d2..af7054b04155 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -918,16 +918,15 @@ static int hantro_probe(struct platform_device *pdev)
>                 if (!vpu->variant->irqs[i].handler)
>                         continue;
> 
> -               if (vpu->variant->num_clocks > 1) {
> -                       irq_name = vpu->variant->irqs[i].name;
> -                       irq = platform_get_irq_byname(vpu->pdev, irq_name);
> -               } else {
> +               irq_name = vpu->variant->irqs[i].name;
> +               irq = platform_get_irq_byname(vpu->pdev, irq_name);
> +               if (irq <= 0) {
>                         /*
> -                        * If the driver has a single IRQ, chances are there
> -                        * will be no actual name in the DT bindings. +    
>                    * Missing interrupt-names property in device tree, +    
>                    * looking up interrupts by index.
>                          */
>                         irq_name = "default";
> -                       irq = platform_get_irq(vpu->pdev, 0);
> +                       irq = platform_get_irq(vpu->pdev, i);
>                 }
>                 if (irq <= 0)
>                         return -ENXIO;