On 2013-10-01 18:14, Hartley Sweeten wrote:
On Tuesday, October 01, 2013 3:27 AM, Ian Abbott wrote:
On 2013-10-01 01:55, H Hartley Sweeten wrote:
Each analog output channel is individually configurable. The current
analog output 'range_table' selection in this driver assumes that all
the channels are configured as either bipolar or unipolar. User option[2]
is then used during the attach to select the range_table to use.

Add a comedi_lrange table to allow the user to specify the actual range
to use for each channel.

Make sure the approriate DAC2S bit is set in the CFG2 register when writing
straight binary unipolar values to the DAC. Also, the data only needs to
be munged to two's complement when writing bipolar values.

Since this bit only affects how the hardware interprets values written
to the DAC, and all comedi data values are straight binary already,
can't you just leave it initialized to straight binary (already done by
atao_reset()) and write the comedi data values as is without munging?

Ian,

This is the comment in the at-ao-6/10 user manual about the DAC2S*<8..0>
bits in the CFG2 register:

DAC Input Data Format Select Bits.
When DAC2S0* is cleared, a two's complement format is used for
the 16-bit data written to DAC0 and DAC1. This format is useful
for the bipolar analog output mode. When DAC2S0* is set, a
straight binary format is used for the 16-bit data written to DAC0
and DC1. This format is useful for the unipolar analog output
mode. Bit DAC2S2* controls the data format of DAC2 and DAC3,
and in the same way bits DAC2S4* to DAC2S8* control the
corresponding DACs.

Ah, so the driver is currently broken. I'd assumed the register was already being initialized to match comedi's data format.


Signed-off-by: H Hartley Sweeten <hswee...@visionengravers.com>
Cc: Ian Abbott <abbo...@mev.co.uk>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
   drivers/staging/comedi/drivers/ni_at_ao.c | 41 
++++++++++++++++++++++++++-----
   1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/comedi/drivers/ni_at_ao.c 
b/drivers/staging/comedi/drivers/ni_at_ao.c
index 10e3e947..e483c7e 100644
--- a/drivers/staging/comedi/drivers/ni_at_ao.c
+++ b/drivers/staging/comedi/drivers/ni_at_ao.c
@@ -29,9 +29,6 @@
    *   [0] - I/O port base address
    *   [1] - IRQ (unused)
    *   [2] - DMA (unused)
- *   [3] - analog output range, set by jumpers on hardware
- *         0 for -10 to 10V bipolar
- *         1 for 0V to 10V unipolar
    */

   #include <linux/module.h>
@@ -99,6 +96,32 @@
   #define ATAO_2_RTSISHFT_RSI  (1 << 0)
   #define ATAO_2_RTSISTRB_REG  0x07

+/*
+ * Each Analog Output channel can be configured indepentently to
+ * have either a bipolar (factory setting) or unipolar output.
+ * This selection is done with jumpers on the board.
+ *
+ * There are also jumpers to select if the channel uses the internal
+ * 10V reference voltage (factory default) or an exteral reference
+ * voltage applied to the EXTREFx pin on the I/O connector. Each
+ * EXTREFx signal is shared by two DACs that are in the same chip;
+ * that is DAC0 and DAC1 share EXTREF0, DAC2 and DAC3 share EXTREF2,
+ * etc. Each channel is individually configured.
+ *
+ * The following ranges cover all the configuration options. The user
+ * must select the correct range, based on the board configuration,
+ * to correctly calculate the values needed to produce a given output
+ * voltage.
+ */
+static const struct comedi_lrange atao_range = {
+       4, {
+               BIP_RANGE(10),          /* internal bipolar */
+               UNI_RANGE(10),          /* internal unipolar */
+               RANGE_ext(-1, 1),       /* external bipolar */
+               RANGE_ext(0, 1)         /* external unipolar */
+       }
+};
+
   struct atao_board {
        const char *name;
        int n_ao_chans;
@@ -143,9 +166,14 @@ static int atao_ao_insn_write(struct comedi_device *dev,
   {
        struct atao_private *devpriv = dev->private;
        unsigned int chan = CR_CHAN(insn->chanspec);
+       unsigned int range = CR_RANGE(insn->chanspec);
        unsigned int val;
        int i;

+       /* enable straight binary format for unipolar ranges */
+       if (comedi_range_is_unipolar(s, range))
+               outw(ATAO_CFG2_DACS(chan), dev->iobase + ATAO_CFG2_REG);
+

I don't think you need this of the munging of bipolar range values
below, but the above bit of code only seems to be half-working as it
doesn't write anything to the register for bipolar ranges.

Based on the information in the user manual, and since the data from the comedi
core is already in straight binary format, when the specified range is unipolar 
I set
the corresponding DAC2S* bit in the CFG register. This means the data does not
need to be munged.

But, I did miss clearing the DAC2S* bit for bipolar values. Good catch.

        if (chan == 0)
                atao_select_reg_group(dev, 1);

@@ -153,8 +181,9 @@ static int atao_ao_insn_write(struct comedi_device *dev,
                val = data[i];
                devpriv->ao_readback[chan] = val;

-               /* munge offset binary (unsigned) to two's complement */
-               val = comedi_offset_munge(s, val);
+               /* bipolar ranges use two's complement format */
+               if (comedi_range_is_bipolar(s, range))
+                       val = comedi_offset_munge(s, val);
                outw(val, dev->iobase + ATAO_AO_REG(chan));
        }

Just remove the edits from atao_ao_insn_write() and the remainder of the
patch is fine!

If I remove the code that messes with the DAC2S* bits the hardware will always
expect the values to be in two's complement mode.

Logically I think using two's complement for the bipolar ranges and straight 
binary
for the unipolar ranges makes the code clearer. But since the hardware can work
with either format I can redo this patch to just leave the DAC2S* bits cleared 
and
always munge the values or I can set all the bits and remove the munge,

Either way works for me.

Probably best to initialize the DAC2S* bits once in atao_reset() and leave them alone. It may be the case that changing a DAC2Sx bit has an immediate affect on the voltage output by channels 2x and 2x+1 (causing the voltage output voltage to jump by half the output range), which would be undesirable since each bit affects 2 channels. In this case, it makes sense to initialize them all to straight binary to avoid having to munge the values.

--
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbo...@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to