| Message ID | 1364463704-13692-2-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/28/2013 04:41 AM, Maxime Ripard wrote: > More and more sub-architectures are using only the irqchip_init > function. Make the core code call this function if no init_irq field is > provided in the machine description to remove some boilerplate code. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > arch/arm/kernel/irq.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c > index 8e4ef4c..df6f9a1 100644 > --- a/arch/arm/kernel/irq.c > +++ b/arch/arm/kernel/irq.c > @@ -26,6 +26,7 @@ > #include <linux/ioport.h> > #include <linux/interrupt.h> > #include <linux/irq.h> > +#include <linux/irqchip.h> > #include <linux/random.h> > #include <linux/smp.h> > #include <linux/init.h> > @@ -114,7 +115,10 @@ EXPORT_SYMBOL_GPL(set_irq_flags); > > void __init init_IRQ(void) > { > - machine_desc->init_irq(); > + if (machine_desc->init_irq) > + machine_desc->init_irq(); > + else > + irqchip_init(); There needs to be an empty version defined for !OF. Rob > } > > #ifdef CONFIG_MULTI_IRQ_HANDLER >
On Thu, Mar 28, 2013 at 09:48:18AM -0500, Rob Herring wrote: > On 03/28/2013 04:41 AM, Maxime Ripard wrote: > > + if (machine_desc->init_irq) > > + machine_desc->init_irq(); > > + else > > + irqchip_init(); > > There needs to be an empty version defined for !OF. Better: #ifdef CONFIG_OF if (!machine_desc->init_irq) irqchip_init(); else #endif machine_desc->init_irq(); which means we don't even get the test if !OF, and if someone mistakenly sets this to NULL for a !OF platform, they get to know about it.
On Thursday 28 March 2013, Russell King - ARM Linux wrote: > Better: > > #ifdef CONFIG_OF > if (!machine_desc->init_irq) > irqchip_init(); > else > #endif > machine_desc->init_irq(); > > which means we don't even get the test if !OF, and if someone mistakenly > sets this to NULL for a !OF platform, they get to know about it. Right. With the new IS_DEFINED() macro, we could even turn this into if (IS_DEFINED(CONFIG_OF) && !machine_desc->init_irq) irqchip_init(); else machine_desc->init_irq(); to the same effect. Arnd
On Thu, Mar 28, 2013 at 03:25:42PM +0000, Arnd Bergmann wrote: > On Thursday 28 March 2013, Russell King - ARM Linux wrote: > > Better: > > > > #ifdef CONFIG_OF > > if (!machine_desc->init_irq) > > irqchip_init(); > > else > > #endif > > machine_desc->init_irq(); > > > > which means we don't even get the test if !OF, and if someone mistakenly > > sets this to NULL for a !OF platform, they get to know about it. > > Right. With the new IS_DEFINED() macro, we could even turn this into > > if (IS_DEFINED(CONFIG_OF) && !machine_desc->init_irq) > irqchip_init(); > else > machine_desc->init_irq(); > > to the same effect. Provided irqchip_init() keeps its prototype visible, then yes.
On Thursday 28 March 2013, Russell King - ARM Linux wrote: > On Thu, Mar 28, 2013 at 03:25:42PM +0000, Arnd Bergmann wrote: > > if (IS_DEFINED(CONFIG_OF) && !machine_desc->init_irq) > > irqchip_init(); > > else > > machine_desc->init_irq(); > > > > to the same effect. > > Provided irqchip_init() keeps its prototype visible, then yes. Yes, I checked that before my reply. We have a few function declarations that are unfortunately completely hidden when some config symbol is not defined, but this is not one of them. Arnd
Le 28/03/2013 16:49, Arnd Bergmann a écrit : > On Thursday 28 March 2013, Russell King - ARM Linux wrote: >> On Thu, Mar 28, 2013 at 03:25:42PM +0000, Arnd Bergmann wrote: >>> if (IS_DEFINED(CONFIG_OF) && !machine_desc->init_irq) >>> irqchip_init(); >>> else >>> machine_desc->init_irq(); >>> >>> to the same effect. >> >> Provided irqchip_init() keeps its prototype visible, then yes. > > Yes, I checked that before my reply. We have a few function declarations > that are unfortunately completely hidden when some config symbol is not > defined, but this is not one of them. I'll send a v2 with the suggested change. Thanks! Maxime
On 03/28/2013 09:51 AM, Russell King - ARM Linux wrote: > On Thu, Mar 28, 2013 at 09:48:18AM -0500, Rob Herring wrote: >> On 03/28/2013 04:41 AM, Maxime Ripard wrote: >>> + if (machine_desc->init_irq) >>> + machine_desc->init_irq(); >>> + else >>> + irqchip_init(); >> >> There needs to be an empty version defined for !OF. > > Better: > > #ifdef CONFIG_OF > if (!machine_desc->init_irq) > irqchip_init(); > else > #endif Or the new style: if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq) irqchip_init(); else That needs the empty version, but handles your case. Rob > machine_desc->init_irq(); > > which means we don't even get the test if !OF, and if someone mistakenly > sets this to NULL for a !OF platform, they get to know about it. >
On Thu, Mar 28, 2013 at 01:40:09PM -0500, Rob Herring wrote: > On 03/28/2013 09:51 AM, Russell King - ARM Linux wrote: > > On Thu, Mar 28, 2013 at 09:48:18AM -0500, Rob Herring wrote: > >> On 03/28/2013 04:41 AM, Maxime Ripard wrote: > >>> + if (machine_desc->init_irq) > >>> + machine_desc->init_irq(); > >>> + else > >>> + irqchip_init(); > >> > >> There needs to be an empty version defined for !OF. > > > > Better: > > > > #ifdef CONFIG_OF > > if (!machine_desc->init_irq) > > irqchip_init(); > > else > > #endif > > Or the new style: > > if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq) > irqchip_init(); > else > > That needs the empty version, but handles your case. It shouldn't need the empty version at all - because the compiler should optimize the "true" case away entirely.
diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c index 8e4ef4c..df6f9a1 100644 --- a/arch/arm/kernel/irq.c +++ b/arch/arm/kernel/irq.c @@ -26,6 +26,7 @@ #include <linux/ioport.h> #include <linux/interrupt.h> #include <linux/irq.h> +#include <linux/irqchip.h> #include <linux/random.h> #include <linux/smp.h> #include <linux/init.h> @@ -114,7 +115,10 @@ EXPORT_SYMBOL_GPL(set_irq_flags); void __init init_IRQ(void) { - machine_desc->init_irq(); + if (machine_desc->init_irq) + machine_desc->init_irq(); + else + irqchip_init(); } #ifdef CONFIG_MULTI_IRQ_HANDLER
More and more sub-architectures are using only the irqchip_init function. Make the core code call this function if no init_irq field is provided in the machine description to remove some boilerplate code. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- arch/arm/kernel/irq.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)