RE: [PATCH 6/6] staging: comedi: addi_apci_1564: cleanup v_ADDI_Interrupt()

[Hartley Sweeten]

On Thursday, May 29, 2014 9:44 PM, Chase Southwood wrote:
> Move the function apci1564_interrupt() from hwdrv_apci1564.c to
> addi_apci_1564.c.  On moving, for now just strip out all of the
> code for interrupts that the driver does not yet support at this
> time.
>
> Rename the variable ui_InterruptStatus_1564 to ctrl, and change the return
> from IRQ_RETVAL(1) to IRQ_HANDLED.
>
> We also check the device is asserting the shared interrupt line and check
> that interrupts have been enabled.
>
> Signed-off-by: Chase Southwood <chase.southwood@xxxxxxxxx>
> Cc: Ian Abbott <abbotti@xxxxxxxxx>
> Cc: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
> ---
>
> Admittedly, I am not sure if what I have done in the interrupt handler is
> quite sufficient.  Am I missing anything that should be there due to the
> fact that the handler does not yet support all of the types of interrupt
> that the board is capable of issuing (i.e. does the interrupt handler need
> to clear the other interrupts just in case they get triggered, etc?)

This one is a bit of a mess, not your fault, the existing code is pretty ugly.

>  .../comedi/drivers/addi-data/hwdrv_apci1564.c      | 130 ---------------------
>  drivers/staging/comedi/drivers/addi_apci_1564.c    |  34 +++++-
>  2 files changed, 30 insertions(+), 134 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> index 41aa889..ebd763b 100644
> --- a/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> +++ b/drivers/staging/comedi/drivers/addi-data/hwdrv_apci1564.c
> @@ -90,7 +90,6 @@
>  #define APCI1564_TCW_WARN_TIMEBASE_REG(x)		(0x1c + ((x) * 0x20))
>  
>  /* Global variables */
> -static unsigned int ui_InterruptStatus_1564;
>  static unsigned int ui_InterruptData, ui_Type;
 
These static globals need to away.

ui_InterruptStatus_1564 is only used in the interrupt handler. It's also not really used
in the existing code. You fixed this in your patch but I'm not sure it deserves mentioning
in the commit other than maybe "remove unnecessary static global foo".

ui_InterruptData is set in apci1564_do_config() but it is never used. That function is also
broke since it does not follow the comedi API.

ui_Type is set in the interrupt handler but you patch strips it out. The variable is then
used in apci1564_do_read() but that function also abuses the comedi API since it is
returning the diagnostic status in the digital output (*insn_read) operation.

Ugh...

You might want to rework this patch to:

1) Move the static globals into the private data.
2) Merge the existing interrupt handler from hwrdv_apci1564.c into the driver.
   You could tidy up the existing code a bit during the move. Hint, the counters
   could be handled with a for () loop.
3) Now "fix" the digital input interrupt handling.

Since you still have the timer subdevice I would leave the timer/counter handling
in the interrupt handler.

For now, hold off and see if Ian has any comments.

I'll try to apply your series and take a better look at this later today.

Regards,
Hartley

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/