[PATCH 12/30] staging: comedi: dmm32at: use 8255 module for Digital I/O subdevice

[Ian Abbott]

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 at mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-