[PATCH] media: hantro: Fix check for single irq

[PATCH] media: hantro: Fix check for single irq

[Jernej Skrabec]

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) {
 			irq_name = vpu->variant->irqs[i].name;
 			irq = platform_get_irq_byname(vpu->pdev, irq_name);
 		} else {
-- 
2.32.0

Re: [PATCH] media: hantro: Fix check for single irq

[Ezequiel Garcia]

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?

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;



-- 
Kindly,
Ezequiel

Re: [PATCH] media: hantro: Fix check for single irq

[Jernej Škrabec]

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;

Re: [PATCH] media: hantro: Fix check for single irq

[Ezequiel Garcia]

On Fri, 2021-08-06 at 06:44 +0200, Jernej Škrabec wrote:
> 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.
> 

I find it odd to iterate up to num_irqs but then
have a case for num_irqs == 1, and call
platform_get_irq(vpu->pdev, 0).

But OTOH, your fix is correct and it's a oneliner.

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks,
Ezequiel

Re: [PATCH] media: hantro: Fix check for single irq

[Jernej Škrabec]

Dne petek, 06. avgust 2021 ob 16:13:46 CEST je Ezequiel Garcia napisal(a):
> On Fri, 2021-08-06 at 06:44 +0200, Jernej Škrabec wrote:
> > 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.
> 
> I find it odd to iterate up to num_irqs but then
> have a case for num_irqs == 1, and call
> platform_get_irq(vpu->pdev, 0).

True, it was also strange for me at first, but then it's robust and simple. 
Because of that, I just fixed obvious issue.

> 
> But OTOH, your fix is correct and it's a oneliner.
> 
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> 

Thanks!

Best regards,
Jernej

> Thanks,
> Ezequiel

Re: [PATCH] media: hantro: Fix check for single irq

[Emil Velikov]

On 2021/08/05, 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>

D'oh thanks for catching that. This commit is:
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

-Emil