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

[Ian Abbott]

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

On 2013-06-20 10:44, Ian Abbott wrote:
> On 2013/06/19 09:43 PM, H Hartley Sweeten wrote:
>> On Wednesday, June 19, 2013 10:28 AM, Ian Abbott wrote:
>>> 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]'.
>>
>> But, the logic of setting the desired edge doesn't work.
>>
>> do_cmd_ioctl() calls comedi_check_chanlist() before calling the
>> s->do_cmdtest() function. Then, as long as everything is still ok,
>> the s->do_cmd() function will get called.
>>
>> comedi_check_chanlist() will check the chanlist[] to make sure all
>> the channel, range, and aref values are valid for the subdevice. The
>> range_table for all the subdevices in this driver is range_digital. So:
>>
>> CR_CHAN() must be < s->n_chan	(< 24)
>> Ok, channels 0-23 are valid.
>>
>> CR_RANGE() must be < s->range_table->length	(< 1)
>> Not ok, only a range of 0 is valid so only falling-edge detection
>> can be selected.
>>
>> CR_AREF() must be part of the aref s->subdev_flags
>> Not ok, no aref subdev_flags are set so again only falling-edge
>> detection can be selected.
>>
>> So, if the user tries to pass a non-zero CR_RANGE() of CR_AREF()
>> comedi_check_chanlist() will fail with -EINVAL and the command
>> function is never called.
>
> That's true, so it won't work for falling edge detection.  It could be
> changed to check for CR_INVERT for falling edge detection instead, maybe
> with a bit of sanity checking on the whole chanlist to make sure that it
> does not contain both edges of the same channel.

Or rather, it's rising edge detection that's not working.  CR_INVERT 
could be set to enable rising edge detection, although that's different 
to the usual convention where CR_INVERT usually means a low-level or a 
falling edge.  One way out of that conundrum is to use both CR_INVERT 
and CR_EDGE as follows:

0 = falling edge (for backwards compatibility)
CR_EDGE = rising edge
CR_EDGE | CR_INVERT = falling edge
CR_INVERT = ?? maybe treat it as rising edge ??

(not sure about the last one!)

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