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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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;
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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
Thanks,
Ezequiel
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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
>
Thanks!
Best regards,
Jernej
> Thanks,
> Ezequiel
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 <[email protected]>
D'oh thanks for catching that. This commit is:
Reviewed-by: Emil Velikov <[email protected]>
-Emil