On 12/11/14 16:07, Hartley Sweeten wrote:
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.
Oh right, so the other bits in the written value get ignored if that bit
is set.
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.
Yes, it's general purpose for the driver and the comedi core doesn't use
it itself. I don't think the spin-lock is strictly necessary in the
dmm32at_setaitimer() and dmm32at_8255_io() functions as they currently
only get called while dev->mutex is locked.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Web: http://www.mev.co.uk/ )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
