On 2013-08-30 02:00, H Hartley Sweeten wrote:
Make sure that only the state of the channels configured as outputs are
updated.
Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
Cc: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
drivers/staging/comedi/drivers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 75d877b..777be6d 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -198,7 +198,7 @@ EXPORT_SYMBOL_GPL(comedi_dio_insn_config);
unsigned int comedi_dio_update_state(struct comedi_subdevice *s,
unsigned int *data)
{
- unsigned int mask = data[0];
+ unsigned int mask = data[0] & s->io_bits; /* outputs only */
unsigned int bits = data[1];
if (mask) {
I have had second thoughts about that as some application code could be
trying to set the state on an input channel in advance of reconfiguring
it as an output. That wouldn't work on all hardware but would work on
some hardware (e.g. hardware that has a "data output" register plus a
"data direction" register).
But filtering the mask in this way seems like a wholesale change for the
sake of a handful of drivers, and for some of those handful of drivers
it probably doesn't matter anyway.
Callers of comedi_dio_update_state() that want to limit register writes
to outputs only can AND the return value with s->io_bits. The s->state
bits would still be updated for the input channels, but they wouldn't
get written to the hardware (unless they fall in the same register as
the output channels).
For the drivers in patch 17:
* addi_apci_16xx looks like it can just ignore s->io_bits. All the DIO
channels are in the same register anyway. Values written to input
channels would probably take effect if the channels are reconfigured as
outputs.
* addi_apci_3xxx looks like it can just ignore s->io_bits when writing
to registers, although it still uses io_bits to decide which registers
to read back.
* ii_pci20kc looks like it can just ignore s->io_bits. Values written
to input channels would probably take effect if the channels are
reconfigured as outputs.
* me4000 looks like it can just ignore s->io_bits. Values written to
input channels would take effect if the channels are reconfigured as
outputs.
* me_daq looks like it can just ignore s->io_bits when writing to
registers, although it still uses io_bits to decide what to read back.
* pcmuio does weird stuff in its insn_bits handler with s->state. I
think s->state should be the intended output state and not depend on
what gets read from the registers. It does need to take s->io_bits into
account when setting the registers. I think it's worth leaving pcmuio
out of the patch and fixing it properly later.
* s626 looks like it can just ignore s->io_bits. All the DIO channels
for the subdevice are in the same register anyway. Values written to
input channels would probably take effect if the channels are
reconfigured as outputs.
It's probably a good idea to limit the mask to channels that actually
exist though, something like:
unsigned int chanmask = s->n_chans < 32 ?
(1U << s->n_chans) - 1 : 0xffffffff;
unsigned int mask = data[0] & chanmask;
unsigned int bits = data[1];
In summary, I suggest:
1) Changing comedi_dio_update_state() to limit the mask to all channels
instead of channels currently configured as outputs.
2) Skipping the pcmuio driver for now.
3) Removing the s->io_bits filter when writing to registers in the other
drivers.
--
-=( 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
