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