| Message ID | 1314628177-3193-1-git-send-email-svenkatr@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 29 Aug 2011, Venkatraman S wrote: > FIQ_START is an platform specific constant used in the > implementation of enable_fiq and disable_fiq. > Converting then to inline functions enables different > architectures which use the FIQ module to exist in a single > zImage with different values of FIQ_START. > > Signed-off-by: Venkatraman S <svenkatr@ti.com> I fail to see how this patch inproves things. The asm/fiq.h file is just as global as kernel/fiq.c is. > --- > arch/arm/include/asm/fiq.h | 13 +++++++++++-- > arch/arm/kernel/fiq.c | 11 ----------- > 2 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/arch/arm/include/asm/fiq.h b/arch/arm/include/asm/fiq.h > index d493d0b..e84a8cc 100644 > --- a/arch/arm/include/asm/fiq.h > +++ b/arch/arm/include/asm/fiq.h > @@ -17,6 +17,7 @@ > #define __ASM_FIQ_H > > #include <asm/ptrace.h> > +#include <asm/irq.h> > > struct fiq_handler { > struct fiq_handler *next; > @@ -36,8 +37,16 @@ struct fiq_handler { > extern int claim_fiq(struct fiq_handler *f); > extern void release_fiq(struct fiq_handler *f); > extern void set_fiq_handler(void *start, unsigned int length); > -extern void enable_fiq(int fiq); > -extern void disable_fiq(int fiq); > + > +static inline void enable_fiq(int fiq) > +{ > + enable_irq(fiq + FIQ_START); > +} > + > +static inline void disable_fiq(int fiq) > +{ > + disable_irq(fiq + FIQ_START); > +} > > /* helpers defined in fiqasm.S: */ > extern void __set_fiq_regs(unsigned long const *regs); > diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c > index 4c164ec..49e5355 100644 > --- a/arch/arm/kernel/fiq.c > +++ b/arch/arm/kernel/fiq.c > @@ -122,23 +122,12 @@ void release_fiq(struct fiq_handler *f) > while (current_fiq->fiq_op(current_fiq->dev_id, 0)); > } > > -void enable_fiq(int fiq) > -{ > - enable_irq(fiq + FIQ_START); > -} > - > -void disable_fiq(int fiq) > -{ > - disable_irq(fiq + FIQ_START); > -} > > EXPORT_SYMBOL(set_fiq_handler); > EXPORT_SYMBOL(__set_fiq_regs); /* defined in fiqasm.S */ > EXPORT_SYMBOL(__get_fiq_regs); /* defined in fiqasm.S */ > EXPORT_SYMBOL(claim_fiq); > EXPORT_SYMBOL(release_fiq); > -EXPORT_SYMBOL(enable_fiq); > -EXPORT_SYMBOL(disable_fiq); > > void __init init_FIQ(void) > { > -- > 1.7.1 >
On Mon, Aug 29, 2011 at 8:44 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > On Mon, 29 Aug 2011, Venkatraman S wrote: > >> FIQ_START is an platform specific constant used in the >> implementation of enable_fiq and disable_fiq. >> Converting then to inline functions enables different >> architectures which use the FIQ module to exist in a single >> zImage with different values of FIQ_START. >> >> Signed-off-by: Venkatraman S <svenkatr@ti.com> > > I fail to see how this patch inproves things. The asm/fiq.h file is just > as global as kernel/fiq.c is. > Argh!! I intended this to be a macro like this #define enable_fiq(x) enable_irq((x) + FIQ_START) so that the constant is needed only at places where it is expanded. This way different platforms can have their local irqs.h supply the constant, where the macros are used. Somehow I thought inline functions are more readable and assumed the same macro'ish behaviour. Will a macro variant of this be more useful ? Thanks, Venkat.
On Mon, 29 Aug 2011, S, Venkatraman wrote: > On Mon, Aug 29, 2011 at 8:44 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > > On Mon, 29 Aug 2011, Venkatraman S wrote: > > > >> FIQ_START is an platform specific constant used in the > >> implementation of enable_fiq and disable_fiq. > >> Converting then to inline functions enables different > >> architectures which use the FIQ module to exist in a single > >> zImage with different values of FIQ_START. > >> > >> Signed-off-by: Venkatraman S <svenkatr@ti.com> > > > > I fail to see how this patch inproves things. The asm/fiq.h file is just > > as global as kernel/fiq.c is. > > > > Argh!! I intended this to be a macro like this > #define enable_fiq(x) enable_irq((x) + FIQ_START) > so that the constant is needed only at places where it is expanded. > > This way different platforms can have their local irqs.h supply the constant, > where the macros are used. > > Somehow I thought inline functions are more readable and assumed the > same macro'ish behaviour. > > Will a macro variant of this be more useful ? Well... I think that, given that this FIQ_START is not widely used/defined anyway, it would probably make sense to simply get rid of it and fold its value directly in the actual FIQ definition being used, directly in irqs.h. For example, arch/arm/mach-rpc/include/mach/irqs.h could look like: #define FIQ_START 64 #define FIQ_FLOPPYDATA (FIQ_START + 0) #define FIQ_ECONET (FIQ_START + 2) #define FIQ_SERIALPORT (FIQ_START + 4) #define FIQ_EXPANSIONCARD (FIQ_START + 6) #define FIQ_FORCE (FIQ_START + 7) And then FIQ_START wouldn't have to be used anywhere else. But looking at the whole source tree I still fail to see the usefulness of this FIQ_START symbol, meaning that it could perhaps simply be removed outright. Nicolas
diff --git a/arch/arm/include/asm/fiq.h b/arch/arm/include/asm/fiq.h index d493d0b..e84a8cc 100644 --- a/arch/arm/include/asm/fiq.h +++ b/arch/arm/include/asm/fiq.h @@ -17,6 +17,7 @@ #define __ASM_FIQ_H #include <asm/ptrace.h> +#include <asm/irq.h> struct fiq_handler { struct fiq_handler *next; @@ -36,8 +37,16 @@ struct fiq_handler { extern int claim_fiq(struct fiq_handler *f); extern void release_fiq(struct fiq_handler *f); extern void set_fiq_handler(void *start, unsigned int length); -extern void enable_fiq(int fiq); -extern void disable_fiq(int fiq); + +static inline void enable_fiq(int fiq) +{ + enable_irq(fiq + FIQ_START); +} + +static inline void disable_fiq(int fiq) +{ + disable_irq(fiq + FIQ_START); +} /* helpers defined in fiqasm.S: */ extern void __set_fiq_regs(unsigned long const *regs); diff --git a/arch/arm/kernel/fiq.c b/arch/arm/kernel/fiq.c index 4c164ec..49e5355 100644 --- a/arch/arm/kernel/fiq.c +++ b/arch/arm/kernel/fiq.c @@ -122,23 +122,12 @@ void release_fiq(struct fiq_handler *f) while (current_fiq->fiq_op(current_fiq->dev_id, 0)); } -void enable_fiq(int fiq) -{ - enable_irq(fiq + FIQ_START); -} - -void disable_fiq(int fiq) -{ - disable_irq(fiq + FIQ_START); -} EXPORT_SYMBOL(set_fiq_handler); EXPORT_SYMBOL(__set_fiq_regs); /* defined in fiqasm.S */ EXPORT_SYMBOL(__get_fiq_regs); /* defined in fiqasm.S */ EXPORT_SYMBOL(claim_fiq); EXPORT_SYMBOL(release_fiq); -EXPORT_SYMBOL(enable_fiq); -EXPORT_SYMBOL(disable_fiq); void __init init_FIQ(void) {
FIQ_START is an platform specific constant used in the implementation of enable_fiq and disable_fiq. Converting then to inline functions enables different architectures which use the FIQ module to exist in a single zImage with different values of FIQ_START. Signed-off-by: Venkatraman S <svenkatr@ti.com> --- arch/arm/include/asm/fiq.h | 13 +++++++++++-- arch/arm/kernel/fiq.c | 11 ----------- 2 files changed, 11 insertions(+), 13 deletions(-)