On 2013-08-29 17:39, Hartley Sweeten wrote:
On Thursday, August 29, 2013 5:32 AM, Ian Abbott wrote:
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 <hsweeten@xxxxxxxxxxxxxxxxxxx>
Cc: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
[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.
I'm not sure if the I/O access adds any significant more time than the
test to see if the I/O is needed.
Single register accesses may take a whole microsecond on a PC. Someone
more familiar with PC architecture could probably give a more definitive
answer on that.
For simplicity, and consistency, I removed all these checks of the mask
in this patch. If you think they are needed I will refresh the patch and
leave them all in.
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.
That sounds like a bug in the driver. Or, if this is expected, maybe the core
should automatically clear the effected states when a channel is changed
from input to output.
Yes, either the s->state should be updated to match the outputs, or the
outputs should be rewritten to match the s->state. I'm not sure which
makes most sense, but it's outside the scope of this patch series
anyway. Physically, all channels configured as outputs go low on 8255
whenever the mode register is written to to set the directions.
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?
I'll wait for your reply on this and PATCH 01/11 before I refresh the series.
For now, I'd play it safe and filter the writes if the original code
filtered the writes.
[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!
The direction bits mask ensures that only the output channels are effected.
One of the patches in this series adds that to comedi_dio_insn_bits(). It seems
like a good idea.
The original also filtered on which data bits had flipped, which was
probably overkill!
It does seems a bit goofy that the io_bits mask occurs after the state has been
updated. The user could pass a mask and bits that changes the state of an
input channel but it would not actually get updated. Then the user could pass
a mask and bits that changes an output channel which would get updated but
the previous change in the state would also them update the channel configured
as an input.
It shouldn't matter when the value gets written to an input channel
while it is an input, though it may matter if the channel is later
reconfigured as an output.
This begs the question of whether we should be considering s->io_bits at
all in comedi_dio_insn_bits(), as that means you can't change the output
state of an input channel in advance of reconfiguring it as an output.
Perhaps comedi_dio_insn_bits() should be ANDing the mask with
((1<<s->n_chans)-1) instead of with s->io_bits?
Again, I think this extra checking of the mask, or changed bits, doesn't add any
real time saving.
[snip]
diff --git a/drivers/staging/comedi/drivers/ni_6527.c b/drivers/staging/comedi/drivers/ni_6527.c
index c2745f2..25f9a12 100644
[snip]
ni6527 is one of those drivers that were filtering the register writes
based on the mask.
Noted.
[snip]
diff --git a/drivers/staging/comedi/drivers/pcl711.c b/drivers/staging/comedi/drivers/pcl711.c
index e859f85..a94bfd9 100644
[snip]
pcl711 is another of those drivers that were filtering the register
writes based on the mask.
Again, noted.
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!
I noticed that also. Forgot to mention it in the commit message.
Another reason the helper function is useful. It keeps all the 'mask'
and 'bits' stuff in one place and minimizes the chance for bugs like
this.
That's true. It doesn't help with the ssv_dnp driver returning the read
bits in the wrong element of data[] though.
--
-=( 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
