Re: [PATCH v2] iio: dac: Add support for the AD5592R/AD5593R ADCs/DACs

Re: [PATCH v2] iio: dac: Add support for the AD5592R/AD5593R ADCs/DACs

[Jonathan Cameron]

On 25/02/16 13:36, michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org wrote:
> From: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> 
> This patch adds support for the AD5592R (spi) and AD5593R (i2c)
> ADC/DAC devices.
> 
> Signed-off-by: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>
A few bits inline.

I'll need a gpio review on this (looks fine to me but it does contain
a gpiochip driver.) Not to mention the question of whether they will
be happy with a gpio chip hiding in iio (rather than via an mfd with a
separate driver - which feels like overkill here).

The big question to my mind is whether we can take the view this won't
be the last multipurpose chip we will see so do we need to sort the
binding out to make it generic?  It'll be a bit of a pain for you
but I think we can do it fairly easily.
(either way I'll also need a device tree ack on this one!)

So then we get into the question of the best way of doing the bindings.
The gpio approach seems a little limiting for things as flexible as
this but we should certainly be using their macros where relevant.

J
> ---
> 
> Changes since v1:
> 	* Fix mutex usage
> 	* Remove unnecessary NULL pointer guards
> 	* Add comment explaining the invalid data read
> 	* AD5593R Remove surplus adc readback
> 
>  .../devicetree/bindings/iio/dac/ad5592r.txt        |  88 +++
>  drivers/iio/dac/Kconfig                            |  27 +
>  drivers/iio/dac/Makefile                           |   3 +
>  drivers/iio/dac/ad5592r-base.c                     | 675 +++++++++++++++++++++
>  drivers/iio/dac/ad5592r-base.h                     |  77 +++
>  drivers/iio/dac/ad5592r.c                          | 164 +++++
>  drivers/iio/dac/ad5593r.c                          | 131 ++++
>  include/dt-bindings/iio/adi,ad5592r.h              |  16 +
>  8 files changed, 1181 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/ad5592r.txt
>  create mode 100644 drivers/iio/dac/ad5592r-base.c
>  create mode 100644 drivers/iio/dac/ad5592r-base.h
>  create mode 100644 drivers/iio/dac/ad5592r.c
>  create mode 100644 drivers/iio/dac/ad5593r.c
>  create mode 100644 include/dt-bindings/iio/adi,ad5592r.h
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/ad5592r.txt b/Documentation/devicetree/bindings/iio/dac/ad5592r.txt
> new file mode 100644
> index 0000000..9d7f23a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/ad5592r.txt
> @@ -0,0 +1,88 @@
> +Analog Devices AD5592R/AD5593R DAC/ADC device driver
> +
> +Required properties for the AD5592R:
> +	- compatible: Must be "adi,ad5592r"
> +	- reg: SPI chip select number for the device
> +	- spi-max-frequency: Max SPI frequency to use (< 30000000)
> +	- spi-cpol: The AD5592R requires inverse clock polarity (CPOL) mode
> +
> +Required properties for the AD5593R:
> +	- compatible: Must be "adi,ad5593r"
> +	- reg: I2C address of the device
> +
> +Required properties for all supported chips:
> +	- channel-modes: An array of eight 8-bit values (one per channel)
> +	  describing the mode of each channel. Macros specifying the valid values
> +	  can be found in <dt-bindings/iio/adi,ad5592r.h>.
> +	  The following values are currently supported:
Lets think about making this truely generic.

So we have a basic mode questio first.  I think we separate that from the later
part. Also do we break this down into individual channels?  It think we probably
do want to like we have done for various highly adapatable adcs in the past.

so we'd have something like:
	channels {
		#address-cells = <1>;
		#size-cells = <0>;
		channel@0 {
			mode = CH_MODE_ADC,
		},
		channel@1 {
			mode = CH_MODE_DAC,
		},
		channel@2 {
			mode = CH_MODE_DAC_ADC
		},
		channel@3 {
			mode = CH_MODE_UNUSED,
			offstate = CH_OFFSTATE_PULLDOWN,
		},
		channel@4 {
			mode = CH_MODE_GPIO,
			//lift the opendrain stuff from the gpio bindings macros.	
		}
			
Or take something more similar to the gpio bindings perhaps?  bit fiddly here
as we can have a wide range of stuff and not all of it is always relevant.

Input from others on this most welcome!


		}compatible = "regulator-fixed";
> +		regulator-name = "vref-ad559x";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-always-on;
> +	};
> +
> +	ad5592r@0 {
> +		compatible = "adi,ad5592r";
> +		reg = <0>;
> +		spi-max-frequency = <1000000>;
> +		spi-cpol;
> +
> +		channel-modes = /bits/ 8 <
> +			CH_MODE_DAC
> +			CH_MODE_ADC
> +			CH_MODE_DAC_AND_ADC
> +			CH_MODE_DAC_AND_ADC
> +			CH_MODE_UNUSED_PULL_DOWN
> +			CH_MODE_GPIO
> +			CH_MODE_GPIO
> +			CH_MODE_GPIO
> +		>;
> +
> +		vref-supply = <&vref>; /* optional */
> +		reset-gpios = <&gpio0 86 0>;  /* optional */
> +	};
> +
> +AD5593R Example:
> +
> +	#include <dt-bindings/iio/adi,ad5592r.h>
> +
> +	ad5593r@10 {
> +		compatible = "adi,ad5593r";
> +		reg = <0x10>;
> +		channel-modes = /bits/ 8 <
> +			CH_MODE_DAC
> +			CH_MODE_ADC
> +			CH_MODE_DAC_AND_ADC
> +			CH_MODE_DAC_AND_ADC
> +			CH_MODE_UNUSED_PULL_DOWN
> +			CH_MODE_GPIO
> +			CH_MODE_GPIO
> +			CH_MODE_GPIO
> +		>;
> +
> +	};
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 31a1985..e7dd376 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -74,6 +74,33 @@ config AD5449
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad5449.
>  
> +config AD5592R_BASE
> +	tristate
> +
> +config AD5592R
> +	tristate "Analog Devices AD5592R ADC/DAC driver"
> +	depends on SPI_MASTER
> +	depends on OF
> +	select AD5592R_BASE
> +	help
> +	  Say yes here to build support for Analog Devices AD5592R
> +	  Digital to Analog / Analog to Digital Converter.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad5592r.
> +
> +config AD5593R
> +	tristate "Analog Devices AD5593R ADC/DAC driver"
> +	depends on I2C
> +	depends on OF
> +	select AD5592R_BASE
> +	help
> +	  Say yes here to build support for Analog Devices AD5593R
> +	  Digital to Analog / Analog to Digital Converter.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad5593r.
> +
>  config AD5504
>  	tristate "Analog Devices AD5504/AD5501 DAC SPI driver"
>  	depends on SPI
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index e2deda9..cf23310 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -11,6 +11,9 @@ obj-$(CONFIG_AD5064) += ad5064.o
>  obj-$(CONFIG_AD5504) += ad5504.o
>  obj-$(CONFIG_AD5446) += ad5446.o
>  obj-$(CONFIG_AD5449) += ad5449.o
> +obj-$(CONFIG_AD5592R_BASE) += ad5592r-base.o
> +obj-$(CONFIG_AD5592R) += ad5592r.o
> +obj-$(CONFIG_AD5593R) += ad5593r.o
>  obj-$(CONFIG_AD5755) += ad5755.o
>  obj-$(CONFIG_AD5761) += ad5761.o
>  obj-$(CONFIG_AD5764) += ad5764.o
> diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
> new file mode 100644
> index 0000000..6dd4eab
> --- /dev/null
> +++ b/drivers/iio/dac/ad5592r-base.c
> @@ -0,0 +1,675 @@
> +/*
> + * AD5592R Digital <-> Analog converters driver
> + *
> + * Copyright 2014-2016 Analog Devices Inc.
> + * Author: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio.h>
> +
> +#include <dt-bindings/iio/adi,ad5592r.h>
> +
> +#include "ad5592r-base.h"
> +
> +#ifdef CONFIG_GPIOLIB
> +
> +static int ad5592r_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct ad5592r_state *st = gpiochip_get_data(chip);
> +	int ret = 0;
> +	u8 val;
> +
> +	mutex_lock(&st->gpio_lock);
> +
> +	if (st->gpio_out & BIT(offset))
> +		val = st->gpio_val;
> +	else
> +		ret = st->ops->gpio_read(st, &val);
> +
> +	mutex_unlock(&st->gpio_lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return !!(val & BIT(offset));
> +}
> +
> +static void ad5592r_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> +	struct ad5592r_state *st = gpiochip_get_data(chip);
> +
> +	mutex_lock(&st->gpio_lock);
> +
> +	if (value)
> +		st->gpio_val |= BIT(offset);
> +	else
> +		st->gpio_val &= ~BIT(offset);
> +
> +	st->ops->reg_write(st, AD5592R_REG_GPIO_SET, st->gpio_val);
> +
> +	mutex_unlock(&st->gpio_lock);
> +}
> +
> +static int ad5592r_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct ad5592r_state *st = gpiochip_get_data(chip);
> +	int ret;
> +
> +	mutex_lock(&st->gpio_lock);
> +
> +	st->gpio_out &= ~BIT(offset);
> +	st->gpio_in |= BIT(offset);
> +
> +	ret = st->ops->reg_write(st, AD5592R_REG_GPIO_OUT_EN, st->gpio_out);
> +	if (ret < 0)
> +		goto err_unlock;
> +
> +	ret = st->ops->reg_write(st, AD5592R_REG_GPIO_IN_EN, st->gpio_in);
> +
> +err_unlock:
> +	mutex_unlock(&st->gpio_lock);
> +
> +	return ret;
> +}
> +
> +static int ad5592r_gpio_direction_output(struct gpio_chip *chip,
> +					 unsigned offset, int value)
> +{
> +	struct ad5592r_state *st = gpiochip_get_data(chip);
> +	int ret;
> +
> +	mutex_lock(&st->gpio_lock);
> +
> +	if (value)
> +		st->gpio_val |= BIT(offset);
> +	else
> +		st->gpio_val &= ~BIT(offset);
> +
> +	st->gpio_in &= ~BIT(offset);
> +	st->gpio_out |= BIT(offset);
> +
> +	ret = st->ops->reg_write(st, AD5592R_REG_GPIO_SET, st->gpio_val);
> +	if (ret < 0)
> +		goto err_unlock;
> +
> +	ret = st->ops->reg_write(st, AD5592R_REG_GPIO_OUT_EN, st->gpio_out);
> +	if (ret < 0)
> +		goto err_unlock;
> +
> +	ret = st->ops->reg_write(st, AD5592R_REG_GPIO_IN_EN, st->gpio_in);
> +
> +err_unlock:
> +	mutex_unlock(&st->gpio_lock);
> +
> +	return ret;
> +}
> +
> +static int ad5592r_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> +	struct ad5592r_state *st = gpiochip_get_data(chip);
> +
> +	if (!(st->gpio_map & BIT(offset))) {
> +		dev_err(st->dev, "GPIO %d is reserved by alternate function\n",
> +			offset);
> +		return -ENODEV;
> +	}
> +
> +	if (offset >= chip->ngpio)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ad5592r_gpio_init(struct ad5592r_state *st)
> +{
> +	st->gpiochip.label = dev_name(st->dev);
> +	st->gpiochip.base = -1;
> +	st->gpiochip.ngpio = 8;
> +	st->gpiochip.parent = st->dev;
> +	st->gpiochip.can_sleep = true;
> +	st->gpiochip.direction_input = ad5592r_gpio_direction_input;
> +	st->gpiochip.direction_output = ad5592r_gpio_direction_output;
> +	st->gpiochip.get = ad5592r_gpio_get;
> +	st->gpiochip.set = ad5592r_gpio_set;
> +	st->gpiochip.request = ad5592r_gpio_request;
> +	st->gpiochip.owner = THIS_MODULE;
> +
> +	mutex_init(&st->gpio_lock);
> +
> +	return gpiochip_add_data(&st->gpiochip, st);
> +}
> +
> +static void ad5592r_gpio_cleanup(struct ad5592r_state *st)
> +{
> +	gpiochip_remove(&st->gpiochip);
> +}
> +#else
> +static int ad5592r_gpio_init(struct ad5592r_state *st) { return 0 };
> +static void ad5592r_gpio_cleanup(struct ad5592r_state *st) { };
> +#endif /* CONFIG_GPIOLIB */
> +
> +static int ad5592r_reset(struct ad5592r_state *st)
> +{
> +	struct gpio_desc *gpio;
> +	struct iio_dev *iio_dev = iio_priv_to_dev(st);
> +
> +	gpio = devm_gpiod_get_optional(st->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpio))
> +		return PTR_ERR(gpio);
> +
> +	if (gpio) {
> +		udelay(1);
> +		gpiod_set_value(gpio, 1);
> +	} else {
> +		mutex_lock(&iio_dev->mlock);
> +		st->ops->reg_write(st, AD5592R_REG_RESET, 0xdac);
> +		mutex_unlock(&iio_dev->mlock);
> +	}
> +
> +	udelay(250);
> +
> +	return 0;
> +}
> +
> +static int ad5592r_get_vref(struct ad5592r_state *st)
> +{
> +	int ret;
> +
> +	if (st->reg) {
> +		ret = regulator_get_voltage(st->reg);
> +		if (ret < 0)
> +			return ret;
> +
> +		return ret / 1000;
> +	} else {
> +		return 2500;
> +	}
> +}
> +
> +static int ad5592r_set_channel_modes(struct ad5592r_state *st)
> +{
> +	const struct ad5592r_rw_ops *ops = st->ops;
> +	int ret;
> +	unsigned i;
> +	struct iio_dev *iio_dev = iio_priv_to_dev(st);
> +	u8 pulldown = 0, open_drain = 0, tristate = 0,
> +	   dac = 0, adc = 0;
> +	u16 read_back;
> +
> +	for (i = 0; i < st->num_channels; i++) {
> +		switch (st->channel_modes[i]) {
> +		case CH_MODE_DAC:
> +			dac |= BIT(i);
> +			break;
> +
> +		case CH_MODE_ADC:
> +			adc |= BIT(i);
> +			break;
> +
> +		case CH_MODE_DAC_AND_ADC:
> +			dac |= BIT(i);
> +			adc |= BIT(i);
> +			break;
> +
> +		case CH_MODE_UNUSED_PULL_DOWN:
> +			pulldown |= BIT(i);
> +			break;
> +
> +		case CH_MODE_UNUSED_OUT_TRISTATE:
> +			tristate |= BIT(i);
> +			break;
> +
> +		case CH_MODE_UNUSED_OUT_LOW:
> +			st->gpio_out |= BIT(i);
> +			break;
> +
> +		case CH_MODE_UNUSED_OUT_HIGH:
> +			st->gpio_out |= BIT(i);
> +			st->gpio_val |= BIT(i);
> +			break;
> +
> +		case CH_MODE_GPIO_OPEN_DRAIN:
> +			open_drain |= BIT(i);
> +
> +			/* fall-through */
> +
> +		case CH_MODE_GPIO:
> +			st->gpio_map |= BIT(i);
> +			st->gpio_in |= BIT(i); /* Default to input */
> +			break;
> +
> +		default:
> +			pulldown |= BIT(i);
> +			break;
> +		}
> +	}
> +
> +	mutex_lock(&iio_dev->mlock);
> +
> +	/* Pull down unused pins to GND */
> +	ret = ops->reg_write(st, AD5592R_REG_PULLDOWN, pulldown);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_TRISTATE, tristate);
> +	if (ret)
> +		goto err_unlock;
> +
> +	/* Configure pins that we use */
> +	ret = ops->reg_write(st, AD5592R_REG_DAC_EN, dac);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_ADC_EN, adc);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_OPEN_DRAIN, open_drain);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_GPIO_SET, st->gpio_val);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_GPIO_OUT_EN, st->gpio_out);
> +	if (ret)
> +		goto err_unlock;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_GPIO_IN_EN, st->gpio_in);
> +	if (ret)
> +		goto err_unlock;
> +
> +	/* Verify that we can read back at least one register */
> +	ret = ops->reg_read(st, AD5592R_REG_ADC_EN, &read_back);
> +	if (!ret && (read_back & 0xff) != adc)
> +		ret = -EIO;
> +
> +err_unlock:
> +	mutex_unlock(&iio_dev->mlock);
> +	return ret;
> +}
> +
> +static int ad5592r_write_raw(struct iio_dev *iio_dev,
> +	struct iio_chan_spec const *chan, int val, int val2, long mask)
> +{
> +	struct ad5592r_state *st = iio_priv(iio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val >= (1 << chan->scan_type.realbits) || val < 0)
> +			return -EINVAL;
> +
> +		/* Warn if we try to write to a ADC channel */
> +		WARN_ON(!chan->output);
Probably just wants to return an error rather than filling the logs.
People do silly things all time like that.
> +
> +		mutex_lock(&iio_dev->mlock);
> +		ret = st->ops->write_dac(st, chan->channel, val);
> +		if (!ret)
> +			st->cached_dac[chan->channel] = val;
> +		mutex_unlock(&iio_dev->mlock);
> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_VOLTAGE) {
> +			bool gain;
> +
> +			if (val == st->scale_avail[0][0] &&
> +				val2 == st->scale_avail[0][1])
> +				gain = false;
> +			else if (val == st->scale_avail[1][0] &&
> +				 val2 == st->scale_avail[1][1])
> +				gain = true;
> +			else
> +				return -EINVAL;
> +
> +			mutex_lock(&iio_dev->mlock);
> +
> +			ret = st->ops->reg_read(st, AD5592R_REG_CTRL,
> +						&st->cached_gp_ctrl);
> +			if (ret < 0) {
> +				mutex_unlock(&iio_dev->mlock);
> +				return ret;
> +			}
> +
> +			if (chan->output) {
> +				if (gain)
> +					st->cached_gp_ctrl |=
> +						AD5592R_REG_CTRL_DAC_RANGE;
> +				else
> +					st->cached_gp_ctrl &=
> +						~AD5592R_REG_CTRL_DAC_RANGE;
> +			} else {
> +				if (gain)
> +					st->cached_gp_ctrl |=
> +						AD5592R_REG_CTRL_ADC_RANGE;
> +				else
> +					st->cached_gp_ctrl &=
> +						~AD5592R_REG_CTRL_ADC_RANGE;
> +			}
> +
> +			ret = st->ops->reg_write(st, AD5592R_REG_CTRL,
> +						 st->cached_gp_ctrl);
> +			mutex_unlock(&iio_dev->mlock);
> +			if (ret < 0)
> +				return ret;
> +
> +			return ret;
> +
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad5592r_read_raw(struct iio_dev *iio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long m)
> +{
> +	struct ad5592r_state *st = iio_priv(iio_dev);
> +	u16 read_val;
> +	int ret;
> +
> +	mutex_lock(&iio_dev->mlock);
Move the lock into the switch statement as it doesn't
need to be held for a fair bit of the code below (such as the scale read
backs).
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +
> +		if (!chan->output) {
> +			ret = st->ops->read_adc(st, chan->channel, &read_val);
> +			if (ret)
> +				goto unlock;
> +
> +			if ((read_val >> 12 & 0x7) != (chan->channel & 0x7)) {
> +				dev_err(st->dev, "Error while reading channel %u\n",
> +						chan->channel);
> +				ret = -EIO;
> +				goto unlock;
> +			}
> +
> +			read_val &= GENMASK(11, 0);
> +
> +		} else {
> +			read_val = st->cached_dac[chan->channel];
> +		}
> +
> +		dev_dbg(st->dev, "Channel %u read: 0x%04hX\n",
> +				chan->channel, read_val);
> +
> +		*val = (int) read_val;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +
> +		*val = ad5592r_get_vref(st);
> +
> +		if (chan->type == IIO_TEMP) {
> +			s64 tmp = *val * (3767897513LL / 25LL);
> +			*val = div_s64_rem(tmp, 1000000000LL, val2);
> +
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +		} else {
> +			int mult;
> +
> +			if (chan->output)
> +				mult = !!(st->cached_gp_ctrl &
> +					AD5592R_REG_CTRL_DAC_RANGE);
> +			else
> +				mult = !!(st->cached_gp_ctrl &
> +					AD5592R_REG_CTRL_ADC_RANGE);
> +
> +			*val *= ++mult;
> +
> +			*val2 = chan->scan_type.realbits;
> +			ret = IIO_VAL_FRACTIONAL_LOG2;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_OFFSET:
> +
> +		ret = ad5592r_get_vref(st);
> +
> +		if (st->cached_gp_ctrl & AD5592R_REG_CTRL_ADC_RANGE)
> +			*val = (-34365 * 25) / ret;
> +		else
> +			*val = (-75365 * 25) / ret;
> +		ret =  IIO_VAL_INT;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +unlock:
> +	mutex_unlock(&iio_dev->mlock);
> +	return ret;
> +}
> +
> +static int ad5592r_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return IIO_VAL_INT_PLUS_NANO;
> +
> +	default:
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info ad5592r_info = {
> +	.read_raw = ad5592r_read_raw,
> +	.write_raw = ad5592r_write_raw,
> +	.write_raw_get_fmt = ad5592r_write_raw_get_fmt,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static ssize_t ad5592r_show_scale_available(struct iio_dev *iio_dev,
> +					   uintptr_t private,
> +					   const struct iio_chan_spec *chan,
> +					   char *buf)
> +{
> +	struct ad5592r_state *st = iio_priv(iio_dev);
> +
> +	return sprintf(buf, "%d.%09u %d.%09u\n",
> +		st->scale_avail[0][0], st->scale_avail[0][1],
> +		st->scale_avail[1][0], st->scale_avail[1][1]);
> +}
> +
> +static struct iio_chan_spec_ext_info ad5592r_ext_info[] = {
> +	{
> +	 .name = "scale_available",
> +	 .read = ad5592r_show_scale_available,
> +	 .shared = true,
> +	 },
> +	{},
> +};
> +
> +static void ad5592r_setup_channel(struct iio_dev *iio_dev,
> +		struct iio_chan_spec *chan, bool output, unsigned id)
> +{
> +	chan->type = IIO_VOLTAGE;
> +	chan->indexed = 1;
> +	chan->output = output;
> +	chan->channel = id;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> +	chan->scan_type.sign = 'u';
> +	chan->scan_type.realbits = 12;
> +	chan->scan_type.storagebits = 16;
> +	chan->ext_info = ad5592r_ext_info;
> +}
> +
> +static int ad5592r_alloc_channels(struct ad5592r_state *st)
> +{
> +	unsigned i, curr_channel = 0,
> +		 num_channels = st->num_channels;
> +	struct iio_dev *iio_dev = iio_priv_to_dev(st);
> +	struct iio_chan_spec *channels;
> +	int ret;
> +
> +	ret = device_property_read_u8_array(st->dev, "channel-modes",
> +			st->channel_modes, num_channels);
> +	if (ret)
> +		return ret;
> +
> +	channels = devm_kzalloc(st->dev,
> +			(1 + 2 * num_channels) * sizeof(*channels), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_channels; i++) {
> +		switch (st->channel_modes[i]) {
> +		case CH_MODE_DAC:
> +			ad5592r_setup_channel(iio_dev, &channels[curr_channel],
> +					true, i);
> +			curr_channel++;
> +			break;
> +
> +		case CH_MODE_ADC:
> +			ad5592r_setup_channel(iio_dev, &channels[curr_channel],
> +					false, i);
> +			curr_channel++;
> +			break;
> +
> +		case CH_MODE_DAC_AND_ADC:
> +			ad5592r_setup_channel(iio_dev, &channels[curr_channel],
> +					true, i);
> +			curr_channel++;
> +			ad5592r_setup_channel(iio_dev, &channels[curr_channel],
> +					false, i);
> +			curr_channel++;
> +			break;
> +
> +		default:
> +			continue;
> +		}
> +	}
> +
> +	channels[curr_channel].type = IIO_TEMP;
> +	channels[curr_channel].channel = 8;
> +	channels[curr_channel].info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				   BIT(IIO_CHAN_INFO_SCALE) |
> +				   BIT(IIO_CHAN_INFO_OFFSET);
> +	curr_channel++;
> +
> +	iio_dev->num_channels = curr_channel;
> +	iio_dev->channels = channels;
> +
> +	return 0;
> +}
> +
> +static void ad5592r_init_scales(struct ad5592r_state *st, int vref_mV)
> +{
> +	s64 tmp = (s64)vref_mV * 1000000000LL >> 12;
> +
> +	st->scale_avail[0][0] =
> +		div_s64_rem(tmp, 1000000000LL, &st->scale_avail[0][1]);
> +	st->scale_avail[1][0] =
> +		div_s64_rem(tmp * 2, 1000000000LL, &st->scale_avail[1][1]);
> +}
> +
> +int ad5592r_probe(struct device *dev, const char *name,
> +		const struct ad5592r_rw_ops *ops)
> +{
> +	struct iio_dev *iio_dev;
> +	struct ad5592r_state *st;
> +	int ret;
> +
> +	iio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!iio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(iio_dev);
> +	st->dev = dev;
> +	st->ops = ops;
> +	st->num_channels = 8;
> +	dev_set_drvdata(dev, iio_dev);
> +
> +	st->reg = devm_regulator_get_optional(dev, "vref");
> +	if (IS_ERR(st->reg)) {
> +		if ((PTR_ERR(st->reg) != -ENODEV) && dev->of_node)
> +			return PTR_ERR(st->reg);
> +
> +		st->reg = NULL;
> +	} else {
> +		ret = regulator_enable(st->reg);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	iio_dev->dev.parent = dev;
> +	iio_dev->name = name;
> +	iio_dev->info = &ad5592r_info;
> +	iio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ad5592r_init_scales(st, ad5592r_get_vref(st));
> +
> +	ret = ad5592r_reset(st);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = ops->reg_write(st, AD5592R_REG_PD,
> +		     (st->reg == NULL) ? AD5592R_REG_PD_EN_REF : 0);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = ad5592r_alloc_channels(st);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = ad5592r_set_channel_modes(st);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = devm_iio_device_register(dev, iio_dev);
> +	if (ret)
> +		goto error_disable_reg;
If you use a managed iio_device_register here it will only
be unregistered after the end of the remove function.  It certainly looks
like it really wants to be done at the start of the remove function
so as to remove the exposed interfaces (user and in kernel) before changing
the channel modes etc.

Hence use the unmanaged version and iio_device_unregister in the remove.
> +
> +	return ad5592r_gpio_init(st);
> +
> +error_disable_reg:
> +	if (st->reg)
> +		regulator_disable(st->reg);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(ad5592r_probe);
> +
> +int ad5592r_remove(struct device *dev)
> +{
> +	struct iio_dev *iio_dev = dev_get_drvdata(dev);
> +	struct ad5592r_state *st = iio_priv(iio_dev);
> +	unsigned int i;
> +
> +	/* Reset all channels */
> +	for (i = 0; i < ARRAY_SIZE(st->channel_modes); i++)
> +		st->channel_modes[i] = CH_MODE_UNUSED_PULL_DOWN;
> +
> +	if (st->reg)
> +		regulator_disable(st->reg);
> +
> +	if (st->gpio_map)
> +		ad5592r_gpio_cleanup(st);
> +
> +	return ad5592r_set_channel_modes(st);
> +}
> +EXPORT_SYMBOL_GPL(ad5592r_remove);
> +
> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/dac/ad5592r-base.h b/drivers/iio/dac/ad5592r-base.h
> new file mode 100644
> index 0000000..162e833
> --- /dev/null
> +++ b/drivers/iio/dac/ad5592r-base.h
> @@ -0,0 +1,77 @@
> +/*
> + * AD5592R / AD5593R Digital <-> Analog converters driver
> + *
> + * Copyright 2015-2016 Analog Devices Inc.
> + * Author: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#ifndef __DRIVERS_IIO_DAC_AD5592R_BASE_H__
> +#define __DRIVERS_IIO_DAC_AD5592R_BASE_H__
> +
> +#include <linux/types.h>
> +#include <linux/cache.h>
> +#include <linux/mutex.h>
> +#include <linux/gpio/driver.h>
> +
> +struct device;
> +struct ad5592r_state;
> +
> +enum ad5592r_registers {
> +	AD5592R_REG_NOOP		= 0x0,
> +	AD5592R_REG_DAC_READBACK	= 0x1,
> +	AD5592R_REG_ADC_SEQ		= 0x2,
> +	AD5592R_REG_CTRL		= 0x3,
> +	AD5592R_REG_ADC_EN		= 0x4,
> +	AD5592R_REG_DAC_EN		= 0x5,
> +	AD5592R_REG_PULLDOWN		= 0x6,
> +	AD5592R_REG_LDAC		= 0x7,
> +	AD5592R_REG_GPIO_OUT_EN		= 0x8,
> +	AD5592R_REG_GPIO_SET		= 0x9,
> +	AD5592R_REG_GPIO_IN_EN		= 0xA,
> +	AD5592R_REG_PD			= 0xB,
> +	AD5592R_REG_OPEN_DRAIN		= 0xC,
> +	AD5592R_REG_TRISTATE		= 0xD,
> +	AD5592R_REG_RESET		= 0xF,
> +};
> +
> +#define AD5592R_REG_PD_EN_REF		BIT(9)
> +#define AD5592R_REG_CTRL_ADC_RANGE	BIT(5)
> +#define AD5592R_REG_CTRL_DAC_RANGE	BIT(4)
> +
> +struct ad5592r_rw_ops {
> +	int (*write_dac)(struct ad5592r_state *st, unsigned chan, u16 value);
> +	int (*read_adc)(struct ad5592r_state *st, unsigned chan, u16 *value);
> +	int (*reg_write)(struct ad5592r_state *st, u8 reg, u16 value);
> +	int (*reg_read)(struct ad5592r_state *st, u8 reg, u16 *value);
> +	int (*gpio_read)(struct ad5592r_state *st, u8 *value);
> +};
> +
> +struct ad5592r_state {
> +	struct device *dev;
> +	struct regulator *reg;
> +#ifdef CONFIG_GPIOLIB
> +	struct gpio_chip gpiochip;
> +	struct mutex gpio_lock;	/* Protect cached gpio_out, gpio_val, etc. */
> +#endif
> +	unsigned int num_channels;
> +	const struct ad5592r_rw_ops *ops;
> +	int scale_avail[2][2];
> +	u16 cached_dac[8];
> +	u16 cached_gp_ctrl;
> +	u8 channel_modes[8];
> +	u8 gpio_map;
> +	u8 gpio_out;
> +	u8 gpio_in;
> +	u8 gpio_val;
> +
> +	__be16 spi_msg ____cacheline_aligned;
> +	__be16 spi_msg_nop;
> +};
> +
> +int ad5592r_probe(struct device *dev, const char *name,
> +		const struct ad5592r_rw_ops *ops);
> +int ad5592r_remove(struct device *dev);
> +
> +#endif /* __DRIVERS_IIO_DAC_AD5592R_BASE_H__ */
> diff --git a/drivers/iio/dac/ad5592r.c b/drivers/iio/dac/ad5592r.c
> new file mode 100644
> index 0000000..0b235a2
> --- /dev/null
> +++ b/drivers/iio/dac/ad5592r.c
> @@ -0,0 +1,164 @@
> +/*
> + * AD5592R Digital <-> Analog converters driver
> + *
> + * Copyright 2015-2016 Analog Devices Inc.
> + * Author: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include "ad5592r-base.h"
> +
> +#include <linux/bitops.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
> +
> +#define AD5592R_GPIO_READBACK_EN	BIT(10)
> +#define AD5592R_LDAC_READBACK_EN	BIT(6)
> +
> +static int ad5592r_spi_wnop_r16(struct ad5592r_state *st, u16 *buf)
> +{
> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
> +	struct spi_transfer t = {
> +			.tx_buf	= &st->spi_msg_nop,
> +			.rx_buf	= buf,
> +			.len = 2
> +		};
> +
> +	st->spi_msg_nop = 0; /* NOP */
> +
> +	return spi_sync_transfer(spi, &t, 1);
> +}
> +
> +static int ad5592r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value)
> +{
> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
> +
> +	st->spi_msg = cpu_to_be16(BIT(15) | (chan << 12) | value);
> +
> +	return spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
> +}
> +
> +static int ad5592r_read_adc(struct ad5592r_state *st, unsigned chan, u16 *value)
> +{
> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
> +	int ret;
> +
> +	st->spi_msg = cpu_to_be16((AD5592R_REG_ADC_SEQ << 11) | BIT(chan));
> +
> +	ret = spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Invalid data:
> +	 * See Figure 40. Single-Channel ADC Conversion Sequence
> +	 */
> +	ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
> +	if (ret)
> +		return ret;
> +
> +	*value = be16_to_cpu(st->spi_msg);
> +
> +	return 0;
> +}
> +
> +static int ad5592r_reg_write(struct ad5592r_state *st, u8 reg, u16 value)
> +{
> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
> +
> +	st->spi_msg = cpu_to_be16((reg << 11) | value);
> +
> +	return spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
> +}
> +
> +static int ad5592r_reg_read(struct ad5592r_state *st, u8 reg, u16 *value)
> +{
> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
> +	int ret;
> +
> +	st->spi_msg = cpu_to_be16((AD5592R_REG_LDAC << 11) |
> +				   AD5592R_LDAC_READBACK_EN | (reg << 2));
> +
> +	ret = spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
> +	if (ret)
> +		return ret;
> +
> +	ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
> +	if (ret)
> +		return ret;
> +
> +	*value = be16_to_cpu(st->spi_msg);
> +
> +	return 0;
> +}
> +
> +static int ad5593r_gpio_read(struct ad5592r_state *st, u8 *value)
> +{
> +	int ret;
> +
> +	ret = ad5592r_reg_write(st, AD5592R_REG_GPIO_IN_EN,
> +				AD5592R_GPIO_READBACK_EN | st->gpio_in);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
> +	if (ret)
> +		return ret;
> +
> +	*value = (u8) be16_to_cpu(st->spi_msg);
> +
> +	return 0;
> +}
> +
> +static const struct ad5592r_rw_ops ad5592r_rw_ops = {
> +	.write_dac = ad5592r_write_dac,
> +	.read_adc = ad5592r_read_adc,
> +	.reg_write = ad5592r_reg_write,
> +	.reg_read = ad5592r_reg_read,
> +	.gpio_read = ad5593r_gpio_read,
> +};
> +
> +static int ad5592r_spi_probe(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +
> +	return ad5592r_probe(&spi->dev, id->name, &ad5592r_rw_ops);
> +}
> +
> +static int ad5592r_spi_remove(struct spi_device *spi)
> +{
> +	return ad5592r_remove(&spi->dev);
> +}
> +
> +static const struct spi_device_id ad5592r_spi_ids[] = {
> +	{ .name = "ad5592r", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, ad5592r_spi_ids);
> +
> +static const struct of_device_id ad5592r_of_match[] = {
> +	{ .compatible = "adi,ad5592r", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ad5592r_of_match);
> +
> +static struct spi_driver ad5592r_spi_driver = {
> +	.driver = {
> +		.name = "ad5592r",
> +		.of_match_table = of_match_ptr(ad5592r_of_match),
> +	},
> +	.probe = ad5592r_spi_probe,
> +	.remove = ad5592r_spi_remove,
> +	.id_table = ad5592r_spi_ids,
> +};
> +module_spi_driver(ad5592r_spi_driver);
> +
> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/dac/ad5593r.c b/drivers/iio/dac/ad5593r.c
> new file mode 100644
> index 0000000..dca158a
> --- /dev/null
> +++ b/drivers/iio/dac/ad5593r.c
> @@ -0,0 +1,131 @@
> +/*
> + * AD5593R Digital <-> Analog converters driver
> + *
> + * Copyright 2015-2016 Analog Devices Inc.
> + * Author: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include "ad5592r-base.h"
> +
> +#include <linux/bitops.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#define AD5593R_MODE_CONF		(0 << 4)
> +#define AD5593R_MODE_DAC_WRITE		(1 << 4)
> +#define AD5593R_MODE_ADC_READBACK	(4 << 4)
> +#define AD5593R_MODE_DAC_READBACK	(5 << 4)
> +#define AD5593R_MODE_GPIO_READBACK	(6 << 4)
> +#define AD5593R_MODE_REG_READBACK	(7 << 4)
> +
> +static int ad5593r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value)
> +{
> +	struct i2c_client *i2c = to_i2c_client(st->dev);
> +
> +	return i2c_smbus_write_word_swapped(i2c,
> +			AD5593R_MODE_DAC_WRITE | chan, value);
> +}
> +
> +static int ad5593r_read_adc(struct ad5592r_state *st, unsigned chan, u16 *value)
> +{
> +	struct i2c_client *i2c = to_i2c_client(st->dev);
> +	s32 val;
> +
> +	val = i2c_smbus_write_word_swapped(i2c,
> +			AD5593R_MODE_CONF | AD5592R_REG_ADC_SEQ, BIT(chan));
> +	if (val < 0)
> +		return (int) val;
> +
> +	val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_ADC_READBACK);
> +	if (val < 0)
> +		return (int) val;
> +
> +	*value = (u16) val;
> +
> +	return 0;
> +}
> +
> +static int ad5593r_reg_write(struct ad5592r_state *st, u8 reg, u16 value)
> +{
> +	struct i2c_client *i2c = to_i2c_client(st->dev);
> +
> +	return i2c_smbus_write_word_swapped(i2c,
> +			AD5593R_MODE_CONF | reg, value);
> +}
> +
> +static int ad5593r_reg_read(struct ad5592r_state *st, u8 reg, u16 *value)
> +{
> +	struct i2c_client *i2c = to_i2c_client(st->dev);
> +	s32 val;
> +
> +	val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_REG_READBACK | reg);
> +	if (val < 0)
> +		return (int) val;
> +
> +	*value = (u16) val;
> +
> +	return 0;
> +}
> +
> +static int ad5593r_gpio_read(struct ad5592r_state *st, u8 *value)
> +{
> +	struct i2c_client *i2c = to_i2c_client(st->dev);
> +	s32 val;
> +
> +	val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_GPIO_READBACK);
> +	if (val < 0)
> +		return (int) val;
> +
> +	*value = (u8) val;
> +
> +	return 0;
> +}
> +
> +static const struct ad5592r_rw_ops ad5593r_rw_ops = {
> +	.write_dac = ad5593r_write_dac,
> +	.read_adc = ad5593r_read_adc,
> +	.reg_write = ad5593r_reg_write,
> +	.reg_read = ad5593r_reg_read,
> +	.gpio_read = ad5593r_gpio_read,
> +};
> +
> +static int ad5593r_i2c_probe(struct i2c_client *i2c,
> +		const struct i2c_device_id *id)
> +{
> +	return ad5592r_probe(&i2c->dev, id->name, &ad5593r_rw_ops);
> +}
> +
> +static int ad5593r_i2c_remove(struct i2c_client *i2c)
> +{
> +	return ad5592r_remove(&i2c->dev);
> +}
> +
> +static const struct i2c_device_id ad5593r_i2c_ids[] = {
> +	{ .name = "ad5593r", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, ad5593r_i2c_ids);
> +
> +static const struct of_device_id ad5593r_of_match[] = {
> +	{ .compatible = "adi,ad5593r", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ad5593r_of_match);
> +
> +static struct i2c_driver ad5593r_driver = {
> +	.driver = {
> +		.name = "ad5593r",
> +		.of_match_table = of_match_ptr(ad5593r_of_match),
> +	},
> +	.probe = ad5593r_i2c_probe,
> +	.remove = ad5593r_i2c_remove,
> +	.id_table = ad5593r_i2c_ids,
> +};
> +module_i2c_driver(ad5593r_driver);
> +
> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/dt-bindings/iio/adi,ad5592r.h b/include/dt-bindings/iio/adi,ad5592r.h
> new file mode 100644
> index 0000000..6bd519b
> --- /dev/null
> +++ b/include/dt-bindings/iio/adi,ad5592r.h
> @@ -0,0 +1,16 @@
> +
> +#ifndef _DT_BINDINGS_ADI_AD5592R_H
> +#define _DT_BINDINGS_ADI_AD5592R_H
> +
> +
> +#define CH_MODE_ADC			1
> +#define CH_MODE_DAC			2
> +#define CH_MODE_DAC_AND_ADC		3
> +#define CH_MODE_UNUSED_PULL_DOWN	4
> +#define CH_MODE_UNUSED_OUT_LOW		5
> +#define CH_MODE_UNUSED_OUT_HIGH		6
> +#define CH_MODE_UNUSED_OUT_TRISTATE	7
> +#define CH_MODE_GPIO			8
> +#define CH_MODE_GPIO_OPEN_DRAIN		9
> +
> +#endif /* _DT_BINDINGS_ADI_AD5592R_H */
> 

Re: [PATCH v2] iio: dac: Add support for the AD5592R/AD5593R ADCs/DACs

[Michael Hennerich]

On 02/27/2016 06:50 PM, Jonathan Cameron wrote:
> On 25/02/16 13:36, michael.hennerich@analog.com wrote:
>> From: Paul Cercueil <paul.cercueil@analog.com>
>>
>> This patch adds support for the AD5592R (spi) and AD5593R (i2c)
>> ADC/DAC devices.
>>
>> Signed-off-by: Paul Cercueil <paul.cercueil@analog.com>
>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>
> A few bits inline.
>
> I'll need a gpio review on this (looks fine to me but it does contain
> a gpiochip driver.) Not to mention the question of whether they will
> be happy with a gpio chip hiding in iio (rather than via an mfd with a
> separate driver - which feels like overkill here).
>
> The big question to my mind is whether we can take the view this won't
> be the last multipurpose chip we will see so do we need to sort the
> binding out to make it generic?  It'll be a bit of a pain for you
> but I think we can do it fairly easily.
> (either way I'll also need a device tree ack on this one!)
>
> So then we get into the question of the best way of doing the bindings.
> The gpio approach seems a little limiting for things as flexible as
> this but we should certainly be using their macros where relevant.

Hi Jonathan,

Thanks for the review.

The problem is see is that using GPIOF_OPEN_DRAIN, will simulate OPEN 
DRAIN behaviour only, by configuring the device for input when outputing 
logic high.

In this case the chip does it.

See comments below.


>
> J
>> ---
>>
>> Changes since v1:
>> 	* Fix mutex usage
>> 	* Remove unnecessary NULL pointer guards
>> 	* Add comment explaining the invalid data read
>> 	* AD5593R Remove surplus adc readback
>>
>>   .../devicetree/bindings/iio/dac/ad5592r.txt        |  88 +++
>>   drivers/iio/dac/Kconfig                            |  27 +
>>   drivers/iio/dac/Makefile                           |   3 +
>>   drivers/iio/dac/ad5592r-base.c                     | 675 +++++++++++++++++++++
>>   drivers/iio/dac/ad5592r-base.h                     |  77 +++
>>   drivers/iio/dac/ad5592r.c                          | 164 +++++
>>   drivers/iio/dac/ad5593r.c                          | 131 ++++
>>   include/dt-bindings/iio/adi,ad5592r.h              |  16 +
>>   8 files changed, 1181 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/dac/ad5592r.txt
>>   create mode 100644 drivers/iio/dac/ad5592r-base.c
>>   create mode 100644 drivers/iio/dac/ad5592r-base.h
>>   create mode 100644 drivers/iio/dac/ad5592r.c
>>   create mode 100644 drivers/iio/dac/ad5593r.c
>>   create mode 100644 include/dt-bindings/iio/adi,ad5592r.h
>>
>> diff --git a/Documentation/devicetree/bindings/iio/dac/ad5592r.txt b/Documentation/devicetree/bindings/iio/dac/ad5592r.txt
>> new file mode 100644
>> index 0000000..9d7f23a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/dac/ad5592r.txt
>> @@ -0,0 +1,88 @@
>> +Analog Devices AD5592R/AD5593R DAC/ADC device driver
>> +
>> +Required properties for the AD5592R:
>> +	- compatible: Must be "adi,ad5592r"
>> +	- reg: SPI chip select number for the device
>> +	- spi-max-frequency: Max SPI frequency to use (< 30000000)
>> +	- spi-cpol: The AD5592R requires inverse clock polarity (CPOL) mode
>> +
>> +Required properties for the AD5593R:
>> +	- compatible: Must be "adi,ad5593r"
>> +	- reg: I2C address of the device
>> +
>> +Required properties for all supported chips:
>> +	- channel-modes: An array of eight 8-bit values (one per channel)
>> +	  describing the mode of each channel. Macros specifying the valid values
>> +	  can be found in <dt-bindings/iio/adi,ad5592r.h>.
>> +	  The following values are currently supported:
> Lets think about making this truely generic.
>
> So we have a basic mode questio first.  I think we separate that from the later
> part. Also do we break this down into individual channels?  It think we probably
> do want to like we have done for various highly adapatable adcs in the past.
>
> so we'd have something like:
> 	channels {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		channel@0 {
> 			mode = CH_MODE_ADC,
> 		},
> 		channel@1 {
> 			mode = CH_MODE_DAC,
> 		},
> 		channel@2 {
> 			mode = CH_MODE_DAC_ADC
> 		},
> 		channel@3 {
> 			mode = CH_MODE_UNUSED,
> 			offstate = CH_OFFSTATE_PULLDOWN,
> 		},
> 		channel@4 {
> 			mode = CH_MODE_GPIO,
> 			//lift the opendrain stuff from the gpio bindings macros.	
> 		}
> 			
> Or take something more similar to the gpio bindings perhaps?  bit fiddly here
> as we can have a wide range of stuff and not all of it is always relevant.
>
> Input from others on this most welcome!

Let's go for this.


>
>
> 		}compatible = "regulator-fixed";
>> +		regulator-name = "vref-ad559x";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		regulator-always-on;
>> +	};
>> +
>> +	ad5592r@0 {
>> +		compatible = "adi,ad5592r";
>> +		reg = <0>;
>> +		spi-max-frequency = <1000000>;
>> +		spi-cpol;
>> +
>> +		channel-modes = /bits/ 8 <
>> +			CH_MODE_DAC
>> +			CH_MODE_ADC
>> +			CH_MODE_DAC_AND_ADC
>> +			CH_MODE_DAC_AND_ADC
>> +			CH_MODE_UNUSED_PULL_DOWN
>> +			CH_MODE_GPIO
>> +			CH_MODE_GPIO
>> +			CH_MODE_GPIO
>> +		>;
>> +
>> +		vref-supply = <&vref>; /* optional */
>> +		reset-gpios = <&gpio0 86 0>;  /* optional */
>> +	};
>> +
>> +AD5593R Example:
>> +
>> +	#include <dt-bindings/iio/adi,ad5592r.h>
>> +
>> +	ad5593r@10 {
>> +		compatible = "adi,ad5593r";
>> +		reg = <0x10>;
>> +		channel-modes = /bits/ 8 <
>> +			CH_MODE_DAC
>> +			CH_MODE_ADC
>> +			CH_MODE_DAC_AND_ADC
>> +			CH_MODE_DAC_AND_ADC
>> +			CH_MODE_UNUSED_PULL_DOWN
>> +			CH_MODE_GPIO
>> +			CH_MODE_GPIO
>> +			CH_MODE_GPIO
>> +		>;
>> +
>> +	};
>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>> index 31a1985..e7dd376 100644
>> --- a/drivers/iio/dac/Kconfig
>> +++ b/drivers/iio/dac/Kconfig
>> @@ -74,6 +74,33 @@ config AD5449
>>   	  To compile this driver as a module, choose M here: the
>>   	  module will be called ad5449.
>>
>> +config AD5592R_BASE
>> +	tristate
>> +
>> +config AD5592R
>> +	tristate "Analog Devices AD5592R ADC/DAC driver"
>> +	depends on SPI_MASTER
>> +	depends on OF
>> +	select AD5592R_BASE
>> +	help
>> +	  Say yes here to build support for Analog Devices AD5592R
>> +	  Digital to Analog / Analog to Digital Converter.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called ad5592r.
>> +
>> +config AD5593R
>> +	tristate "Analog Devices AD5593R ADC/DAC driver"
>> +	depends on I2C
>> +	depends on OF
>> +	select AD5592R_BASE
>> +	help
>> +	  Say yes here to build support for Analog Devices AD5593R
>> +	  Digital to Analog / Analog to Digital Converter.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called ad5593r.
>> +
>>   config AD5504
>>   	tristate "Analog Devices AD5504/AD5501 DAC SPI driver"
>>   	depends on SPI
>> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
>> index e2deda9..cf23310 100644
>> --- a/drivers/iio/dac/Makefile
>> +++ b/drivers/iio/dac/Makefile
>> @@ -11,6 +11,9 @@ obj-$(CONFIG_AD5064) += ad5064.o
>>   obj-$(CONFIG_AD5504) += ad5504.o
>>   obj-$(CONFIG_AD5446) += ad5446.o
>>   obj-$(CONFIG_AD5449) += ad5449.o
>> +obj-$(CONFIG_AD5592R_BASE) += ad5592r-base.o
>> +obj-$(CONFIG_AD5592R) += ad5592r.o
>> +obj-$(CONFIG_AD5593R) += ad5593r.o
>>   obj-$(CONFIG_AD5755) += ad5755.o
>>   obj-$(CONFIG_AD5761) += ad5761.o
>>   obj-$(CONFIG_AD5764) += ad5764.o
>> diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
>> new file mode 100644
>> index 0000000..6dd4eab
>> --- /dev/null
>> +++ b/drivers/iio/dac/ad5592r-base.c
>> @@ -0,0 +1,675 @@
>> +/*
>> + * AD5592R Digital <-> Analog converters driver
>> + *
>> + * Copyright 2014-2016 Analog Devices Inc.
>> + * Author: Paul Cercueil <paul.cercueil@analog.com>
>> + *
>> + * Licensed under the GPL-2.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/delay.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/gpio.h>
>> +
>> +#include <dt-bindings/iio/adi,ad5592r.h>
>> +
>> +#include "ad5592r-base.h"
>> +
>> +#ifdef CONFIG_GPIOLIB
>> +
>> +static int ad5592r_gpio_get(struct gpio_chip *chip, unsigned offset)
>> +{
>> +	struct ad5592r_state *st = gpiochip_get_data(chip);
>> +	int ret = 0;
>> +	u8 val;
>> +
>> +	mutex_lock(&st->gpio_lock);
>> +
>> +	if (st->gpio_out & BIT(offset))
>> +		val = st->gpio_val;
>> +	else
>> +		ret = st->ops->gpio_read(st, &val);
>> +
>> +	mutex_unlock(&st->gpio_lock);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return !!(val & BIT(offset));
>> +}
>> +
>> +static void ad5592r_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>> +{
>> +	struct ad5592r_state *st = gpiochip_get_data(chip);
>> +
>> +	mutex_lock(&st->gpio_lock);
>> +
>> +	if (value)
>> +		st->gpio_val |= BIT(offset);
>> +	else
>> +		st->gpio_val &= ~BIT(offset);
>> +
>> +	st->ops->reg_write(st, AD5592R_REG_GPIO_SET, st->gpio_val);
>> +
>> +	mutex_unlock(&st->gpio_lock);
>> +}
>> +
>> +static int ad5592r_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
>> +{
>> +	struct ad5592r_state *st = gpiochip_get_data(chip);
>> +	int ret;
>> +
>> +	mutex_lock(&st->gpio_lock);
>> +
>> +	st->gpio_out &= ~BIT(offset);
>> +	st->gpio_in |= BIT(offset);
>> +
>> +	ret = st->ops->reg_write(st, AD5592R_REG_GPIO_OUT_EN, st->gpio_out);
>> +	if (ret < 0)
>> +		goto err_unlock;
>> +
>> +	ret = st->ops->reg_write(st, AD5592R_REG_GPIO_IN_EN, st->gpio_in);
>> +
>> +err_unlock:
>> +	mutex_unlock(&st->gpio_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ad5592r_gpio_direction_output(struct gpio_chip *chip,
>> +					 unsigned offset, int value)
>> +{
>> +	struct ad5592r_state *st = gpiochip_get_data(chip);
>> +	int ret;
>> +
>> +	mutex_lock(&st->gpio_lock);
>> +
>> +	if (value)
>> +		st->gpio_val |= BIT(offset);
>> +	else
>> +		st->gpio_val &= ~BIT(offset);
>> +
>> +	st->gpio_in &= ~BIT(offset);
>> +	st->gpio_out |= BIT(offset);
>> +
>> +	ret = st->ops->reg_write(st, AD5592R_REG_GPIO_SET, st->gpio_val);
>> +	if (ret < 0)
>> +		goto err_unlock;
>> +
>> +	ret = st->ops->reg_write(st, AD5592R_REG_GPIO_OUT_EN, st->gpio_out);
>> +	if (ret < 0)
>> +		goto err_unlock;
>> +
>> +	ret = st->ops->reg_write(st, AD5592R_REG_GPIO_IN_EN, st->gpio_in);
>> +
>> +err_unlock:
>> +	mutex_unlock(&st->gpio_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ad5592r_gpio_request(struct gpio_chip *chip, unsigned offset)
>> +{
>> +	struct ad5592r_state *st = gpiochip_get_data(chip);
>> +
>> +	if (!(st->gpio_map & BIT(offset))) {
>> +		dev_err(st->dev, "GPIO %d is reserved by alternate function\n",
>> +			offset);
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (offset >= chip->ngpio)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ad5592r_gpio_init(struct ad5592r_state *st)
>> +{
>> +	st->gpiochip.label = dev_name(st->dev);
>> +	st->gpiochip.base = -1;
>> +	st->gpiochip.ngpio = 8;
>> +	st->gpiochip.parent = st->dev;
>> +	st->gpiochip.can_sleep = true;
>> +	st->gpiochip.direction_input = ad5592r_gpio_direction_input;
>> +	st->gpiochip.direction_output = ad5592r_gpio_direction_output;
>> +	st->gpiochip.get = ad5592r_gpio_get;
>> +	st->gpiochip.set = ad5592r_gpio_set;
>> +	st->gpiochip.request = ad5592r_gpio_request;
>> +	st->gpiochip.owner = THIS_MODULE;
>> +
>> +	mutex_init(&st->gpio_lock);
>> +
>> +	return gpiochip_add_data(&st->gpiochip, st);
>> +}
>> +
>> +static void ad5592r_gpio_cleanup(struct ad5592r_state *st)
>> +{
>> +	gpiochip_remove(&st->gpiochip);
>> +}
>> +#else
>> +static int ad5592r_gpio_init(struct ad5592r_state *st) { return 0 };
>> +static void ad5592r_gpio_cleanup(struct ad5592r_state *st) { };
>> +#endif /* CONFIG_GPIOLIB */
>> +
>> +static int ad5592r_reset(struct ad5592r_state *st)
>> +{
>> +	struct gpio_desc *gpio;
>> +	struct iio_dev *iio_dev = iio_priv_to_dev(st);
>> +
>> +	gpio = devm_gpiod_get_optional(st->dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(gpio))
>> +		return PTR_ERR(gpio);
>> +
>> +	if (gpio) {
>> +		udelay(1);
>> +		gpiod_set_value(gpio, 1);
>> +	} else {
>> +		mutex_lock(&iio_dev->mlock);
>> +		st->ops->reg_write(st, AD5592R_REG_RESET, 0xdac);
>> +		mutex_unlock(&iio_dev->mlock);
>> +	}
>> +
>> +	udelay(250);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ad5592r_get_vref(struct ad5592r_state *st)
>> +{
>> +	int ret;
>> +
>> +	if (st->reg) {
>> +		ret = regulator_get_voltage(st->reg);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		return ret / 1000;
>> +	} else {
>> +		return 2500;
>> +	}
>> +}
>> +
>> +static int ad5592r_set_channel_modes(struct ad5592r_state *st)
>> +{
>> +	const struct ad5592r_rw_ops *ops = st->ops;
>> +	int ret;
>> +	unsigned i;
>> +	struct iio_dev *iio_dev = iio_priv_to_dev(st);
>> +	u8 pulldown = 0, open_drain = 0, tristate = 0,
>> +	   dac = 0, adc = 0;
>> +	u16 read_back;
>> +
>> +	for (i = 0; i < st->num_channels; i++) {
>> +		switch (st->channel_modes[i]) {
>> +		case CH_MODE_DAC:
>> +			dac |= BIT(i);
>> +			break;
>> +
>> +		case CH_MODE_ADC:
>> +			adc |= BIT(i);
>> +			break;
>> +
>> +		case CH_MODE_DAC_AND_ADC:
>> +			dac |= BIT(i);
>> +			adc |= BIT(i);
>> +			break;
>> +
>> +		case CH_MODE_UNUSED_PULL_DOWN:
>> +			pulldown |= BIT(i);
>> +			break;
>> +
>> +		case CH_MODE_UNUSED_OUT_TRISTATE:
>> +			tristate |= BIT(i);
>> +			break;
>> +
>> +		case CH_MODE_UNUSED_OUT_LOW:
>> +			st->gpio_out |= BIT(i);
>> +			break;
>> +
>> +		case CH_MODE_UNUSED_OUT_HIGH:
>> +			st->gpio_out |= BIT(i);
>> +			st->gpio_val |= BIT(i);
>> +			break;
>> +
>> +		case CH_MODE_GPIO_OPEN_DRAIN:
>> +			open_drain |= BIT(i);
>> +
>> +			/* fall-through */
>> +
>> +		case CH_MODE_GPIO:
>> +			st->gpio_map |= BIT(i);
>> +			st->gpio_in |= BIT(i); /* Default to input */
>> +			break;
>> +
>> +		default:
>> +			pulldown |= BIT(i);
>> +			break;
>> +		}
>> +	}
>> +
>> +	mutex_lock(&iio_dev->mlock);
>> +
>> +	/* Pull down unused pins to GND */
>> +	ret = ops->reg_write(st, AD5592R_REG_PULLDOWN, pulldown);
>> +	if (ret)
>> +		goto err_unlock;
>> +
>> +	ret = ops->reg_write(st, AD5592R_REG_TRISTATE, tristate);
>> +	if (ret)
>> +		goto err_unlock;
>> +
>> +	/* Configure pins that we use */
>> +	ret = ops->reg_write(st, AD5592R_REG_DAC_EN, dac);
>> +	if (ret)
>> +		goto err_unlock;
>> +
>> +	ret = ops->reg_write(st, AD5592R_REG_ADC_EN, adc);
>> +	if (ret)
>> +		goto err_unlock;
>> +
>> +	ret = ops->reg_write(st, AD5592R_REG_OPEN_DRAIN, open_drain);
>> +	if (ret)
>> +		goto err_unlock;
>> +
>> +	ret = ops->reg_write(st, AD5592R_REG_GPIO_SET, st->gpio_val);
>> +	if (ret)
>> +		goto err_unlock;
>> +
>> +	ret = ops->reg_write(st, AD5592R_REG_GPIO_OUT_EN, st->gpio_out);
>> +	if (ret)
>> +		goto err_unlock;
>> +
>> +	ret = ops->reg_write(st, AD5592R_REG_GPIO_IN_EN, st->gpio_in);
>> +	if (ret)
>> +		goto err_unlock;
>> +
>> +	/* Verify that we can read back at least one register */
>> +	ret = ops->reg_read(st, AD5592R_REG_ADC_EN, &read_back);
>> +	if (!ret && (read_back & 0xff) != adc)
>> +		ret = -EIO;
>> +
>> +err_unlock:
>> +	mutex_unlock(&iio_dev->mlock);
>> +	return ret;
>> +}
>> +
>> +static int ad5592r_write_raw(struct iio_dev *iio_dev,
>> +	struct iio_chan_spec const *chan, int val, int val2, long mask)
>> +{
>> +	struct ad5592r_state *st = iio_priv(iio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		if (val >= (1 << chan->scan_type.realbits) || val < 0)
>> +			return -EINVAL;
>> +
>> +		/* Warn if we try to write to a ADC channel */
>> +		WARN_ON(!chan->output);
> Probably just wants to return an error rather than filling the logs.
> People do silly things all time like that.

ok

>> +
>> +		mutex_lock(&iio_dev->mlock);
>> +		ret = st->ops->write_dac(st, chan->channel, val);
>> +		if (!ret)
>> +			st->cached_dac[chan->channel] = val;
>> +		mutex_unlock(&iio_dev->mlock);
>> +		return ret;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		if (chan->type == IIO_VOLTAGE) {
>> +			bool gain;
>> +
>> +			if (val == st->scale_avail[0][0] &&
>> +				val2 == st->scale_avail[0][1])
>> +				gain = false;
>> +			else if (val == st->scale_avail[1][0] &&
>> +				 val2 == st->scale_avail[1][1])
>> +				gain = true;
>> +			else
>> +				return -EINVAL;
>> +
>> +			mutex_lock(&iio_dev->mlock);
>> +
>> +			ret = st->ops->reg_read(st, AD5592R_REG_CTRL,
>> +						&st->cached_gp_ctrl);
>> +			if (ret < 0) {
>> +				mutex_unlock(&iio_dev->mlock);
>> +				return ret;
>> +			}
>> +
>> +			if (chan->output) {
>> +				if (gain)
>> +					st->cached_gp_ctrl |=
>> +						AD5592R_REG_CTRL_DAC_RANGE;
>> +				else
>> +					st->cached_gp_ctrl &=
>> +						~AD5592R_REG_CTRL_DAC_RANGE;
>> +			} else {
>> +				if (gain)
>> +					st->cached_gp_ctrl |=
>> +						AD5592R_REG_CTRL_ADC_RANGE;
>> +				else
>> +					st->cached_gp_ctrl &=
>> +						~AD5592R_REG_CTRL_ADC_RANGE;
>> +			}
>> +
>> +			ret = st->ops->reg_write(st, AD5592R_REG_CTRL,
>> +						 st->cached_gp_ctrl);
>> +			mutex_unlock(&iio_dev->mlock);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			return ret;
>> +
>> +		}
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ad5592r_read_raw(struct iio_dev *iio_dev,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val, int *val2, long m)
>> +{
>> +	struct ad5592r_state *st = iio_priv(iio_dev);
>> +	u16 read_val;
>> +	int ret;
>> +
>> +	mutex_lock(&iio_dev->mlock);
> Move the lock into the switch statement as it doesn't
> need to be held for a fair bit of the code below (such as the scale read
> backs).

ok

>> +
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_RAW:
>> +
>> +		if (!chan->output) {
>> +			ret = st->ops->read_adc(st, chan->channel, &read_val);
>> +			if (ret)
>> +				goto unlock;
>> +
>> +			if ((read_val >> 12 & 0x7) != (chan->channel & 0x7)) {
>> +				dev_err(st->dev, "Error while reading channel %u\n",
>> +						chan->channel);
>> +				ret = -EIO;
>> +				goto unlock;
>> +			}
>> +
>> +			read_val &= GENMASK(11, 0);
>> +
>> +		} else {
>> +			read_val = st->cached_dac[chan->channel];
>> +		}
>> +
>> +		dev_dbg(st->dev, "Channel %u read: 0x%04hX\n",
>> +				chan->channel, read_val);
>> +
>> +		*val = (int) read_val;
>> +		ret = IIO_VAL_INT;
>> +		break;
>> +	case IIO_CHAN_INFO_SCALE:
>> +
>> +		*val = ad5592r_get_vref(st);
>> +
>> +		if (chan->type == IIO_TEMP) {
>> +			s64 tmp = *val * (3767897513LL / 25LL);
>> +			*val = div_s64_rem(tmp, 1000000000LL, val2);
>> +
>> +			ret = IIO_VAL_INT_PLUS_MICRO;
>> +		} else {
>> +			int mult;
>> +
>> +			if (chan->output)
>> +				mult = !!(st->cached_gp_ctrl &
>> +					AD5592R_REG_CTRL_DAC_RANGE);
>> +			else
>> +				mult = !!(st->cached_gp_ctrl &
>> +					AD5592R_REG_CTRL_ADC_RANGE);
>> +
>> +			*val *= ++mult;
>> +
>> +			*val2 = chan->scan_type.realbits;
>> +			ret = IIO_VAL_FRACTIONAL_LOG2;
>> +		}
>> +		break;
>> +	case IIO_CHAN_INFO_OFFSET:
>> +
>> +		ret = ad5592r_get_vref(st);
>> +
>> +		if (st->cached_gp_ctrl & AD5592R_REG_CTRL_ADC_RANGE)
>> +			*val = (-34365 * 25) / ret;
>> +		else
>> +			*val = (-75365 * 25) / ret;
>> +		ret =  IIO_VAL_INT;
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
>> +unlock:
>> +	mutex_unlock(&iio_dev->mlock);
>> +	return ret;
>> +}
>> +
>> +static int ad5592r_write_raw_get_fmt(struct iio_dev *indio_dev,
>> +				 struct iio_chan_spec const *chan, long mask)
>> +{
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		return IIO_VAL_INT_PLUS_NANO;
>> +
>> +	default:
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info ad5592r_info = {
>> +	.read_raw = ad5592r_read_raw,
>> +	.write_raw = ad5592r_write_raw,
>> +	.write_raw_get_fmt = ad5592r_write_raw_get_fmt,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +static ssize_t ad5592r_show_scale_available(struct iio_dev *iio_dev,
>> +					   uintptr_t private,
>> +					   const struct iio_chan_spec *chan,
>> +					   char *buf)
>> +{
>> +	struct ad5592r_state *st = iio_priv(iio_dev);
>> +
>> +	return sprintf(buf, "%d.%09u %d.%09u\n",
>> +		st->scale_avail[0][0], st->scale_avail[0][1],
>> +		st->scale_avail[1][0], st->scale_avail[1][1]);
>> +}
>> +
>> +static struct iio_chan_spec_ext_info ad5592r_ext_info[] = {
>> +	{
>> +	 .name = "scale_available",
>> +	 .read = ad5592r_show_scale_available,
>> +	 .shared = true,
>> +	 },
>> +	{},
>> +};
>> +
>> +static void ad5592r_setup_channel(struct iio_dev *iio_dev,
>> +		struct iio_chan_spec *chan, bool output, unsigned id)
>> +{
>> +	chan->type = IIO_VOLTAGE;
>> +	chan->indexed = 1;
>> +	chan->output = output;
>> +	chan->channel = id;
>> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
>> +	chan->scan_type.sign = 'u';
>> +	chan->scan_type.realbits = 12;
>> +	chan->scan_type.storagebits = 16;
>> +	chan->ext_info = ad5592r_ext_info;
>> +}
>> +
>> +static int ad5592r_alloc_channels(struct ad5592r_state *st)
>> +{
>> +	unsigned i, curr_channel = 0,
>> +		 num_channels = st->num_channels;
>> +	struct iio_dev *iio_dev = iio_priv_to_dev(st);
>> +	struct iio_chan_spec *channels;
>> +	int ret;
>> +
>> +	ret = device_property_read_u8_array(st->dev, "channel-modes",
>> +			st->channel_modes, num_channels);
>> +	if (ret)
>> +		return ret;
>> +
>> +	channels = devm_kzalloc(st->dev,
>> +			(1 + 2 * num_channels) * sizeof(*channels), GFP_KERNEL);
>> +	if (!channels)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < num_channels; i++) {
>> +		switch (st->channel_modes[i]) {
>> +		case CH_MODE_DAC:
>> +			ad5592r_setup_channel(iio_dev, &channels[curr_channel],
>> +					true, i);
>> +			curr_channel++;
>> +			break;
>> +
>> +		case CH_MODE_ADC:
>> +			ad5592r_setup_channel(iio_dev, &channels[curr_channel],
>> +					false, i);
>> +			curr_channel++;
>> +			break;
>> +
>> +		case CH_MODE_DAC_AND_ADC:
>> +			ad5592r_setup_channel(iio_dev, &channels[curr_channel],
>> +					true, i);
>> +			curr_channel++;
>> +			ad5592r_setup_channel(iio_dev, &channels[curr_channel],
>> +					false, i);
>> +			curr_channel++;
>> +			break;
>> +
>> +		default:
>> +			continue;
>> +		}
>> +	}
>> +
>> +	channels[curr_channel].type = IIO_TEMP;
>> +	channels[curr_channel].channel = 8;
>> +	channels[curr_channel].info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +				   BIT(IIO_CHAN_INFO_SCALE) |
>> +				   BIT(IIO_CHAN_INFO_OFFSET);
>> +	curr_channel++;
>> +
>> +	iio_dev->num_channels = curr_channel;
>> +	iio_dev->channels = channels;
>> +
>> +	return 0;
>> +}
>> +
>> +static void ad5592r_init_scales(struct ad5592r_state *st, int vref_mV)
>> +{
>> +	s64 tmp = (s64)vref_mV * 1000000000LL >> 12;
>> +
>> +	st->scale_avail[0][0] =
>> +		div_s64_rem(tmp, 1000000000LL, &st->scale_avail[0][1]);
>> +	st->scale_avail[1][0] =
>> +		div_s64_rem(tmp * 2, 1000000000LL, &st->scale_avail[1][1]);
>> +}
>> +
>> +int ad5592r_probe(struct device *dev, const char *name,
>> +		const struct ad5592r_rw_ops *ops)
>> +{
>> +	struct iio_dev *iio_dev;
>> +	struct ad5592r_state *st;
>> +	int ret;
>> +
>> +	iio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>> +	if (!iio_dev)
>> +		return -ENOMEM;
>> +
>> +	st = iio_priv(iio_dev);
>> +	st->dev = dev;
>> +	st->ops = ops;
>> +	st->num_channels = 8;
>> +	dev_set_drvdata(dev, iio_dev);
>> +
>> +	st->reg = devm_regulator_get_optional(dev, "vref");
>> +	if (IS_ERR(st->reg)) {
>> +		if ((PTR_ERR(st->reg) != -ENODEV) && dev->of_node)
>> +			return PTR_ERR(st->reg);
>> +
>> +		st->reg = NULL;
>> +	} else {
>> +		ret = regulator_enable(st->reg);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	iio_dev->dev.parent = dev;
>> +	iio_dev->name = name;
>> +	iio_dev->info = &ad5592r_info;
>> +	iio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	ad5592r_init_scales(st, ad5592r_get_vref(st));
>> +
>> +	ret = ad5592r_reset(st);
>> +	if (ret)
>> +		goto error_disable_reg;
>> +
>> +	ret = ops->reg_write(st, AD5592R_REG_PD,
>> +		     (st->reg == NULL) ? AD5592R_REG_PD_EN_REF : 0);
>> +	if (ret)
>> +		goto error_disable_reg;
>> +
>> +	ret = ad5592r_alloc_channels(st);
>> +	if (ret)
>> +		goto error_disable_reg;
>> +
>> +	ret = ad5592r_set_channel_modes(st);
>> +	if (ret)
>> +		goto error_disable_reg;
>> +
>> +	ret = devm_iio_device_register(dev, iio_dev);
>> +	if (ret)
>> +		goto error_disable_reg;
> If you use a managed iio_device_register here it will only
> be unregistered after the end of the remove function.  It certainly looks
> like it really wants to be done at the start of the remove function
> so as to remove the exposed interfaces (user and in kernel) before changing
> the channel modes etc.
>
> Hence use the unmanaged version and iio_device_unregister in the remove.

I go for the unmanged version in both cases.

>> +
>> +	return ad5592r_gpio_init(st);
>> +
>> +error_disable_reg:
>> +	if (st->reg)
>> +		regulator_disable(st->reg);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(ad5592r_probe);
>> +
>> +int ad5592r_remove(struct device *dev)
>> +{
>> +	struct iio_dev *iio_dev = dev_get_drvdata(dev);
>> +	struct ad5592r_state *st = iio_priv(iio_dev);
>> +	unsigned int i;
>> +
>> +	/* Reset all channels */
>> +	for (i = 0; i < ARRAY_SIZE(st->channel_modes); i++)
>> +		st->channel_modes[i] = CH_MODE_UNUSED_PULL_DOWN;
>> +
>> +	if (st->reg)
>> +		regulator_disable(st->reg);
>> +
>> +	if (st->gpio_map)
>> +		ad5592r_gpio_cleanup(st);
>> +
>> +	return ad5592r_set_channel_modes(st);
>> +}
>> +EXPORT_SYMBOL_GPL(ad5592r_remove);
>> +
>> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil@analog.com>");
>> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/iio/dac/ad5592r-base.h b/drivers/iio/dac/ad5592r-base.h
>> new file mode 100644
>> index 0000000..162e833
>> --- /dev/null
>> +++ b/drivers/iio/dac/ad5592r-base.h
>> @@ -0,0 +1,77 @@
>> +/*
>> + * AD5592R / AD5593R Digital <-> Analog converters driver
>> + *
>> + * Copyright 2015-2016 Analog Devices Inc.
>> + * Author: Paul Cercueil <paul.cercueil@analog.com>
>> + *
>> + * Licensed under the GPL-2.
>> + */
>> +
>> +#ifndef __DRIVERS_IIO_DAC_AD5592R_BASE_H__
>> +#define __DRIVERS_IIO_DAC_AD5592R_BASE_H__
>> +
>> +#include <linux/types.h>
>> +#include <linux/cache.h>
>> +#include <linux/mutex.h>
>> +#include <linux/gpio/driver.h>
>> +
>> +struct device;
>> +struct ad5592r_state;
>> +
>> +enum ad5592r_registers {
>> +	AD5592R_REG_NOOP		= 0x0,
>> +	AD5592R_REG_DAC_READBACK	= 0x1,
>> +	AD5592R_REG_ADC_SEQ		= 0x2,
>> +	AD5592R_REG_CTRL		= 0x3,
>> +	AD5592R_REG_ADC_EN		= 0x4,
>> +	AD5592R_REG_DAC_EN		= 0x5,
>> +	AD5592R_REG_PULLDOWN		= 0x6,
>> +	AD5592R_REG_LDAC		= 0x7,
>> +	AD5592R_REG_GPIO_OUT_EN		= 0x8,
>> +	AD5592R_REG_GPIO_SET		= 0x9,
>> +	AD5592R_REG_GPIO_IN_EN		= 0xA,
>> +	AD5592R_REG_PD			= 0xB,
>> +	AD5592R_REG_OPEN_DRAIN		= 0xC,
>> +	AD5592R_REG_TRISTATE		= 0xD,
>> +	AD5592R_REG_RESET		= 0xF,
>> +};
>> +
>> +#define AD5592R_REG_PD_EN_REF		BIT(9)
>> +#define AD5592R_REG_CTRL_ADC_RANGE	BIT(5)
>> +#define AD5592R_REG_CTRL_DAC_RANGE	BIT(4)
>> +
>> +struct ad5592r_rw_ops {
>> +	int (*write_dac)(struct ad5592r_state *st, unsigned chan, u16 value);
>> +	int (*read_adc)(struct ad5592r_state *st, unsigned chan, u16 *value);
>> +	int (*reg_write)(struct ad5592r_state *st, u8 reg, u16 value);
>> +	int (*reg_read)(struct ad5592r_state *st, u8 reg, u16 *value);
>> +	int (*gpio_read)(struct ad5592r_state *st, u8 *value);
>> +};
>> +
>> +struct ad5592r_state {
>> +	struct device *dev;
>> +	struct regulator *reg;
>> +#ifdef CONFIG_GPIOLIB
>> +	struct gpio_chip gpiochip;
>> +	struct mutex gpio_lock;	/* Protect cached gpio_out, gpio_val, etc. */
>> +#endif
>> +	unsigned int num_channels;
>> +	const struct ad5592r_rw_ops *ops;
>> +	int scale_avail[2][2];
>> +	u16 cached_dac[8];
>> +	u16 cached_gp_ctrl;
>> +	u8 channel_modes[8];
>> +	u8 gpio_map;
>> +	u8 gpio_out;
>> +	u8 gpio_in;
>> +	u8 gpio_val;
>> +
>> +	__be16 spi_msg ____cacheline_aligned;
>> +	__be16 spi_msg_nop;
>> +};
>> +
>> +int ad5592r_probe(struct device *dev, const char *name,
>> +		const struct ad5592r_rw_ops *ops);
>> +int ad5592r_remove(struct device *dev);
>> +
>> +#endif /* __DRIVERS_IIO_DAC_AD5592R_BASE_H__ */
>> diff --git a/drivers/iio/dac/ad5592r.c b/drivers/iio/dac/ad5592r.c
>> new file mode 100644
>> index 0000000..0b235a2
>> --- /dev/null
>> +++ b/drivers/iio/dac/ad5592r.c
>> @@ -0,0 +1,164 @@
>> +/*
>> + * AD5592R Digital <-> Analog converters driver
>> + *
>> + * Copyright 2015-2016 Analog Devices Inc.
>> + * Author: Paul Cercueil <paul.cercueil@analog.com>
>> + *
>> + * Licensed under the GPL-2.
>> + */
>> +
>> +#include "ad5592r-base.h"
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#define AD5592R_GPIO_READBACK_EN	BIT(10)
>> +#define AD5592R_LDAC_READBACK_EN	BIT(6)
>> +
>> +static int ad5592r_spi_wnop_r16(struct ad5592r_state *st, u16 *buf)
>> +{
>> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>> +	struct spi_transfer t = {
>> +			.tx_buf	= &st->spi_msg_nop,
>> +			.rx_buf	= buf,
>> +			.len = 2
>> +		};
>> +
>> +	st->spi_msg_nop = 0; /* NOP */
>> +
>> +	return spi_sync_transfer(spi, &t, 1);
>> +}
>> +
>> +static int ad5592r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value)
>> +{
>> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>> +
>> +	st->spi_msg = cpu_to_be16(BIT(15) | (chan << 12) | value);
>> +
>> +	return spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
>> +}
>> +
>> +static int ad5592r_read_adc(struct ad5592r_state *st, unsigned chan, u16 *value)
>> +{
>> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>> +	int ret;
>> +
>> +	st->spi_msg = cpu_to_be16((AD5592R_REG_ADC_SEQ << 11) | BIT(chan));
>> +
>> +	ret = spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Invalid data:
>> +	 * See Figure 40. Single-Channel ADC Conversion Sequence
>> +	 */
>> +	ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*value = be16_to_cpu(st->spi_msg);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ad5592r_reg_write(struct ad5592r_state *st, u8 reg, u16 value)
>> +{
>> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>> +
>> +	st->spi_msg = cpu_to_be16((reg << 11) | value);
>> +
>> +	return spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
>> +}
>> +
>> +static int ad5592r_reg_read(struct ad5592r_state *st, u8 reg, u16 *value)
>> +{
>> +	struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>> +	int ret;
>> +
>> +	st->spi_msg = cpu_to_be16((AD5592R_REG_LDAC << 11) |
>> +				   AD5592R_LDAC_READBACK_EN | (reg << 2));
>> +
>> +	ret = spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*value = be16_to_cpu(st->spi_msg);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ad5593r_gpio_read(struct ad5592r_state *st, u8 *value)
>> +{
>> +	int ret;
>> +
>> +	ret = ad5592r_reg_write(st, AD5592R_REG_GPIO_IN_EN,
>> +				AD5592R_GPIO_READBACK_EN | st->gpio_in);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*value = (u8) be16_to_cpu(st->spi_msg);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct ad5592r_rw_ops ad5592r_rw_ops = {
>> +	.write_dac = ad5592r_write_dac,
>> +	.read_adc = ad5592r_read_adc,
>> +	.reg_write = ad5592r_reg_write,
>> +	.reg_read = ad5592r_reg_read,
>> +	.gpio_read = ad5593r_gpio_read,
>> +};
>> +
>> +static int ad5592r_spi_probe(struct spi_device *spi)
>> +{
>> +	const struct spi_device_id *id = spi_get_device_id(spi);
>> +
>> +	return ad5592r_probe(&spi->dev, id->name, &ad5592r_rw_ops);
>> +}
>> +
>> +static int ad5592r_spi_remove(struct spi_device *spi)
>> +{
>> +	return ad5592r_remove(&spi->dev);
>> +}
>> +
>> +static const struct spi_device_id ad5592r_spi_ids[] = {
>> +	{ .name = "ad5592r", },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(spi, ad5592r_spi_ids);
>> +
>> +static const struct of_device_id ad5592r_of_match[] = {
>> +	{ .compatible = "adi,ad5592r", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ad5592r_of_match);
>> +
>> +static struct spi_driver ad5592r_spi_driver = {
>> +	.driver = {
>> +		.name = "ad5592r",
>> +		.of_match_table = of_match_ptr(ad5592r_of_match),
>> +	},
>> +	.probe = ad5592r_spi_probe,
>> +	.remove = ad5592r_spi_remove,
>> +	.id_table = ad5592r_spi_ids,
>> +};
>> +module_spi_driver(ad5592r_spi_driver);
>> +
>> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil@analog.com>");
>> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/iio/dac/ad5593r.c b/drivers/iio/dac/ad5593r.c
>> new file mode 100644
>> index 0000000..dca158a
>> --- /dev/null
>> +++ b/drivers/iio/dac/ad5593r.c
>> @@ -0,0 +1,131 @@
>> +/*
>> + * AD5593R Digital <-> Analog converters driver
>> + *
>> + * Copyright 2015-2016 Analog Devices Inc.
>> + * Author: Paul Cercueil <paul.cercueil@analog.com>
>> + *
>> + * Licensed under the GPL-2.
>> + */
>> +
>> +#include "ad5592r-base.h"
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +
>> +#define AD5593R_MODE_CONF		(0 << 4)
>> +#define AD5593R_MODE_DAC_WRITE		(1 << 4)
>> +#define AD5593R_MODE_ADC_READBACK	(4 << 4)
>> +#define AD5593R_MODE_DAC_READBACK	(5 << 4)
>> +#define AD5593R_MODE_GPIO_READBACK	(6 << 4)
>> +#define AD5593R_MODE_REG_READBACK	(7 << 4)
>> +
>> +static int ad5593r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value)
>> +{
>> +	struct i2c_client *i2c = to_i2c_client(st->dev);
>> +
>> +	return i2c_smbus_write_word_swapped(i2c,
>> +			AD5593R_MODE_DAC_WRITE | chan, value);
>> +}
>> +
>> +static int ad5593r_read_adc(struct ad5592r_state *st, unsigned chan, u16 *value)
>> +{
>> +	struct i2c_client *i2c = to_i2c_client(st->dev);
>> +	s32 val;
>> +
>> +	val = i2c_smbus_write_word_swapped(i2c,
>> +			AD5593R_MODE_CONF | AD5592R_REG_ADC_SEQ, BIT(chan));
>> +	if (val < 0)
>> +		return (int) val;
>> +
>> +	val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_ADC_READBACK);
>> +	if (val < 0)
>> +		return (int) val;
>> +
>> +	*value = (u16) val;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ad5593r_reg_write(struct ad5592r_state *st, u8 reg, u16 value)
>> +{
>> +	struct i2c_client *i2c = to_i2c_client(st->dev);
>> +
>> +	return i2c_smbus_write_word_swapped(i2c,
>> +			AD5593R_MODE_CONF | reg, value);
>> +}
>> +
>> +static int ad5593r_reg_read(struct ad5592r_state *st, u8 reg, u16 *value)
>> +{
>> +	struct i2c_client *i2c = to_i2c_client(st->dev);
>> +	s32 val;
>> +
>> +	val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_REG_READBACK | reg);
>> +	if (val < 0)
>> +		return (int) val;
>> +
>> +	*value = (u16) val;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ad5593r_gpio_read(struct ad5592r_state *st, u8 *value)
>> +{
>> +	struct i2c_client *i2c = to_i2c_client(st->dev);
>> +	s32 val;
>> +
>> +	val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_GPIO_READBACK);
>> +	if (val < 0)
>> +		return (int) val;
>> +
>> +	*value = (u8) val;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct ad5592r_rw_ops ad5593r_rw_ops = {
>> +	.write_dac = ad5593r_write_dac,
>> +	.read_adc = ad5593r_read_adc,
>> +	.reg_write = ad5593r_reg_write,
>> +	.reg_read = ad5593r_reg_read,
>> +	.gpio_read = ad5593r_gpio_read,
>> +};
>> +
>> +static int ad5593r_i2c_probe(struct i2c_client *i2c,
>> +		const struct i2c_device_id *id)
>> +{
>> +	return ad5592r_probe(&i2c->dev, id->name, &ad5593r_rw_ops);
>> +}
>> +
>> +static int ad5593r_i2c_remove(struct i2c_client *i2c)
>> +{
>> +	return ad5592r_remove(&i2c->dev);
>> +}
>> +
>> +static const struct i2c_device_id ad5593r_i2c_ids[] = {
>> +	{ .name = "ad5593r", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ad5593r_i2c_ids);
>> +
>> +static const struct of_device_id ad5593r_of_match[] = {
>> +	{ .compatible = "adi,ad5593r", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ad5593r_of_match);
>> +
>> +static struct i2c_driver ad5593r_driver = {
>> +	.driver = {
>> +		.name = "ad5593r",
>> +		.of_match_table = of_match_ptr(ad5593r_of_match),
>> +	},
>> +	.probe = ad5593r_i2c_probe,
>> +	.remove = ad5593r_i2c_remove,
>> +	.id_table = ad5593r_i2c_ids,
>> +};
>> +module_i2c_driver(ad5593r_driver);
>> +
>> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil@analog.com>");
>> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/dt-bindings/iio/adi,ad5592r.h b/include/dt-bindings/iio/adi,ad5592r.h
>> new file mode 100644
>> index 0000000..6bd519b
>> --- /dev/null
>> +++ b/include/dt-bindings/iio/adi,ad5592r.h
>> @@ -0,0 +1,16 @@
>> +
>> +#ifndef _DT_BINDINGS_ADI_AD5592R_H
>> +#define _DT_BINDINGS_ADI_AD5592R_H
>> +
>> +
>> +#define CH_MODE_ADC			1
>> +#define CH_MODE_DAC			2
>> +#define CH_MODE_DAC_AND_ADC		3
>> +#define CH_MODE_UNUSED_PULL_DOWN	4
>> +#define CH_MODE_UNUSED_OUT_LOW		5
>> +#define CH_MODE_UNUSED_OUT_HIGH		6
>> +#define CH_MODE_UNUSED_OUT_TRISTATE	7
>> +#define CH_MODE_GPIO			8
>> +#define CH_MODE_GPIO_OPEN_DRAIN		9
>> +
>> +#endif /* _DT_BINDINGS_ADI_AD5592R_H */
>>
>


-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif

Re: [PATCH v2] iio: dac: Add support for the AD5592R/AD5593R ADCs/DACs

[Jonathan Cameron]

On 02/03/16 16:21, Michael Hennerich wrote:
> On 02/27/2016 06:50 PM, Jonathan Cameron wrote:
>> On 25/02/16 13:36, michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org wrote:
>>> From: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>>
>>> This patch adds support for the AD5592R (spi) and AD5593R (i2c)
>>> ADC/DAC devices.
>>>
>>> Signed-off-by: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>>
>> A few bits inline.
>>
>> I'll need a gpio review on this (looks fine to me but it does contain
>> a gpiochip driver.) Not to mention the question of whether they will
>> be happy with a gpio chip hiding in iio (rather than via an mfd with a
>> separate driver - which feels like overkill here).
>>
>> The big question to my mind is whether we can take the view this won't
>> be the last multipurpose chip we will see so do we need to sort the
>> binding out to make it generic?  It'll be a bit of a pain for you
>> but I think we can do it fairly easily.
>> (either way I'll also need a device tree ack on this one!)
>>
>> So then we get into the question of the best way of doing the bindings.
>> The gpio approach seems a little limiting for things as flexible as
>> this but we should certainly be using their macros where relevant.
> 
> Hi Jonathan,
> 
> Thanks for the review.
> 
> The problem is see is that using GPIOF_OPEN_DRAIN, will simulate OPEN DRAIN behaviour only, by configuring the device for input when outputing logic high.
> 
Is it actually documented as such anywhere? (that's hideous!)  

> In this case the chip does it.
> 
> See comments below.
> 
> 
>>
>> J
>>> ---
>>>
>>> Changes since v1:
>>>     * Fix mutex usage
>>>     * Remove unnecessary NULL pointer guards
>>>     * Add comment explaining the invalid data read
>>>     * AD5593R Remove surplus adc readback
>>>
>>>   .../devicetree/bindings/iio/dac/ad5592r.txt        |  88 +++
>>>   drivers/iio/dac/Kconfig                            |  27 +
>>>   drivers/iio/dac/Makefile                           |   3 +
>>>   drivers/iio/dac/ad5592r-base.c                     | 675 +++++++++++++++++++++
>>>   drivers/iio/dac/ad5592r-base.h                     |  77 +++
>>>   drivers/iio/dac/ad5592r.c                          | 164 +++++
>>>   drivers/iio/dac/ad5593r.c                          | 131 ++++
>>>   include/dt-bindings/iio/adi,ad5592r.h              |  16 +
>>>   8 files changed, 1181 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/iio/dac/ad5592r.txt
>>>   create mode 100644 drivers/iio/dac/ad5592r-base.c
>>>   create mode 100644 drivers/iio/dac/ad5592r-base.h
>>>   create mode 100644 drivers/iio/dac/ad5592r.c
>>>   create mode 100644 drivers/iio/dac/ad5593r.c
>>>   create mode 100644 include/dt-bindings/iio/adi,ad5592r.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/dac/ad5592r.txt b/Documentation/devicetree/bindings/iio/dac/ad5592r.txt
>>> new file mode 100644
>>> index 0000000..9d7f23a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/dac/ad5592r.txt
>>> @@ -0,0 +1,88 @@
>>> +Analog Devices AD5592R/AD5593R DAC/ADC device driver
>>> +
>>> +Required properties for the AD5592R:
>>> +    - compatible: Must be "adi,ad5592r"
>>> +    - reg: SPI chip select number for the device
>>> +    - spi-max-frequency: Max SPI frequency to use (< 30000000)
>>> +    - spi-cpol: The AD5592R requires inverse clock polarity (CPOL) mode
>>> +
>>> +Required properties for the AD5593R:
>>> +    - compatible: Must be "adi,ad5593r"
>>> +    - reg: I2C address of the device
>>> +
>>> +Required properties for all supported chips:
>>> +    - channel-modes: An array of eight 8-bit values (one per channel)
>>> +      describing the mode of each channel. Macros specifying the valid values
>>> +      can be found in <dt-bindings/iio/adi,ad5592r.h>.
>>> +      The following values are currently supported:
>> Lets think about making this truely generic.
>>
>> So we have a basic mode questio first.  I think we separate that from the later
>> part. Also do we break this down into individual channels?  It think we probably
>> do want to like we have done for various highly adapatable adcs in the past.
>>
>> so we'd have something like:
>>     channels {
>>         #address-cells = <1>;
>>         #size-cells = <0>;
>>         channel@0 {
>>             mode = CH_MODE_ADC,
>>         },
>>         channel@1 {
>>             mode = CH_MODE_DAC,
>>         },
>>         channel@2 {
>>             mode = CH_MODE_DAC_ADC
>>         },
>>         channel@3 {
>>             mode = CH_MODE_UNUSED,
>>             offstate = CH_OFFSTATE_PULLDOWN,
>>         },
>>         channel@4 {
>>             mode = CH_MODE_GPIO,
>>             //lift the opendrain stuff from the gpio bindings macros.   
>>         }
>>            
>> Or take something more similar to the gpio bindings perhaps?  bit fiddly here
>> as we can have a wide range of stuff and not all of it is always relevant.
>>
>> Input from others on this most welcome!
> 
> Let's go for this.
> 
> 
>>
>>
>>         }compatible = "regulator-fixed";
>>> +        regulator-name = "vref-ad559x";
>>> +        regulator-min-microvolt = <3300000>;
>>> +        regulator-max-microvolt = <3300000>;
>>> +        regulator-always-on;
>>> +    };
>>> +
>>> +    ad5592r@0 {
>>> +        compatible = "adi,ad5592r";
>>> +        reg = <0>;
>>> +        spi-max-frequency = <1000000>;
>>> +        spi-cpol;
>>> +
>>> +        channel-modes = /bits/ 8 <
>>> +            CH_MODE_DAC
>>> +            CH_MODE_ADC
>>> +            CH_MODE_DAC_AND_ADC
>>> +            CH_MODE_DAC_AND_ADC
>>> +            CH_MODE_UNUSED_PULL_DOWN
>>> +            CH_MODE_GPIO
>>> +            CH_MODE_GPIO
>>> +            CH_MODE_GPIO
>>> +        >;
>>> +
>>> +        vref-supply = <&vref>; /* optional */
>>> +        reset-gpios = <&gpio0 86 0>;  /* optional */
>>> +    };
>>> +
>>> +AD5593R Example:
>>> +
>>> +    #include <dt-bindings/iio/adi,ad5592r.h>
>>> +
>>> +    ad5593r@10 {
>>> +        compatible = "adi,ad5593r";
>>> +        reg = <0x10>;
>>> +        channel-modes = /bits/ 8 <
>>> +            CH_MODE_DAC
>>> +            CH_MODE_ADC
>>> +            CH_MODE_DAC_AND_ADC
>>> +            CH_MODE_DAC_AND_ADC
>>> +            CH_MODE_UNUSED_PULL_DOWN
>>> +            CH_MODE_GPIO
>>> +            CH_MODE_GPIO
>>> +            CH_MODE_GPIO
>>> +        >;
>>> +
>>> +    };
>>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>>> index 31a1985..e7dd376 100644
>>> --- a/drivers/iio/dac/Kconfig
>>> +++ b/drivers/iio/dac/Kconfig
>>> @@ -74,6 +74,33 @@ config AD5449
>>>         To compile this driver as a module, choose M here: the
>>>         module will be called ad5449.
>>>
>>> +config AD5592R_BASE
>>> +    tristate
>>> +
>>> +config AD5592R
>>> +    tristate "Analog Devices AD5592R ADC/DAC driver"
>>> +    depends on SPI_MASTER
>>> +    depends on OF
>>> +    select AD5592R_BASE
>>> +    help
>>> +      Say yes here to build support for Analog Devices AD5592R
>>> +      Digital to Analog / Analog to Digital Converter.
>>> +
>>> +      To compile this driver as a module, choose M here: the
>>> +      module will be called ad5592r.
>>> +
>>> +config AD5593R
>>> +    tristate "Analog Devices AD5593R ADC/DAC driver"
>>> +    depends on I2C
>>> +    depends on OF
>>> +    select AD5592R_BASE
>>> +    help
>>> +      Say yes here to build support for Analog Devices AD5593R
>>> +      Digital to Analog / Analog to Digital Converter.
>>> +
>>> +      To compile this driver as a module, choose M here: the
>>> +      module will be called ad5593r.
>>> +
>>>   config AD5504
>>>       tristate "Analog Devices AD5504/AD5501 DAC SPI driver"
>>>       depends on SPI
>>> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
>>> index e2deda9..cf23310 100644
>>> --- a/drivers/iio/dac/Makefile
>>> +++ b/drivers/iio/dac/Makefile
>>> @@ -11,6 +11,9 @@ obj-$(CONFIG_AD5064) += ad5064.o
>>>   obj-$(CONFIG_AD5504) += ad5504.o
>>>   obj-$(CONFIG_AD5446) += ad5446.o
>>>   obj-$(CONFIG_AD5449) += ad5449.o
>>> +obj-$(CONFIG_AD5592R_BASE) += ad5592r-base.o
>>> +obj-$(CONFIG_AD5592R) += ad5592r.o
>>> +obj-$(CONFIG_AD5593R) += ad5593r.o
>>>   obj-$(CONFIG_AD5755) += ad5755.o
>>>   obj-$(CONFIG_AD5761) += ad5761.o
>>>   obj-$(CONFIG_AD5764) += ad5764.o
>>> diff --git a/drivers/iio/dac/ad5592r-base.c b/drivers/iio/dac/ad5592r-base.c
>>> new file mode 100644
>>> index 0000000..6dd4eab
>>> --- /dev/null
>>> +++ b/drivers/iio/dac/ad5592r-base.c
>>> @@ -0,0 +1,675 @@
>>> +/*
>>> + * AD5592R Digital <-> Analog converters driver
>>> + *
>>> + * Copyright 2014-2016 Analog Devices Inc.
>>> + * Author: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>> + *
>>> + * Licensed under the GPL-2.
>>> + */
>>> +
>>> +#include <linux/bitops.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/of.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/gpio/consumer.h>
>>> +#include <linux/gpio/driver.h>
>>> +#include <linux/gpio.h>
>>> +
>>> +#include <dt-bindings/iio/adi,ad5592r.h>
>>> +
>>> +#include "ad5592r-base.h"
>>> +
>>> +#ifdef CONFIG_GPIOLIB
>>> +
>>> +static int ad5592r_gpio_get(struct gpio_chip *chip, unsigned offset)
>>> +{
>>> +    struct ad5592r_state *st = gpiochip_get_data(chip);
>>> +    int ret = 0;
>>> +    u8 val;
>>> +
>>> +    mutex_lock(&st->gpio_lock);
>>> +
>>> +    if (st->gpio_out & BIT(offset))
>>> +        val = st->gpio_val;
>>> +    else
>>> +        ret = st->ops->gpio_read(st, &val);
>>> +
>>> +    mutex_unlock(&st->gpio_lock);
>>> +
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    return !!(val & BIT(offset));
>>> +}
>>> +
>>> +static void ad5592r_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>>> +{
>>> +    struct ad5592r_state *st = gpiochip_get_data(chip);
>>> +
>>> +    mutex_lock(&st->gpio_lock);
>>> +
>>> +    if (value)
>>> +        st->gpio_val |= BIT(offset);
>>> +    else
>>> +        st->gpio_val &= ~BIT(offset);
>>> +
>>> +    st->ops->reg_write(st, AD5592R_REG_GPIO_SET, st->gpio_val);
>>> +
>>> +    mutex_unlock(&st->gpio_lock);
>>> +}
>>> +
>>> +static int ad5592r_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
>>> +{
>>> +    struct ad5592r_state *st = gpiochip_get_data(chip);
>>> +    int ret;
>>> +
>>> +    mutex_lock(&st->gpio_lock);
>>> +
>>> +    st->gpio_out &= ~BIT(offset);
>>> +    st->gpio_in |= BIT(offset);
>>> +
>>> +    ret = st->ops->reg_write(st, AD5592R_REG_GPIO_OUT_EN, st->gpio_out);
>>> +    if (ret < 0)
>>> +        goto err_unlock;
>>> +
>>> +    ret = st->ops->reg_write(st, AD5592R_REG_GPIO_IN_EN, st->gpio_in);
>>> +
>>> +err_unlock:
>>> +    mutex_unlock(&st->gpio_lock);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int ad5592r_gpio_direction_output(struct gpio_chip *chip,
>>> +                     unsigned offset, int value)
>>> +{
>>> +    struct ad5592r_state *st = gpiochip_get_data(chip);
>>> +    int ret;
>>> +
>>> +    mutex_lock(&st->gpio_lock);
>>> +
>>> +    if (value)
>>> +        st->gpio_val |= BIT(offset);
>>> +    else
>>> +        st->gpio_val &= ~BIT(offset);
>>> +
>>> +    st->gpio_in &= ~BIT(offset);
>>> +    st->gpio_out |= BIT(offset);
>>> +
>>> +    ret = st->ops->reg_write(st, AD5592R_REG_GPIO_SET, st->gpio_val);
>>> +    if (ret < 0)
>>> +        goto err_unlock;
>>> +
>>> +    ret = st->ops->reg_write(st, AD5592R_REG_GPIO_OUT_EN, st->gpio_out);
>>> +    if (ret < 0)
>>> +        goto err_unlock;
>>> +
>>> +    ret = st->ops->reg_write(st, AD5592R_REG_GPIO_IN_EN, st->gpio_in);
>>> +
>>> +err_unlock:
>>> +    mutex_unlock(&st->gpio_lock);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int ad5592r_gpio_request(struct gpio_chip *chip, unsigned offset)
>>> +{
>>> +    struct ad5592r_state *st = gpiochip_get_data(chip);
>>> +
>>> +    if (!(st->gpio_map & BIT(offset))) {
>>> +        dev_err(st->dev, "GPIO %d is reserved by alternate function\n",
>>> +            offset);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    if (offset >= chip->ngpio)
>>> +        return -EINVAL;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ad5592r_gpio_init(struct ad5592r_state *st)
>>> +{
>>> +    st->gpiochip.label = dev_name(st->dev);
>>> +    st->gpiochip.base = -1;
>>> +    st->gpiochip.ngpio = 8;
>>> +    st->gpiochip.parent = st->dev;
>>> +    st->gpiochip.can_sleep = true;
>>> +    st->gpiochip.direction_input = ad5592r_gpio_direction_input;
>>> +    st->gpiochip.direction_output = ad5592r_gpio_direction_output;
>>> +    st->gpiochip.get = ad5592r_gpio_get;
>>> +    st->gpiochip.set = ad5592r_gpio_set;
>>> +    st->gpiochip.request = ad5592r_gpio_request;
>>> +    st->gpiochip.owner = THIS_MODULE;
>>> +
>>> +    mutex_init(&st->gpio_lock);
>>> +
>>> +    return gpiochip_add_data(&st->gpiochip, st);
>>> +}
>>> +
>>> +static void ad5592r_gpio_cleanup(struct ad5592r_state *st)
>>> +{
>>> +    gpiochip_remove(&st->gpiochip);
>>> +}
>>> +#else
>>> +static int ad5592r_gpio_init(struct ad5592r_state *st) { return 0 };
>>> +static void ad5592r_gpio_cleanup(struct ad5592r_state *st) { };
>>> +#endif /* CONFIG_GPIOLIB */
>>> +
>>> +static int ad5592r_reset(struct ad5592r_state *st)
>>> +{
>>> +    struct gpio_desc *gpio;
>>> +    struct iio_dev *iio_dev = iio_priv_to_dev(st);
>>> +
>>> +    gpio = devm_gpiod_get_optional(st->dev, "reset", GPIOD_OUT_LOW);
>>> +    if (IS_ERR(gpio))
>>> +        return PTR_ERR(gpio);
>>> +
>>> +    if (gpio) {
>>> +        udelay(1);
>>> +        gpiod_set_value(gpio, 1);
>>> +    } else {
>>> +        mutex_lock(&iio_dev->mlock);
>>> +        st->ops->reg_write(st, AD5592R_REG_RESET, 0xdac);
>>> +        mutex_unlock(&iio_dev->mlock);
>>> +    }
>>> +
>>> +    udelay(250);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ad5592r_get_vref(struct ad5592r_state *st)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (st->reg) {
>>> +        ret = regulator_get_voltage(st->reg);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +
>>> +        return ret / 1000;
>>> +    } else {
>>> +        return 2500;
>>> +    }
>>> +}
>>> +
>>> +static int ad5592r_set_channel_modes(struct ad5592r_state *st)
>>> +{
>>> +    const struct ad5592r_rw_ops *ops = st->ops;
>>> +    int ret;
>>> +    unsigned i;
>>> +    struct iio_dev *iio_dev = iio_priv_to_dev(st);
>>> +    u8 pulldown = 0, open_drain = 0, tristate = 0,
>>> +       dac = 0, adc = 0;
>>> +    u16 read_back;
>>> +
>>> +    for (i = 0; i < st->num_channels; i++) {
>>> +        switch (st->channel_modes[i]) {
>>> +        case CH_MODE_DAC:
>>> +            dac |= BIT(i);
>>> +            break;
>>> +
>>> +        case CH_MODE_ADC:
>>> +            adc |= BIT(i);
>>> +            break;
>>> +
>>> +        case CH_MODE_DAC_AND_ADC:
>>> +            dac |= BIT(i);
>>> +            adc |= BIT(i);
>>> +            break;
>>> +
>>> +        case CH_MODE_UNUSED_PULL_DOWN:
>>> +            pulldown |= BIT(i);
>>> +            break;
>>> +
>>> +        case CH_MODE_UNUSED_OUT_TRISTATE:
>>> +            tristate |= BIT(i);
>>> +            break;
>>> +
>>> +        case CH_MODE_UNUSED_OUT_LOW:
>>> +            st->gpio_out |= BIT(i);
>>> +            break;
>>> +
>>> +        case CH_MODE_UNUSED_OUT_HIGH:
>>> +            st->gpio_out |= BIT(i);
>>> +            st->gpio_val |= BIT(i);
>>> +            break;
>>> +
>>> +        case CH_MODE_GPIO_OPEN_DRAIN:
>>> +            open_drain |= BIT(i);
>>> +
>>> +            /* fall-through */
>>> +
>>> +        case CH_MODE_GPIO:
>>> +            st->gpio_map |= BIT(i);
>>> +            st->gpio_in |= BIT(i); /* Default to input */
>>> +            break;
>>> +
>>> +        default:
>>> +            pulldown |= BIT(i);
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    mutex_lock(&iio_dev->mlock);
>>> +
>>> +    /* Pull down unused pins to GND */
>>> +    ret = ops->reg_write(st, AD5592R_REG_PULLDOWN, pulldown);
>>> +    if (ret)
>>> +        goto err_unlock;
>>> +
>>> +    ret = ops->reg_write(st, AD5592R_REG_TRISTATE, tristate);
>>> +    if (ret)
>>> +        goto err_unlock;
>>> +
>>> +    /* Configure pins that we use */
>>> +    ret = ops->reg_write(st, AD5592R_REG_DAC_EN, dac);
>>> +    if (ret)
>>> +        goto err_unlock;
>>> +
>>> +    ret = ops->reg_write(st, AD5592R_REG_ADC_EN, adc);
>>> +    if (ret)
>>> +        goto err_unlock;
>>> +
>>> +    ret = ops->reg_write(st, AD5592R_REG_OPEN_DRAIN, open_drain);
>>> +    if (ret)
>>> +        goto err_unlock;
>>> +
>>> +    ret = ops->reg_write(st, AD5592R_REG_GPIO_SET, st->gpio_val);
>>> +    if (ret)
>>> +        goto err_unlock;
>>> +
>>> +    ret = ops->reg_write(st, AD5592R_REG_GPIO_OUT_EN, st->gpio_out);
>>> +    if (ret)
>>> +        goto err_unlock;
>>> +
>>> +    ret = ops->reg_write(st, AD5592R_REG_GPIO_IN_EN, st->gpio_in);
>>> +    if (ret)
>>> +        goto err_unlock;
>>> +
>>> +    /* Verify that we can read back at least one register */
>>> +    ret = ops->reg_read(st, AD5592R_REG_ADC_EN, &read_back);
>>> +    if (!ret && (read_back & 0xff) != adc)
>>> +        ret = -EIO;
>>> +
>>> +err_unlock:
>>> +    mutex_unlock(&iio_dev->mlock);
>>> +    return ret;
>>> +}
>>> +
>>> +static int ad5592r_write_raw(struct iio_dev *iio_dev,
>>> +    struct iio_chan_spec const *chan, int val, int val2, long mask)
>>> +{
>>> +    struct ad5592r_state *st = iio_priv(iio_dev);
>>> +    int ret;
>>> +
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_RAW:
>>> +        if (val >= (1 << chan->scan_type.realbits) || val < 0)
>>> +            return -EINVAL;
>>> +
>>> +        /* Warn if we try to write to a ADC channel */
>>> +        WARN_ON(!chan->output);
>> Probably just wants to return an error rather than filling the logs.
>> People do silly things all time like that.
> 
> ok
> 
>>> +
>>> +        mutex_lock(&iio_dev->mlock);
>>> +        ret = st->ops->write_dac(st, chan->channel, val);
>>> +        if (!ret)
>>> +            st->cached_dac[chan->channel] = val;
>>> +        mutex_unlock(&iio_dev->mlock);
>>> +        return ret;
>>> +    case IIO_CHAN_INFO_SCALE:
>>> +        if (chan->type == IIO_VOLTAGE) {
>>> +            bool gain;
>>> +
>>> +            if (val == st->scale_avail[0][0] &&
>>> +                val2 == st->scale_avail[0][1])
>>> +                gain = false;
>>> +            else if (val == st->scale_avail[1][0] &&
>>> +                 val2 == st->scale_avail[1][1])
>>> +                gain = true;
>>> +            else
>>> +                return -EINVAL;
>>> +
>>> +            mutex_lock(&iio_dev->mlock);
>>> +
>>> +            ret = st->ops->reg_read(st, AD5592R_REG_CTRL,
>>> +                        &st->cached_gp_ctrl);
>>> +            if (ret < 0) {
>>> +                mutex_unlock(&iio_dev->mlock);
>>> +                return ret;
>>> +            }
>>> +
>>> +            if (chan->output) {
>>> +                if (gain)
>>> +                    st->cached_gp_ctrl |=
>>> +                        AD5592R_REG_CTRL_DAC_RANGE;
>>> +                else
>>> +                    st->cached_gp_ctrl &=
>>> +                        ~AD5592R_REG_CTRL_DAC_RANGE;
>>> +            } else {
>>> +                if (gain)
>>> +                    st->cached_gp_ctrl |=
>>> +                        AD5592R_REG_CTRL_ADC_RANGE;
>>> +                else
>>> +                    st->cached_gp_ctrl &=
>>> +                        ~AD5592R_REG_CTRL_ADC_RANGE;
>>> +            }
>>> +
>>> +            ret = st->ops->reg_write(st, AD5592R_REG_CTRL,
>>> +                         st->cached_gp_ctrl);
>>> +            mutex_unlock(&iio_dev->mlock);
>>> +            if (ret < 0)
>>> +                return ret;
>>> +
>>> +            return ret;
>>> +
>>> +        }
>>> +        break;
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ad5592r_read_raw(struct iio_dev *iio_dev,
>>> +               struct iio_chan_spec const *chan,
>>> +               int *val, int *val2, long m)
>>> +{
>>> +    struct ad5592r_state *st = iio_priv(iio_dev);
>>> +    u16 read_val;
>>> +    int ret;
>>> +
>>> +    mutex_lock(&iio_dev->mlock);
>> Move the lock into the switch statement as it doesn't
>> need to be held for a fair bit of the code below (such as the scale read
>> backs).
> 
> ok
> 
>>> +
>>> +    switch (m) {
>>> +    case IIO_CHAN_INFO_RAW:
>>> +
>>> +        if (!chan->output) {
>>> +            ret = st->ops->read_adc(st, chan->channel, &read_val);
>>> +            if (ret)
>>> +                goto unlock;
>>> +
>>> +            if ((read_val >> 12 & 0x7) != (chan->channel & 0x7)) {
>>> +                dev_err(st->dev, "Error while reading channel %u\n",
>>> +                        chan->channel);
>>> +                ret = -EIO;
>>> +                goto unlock;
>>> +            }
>>> +
>>> +            read_val &= GENMASK(11, 0);
>>> +
>>> +        } else {
>>> +            read_val = st->cached_dac[chan->channel];
>>> +        }
>>> +
>>> +        dev_dbg(st->dev, "Channel %u read: 0x%04hX\n",
>>> +                chan->channel, read_val);
>>> +
>>> +        *val = (int) read_val;
>>> +        ret = IIO_VAL_INT;
>>> +        break;
>>> +    case IIO_CHAN_INFO_SCALE:
>>> +
>>> +        *val = ad5592r_get_vref(st);
>>> +
>>> +        if (chan->type == IIO_TEMP) {
>>> +            s64 tmp = *val * (3767897513LL / 25LL);
>>> +            *val = div_s64_rem(tmp, 1000000000LL, val2);
>>> +
>>> +            ret = IIO_VAL_INT_PLUS_MICRO;
>>> +        } else {
>>> +            int mult;
>>> +
>>> +            if (chan->output)
>>> +                mult = !!(st->cached_gp_ctrl &
>>> +                    AD5592R_REG_CTRL_DAC_RANGE);
>>> +            else
>>> +                mult = !!(st->cached_gp_ctrl &
>>> +                    AD5592R_REG_CTRL_ADC_RANGE);
>>> +
>>> +            *val *= ++mult;
>>> +
>>> +            *val2 = chan->scan_type.realbits;
>>> +            ret = IIO_VAL_FRACTIONAL_LOG2;
>>> +        }
>>> +        break;
>>> +    case IIO_CHAN_INFO_OFFSET:
>>> +
>>> +        ret = ad5592r_get_vref(st);
>>> +
>>> +        if (st->cached_gp_ctrl & AD5592R_REG_CTRL_ADC_RANGE)
>>> +            *val = (-34365 * 25) / ret;
>>> +        else
>>> +            *val = (-75365 * 25) / ret;
>>> +        ret =  IIO_VAL_INT;
>>> +        break;
>>> +    default:
>>> +        ret = -EINVAL;
>>> +    }
>>> +
>>> +unlock:
>>> +    mutex_unlock(&iio_dev->mlock);
>>> +    return ret;
>>> +}
>>> +
>>> +static int ad5592r_write_raw_get_fmt(struct iio_dev *indio_dev,
>>> +                 struct iio_chan_spec const *chan, long mask)
>>> +{
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_SCALE:
>>> +        return IIO_VAL_INT_PLUS_NANO;
>>> +
>>> +    default:
>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>> +    }
>>> +
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static const struct iio_info ad5592r_info = {
>>> +    .read_raw = ad5592r_read_raw,
>>> +    .write_raw = ad5592r_write_raw,
>>> +    .write_raw_get_fmt = ad5592r_write_raw_get_fmt,
>>> +    .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +static ssize_t ad5592r_show_scale_available(struct iio_dev *iio_dev,
>>> +                       uintptr_t private,
>>> +                       const struct iio_chan_spec *chan,
>>> +                       char *buf)
>>> +{
>>> +    struct ad5592r_state *st = iio_priv(iio_dev);
>>> +
>>> +    return sprintf(buf, "%d.%09u %d.%09u\n",
>>> +        st->scale_avail[0][0], st->scale_avail[0][1],
>>> +        st->scale_avail[1][0], st->scale_avail[1][1]);
>>> +}
>>> +
>>> +static struct iio_chan_spec_ext_info ad5592r_ext_info[] = {
>>> +    {
>>> +     .name = "scale_available",
>>> +     .read = ad5592r_show_scale_available,
>>> +     .shared = true,
>>> +     },
>>> +    {},
>>> +};
>>> +
>>> +static void ad5592r_setup_channel(struct iio_dev *iio_dev,
>>> +        struct iio_chan_spec *chan, bool output, unsigned id)
>>> +{
>>> +    chan->type = IIO_VOLTAGE;
>>> +    chan->indexed = 1;
>>> +    chan->output = output;
>>> +    chan->channel = id;
>>> +    chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>>> +    chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
>>> +    chan->scan_type.sign = 'u';
>>> +    chan->scan_type.realbits = 12;
>>> +    chan->scan_type.storagebits = 16;
>>> +    chan->ext_info = ad5592r_ext_info;
>>> +}
>>> +
>>> +static int ad5592r_alloc_channels(struct ad5592r_state *st)
>>> +{
>>> +    unsigned i, curr_channel = 0,
>>> +         num_channels = st->num_channels;
>>> +    struct iio_dev *iio_dev = iio_priv_to_dev(st);
>>> +    struct iio_chan_spec *channels;
>>> +    int ret;
>>> +
>>> +    ret = device_property_read_u8_array(st->dev, "channel-modes",
>>> +            st->channel_modes, num_channels);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    channels = devm_kzalloc(st->dev,
>>> +            (1 + 2 * num_channels) * sizeof(*channels), GFP_KERNEL);
>>> +    if (!channels)
>>> +        return -ENOMEM;
>>> +
>>> +    for (i = 0; i < num_channels; i++) {
>>> +        switch (st->channel_modes[i]) {
>>> +        case CH_MODE_DAC:
>>> +            ad5592r_setup_channel(iio_dev, &channels[curr_channel],
>>> +                    true, i);
>>> +            curr_channel++;
>>> +            break;
>>> +
>>> +        case CH_MODE_ADC:
>>> +            ad5592r_setup_channel(iio_dev, &channels[curr_channel],
>>> +                    false, i);
>>> +            curr_channel++;
>>> +            break;
>>> +
>>> +        case CH_MODE_DAC_AND_ADC:
>>> +            ad5592r_setup_channel(iio_dev, &channels[curr_channel],
>>> +                    true, i);
>>> +            curr_channel++;
>>> +            ad5592r_setup_channel(iio_dev, &channels[curr_channel],
>>> +                    false, i);
>>> +            curr_channel++;
>>> +            break;
>>> +
>>> +        default:
>>> +            continue;
>>> +        }
>>> +    }
>>> +
>>> +    channels[curr_channel].type = IIO_TEMP;
>>> +    channels[curr_channel].channel = 8;
>>> +    channels[curr_channel].info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>> +                   BIT(IIO_CHAN_INFO_SCALE) |
>>> +                   BIT(IIO_CHAN_INFO_OFFSET);
>>> +    curr_channel++;
>>> +
>>> +    iio_dev->num_channels = curr_channel;
>>> +    iio_dev->channels = channels;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void ad5592r_init_scales(struct ad5592r_state *st, int vref_mV)
>>> +{
>>> +    s64 tmp = (s64)vref_mV * 1000000000LL >> 12;
>>> +
>>> +    st->scale_avail[0][0] =
>>> +        div_s64_rem(tmp, 1000000000LL, &st->scale_avail[0][1]);
>>> +    st->scale_avail[1][0] =
>>> +        div_s64_rem(tmp * 2, 1000000000LL, &st->scale_avail[1][1]);
>>> +}
>>> +
>>> +int ad5592r_probe(struct device *dev, const char *name,
>>> +        const struct ad5592r_rw_ops *ops)
>>> +{
>>> +    struct iio_dev *iio_dev;
>>> +    struct ad5592r_state *st;
>>> +    int ret;
>>> +
>>> +    iio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>>> +    if (!iio_dev)
>>> +        return -ENOMEM;
>>> +
>>> +    st = iio_priv(iio_dev);
>>> +    st->dev = dev;
>>> +    st->ops = ops;
>>> +    st->num_channels = 8;
>>> +    dev_set_drvdata(dev, iio_dev);
>>> +
>>> +    st->reg = devm_regulator_get_optional(dev, "vref");
>>> +    if (IS_ERR(st->reg)) {
>>> +        if ((PTR_ERR(st->reg) != -ENODEV) && dev->of_node)
>>> +            return PTR_ERR(st->reg);
>>> +
>>> +        st->reg = NULL;
>>> +    } else {
>>> +        ret = regulator_enable(st->reg);
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +
>>> +    iio_dev->dev.parent = dev;
>>> +    iio_dev->name = name;
>>> +    iio_dev->info = &ad5592r_info;
>>> +    iio_dev->modes = INDIO_DIRECT_MODE;
>>> +
>>> +    ad5592r_init_scales(st, ad5592r_get_vref(st));
>>> +
>>> +    ret = ad5592r_reset(st);
>>> +    if (ret)
>>> +        goto error_disable_reg;
>>> +
>>> +    ret = ops->reg_write(st, AD5592R_REG_PD,
>>> +             (st->reg == NULL) ? AD5592R_REG_PD_EN_REF : 0);
>>> +    if (ret)
>>> +        goto error_disable_reg;
>>> +
>>> +    ret = ad5592r_alloc_channels(st);
>>> +    if (ret)
>>> +        goto error_disable_reg;
>>> +
>>> +    ret = ad5592r_set_channel_modes(st);
>>> +    if (ret)
>>> +        goto error_disable_reg;
>>> +
>>> +    ret = devm_iio_device_register(dev, iio_dev);
>>> +    if (ret)
>>> +        goto error_disable_reg;
>> If you use a managed iio_device_register here it will only
>> be unregistered after the end of the remove function.  It certainly looks
>> like it really wants to be done at the start of the remove function
>> so as to remove the exposed interfaces (user and in kernel) before changing
>> the channel modes etc.
>>
>> Hence use the unmanaged version and iio_device_unregister in the remove.
> 
> I go for the unmanged version in both cases.
> 
>>> +
>>> +    return ad5592r_gpio_init(st);
>>> +
>>> +error_disable_reg:
>>> +    if (st->reg)
>>> +        regulator_disable(st->reg);
>>> +
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(ad5592r_probe);
>>> +
>>> +int ad5592r_remove(struct device *dev)
>>> +{
>>> +    struct iio_dev *iio_dev = dev_get_drvdata(dev);
>>> +    struct ad5592r_state *st = iio_priv(iio_dev);
>>> +    unsigned int i;
>>> +
>>> +    /* Reset all channels */
>>> +    for (i = 0; i < ARRAY_SIZE(st->channel_modes); i++)
>>> +        st->channel_modes[i] = CH_MODE_UNUSED_PULL_DOWN;
>>> +
>>> +    if (st->reg)
>>> +        regulator_disable(st->reg);
>>> +
>>> +    if (st->gpio_map)
>>> +        ad5592r_gpio_cleanup(st);
>>> +
>>> +    return ad5592r_set_channel_modes(st);
>>> +}
>>> +EXPORT_SYMBOL_GPL(ad5592r_remove);
>>> +
>>> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>");
>>> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/iio/dac/ad5592r-base.h b/drivers/iio/dac/ad5592r-base.h
>>> new file mode 100644
>>> index 0000000..162e833
>>> --- /dev/null
>>> +++ b/drivers/iio/dac/ad5592r-base.h
>>> @@ -0,0 +1,77 @@
>>> +/*
>>> + * AD5592R / AD5593R Digital <-> Analog converters driver
>>> + *
>>> + * Copyright 2015-2016 Analog Devices Inc.
>>> + * Author: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>> + *
>>> + * Licensed under the GPL-2.
>>> + */
>>> +
>>> +#ifndef __DRIVERS_IIO_DAC_AD5592R_BASE_H__
>>> +#define __DRIVERS_IIO_DAC_AD5592R_BASE_H__
>>> +
>>> +#include <linux/types.h>
>>> +#include <linux/cache.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/gpio/driver.h>
>>> +
>>> +struct device;
>>> +struct ad5592r_state;
>>> +
>>> +enum ad5592r_registers {
>>> +    AD5592R_REG_NOOP        = 0x0,
>>> +    AD5592R_REG_DAC_READBACK    = 0x1,
>>> +    AD5592R_REG_ADC_SEQ        = 0x2,
>>> +    AD5592R_REG_CTRL        = 0x3,
>>> +    AD5592R_REG_ADC_EN        = 0x4,
>>> +    AD5592R_REG_DAC_EN        = 0x5,
>>> +    AD5592R_REG_PULLDOWN        = 0x6,
>>> +    AD5592R_REG_LDAC        = 0x7,
>>> +    AD5592R_REG_GPIO_OUT_EN        = 0x8,
>>> +    AD5592R_REG_GPIO_SET        = 0x9,
>>> +    AD5592R_REG_GPIO_IN_EN        = 0xA,
>>> +    AD5592R_REG_PD            = 0xB,
>>> +    AD5592R_REG_OPEN_DRAIN        = 0xC,
>>> +    AD5592R_REG_TRISTATE        = 0xD,
>>> +    AD5592R_REG_RESET        = 0xF,
>>> +};
>>> +
>>> +#define AD5592R_REG_PD_EN_REF        BIT(9)
>>> +#define AD5592R_REG_CTRL_ADC_RANGE    BIT(5)
>>> +#define AD5592R_REG_CTRL_DAC_RANGE    BIT(4)
>>> +
>>> +struct ad5592r_rw_ops {
>>> +    int (*write_dac)(struct ad5592r_state *st, unsigned chan, u16 value);
>>> +    int (*read_adc)(struct ad5592r_state *st, unsigned chan, u16 *value);
>>> +    int (*reg_write)(struct ad5592r_state *st, u8 reg, u16 value);
>>> +    int (*reg_read)(struct ad5592r_state *st, u8 reg, u16 *value);
>>> +    int (*gpio_read)(struct ad5592r_state *st, u8 *value);
>>> +};
>>> +
>>> +struct ad5592r_state {
>>> +    struct device *dev;
>>> +    struct regulator *reg;
>>> +#ifdef CONFIG_GPIOLIB
>>> +    struct gpio_chip gpiochip;
>>> +    struct mutex gpio_lock;    /* Protect cached gpio_out, gpio_val, etc. */
>>> +#endif
>>> +    unsigned int num_channels;
>>> +    const struct ad5592r_rw_ops *ops;
>>> +    int scale_avail[2][2];
>>> +    u16 cached_dac[8];
>>> +    u16 cached_gp_ctrl;
>>> +    u8 channel_modes[8];
>>> +    u8 gpio_map;
>>> +    u8 gpio_out;
>>> +    u8 gpio_in;
>>> +    u8 gpio_val;
>>> +
>>> +    __be16 spi_msg ____cacheline_aligned;
>>> +    __be16 spi_msg_nop;
>>> +};
>>> +
>>> +int ad5592r_probe(struct device *dev, const char *name,
>>> +        const struct ad5592r_rw_ops *ops);
>>> +int ad5592r_remove(struct device *dev);
>>> +
>>> +#endif /* __DRIVERS_IIO_DAC_AD5592R_BASE_H__ */
>>> diff --git a/drivers/iio/dac/ad5592r.c b/drivers/iio/dac/ad5592r.c
>>> new file mode 100644
>>> index 0000000..0b235a2
>>> --- /dev/null
>>> +++ b/drivers/iio/dac/ad5592r.c
>>> @@ -0,0 +1,164 @@
>>> +/*
>>> + * AD5592R Digital <-> Analog converters driver
>>> + *
>>> + * Copyright 2015-2016 Analog Devices Inc.
>>> + * Author: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>> + *
>>> + * Licensed under the GPL-2.
>>> + */
>>> +
>>> +#include "ad5592r-base.h"
>>> +
>>> +#include <linux/bitops.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/spi/spi.h>
>>> +
>>> +#define AD5592R_GPIO_READBACK_EN    BIT(10)
>>> +#define AD5592R_LDAC_READBACK_EN    BIT(6)
>>> +
>>> +static int ad5592r_spi_wnop_r16(struct ad5592r_state *st, u16 *buf)
>>> +{
>>> +    struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>>> +    struct spi_transfer t = {
>>> +            .tx_buf    = &st->spi_msg_nop,
>>> +            .rx_buf    = buf,
>>> +            .len = 2
>>> +        };
>>> +
>>> +    st->spi_msg_nop = 0; /* NOP */
>>> +
>>> +    return spi_sync_transfer(spi, &t, 1);
>>> +}
>>> +
>>> +static int ad5592r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value)
>>> +{
>>> +    struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>>> +
>>> +    st->spi_msg = cpu_to_be16(BIT(15) | (chan << 12) | value);
>>> +
>>> +    return spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
>>> +}
>>> +
>>> +static int ad5592r_read_adc(struct ad5592r_state *st, unsigned chan, u16 *value)
>>> +{
>>> +    struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>>> +    int ret;
>>> +
>>> +    st->spi_msg = cpu_to_be16((AD5592R_REG_ADC_SEQ << 11) | BIT(chan));
>>> +
>>> +    ret = spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    /*
>>> +     * Invalid data:
>>> +     * See Figure 40. Single-Channel ADC Conversion Sequence
>>> +     */
>>> +    ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    *value = be16_to_cpu(st->spi_msg);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ad5592r_reg_write(struct ad5592r_state *st, u8 reg, u16 value)
>>> +{
>>> +    struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>>> +
>>> +    st->spi_msg = cpu_to_be16((reg << 11) | value);
>>> +
>>> +    return spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
>>> +}
>>> +
>>> +static int ad5592r_reg_read(struct ad5592r_state *st, u8 reg, u16 *value)
>>> +{
>>> +    struct spi_device *spi = container_of(st->dev, struct spi_device, dev);
>>> +    int ret;
>>> +
>>> +    st->spi_msg = cpu_to_be16((AD5592R_REG_LDAC << 11) |
>>> +                   AD5592R_LDAC_READBACK_EN | (reg << 2));
>>> +
>>> +    ret = spi_write(spi, &st->spi_msg, sizeof(st->spi_msg));
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    *value = be16_to_cpu(st->spi_msg);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ad5593r_gpio_read(struct ad5592r_state *st, u8 *value)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = ad5592r_reg_write(st, AD5592R_REG_GPIO_IN_EN,
>>> +                AD5592R_GPIO_READBACK_EN | st->gpio_in);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = ad5592r_spi_wnop_r16(st, &st->spi_msg);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    *value = (u8) be16_to_cpu(st->spi_msg);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct ad5592r_rw_ops ad5592r_rw_ops = {
>>> +    .write_dac = ad5592r_write_dac,
>>> +    .read_adc = ad5592r_read_adc,
>>> +    .reg_write = ad5592r_reg_write,
>>> +    .reg_read = ad5592r_reg_read,
>>> +    .gpio_read = ad5593r_gpio_read,
>>> +};
>>> +
>>> +static int ad5592r_spi_probe(struct spi_device *spi)
>>> +{
>>> +    const struct spi_device_id *id = spi_get_device_id(spi);
>>> +
>>> +    return ad5592r_probe(&spi->dev, id->name, &ad5592r_rw_ops);
>>> +}
>>> +
>>> +static int ad5592r_spi_remove(struct spi_device *spi)
>>> +{
>>> +    return ad5592r_remove(&spi->dev);
>>> +}
>>> +
>>> +static const struct spi_device_id ad5592r_spi_ids[] = {
>>> +    { .name = "ad5592r", },
>>> +    {}
>>> +};
>>> +MODULE_DEVICE_TABLE(spi, ad5592r_spi_ids);
>>> +
>>> +static const struct of_device_id ad5592r_of_match[] = {
>>> +    { .compatible = "adi,ad5592r", },
>>> +    {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ad5592r_of_match);
>>> +
>>> +static struct spi_driver ad5592r_spi_driver = {
>>> +    .driver = {
>>> +        .name = "ad5592r",
>>> +        .of_match_table = of_match_ptr(ad5592r_of_match),
>>> +    },
>>> +    .probe = ad5592r_spi_probe,
>>> +    .remove = ad5592r_spi_remove,
>>> +    .id_table = ad5592r_spi_ids,
>>> +};
>>> +module_spi_driver(ad5592r_spi_driver);
>>> +
>>> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>");
>>> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/iio/dac/ad5593r.c b/drivers/iio/dac/ad5593r.c
>>> new file mode 100644
>>> index 0000000..dca158a
>>> --- /dev/null
>>> +++ b/drivers/iio/dac/ad5593r.c
>>> @@ -0,0 +1,131 @@
>>> +/*
>>> + * AD5593R Digital <-> Analog converters driver
>>> + *
>>> + * Copyright 2015-2016 Analog Devices Inc.
>>> + * Author: Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>> + *
>>> + * Licensed under the GPL-2.
>>> + */
>>> +
>>> +#include "ad5592r-base.h"
>>> +
>>> +#include <linux/bitops.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +
>>> +#define AD5593R_MODE_CONF        (0 << 4)
>>> +#define AD5593R_MODE_DAC_WRITE        (1 << 4)
>>> +#define AD5593R_MODE_ADC_READBACK    (4 << 4)
>>> +#define AD5593R_MODE_DAC_READBACK    (5 << 4)
>>> +#define AD5593R_MODE_GPIO_READBACK    (6 << 4)
>>> +#define AD5593R_MODE_REG_READBACK    (7 << 4)
>>> +
>>> +static int ad5593r_write_dac(struct ad5592r_state *st, unsigned chan, u16 value)
>>> +{
>>> +    struct i2c_client *i2c = to_i2c_client(st->dev);
>>> +
>>> +    return i2c_smbus_write_word_swapped(i2c,
>>> +            AD5593R_MODE_DAC_WRITE | chan, value);
>>> +}
>>> +
>>> +static int ad5593r_read_adc(struct ad5592r_state *st, unsigned chan, u16 *value)
>>> +{
>>> +    struct i2c_client *i2c = to_i2c_client(st->dev);
>>> +    s32 val;
>>> +
>>> +    val = i2c_smbus_write_word_swapped(i2c,
>>> +            AD5593R_MODE_CONF | AD5592R_REG_ADC_SEQ, BIT(chan));
>>> +    if (val < 0)
>>> +        return (int) val;
>>> +
>>> +    val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_ADC_READBACK);
>>> +    if (val < 0)
>>> +        return (int) val;
>>> +
>>> +    *value = (u16) val;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ad5593r_reg_write(struct ad5592r_state *st, u8 reg, u16 value)
>>> +{
>>> +    struct i2c_client *i2c = to_i2c_client(st->dev);
>>> +
>>> +    return i2c_smbus_write_word_swapped(i2c,
>>> +            AD5593R_MODE_CONF | reg, value);
>>> +}
>>> +
>>> +static int ad5593r_reg_read(struct ad5592r_state *st, u8 reg, u16 *value)
>>> +{
>>> +    struct i2c_client *i2c = to_i2c_client(st->dev);
>>> +    s32 val;
>>> +
>>> +    val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_REG_READBACK | reg);
>>> +    if (val < 0)
>>> +        return (int) val;
>>> +
>>> +    *value = (u16) val;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ad5593r_gpio_read(struct ad5592r_state *st, u8 *value)
>>> +{
>>> +    struct i2c_client *i2c = to_i2c_client(st->dev);
>>> +    s32 val;
>>> +
>>> +    val = i2c_smbus_read_word_swapped(i2c, AD5593R_MODE_GPIO_READBACK);
>>> +    if (val < 0)
>>> +        return (int) val;
>>> +
>>> +    *value = (u8) val;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct ad5592r_rw_ops ad5593r_rw_ops = {
>>> +    .write_dac = ad5593r_write_dac,
>>> +    .read_adc = ad5593r_read_adc,
>>> +    .reg_write = ad5593r_reg_write,
>>> +    .reg_read = ad5593r_reg_read,
>>> +    .gpio_read = ad5593r_gpio_read,
>>> +};
>>> +
>>> +static int ad5593r_i2c_probe(struct i2c_client *i2c,
>>> +        const struct i2c_device_id *id)
>>> +{
>>> +    return ad5592r_probe(&i2c->dev, id->name, &ad5593r_rw_ops);
>>> +}
>>> +
>>> +static int ad5593r_i2c_remove(struct i2c_client *i2c)
>>> +{
>>> +    return ad5592r_remove(&i2c->dev);
>>> +}
>>> +
>>> +static const struct i2c_device_id ad5593r_i2c_ids[] = {
>>> +    { .name = "ad5593r", },
>>> +    {},
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, ad5593r_i2c_ids);
>>> +
>>> +static const struct of_device_id ad5593r_of_match[] = {
>>> +    { .compatible = "adi,ad5593r", },
>>> +    {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ad5593r_of_match);
>>> +
>>> +static struct i2c_driver ad5593r_driver = {
>>> +    .driver = {
>>> +        .name = "ad5593r",
>>> +        .of_match_table = of_match_ptr(ad5593r_of_match),
>>> +    },
>>> +    .probe = ad5593r_i2c_probe,
>>> +    .remove = ad5593r_i2c_remove,
>>> +    .id_table = ad5593r_i2c_ids,
>>> +};
>>> +module_i2c_driver(ad5593r_driver);
>>> +
>>> +MODULE_AUTHOR("Paul Cercueil <paul.cercueil-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>");
>>> +MODULE_DESCRIPTION("Analog Devices AD5592R multi-channel converters");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/include/dt-bindings/iio/adi,ad5592r.h b/include/dt-bindings/iio/adi,ad5592r.h
>>> new file mode 100644
>>> index 0000000..6bd519b
>>> --- /dev/null
>>> +++ b/include/dt-bindings/iio/adi,ad5592r.h
>>> @@ -0,0 +1,16 @@
>>> +
>>> +#ifndef _DT_BINDINGS_ADI_AD5592R_H
>>> +#define _DT_BINDINGS_ADI_AD5592R_H
>>> +
>>> +
>>> +#define CH_MODE_ADC            1
>>> +#define CH_MODE_DAC            2
>>> +#define CH_MODE_DAC_AND_ADC        3
>>> +#define CH_MODE_UNUSED_PULL_DOWN    4
>>> +#define CH_MODE_UNUSED_OUT_LOW        5
>>> +#define CH_MODE_UNUSED_OUT_HIGH        6
>>> +#define CH_MODE_UNUSED_OUT_TRISTATE    7
>>> +#define CH_MODE_GPIO            8
>>> +#define CH_MODE_GPIO_OPEN_DRAIN        9
>>> +
>>> +#endif /* _DT_BINDINGS_ADI_AD5592R_H */
>>>
>>
> 
> 

Re: [PATCH v2] iio: dac: Add support for the AD5592R/AD5593R ADCs/DACs

[Michael Hennerich]

On 03/05/2016 06:58 PM, Jonathan Cameron wrote:
> On 02/03/16 16:21, Michael Hennerich wrote:
>> On 02/27/2016 06:50 PM, Jonathan Cameron wrote:
>>> On 25/02/16 13:36, michael.hennerich@analog.com wrote:
>>>> From: Paul Cercueil <paul.cercueil@analog.com>
>>>>
>>>> This patch adds support for the AD5592R (spi) and AD5593R (i2c)
>>>> ADC/DAC devices.
>>>>
>>>> Signed-off-by: Paul Cercueil <paul.cercueil@analog.com>
>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>
>>> A few bits inline.
>>>
>>> I'll need a gpio review on this (looks fine to me but it does contain
>>> a gpiochip driver.) Not to mention the question of whether they will
>>> be happy with a gpio chip hiding in iio (rather than via an mfd with a
>>> separate driver - which feels like overkill here).
>>>
>>> The big question to my mind is whether we can take the view this won't
>>> be the last multipurpose chip we will see so do we need to sort the
>>> binding out to make it generic?  It'll be a bit of a pain for you
>>> but I think we can do it fairly easily.
>>> (either way I'll also need a device tree ack on this one!)
>>>
>>> So then we get into the question of the best way of doing the bindings.
>>> The gpio approach seems a little limiting for things as flexible as
>>> this but we should certainly be using their macros where relevant.
>>
>> Hi Jonathan,
>>
>> Thanks for the review.
>>
>> The problem is see is that using GPIOF_OPEN_DRAIN, will simulate OPEN DRAIN behaviour only, by configuring the device for input when outputing logic high.
>>
> Is it actually documented as such anywhere? (that's hideous!)


In fact it is -
www.kernel.org/doc/Documentation/gpio/gpio-legacy.txt

"When setting the flag as GPIOF_OPEN_DRAIN then it will assume that pins is
open drain type. Such pins will not be driven to 1 in output mode. It is
require to connect pull-up on such pins. By enabling this flag, gpio lib 
will
make the direction to input when it is asked to set value of 1 in output 
mode
to make the pin HIGH. The pin is make to LOW by driving value 0 in 
output mode."

And that's exactly what the gpiolib code does.

-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif

Re: [PATCH v2] iio: dac: Add support for the AD5592R/AD5593R ADCs/DACs

[Linus Walleij]

On Sun, Feb 28, 2016 at 12:50 AM, Jonathan Cameron <jic23@kernel.org> wrote:

>> +Analog Devices AD5592R/AD5593R DAC/ADC device driver
>> +
>> +Required properties for the AD5592R:
>> +     - compatible: Must be "adi,ad5592r"
>> +     - reg: SPI chip select number for the device
>> +     - spi-max-frequency: Max SPI frequency to use (< 30000000)
>> +     - spi-cpol: The AD5592R requires inverse clock polarity (CPOL) mode

If this should be a gpiochip, this shall also be reflected in the
device tree bindings and the example in the bindings by
stating gpio-controller; and #gpio-cells, referring to
the binding in gpio/gpio.txt so consumers can pick a GPIO
from this device.

I haven't seen the original patch, please mail the next version
to me so I can take a look.

>> +config AD5592R_BASE
>> +     tristate
>> +
>> +config AD5592R
>> +     tristate "Analog Devices AD5592R ADC/DAC driver"
>> +     depends on SPI_MASTER
>> +     depends on OF
>> +     select AD5592R_BASE
>> +     help
>> +       Say yes here to build support for Analog Devices AD5592R
>> +       Digital to Analog / Analog to Digital Converter.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called ad5592r.
>> +
>> +config AD5593R
>> +     tristate "Analog Devices AD5593R ADC/DAC driver"
>> +     depends on I2C
>> +     depends on OF
>> +     select AD5592R_BASE
>> +     help
>> +       Say yes here to build support for Analog Devices AD5593R
>> +       Digital to Analog / Analog to Digital Converter.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called ad5593r.
>> +

I guess something here should select GPIOLIB
and depend of OF_GPIO

>> +#ifdef CONFIG_GPIOLIB

Naaaaah really? Just select GPIOLIB and get rid of ifdeffery.
It's cool to have gpios available.

>> +static int ad5592r_gpio_request(struct gpio_chip *chip, unsigned offset)
>> +{
>> +     struct ad5592r_state *st = gpiochip_get_data(chip);
>> +
>> +     if (!(st->gpio_map & BIT(offset))) {
>> +             dev_err(st->dev, "GPIO %d is reserved by alternate function\n",
>> +                     offset);

This gpio_map looks a bit like pin control.

It might be overkill to use all of the pinctrl subsystem,
we have circumvented it in other places.

>> +             return -ENODEV;
>> +     }
>> +
>> +     if (offset >= chip->ngpio)
>> +             return -EINVAL;

gpiolib already guards against this I think.

>> +static int ad5592r_gpio_init(struct ad5592r_state *st)
>> +{
>> +     st->gpiochip.label = dev_name(st->dev);
>> +     st->gpiochip.base = -1;
>> +     st->gpiochip.ngpio = 8;
>> +     st->gpiochip.parent = st->dev;
>> +     st->gpiochip.can_sleep = true;
>> +     st->gpiochip.direction_input = ad5592r_gpio_direction_input;
>> +     st->gpiochip.direction_output = ad5592r_gpio_direction_output;
>> +     st->gpiochip.get = ad5592r_gpio_get;
>> +     st->gpiochip.set = ad5592r_gpio_set;
>> +     st->gpiochip.request = ad5592r_gpio_request;
>> +     st->gpiochip.owner = THIS_MODULE;
>> +
>> +     mutex_init(&st->gpio_lock);
>> +
>> +     return gpiochip_add_data(&st->gpiochip, st);

The gpiolib should be fine with the of_node from the parent so
looks fine.

>> +             mutex_lock(&iio_dev->mlock);
>> +             st->ops->reg_write(st, AD5592R_REG_RESET, 0xdac);
>> +             mutex_unlock(&iio_dev->mlock);

What's that? (0xdac)? Clever magic?

>> +             case CH_MODE_DAC_AND_ADC:
>> +                     dac |= BIT(i);
>> +                     adc |= BIT(i);
>> +                     break;
>> +
>> +             case CH_MODE_UNUSED_PULL_DOWN:
>> +                     pulldown |= BIT(i);
>> +                     break;
>> +
>> +             case CH_MODE_UNUSED_OUT_TRISTATE:
>> +                     tristate |= BIT(i);
>> +                     break;
>> +
>> +             case CH_MODE_UNUSED_OUT_LOW:
>> +                     st->gpio_out |= BIT(i);
>> +                     break;
>> +
>> +             case CH_MODE_UNUSED_OUT_HIGH:
>> +                     st->gpio_out |= BIT(i);
>> +                     st->gpio_val |= BIT(i);
>> +                     break;
>> +
>> +             case CH_MODE_GPIO_OPEN_DRAIN:
>> +                     open_drain |= BIT(i);

Deja-vu with include/linux/pinctrl/pinconf-generic.h

We call tristate "bias high impedance".

Yours,
Linus Walleij

Re: [PATCH v2] iio: dac: Add support for the AD5592R/AD5593R ADCs/DACs

[Michael Hennerich]

On 03/11/2016 05:28 PM, Linus Walleij wrote:
> On Sun, Feb 28, 2016 at 12:50 AM, Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
>>> +Analog Devices AD5592R/AD5593R DAC/ADC device driver
>>> +
>>> +Required properties for the AD5592R:
>>> +     - compatible: Must be "adi,ad5592r"
>>> +     - reg: SPI chip select number for the device
>>> +     - spi-max-frequency: Max SPI frequency to use (< 30000000)
>>> +     - spi-cpol: The AD5592R requires inverse clock polarity (CPOL) mode
>
> If this should be a gpiochip, this shall also be reflected in the
> device tree bindings and the example in the bindings by
> stating gpio-controller; and #gpio-cells, referring to
> the binding in gpio/gpio.txt so consumers can pick a GPIO
> from this device.

Hi Linus,

Thanks for the feedback.

- Will add this to the documentation.


>
> I haven't seen the original patch, please mail the next version
> to me so I can take a look.
>
>>> +config AD5592R_BASE
>>> +     tristate
>>> +
>>> +config AD5592R
>>> +     tristate "Analog Devices AD5592R ADC/DAC driver"
>>> +     depends on SPI_MASTER
>>> +     depends on OF
>>> +     select AD5592R_BASE
>>> +     help
>>> +       Say yes here to build support for Analog Devices AD5592R
>>> +       Digital to Analog / Analog to Digital Converter.
>>> +
>>> +       To compile this driver as a module, choose M here: the
>>> +       module will be called ad5592r.
>>> +
>>> +config AD5593R
>>> +     tristate "Analog Devices AD5593R ADC/DAC driver"
>>> +     depends on I2C
>>> +     depends on OF
>>> +     select AD5592R_BASE
>>> +     help
>>> +       Say yes here to build support for Analog Devices AD5593R
>>> +       Digital to Analog / Analog to Digital Converter.
>>> +
>>> +       To compile this driver as a module, choose M here: the
>>> +       module will be called ad5593r.
>>> +
>
> I guess something here should select GPIOLIB
> and depend of OF_GPIO


That reminds me - On my dev tree I removed to the depends on OF from 
kconfig. But I haven't rebased this commit into my for_upstream branch.

I tried to avoid any OF dependency in the driver by using the 
linux/property API.

I'll add select GPIOLIB - and gpio/Kconfig will select OF_GPIO if OF.


>
>>> +#ifdef CONFIG_GPIOLIB
>
> Naaaaah really? Just select GPIOLIB and get rid of ifdeffery.
> It's cool to have gpios available.

Convinced me.

>
>>> +static int ad5592r_gpio_request(struct gpio_chip *chip, unsigned offset)
>>> +{
>>> +     struct ad5592r_state *st = gpiochip_get_data(chip);
>>> +
>>> +     if (!(st->gpio_map & BIT(offset))) {
>>> +             dev_err(st->dev, "GPIO %d is reserved by alternate function\n",
>>> +                     offset);
>
> This gpio_map looks a bit like pin control.
>
> It might be overkill to use all of the pinctrl subsystem,
> we have circumvented it in other places.

I think it's overkill too.

>
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     if (offset >= chip->ngpio)
>>> +             return -EINVAL;
>
> gpiolib already guards against this I think.

Then I drop it

>
>>> +static int ad5592r_gpio_init(struct ad5592r_state *st)
>>> +{
>>> +     st->gpiochip.label = dev_name(st->dev);
>>> +     st->gpiochip.base = -1;
>>> +     st->gpiochip.ngpio = 8;
>>> +     st->gpiochip.parent = st->dev;
>>> +     st->gpiochip.can_sleep = true;
>>> +     st->gpiochip.direction_input = ad5592r_gpio_direction_input;
>>> +     st->gpiochip.direction_output = ad5592r_gpio_direction_output;
>>> +     st->gpiochip.get = ad5592r_gpio_get;
>>> +     st->gpiochip.set = ad5592r_gpio_set;
>>> +     st->gpiochip.request = ad5592r_gpio_request;
>>> +     st->gpiochip.owner = THIS_MODULE;
>>> +
>>> +     mutex_init(&st->gpio_lock);
>>> +
>>> +     return gpiochip_add_data(&st->gpiochip, st);
>
> The gpiolib should be fine with the of_node from the parent so
> looks fine.
>
>>> +             mutex_lock(&iio_dev->mlock);
>>> +             st->ops->reg_write(st, AD5592R_REG_RESET, 0xdac);
>>> +             mutex_unlock(&iio_dev->mlock);
>
> What's that? (0xdac)? Clever magic?

Clever magic - to avoid accidental resets.

>
>>> +             case CH_MODE_DAC_AND_ADC:
>>> +                     dac |= BIT(i);
>>> +                     adc |= BIT(i);
>>> +                     break;
>>> +
>>> +             case CH_MODE_UNUSED_PULL_DOWN:
>>> +                     pulldown |= BIT(i);
>>> +                     break;
>>> +
>>> +             case CH_MODE_UNUSED_OUT_TRISTATE:
>>> +                     tristate |= BIT(i);
>>> +                     break;
>>> +
>>> +             case CH_MODE_UNUSED_OUT_LOW:
>>> +                     st->gpio_out |= BIT(i);
>>> +                     break;
>>> +
>>> +             case CH_MODE_UNUSED_OUT_HIGH:
>>> +                     st->gpio_out |= BIT(i);
>>> +                     st->gpio_val |= BIT(i);
>>> +                     break;
>>> +
>>> +             case CH_MODE_GPIO_OPEN_DRAIN:
>>> +                     open_drain |= BIT(i);
>
> Deja-vu with include/linux/pinctrl/pinconf-generic.h
>
> We call tristate "bias high impedance".
>
> Yours,
> Linus Walleij
>


-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif

Re: [PATCH v2] iio: dac: Add support for the AD5592R/AD5593R ADCs/DACs

[Linus Walleij]

On Mon, Mar 21, 2016 at 3:07 PM, Michael Hennerich
<michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org> wrote:
> On 03/11/2016 05:28 PM, Linus Walleij wrote:

>>>> +             mutex_lock(&iio_dev->mlock);
>>>> +             st->ops->reg_write(st, AD5592R_REG_RESET, 0xdac);
>>>> +             mutex_unlock(&iio_dev->mlock);
>>
>> What's that? (0xdac)? Clever magic?
>
> Clever magic - to avoid accidental resets.

I see. Put in a small comment like:

/* Writing this magic value resets the device */

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html