Re: [PATCH v3 6/9] media: hantro: add fallback handling for single irq/clk
From: Ezequiel Garcia <hidden>
Date: 2021-03-31 19:58:52
Also in:
linux-media, linux-rockchip
Hi Emil, Thanks for the patch. Looking good! A few comments below. On Wed, 2021-03-31 at 18:35 +0100, Emil Velikov wrote:
From: Emil Velikov <redacted> Currently the driver expects that each irq/clk will have a name specified. A valid point was raised by the DT maintainers - when there is a single interrupt line or clock - the names are not needed. This is handled: - clk - implicitly - ultimately we'll call of_clk_get_hw(..., 0, NULL which will get the first clock from the pmc - irq - explicitly - platform_get_irq(..., 0) To gracefully handle potential bugs, add respective WARN_ON() if we're having more than one irq/clk, yet lacking the respective names. Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Since this is a new patch, I'm unsure where was this Acked-by?
quoted hunk ↗ jump to hunk
Suggested-by: Ezequiel Garcia <redacted> Signed-off-by: Emil Velikov <redacted> --- v3 - New patch --- drivers/staging/media/hantro/hantro_drv.c | 24 +++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c index e5f200e64993..d1294eb9cd07 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c@@ -752,8 +752,16 @@ static int hantro_probe(struct platform_device *pdev)if (!vpu->clocks) return -ENOMEM; - for (i = 0; i < vpu->variant->num_clocks; i++) + for (i = 0; i < vpu->variant->num_clocks; i++) { vpu->clocks[i].id = vpu->variant->clk_names[i]; + + /* + * Warn and refuse to load if the driver has multiple clocks, + * yet they are lacking the respective names. + */ + if (WARN_ON(!vpu->variant->clk_names[i] &&
We already have more WARN_ON than we should in this driver, I would just remove this condition entirely.
quoted hunk ↗ jump to hunk
+ return -ENXIO; + } ret = devm_clk_bulk_get(&pdev->dev, vpu->variant->num_clocks, vpu->clocks); if (ret)@@ -791,7 +799,19 @@ static int hantro_probe(struct platform_device *pdev)if (!vpu->variant->irqs[i].handler) continue; - irq = platform_get_irq_byname(vpu->pdev, irq_name); + /* + * If the driver has a single IRQ, chances are there will be no + * actual name in the DT bindings. + */ + if (!irq_name) { + if (WARN_ON(i)) + return -ENXIO; + + irq_name = "default"; + irq = platform_get_irq(vpu->pdev, 0); + } else { + irq = platform_get_irq_byname(vpu->pdev, irq_name); + }
How about this instead: irq = platform_get_irq_byname(vpu->pdev, irq_name); if (irq <= 0) irq = platform_get_irq(vpu->pdev, i); (and leave the irq name in the sama5d4 code). Thanks, Ezequiel