On Thursday, September 24, 2015 10:57 AM, Ian Abbott wrote:
> On 24/09/15 18:43, Ian Abbott wrote:
>> On 24/09/15 18:20, Hartley Sweeten wrote:
>>> I guess the sign bit would also need to be extended for the bipolar
>>> values. So:
>>>
>>> for (i = 0; i < insn->n; ++i) {
>>> unsigned int val = data[i];
>>>
>>> /* bipolar ranges use 2's complement values */
>>> if (comedi_range_is_bipolar(s, range)) {
>>> val = comedi_offset_munge(s, val);
>>> /* extend the sign bit */
>>> if (val > 2048)
>>> val |= 0x1000;
>>> }
>>>
>>> /* shift 12-bit data (+sign) to match the register */
>>> val <<= 3;
>>>
>>> How does that look?
>>
>> It looks okay except that the test for extending the sign bit should be
>> 'if (val >= 2048)'. You could also avoid the conditional and extend the
>> bit regardless:
>>
>> val += (val & 0x800);
>
> Although that assumes that val is in range to start with, otherwise the
> carry from the '+' might propagate too far. There are various ways to
> fix that, e.g.:
>
> val |= (val & 0x800) << 1;
All the data values passed with an INSN_WRITE are validated by parse_insn()
before the (*insn_write) is called. But, aesthetically I like the change above.
Or, if it's based on 'maxdata'...
val |= (val & ((s->maxdata + 1) >> 1) << 1;
But, that starts to look like black magic....
Regards,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
