| Message ID | 20210805190416.332563-1-jernej.skrabec@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: hantro: Fix check for single irq | expand |
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;
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;
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
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
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
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 {
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(-)