On 14/08/15 18:03, Hartley Sweeten wrote:
On Thursday, August 13, 2015 2:29 AM, Ian Abbott wrote:
On 13/08/15 01:00, H Hartley Sweeten wrote:
The comedi_offset_munge() helper is used to convert unsigned int data
values from the comedi offset binary format to two's complement values
for hardware that needs the data in that format. The comedi data is
always checked against s->maxdata before writing to the hardware so
the current implementation always works correctly.
It also works to convert the hardware two's complement data into the
offset binary format used by comedi. Unfortunately, some boards
return extra data in the upper bits. The current implementation
ends up leaving these bits in the converted value.
Mask the converted value to ensure that any extra bits are removed.
Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
Cc: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
drivers/staging/comedi/comedidev.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
index 28a5d3a..1c8ed5d 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -388,7 +388,7 @@ static inline bool comedi_chan_range_is_external(struct comedi_subdevice *s,
static inline unsigned int comedi_offset_munge(struct comedi_subdevice *s,
unsigned int val)
{
- return val ^ s->maxdata ^ (s->maxdata >> 1);
+ return (val ^ s->maxdata ^ (s->maxdata >> 1)) & s->maxdata;
}
/**
I don't really like this. I think the function should only be concerned
with converting samples between 2's complement and offset binary format
and leave any other manipulations up to the driver. For example, the
amplc_pci230 driver needs to right-shift the raw register value from
PCI230 in addition to converting from 2's complement (although it does
not currently use comedi_offset_munge() to do it). This change to
comedi_offset_munge() also hides whether there may or may not have been
extra bits that needed to be stripped.
I disagree.
The drivers should be dealing with any of the "extra" bits and any special shifting
of the data before calling comedi_offset_munge() to convert the data. This is
especially true in the (*insn_read) operations where the samples read from the
hardware are directly added to the returned data:
val = inx(iobase);
/*
* Handle any special "extra" bits in the sample data and/or any shifting
* of the sample that only the driver knows about.
*/
/* return the munged data to the user */
data[i] = comedi_offset_munge(val);
This way the "val" does not require any extra masking before doing the conversion
and returning the data to the user. The right-shift operation is still handled by the
driver because it's the only one that knows this is needed.
My argument is that comedi_offset_munge() shouldn't be responsible for
stripping these extra bits, it should be the driver's responsibility to
do that before calling comedi_offset_munge().
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Web: http://www.mev.co.uk/ )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
