| Message ID | 1344239199-11445-3-git-send-email-monstr@monstr.eu |
|---|---|
| State | Accepted |
| Commit | 8706908a2522f7d38f916e38ee13ca91e9f30bc5 |
| Delegated to: | Mike Frysinger |
| Headers | show |
Am Montag, den 06.08.2012, 09:46 +0200 schrieb Michal Simek: > Return value to find out if un/registration was succesful. > > Signed-off-by: Michal Simek <monstr@monstr.eu> > > --- > v2: Add comment to header file to describe parameters and return codes > --- > arch/microblaze/cpu/interrupts.c | 16 +++++++++------- > arch/microblaze/include/asm/microblaze_intc.h | 11 ++++++++++- > 2 files changed, 19 insertions(+), 8 deletions(-) > > diff --git a/arch/microblaze/cpu/interrupts.c b/arch/microblaze/cpu/interrupts.c > index ee67082..08f6bad 100644 > --- a/arch/microblaze/cpu/interrupts.c > +++ b/arch/microblaze/cpu/interrupts.c > @@ -91,14 +91,13 @@ static void disable_one_interrupt(int irq) > #endif > } > > -/* adding new handler for interrupt */ > -void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, void *arg) > +int install_interrupt_handler(int irq, interrupt_handler_t *hdlr, void *arg) > { > struct irq_action *act; > /* irq out of range */ > if ((irq < 0) || (irq > irq_no)) { > puts ("IRQ out of range\n"); > - return; > + return -1; > } > act = &vecs[irq]; > if (hdlr) { /* enable */ > @@ -106,11 +105,14 @@ void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, void *arg) > act->arg = arg; > act->count = 0; > enable_one_interrupt (irq); > - } else { /* disable */ > - act->handler = (interrupt_handler_t *) def_hdlr; > - act->arg = (void *)irq; > - disable_one_interrupt (irq); > + return 0; > } > + > + /* Disable */ > + act->handler = (interrupt_handler_t *) def_hdlr; > + act->arg = (void *)irq; > + disable_one_interrupt(irq); > + return 1; > } > > /* initialization interrupt controller - hardware */ > diff --git a/arch/microblaze/include/asm/microblaze_intc.h b/arch/microblaze/include/asm/microblaze_intc.h > index 6142b9c..e9640f5 100644 > --- a/arch/microblaze/include/asm/microblaze_intc.h > +++ b/arch/microblaze/include/asm/microblaze_intc.h > @@ -39,7 +39,16 @@ struct irq_action { > int count; /* number of interrupt */ > }; > > -void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, > +/** > + * Register and unregister interrupt handler rutines > + * > + * @param irq IRQ number > + * @param hdlr Interrupt handler rutine > + * @param arg Pointer to argument which is passed to int. handler rutine > + * @return 0 if registration pass, 1 if unregistration pass, > + * or an error code < 0 otherwise > + */ > +int install_interrupt_handler(int irq, interrupt_handler_t *hdlr, > void *arg); Hi Michal, why not two different functions here, one for registration, another one for unregistration? To mee it is puzzling to use a 'install' function for unregistration ... ... whatever, you should evaluate the return code in fsl_init2() too. > > int interrupts_init(void);
On 08/07/2012 10:10 PM, Stephan Linz wrote: > Am Montag, den 06.08.2012, 09:46 +0200 schrieb Michal Simek: >> Return value to find out if un/registration was succesful. >> >> Signed-off-by: Michal Simek <monstr@monstr.eu> >> >> --- >> v2: Add comment to header file to describe parameters and return codes >> --- >> arch/microblaze/cpu/interrupts.c | 16 +++++++++------- >> arch/microblaze/include/asm/microblaze_intc.h | 11 ++++++++++- >> 2 files changed, 19 insertions(+), 8 deletions(-) >> >> diff --git a/arch/microblaze/cpu/interrupts.c b/arch/microblaze/cpu/interrupts.c >> index ee67082..08f6bad 100644 >> --- a/arch/microblaze/cpu/interrupts.c >> +++ b/arch/microblaze/cpu/interrupts.c >> @@ -91,14 +91,13 @@ static void disable_one_interrupt(int irq) >> #endif >> } >> >> -/* adding new handler for interrupt */ >> -void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, void *arg) >> +int install_interrupt_handler(int irq, interrupt_handler_t *hdlr, void *arg) >> { >> struct irq_action *act; >> /* irq out of range */ >> if ((irq < 0) || (irq > irq_no)) { >> puts ("IRQ out of range\n"); >> - return; >> + return -1; >> } >> act = &vecs[irq]; >> if (hdlr) { /* enable */ >> @@ -106,11 +105,14 @@ void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, void *arg) >> act->arg = arg; >> act->count = 0; >> enable_one_interrupt (irq); >> - } else { /* disable */ >> - act->handler = (interrupt_handler_t *) def_hdlr; >> - act->arg = (void *)irq; >> - disable_one_interrupt (irq); >> + return 0; >> } >> + >> + /* Disable */ >> + act->handler = (interrupt_handler_t *) def_hdlr; >> + act->arg = (void *)irq; >> + disable_one_interrupt(irq); >> + return 1; >> } >> >> /* initialization interrupt controller - hardware */ >> diff --git a/arch/microblaze/include/asm/microblaze_intc.h b/arch/microblaze/include/asm/microblaze_intc.h >> index 6142b9c..e9640f5 100644 >> --- a/arch/microblaze/include/asm/microblaze_intc.h >> +++ b/arch/microblaze/include/asm/microblaze_intc.h >> @@ -39,7 +39,16 @@ struct irq_action { >> int count; /* number of interrupt */ >> }; >> >> -void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, >> +/** >> + * Register and unregister interrupt handler rutines >> + * >> + * @param irq IRQ number >> + * @param hdlr Interrupt handler rutine >> + * @param arg Pointer to argument which is passed to int. handler rutine >> + * @return 0 if registration pass, 1 if unregistration pass, >> + * or an error code < 0 otherwise >> + */ >> +int install_interrupt_handler(int irq, interrupt_handler_t *hdlr, >> void *arg); > > Hi Michal, > > why not two different functions here, one for registration, another one > for unregistration? To mee it is puzzling to use a 'install' function > for unregistration ... partially agree with that. Maybe we could introduce one macro for that. #define uninstall_interrupt_handler(irq) install_interrupt_handler(irq, NULL, NULL) > ... whatever, you should evaluate the return code in fsl_init2() too. Not necessary to do it in this patch. Thanks, Michal
Am Mittwoch, den 08.08.2012, 10:27 +0200 schrieb Michal Simek: > On 08/07/2012 10:10 PM, Stephan Linz wrote: > > Am Montag, den 06.08.2012, 09:46 +0200 schrieb Michal Simek: > >> Return value to find out if un/registration was succesful. > >> > >> Signed-off-by: Michal Simek <monstr@monstr.eu> > >> > >> --- > >> v2: Add comment to header file to describe parameters and return codes > >> --- > >> arch/microblaze/cpu/interrupts.c | 16 +++++++++------- > >> arch/microblaze/include/asm/microblaze_intc.h | 11 ++++++++++- > >> 2 files changed, 19 insertions(+), 8 deletions(-) > >> > >> diff --git a/arch/microblaze/cpu/interrupts.c b/arch/microblaze/cpu/interrupts.c > >> index ee67082..08f6bad 100644 > >> --- a/arch/microblaze/cpu/interrupts.c > >> +++ b/arch/microblaze/cpu/interrupts.c > >> @@ -91,14 +91,13 @@ static void disable_one_interrupt(int irq) > >> #endif > >> } > >> > >> -/* adding new handler for interrupt */ > >> -void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, void *arg) > >> +int install_interrupt_handler(int irq, interrupt_handler_t *hdlr, void *arg) > >> { > >> struct irq_action *act; > >> /* irq out of range */ > >> if ((irq < 0) || (irq > irq_no)) { > >> puts ("IRQ out of range\n"); > >> - return; > >> + return -1; > >> } > >> act = &vecs[irq]; > >> if (hdlr) { /* enable */ > >> @@ -106,11 +105,14 @@ void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, void *arg) > >> act->arg = arg; > >> act->count = 0; > >> enable_one_interrupt (irq); > >> - } else { /* disable */ > >> - act->handler = (interrupt_handler_t *) def_hdlr; > >> - act->arg = (void *)irq; > >> - disable_one_interrupt (irq); > >> + return 0; > >> } > >> + > >> + /* Disable */ > >> + act->handler = (interrupt_handler_t *) def_hdlr; > >> + act->arg = (void *)irq; > >> + disable_one_interrupt(irq); > >> + return 1; > >> } > >> > >> /* initialization interrupt controller - hardware */ > >> diff --git a/arch/microblaze/include/asm/microblaze_intc.h b/arch/microblaze/include/asm/microblaze_intc.h > >> index 6142b9c..e9640f5 100644 > >> --- a/arch/microblaze/include/asm/microblaze_intc.h > >> +++ b/arch/microblaze/include/asm/microblaze_intc.h > >> @@ -39,7 +39,16 @@ struct irq_action { > >> int count; /* number of interrupt */ > >> }; > >> > >> -void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, > >> +/** > >> + * Register and unregister interrupt handler rutines > >> + * > >> + * @param irq IRQ number > >> + * @param hdlr Interrupt handler rutine > >> + * @param arg Pointer to argument which is passed to int. handler rutine > >> + * @return 0 if registration pass, 1 if unregistration pass, > >> + * or an error code < 0 otherwise > >> + */ > >> +int install_interrupt_handler(int irq, interrupt_handler_t *hdlr, > >> void *arg); > > > > Hi Michal, > > > > why not two different functions here, one for registration, another one > > for unregistration? To mee it is puzzling to use a 'install' function > > for unregistration ... > > partially agree with that. Maybe we could introduce one macro for that. > > #define uninstall_interrupt_handler(irq) install_interrupt_handler(irq, NULL, NULL) yes, that is ok ... > > > > ... whatever, you should evaluate the return code in fsl_init2() too. > > Not necessary to do it in this patch. yes, it's another part ... br, Stephan
diff --git a/arch/microblaze/cpu/interrupts.c b/arch/microblaze/cpu/interrupts.c index ee67082..08f6bad 100644 --- a/arch/microblaze/cpu/interrupts.c +++ b/arch/microblaze/cpu/interrupts.c @@ -91,14 +91,13 @@ static void disable_one_interrupt(int irq) #endif } -/* adding new handler for interrupt */ -void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, void *arg) +int install_interrupt_handler(int irq, interrupt_handler_t *hdlr, void *arg) { struct irq_action *act; /* irq out of range */ if ((irq < 0) || (irq > irq_no)) { puts ("IRQ out of range\n"); - return; + return -1; } act = &vecs[irq]; if (hdlr) { /* enable */ @@ -106,11 +105,14 @@ void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, void *arg) act->arg = arg; act->count = 0; enable_one_interrupt (irq); - } else { /* disable */ - act->handler = (interrupt_handler_t *) def_hdlr; - act->arg = (void *)irq; - disable_one_interrupt (irq); + return 0; } + + /* Disable */ + act->handler = (interrupt_handler_t *) def_hdlr; + act->arg = (void *)irq; + disable_one_interrupt(irq); + return 1; } /* initialization interrupt controller - hardware */ diff --git a/arch/microblaze/include/asm/microblaze_intc.h b/arch/microblaze/include/asm/microblaze_intc.h index 6142b9c..e9640f5 100644 --- a/arch/microblaze/include/asm/microblaze_intc.h +++ b/arch/microblaze/include/asm/microblaze_intc.h @@ -39,7 +39,16 @@ struct irq_action { int count; /* number of interrupt */ }; -void install_interrupt_handler (int irq, interrupt_handler_t * hdlr, +/** + * Register and unregister interrupt handler rutines + * + * @param irq IRQ number + * @param hdlr Interrupt handler rutine + * @param arg Pointer to argument which is passed to int. handler rutine + * @return 0 if registration pass, 1 if unregistration pass, + * or an error code < 0 otherwise + */ +int install_interrupt_handler(int irq, interrupt_handler_t *hdlr, void *arg); int interrupts_init(void);
Return value to find out if un/registration was succesful. Signed-off-by: Michal Simek <monstr@monstr.eu> --- v2: Add comment to header file to describe parameters and return codes --- arch/microblaze/cpu/interrupts.c | 16 +++++++++------- arch/microblaze/include/asm/microblaze_intc.h | 11 ++++++++++- 2 files changed, 19 insertions(+), 8 deletions(-)