RE: [PATCH 08/11] staging: comedi: rtd520: tidy up rtd_ao_winsn()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux