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

[Ezequiel Garcia <ezequiel@collabora.com>]

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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip