diff mbox series

[v3,6/9] media: hantro: add fallback handling for single irq/clk

Message ID 20210331173520.184191-7-emil.l.velikov@gmail.com
State New
Headers show
Series Microchip SAMA5D4 VPU support et al | expand

Commit Message

Emil Velikov March 31, 2021, 5:35 p.m. UTC 
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>
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(-)

Comments

Ezequiel Garcia March 31, 2021, 7:57 p.m. UTC | #1 
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 mbox series

Patch

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;