Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 20 May 2018 11:37:13 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Stefan Popa <stefan.popa@analog.com>
Cc:     <Michael.Hennerich@analog.com>, <lars@metafoo.de>,
        <knaack.h@gmx.de>, <pmeerw@pmeerw.net>, <linux-pm@vger.kernel.org>,
        <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] iio:dac:ad5686: Add
 AD5691R/AD5692R/AD5693/AD5693R support
Message-ID: <20180520113713.5c22a99a@archlinux>
In-Reply-To: <1526656970-28852-1-git-send-email-stefan.popa@analog.com>
References: <1526656970-28852-1-git-send-email-stefan.popa@analog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 18 May 2018 18:22:50 +0300
Stefan Popa <stefan.popa@analog.com> 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 <stefan.popa@analog.com>

Nice fast turn around and a nice little patch.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> ---
> Changes in v2:
> 	- Removed the initialization for ref_bit_msk and shift variables
> 	  from ad5686_write_dac_powerdown() and ad5686_probe().
> 	- Added the the missing default case from
> 	  ad5686_write_dac_powerdown() and ad5686_probe().
> 
>  drivers/iio/dac/ad5686.c     | 93 +++++++++++++++++++++++++++++++++++++++++---
>  drivers/iio/dac/ad5686.h     | 16 ++++++++
>  drivers/iio/dac/ad5696-i2c.c |  7 +++-
>  3 files changed, 109 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index 89c5f08..1fc0c56 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;
> +	u8 shift;
>  
>  	ret = strtobool(buf, &readin);
>  	if (ret)
> @@ -80,9 +82,24 @@ 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;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	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 +192,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 +222,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,
> +		.regmap_type = AD5686_REGMAP,
> +	},
> +	[ID_AD5691R] = {
> +		.channels = ad5691r_channels,
> +		.int_vref_mv = 2500,
> +		.num_channels = 1,
> +		.regmap_type = AD5693_REGMAP,
> +	},
> +	[ID_AD5692R] = {
> +		.channels = ad5692r_channels,
> +		.int_vref_mv = 2500,
> +		.num_channels = 1,
> +		.regmap_type = AD5693_REGMAP,
> +	},
> +	[ID_AD5693] = {
> +		.channels = ad5693_channels,
> +		.num_channels = 1,
> +		.regmap_type = AD5693_REGMAP,
> +	},
> +	[ID_AD5693R] = {
> +		.channels = ad5693_channels,
> +		.int_vref_mv = 2500,
> +		.num_channels = 1,
> +		.regmap_type = AD5693_REGMAP,
>  	},
>  	[ID_AD5694] = {
>  		.channels = ad5684_channels,
>  		.num_channels = 4,
> +		.regmap_type = AD5686_REGMAP,
>  	},
>  	[ID_AD5694R] = {
>  		.channels = ad5684_channels,
>  		.int_vref_mv = 2500,
>  		.num_channels = 4,
> +		.regmap_type = AD5686_REGMAP,
>  	},
>  	[ID_AD5696] = {
>  		.channels = ad5686_channels,
>  		.num_channels = 4,
> +		.regmap_type = AD5686_REGMAP,
>  	},
>  	[ID_AD5696R] = {
>  		.channels = ad5686_channels,
>  		.int_vref_mv = 2500,
>  		.num_channels = 4,
> +		.regmap_type = AD5686_REGMAP,
>  	},
>  };
>  
> @@ -276,7 +338,9 @@ int ad5686_probe(struct device *dev,
>  {
>  	struct ad5686_state *st;
>  	struct iio_dev *indio_dev;
> -	int ret, voltage_uv = 0;
> +	unsigned int val, ref_bit_msk;
> +	u8 cmd;
> +	int ret, i, voltage_uv = 0;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>  	if (indio_dev == NULL)
> @@ -310,7 +374,8 @@ int ad5686_probe(struct device *dev,
>  		st->vref_mv = st->chip_info->int_vref_mv;
>  
>  	/* Set all the power down mode for all channels to 1K pulldown */
> -	st->pwr_down_mode = 0x55;
> +	for (i = 0; i < st->chip_info->num_channels; i++)
> +		st->pwr_down_mode |= (0x01 << (i * 2));
>  
>  	indio_dev->dev.parent = dev;
>  	indio_dev->name = name;
> @@ -319,8 +384,24 @@ int ad5686_probe(struct device *dev,
>  	indio_dev->channels = st->chip_info->channels;
>  	indio_dev->num_channels = st->chip_info->num_channels;
>  
> -	ret = st->write(st, AD5686_CMD_INTERNAL_REFER_SETUP,
> -			0, !!voltage_uv);
> +	switch (st->chip_info->regmap_type) {
> +	case AD5686_REGMAP:
> +		cmd = AD5686_CMD_INTERNAL_REFER_SETUP;
> +		ref_bit_msk = 0;
> +		break;
> +	case AD5693_REGMAP:
> +		cmd = AD5686_CMD_CONTROL_REG;
> +		ref_bit_msk = AD5693_REF_BIT_MSK;
> +		st->use_internal_vref = !voltage_uv;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto error_disable_reg;
> +	}
> +
> +	val = (voltage_uv | ref_bit_msk);
> +
> +	ret = st->write(st, cmd, 0, !!val);
>  	if (ret)
>  		goto error_disable_reg;
>  
> diff --git a/drivers/iio/dac/ad5686.h b/drivers/iio/dac/ad5686.h
> index 05f0ce9..6c6879d 100644
> --- a/drivers/iio/dac/ad5686.h
> +++ b/drivers/iio/dac/ad5686.h
> @@ -35,6 +35,9 @@
>  #define AD5686_LDAC_PWRDN_100K			0x2
>  #define AD5686_LDAC_PWRDN_3STATE		0x3
>  
> +#define AD5686_CMD_CONTROL_REG			0x4
> +#define AD5693_REF_BIT_MSK			BIT(12)
> +
>  /**
>   * ad5686_supported_device_ids:
>   */
> @@ -49,6 +52,10 @@ enum ad5686_supported_device_ids {
>  	ID_AD5685R,
>  	ID_AD5686,
>  	ID_AD5686R,
> +	ID_AD5691R,
> +	ID_AD5692R,
> +	ID_AD5693,
> +	ID_AD5693R,
>  	ID_AD5694,
>  	ID_AD5694R,
>  	ID_AD5695R,
> @@ -56,6 +63,11 @@ enum ad5686_supported_device_ids {
>  	ID_AD5696R,
>  };
>  
> +enum ad5686_regmap_type {
> +	AD5686_REGMAP,
> +	AD5693_REGMAP
> +};
> +
>  struct ad5686_state;
>  
>  typedef int (*ad5686_write_func)(struct ad5686_state *st,
> @@ -68,12 +80,14 @@ typedef int (*ad5686_read_func)(struct ad5686_state *st, u8 addr);
>   * @int_vref_mv:	AD5620/40/60: the internal reference voltage
>   * @num_channels:	number of channels
>   * @channel:		channel specification
> + * @regmap_type:	register map layout variant
>   */
>  
>  struct ad5686_chip_info {
>  	u16				int_vref_mv;
>  	unsigned int			num_channels;
>  	struct iio_chan_spec		*channels;
> +	enum ad5686_regmap_type		regmap_type;
>  };
>  
>  /**
> @@ -84,6 +98,7 @@ struct ad5686_chip_info {
>   * @vref_mv:		actual reference voltage used
>   * @pwr_down_mask:	power down mask
>   * @pwr_down_mode:	current power down mode
> + * @use_internal_vref:	set to true if the internal reference voltage is used
>   * @data:		spi transfer buffers
>   */
>  
> @@ -96,6 +111,7 @@ struct ad5686_state {
>  	unsigned int			pwr_down_mode;
>  	ad5686_write_func		write;
>  	ad5686_read_func		read;
> +	bool				use_internal_vref;
>  
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
> diff --git a/drivers/iio/dac/ad5696-i2c.c b/drivers/iio/dac/ad5696-i2c.c
> index 275e032..d18735d 100644
> --- a/drivers/iio/dac/ad5696-i2c.c
> +++ b/drivers/iio/dac/ad5696-i2c.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
> - * AD5671R, AD5675R, AD5694, AD5694R, AD5695R, AD5696, AD5696R
> + * AD5671R, AD5675R, AD5691R, AD5692R, AD5693, AD5693R,
> + * AD5694, AD5694R, AD5695R, AD5696, AD5696R
>   * Digital to analog converters driver
>   *
>   * Copyright 2018 Analog Devices Inc.
> @@ -72,6 +73,10 @@ static int ad5686_i2c_remove(struct i2c_client *i2c)
>  static const struct i2c_device_id ad5686_i2c_id[] = {
>  	{"ad5671r", ID_AD5671R},
>  	{"ad5675r", ID_AD5675R},
> +	{"ad5691r", ID_AD5691R},
> +	{"ad5692r", ID_AD5692R},
> +	{"ad5693", ID_AD5693},
> +	{"ad5693r", ID_AD5693R},
>  	{"ad5694", ID_AD5694},
>  	{"ad5694r", ID_AD5694R},
>  	{"ad5695r", ID_AD5695R},