Re: [PATCH] h8300 generic irq
At Fri, 27 Apr 2007 19:09:35 -0700, Andrew Morton wrote: > > On Thu, 26 Apr 2007 17:34:37 +0900 > Yoshinori Sato <[EMAIL PROTECTED]> wrote: > > > h8300 using generic irq handler patch. > > > > Signed-off-by: Yoshinori Sato <[EMAIL PROTECTED]> > > > > Minor things: > > > > > --- /dev/null > > +++ b/arch/h8300/kernel/irq.c > > @@ -0,0 +1,211 @@ > > +/* > > + * linux/arch/h8300/kernel/irq.c > > + * > > + * Copyright 2007 Yoshinori Sato <[EMAIL PROTECTED]> > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +/*#define DEBUG*/ > > + > > +extern unsigned long *interrupt_redirect_table; > > +extern const int h8300_saved_vectors[]; > > +extern const unsigned long h8300_trap_table[]; > > +int h8300_enable_irq_pin(unsigned int irq); > > +void h8300_disable_irq_pin(unsigned int irq); > > Please always avoid putting extern declarations into C files. Please them > in a header file which is visible tot he definition site asw well as all > callers/users. > > For something which is defined in assembly code (like > interrupt_redirect_table) it isn't so clear, because we cannot do > typechecking. But I think it's still best to include the declaration in a > header file so that we only have to declare it once. Plus it _is_ a global > symbol. I was sure. correct it. > > + > > +/* > > + * h8300 interrupt controler implementation > > + */ > > +struct irq_chip h8300irq_chip = { > > + .name = "H8300-INTC", > > + .startup= h8300_startup_irq, > > + .shutdown = h8300_shutdown_irq, > > + .enable = h8300_enable_irq, > > + .disable= h8300_disable_irq, > > + .ack= NULL, > > + .end= h8300_end_irq, > > +}; > > I think this could have static scope. I do not need to refer from the outside. I make a change in static. > > +void ack_bad_irq(unsigned int irq) > > +{ > > + printk("unexpected IRQ trap at vector %02x\n", irq); > > +} > > printks should generally have facility levels (KERN_*) > > > + panic("interrupt vector serup failed."); > > typo > > > + for ( i = 0; i < NR_IRQS; i++) { > > for (i = 0 > > > + if (i == *saved_vector) { > > + ramvec_p++; > > + saved_vector++; > > + } else { > > + if ( i < NR_TRAPS ) { > > if (i < NR_TRAPS) > > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] h8300 generic irq
At Fri, 27 Apr 2007 19:09:35 -0700, Andrew Morton wrote: On Thu, 26 Apr 2007 17:34:37 +0900 Yoshinori Sato [EMAIL PROTECTED] wrote: h8300 using generic irq handler patch. Signed-off-by: Yoshinori Sato [EMAIL PROTECTED] Minor things: --- /dev/null +++ b/arch/h8300/kernel/irq.c @@ -0,0 +1,211 @@ +/* + * linux/arch/h8300/kernel/irq.c + * + * Copyright 2007 Yoshinori Sato [EMAIL PROTECTED] + */ + +#include linux/module.h +#include linux/types.h +#include linux/kernel.h +#include linux/sched.h +#include linux/kernel_stat.h +#include linux/seq_file.h +#include linux/init.h +#include linux/random.h +#include linux/bootmem.h +#include linux/irq.h + +#include asm/system.h +#include asm/traps.h +#include asm/io.h +#include asm/setup.h +#include asm/errno.h + +/*#define DEBUG*/ + +extern unsigned long *interrupt_redirect_table; +extern const int h8300_saved_vectors[]; +extern const unsigned long h8300_trap_table[]; +int h8300_enable_irq_pin(unsigned int irq); +void h8300_disable_irq_pin(unsigned int irq); Please always avoid putting extern declarations into C files. Please them in a header file which is visible tot he definition site asw well as all callers/users. For something which is defined in assembly code (like interrupt_redirect_table) it isn't so clear, because we cannot do typechecking. But I think it's still best to include the declaration in a header file so that we only have to declare it once. Plus it _is_ a global symbol. I was sure. correct it. + +/* + * h8300 interrupt controler implementation + */ +struct irq_chip h8300irq_chip = { + .name = H8300-INTC, + .startup= h8300_startup_irq, + .shutdown = h8300_shutdown_irq, + .enable = h8300_enable_irq, + .disable= h8300_disable_irq, + .ack= NULL, + .end= h8300_end_irq, +}; I think this could have static scope. I do not need to refer from the outside. I make a change in static. +void ack_bad_irq(unsigned int irq) +{ + printk(unexpected IRQ trap at vector %02x\n, irq); +} printks should generally have facility levels (KERN_*) + panic(interrupt vector serup failed.); typo + for ( i = 0; i NR_IRQS; i++) { for (i = 0 + if (i == *saved_vector) { + ramvec_p++; + saved_vector++; + } else { + if ( i NR_TRAPS ) { if (i NR_TRAPS) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] h8300 generic irq
On Fri, Apr 27, 2007 at 07:09:35PM -0700, Andrew Morton wrote: > > +/* > > + * h8300 interrupt controler implementation > > + */ > > +struct irq_chip h8300irq_chip = { > > + .name = "H8300-INTC", > > + .startup= h8300_startup_irq, > > + .shutdown = h8300_shutdown_irq, > > + .enable = h8300_enable_irq, > > + .disable= h8300_disable_irq, > > + .ack= NULL, > > + .end= h8300_end_irq, > > +}; > > I think this could have static scope. Pluse there's not need to set .ack to NULL explicitly. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] h8300 generic irq
On Fri, Apr 27, 2007 at 07:09:35PM -0700, Andrew Morton wrote: +/* + * h8300 interrupt controler implementation + */ +struct irq_chip h8300irq_chip = { + .name = H8300-INTC, + .startup= h8300_startup_irq, + .shutdown = h8300_shutdown_irq, + .enable = h8300_enable_irq, + .disable= h8300_disable_irq, + .ack= NULL, + .end= h8300_end_irq, +}; I think this could have static scope. Pluse there's not need to set .ack to NULL explicitly. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] h8300 generic irq
On Thu, 26 Apr 2007 17:34:37 +0900 Yoshinori Sato <[EMAIL PROTECTED]> wrote: > h8300 using generic irq handler patch. > > Signed-off-by: Yoshinori Sato <[EMAIL PROTECTED]> > Minor things: > > --- /dev/null > +++ b/arch/h8300/kernel/irq.c > @@ -0,0 +1,211 @@ > +/* > + * linux/arch/h8300/kernel/irq.c > + * > + * Copyright 2007 Yoshinori Sato <[EMAIL PROTECTED]> > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +/*#define DEBUG*/ > + > +extern unsigned long *interrupt_redirect_table; > +extern const int h8300_saved_vectors[]; > +extern const unsigned long h8300_trap_table[]; > +int h8300_enable_irq_pin(unsigned int irq); > +void h8300_disable_irq_pin(unsigned int irq); Please always avoid putting extern declarations into C files. Please them in a header file which is visible tot he definition site asw well as all callers/users. For something which is defined in assembly code (like interrupt_redirect_table) it isn't so clear, because we cannot do typechecking. But I think it's still best to include the declaration in a header file so that we only have to declare it once. Plus it _is_ a global symbol. > + > +/* > + * h8300 interrupt controler implementation > + */ > +struct irq_chip h8300irq_chip = { > + .name = "H8300-INTC", > + .startup= h8300_startup_irq, > + .shutdown = h8300_shutdown_irq, > + .enable = h8300_enable_irq, > + .disable= h8300_disable_irq, > + .ack= NULL, > + .end= h8300_end_irq, > +}; I think this could have static scope. > +void ack_bad_irq(unsigned int irq) > +{ > + printk("unexpected IRQ trap at vector %02x\n", irq); > +} printks should generally have facility levels (KERN_*) > + panic("interrupt vector serup failed."); typo > + for ( i = 0; i < NR_IRQS; i++) { for (i = 0 > + if (i == *saved_vector) { > + ramvec_p++; > + saved_vector++; > + } else { > + if ( i < NR_TRAPS ) { if (i < NR_TRAPS) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] h8300 generic irq
On Thu, 26 Apr 2007 17:34:37 +0900 Yoshinori Sato [EMAIL PROTECTED] wrote: h8300 using generic irq handler patch. Signed-off-by: Yoshinori Sato [EMAIL PROTECTED] Minor things: --- /dev/null +++ b/arch/h8300/kernel/irq.c @@ -0,0 +1,211 @@ +/* + * linux/arch/h8300/kernel/irq.c + * + * Copyright 2007 Yoshinori Sato [EMAIL PROTECTED] + */ + +#include linux/module.h +#include linux/types.h +#include linux/kernel.h +#include linux/sched.h +#include linux/kernel_stat.h +#include linux/seq_file.h +#include linux/init.h +#include linux/random.h +#include linux/bootmem.h +#include linux/irq.h + +#include asm/system.h +#include asm/traps.h +#include asm/io.h +#include asm/setup.h +#include asm/errno.h + +/*#define DEBUG*/ + +extern unsigned long *interrupt_redirect_table; +extern const int h8300_saved_vectors[]; +extern const unsigned long h8300_trap_table[]; +int h8300_enable_irq_pin(unsigned int irq); +void h8300_disable_irq_pin(unsigned int irq); Please always avoid putting extern declarations into C files. Please them in a header file which is visible tot he definition site asw well as all callers/users. For something which is defined in assembly code (like interrupt_redirect_table) it isn't so clear, because we cannot do typechecking. But I think it's still best to include the declaration in a header file so that we only have to declare it once. Plus it _is_ a global symbol. + +/* + * h8300 interrupt controler implementation + */ +struct irq_chip h8300irq_chip = { + .name = H8300-INTC, + .startup= h8300_startup_irq, + .shutdown = h8300_shutdown_irq, + .enable = h8300_enable_irq, + .disable= h8300_disable_irq, + .ack= NULL, + .end= h8300_end_irq, +}; I think this could have static scope. +void ack_bad_irq(unsigned int irq) +{ + printk(unexpected IRQ trap at vector %02x\n, irq); +} printks should generally have facility levels (KERN_*) + panic(interrupt vector serup failed.); typo + for ( i = 0; i NR_IRQS; i++) { for (i = 0 + if (i == *saved_vector) { + ramvec_p++; + saved_vector++; + } else { + if ( i NR_TRAPS ) { if (i NR_TRAPS) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/