Re: [PATCH] iio:dac: Add ad5755 driver

[Lars-Peter Clausen <lars@metafoo.de>]

On 09/13/2012 10:34 PM, Jonathan Cameron wrote:
> On 09/13/2012 02:13 PM, Lars-Peter Clausen wrote:
>> This patch adds support for the AD5755, AD5755-1, AD5757, AD5735, AD5737 16 and
>> 14 bit quad-channel DACs. The AD5757/AD5737 only have current outputs, but
>> for the AD5755/AD5757 each of the outputs can be configured to either be a
>> voltage or a current output. We only allow to configure this at device probe
>> time since usually this needs to match the external circuitry and should not be
>> changed on the fly.
>>
> Fair enough.
> 
> Couple of little bits inline.

Thanks for the review.

>> +static void ad5755_get_min_max(struct ad5755_state *st,
>> +	struct iio_chan_spec const *chan, int *min, int *max)
>> +{
>> +	enum ad5755_mode mode = st->ctrl[chan->channel] & 7;
>> +
> This might be a bit cleaner as a lookup table than as
> a switch statement.
> static const modemaxmin[2][] = {
>        [AD5755_MOD_VOLTAGE_0V_5V] = { 0, 5000},
> etc.
> };

Yes, definitely. I actually feel kind of stupid now for implementing this
with a switch case statement ;)

>> +	switch (mode) {
>> +	case AD5755_MODE_VOLTAGE_0V_5V:
>> +		*min = 0;
>> +		*max = 5000;
>> +		break;
>> +	case AD5755_MODE_VOLTAGE_0V_10V:
>> +		*min = 0;
>> +		*max = 10000;
>> +		break;
>> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
>> +		*min = -5000;
>> +		*max = 5000;
>> +		break;
>> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
>> +		*min = -10000;
>> +		*max = 10000;
>> +		break;
>> +	case AD5755_MODE_CURRENT_4mA_20mA:
>> +		*min = 4;
>> +		*max = 20;
>> +		break;
>> +	case AD5755_MODE_CURRENT_0mA_20mA:
>> +		*min = 0;
>> +		*max = 20;
>> +		break;
>> +	case AD5755_MODE_CURRENT_0mA_24mA:
>> +		*min = 0;
>> +		*max = 24;
>> +		break;
>> +	default:
> When does this apply?

In practice never. But 'st->ctrl[chan->channel] & 7' could in theory lead
here, so it was to keep the compiler to not complain about maybe
uninitialized variables.

>> +		*min = 0;
>> +		*max = 1;
>> +		break;
>> +	}
>> +}
>>