On 24/09/15 18:20, Hartley Sweeten wrote:
On Thursday, September 24, 2015 4:00 AM, Ian Abbott wrote:
On 23/09/15 20:16, H Hartley Sweeten wrote:
For aesthetics, rename this function and tidy it up a bit.
Use the comedi_range_is_bipolar() and comedi_offset_munge() helpers
to handle the 2's complement munging.
Save the readback value after the conversion is complete.
Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
Cc: Ian Abbott <abbotti@xxxxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
drivers/staging/comedi/drivers/rtd520.c | 35 ++++++++++++++++-----------------
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/comedi/drivers/rtd520.c b/drivers/staging/comedi/drivers/rtd520.c
index 5fe92c4..2df8494 100644
--- a/drivers/staging/comedi/drivers/rtd520.c
+++ b/drivers/staging/comedi/drivers/rtd520.c
@@ -1046,42 +1046,41 @@ static int rtd_ao_eoc(struct comedi_device *dev,
return -EBUSY;
}
-static int rtd_ao_winsn(struct comedi_device *dev,
- struct comedi_subdevice *s, struct comedi_insn *insn,
- unsigned int *data)
+static int rtd_ao_insn_write(struct comedi_device *dev,
+ struct comedi_subdevice *s,
+ struct comedi_insn *insn,
+ unsigned int *data)
{
struct rtd_private *devpriv = dev->private;
- int i;
- int chan = CR_CHAN(insn->chanspec);
- int range = CR_RANGE(insn->chanspec);
+ unsigned int chan = CR_CHAN(insn->chanspec);
+ unsigned int range = CR_RANGE(insn->chanspec);
int ret;
+ int i;
/* Configure the output range (table index matches the range values) */
writew(range & 7, dev->mmio + LAS0_DAC_CTRL(chan));
for (i = 0; i < insn->n; ++i) {
- int val = data[i] << 3;
+ unsigned int val = data[i];
- /* VERIFY: comedi range and offset conversions */
+ /* bipolar ranges use 2's complement values */
+ if (comedi_range_is_bipolar(s, range))
+ val = comedi_offset_munge(s, val);
- if ((range > 1) && (data[i] < 2048)) { /* bipolar */
- /* offset and sign extend */
- val = (((int)data[i]) - 2048) << 3;
- } else { /* unipolor */
- val = data[i] << 3;
- }
+ /* the 12-bit data is left justified to 16-bits */
+ val <<= 3;
12 + 3 = 15. The original mapping of values for bipolar ranges looks a
Bit peculiar, and is different to the new mapping. Let's see:
Comedi Old raw << 3 New raw << 3
------------- --------------- ---------------
[0x000,0x7ff] [0xc000,0xfff8] [0x4000,0x7ff8]
[0x800,0xfff] [0x4000,0x7ff8] [0x0000,0x3ff8]
Section 4.3.3.1 of the manual
<http://www.rtd.com/manuals/DM/DM7520_DM7530_PCI4520_Hardware_manual_v20.pdf>
says, "A write programs the D/A1 FIFO in the format shown below.
Because of the extended sign bit in a bipolar and in unipolar mode also
the data format is two's complement."
| D15 | D14 - D03 | D02 | D01 | D00 |
| Sign | 12-bit data | (see manual) |
The "extended sign bit" and "two's complement" suggests that the mapping
of values for bipolar ranges ought to be:
Comedi Raw << 3
------------- ---------------
[0x000,0x7ff] [0xc000,0xfff8]
[0x800,0xfff] [0x0000,0x3ff8]
I guess the original mapping works (assuming it does) because of the the
redundancy. Perhaps bit 14 gets ignored in favour of bit 15 in two's
complement mode. In fact bit 14 seems to be always '1' in the original
mapping!
I think it's safest to stick with the original mapping.
Ian,
Thanks for the link to the manual. I didn't have this one.
I agree that my comment about shifting the data is incorrect. But the original code
still looks wrong.
Section 4.3.3.1 does describe the register as you indicated:
| D15 | D14 - D03 | D02 | D01 | D00 |
| Sign | 12-bit data | (see manual) |
But the 'sign' for unipolar values is always 0 (+). For bipolar values it is the
same as bit 12 of the data.
The tables on pages 95 and 96 indicated that the data is 2's complement for
bipolar and offset binary for unipolar. Here's a chunk from each table:
Bipolar -5 to +5 -10 to +10
D/A Bit Weight millivolts millivolts
------------------- ----------- -----------
2047 (0x07ff) +4997.56 +9995.12
0 (0x0000) 0.00 0.00
-2048 (0x0800) -5000.00 -10000.00
Unipolar 0 to +5 0 to +10
D/A Bit Weight millivolts millivolts
------------------- ----------- -----------
4095 (0x0fff) +4998.78 +9997.56
2048 (0x0800) +2500.00 +5000.00
0 (0x0000) 0.00 0.00
This follows the concept used for comedi_offset_munge().
The piece in the original code that looks wrong is:
if ((range > 1) && (data[i] < 2048)) { /* bipolar */
This ends up flipping the sign for all the values. If that code is
changed to:
if ((range > 1) && (data[i] > 2048)) { /* bipolar */
The result is the same as:
if (comedi_range_is_bipolar(s, range))
val = comedi_offset_munge(s, val);
The value then needs to be shifted (<< 3) to match the register.
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);
I suspect the hardware is just ignoring bit 14 of the register value in
bipolar mode.
--
-=( 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
