| Message ID | 20240223172905.673053-1-varshini.rajendran@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 23.02.2024 19:29, Varshini Rajendran wrote: > Add support to get number of IRQs from the respective DT node for sam9x60 > and sam9x7 devices. Since only this factor differs between the two SoCs, > this patch adds support for the same. Adapt the sam9x60 dtsi > accordingly. > > Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> > --- > Changes in v4: > - Changed the implementation to fetch the NIRQs from DT as per the > comment to avoid introducing a new compatible when this is the only > difference between the SoCs related to this IP. > --- > arch/arm/boot/dts/microchip/sam9x60.dtsi | 1 + > drivers/irqchip/irq-atmel-aic5.c | 11 ++++++++--- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi > index 73d570a17269..e405f68c9f54 100644 > --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi > +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi > @@ -1201,6 +1201,7 @@ aic: interrupt-controller@fffff100 { > interrupt-controller; > reg = <0xfffff100 0x100>; > atmel,external-irqs = <31>; > + microchip,nr-irqs = <50>; > }; > > dbgu: serial@fffff200 { > diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c > index 145535bd7560..5d96ad8860d3 100644 > --- a/drivers/irqchip/irq-atmel-aic5.c > +++ b/drivers/irqchip/irq-atmel-aic5.c > @@ -398,11 +398,16 @@ static int __init sama5d4_aic5_of_init(struct device_node *node, > } > IRQCHIP_DECLARE(sama5d4_aic5, "atmel,sama5d4-aic", sama5d4_aic5_of_init); > > -#define NR_SAM9X60_IRQS 50 > - > static int __init sam9x60_aic5_of_init(struct device_node *node, > struct device_node *parent) > { > - return aic5_of_init(node, parent, NR_SAM9X60_IRQS); > + int ret, nr_irqs; > + > + ret = of_property_read_u32(node, "microchip,nr-irqs", &nr_irqs); > + if (ret) { > + pr_err("Not found microchip,nr-irqs property\n"); This breaks the ABI. You should ensure old device trees are still working with this patch. > + return ret; > + } > + return aic5_of_init(node, parent, nr_irqs); > } > IRQCHIP_DECLARE(sam9x60_aic5, "microchip,sam9x60-aic", sam9x60_aic5_of_init);
On 03/03/24 5:51 pm, claudiu beznea wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 23.02.2024 19:29, Varshini Rajendran wrote: >> Add support to get number of IRQs from the respective DT node for sam9x60 >> and sam9x7 devices. Since only this factor differs between the two SoCs, >> this patch adds support for the same. Adapt the sam9x60 dtsi >> accordingly. >> >> Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> >> --- >> Changes in v4: >> - Changed the implementation to fetch the NIRQs from DT as per the >> comment to avoid introducing a new compatible when this is the only >> difference between the SoCs related to this IP. >> --- >> arch/arm/boot/dts/microchip/sam9x60.dtsi | 1 + >> drivers/irqchip/irq-atmel-aic5.c | 11 ++++++++--- >> 2 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi >> index 73d570a17269..e405f68c9f54 100644 >> --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi >> +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi >> @@ -1201,6 +1201,7 @@ aic: interrupt-controller@fffff100 { >> interrupt-controller; >> reg = <0xfffff100 0x100>; >> atmel,external-irqs = <31>; >> + microchip,nr-irqs = <50>; >> }; >> >> dbgu: serial@fffff200 { >> diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c >> index 145535bd7560..5d96ad8860d3 100644 >> --- a/drivers/irqchip/irq-atmel-aic5.c >> +++ b/drivers/irqchip/irq-atmel-aic5.c >> @@ -398,11 +398,16 @@ static int __init sama5d4_aic5_of_init(struct device_node *node, >> } >> IRQCHIP_DECLARE(sama5d4_aic5, "atmel,sama5d4-aic", sama5d4_aic5_of_init); >> >> -#define NR_SAM9X60_IRQS 50 >> - >> static int __init sam9x60_aic5_of_init(struct device_node *node, >> struct device_node *parent) >> { >> - return aic5_of_init(node, parent, NR_SAM9X60_IRQS); >> + int ret, nr_irqs; >> + >> + ret = of_property_read_u32(node, "microchip,nr-irqs", &nr_irqs); >> + if (ret) { >> + pr_err("Not found microchip,nr-irqs property\n"); > > This breaks the ABI. You should ensure old device trees are still working > with this patch. The only older device that uses this API is sam9x60 and the newly added sam9x7. This change has been tested to be working fine in both the devices. If you still want me to use the macros as a fallback in the failure case I can do it. But this change was proposed to avoid adding macros in the first place. I can remove the error check just like they do while getting other device tree properties. Or if this is just a concern of the old devices working with the new change, then sam9x60 works. Please let me know how to proceed. > >> + return ret; >> + } >> + return aic5_of_init(node, parent, nr_irqs); >> } >> IRQCHIP_DECLARE(sam9x60_aic5, "microchip,sam9x60-aic", sam9x60_aic5_of_init);
On Fri, Mar 08, 2024 at 08:50:43AM +0000, Varshini.Rajendran@microchip.com wrote: > On 03/03/24 5:51 pm, claudiu beznea wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On 23.02.2024 19:29, Varshini Rajendran wrote: > >> Add support to get number of IRQs from the respective DT node for sam9x60 > >> and sam9x7 devices. Since only this factor differs between the two SoCs, > >> this patch adds support for the same. Adapt the sam9x60 dtsi > >> accordingly. > >> > >> Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> > >> --- > >> Changes in v4: > >> - Changed the implementation to fetch the NIRQs from DT as per the > >> comment to avoid introducing a new compatible when this is the only > >> difference between the SoCs related to this IP. > >> --- > >> arch/arm/boot/dts/microchip/sam9x60.dtsi | 1 + > >> drivers/irqchip/irq-atmel-aic5.c | 11 ++++++++--- Driver and binding changes should be in different patches. Having them in the same patch is usually a red flag for ABI breakage. > >> 2 files changed, 9 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi > >> index 73d570a17269..e405f68c9f54 100644 > >> --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi > >> +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi > >> @@ -1201,6 +1201,7 @@ aic: interrupt-controller@fffff100 { > >> interrupt-controller; > >> reg = <0xfffff100 0x100>; > >> atmel,external-irqs = <31>; > >> + microchip,nr-irqs = <50>; > >> }; > >> > >> dbgu: serial@fffff200 { > >> diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c > >> index 145535bd7560..5d96ad8860d3 100644 > >> --- a/drivers/irqchip/irq-atmel-aic5.c > >> +++ b/drivers/irqchip/irq-atmel-aic5.c > >> @@ -398,11 +398,16 @@ static int __init sama5d4_aic5_of_init(struct device_node *node, > >> } > >> IRQCHIP_DECLARE(sama5d4_aic5, "atmel,sama5d4-aic", sama5d4_aic5_of_init); > >> > >> -#define NR_SAM9X60_IRQS 50 > >> - > >> static int __init sam9x60_aic5_of_init(struct device_node *node, > >> struct device_node *parent) > >> { > >> - return aic5_of_init(node, parent, NR_SAM9X60_IRQS); > >> + int ret, nr_irqs; > >> + > >> + ret = of_property_read_u32(node, "microchip,nr-irqs", &nr_irqs); > >> + if (ret) { > >> + pr_err("Not found microchip,nr-irqs property\n"); > > > > This breaks the ABI. You should ensure old device trees are still working > > with this patch. > > The only older device that uses this API is sam9x60 and the newly added > sam9x7. This change has been tested to be working fine in both the > devices. Does it still work for a sam9x60 that does not have "microchip,nr-irqs"? I can't see how it would, because you remove the define and return an error. That's a pretty clear ABI breakage to me and I don't think it is justified. > If you still want me to use the macros as a fallback in the > failure case I can do it. But this change was proposed to avoid adding > macros in the first place. I can remove the error check just like they > do while getting other device tree properties. Or if this is just a > concern of the old devices working with the new change, then sam9x60 > works. Please let me know how to proceed. I just noticed that this property is not documented in a binding. The first thing you would will be asked when trying to add that is "why can this not be determined based on the compatible", which means back to having a define in the driver. That said, having specific $soc_aic5_of_init() functions for each SoC seems silly when usually only the number of interrupts changes. The number of IRQs could be in the match data and you could use aic5_of_init in your IRQCHIP_DECLARE directly. > >> + return ret; > >> + } > >> + return aic5_of_init(node, parent, nr_irqs); > >> } > >> IRQCHIP_DECLARE(sam9x60_aic5, "microchip,sam9x60-aic", sam9x60_aic5_of_init); > > -- > Thanks and Regards, > Varshini Rajendran. >
On 08.03.2024 10:50, Varshini.Rajendran@microchip.com wrote: > On 03/03/24 5:51 pm, claudiu beznea wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 23.02.2024 19:29, Varshini Rajendran wrote: >>> Add support to get number of IRQs from the respective DT node for sam9x60 >>> and sam9x7 devices. Since only this factor differs between the two SoCs, >>> this patch adds support for the same. Adapt the sam9x60 dtsi >>> accordingly. >>> >>> Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> >>> --- >>> Changes in v4: >>> - Changed the implementation to fetch the NIRQs from DT as per the >>> comment to avoid introducing a new compatible when this is the only >>> difference between the SoCs related to this IP. >>> --- >>> arch/arm/boot/dts/microchip/sam9x60.dtsi | 1 + >>> drivers/irqchip/irq-atmel-aic5.c | 11 ++++++++--- >>> 2 files changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi >>> index 73d570a17269..e405f68c9f54 100644 >>> --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi >>> +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi >>> @@ -1201,6 +1201,7 @@ aic: interrupt-controller@fffff100 { >>> interrupt-controller; >>> reg = <0xfffff100 0x100>; >>> atmel,external-irqs = <31>; >>> + microchip,nr-irqs = <50>; >>> }; >>> >>> dbgu: serial@fffff200 { >>> diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c >>> index 145535bd7560..5d96ad8860d3 100644 >>> --- a/drivers/irqchip/irq-atmel-aic5.c >>> +++ b/drivers/irqchip/irq-atmel-aic5.c >>> @@ -398,11 +398,16 @@ static int __init sama5d4_aic5_of_init(struct device_node *node, >>> } >>> IRQCHIP_DECLARE(sama5d4_aic5, "atmel,sama5d4-aic", sama5d4_aic5_of_init); >>> >>> -#define NR_SAM9X60_IRQS 50 >>> - >>> static int __init sam9x60_aic5_of_init(struct device_node *node, >>> struct device_node *parent) >>> { >>> - return aic5_of_init(node, parent, NR_SAM9X60_IRQS); >>> + int ret, nr_irqs; >>> + >>> + ret = of_property_read_u32(node, "microchip,nr-irqs", &nr_irqs); >>> + if (ret) { >>> + pr_err("Not found microchip,nr-irqs property\n"); >> >> This breaks the ABI. You should ensure old device trees are still working >> with this patch. > > The only older device that uses this API is sam9x60 and the newly added > sam9x7. This change has been tested to be working fine in both the > devices. As Conor explained, the code in this patch should work with device trees from previous kernel releases (thus not having microchip,nr-irqs DT binding). > If you still want me to use the macros as a fallback in the > failure case I can do it. But this change was proposed to avoid adding > macros in the first place. I can remove the error check just like they > do while getting other device tree properties. Or if this is just a > concern of the old devices working with the new change, then sam9x60 > works. Please let me know how to proceed. >> >>> + return ret; >>> + } >>> + return aic5_of_init(node, parent, nr_irqs); >>> } >>> IRQCHIP_DECLARE(sam9x60_aic5, "microchip,sam9x60-aic", sam9x60_aic5_of_init); >
diff --git a/arch/arm/boot/dts/microchip/sam9x60.dtsi b/arch/arm/boot/dts/microchip/sam9x60.dtsi index 73d570a17269..e405f68c9f54 100644 --- a/arch/arm/boot/dts/microchip/sam9x60.dtsi +++ b/arch/arm/boot/dts/microchip/sam9x60.dtsi @@ -1201,6 +1201,7 @@ aic: interrupt-controller@fffff100 { interrupt-controller; reg = <0xfffff100 0x100>; atmel,external-irqs = <31>; + microchip,nr-irqs = <50>; }; dbgu: serial@fffff200 { diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c index 145535bd7560..5d96ad8860d3 100644 --- a/drivers/irqchip/irq-atmel-aic5.c +++ b/drivers/irqchip/irq-atmel-aic5.c @@ -398,11 +398,16 @@ static int __init sama5d4_aic5_of_init(struct device_node *node, } IRQCHIP_DECLARE(sama5d4_aic5, "atmel,sama5d4-aic", sama5d4_aic5_of_init); -#define NR_SAM9X60_IRQS 50 - static int __init sam9x60_aic5_of_init(struct device_node *node, struct device_node *parent) { - return aic5_of_init(node, parent, NR_SAM9X60_IRQS); + int ret, nr_irqs; + + ret = of_property_read_u32(node, "microchip,nr-irqs", &nr_irqs); + if (ret) { + pr_err("Not found microchip,nr-irqs property\n"); + return ret; + } + return aic5_of_init(node, parent, nr_irqs); } IRQCHIP_DECLARE(sam9x60_aic5, "microchip,sam9x60-aic", sam9x60_aic5_of_init);
Add support to get number of IRQs from the respective DT node for sam9x60 and sam9x7 devices. Since only this factor differs between the two SoCs, this patch adds support for the same. Adapt the sam9x60 dtsi accordingly. Signed-off-by: Varshini Rajendran <varshini.rajendran@microchip.com> --- Changes in v4: - Changed the implementation to fetch the NIRQs from DT as per the comment to avoid introducing a new compatible when this is the only difference between the SoCs related to this IP. --- arch/arm/boot/dts/microchip/sam9x60.dtsi | 1 + drivers/irqchip/irq-atmel-aic5.c | 11 ++++++++--- 2 files changed, 9 insertions(+), 3 deletions(-)