[PATCH 00/21] staging: comedi: pcmuio: clean up driver
Ian Abbott
abbotti at mev.co.uk
Thu Jun 27 13:22:17 UTC 2013
[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 )=-
More information about the devel
mailing list