On 2013-08-28 21:28, H Hartley Sweeten wrote:
Use comedi_dio_insn_bits() to handle the boilerplate code to update
the subdevice s->state for DIO and DO subdevices.
Signed-off-by: H Hartley Sweeten <hswee...@visionengravers.com>
Cc: Ian Abbott <abbo...@mev.co.uk>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
[snip]
diff --git a/drivers/staging/comedi/drivers/8255.c
b/drivers/staging/comedi/drivers/8255.c
index 2f070fd..d29f404 100644
--- a/drivers/staging/comedi/drivers/8255.c
+++ b/drivers/staging/comedi/drivers/8255.c
@@ -126,30 +126,17 @@ EXPORT_SYMBOL_GPL(subdev_8255_interrupt);
static int subdev_8255_insn(struct comedi_device *dev,
struct comedi_subdevice *s,
- struct comedi_insn *insn, unsigned int *data)
+ struct comedi_insn *insn,
+ unsigned int *data)
{
struct subdev_8255_private *spriv = s->private;
unsigned long iobase = spriv->iobase;
- unsigned int mask;
- unsigned int bits;
unsigned int v;
- mask = data[0];
- bits = data[1];
-
- if (mask) {
- v = s->state;
- v &= ~mask;
- v |= (bits & mask);
-
- if (mask & 0xff)
- spriv->io(1, _8255_DATA, v & 0xff, iobase);
- if (mask & 0xff00)
- spriv->io(1, _8255_DATA + 1, (v >> 8) & 0xff, iobase);
- if (mask & 0xff0000)
- spriv->io(1, _8255_DATA + 2, (v >> 16) & 0xff, iobase);
-
- s->state = v;
+ if (comedi_dio_insn_bits(dev, s, insn, data)) {
+ spriv->io(1, _8255_DATA, s->state & 0xff, iobase);
+ spriv->io(1, _8255_DATA + 1, (s->state >> 8) & 0xff, iobase);
+ spriv->io(1, _8255_DATA + 2, (s->state >> 16) & 0xff, iobase);
}
There's extra register access overhead here that wasn't in the original.
Quite often, the mask only has one bit set (when insn_bits is being
used to emulate insn_write, for example), which would only write one
register. For PC I/O space, register writes typically take a
microsecond. Or at least they used to, but I think it might be less on
newer hardware.
For 8255, this change may also result in the output pin values being
different than they were before due to a quirk of the 8255. Following
an INSN_CONFIG_DIO_INPUT or INSN_CONFIG_DIO_OUTPUT, all output pins are
set to 0 by the chip, although comedi's s->state value is not affected.
Then a following INSN_BITS with a mask will set _all_ the output pins
to the new s->state, rather than setting _some of_ the output pins to
the new s->state. Maybe that's a good thing, although it's a change to
the existing behaviour.
Probably best to play it safe and write to the output registers
selectively according to the mask for the 8255 module.
Just a thought - perhaps comedi_dio_insn_bits() could return the mask
value to allow the caller to selectively write to registers based on the
mask if it wants to?
[snip]
diff --git a/drivers/staging/comedi/drivers/dt2817.c
b/drivers/staging/comedi/drivers/dt2817.c
index f4a8529..5de1330 100644
--- a/drivers/staging/comedi/drivers/dt2817.c
+++ b/drivers/staging/comedi/drivers/dt2817.c
@@ -80,36 +80,24 @@ static int dt2817_dio_insn_config(struct comedi_device *dev,
static int dt2817_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)
{
- unsigned int changed;
-
- /* It's questionable whether it is more important in
- * a driver like this to be deterministic or fast.
- * We choose fast. */
-
- if (data[0]) {
- changed = s->state;
- s->state &= ~data[0];
- s->state |= (data[0] & data[1]);
- changed ^= s->state;
- changed &= s->io_bits;
- if (changed & 0x000000ff)
- outb(s->state & 0xff, dev->iobase + DT2817_DATA + 0);
- if (changed & 0x0000ff00)
- outb((s->state >> 8) & 0xff,
- dev->iobase + DT2817_DATA + 1);
- if (changed & 0x00ff0000)
- outb((s->state >> 16) & 0xff,
- dev->iobase + DT2817_DATA + 2);
- if (changed & 0xff000000)
- outb((s->state >> 24) & 0xff,
- dev->iobase + DT2817_DATA + 3);
+ unsigned int val;
+
+ if (comedi_dio_insn_bits(dev, s, insn, data)) {
+ outb(s->state & 0xff, dev->iobase + DT2817_DATA + 0);
+ outb((s->state >> 8) & 0xff, dev->iobase + DT2817_DATA + 1);
+ outb((s->state >> 16) & 0xff, dev->iobase + DT2817_DATA + 2);
+ outb((s->state >> 24) & 0xff, dev->iobase + DT2817_DATA + 3);
}
I'm just noting the drivers that do some filtering on the mask, although
dt2817 goes one stage further and also filters on the changed state bits
and the direction bits!
[snip]
diff --git a/drivers/staging/comedi/drivers/ni_6527.c
b/drivers/staging/comedi/drivers/ni_6527.c
index c2745f2..25f9a12 100644
--- a/drivers/staging/comedi/drivers/ni_6527.c
+++ b/drivers/staging/comedi/drivers/ni_6527.c
@@ -163,29 +163,19 @@ static int ni6527_di_insn_bits(struct comedi_device *dev,
static int ni6527_do_insn_bits(struct comedi_device *dev,
struct comedi_subdevice *s,
- struct comedi_insn *insn, unsigned int *data)
+ struct comedi_insn *insn,
+ unsigned int *data)
{
struct ni6527_private *devpriv = dev->private;
+ void __iomem *io_addr = devpriv->mite->daq_io_addr;
- if (data[0]) {
- s->state &= ~data[0];
- s->state |= (data[0] & data[1]);
-
- /* The open relay state on the board cooresponds to 1,
- * but in Comedi, it is represented by 0. */
- if (data[0] & 0x0000ff) {
- writeb((s->state ^ 0xff),
- devpriv->mite->daq_io_addr + Port_Register(3));
- }
- if (data[0] & 0x00ff00) {
- writeb((s->state >> 8) ^ 0xff,
- devpriv->mite->daq_io_addr + Port_Register(4));
- }
- if (data[0] & 0xff0000) {
- writeb((s->state >> 16) ^ 0xff,
- devpriv->mite->daq_io_addr + Port_Register(5));
- }
+ if (comedi_dio_insn_bits(dev, s, insn, data)) {
+ /* Outputs are inverted */
+ writeb(s->state ^ 0xff, io_addr + Port_Register(3));
+ writeb((s->state >> 8) ^ 0xff, io_addr + Port_Register(4));
+ writeb((s->state >> 16) ^ 0xff, io_addr + Port_Register(5));
}
ni6527 is one of those drivers that were filtering the register writes
based on the mask.
[snip]
diff --git a/drivers/staging/comedi/drivers/pcl711.c
b/drivers/staging/comedi/drivers/pcl711.c
index e859f85..a94bfd9 100644
--- a/drivers/staging/comedi/drivers/pcl711.c
+++ b/drivers/staging/comedi/drivers/pcl711.c
@@ -422,19 +422,15 @@ static int pcl711_di_insn_bits(struct comedi_device *dev,
return insn->n;
}
-/* Digital port write - Untested on 8112 */
static int pcl711_do_insn_bits(struct comedi_device *dev,
struct comedi_subdevice *s,
- struct comedi_insn *insn, unsigned int *data)
+ struct comedi_insn *insn,
+ unsigned int *data)
{
- if (data[0]) {
- s->state &= ~data[0];
- s->state |= data[0] & data[1];
- }
- if (data[0] & 0x00ff)
+ if (comedi_dio_insn_bits(dev, s, insn, data)) {
outb(s->state & 0xff, dev->iobase + PCL711_DO_LO);
- if (data[0] & 0xff00)
outb((s->state >> 8), dev->iobase + PCL711_DO_HI);
+ }
data[1] = s->state;
pcl711 is another of those drivers that were filtering the register
writes based on the mask.
diff --git a/drivers/staging/comedi/drivers/pcl726.c
b/drivers/staging/comedi/drivers/pcl726.c
index a4d0bcc..aa5e000 100644
--- a/drivers/staging/comedi/drivers/pcl726.c
+++ b/drivers/staging/comedi/drivers/pcl726.c
@@ -196,18 +196,15 @@ static int pcl726_di_insn_bits(struct comedi_device *dev,
static int pcl726_do_insn_bits(struct comedi_device *dev,
struct comedi_subdevice *s,
- struct comedi_insn *insn, unsigned int *data)
+ struct comedi_insn *insn,
+ unsigned int *data)
{
const struct pcl726_board *board = comedi_board(dev);
- if (data[0]) {
- s->state &= ~data[0];
- s->state |= data[0] & data[1];
- }
- if (data[1] & 0x00ff)
+ if (comedi_dio_insn_bits(dev, s, insn, data)) {
outb(s->state & 0xff, dev->iobase + board->do_lo);
- if (data[1] & 0xff00)
outb((s->state >> 8), dev->iobase + board->do_hi);
+ }
data[1] = s->state;
pcl726 was also fitlering the writes, but it was filtering on the wrong
thing, i.e. the data instead of the mask. That was a bug!
[snip]
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbo...@mev.co.uk> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
