On Wednesday, November 05, 2014 7:48 AM, Ian Abbott wrote: > On 04/11/14 17:53, H Hartley Sweeten wrote: >> Following is the big cleanup series for the ADDI-DATA APCI-3120 driver. >> >> Quick summary of the cleanup: >> >> * Removes all the CamelCase >> * Cleans up the register map defines >> * Fixes the Analog Input subdevice >> * Fixes the async command support for the Analog Input subdevice >> * Cleans up all the timer support code >> * Cleans up the DMA support code >> * Removes the included addi-data/hwdrv_apci3120.c source file >> * Removes all the cruft [snip] >> .../comedi/drivers/addi-data/hwdrv_apci3120.c | 1903 -------------------- >> drivers/staging/comedi/drivers/addi_apci_3120.c | 912 +++++++++- >> drivers/staging/comedi/drivers/amcc_s5933.h | 2 + >> 3 files changed, 885 insertions(+), 1932 deletions(-) >> delete mode 100644 drivers/staging/comedi/drivers/addi-data/hwdrv_apci3120.c >> > > You've been busy! I think I'll review this as a whole rather than > individually. Yah... This series has been in my queue for a while. > My only comments are: > > 1) It would be preferable to not switch on the I8254_MODE<n> constants > for the timer mode in PATCH 091 as they're pretty irrelevant here. > Rather, just add some new constants to comedi.h - perhaps the > APCI3120_TIMER_MODE<n> constants introduced in PATCH 010. The documentation for the board says this about the timer modes: Mode = 0: corresponds to Mode 0 in 8254 Mode = 1: corresponds to Mode 2 in 8254 Mode = 2: corresponds to Mode 4 in 8254 Mode = 3: corresponds to Mode 5 in 8254 Based on that I feel that the I8254_MODE<n> constants are relevant. Also, the uapi header (comedi.h) already has too many driver specific defines in it. I think adding more will just make using comedilib more cumbersome (and possibly confusing). > 2) The "IRQ from unknown source\n" message should be removed to avoid > spamming the kernel log since this is likely to be due to an interrupt > from a different device on the same IRQ number. (That was copied over > from the original code by PATCH 102.) Ah... Missed that one. I'll post a patch to remove it after this series is applied. > 3) A comedi driver header comment would be nice! Thought about that but wasn't sure what to add for the Author field. Right now I think git blame would pretty much indicate it was me since this series was basically a full rewrite of the driver. > Overall, a splendid job! Thanks! > Reviewed-by: Ian Abbott <abbotti@xxxxxxxxx> Regards, Hartley _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
