On Wednesday, November 12, 2014 3:12 AM, Ian Abbott wrote: > On 11/11/14 23:55, H Hartley Sweeten wrote: >> The Dimond-MM-32-AT board uses an internal 82C55-type digital I/O circuit to >> provide the 24 digital I/O lines. The only quirk is the need to set the page >> selection bits in the control register to select page 1 addresses. >> >> Instead of duplicating the 8255 code, provide an (*io) callback and use the >> 8255 module to support this subdevice. >> >> This also removes the need for the private data in this driver. > > The patch is fine, but there's a bug in the original code which means we > might need the private data back (unless the DMM32AT_CNTRL register is > read-write rather than read-only). The bug is that the ISR routine > clobbers the page selection bits in the register. To avoid that, the > current page would either need to be stored in private data or read from > the register (if possible) to avoid clobbering it, and updates to the > register would need a spin-lock. Ian, Thanks for the review. The write to the Miscellaneous Control register (DMM32AT_CTRL_REG) in the ISR routine is actually safe. According to the user manual: INTRST Writing a 1 to this location resets the interrupt request circuit on the board. The programmer must write a 1 to this bit during the interrupt service routine, or further interrupts will not occur. Writing a 1 to this bit does not disturb the values of the PAGE bits. The only other writes to the DMM32AT_CTRL_REG are in: dmm32at_reset() - sets the RESETA bit to cause a full reset of the board dmm32at_ai_cmd() - sets INTRST to reset the interrupt (this is probably not necessary, maybe it should be moved to the (*cancel)). dmm32at_setaitimer() - selects the 8254 page dmm32at_8255_io() - selects the 8255 page The only place the protection is needed is in the dmm32at_setaitimer() and dmm32at_8255_io() functions. The PAGE bits are readable in the FIFO Status register (DMM32AT_FIFO_STATUS_REG) but a spin-lock is probably safer. Could we use the comedi_device 'spinlock' for this? The core does not seem to use it. Is this just ageneral purpose spin-lock for the drivers? It really needs a comment of some sort. Thanks, Hartley _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
