[PATCH 04/11] staging: comedi: drivers: use comedi_dio_insn_bits()

Ian Abbott abbotti at mev.co.uk
Thu Aug 29 17:28:08 UTC 2013


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 at visionengravers.com>
>>> Cc: Ian Abbott <abbotti at mev.co.uk>
>>> Cc: Greg Kroah-Hartman <gregkh at 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.
>
> 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 at mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-


More information about the devel mailing list