| Message ID | 20210331173520.184191-7-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Microchip SAMA5D4 VPU support et al | expand |
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 <emil.velikov@collabora.com> > > 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? > Suggested-by: Ezequiel Garcia <ezequiel@collabora.com> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > --- > 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. > + 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
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] && i)) + 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); + } if (irq <= 0) return -ENXIO;