[PATCH 20/21] staging: comedi: ni_at_ao: fix analog output ranges
Ian Abbott
abbotti at mev.co.uk
Wed Oct 2 09:04:29 UTC 2013
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 <hsweeten at visionengravers.com>
>>> Cc: Ian Abbott <abbotti at mev.co.uk>
>>> Cc: Greg Kroah-Hartman <gregkh at 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: <abbotti at mev.co.uk> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
More information about the devel
mailing list