subject:"\[PATCH 1\/2\] iio\:dac\:ad5686\: Add AD5691R\/AD5692R\/AD5693\/AD5693R support"

Re: [PATCH 1/2] iio:dac:ad5686: Add AD5691R/AD5692R/AD5693/AD5693R support

2018-05-18 Thread Jonathan Cameron

On Fri, 18 May 2018 17:16:34 +0300
Stefan Popa  wrote:

> 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 and
> AD5694/AD5694R/AD5695R/AD5696/AD5696R, except that they have 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 separate
> 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 

A few really quick comments inline.

> ---
>  drivers/iio/dac/ad5686.c | 88 
> +---
>  drivers/iio/dac/ad5686.h | 16 
>  drivers/iio/dac/ad5696-i2c.c |  7 +++-
>  3 files changed, 104 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index 89c5f08..be803e8 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c
> @@ -70,6 +70,8 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev 
> *indio_dev,
>   bool readin;
>   int ret;
>   struct ad5686_state *st = iio_priv(indio_dev);
> + unsigned int val, ref_bit_msk = 0;
> + u8 shift = 0;
>  
>   ret = strtobool(buf, );
>   if (ret)
> @@ -80,9 +82,22 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev 
> *indio_dev,
>   else
>   st->pwr_down_mask &= ~(0x3 << (chan->channel * 2));
>  
> - ret = st->write(st, AD5686_CMD_POWERDOWN_DAC, 0,
> - st->pwr_down_mask & st->pwr_down_mode);
> + switch (st->chip_info->regmap_type) {
> + case AD5686_REGMAP:
> + shift = 0;
> + ref_bit_msk = 0;
you initialize to these values above. Any reason to set them again here?

Actually I'd prefer you didn't set them above and only set them here as
then it is obvious what they are set to the for the two different cases.

> + break;
> + case AD5693_REGMAP:
> + shift = 13;
> + ref_bit_msk = AD5693_REF_BIT_MSK;
> + break;
default: 
some sort of error response.  We'll get compile warnings on this 
otherwise.

> + }
>  
> + val = ((st->pwr_down_mask & st->pwr_down_mode) << shift);
> + if (!st->use_internal_vref)
> + val |= ref_bit_msk;
> +
> + ret = st->write(st, AD5686_CMD_POWERDOWN_DAC, 0, val);
>  
>   return ret ? ret : len;
>  }
> @@ -175,6 +190,11 @@ static const struct iio_chan_spec_ext_info 
> ad5686_ext_info[] = {
>   .ext_info = ad5686_ext_info,\
>  }
>  
> +#define DECLARE_AD5693_CHANNELS(name, bits, _shift)  \
> +static struct iio_chan_spec name[] = {   \
> + AD5868_CHANNEL(0, 0, bits, _shift), \
> +}
> +
>  #define DECLARE_AD5686_CHANNELS(name, bits, _shift)  \
>  static struct iio_chan_spec name[] = {   \
>   AD5868_CHANNEL(0, 1, bits, _shift), \
> @@ -200,72 +220,112 @@ DECLARE_AD5676_CHANNELS(ad5676_channels, 16, 0);
>  DECLARE_AD5686_CHANNELS(ad5684_channels, 12, 4);
>  DECLARE_AD5686_CHANNELS(ad5685r_channels, 14, 2);
>  DECLARE_AD5686_CHANNELS(ad5686_channels, 16, 0);
> +DECLARE_AD5693_CHANNELS(ad5693_channels, 16, 0);
> +DECLARE_AD5693_CHANNELS(ad5692r_channels, 14, 2);
> +DECLARE_AD5693_CHANNELS(ad5691r_channels, 12, 4);
>  
>  static const struct ad5686_chip_info ad5686_chip_info_tbl[] = {
>   [ID_AD5671R] = {
>   .channels = ad5672_channels,
>   .int_vref_mv = 2500,
>   .num_channels = 8,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5672R] = {
>   .channels = ad5672_channels,
>   .int_vref_mv = 2500,
>   .num_channels = 8,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5675R] = {
>   .channels = ad5676_channels,
>   .int_vref_mv = 2500,
>   .num_channels = 8,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5676] = {
>   .channels = ad5676_channels,
>   .num_channels = 8,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5676R] = {
>   .channels = ad5676_channels,
>   .int_vref_mv = 2500,
>   .num_channels = 8,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5684] = {
>   .channels = ad5684_channels,
>   .num_channels = 4,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5684R] = {
>   .channels = ad5684_channels,
>   .int_vref_mv = 2500,
>   .num_channels = 4,
> +

Re: [PATCH 1/2] iio:dac:ad5686: Add AD5691R/AD5692R/AD5693/AD5693R support

2018-05-18 Thread Jonathan Cameron

On Fri, 18 May 2018 17:16:34 +0300
Stefan Popa  wrote:

> 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 and
> AD5694/AD5694R/AD5695R/AD5696/AD5696R, except that they have 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 separate
> 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 

A few really quick comments inline.

> ---
>  drivers/iio/dac/ad5686.c | 88 
> +---
>  drivers/iio/dac/ad5686.h | 16 
>  drivers/iio/dac/ad5696-i2c.c |  7 +++-
>  3 files changed, 104 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index 89c5f08..be803e8 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c
> @@ -70,6 +70,8 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev 
> *indio_dev,
>   bool readin;
>   int ret;
>   struct ad5686_state *st = iio_priv(indio_dev);
> + unsigned int val, ref_bit_msk = 0;
> + u8 shift = 0;
>  
>   ret = strtobool(buf, );
>   if (ret)
> @@ -80,9 +82,22 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev 
> *indio_dev,
>   else
>   st->pwr_down_mask &= ~(0x3 << (chan->channel * 2));
>  
> - ret = st->write(st, AD5686_CMD_POWERDOWN_DAC, 0,
> - st->pwr_down_mask & st->pwr_down_mode);
> + switch (st->chip_info->regmap_type) {
> + case AD5686_REGMAP:
> + shift = 0;
> + ref_bit_msk = 0;
you initialize to these values above. Any reason to set them again here?

Actually I'd prefer you didn't set them above and only set them here as
then it is obvious what they are set to the for the two different cases.

> + break;
> + case AD5693_REGMAP:
> + shift = 13;
> + ref_bit_msk = AD5693_REF_BIT_MSK;
> + break;
default: 
some sort of error response.  We'll get compile warnings on this 
otherwise.

> + }
>  
> + val = ((st->pwr_down_mask & st->pwr_down_mode) << shift);
> + if (!st->use_internal_vref)
> + val |= ref_bit_msk;
> +
> + ret = st->write(st, AD5686_CMD_POWERDOWN_DAC, 0, val);
>  
>   return ret ? ret : len;
>  }
> @@ -175,6 +190,11 @@ static const struct iio_chan_spec_ext_info 
> ad5686_ext_info[] = {
>   .ext_info = ad5686_ext_info,\
>  }
>  
> +#define DECLARE_AD5693_CHANNELS(name, bits, _shift)  \
> +static struct iio_chan_spec name[] = {   \
> + AD5868_CHANNEL(0, 0, bits, _shift), \
> +}
> +
>  #define DECLARE_AD5686_CHANNELS(name, bits, _shift)  \
>  static struct iio_chan_spec name[] = {   \
>   AD5868_CHANNEL(0, 1, bits, _shift), \
> @@ -200,72 +220,112 @@ DECLARE_AD5676_CHANNELS(ad5676_channels, 16, 0);
>  DECLARE_AD5686_CHANNELS(ad5684_channels, 12, 4);
>  DECLARE_AD5686_CHANNELS(ad5685r_channels, 14, 2);
>  DECLARE_AD5686_CHANNELS(ad5686_channels, 16, 0);
> +DECLARE_AD5693_CHANNELS(ad5693_channels, 16, 0);
> +DECLARE_AD5693_CHANNELS(ad5692r_channels, 14, 2);
> +DECLARE_AD5693_CHANNELS(ad5691r_channels, 12, 4);
>  
>  static const struct ad5686_chip_info ad5686_chip_info_tbl[] = {
>   [ID_AD5671R] = {
>   .channels = ad5672_channels,
>   .int_vref_mv = 2500,
>   .num_channels = 8,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5672R] = {
>   .channels = ad5672_channels,
>   .int_vref_mv = 2500,
>   .num_channels = 8,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5675R] = {
>   .channels = ad5676_channels,
>   .int_vref_mv = 2500,
>   .num_channels = 8,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5676] = {
>   .channels = ad5676_channels,
>   .num_channels = 8,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5676R] = {
>   .channels = ad5676_channels,
>   .int_vref_mv = 2500,
>   .num_channels = 8,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5684] = {
>   .channels = ad5684_channels,
>   .num_channels = 4,
> + .regmap_type = AD5686_REGMAP,
>   },
>   [ID_AD5684R] = {
>   .channels = ad5684_channels,
>   .int_vref_mv = 2500,
>   .num_channels = 4,
> + .regmap_type = AD5686_REGMAP,
>   },

[PATCH 1/2] iio:dac:ad5686: Add AD5691R/AD5692R/AD5693/AD5693R support

2018-05-18 Thread Stefan Popa

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 and
AD5694/AD5694R/AD5695R/AD5696/AD5696R, except that they have 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 separate
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 
---
 drivers/iio/dac/ad5686.c | 88 +---
 drivers/iio/dac/ad5686.h | 16 
 drivers/iio/dac/ad5696-i2c.c |  7 +++-
 3 files changed, 104 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
index 89c5f08..be803e8 100644
--- a/drivers/iio/dac/ad5686.c
+++ b/drivers/iio/dac/ad5686.c
@@ -70,6 +70,8 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev 
*indio_dev,
bool readin;
int ret;
struct ad5686_state *st = iio_priv(indio_dev);
+   unsigned int val, ref_bit_msk = 0;
+   u8 shift = 0;
 
ret = strtobool(buf, );
if (ret)
@@ -80,9 +82,22 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev 
*indio_dev,
else
st->pwr_down_mask &= ~(0x3 << (chan->channel * 2));
 
-   ret = st->write(st, AD5686_CMD_POWERDOWN_DAC, 0,
-   st->pwr_down_mask & st->pwr_down_mode);
+   switch (st->chip_info->regmap_type) {
+   case AD5686_REGMAP:
+   shift = 0;
+   ref_bit_msk = 0;
+   break;
+   case AD5693_REGMAP:
+   shift = 13;
+   ref_bit_msk = AD5693_REF_BIT_MSK;
+   break;
+   }
 
+   val = ((st->pwr_down_mask & st->pwr_down_mode) << shift);
+   if (!st->use_internal_vref)
+   val |= ref_bit_msk;
+
+   ret = st->write(st, AD5686_CMD_POWERDOWN_DAC, 0, val);
 
return ret ? ret : len;
 }
@@ -175,6 +190,11 @@ static const struct iio_chan_spec_ext_info 
ad5686_ext_info[] = {
.ext_info = ad5686_ext_info,\
 }
 
+#define DECLARE_AD5693_CHANNELS(name, bits, _shift)\
+static struct iio_chan_spec name[] = { \
+   AD5868_CHANNEL(0, 0, bits, _shift), \
+}
+
 #define DECLARE_AD5686_CHANNELS(name, bits, _shift)\
 static struct iio_chan_spec name[] = { \
AD5868_CHANNEL(0, 1, bits, _shift), \
@@ -200,72 +220,112 @@ DECLARE_AD5676_CHANNELS(ad5676_channels, 16, 0);
 DECLARE_AD5686_CHANNELS(ad5684_channels, 12, 4);
 DECLARE_AD5686_CHANNELS(ad5685r_channels, 14, 2);
 DECLARE_AD5686_CHANNELS(ad5686_channels, 16, 0);
+DECLARE_AD5693_CHANNELS(ad5693_channels, 16, 0);
+DECLARE_AD5693_CHANNELS(ad5692r_channels, 14, 2);
+DECLARE_AD5693_CHANNELS(ad5691r_channels, 12, 4);
 
 static const struct ad5686_chip_info ad5686_chip_info_tbl[] = {
[ID_AD5671R] = {
.channels = ad5672_channels,
.int_vref_mv = 2500,
.num_channels = 8,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5672R] = {
.channels = ad5672_channels,
.int_vref_mv = 2500,
.num_channels = 8,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5675R] = {
.channels = ad5676_channels,
.int_vref_mv = 2500,
.num_channels = 8,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5676] = {
.channels = ad5676_channels,
.num_channels = 8,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5676R] = {
.channels = ad5676_channels,
.int_vref_mv = 2500,
.num_channels = 8,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5684] = {
.channels = ad5684_channels,
.num_channels = 4,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5684R] = {
.channels = ad5684_channels,
.int_vref_mv = 2500,
.num_channels = 4,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5685R] = {
.channels = ad5685r_channels,
.int_vref_mv = 2500,
.num_channels = 4,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5686] = {
.channels = ad5686_channels,
.num_channels = 4,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5686R] = {
.channels = ad5686_channels,
.int_vref_mv = 2500,
.num_channels = 4,

[PATCH 1/2] iio:dac:ad5686: Add AD5691R/AD5692R/AD5693/AD5693R support

2018-05-18 Thread Stefan Popa

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 and
AD5694/AD5694R/AD5695R/AD5696/AD5696R, except that they have 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 separate
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 
---
 drivers/iio/dac/ad5686.c | 88 +---
 drivers/iio/dac/ad5686.h | 16 
 drivers/iio/dac/ad5696-i2c.c |  7 +++-
 3 files changed, 104 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
index 89c5f08..be803e8 100644
--- a/drivers/iio/dac/ad5686.c
+++ b/drivers/iio/dac/ad5686.c
@@ -70,6 +70,8 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev 
*indio_dev,
bool readin;
int ret;
struct ad5686_state *st = iio_priv(indio_dev);
+   unsigned int val, ref_bit_msk = 0;
+   u8 shift = 0;
 
ret = strtobool(buf, );
if (ret)
@@ -80,9 +82,22 @@ static ssize_t ad5686_write_dac_powerdown(struct iio_dev 
*indio_dev,
else
st->pwr_down_mask &= ~(0x3 << (chan->channel * 2));
 
-   ret = st->write(st, AD5686_CMD_POWERDOWN_DAC, 0,
-   st->pwr_down_mask & st->pwr_down_mode);
+   switch (st->chip_info->regmap_type) {
+   case AD5686_REGMAP:
+   shift = 0;
+   ref_bit_msk = 0;
+   break;
+   case AD5693_REGMAP:
+   shift = 13;
+   ref_bit_msk = AD5693_REF_BIT_MSK;
+   break;
+   }
 
+   val = ((st->pwr_down_mask & st->pwr_down_mode) << shift);
+   if (!st->use_internal_vref)
+   val |= ref_bit_msk;
+
+   ret = st->write(st, AD5686_CMD_POWERDOWN_DAC, 0, val);
 
return ret ? ret : len;
 }
@@ -175,6 +190,11 @@ static const struct iio_chan_spec_ext_info 
ad5686_ext_info[] = {
.ext_info = ad5686_ext_info,\
 }
 
+#define DECLARE_AD5693_CHANNELS(name, bits, _shift)\
+static struct iio_chan_spec name[] = { \
+   AD5868_CHANNEL(0, 0, bits, _shift), \
+}
+
 #define DECLARE_AD5686_CHANNELS(name, bits, _shift)\
 static struct iio_chan_spec name[] = { \
AD5868_CHANNEL(0, 1, bits, _shift), \
@@ -200,72 +220,112 @@ DECLARE_AD5676_CHANNELS(ad5676_channels, 16, 0);
 DECLARE_AD5686_CHANNELS(ad5684_channels, 12, 4);
 DECLARE_AD5686_CHANNELS(ad5685r_channels, 14, 2);
 DECLARE_AD5686_CHANNELS(ad5686_channels, 16, 0);
+DECLARE_AD5693_CHANNELS(ad5693_channels, 16, 0);
+DECLARE_AD5693_CHANNELS(ad5692r_channels, 14, 2);
+DECLARE_AD5693_CHANNELS(ad5691r_channels, 12, 4);
 
 static const struct ad5686_chip_info ad5686_chip_info_tbl[] = {
[ID_AD5671R] = {
.channels = ad5672_channels,
.int_vref_mv = 2500,
.num_channels = 8,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5672R] = {
.channels = ad5672_channels,
.int_vref_mv = 2500,
.num_channels = 8,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5675R] = {
.channels = ad5676_channels,
.int_vref_mv = 2500,
.num_channels = 8,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5676] = {
.channels = ad5676_channels,
.num_channels = 8,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5676R] = {
.channels = ad5676_channels,
.int_vref_mv = 2500,
.num_channels = 8,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5684] = {
.channels = ad5684_channels,
.num_channels = 4,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5684R] = {
.channels = ad5684_channels,
.int_vref_mv = 2500,
.num_channels = 4,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5685R] = {
.channels = ad5685r_channels,
.int_vref_mv = 2500,
.num_channels = 4,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5686] = {
.channels = ad5686_channels,
.num_channels = 4,
+   .regmap_type = AD5686_REGMAP,
},
[ID_AD5686R] = {
.channels = ad5686_channels,
.int_vref_mv = 2500,
.num_channels = 4,
+

Re: [PATCH 1/2] iio:dac:ad5686: Add AD5691R/AD5692R/AD5693/AD5693R support

Re: [PATCH 1/2] iio:dac:ad5686: Add AD5691R/AD5692R/AD5693/AD5693R support

[PATCH 1/2] iio:dac:ad5686: Add AD5691R/AD5692R/AD5693/AD5693R support

[PATCH 1/2] iio:dac:ad5686: Add AD5691R/AD5692R/AD5693/AD5693R support

4 matches

Site Navigation

Mail list logo

Footer information