Re: [PATCH v3 1/2] arch/arm: irq: Add platform_get_irq_byname() implementation

[Julien Grall <julien@xxxxxxx>]

Hi Andrei,

I realized this has already been merged. But I would like to point out a 
few things for future series.
On 13/03/2023 13:08, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
>
> Moved implementation for the function which parses the IRQs of a DT
> node by the "interrupt-names" property from the SMMU-v3 driver
> to the IRQ core code and made it non-static to be used as helper.
>
> Also changed it to receive a "struct dt_device_node*" as parameter,
> like its counterpart, platform_get_irq(). Updated its usage inside
> the SMMU-v3 driver accordingly.
>
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
>   xen/arch/arm/include/asm/irq.h        |  2 ++
>   xen/arch/arm/irq.c                    | 14 +++++++++++
>   xen/drivers/passthrough/arm/smmu-v3.c | 35 +++++----------------------
>   3 files changed, 22 insertions(+), 29 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
> index 245f49dcba..af94f41994 100644
> --- a/xen/arch/arm/include/asm/irq.h
> +++ b/xen/arch/arm/include/asm/irq.h
> @@ -89,6 +89,8 @@ int irq_set_type(unsigned int irq, unsigned int type);
>   
>   int platform_get_irq(const struct dt_device_node *device, int index);
>   
> +int platform_get_irq_byname(struct dt_device_node *np, const char *name);
> +
>   void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
>   
>   /*
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 79718f68e7..ded495792b 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -718,6 +718,20 @@ int platform_get_irq(const struct dt_device_node *device, 
> int index)
>       return irq;
>   }
>   
> +int platform_get_irq_byname(struct dt_device_node *np, const char *name)
You are changing the name but don't really explain why. "np" also ought 
to be const as this is not meant to be modified.
> +{
> +       int index;
> +
> +       if ( unlikely(!name) )
> +               return -EINVAL;
> +
> +       index = dt_property_match_string(np, "interrupt-names", name);
> +       if ( index < 0 )
> +               return index;
> +
> +       return platform_get_irq(np, index);
The existing helper was returning -ENODEV when there is an error. But 
here, you went differently. This is the sort of thing that ought to be 
explained in the commit message because it is not obvious why you 
changed it *and* that you actually checked the callers are OK with that.
Thankfully they are all.

Cheers,

--
Julien Grall