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@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
Since this is a new patch, I'm unsure where was this Acked-by?
> Suggested-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> Signed-off-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
> ---
> 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
