[PATCH 20/21] staging: comedi: ni_at_ao: fix analog output ranges

[Hartley Sweeten]

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.

>> 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.

>>
>> @@ -361,7 +390,7 @@ static int atao_attach(struct comedi_device *dev, struct comedi_devconfig *it) 
>>   	s->subdev_flags	= SDF_WRITABLE;
>>   	s->n_chan	= board->n_ao_chans;
>>   	s->maxdata	= 0x0fff;
>> -	s->range_table	= it->options[3] ? &range_unipolar10 : &range_bipolar10;
>> +	s->range_table	= &atao_range;
>>   	s->insn_write	= atao_ao_insn_write;
>>   	s->insn_read	= atao_ao_insn_read;
>
> Since you mentioned it in your patch 00 message, I'm not bothered about 
> removing the configuration option as the new way of handling the range 
> is more flexible.  (The old way should probably have had one 
> configuration option per channel anyway.)

OK. Let me know what you prefer in the (*insn_write) function.

Thanks,
Hartley