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

[Andrei Cherechesu <andrei.cherechesu@xxxxxxxxxxx>]

Hi Julien,

On 21/03/2023 18:56, Julien Grall wrote:
> 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.
> 

I did not necessarily see it as a name change, but rather as adding a
more generic helper to be used across the codebase, and the "_optional"
suffix did not seem a good fit since it is an alternative to
"platform_get_irq" functionally, and I tried to keep the naming
convention. I will have it in mind for future series.

I agree with "np" being const, I saw you have already submitted a patch
to change it.


>> +{
>> +    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.

The existing helper was only visible and used within the scope of
smmu-v3.c, so changing it was not a big impact. I agree that it is not
obvious, though, and I will mention something like that in future series.

Thank you for reviewing it.

Andrei

> 
> Cheers,
>