On 2013-08-28 21:30, H Hartley Sweeten wrote:
Use comedi_dio_insn_bits() to handle the boilerplate code to update
the subdevice s->state.
Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
Cc: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
drivers/staging/comedi/drivers/ssv_dnp.c | 51 +++++++++++++-------------------
1 file changed, 20 insertions(+), 31 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ssv_dnp.c b/drivers/staging/comedi/drivers/ssv_dnp.c
index 11758a5..b79e691 100644
--- a/drivers/staging/comedi/drivers/ssv_dnp.c
+++ b/drivers/staging/comedi/drivers/ssv_dnp.c
@@ -46,51 +46,40 @@ Status: unknown
#define PCMR 0xa3 /* Port C Mode Register */
#define PCDR 0xa7 /* Port C Data Register */
-/* ------------------------------------------------------------------------- */
-/* The insn_bits interface allows packed reading/writing of DIO channels. */
-/* The comedi core can convert between insn_bits and insn_read/write, so you */
-/* are able to use these instructions as well. */
-/* ------------------------------------------------------------------------- */
-
static int dnp_dio_insn_bits(struct comedi_device *dev,
struct comedi_subdevice *s,
- struct comedi_insn *insn, unsigned int *data)
+ struct comedi_insn *insn,
+ unsigned int *data)
{
- /* The insn data is a mask in data[0] and the new data in data[1], */
- /* each channel cooresponding to a bit. */
-
- /* Ports A and B are straight forward: each bit corresponds to an */
- /* output pin with the same order. Port C is different: bits 0...3 */
- /* correspond to bits 4...7 of the output register (PCDR). */
+ unsigned int val;
- if (data[0]) {
+ /*
+ * Ports A and B are straight forward: each bit corresponds to an
+ * output pin with the same order. Port C is different: bits 0...3
+ * correspond to bits 4...7 of the output register (PCDR).
+ */
+ outb(PADR, CSCIR);
+ s->state = inb(CSCDR);
+ outb(PBDR, CSCIR);
+ s->state |= (inb(CSCDR) << 8);
+ outb(PCDR, CSCIR);
+ val = inb(CSCDR);
+ s->state |= ((val & 0xf0) << 12);
I'm not sure what's going on there. We shouldn't be reading registers
to update s->state. Probably best just to skip that part.
+ if (comedi_dio_insn_bits(dev, s, insn, data)) {
The comedi_dio_insn_bits() will AND the mask with s->io_bits, but this
driver module currently does nothing with s->io_bits. If you want to
use comedi_dio_insn_bits(), s->io_bits will need to be modified by
dnp_dio_insn_config().
outb(PADR, CSCIR);
- outb((inb(CSCDR)
- & ~(u8) (data[0] & 0x0000FF))
- | (u8) (data[1] & 0x0000FF), CSCDR);
+ outb(s->state & 0xff, CSCDR);
outb(PBDR, CSCIR);
- outb((inb(CSCDR)
- & ~(u8) ((data[0] & 0x00FF00) >> 8))
- | (u8) ((data[1] & 0x00FF00) >> 8), CSCDR);
+ outb((s->state >> 8) & 0xff, CSCDR);
outb(PCDR, CSCIR);
- outb((inb(CSCDR)
- & ~(u8) ((data[0] & 0x0F0000) >> 12))
- | (u8) ((data[1] & 0x0F0000) >> 12), CSCDR);
+ outb(((val & 0x0f) | (s->state >> 12)) & 0xff, CSCDR);
}
- /* on return, data[1] contains the value of the digital input lines. */
- outb(PADR, CSCIR);
- data[0] = inb(CSCDR);
- outb(PBDR, CSCIR);
- data[0] += inb(CSCDR) << 8;
- outb(PCDR, CSCIR);
- data[0] += ((inb(CSCDR) & 0xF0) << 12);
+ data[0] = s->state;
I see why you read those registers earlier, but it is better to read
them after writing the outputs as that's what all the other drivers do.
Also, it should be setting data[1], not data[0] although that is a bug
in the original version.
return insn->n;
-
}
static int dnp_dio_insn_config(struct comedi_device *dev,
--
-=( 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
