On Tuesday, August 06, 2013 6:38 AM, Ian Abbott wrote:
> On 2013-08-05 23:01, H Hartley Sweeten wrote:
>> Convert the drivers with simple, per channel programmable i/o, to use the
>> comedi_dio_insn_config() helper function.
>>
>> All of these pass a 'mask' of '0' to comedi_dio_insn_config() this causes
>> the per channel mask to be used to configure the i/o direction.
>>
>> Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/staging/comedi/drivers/cb_pcidas64.c | 25 +++------
>> drivers/staging/comedi/drivers/comedi_bond.c | 51 +++++++-----------
>> drivers/staging/comedi/drivers/ni_65xx.c | 42 ++++++---------
>> drivers/staging/comedi/drivers/ni_670x.c | 25 +++------
>> drivers/staging/comedi/drivers/ni_daq_700.c | 24 ++++-----
>> drivers/staging/comedi/drivers/ni_mio_common.c | 57 +++++----------------
>> drivers/staging/comedi/drivers/ni_pcidio.c | 26 +++-------
>> drivers/staging/comedi/drivers/pcmmio.c | 71 ++++++--------------------
>> drivers/staging/comedi/drivers/pcmuio.c | 24 +++------
>> drivers/staging/comedi/drivers/rtd520.c | 20 ++------
>> drivers/staging/comedi/drivers/s626.c | 22 ++------
>> drivers/staging/comedi/drivers/ssv_dnp.c | 69 ++++++++-----------------
>> drivers/staging/comedi/drivers/usbdux.c | 19 ++-----
>> drivers/staging/comedi/drivers/usbduxsigma.c | 20 ++------
>> 14 files changed, 136 insertions(+), 359 deletions(-)
>
> This won't work for ni_65xx as the DIO subdevice has 96 channels, but
> s->io_bits only supports 32 channels.
Ah, I thought I caught all the > 32 channel uses. Missed this one due to the
use of the boardinfo.
It looks like the original driver has a bug anyway since it uses the s->io_bits
in the (*insn_config).
I'll drop this one.
> Also, comedi_bond supports a maximum of 256 channels although it
> probably doesn't work correctly for more than 32 channels as it
> currently uses s->io_bits. To fix it, we wouldn't be able to use your
> new function. (Actually, I'm still working out how it's supposed to
> work, but I'm pretty sure it needs to be able to record the direction
> and state for MAX_CHANS (256) channels.)
I'll drop this one also.
> I suggest leaving ni_65xx and comedi_bond out of this patch.
>
> The rest looks okay, though I think the existing behaviour of the DIO
> subdevices of pcmmio.c and pcmuio.c will need fixing later.
Those two drivers appear to have the same DIO support code. The pcmmio
has an additional AI and AO subdevice. I'm wondering if it's worth merging
them into a common driver.
<snip>
>> diff --git a/drivers/staging/comedi/drivers/ssv_dnp.c b/drivers/staging/comedi/drivers/ssv_dnp.c
>> index 4da4d32..6c56864 100644
>> --- a/drivers/staging/comedi/drivers/ssv_dnp.c
>> +++ b/drivers/staging/comedi/drivers/ssv_dnp.c
>> @@ -93,68 +93,39 @@ static int dnp_dio_insn_bits(struct comedi_device *dev,
>>
>> }
>>
>> -/* ------------------------------------------------------------------------- */
>> -/* Configure the direction of the bidirectional digital i/o pins. chanspec */
>> -/* contains the channel to be changed and data[0] contains either */
>> -/* COMEDI_INPUT or COMEDI_OUTPUT. */
>> -/* ------------------------------------------------------------------------- */
>> -
>> static int dnp_dio_insn_config(struct comedi_device *dev,
>> struct comedi_subdevice *s,
>> - struct comedi_insn *insn, unsigned int *data)
>> + struct comedi_insn *insn,
>> + unsigned int *data)
>> {
>> + unsigned int chan = CR_CHAN(insn->chanspec);
>> + unsigned int mask;
>> + unsigned int val;
>> + int ret;
>>
>> - u8 register_buffer;
>> -
>> - /* reduces chanspec to lower 16 bits */
>> - int chan = CR_CHAN(insn->chanspec);
>> -
>> - switch (data[0]) {
>> - case INSN_CONFIG_DIO_OUTPUT:
>> - case INSN_CONFIG_DIO_INPUT:
>> - break;
>> - case INSN_CONFIG_DIO_QUERY:
>> - data[1] =
>> - (inb(CSCDR) & (1 << chan)) ? COMEDI_OUTPUT : COMEDI_INPUT;
>> - return insn->n;
>> - break;
>> - default:
>> - return -EINVAL;
>> - break;
>> - }
>> - /* Test: which port does the channel belong to? */
>> -
>> - /* We have to pay attention with port C: this is the meaning of PCMR: */
>> - /* Bit in PCMR: 7 6 5 4 3 2 1 0 */
>> - /* Corresponding port C pin: d 3 d 2 d 1 d 0 d= don't touch */
>
> I think something like that comment needs to be retained somewhere to
> explain the mysterious multiplication by 2 for port C below.
I'll add the comment back when I re-post the series.
Thanks,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
