diff mbox series

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

Message ID 20230313130803.3499098-2-andrei.cherechesu@oss.nxp.com (mailing list archive)
State New, archived
Headers show
Series Fix ARM Generic Timer interrupt parsing | expand

Commit Message

Andrei Cherechesu March 13, 2023, 1:08 p.m. UTC 
From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

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@nxp.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
---
 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(-)

Comments

Julien Grall March 21, 2023, 4:56 p.m. UTC | #1 
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@nxp.com>
> 
> 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@nxp.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
>   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,
Andrei Cherechesu March 22, 2023, 3 p.m. UTC | #2 
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@nxp.com>
>>
>> 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@nxp.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> ---
>>   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,
>
diff mbox series

Patch

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)
+{
+	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);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index d58c5cd0bf..bfdb62b395 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -200,30 +200,6 @@  static inline void dev_iommu_priv_set(struct device *dev, void *priv)
 	fwspec->iommu_priv = priv;
 }
 
-static int platform_get_irq_byname_optional(struct device *dev,
-				const char *name)
-{
-	int index, ret;
-	struct dt_device_node *np  = dev_to_dt(dev);
-
-	if (unlikely(!name))
-		return -EINVAL;
-
-	index = dt_property_match_string(np, "interrupt-names", name);
-	if (index < 0) {
-		dev_info(dev, "IRQ %s not found\n", name);
-		return index;
-	}
-
-	ret = platform_get_irq(np, index);
-	if (ret < 0) {
-		dev_err(dev, "failed to get irq index %d\n", index);
-		return -ENODEV;
-	}
-
-	return ret;
-}
-
 /* Start of Linux SMMUv3 code */
 static bool disable_bypass = 1;
 
@@ -2434,6 +2410,7 @@  static int arm_smmu_device_probe(struct platform_device *pdev)
 	int irq, ret;
 	paddr_t ioaddr, iosize;
 	struct arm_smmu_device *smmu;
+	struct dt_device_node *np = dev_to_dt(pdev);
 
 	smmu = xzalloc(struct arm_smmu_device);
 	if (!smmu)
@@ -2451,7 +2428,7 @@  static int arm_smmu_device_probe(struct platform_device *pdev)
 	}
 
 	/* Base address */
-	ret = dt_device_get_address(dev_to_dt(pdev), 0, &ioaddr, &iosize);
+	ret = dt_device_get_address(np, 0, &ioaddr, &iosize);
 	if (ret)
 		goto out_free_smmu;
 
@@ -2484,19 +2461,19 @@  static int arm_smmu_device_probe(struct platform_device *pdev)
 
 	/* Interrupt lines */
 
-	irq = platform_get_irq_byname_optional(pdev, "combined");
+	irq = platform_get_irq_byname(np, "combined");
 	if (irq > 0)
 		smmu->combined_irq = irq;
 	else {
-		irq = platform_get_irq_byname_optional(pdev, "eventq");
+		irq = platform_get_irq_byname(np, "eventq");
 		if (irq > 0)
 			smmu->evtq.q.irq = irq;
 
-		irq = platform_get_irq_byname_optional(pdev, "priq");
+		irq = platform_get_irq_byname(np, "priq");
 		if (irq > 0)
 			smmu->priq.q.irq = irq;
 
-		irq = platform_get_irq_byname_optional(pdev, "gerror");
+		irq = platform_get_irq_byname(np, "gerror");
 		if (irq > 0)
 			smmu->gerr_irq = irq;
 	}