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.
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.
> 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.
> [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.
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.
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.
Regards,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
