Re: [PATCH 3/4] staging: comedi: drivers: use comedi_dio_insn_config() for simple cases

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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 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.

[snip]
diff --git a/drivers/staging/comedi/drivers/comedi_bond.c b/drivers/staging/comedi/drivers/comedi_bond.c
index 7e20bf0..6ef1f75 100644
--- a/drivers/staging/comedi/drivers/comedi_bond.c
+++ b/drivers/staging/comedi/drivers/comedi_bond.c

I suggest leaving comedi_bond.c alone here as mentioned earlier, as the fixed version wouldn't be able to use your function anyway.

[snip]
diff --git a/drivers/staging/comedi/drivers/ni_65xx.c b/drivers/staging/comedi/drivers/ni_65xx.c
index 3ba4c57..19a5d74 100644
--- a/drivers/staging/comedi/drivers/ni_65xx.c
+++ b/drivers/staging/comedi/drivers/ni_65xx.c

As mentioned earlier, ni_65xx cannot use the new function as the DIO subdevice has 96 channels. So leave ni_65xx.c alone here.

[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.

+       ret = comedi_dio_insn_config(dev, s, insn, data, 0);
+       if (ret)
+               return ret;

-       if ((chan >= 0) && (chan <= 7)) {
-               /* this is port A */
+       if (chan < 8) {                 /* Port A */
+               mask = 1 << chan;
                 outb(PAMR, CSCIR);
-       } else if ((chan >= 8) && (chan <= 15)) {
-               /* this is port B */
-               chan -= 8;
+       } else if (chan < 16) {         /* Port B */
+               mask = 1 << (chan - 8);
                 outb(PBMR, CSCIR);
-       } else if ((chan >= 16) && (chan <= 19)) {
-               /* this is port C; multiplication with 2 brings bits into     */
-               /* correct position for PCMR!                                 */
-               chan -= 16;
-               chan *= 2;
+       } else {                        /* Port C */
+               mask = 1 << ((chan - 16) * 2);
                 outb(PCMR, CSCIR);
-       } else {
-               return -EINVAL;
         }

-       /* read 'old' direction of the port and set bits (out=1, in=0)        */
-       register_buffer = inb(CSCDR);
+       val = inb(CSCDR);
         if (data[0] == COMEDI_OUTPUT)
-               register_buffer |= (1 << chan);
+               val |= mask;
         else
-               register_buffer &= ~(1 << chan);
-
-       outb(register_buffer, CSCDR);
+               val &= ~mask;
+       outb(val, CSCDR);

-       return 1;
+       return insn->n;

  }

[snip]

--
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@xxxxxxxxx>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux