iio:dac:ad5686: Add AD5691R/AD5692R/AD5693/AD5693R support

[stefpopa]

The AD5691R/AD5692R/AD5693/AD5693R are a family of one channel DACs with 12-bit,
14-bit and 16-bit precision respectively. The devices have either no built-in reference,
or built-in 2.5V reference.

These devices are pretty similar to AD5671R/AD5675R/AD5694/AD5694R/AD5695R/AD5696/AD5696R,
except that they have only one channel. Another difference is that they use a write control
register(addr 0x04) for setting the power down modes and the internal reference instead of
sepparate registers for each function.

Datasheet:
http://www.analog.com/media/en/technical-documentation/data-sheets/AD5693R_5692R_5691R_5693.pdf

Signed-off-by: Stefan Popa [email protected]

[commodo]

nitpick: indentation may need adjustment here

[stefpopa]

I agree

[commodo]

i'm not sure if using the number of channels as a differentiating parameter is the best idea [from a general design perspective];
that can be problematic in future cases, where some more DACs would need to be added and have 1 channel, and (chan->channel * 2) as shift

maybe, a new set of static int ad569x_xxx_powerdown_mode functions would be needed ;
and assigned to the new devices you are adding ;

[commodo]

hmm ; maybe static int ad5693_xxx_powerdown_mode would be a better name ;

[stefpopa]

I could define a new struct iio_chan_spec_ext_info ad5693_ext_info[] with different implementations for the read_dac_powerdown and write_dac_powerdown functions and pass it to .ext_info filed of AD5868_CHANNEL

[lclausen-adi]

Just add a field to chip_info struct

[lclausen-adi]

Something like this: https://github.com/analogdevicesinc/linux/blob/master/drivers/iio/dac/ad5064.c#L56-L66

[stefpopa]

Ah, ok! Thanks!

[commodo]

same here ; i would not use the number of channels as a differentiating parameter ;
maybe a wrapper function would be needed here ;

also, i'm a bit curios ; you are adding DACs with one channel, but this code handles cases for 1, 4 & 8 channels ;
can you detail this a bit ? :)

[lclausen-adi]

The 8 channel handling should probably have been part of the patch that adds the 8 channel DACs

[stefpopa]

You are right about the 8 channel case. It should be included in a different commit.

[commodo]

would it make sense to move the st->pwr_down_mode field from the ad5686_state struct to the chip_info struct ?

you could initialize it [same as the regmap info] and then use that directly via st->chip_info->pwr_down_mode

[lclausen-adi]

The field is changed at runtime.

[commodo]

oh, my bad ; i did not check in depth that;
then, would it make sense to add the initial pwr_down_mode on chip_info ?
and then we could have:
st->pwr_down_mode = st->chip_info->pwr_down_mode;
at init time ?

[lclausen-adi]

The way this field works is that every channel occupies 2 bits. And for each of those channels we set those 2 bits to b01 when initializing it. If you wanted something that doesn't use if (...) else if (...) you could do

st->pwr_down_mode = 0x55555555;
st->pwr_down_mode &= (1 << (indio_dev->num_channels*2)) - 1; /* Mask out unused channels */

[commodo]

ah
thanks for the clarification :)
if (...) else if (...) should be fine in this case

[lclausen-adi]

Typo in the commit message: "sepparate"

[stefpopa]

changelog V1 -> V2:

• Fixed typo in the commit message
• Added a different commit for initializing the power down mode for 8 channel DACs
• Added a regmap_type field to the ad5686_chip_info struct. This field is used as a differentiating parameter instead of num_channels variable for power-down mode or internal voltage reference operations.

[lclausen-adi]

In general I'm OK with this patch. But what I'd do different is rather than having the regmap specific shift in all those different places I'd keep pwr_down_mask and pwr_down_mode as they are. And only here where we write the value out to the register apply the shift.

val = st->pwr_down_mask & st->pwr_down_mode;
if (st->chip_info->regmap_type == AD5693_REGMAP)
    val <<= 13; /* AD5693 register layout is slightly different */

The other thing you need to take care of here is to not clear the REF bit if it was set in probe. I'd keep a copy of whether REF is enabled or not in the state struct and then set val here accordingly.

[stefpopa]

You are right, I missed the fact that REF bit will be overwritten.
Regarding pwr_down_mode variable, indeed ad5686_write_dac_powerdown is the only place where we actually write a value to the register. However, shouldn't we be able to get and set the power mode independently for each channel?

[lclausen-adi]

Sorry, what I meant was keep the code handling pwr_down_mode and pwr_down_mask as it is (with the shift of chan->channel * 2).

But only here in the write function apply the extra shift of 13 for AD5693.

Or do you mean we should also write the register in ad5686_set_powerdown_mode()?

[stefpopa]

What I mean is that by doing operations such as echo three_state > out_voltage0_powerdown_mode and cat out_voltage0_powerdown_mode we will only write/read the pwr_down_mode variable without reading or writing the actual register value. If we keep the shift of chan->channel * 2 won't we get wrong readings for AD5693?

[lclausen-adi]

Think about pwr_down_mode and pwr_down_mask as purely virtual storage not associated with any hardware register at all. Each channel gets two bits in these fields. channel 0 starts at offset 0, channel 1 starts at offset 2 and so on. Kind of like an array. If you do echo ... > ...powerdown_mode; cat ...power_downmode this will work fine even if there is no hardware attached to the driver at all.

Now when you want to write the register you have to translate the virtual value to the physical value that the register expects. For the AD5686 this will be trivial since both are the same and for the AD5693 the translation is still quite simple. The AD5693 only has one channel so the bits associated with that channel will be at offset 0 in pwr_down_mode. Now the register expects the bits to be at offset 13. So all you need to do is shift the virtual value by 13 to get to the physical value.

[stefpopa]

I got it now! Thanks! :)

[lclausen-adi]

Extra newline

[stefpopa]

changelog V2 -> V3:

• initialize pwr_down_mode variable by using a for loop and running through each channel
• make sure that the REF bit is not overwritten for devices which use the same register for setting both the powerdown mode and internal reference. This is done by checking the use_internal_vref which is set at start up.
• regmap_type is checked only in ad5686_write_dac_powerdown before the register is written.

[stefpopa]

Can you please take a look over this new commit as well? I also added support for Add AD5691R/AD5692R/AD5693/AD5693R DACs

[lclausen-adi]

Upstream won't like this. Try to run coccicheck (https://bottest.wiki.kernel.org/coccicheck) on your patches it will report potential issues.

make C=2 CHECK="scripts/coccicheck" drivers/iio/dac/

Should be

val = (st->pwr_down_mask & st->pwr_down_mode) << 13;
if (!st->use_internal_vref)
   val |= BIT(12); // Even better do a #define for this flag

[lclausen-adi]

Nitpick: I'd say 'is used' instead of 'should be used'

[lclausen-adi]

I'd keep these in alphabetical order.

[lclausen-adi]

crazy hardware people always changing stuff around, just because they can...

But there is still a bit of a pattern here. The code is a copy&pasted version of the AD5693 case. Maybe you can handle this in a more generic way. Like

switch (type) {
case AD5683_REGMAP:
    shift = 12; 
    break;
case AD5693_REGMAP:
    shift = 12; 
    break;
}

[lclausen-adi]

This makes me wonder, how is readback implemented on AD5693? Does reading back the DAC value work?

[lclausen-adi]

OK, the part has no SPI interface, so that answers the question.

[stefpopa]

:)

[lclausen-adi]

But still, do we have to change the i2c read function for AD5693? Looking at the datasheet it looks different from the multi-channel chips.

[stefpopa]

The read operation is similar. Also, I have tested this

[lclausen-adi]

Just one general comment.

Instead of

if (st->chip_info->regmap_type == A_REGMAP)
   ...
else if (st->chip_info->regmap_type == B_REGMAP)
   ...
else if (st->chip_info->regmap_type == C_REGMAP)
   ...

Try to use

switch (st->chip_info->regmap_type) {
case A_REGMAP:
   ...
   break;
case B_REGMAP:
   ...
   break;
case C_REGMAP:
   ...
   break;
}

In my opinion that makes it easier to follow.

[stefpopa]

changelog V3 -> V4:

• Used switch(case) instead of if/else
• Made ad5686_write_dac_powerdown() function more generic
• Defined AD5693_REF_BIT_MSK and AD5683_REF_BIT_MSK
• ad5686_regmap_type constants are defined in alphabetical order

[stefpopa]

Any remarks? Is it OK to merge?

[stefpopa]

changelog V4 -> V5:

• This pull request includes only the commit which adds support for AD5691R/AD5692R/AD5693/AD5693R.
• Support for AD5683R DACs family will be treated in a different pull request.

Changes from the first version:

• Used switch(case) instead of if/else
• Made ad5686_write_dac_powerdown() function more generic
• Defined AD5693_REF_BIT_MSK
• make sure that the REF bit is not overwritten for devices which use the same register for setting both the powerdown mode and internal reference. This is done by checking the use_internal_vref which is set at start up.
• Added a regmap_type field to the ad5686_chip_info struct. This field is used as a differentiating parameter instead of num_channels variable for power-down mode or internal voltage reference operations.
• regmap_type is checked only in ad5686_write_dac_powerdown before the register is written.