[PATCH 00/21] staging: comedi: pcmuio: clean up driver

[Ian Abbott]

[Originally posted 2013-06-19.  Reposting to new driverdev-devel list.]

On 2013-06-19 17:22, H Hartley Sweeten wrote:
> On Wednesday, June 19, 2013 9:09 AM, Ian Abbott wrote:
>> On 2013-06-18 21:20, H Hartley Sweeten wrote:
>>> Clean up and simplify this legacy driver.
>>>
>>> The async command support (interrupts) still needs a bit of work.
>>> The command support is a bit goofy and does not really follow the
>>> comedi API. This will be addressed.
>>
>> Command support for reading DIO subdevices is a bit goofy.  All of them
>> pack the single-bit channels somehow.  Some drivers just return the data
>> from all the channels, where bit 'n' corresponds to the data from
>> channel 'n'.  Other drivers (like this one) set bit 'n' to the data read
>> from channel 'chanlist[n]'.  That's probably closer to the spirit of the
>> handling of the scan data for AI subdevices in that the samples are in
>> the order specified by the chanlist.

Actually, on closer inspection, this driver is setting bit 'n' according 
to whether the desired edge was detected for 'chanlist[n]', not 
according to the actual data value of channel 'chanlist[n]'.

> I think the current command support is actually a bit broken for the
> PCM-UIO96 board. The for() loop that initializes the subdevices will
> try to setup both suibdevice[0] and subdevice[2] to support
> SDF_CMD_READ. The code that does that sets dev->read_subdev
> for both subdevices so it ends up pointing to subdevice[2].
>
> I'm looking at supporting the digital input interrutps like they are
> handled in the addi_apci_1032 driver. So the driver will either
> allocate 3 or 5 subdevices depending on the board type.
> Subdevices 0 and 1, and 2 and 3 on the PCM-UIO96, will still
> be 24 channel DIO subdevices. The last subdevice will be a DI
> subdevice to handle the interrupt support. That subdevice will
> then support the INSN_CONFIG_DIGITAL_TRIG instruction to
> configure the interrupts.
>
> I'm just not sure how to handle the 2 sets of 24 interrupt channels.
>
> I can do it using the left-shift (data[3]) parameter for the instruction.
> The user will need to do two instructions to configure the interrupts.
> One with a left-shift of 0 for the first asic and one with a left-shift of
> 24 for the second asic.
>
> Or, I could use the trigger number (data[1]) to indicate which asic
> is being configured.
>
> Do you have any comments or opinions on this?

It's probably not worth bothering changing the existing set-up.  The 
existing users of the command support (who I guess could probably be 
counted on the fingers of one hand) probably wouldn't appreciate the change.

It is possible to set up commands on either subdevice.  You just need to 
open the subdevice-specific /dev/comediX_subdY files to use read() or 
write() on the non-default read or write subdevice.

The existing scan trigger for the command is a set of channel edges 
specified by the chanlist (each index specifying a channel and an edge 
polarity), which seems reasonable.  One limitation and slight bug is 
that if you have two chanlist entries for the same channel, but 
different edges, only the rising edge will be detected by the interrupt, 
but the scan data bit vector will contain a '1' for each index that 
channel appears in the chanlist regardless of of the edge polarity for 
that chanlist entry.  Another oddity is the use of CR_RANGE() and 
CR_AREF() to choose the edge polarity instead of just checking the 
CR_INVERT flag.

If allocating a separate subdevice for interrupt handling, you still 
don't need INSN_CONFIG_DIGITAL_TRIG as you can still use the chanlist to 
decide which interrupt sources you are triggering on.  The channel 
numbers of the interrupt subdevice would correspond to the interrupt 
sources, which happen to match up nicely with the channel numbers of the 
DIO subdevices.  (For example, amplc_dio200 sets up a 6-channel 
interrupt subdevice for its six possible interrupt sources and the 
chanlist is used to select the sources you want to trigger on.) pcmuio 
is kind of doing the same thing except that the interrupt subdevices are 
merged into the DIO subdevices.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti at mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-