[RFC 1/2] iio: core: added support for a single output that supports both current and voltage

[RFC 1/2] iio: core: added support for a single output that supports both current and voltage

[Sean Nyekjaer]

Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
---
 drivers/iio/industrialio-core.c | 2 ++
 include/linux/iio/iio.h         | 1 +
 include/uapi/linux/iio/types.h  | 1 +
 3 files changed, 4 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index fd01f34..5e1beea 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -77,6 +77,7 @@ static const char * const iio_chan_type_name_spec[] = {
 	[IIO_VELOCITY] = "velocity",
 	[IIO_CONCENTRATION] = "concentration",
 	[IIO_RESISTANCE] = "resistance",
+	[IIO_DUAL_VOLTCUR] = "voltcur",
 };
 
 static const char * const iio_modifier_names[] = {
@@ -146,6 +147,7 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_DEBOUNCE_TIME] = "debounce_time",
 	[IIO_CHAN_INFO_CALIBEMISSIVITY] = "calibemissivity",
 	[IIO_CHAN_INFO_OVERSAMPLING_RATIO] = "oversampling_ratio",
+	[IIO_CHAN_INFO_MODE] = "mode",
 };
 
 /**
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index b589411..ef4c7cd9 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -46,6 +46,7 @@ enum iio_chan_info_enum {
 	IIO_CHAN_INFO_DEBOUNCE_TIME,
 	IIO_CHAN_INFO_CALIBEMISSIVITY,
 	IIO_CHAN_INFO_OVERSAMPLING_RATIO,
+	IIO_CHAN_INFO_MODE,
 };
 
 enum iio_shared_by {
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 7c63bd6..81e6a2a 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -37,6 +37,7 @@ enum iio_chan_type {
 	IIO_VELOCITY,
 	IIO_CONCENTRATION,
 	IIO_RESISTANCE,
+	IIO_DUAL_VOLTCUR,
 };
 
 enum iio_modifier {
-- 
2.7.0

[RFC 2/2] iio: ad5755: added support for switching between voltage and current output

[Sean Nyekjaer]

DAC ad5755 have both support for voltage and current output, before the driver
only had support for switching modes at compile time. Not very smart...

This patch adds support for switching modes from userspace.

Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
---

This patch is not done yet :-) I would like to get some feedback of my work.
I have tested this patch with an ad5755 and it works.

So if you have any ideas on how I should progress please give me some feedback.

 drivers/iio/dac/ad5755.c | 405 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 319 insertions(+), 86 deletions(-)

diff --git a/drivers/iio/dac/ad5755.c b/drivers/iio/dac/ad5755.c
index bfb350a..6e7ab3f 100644
--- a/drivers/iio/dac/ad5755.c
+++ b/drivers/iio/dac/ad5755.c
@@ -63,6 +63,9 @@
 #define AD5755_SLEW_RATE_SHIFT			3
 #define AD5755_SLEW_ENABLE			BIT(12)
 
+#define AD5755_VOLTAGE_MODE			0
+#define AD5755_CURRENT_MODE			1
+
 /**
  * struct ad5755_chip_info - chip specific information
  * @channel_template:	channel specification
@@ -91,6 +94,8 @@ struct ad5755_state {
 	unsigned int			ctrl[AD5755_NUM_CHANNELS];
 	struct iio_chan_spec		channels[AD5755_NUM_CHANNELS];
 
+	struct ad5755_platform_data *pdata;
+
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache lines.
@@ -109,6 +114,163 @@ enum ad5755_type {
 	ID_AD5737,
 };
 
+struct ad5755_ranges {
+	enum ad5755_mode range;
+	unsigned int scale;
+	int offset;
+};
+
+static const struct ad5755_ranges ad5755_range_def[] = {
+	{
+		.range = AD5755_MODE_VOLTAGE_0V_5V,
+		.scale = 76293945,
+		.offset = 0,
+	}, {
+		.range = AD5755_MODE_VOLTAGE_0V_10V,
+		.scale = 152587890,
+		.offset = 0,
+	}, {
+		.range = AD5755_MODE_VOLTAGE_PLUSMINUS_5V,
+		.scale = 152587890,
+		.offset = -(1 << (16 /*REALBITS*/ - 1)),
+	}, {
+		.range = AD5755_MODE_VOLTAGE_PLUSMINUS_10V,
+		.scale = 305175781,
+		.offset = -(1 << (16 /*REALBITS*/ - 1)),
+	}, {
+		.range = AD5755_MODE_CURRENT_4mA_20mA,
+		.scale = 244140,
+		.offset = 16384,
+	}, {
+		.range = AD5755_MODE_CURRENT_0mA_20mA,
+		.scale = 305175,
+		.offset = 0,
+	}, {
+		.range = AD5755_MODE_CURRENT_0mA_24mA,
+		.scale = 366210,
+		.offset = 0,
+	}
+};
+
+static inline enum ad5755_mode ad5755_get_chan_mode(struct ad5755_state *st,
+						    const struct iio_chan_spec
+						    *chan)
+{
+	return st->ctrl[chan->channel] & 7;
+}
+
+static inline int ad5755_get_chan_scale(struct ad5755_state *st,
+					const struct iio_chan_spec *chan)
+{
+	return ad5755_range_def[ad5755_get_chan_mode(st, chan)].scale;
+}
+
+static inline int ad5755_get_chan_offset(struct ad5755_state *st,
+					 const struct iio_chan_spec *chan)
+{
+	return ad5755_range_def[ad5755_get_chan_mode(st, chan)].offset;
+}
+
+static bool ad5755_is_voltage_mode(enum ad5755_mode mode)
+{
+	switch (mode) {
+	case AD5755_MODE_VOLTAGE_0V_5V:
+	case AD5755_MODE_VOLTAGE_0V_10V:
+	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
+	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static ssize_t ad5755_show_voltcur_modes(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return sprintf(buf, "voltage: 0\ncurrent: 1\n");
+}
+
+static ssize_t ad5755_scales(char *buf, bool voltage_mode)
+{
+	int i, len = 0;
+
+	for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
+		if (ad5755_is_voltage_mode(i) != voltage_mode)
+			continue;
+		len += sprintf(buf + len, "0.%09u ", ad5755_range_def[i].scale);
+	}
+	len += sprintf(buf + len, "\n");
+
+	return len;
+}
+
+static ssize_t ad5755_offset(char *buf, bool voltage_mode)
+{
+	int i, len = 0;
+
+	for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
+		if (ad5755_is_voltage_mode(i) != voltage_mode)
+			continue;
+		len += sprintf(buf + len, "%d ", ad5755_range_def[i].offset);
+	}
+	len += sprintf(buf + len, "\n");
+
+	return len;
+}
+
+static ssize_t ad5755_show_voltage_scales(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	return ad5755_scales(buf, true);
+}
+
+static ssize_t ad5755_show_voltage_offset(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	return ad5755_offset(buf, true);
+}
+
+static ssize_t ad5755_show_current_scales(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	return ad5755_scales(buf, false);
+}
+
+static ssize_t ad5755_show_current_offset(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	return ad5755_offset(buf, false);
+}
+
+static IIO_DEVICE_ATTR(out_voltcur_modes_available, S_IRUGO,
+		       ad5755_show_voltcur_modes, NULL, 0);
+static IIO_DEVICE_ATTR(out_voltage_scale_available, S_IRUGO,
+		       ad5755_show_voltage_scales, NULL, 0);
+static IIO_DEVICE_ATTR(out_voltage_offset_available, S_IRUGO,
+		       ad5755_show_voltage_offset, NULL, 0);
+static IIO_DEVICE_ATTR(out_current_scale_available, S_IRUGO,
+		       ad5755_show_current_scales, NULL, 0);
+static IIO_DEVICE_ATTR(out_current_offset_available, S_IRUGO,
+		       ad5755_show_current_offset, NULL, 0);
+
+static struct attribute *ad5755_attributes[] = {
+	&iio_dev_attr_out_voltcur_modes_available.dev_attr.attr,
+	&iio_dev_attr_out_voltage_scale_available.dev_attr.attr,
+	&iio_dev_attr_out_voltage_offset_available.dev_attr.attr,
+	&iio_dev_attr_out_current_scale_available.dev_attr.attr,
+	&iio_dev_attr_out_current_offset_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ad5755_attribute_group = {
+	.attrs = ad5755_attributes,
+};
+
 static int ad5755_write_unlocked(struct iio_dev *indio_dev,
 	unsigned int reg, unsigned int val)
 {
@@ -226,31 +388,54 @@ out_unlock:
 	return 0;
 }
 
-static const int ad5755_min_max_table[][2] = {
-	[AD5755_MODE_VOLTAGE_0V_5V] = { 0, 5000 },
-	[AD5755_MODE_VOLTAGE_0V_10V] = { 0, 10000 },
-	[AD5755_MODE_VOLTAGE_PLUSMINUS_5V] = { -5000, 5000 },
-	[AD5755_MODE_VOLTAGE_PLUSMINUS_10V] = { -10000, 10000 },
-	[AD5755_MODE_CURRENT_4mA_20mA] = { 4, 20 },
-	[AD5755_MODE_CURRENT_0mA_20mA] = { 0, 20 },
-	[AD5755_MODE_CURRENT_0mA_24mA] = { 0, 24 },
-};
+static bool ad5755_is_valid_mode(struct ad5755_state *st, enum ad5755_mode mode)
+{
+	switch (mode) {
+	case AD5755_MODE_VOLTAGE_0V_5V:
+	case AD5755_MODE_VOLTAGE_0V_10V:
+	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
+	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
+		return st->chip_info->has_voltage_out;
+	case AD5755_MODE_CURRENT_4mA_20mA:
+	case AD5755_MODE_CURRENT_0mA_20mA:
+	case AD5755_MODE_CURRENT_0mA_24mA:
+		return true;
+	default:
+		return false;
+	}
+}
 
-static void ad5755_get_min_max(struct ad5755_state *st,
-	struct iio_chan_spec const *chan, int *min, int *max)
+static int ad5755_clear_dac(struct iio_dev *indio_dev, int channel)
 {
-	enum ad5755_mode mode = st->ctrl[chan->channel] & 7;
-	*min = ad5755_min_max_table[mode][0];
-	*max = ad5755_min_max_table[mode][1];
+	int i = channel;
+	int ret;
+
+	ret = ad5755_update_dac_ctrl(indio_dev, i, 0, UINT_MAX);
+	return ret;
+
 }
 
-static inline int ad5755_get_offset(struct ad5755_state *st,
-	struct iio_chan_spec const *chan)
+static int ad5755_setup_dac(struct iio_dev *indio_dev, int channel)
 {
-	int min, max;
+	int i = channel;
+	int ret;
+	struct ad5755_state *st = iio_priv(indio_dev);
+	unsigned int val;
+	struct ad5755_platform_data *pdata = st->pdata;
+
+	if (!ad5755_is_valid_mode(st, pdata->dac[i].mode))
+		return -EINVAL;
+
+	val = 0;
+	if (!pdata->dac[i].ext_current_sense_resistor)
+		val |= AD5755_DAC_INT_CURRENT_SENSE_RESISTOR;
+	if (pdata->dac[i].enable_voltage_overrange)
+		val |= AD5755_DAC_VOLTAGE_OVERRANGE_EN;
+	val |= pdata->dac[i].mode;
+
+	ret = ad5755_update_dac_ctrl(indio_dev, i, val, 0);
+	return ret;
 
-	ad5755_get_min_max(st, chan, &min, &max);
-	return (min * (1 << chan->scan_type.realbits)) / (max - min);
 }
 
 static int ad5755_chan_reg_info(struct ad5755_state *st,
@@ -294,18 +479,25 @@ static int ad5755_read_raw(struct iio_dev *indio_dev,
 {
 	struct ad5755_state *st = iio_priv(indio_dev);
 	unsigned int reg, shift, offset;
-	int min, max;
 	int ret;
 
 	switch (info) {
 	case IIO_CHAN_INFO_SCALE:
-		ad5755_get_min_max(st, chan, &min, &max);
-		*val = max - min;
-		*val2 = chan->scan_type.realbits;
-		return IIO_VAL_FRACTIONAL_LOG2;
+		*val = 0;
+		*val2 = ad5755_get_chan_scale(st, chan);
+		return IIO_VAL_INT_PLUS_NANO;
 	case IIO_CHAN_INFO_OFFSET:
-		*val = ad5755_get_offset(st, chan);
+		*val = ad5755_get_chan_offset(st, chan);
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_MODE:
+		if (ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode)) {
+			*val = AD5755_VOLTAGE_MODE;
+			return IIO_VAL_INT;
+		} else {
+			*val = AD5755_CURRENT_MODE;
+			return IIO_VAL_INT;
+		}
+		break;
 	default:
 		ret = ad5755_chan_reg_info(st, chan, info, false,
 						&reg, &shift, &offset);
@@ -325,24 +517,100 @@ static int ad5755_read_raw(struct iio_dev *indio_dev,
 }
 
 static int ad5755_write_raw(struct iio_dev *indio_dev,
-	const struct iio_chan_spec *chan, int val, int val2, long info)
+			    const struct iio_chan_spec *chan, int val, int val2,
+			    long info)
 {
 	struct ad5755_state *st = iio_priv(indio_dev);
-	unsigned int shift, reg, offset;
-	int ret;
+	unsigned int shift, reg;
+	bool is_voltage;
+	int offset;
+	unsigned int scale;
+	int ret, i;
 
-	ret = ad5755_chan_reg_info(st, chan, info, true,
-					&reg, &shift, &offset);
-	if (ret)
-		return ret;
-
-	val <<= shift;
-	val += offset;
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+	case IIO_CHAN_INFO_OFFSET:
+	case IIO_CHAN_INFO_MODE:
+		if (!(bool) (st->pwr_down & (1 << chan->channel))) {
+			printk
+			    ("POWER DOWN off - Power down before shifting modes\n");
+			return -EPERM;
+		}
+	}
 
-	if (val < 0 || val > 0xffff)
+	switch (info) {
+	case IIO_CHAN_INFO_SCALE:
+		is_voltage =
+		    ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode);
+		offset =
+		    ad5755_range_def[st->pdata->dac[chan->address].mode].offset;
+		/* is new scale allowed */
+		for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
+			if (val2 == ad5755_range_def[i].scale &&
+			    offset == ad5755_range_def[i].offset &&
+			    is_voltage == ad5755_is_voltage_mode(i)) {
+				st->pdata->dac[chan->address].mode = i;
+				ad5755_clear_dac(indio_dev, chan->address);
+				return ad5755_setup_dac(indio_dev, chan->address);
+			}
+		}
 		return -EINVAL;
+	case IIO_CHAN_INFO_OFFSET:
+		is_voltage =
+		    ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode);
+		scale =
+		    ad5755_range_def[st->pdata->dac[chan->address].mode].scale;
+		/* is new offset allowed */
+		for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
+			if (val == ad5755_range_def[i].offset &&
+			    scale == ad5755_range_def[i].scale &&
+			    is_voltage == ad5755_is_voltage_mode(i)) {
+				st->pdata->dac[chan->address].mode = i;
+				ad5755_clear_dac(indio_dev, chan->address);
+				return ad5755_setup_dac(indio_dev, chan->address);
+			}
+		}
+		return -EINVAL;
+	case IIO_CHAN_INFO_MODE:
+		if (val2 != 0)
+			return -EINVAL;
+		if (val == AD5755_VOLTAGE_MODE)
+			st->pdata->dac[chan->address].mode = AD5755_MODE_VOLTAGE_0V_5V;
+		else
+			st->pdata->dac[chan->address].mode = AD5755_MODE_CURRENT_0mA_20mA;
+		ad5755_clear_dac(indio_dev, chan->address);
+
+		return ad5755_setup_dac(indio_dev, chan->address);
+		break;
+	default:
+		ret = ad5755_chan_reg_info(st, chan, info, true,
+					   &reg, &shift, &offset);
+		if (ret)
+			return ret;
+
+		val <<= shift;
+		val += offset;
+
+		if (val < 0 || val > 0xffff)
+			return -EINVAL;
+
+		return ad5755_write(indio_dev, reg, val);
+	}
+
+	return -EINVAL;
+}
+
+static int ad5755_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;
+	}
 
-	return ad5755_write(indio_dev, reg, val);
+	return -EINVAL;
 }
 
 static ssize_t ad5755_read_powerdown(struct iio_dev *indio_dev, uintptr_t priv,
@@ -371,6 +639,8 @@ static ssize_t ad5755_write_powerdown(struct iio_dev *indio_dev, uintptr_t priv,
 static const struct iio_info ad5755_info = {
 	.read_raw = ad5755_read_raw,
 	.write_raw = ad5755_write_raw,
+	.write_raw_get_fmt = &ad5755_write_raw_get_fmt,
+	.attrs = &ad5755_attribute_group,
 	.driver_module = THIS_MODULE,
 };
 
@@ -391,7 +661,8 @@ static const struct iio_chan_spec_ext_info ad5755_ext_info[] = {
 		BIT(IIO_CHAN_INFO_SCALE) |			\
 		BIT(IIO_CHAN_INFO_OFFSET) |			\
 		BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
-		BIT(IIO_CHAN_INFO_CALIBBIAS),			\
+		BIT(IIO_CHAN_INFO_CALIBBIAS)  |			\
+		BIT(IIO_CHAN_INFO_MODE),			\
 	.scan_type = {						\
 		.sign = 'u',					\
 		.realbits = (_bits),				\
@@ -424,27 +695,9 @@ static const struct ad5755_chip_info ad5755_chip_info_tbl[] = {
 	},
 };
 
-static bool ad5755_is_valid_mode(struct ad5755_state *st, enum ad5755_mode mode)
-{
-	switch (mode) {
-	case AD5755_MODE_VOLTAGE_0V_5V:
-	case AD5755_MODE_VOLTAGE_0V_10V:
-	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
-	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
-		return st->chip_info->has_voltage_out;
-	case AD5755_MODE_CURRENT_4mA_20mA:
-	case AD5755_MODE_CURRENT_0mA_20mA:
-	case AD5755_MODE_CURRENT_0mA_24mA:
-		return true;
-	default:
-		return false;
-	}
-}
-
 static int ad5755_setup_pdata(struct iio_dev *indio_dev,
-			      const struct ad5755_platform_data *pdata)
+			      struct ad5755_platform_data *pdata)
 {
-	struct ad5755_state *st = iio_priv(indio_dev);
 	unsigned int val;
 	unsigned int i;
 	int ret;
@@ -479,39 +732,17 @@ static int ad5755_setup_pdata(struct iio_dev *indio_dev,
 	}
 
 	for (i = 0; i < ARRAY_SIZE(pdata->dac); ++i) {
-		if (!ad5755_is_valid_mode(st, pdata->dac[i].mode))
-			return -EINVAL;
-
-		val = 0;
-		if (!pdata->dac[i].ext_current_sense_resistor)
-			val |= AD5755_DAC_INT_CURRENT_SENSE_RESISTOR;
-		if (pdata->dac[i].enable_voltage_overrange)
-			val |= AD5755_DAC_VOLTAGE_OVERRANGE_EN;
-		val |= pdata->dac[i].mode;
-
-		ret = ad5755_update_dac_ctrl(indio_dev, i, val, 0);
+		ret = ad5755_setup_dac(indio_dev, i);
 		if (ret < 0)
 			return ret;
+
 	}
 
 	return 0;
 }
 
-static bool ad5755_is_voltage_mode(enum ad5755_mode mode)
-{
-	switch (mode) {
-	case AD5755_MODE_VOLTAGE_0V_5V:
-	case AD5755_MODE_VOLTAGE_0V_10V:
-	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
-	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
-		return true;
-	default:
-		return false;
-	}
-}
-
 static int ad5755_init_channels(struct iio_dev *indio_dev,
-				const struct ad5755_platform_data *pdata)
+				struct ad5755_platform_data *pdata)
 {
 	struct ad5755_state *st = iio_priv(indio_dev);
 	struct iio_chan_spec *channels = st->channels;
@@ -521,8 +752,8 @@ static int ad5755_init_channels(struct iio_dev *indio_dev,
 		channels[i] = st->chip_info->channel_template;
 		channels[i].channel = i;
 		channels[i].address = i;
-		if (pdata && ad5755_is_voltage_mode(pdata->dac[i].mode))
-			channels[i].type = IIO_VOLTAGE;
+		if (st->chip_info->has_voltage_out)
+			channels[i].type = IIO_DUAL_VOLTCUR;
 		else
 			channels[i].type = IIO_CURRENT;
 	}
@@ -543,7 +774,7 @@ static int ad5755_init_channels(struct iio_dev *indio_dev,
 		}, \
 	}
 
-static const struct ad5755_platform_data ad5755_default_pdata = {
+struct ad5755_platform_data ad5755_default_pdata = {
 	.ext_dc_dc_compenstation_resistor = false,
 	.dc_dc_phase = AD5755_DC_DC_PHASE_ALL_SAME_EDGE,
 	.dc_dc_freq = AD5755_DC_DC_FREQ_410kHZ,
@@ -559,7 +790,7 @@ static const struct ad5755_platform_data ad5755_default_pdata = {
 static int ad5755_probe(struct spi_device *spi)
 {
 	enum ad5755_type type = spi_get_device_id(spi)->driver_data;
-	const struct ad5755_platform_data *pdata = dev_get_platdata(&spi->dev);
+	struct ad5755_platform_data *pdata = dev_get_platdata(&spi->dev);
 	struct iio_dev *indio_dev;
 	struct ad5755_state *st;
 	int ret;
@@ -586,6 +817,8 @@ static int ad5755_probe(struct spi_device *spi)
 	if (!pdata)
 		pdata = &ad5755_default_pdata;
 
+	st->pdata = pdata;
+
 	ret = ad5755_init_channels(indio_dev, pdata);
 	if (ret)
 		return ret;
-- 
2.7.0

Re: [RFC 2/2] iio: ad5755: added support for switching between voltage and current output

[Jonathan Cameron]

On 22/01/16 12:54, Sean Nyekjaer wrote:
> DAC ad5755 have both support for voltage and current output, before the driver
> only had support for switching modes at compile time. Not very smart...
> 
This is currently where we possibly disagree.  It was a very deliberate decision to
do it where it is done. It is way to easy to blow hardware up if the wrong
range is selected on a multirange DAC and as far as I can see this is
effectively the same.

It is btw not at compile time, but at boot time of a given piece of hardware.
It's not as though a kernel for multiple boards will not allow different
combinations for different boards. A fine distinction of course and one
that I suspect is causing you trouble because you are dealing with external
connections and no knowledge of what is beyond them (i.e. a PLC or general
purpose output board?)


Ah, I'll revise this (having dived into the datasheet) 
what wasn't clear immediately is that the chip puts the voltage and current
outputs on different pins - but only one is enabled at a time.  This makes for
a rather different set of circumstances and explains when you might
want to allow runtime configuation.  Note however, that this means they really
are different channels - just ones with some constraints on which combinations
are enabled at any given time.  The datasheet does say you can tie the two
together (I guess for use as a flexible PLC output for example) but they
aren't tied together on the chip so we don't have to care ;)

I wonder if what we really need here is a standard way of applying platform
data based channel restrictions...  This applies to all multirange output
parts. 

Requirements:

1) List of 'safe ranges' for a given piece of hardware.
2) In dual parts like this one, list of 'safe ranges' for both current and voltage
outputs.

If people then want to operate in only one mode (so on a board where the use is known)
then they provide only one entry and that's all the part will be used in.

If, as I guess you are doing, the range is unknown then the part is either powered down
until the range is set by userspace, or powered up in the 'safest' mode of those listed.

Hence if it was unsafe to use it as a current output at all, that list would be empty
for this part (same with voltage range list) and the driver would refuse to put
it in that mode at all.

So for your usecase you'd simply specify that all ranges are fine.  If someone blows
external kit up, it's their own fault as we had no way of knowing what was safe.

Still, here definitely separate current and voltage channels!

Jonathan
> This patch adds support for switching modes from userspace.
> 
> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
> ---

As I stated in my last email in response to patch 1 I think continuing the
discussion on how to handle this here makes more sense...
> 
> Register them as two independent channels. One for current and one for
>  voltage. In a sense the change of measurement type is just a different type
>  of front end mux like you see on many ADCs.

Hmm. Thinking more on this I'm worried that, as with multirange DACs,
it might be possible to switch this DAC into a mode that will actually
damage the hardware.  I note it is also a multirange part and that was
originaly controlled by platform data.

I would thing we either need to restrict the choice to platform data only,
or add some concept of limiting the resulting ranges in platform data.

You are in a fairly odd situation if you want to, at runtime switch a
given bit of circuitry from one mode to the other.

What is your usecase?

I really don't like the combined channel as it's either one or the other
at any given time, not some mixture of the two. Given the simple nature
of powerdown on channels on this device, here we can just do it by allowing
only the voltage or the current channel to be powered up at any given time.

See below for various other comemnts.

Lars, any thoughts on this?  You'd probably come across more of these
multirange parts than I have.

Jonathan

> 
> This patch is not done yet :-) I would like to get some feedback of my work.
> I have tested this patch with an ad5755 and it works.
> 
> So if you have any ideas on how I should progress please give me some feedback.
> 
>  drivers/iio/dac/ad5755.c | 405 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 319 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad5755.c b/drivers/iio/dac/ad5755.c
> index bfb350a..6e7ab3f 100644
> --- a/drivers/iio/dac/ad5755.c
> +++ b/drivers/iio/dac/ad5755.c
> @@ -63,6 +63,9 @@
>  #define AD5755_SLEW_RATE_SHIFT			3
>  #define AD5755_SLEW_ENABLE			BIT(12)
>  
> +#define AD5755_VOLTAGE_MODE			0
> +#define AD5755_CURRENT_MODE			1
> +
>  /**
>   * struct ad5755_chip_info - chip specific information
>   * @channel_template:	channel specification
> @@ -91,6 +94,8 @@ struct ad5755_state {
>  	unsigned int			ctrl[AD5755_NUM_CHANNELS];
>  	struct iio_chan_spec		channels[AD5755_NUM_CHANNELS];
>  
> +	struct ad5755_platform_data *pdata;
> +
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> @@ -109,6 +114,163 @@ enum ad5755_type {
>  	ID_AD5737,
>  };
>  
> +struct ad5755_ranges {
> +	enum ad5755_mode range;
> +	unsigned int scale;
> +	int offset;
> +};
> +
> +static const struct ad5755_ranges ad5755_range_def[] = {
> +	{
> +		.range = AD5755_MODE_VOLTAGE_0V_5V,
> +		.scale = 76293945,
> +		.offset = 0,
> +	}, {
> +		.range = AD5755_MODE_VOLTAGE_0V_10V,
> +		.scale = 152587890,
> +		.offset = 0,
> +	}, {
> +		.range = AD5755_MODE_VOLTAGE_PLUSMINUS_5V,
> +		.scale = 152587890,
> +		.offset = -(1 << (16 /*REALBITS*/ - 1)),
> +	}, {
> +		.range = AD5755_MODE_VOLTAGE_PLUSMINUS_10V,
> +		.scale = 305175781,
> +		.offset = -(1 << (16 /*REALBITS*/ - 1)),
> +	}, {
> +		.range = AD5755_MODE_CURRENT_4mA_20mA,
> +		.scale = 244140,
> +		.offset = 16384,
> +	}, {
> +		.range = AD5755_MODE_CURRENT_0mA_20mA,
> +		.scale = 305175,
> +		.offset = 0,
> +	}, {
> +		.range = AD5755_MODE_CURRENT_0mA_24mA,
> +		.scale = 366210,
> +		.offset = 0,
> +	}
> +};
> +
> +static inline enum ad5755_mode ad5755_get_chan_mode(struct ad5755_state *st,
> +						    const struct iio_chan_spec
> +						    *chan)
> +{
> +	return st->ctrl[chan->channel] & 7;
> +}
> +
> +static inline int ad5755_get_chan_scale(struct ad5755_state *st,
> +					const struct iio_chan_spec *chan)
> +{
> +	return ad5755_range_def[ad5755_get_chan_mode(st, chan)].scale;
> +}
> +
> +static inline int ad5755_get_chan_offset(struct ad5755_state *st,
> +					 const struct iio_chan_spec *chan)
> +{
> +	return ad5755_range_def[ad5755_get_chan_mode(st, chan)].offset;
> +}
> +
> +static bool ad5755_is_voltage_mode(enum ad5755_mode mode)
> +{
> +	switch (mode) {
> +	case AD5755_MODE_VOLTAGE_0V_5V:
> +	case AD5755_MODE_VOLTAGE_0V_10V:
> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static ssize_t ad5755_show_voltcur_modes(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	return sprintf(buf, "voltage: 0\ncurrent: 1\n");
This is just not how it is done.  Value written must = value read
(assuming it succeeded and nothing has change it in the meantime).

We have the IIO_ENUM stuff in iio.h to handle this common case of matching
strings to real values.

My gut feeling is that, subject to be convinced that we want runtime
configuration of current vs voltage output (that it can be prevented
from causing hardware damange), that we want to have
separate channels for the current and voltage and handle this by effectively
controlling whether they are enabled or not.  Only allow either
the current or the voltage channel to power up at a given time
(internally this is presumably what the hardware is doing anyway!)

> +}
> +
> +static ssize_t ad5755_scales(char *buf, bool voltage_mode)
> +{
> +	int i, len = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
> +		if (ad5755_is_voltage_mode(i) != voltage_mode)
> +			continue;
> +		len += sprintf(buf + len, "0.%09u ", ad5755_range_def[i].scale);
> +	}
> +	len += sprintf(buf + len, "\n");
> +
> +	return len;
> +}
> +
> +static ssize_t ad5755_offset(char *buf, bool voltage_mode)
> +{
> +	int i, len = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
> +		if (ad5755_is_voltage_mode(i) != voltage_mode)
> +			continue;
> +		len += sprintf(buf + len, "%d ", ad5755_range_def[i].offset);
> +	}
> +	len += sprintf(buf + len, "\n");
> +
> +	return len;
> +}
> +
> +static ssize_t ad5755_show_voltage_scales(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	return ad5755_scales(buf, true);
> +}
> +
> +static ssize_t ad5755_show_voltage_offset(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	return ad5755_offset(buf, true);
> +}
> +
> +static ssize_t ad5755_show_current_scales(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	return ad5755_scales(buf, false);
> +}
> +
> +static ssize_t ad5755_show_current_offset(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	return ad5755_offset(buf, false);
> +}
> +
> +static IIO_DEVICE_ATTR(out_voltcur_modes_available, S_IRUGO,
> +		       ad5755_show_voltcur_modes, NULL, 0);
> +static IIO_DEVICE_ATTR(out_voltage_scale_available, S_IRUGO,
> +		       ad5755_show_voltage_scales, NULL, 0);
> +static IIO_DEVICE_ATTR(out_voltage_offset_available, S_IRUGO,
> +		       ad5755_show_voltage_offset, NULL, 0);
> +static IIO_DEVICE_ATTR(out_current_scale_available, S_IRUGO,
> +		       ad5755_show_current_scales, NULL, 0);
> +static IIO_DEVICE_ATTR(out_current_offset_available, S_IRUGO,
> +		       ad5755_show_current_offset, NULL, 0);
> +
> +static struct attribute *ad5755_attributes[] = {
> +	&iio_dev_attr_out_voltcur_modes_available.dev_attr.attr,
> +	&iio_dev_attr_out_voltage_scale_available.dev_attr.attr,
> +	&iio_dev_attr_out_voltage_offset_available.dev_attr.attr,
> +	&iio_dev_attr_out_current_scale_available.dev_attr.attr,
> +	&iio_dev_attr_out_current_offset_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ad5755_attribute_group = {
> +	.attrs = ad5755_attributes,
> +};
> +
>  static int ad5755_write_unlocked(struct iio_dev *indio_dev,
>  	unsigned int reg, unsigned int val)
>  {
> @@ -226,31 +388,54 @@ out_unlock:
>  	return 0;
>  }
>  
> -static const int ad5755_min_max_table[][2] = {
> -	[AD5755_MODE_VOLTAGE_0V_5V] = { 0, 5000 },
> -	[AD5755_MODE_VOLTAGE_0V_10V] = { 0, 10000 },
> -	[AD5755_MODE_VOLTAGE_PLUSMINUS_5V] = { -5000, 5000 },
> -	[AD5755_MODE_VOLTAGE_PLUSMINUS_10V] = { -10000, 10000 },
> -	[AD5755_MODE_CURRENT_4mA_20mA] = { 4, 20 },
> -	[AD5755_MODE_CURRENT_0mA_20mA] = { 0, 20 },
> -	[AD5755_MODE_CURRENT_0mA_24mA] = { 0, 24 },
> -};
> +static bool ad5755_is_valid_mode(struct ad5755_state *st, enum ad5755_mode mode)
> +{
> +	switch (mode) {
> +	case AD5755_MODE_VOLTAGE_0V_5V:
> +	case AD5755_MODE_VOLTAGE_0V_10V:
> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
> +		return st->chip_info->has_voltage_out;
> +	case AD5755_MODE_CURRENT_4mA_20mA:
> +	case AD5755_MODE_CURRENT_0mA_20mA:
> +	case AD5755_MODE_CURRENT_0mA_24mA:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
>  
> -static void ad5755_get_min_max(struct ad5755_state *st,
> -	struct iio_chan_spec const *chan, int *min, int *max)
> +static int ad5755_clear_dac(struct iio_dev *indio_dev, int channel)
>  {
> -	enum ad5755_mode mode = st->ctrl[chan->channel] & 7;
> -	*min = ad5755_min_max_table[mode][0];
> -	*max = ad5755_min_max_table[mode][1];
> +	int i = channel;
> +	int ret;
> +
> +	ret = ad5755_update_dac_ctrl(indio_dev, i, 0, UINT_MAX);
> +	return ret;
> +
>  }
>  
> -static inline int ad5755_get_offset(struct ad5755_state *st,
> -	struct iio_chan_spec const *chan)
> +static int ad5755_setup_dac(struct iio_dev *indio_dev, int channel)
>  {
> -	int min, max;
> +	int i = channel;
> +	int ret;
> +	struct ad5755_state *st = iio_priv(indio_dev);
> +	unsigned int val;
> +	struct ad5755_platform_data *pdata = st->pdata;
> +
> +	if (!ad5755_is_valid_mode(st, pdata->dac[i].mode))
> +		return -EINVAL;
> +
> +	val = 0;
> +	if (!pdata->dac[i].ext_current_sense_resistor)
> +		val |= AD5755_DAC_INT_CURRENT_SENSE_RESISTOR;
> +	if (pdata->dac[i].enable_voltage_overrange)
> +		val |= AD5755_DAC_VOLTAGE_OVERRANGE_EN;
> +	val |= pdata->dac[i].mode;
> +
> +	ret = ad5755_update_dac_ctrl(indio_dev, i, val, 0);
> +	return ret;
>  
> -	ad5755_get_min_max(st, chan, &min, &max);
> -	return (min * (1 << chan->scan_type.realbits)) / (max - min);
>  }
>  
>  static int ad5755_chan_reg_info(struct ad5755_state *st,
> @@ -294,18 +479,25 @@ static int ad5755_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct ad5755_state *st = iio_priv(indio_dev);
>  	unsigned int reg, shift, offset;
> -	int min, max;
>  	int ret;
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_SCALE:
> -		ad5755_get_min_max(st, chan, &min, &max);
> -		*val = max - min;
> -		*val2 = chan->scan_type.realbits;
> -		return IIO_VAL_FRACTIONAL_LOG2;
> +		*val = 0;
> +		*val2 = ad5755_get_chan_scale(st, chan);
> +		return IIO_VAL_INT_PLUS_NANO;
>  	case IIO_CHAN_INFO_OFFSET:
> -		*val = ad5755_get_offset(st, chan);
> +		*val = ad5755_get_chan_offset(st, chan);
>  		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_MODE:
> +		if (ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode)) {
> +			*val = AD5755_VOLTAGE_MODE;
> +			return IIO_VAL_INT;
> +		} else {
> +			*val = AD5755_CURRENT_MODE;
> +			return IIO_VAL_INT;
> +		}
> +		break;
>  	default:
>  		ret = ad5755_chan_reg_info(st, chan, info, false,
>  						&reg, &shift, &offset);
> @@ -325,24 +517,100 @@ static int ad5755_read_raw(struct iio_dev *indio_dev,
>  }
>  
>  static int ad5755_write_raw(struct iio_dev *indio_dev,
> -	const struct iio_chan_spec *chan, int val, int val2, long info)
> +			    const struct iio_chan_spec *chan, int val, int val2,
> +			    long info)
This is just reformatting. Might be worthwhile, but should be in a separate
patch.
>  {
>  	struct ad5755_state *st = iio_priv(indio_dev);
> -	unsigned int shift, reg, offset;
> -	int ret;
> +	unsigned int shift, reg;
> +	bool is_voltage;
> +	int offset;
> +	unsigned int scale;
> +	int ret, i;
>  
> -	ret = ad5755_chan_reg_info(st, chan, info, true,
> -					&reg, &shift, &offset);
> -	if (ret)
> -		return ret;
> -
> -	val <<= shift;
> -	val += offset;
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +	case IIO_CHAN_INFO_OFFSET:
> +	case IIO_CHAN_INFO_MODE:
> +		if (!(bool) (st->pwr_down & (1 << chan->channel))) {
> +			printk
> +			    ("POWER DOWN off - Power down before shifting modes\n");
> +			return -EPERM;
> +		}
> +	}
>  
> -	if (val < 0 || val > 0xffff)
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		is_voltage =
> +		    ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode);
> +		offset =
> +		    ad5755_range_def[st->pdata->dac[chan->address].mode].offset;
> +		/* is new scale allowed */
> +		for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
> +			if (val2 == ad5755_range_def[i].scale &&
> +			    offset == ad5755_range_def[i].offset &&
> +			    is_voltage == ad5755_is_voltage_mode(i)) {
> +				st->pdata->dac[chan->address].mode = i;
> +				ad5755_clear_dac(indio_dev, chan->address);
> +				return ad5755_setup_dac(indio_dev, chan->address);
> +			}
> +		}
>  		return -EINVAL;
> +	case IIO_CHAN_INFO_OFFSET:
> +		is_voltage =
> +		    ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode);
> +		scale =
> +		    ad5755_range_def[st->pdata->dac[chan->address].mode].scale;
> +		/* is new offset allowed */
> +		for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
> +			if (val == ad5755_range_def[i].offset &&
> +			    scale == ad5755_range_def[i].scale &&
> +			    is_voltage == ad5755_is_voltage_mode(i)) {
> +				st->pdata->dac[chan->address].mode = i;
> +				ad5755_clear_dac(indio_dev, chan->address);
> +				return ad5755_setup_dac(indio_dev, chan->address);
> +			}
> +		}
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_MODE:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		if (val == AD5755_VOLTAGE_MODE)
This looks like magic values to me.
If we did end up with such an interface, you'd want to be using the
extended attribute stuff and the enum support it has to give
meaningful names to these.

> +			st->pdata->dac[chan->address].mode = AD5755_MODE_VOLTAGE_0V_5V;
> +		else
> +			st->pdata->dac[chan->address].mode = AD5755_MODE_CURRENT_0mA_20mA;
> +		ad5755_clear_dac(indio_dev, chan->address);
> +
> +		return ad5755_setup_dac(indio_dev, chan->address);
> +		break;
> +	default:
> +		ret = ad5755_chan_reg_info(st, chan, info, true,
> +					   &reg, &shift, &offset);
> +		if (ret)
> +			return ret;
> +
> +		val <<= shift;
> +		val += offset;
> +
> +		if (val < 0 || val > 0xffff)
> +			return -EINVAL;
> +
> +		return ad5755_write(indio_dev, reg, val);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad5755_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;
> +	}
>  
> -	return ad5755_write(indio_dev, reg, val);
> +	return -EINVAL;
>  }
>  
>  static ssize_t ad5755_read_powerdown(struct iio_dev *indio_dev, uintptr_t priv,
> @@ -371,6 +639,8 @@ static ssize_t ad5755_write_powerdown(struct iio_dev *indio_dev, uintptr_t priv,
>  static const struct iio_info ad5755_info = {
>  	.read_raw = ad5755_read_raw,
>  	.write_raw = ad5755_write_raw,
> +	.write_raw_get_fmt = &ad5755_write_raw_get_fmt,
> +	.attrs = &ad5755_attribute_group,
>  	.driver_module = THIS_MODULE,
>  };
>  
> @@ -391,7 +661,8 @@ static const struct iio_chan_spec_ext_info ad5755_ext_info[] = {
>  		BIT(IIO_CHAN_INFO_SCALE) |			\
>  		BIT(IIO_CHAN_INFO_OFFSET) |			\
>  		BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
> -		BIT(IIO_CHAN_INFO_CALIBBIAS),			\
> +		BIT(IIO_CHAN_INFO_CALIBBIAS)  |			\
> +		BIT(IIO_CHAN_INFO_MODE),			\
>  	.scan_type = {						\
>  		.sign = 'u',					\
>  		.realbits = (_bits),				\
> @@ -424,27 +695,9 @@ static const struct ad5755_chip_info ad5755_chip_info_tbl[] = {
>  	},
>  };
>  
> -static bool ad5755_is_valid_mode(struct ad5755_state *st, enum ad5755_mode mode)
> -{
> -	switch (mode) {
> -	case AD5755_MODE_VOLTAGE_0V_5V:
> -	case AD5755_MODE_VOLTAGE_0V_10V:
> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
> -		return st->chip_info->has_voltage_out;
> -	case AD5755_MODE_CURRENT_4mA_20mA:
> -	case AD5755_MODE_CURRENT_0mA_20mA:
> -	case AD5755_MODE_CURRENT_0mA_24mA:
> -		return true;
> -	default:
> -		return false;
> -	}
> -}
> -
>  static int ad5755_setup_pdata(struct iio_dev *indio_dev,
> -			      const struct ad5755_platform_data *pdata)
> +			      struct ad5755_platform_data *pdata)
>  {
> -	struct ad5755_state *st = iio_priv(indio_dev);
>  	unsigned int val;
>  	unsigned int i;
>  	int ret;
> @@ -479,39 +732,17 @@ static int ad5755_setup_pdata(struct iio_dev *indio_dev,
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(pdata->dac); ++i) {
> -		if (!ad5755_is_valid_mode(st, pdata->dac[i].mode))
> -			return -EINVAL;
> -
> -		val = 0;
> -		if (!pdata->dac[i].ext_current_sense_resistor)
> -			val |= AD5755_DAC_INT_CURRENT_SENSE_RESISTOR;
> -		if (pdata->dac[i].enable_voltage_overrange)
> -			val |= AD5755_DAC_VOLTAGE_OVERRANGE_EN;
> -		val |= pdata->dac[i].mode;
> -
> -		ret = ad5755_update_dac_ctrl(indio_dev, i, val, 0);
> +		ret = ad5755_setup_dac(indio_dev, i);
>  		if (ret < 0)
>  			return ret;
> +
>  	}
>  
>  	return 0;
>  }
>  
> -static bool ad5755_is_voltage_mode(enum ad5755_mode mode)
> -{
> -	switch (mode) {
> -	case AD5755_MODE_VOLTAGE_0V_5V:
> -	case AD5755_MODE_VOLTAGE_0V_10V:
> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
> -		return true;
> -	default:
> -		return false;
> -	}
> -}
> -
>  static int ad5755_init_channels(struct iio_dev *indio_dev,
> -				const struct ad5755_platform_data *pdata)
> +				struct ad5755_platform_data *pdata)
>  {
>  	struct ad5755_state *st = iio_priv(indio_dev);
>  	struct iio_chan_spec *channels = st->channels;
> @@ -521,8 +752,8 @@ static int ad5755_init_channels(struct iio_dev *indio_dev,
>  		channels[i] = st->chip_info->channel_template;
>  		channels[i].channel = i;
>  		channels[i].address = i;
> -		if (pdata && ad5755_is_voltage_mode(pdata->dac[i].mode))
> -			channels[i].type = IIO_VOLTAGE;
> +		if (st->chip_info->has_voltage_out)
> +			channels[i].type = IIO_DUAL_VOLTCUR;
>  		else
>  			channels[i].type = IIO_CURRENT;
>  	}
> @@ -543,7 +774,7 @@ static int ad5755_init_channels(struct iio_dev *indio_dev,
>  		}, \
>  	}
>  
> -static const struct ad5755_platform_data ad5755_default_pdata = {
> +struct ad5755_platform_data ad5755_default_pdata = {
>  	.ext_dc_dc_compenstation_resistor = false,
>  	.dc_dc_phase = AD5755_DC_DC_PHASE_ALL_SAME_EDGE,
>  	.dc_dc_freq = AD5755_DC_DC_FREQ_410kHZ,
> @@ -559,7 +790,7 @@ static const struct ad5755_platform_data ad5755_default_pdata = {
>  static int ad5755_probe(struct spi_device *spi)
>  {
>  	enum ad5755_type type = spi_get_device_id(spi)->driver_data;
> -	const struct ad5755_platform_data *pdata = dev_get_platdata(&spi->dev);
> +	struct ad5755_platform_data *pdata = dev_get_platdata(&spi->dev);
>  	struct iio_dev *indio_dev;
>  	struct ad5755_state *st;
>  	int ret;
> @@ -586,6 +817,8 @@ static int ad5755_probe(struct spi_device *spi)
>  	if (!pdata)
>  		pdata = &ad5755_default_pdata;
>  
> +	st->pdata = pdata;
> +
>  	ret = ad5755_init_channels(indio_dev, pdata);
>  	if (ret)
>  		return ret;
> 

Re: [RFC 2/2] iio: ad5755: added support for switching between voltage and current output

[Sean Nyekjær]

Hi Jonathan

I havn't had any luck to export two outputs for one channel :-(
Can you point me in the right direction? Maybe I need to modify 
something in iio-core to allow this...

If i'm trying this: I only get voltage export and they are numberet from 4

#define AD5755_NUM_CHANNELS 8

static int ad5755_init_channels(struct iio_dev *indio_dev,
                                 struct ad5755_platform_data *pdata)
{
         struct ad5755_state *st = iio_priv(indio_dev);
         struct iio_chan_spec *channels = st->channels;
         unsigned int i;

         for (i = 0; i < AD5755_NUM_CHANNELS; ++i) {
                 channels[i] = st->chip_info->channel_template;
                 if(i>3) {
                         channels[i].channel = i;
                         channels[i].address = i;
                         channels[i].type = IIO_VOLTAGE;
                 } else {
                         channels[i].channel = i-4;
                         channels[i].address = i-4;
                         channels[i].type = IIO_CURRENT;
                 }
         }

         indio_dev->channels = channels;

         return 0;
}

/Sean

On 2016-01-24 16:49, Jonathan Cameron wrote:
> On 22/01/16 12:54, Sean Nyekjaer wrote:
>> DAC ad5755 have both support for voltage and current output, before the driver
>> only had support for switching modes at compile time. Not very smart...
>>
> This is currently where we possibly disagree.  It was a very deliberate decision to
> do it where it is done. It is way to easy to blow hardware up if the wrong
> range is selected on a multirange DAC and as far as I can see this is
> effectively the same.
>
> It is btw not at compile time, but at boot time of a given piece of hardware.
> It's not as though a kernel for multiple boards will not allow different
> combinations for different boards. A fine distinction of course and one
> that I suspect is causing you trouble because you are dealing with external
> connections and no knowledge of what is beyond them (i.e. a PLC or general
> purpose output board?)
>
>
> Ah, I'll revise this (having dived into the datasheet)
> what wasn't clear immediately is that the chip puts the voltage and current
> outputs on different pins - but only one is enabled at a time.  This makes for
> a rather different set of circumstances and explains when you might
> want to allow runtime configuation.  Note however, that this means they really
> are different channels - just ones with some constraints on which combinations
> are enabled at any given time.  The datasheet does say you can tie the two
> together (I guess for use as a flexible PLC output for example) but they
> aren't tied together on the chip so we don't have to care ;)
>
> I wonder if what we really need here is a standard way of applying platform
> data based channel restrictions...  This applies to all multirange output
> parts.
>
> Requirements:
>
> 1) List of 'safe ranges' for a given piece of hardware.
> 2) In dual parts like this one, list of 'safe ranges' for both current and voltage
> outputs.
>
> If people then want to operate in only one mode (so on a board where the use is known)
> then they provide only one entry and that's all the part will be used in.
>
> If, as I guess you are doing, the range is unknown then the part is either powered down
> until the range is set by userspace, or powered up in the 'safest' mode of those listed.
>
> Hence if it was unsafe to use it as a current output at all, that list would be empty
> for this part (same with voltage range list) and the driver would refuse to put
> it in that mode at all.
>
> So for your usecase you'd simply specify that all ranges are fine.  If someone blows
> external kit up, it's their own fault as we had no way of knowing what was safe.
>
> Still, here definitely separate current and voltage channels!
>
> Jonathan
>> This patch adds support for switching modes from userspace.
>>
>> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
>> ---
> As I stated in my last email in response to patch 1 I think continuing the
> discussion on how to handle this here makes more sense...
>> Register them as two independent channels. One for current and one for
>>   voltage. In a sense the change of measurement type is just a different type
>>   of front end mux like you see on many ADCs.
> Hmm. Thinking more on this I'm worried that, as with multirange DACs,
> it might be possible to switch this DAC into a mode that will actually
> damage the hardware.  I note it is also a multirange part and that was
> originaly controlled by platform data.
>
> I would thing we either need to restrict the choice to platform data only,
> or add some concept of limiting the resulting ranges in platform data.
>
> You are in a fairly odd situation if you want to, at runtime switch a
> given bit of circuitry from one mode to the other.
>
> What is your usecase?
>
> I really don't like the combined channel as it's either one or the other
> at any given time, not some mixture of the two. Given the simple nature
> of powerdown on channels on this device, here we can just do it by allowing
> only the voltage or the current channel to be powered up at any given time.
>
> See below for various other comemnts.
>
> Lars, any thoughts on this?  You'd probably come across more of these
> multirange parts than I have.
>
> Jonathan
>
>> This patch is not done yet :-) I would like to get some feedback of my work.
>> I have tested this patch with an ad5755 and it works.
>>
>> So if you have any ideas on how I should progress please give me some feedback.
>>
>>   drivers/iio/dac/ad5755.c | 405 +++++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 319 insertions(+), 86 deletions(-)
>>
>> diff --git a/drivers/iio/dac/ad5755.c b/drivers/iio/dac/ad5755.c
>> index bfb350a..6e7ab3f 100644
>> --- a/drivers/iio/dac/ad5755.c
>> +++ b/drivers/iio/dac/ad5755.c
>> @@ -63,6 +63,9 @@
>>   #define AD5755_SLEW_RATE_SHIFT			3
>>   #define AD5755_SLEW_ENABLE			BIT(12)
>>   
>> +#define AD5755_VOLTAGE_MODE			0
>> +#define AD5755_CURRENT_MODE			1
>> +
>>   /**
>>    * struct ad5755_chip_info - chip specific information
>>    * @channel_template:	channel specification
>> @@ -91,6 +94,8 @@ struct ad5755_state {
>>   	unsigned int			ctrl[AD5755_NUM_CHANNELS];
>>   	struct iio_chan_spec		channels[AD5755_NUM_CHANNELS];
>>   
>> +	struct ad5755_platform_data *pdata;
>> +
>>   	/*
>>   	 * DMA (thus cache coherency maintenance) requires the
>>   	 * transfer buffers to live in their own cache lines.
>> @@ -109,6 +114,163 @@ enum ad5755_type {
>>   	ID_AD5737,
>>   };
>>   
>> +struct ad5755_ranges {
>> +	enum ad5755_mode range;
>> +	unsigned int scale;
>> +	int offset;
>> +};
>> +
>> +static const struct ad5755_ranges ad5755_range_def[] = {
>> +	{
>> +		.range = AD5755_MODE_VOLTAGE_0V_5V,
>> +		.scale = 76293945,
>> +		.offset = 0,
>> +	}, {
>> +		.range = AD5755_MODE_VOLTAGE_0V_10V,
>> +		.scale = 152587890,
>> +		.offset = 0,
>> +	}, {
>> +		.range = AD5755_MODE_VOLTAGE_PLUSMINUS_5V,
>> +		.scale = 152587890,
>> +		.offset = -(1 << (16 /*REALBITS*/ - 1)),
>> +	}, {
>> +		.range = AD5755_MODE_VOLTAGE_PLUSMINUS_10V,
>> +		.scale = 305175781,
>> +		.offset = -(1 << (16 /*REALBITS*/ - 1)),
>> +	}, {
>> +		.range = AD5755_MODE_CURRENT_4mA_20mA,
>> +		.scale = 244140,
>> +		.offset = 16384,
>> +	}, {
>> +		.range = AD5755_MODE_CURRENT_0mA_20mA,
>> +		.scale = 305175,
>> +		.offset = 0,
>> +	}, {
>> +		.range = AD5755_MODE_CURRENT_0mA_24mA,
>> +		.scale = 366210,
>> +		.offset = 0,
>> +	}
>> +};
>> +
>> +static inline enum ad5755_mode ad5755_get_chan_mode(struct ad5755_state *st,
>> +						    const struct iio_chan_spec
>> +						    *chan)
>> +{
>> +	return st->ctrl[chan->channel] & 7;
>> +}
>> +
>> +static inline int ad5755_get_chan_scale(struct ad5755_state *st,
>> +					const struct iio_chan_spec *chan)
>> +{
>> +	return ad5755_range_def[ad5755_get_chan_mode(st, chan)].scale;
>> +}
>> +
>> +static inline int ad5755_get_chan_offset(struct ad5755_state *st,
>> +					 const struct iio_chan_spec *chan)
>> +{
>> +	return ad5755_range_def[ad5755_get_chan_mode(st, chan)].offset;
>> +}
>> +
>> +static bool ad5755_is_voltage_mode(enum ad5755_mode mode)
>> +{
>> +	switch (mode) {
>> +	case AD5755_MODE_VOLTAGE_0V_5V:
>> +	case AD5755_MODE_VOLTAGE_0V_10V:
>> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
>> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static ssize_t ad5755_show_voltcur_modes(struct device *dev,
>> +					 struct device_attribute *attr,
>> +					 char *buf)
>> +{
>> +	return sprintf(buf, "voltage: 0\ncurrent: 1\n");
> This is just not how it is done.  Value written must = value read
> (assuming it succeeded and nothing has change it in the meantime).
>
> We have the IIO_ENUM stuff in iio.h to handle this common case of matching
> strings to real values.
>
> My gut feeling is that, subject to be convinced that we want runtime
> configuration of current vs voltage output (that it can be prevented
> from causing hardware damange), that we want to have
> separate channels for the current and voltage and handle this by effectively
> controlling whether they are enabled or not.  Only allow either
> the current or the voltage channel to power up at a given time
> (internally this is presumably what the hardware is doing anyway!)
>
>> +}
>> +
>> +static ssize_t ad5755_scales(char *buf, bool voltage_mode)
>> +{
>> +	int i, len = 0;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
>> +		if (ad5755_is_voltage_mode(i) != voltage_mode)
>> +			continue;
>> +		len += sprintf(buf + len, "0.%09u ", ad5755_range_def[i].scale);
>> +	}
>> +	len += sprintf(buf + len, "\n");
>> +
>> +	return len;
>> +}
>> +
>> +static ssize_t ad5755_offset(char *buf, bool voltage_mode)
>> +{
>> +	int i, len = 0;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
>> +		if (ad5755_is_voltage_mode(i) != voltage_mode)
>> +			continue;
>> +		len += sprintf(buf + len, "%d ", ad5755_range_def[i].offset);
>> +	}
>> +	len += sprintf(buf + len, "\n");
>> +
>> +	return len;
>> +}
>> +
>> +static ssize_t ad5755_show_voltage_scales(struct device *dev,
>> +					  struct device_attribute *attr,
>> +					  char *buf)
>> +{
>> +	return ad5755_scales(buf, true);
>> +}
>> +
>> +static ssize_t ad5755_show_voltage_offset(struct device *dev,
>> +					  struct device_attribute *attr,
>> +					  char *buf)
>> +{
>> +	return ad5755_offset(buf, true);
>> +}
>> +
>> +static ssize_t ad5755_show_current_scales(struct device *dev,
>> +					  struct device_attribute *attr,
>> +					  char *buf)
>> +{
>> +	return ad5755_scales(buf, false);
>> +}
>> +
>> +static ssize_t ad5755_show_current_offset(struct device *dev,
>> +					  struct device_attribute *attr,
>> +					  char *buf)
>> +{
>> +	return ad5755_offset(buf, false);
>> +}
>> +
>> +static IIO_DEVICE_ATTR(out_voltcur_modes_available, S_IRUGO,
>> +		       ad5755_show_voltcur_modes, NULL, 0);
>> +static IIO_DEVICE_ATTR(out_voltage_scale_available, S_IRUGO,
>> +		       ad5755_show_voltage_scales, NULL, 0);
>> +static IIO_DEVICE_ATTR(out_voltage_offset_available, S_IRUGO,
>> +		       ad5755_show_voltage_offset, NULL, 0);
>> +static IIO_DEVICE_ATTR(out_current_scale_available, S_IRUGO,
>> +		       ad5755_show_current_scales, NULL, 0);
>> +static IIO_DEVICE_ATTR(out_current_offset_available, S_IRUGO,
>> +		       ad5755_show_current_offset, NULL, 0);
>> +
>> +static struct attribute *ad5755_attributes[] = {
>> +	&iio_dev_attr_out_voltcur_modes_available.dev_attr.attr,
>> +	&iio_dev_attr_out_voltage_scale_available.dev_attr.attr,
>> +	&iio_dev_attr_out_voltage_offset_available.dev_attr.attr,
>> +	&iio_dev_attr_out_current_scale_available.dev_attr.attr,
>> +	&iio_dev_attr_out_current_offset_available.dev_attr.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group ad5755_attribute_group = {
>> +	.attrs = ad5755_attributes,
>> +};
>> +
>>   static int ad5755_write_unlocked(struct iio_dev *indio_dev,
>>   	unsigned int reg, unsigned int val)
>>   {
>> @@ -226,31 +388,54 @@ out_unlock:
>>   	return 0;
>>   }
>>   
>> -static const int ad5755_min_max_table[][2] = {
>> -	[AD5755_MODE_VOLTAGE_0V_5V] = { 0, 5000 },
>> -	[AD5755_MODE_VOLTAGE_0V_10V] = { 0, 10000 },
>> -	[AD5755_MODE_VOLTAGE_PLUSMINUS_5V] = { -5000, 5000 },
>> -	[AD5755_MODE_VOLTAGE_PLUSMINUS_10V] = { -10000, 10000 },
>> -	[AD5755_MODE_CURRENT_4mA_20mA] = { 4, 20 },
>> -	[AD5755_MODE_CURRENT_0mA_20mA] = { 0, 20 },
>> -	[AD5755_MODE_CURRENT_0mA_24mA] = { 0, 24 },
>> -};
>> +static bool ad5755_is_valid_mode(struct ad5755_state *st, enum ad5755_mode mode)
>> +{
>> +	switch (mode) {
>> +	case AD5755_MODE_VOLTAGE_0V_5V:
>> +	case AD5755_MODE_VOLTAGE_0V_10V:
>> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
>> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
>> +		return st->chip_info->has_voltage_out;
>> +	case AD5755_MODE_CURRENT_4mA_20mA:
>> +	case AD5755_MODE_CURRENT_0mA_20mA:
>> +	case AD5755_MODE_CURRENT_0mA_24mA:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>>   
>> -static void ad5755_get_min_max(struct ad5755_state *st,
>> -	struct iio_chan_spec const *chan, int *min, int *max)
>> +static int ad5755_clear_dac(struct iio_dev *indio_dev, int channel)
>>   {
>> -	enum ad5755_mode mode = st->ctrl[chan->channel] & 7;
>> -	*min = ad5755_min_max_table[mode][0];
>> -	*max = ad5755_min_max_table[mode][1];
>> +	int i = channel;
>> +	int ret;
>> +
>> +	ret = ad5755_update_dac_ctrl(indio_dev, i, 0, UINT_MAX);
>> +	return ret;
>> +
>>   }
>>   
>> -static inline int ad5755_get_offset(struct ad5755_state *st,
>> -	struct iio_chan_spec const *chan)
>> +static int ad5755_setup_dac(struct iio_dev *indio_dev, int channel)
>>   {
>> -	int min, max;
>> +	int i = channel;
>> +	int ret;
>> +	struct ad5755_state *st = iio_priv(indio_dev);
>> +	unsigned int val;
>> +	struct ad5755_platform_data *pdata = st->pdata;
>> +
>> +	if (!ad5755_is_valid_mode(st, pdata->dac[i].mode))
>> +		return -EINVAL;
>> +
>> +	val = 0;
>> +	if (!pdata->dac[i].ext_current_sense_resistor)
>> +		val |= AD5755_DAC_INT_CURRENT_SENSE_RESISTOR;
>> +	if (pdata->dac[i].enable_voltage_overrange)
>> +		val |= AD5755_DAC_VOLTAGE_OVERRANGE_EN;
>> +	val |= pdata->dac[i].mode;
>> +
>> +	ret = ad5755_update_dac_ctrl(indio_dev, i, val, 0);
>> +	return ret;
>>   
>> -	ad5755_get_min_max(st, chan, &min, &max);
>> -	return (min * (1 << chan->scan_type.realbits)) / (max - min);
>>   }
>>   
>>   static int ad5755_chan_reg_info(struct ad5755_state *st,
>> @@ -294,18 +479,25 @@ static int ad5755_read_raw(struct iio_dev *indio_dev,
>>   {
>>   	struct ad5755_state *st = iio_priv(indio_dev);
>>   	unsigned int reg, shift, offset;
>> -	int min, max;
>>   	int ret;
>>   
>>   	switch (info) {
>>   	case IIO_CHAN_INFO_SCALE:
>> -		ad5755_get_min_max(st, chan, &min, &max);
>> -		*val = max - min;
>> -		*val2 = chan->scan_type.realbits;
>> -		return IIO_VAL_FRACTIONAL_LOG2;
>> +		*val = 0;
>> +		*val2 = ad5755_get_chan_scale(st, chan);
>> +		return IIO_VAL_INT_PLUS_NANO;
>>   	case IIO_CHAN_INFO_OFFSET:
>> -		*val = ad5755_get_offset(st, chan);
>> +		*val = ad5755_get_chan_offset(st, chan);
>>   		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_MODE:
>> +		if (ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode)) {
>> +			*val = AD5755_VOLTAGE_MODE;
>> +			return IIO_VAL_INT;
>> +		} else {
>> +			*val = AD5755_CURRENT_MODE;
>> +			return IIO_VAL_INT;
>> +		}
>> +		break;
>>   	default:
>>   		ret = ad5755_chan_reg_info(st, chan, info, false,
>>   						&reg, &shift, &offset);
>> @@ -325,24 +517,100 @@ static int ad5755_read_raw(struct iio_dev *indio_dev,
>>   }
>>   
>>   static int ad5755_write_raw(struct iio_dev *indio_dev,
>> -	const struct iio_chan_spec *chan, int val, int val2, long info)
>> +			    const struct iio_chan_spec *chan, int val, int val2,
>> +			    long info)
> This is just reformatting. Might be worthwhile, but should be in a separate
> patch.
>>   {
>>   	struct ad5755_state *st = iio_priv(indio_dev);
>> -	unsigned int shift, reg, offset;
>> -	int ret;
>> +	unsigned int shift, reg;
>> +	bool is_voltage;
>> +	int offset;
>> +	unsigned int scale;
>> +	int ret, i;
>>   
>> -	ret = ad5755_chan_reg_info(st, chan, info, true,
>> -					&reg, &shift, &offset);
>> -	if (ret)
>> -		return ret;
>> -
>> -	val <<= shift;
>> -	val += offset;
>> +	switch (info) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +	case IIO_CHAN_INFO_OFFSET:
>> +	case IIO_CHAN_INFO_MODE:
>> +		if (!(bool) (st->pwr_down & (1 << chan->channel))) {
>> +			printk
>> +			    ("POWER DOWN off - Power down before shifting modes\n");
>> +			return -EPERM;
>> +		}
>> +	}
>>   
>> -	if (val < 0 || val > 0xffff)
>> +	switch (info) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		is_voltage =
>> +		    ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode);
>> +		offset =
>> +		    ad5755_range_def[st->pdata->dac[chan->address].mode].offset;
>> +		/* is new scale allowed */
>> +		for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
>> +			if (val2 == ad5755_range_def[i].scale &&
>> +			    offset == ad5755_range_def[i].offset &&
>> +			    is_voltage == ad5755_is_voltage_mode(i)) {
>> +				st->pdata->dac[chan->address].mode = i;
>> +				ad5755_clear_dac(indio_dev, chan->address);
>> +				return ad5755_setup_dac(indio_dev, chan->address);
>> +			}
>> +		}
>>   		return -EINVAL;
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		is_voltage =
>> +		    ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode);
>> +		scale =
>> +		    ad5755_range_def[st->pdata->dac[chan->address].mode].scale;
>> +		/* is new offset allowed */
>> +		for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
>> +			if (val == ad5755_range_def[i].offset &&
>> +			    scale == ad5755_range_def[i].scale &&
>> +			    is_voltage == ad5755_is_voltage_mode(i)) {
>> +				st->pdata->dac[chan->address].mode = i;
>> +				ad5755_clear_dac(indio_dev, chan->address);
>> +				return ad5755_setup_dac(indio_dev, chan->address);
>> +			}
>> +		}
>> +		return -EINVAL;
>> +	case IIO_CHAN_INFO_MODE:
>> +		if (val2 != 0)
>> +			return -EINVAL;
>> +		if (val == AD5755_VOLTAGE_MODE)
> This looks like magic values to me.
> If we did end up with such an interface, you'd want to be using the
> extended attribute stuff and the enum support it has to give
> meaningful names to these.
>
>> +			st->pdata->dac[chan->address].mode = AD5755_MODE_VOLTAGE_0V_5V;
>> +		else
>> +			st->pdata->dac[chan->address].mode = AD5755_MODE_CURRENT_0mA_20mA;
>> +		ad5755_clear_dac(indio_dev, chan->address);
>> +
>> +		return ad5755_setup_dac(indio_dev, chan->address);
>> +		break;
>> +	default:
>> +		ret = ad5755_chan_reg_info(st, chan, info, true,
>> +					   &reg, &shift, &offset);
>> +		if (ret)
>> +			return ret;
>> +
>> +		val <<= shift;
>> +		val += offset;
>> +
>> +		if (val < 0 || val > 0xffff)
>> +			return -EINVAL;
>> +
>> +		return ad5755_write(indio_dev, reg, val);
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int ad5755_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;
>> +	}
>>   
>> -	return ad5755_write(indio_dev, reg, val);
>> +	return -EINVAL;
>>   }
>>   
>>   static ssize_t ad5755_read_powerdown(struct iio_dev *indio_dev, uintptr_t priv,
>> @@ -371,6 +639,8 @@ static ssize_t ad5755_write_powerdown(struct iio_dev *indio_dev, uintptr_t priv,
>>   static const struct iio_info ad5755_info = {
>>   	.read_raw = ad5755_read_raw,
>>   	.write_raw = ad5755_write_raw,
>> +	.write_raw_get_fmt = &ad5755_write_raw_get_fmt,
>> +	.attrs = &ad5755_attribute_group,
>>   	.driver_module = THIS_MODULE,
>>   };
>>   
>> @@ -391,7 +661,8 @@ static const struct iio_chan_spec_ext_info ad5755_ext_info[] = {
>>   		BIT(IIO_CHAN_INFO_SCALE) |			\
>>   		BIT(IIO_CHAN_INFO_OFFSET) |			\
>>   		BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
>> -		BIT(IIO_CHAN_INFO_CALIBBIAS),			\
>> +		BIT(IIO_CHAN_INFO_CALIBBIAS)  |			\
>> +		BIT(IIO_CHAN_INFO_MODE),			\
>>   	.scan_type = {						\
>>   		.sign = 'u',					\
>>   		.realbits = (_bits),				\
>> @@ -424,27 +695,9 @@ static const struct ad5755_chip_info ad5755_chip_info_tbl[] = {
>>   	},
>>   };
>>   
>> -static bool ad5755_is_valid_mode(struct ad5755_state *st, enum ad5755_mode mode)
>> -{
>> -	switch (mode) {
>> -	case AD5755_MODE_VOLTAGE_0V_5V:
>> -	case AD5755_MODE_VOLTAGE_0V_10V:
>> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
>> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
>> -		return st->chip_info->has_voltage_out;
>> -	case AD5755_MODE_CURRENT_4mA_20mA:
>> -	case AD5755_MODE_CURRENT_0mA_20mA:
>> -	case AD5755_MODE_CURRENT_0mA_24mA:
>> -		return true;
>> -	default:
>> -		return false;
>> -	}
>> -}
>> -
>>   static int ad5755_setup_pdata(struct iio_dev *indio_dev,
>> -			      const struct ad5755_platform_data *pdata)
>> +			      struct ad5755_platform_data *pdata)
>>   {
>> -	struct ad5755_state *st = iio_priv(indio_dev);
>>   	unsigned int val;
>>   	unsigned int i;
>>   	int ret;
>> @@ -479,39 +732,17 @@ static int ad5755_setup_pdata(struct iio_dev *indio_dev,
>>   	}
>>   
>>   	for (i = 0; i < ARRAY_SIZE(pdata->dac); ++i) {
>> -		if (!ad5755_is_valid_mode(st, pdata->dac[i].mode))
>> -			return -EINVAL;
>> -
>> -		val = 0;
>> -		if (!pdata->dac[i].ext_current_sense_resistor)
>> -			val |= AD5755_DAC_INT_CURRENT_SENSE_RESISTOR;
>> -		if (pdata->dac[i].enable_voltage_overrange)
>> -			val |= AD5755_DAC_VOLTAGE_OVERRANGE_EN;
>> -		val |= pdata->dac[i].mode;
>> -
>> -		ret = ad5755_update_dac_ctrl(indio_dev, i, val, 0);
>> +		ret = ad5755_setup_dac(indio_dev, i);
>>   		if (ret < 0)
>>   			return ret;
>> +
>>   	}
>>   
>>   	return 0;
>>   }
>>   
>> -static bool ad5755_is_voltage_mode(enum ad5755_mode mode)
>> -{
>> -	switch (mode) {
>> -	case AD5755_MODE_VOLTAGE_0V_5V:
>> -	case AD5755_MODE_VOLTAGE_0V_10V:
>> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
>> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
>> -		return true;
>> -	default:
>> -		return false;
>> -	}
>> -}
>> -
>>   static int ad5755_init_channels(struct iio_dev *indio_dev,
>> -				const struct ad5755_platform_data *pdata)
>> +				struct ad5755_platform_data *pdata)
>>   {
>>   	struct ad5755_state *st = iio_priv(indio_dev);
>>   	struct iio_chan_spec *channels = st->channels;
>> @@ -521,8 +752,8 @@ static int ad5755_init_channels(struct iio_dev *indio_dev,
>>   		channels[i] = st->chip_info->channel_template;
>>   		channels[i].channel = i;
>>   		channels[i].address = i;
>> -		if (pdata && ad5755_is_voltage_mode(pdata->dac[i].mode))
>> -			channels[i].type = IIO_VOLTAGE;
>> +		if (st->chip_info->has_voltage_out)
>> +			channels[i].type = IIO_DUAL_VOLTCUR;
>>   		else
>>   			channels[i].type = IIO_CURRENT;
>>   	}
>> @@ -543,7 +774,7 @@ static int ad5755_init_channels(struct iio_dev *indio_dev,
>>   		}, \
>>   	}
>>   
>> -static const struct ad5755_platform_data ad5755_default_pdata = {
>> +struct ad5755_platform_data ad5755_default_pdata = {
>>   	.ext_dc_dc_compenstation_resistor = false,
>>   	.dc_dc_phase = AD5755_DC_DC_PHASE_ALL_SAME_EDGE,
>>   	.dc_dc_freq = AD5755_DC_DC_FREQ_410kHZ,
>> @@ -559,7 +790,7 @@ static const struct ad5755_platform_data ad5755_default_pdata = {
>>   static int ad5755_probe(struct spi_device *spi)
>>   {
>>   	enum ad5755_type type = spi_get_device_id(spi)->driver_data;
>> -	const struct ad5755_platform_data *pdata = dev_get_platdata(&spi->dev);
>> +	struct ad5755_platform_data *pdata = dev_get_platdata(&spi->dev);
>>   	struct iio_dev *indio_dev;
>>   	struct ad5755_state *st;
>>   	int ret;
>> @@ -586,6 +817,8 @@ static int ad5755_probe(struct spi_device *spi)
>>   	if (!pdata)
>>   		pdata = &ad5755_default_pdata;
>>   
>> +	st->pdata = pdata;
>> +
>>   	ret = ad5755_init_channels(indio_dev, pdata);
>>   	if (ret)
>>   		return ret;
>>

Re: [RFC 2/2] iio: ad5755: added support for switching between voltage and current output

[Jonathan Cameron]

On 25 January 2016 15:05:10 GMT+00:00, "Sean Nyekjær" <sean.nyekjaer@prevas.dk> wrote:
>Hi Jonathan
>
>I havn't had any luck to export two outputs for one channel :-(
>Can you point me in the right direction? Maybe I need to modify 
>something in iio-core to allow this...
This is a common thing to do. I think you simply have a bug below as that code will give negative channel numbers to the current channels...
>
>If i'm trying this: I only get voltage export and they are numberet
>from 4
>
>#define AD5755_NUM_CHANNELS 8
>
>static int ad5755_init_channels(struct iio_dev *indio_dev,
>                                 struct ad5755_platform_data *pdata)
>{
>         struct ad5755_state *st = iio_priv(indio_dev);
>         struct iio_chan_spec *channels = st->channels;
>         unsigned int i;
>
>         for (i = 0; i < AD5755_NUM_CHANNELS; ++i) {
>                 channels[i] = st->chip_info->channel_template;
>                 if(i>3) {
I would guess you mean i<=3
>                         channels[i].channel = i;
>                         channels[i].address = i;
>                         channels[i].type = IIO_VOLTAGE;
This will indeed register voltage4 etc as it stands
>                 } else {
>                         channels[i].channel = i-4;
Negative channel numbers not allowed.
>                         channels[i].address = i-4;
>                         channels[i].type = IIO_CURRENT;
>                 }
>         }
>
>         indio_dev->channels = channels;
>
>         return 0;
>}
>
>/Sean
>
>On 2016-01-24 16:49, Jonathan Cameron wrote:
>> On 22/01/16 12:54, Sean Nyekjaer wrote:
>>> DAC ad5755 have both support for voltage and current output, before
>the driver
>>> only had support for switching modes at compile time. Not very
>smart...
>>>
>> This is currently where we possibly disagree.  It was a very
>deliberate decision to
>> do it where it is done. It is way to easy to blow hardware up if the
>wrong
>> range is selected on a multirange DAC and as far as I can see this is
>> effectively the same.
>>
>> It is btw not at compile time, but at boot time of a given piece of
>hardware.
>> It's not as though a kernel for multiple boards will not allow
>different
>> combinations for different boards. A fine distinction of course and
>one
>> that I suspect is causing you trouble because you are dealing with
>external
>> connections and no knowledge of what is beyond them (i.e. a PLC or
>general
>> purpose output board?)
>>
>>
>> Ah, I'll revise this (having dived into the datasheet)
>> what wasn't clear immediately is that the chip puts the voltage and
>current
>> outputs on different pins - but only one is enabled at a time.  This
>makes for
>> a rather different set of circumstances and explains when you might
>> want to allow runtime configuation.  Note however, that this means
>they really
>> are different channels - just ones with some constraints on which
>combinations
>> are enabled at any given time.  The datasheet does say you can tie
>the two
>> together (I guess for use as a flexible PLC output for example) but
>they
>> aren't tied together on the chip so we don't have to care ;)
>>
>> I wonder if what we really need here is a standard way of applying
>platform
>> data based channel restrictions...  This applies to all multirange
>output
>> parts.
>>
>> Requirements:
>>
>> 1) List of 'safe ranges' for a given piece of hardware.
>> 2) In dual parts like this one, list of 'safe ranges' for both
>current and voltage
>> outputs.
>>
>> If people then want to operate in only one mode (so on a board where
>the use is known)
>> then they provide only one entry and that's all the part will be used
>in.
>>
>> If, as I guess you are doing, the range is unknown then the part is
>either powered down
>> until the range is set by userspace, or powered up in the 'safest'
>mode of those listed.
>>
>> Hence if it was unsafe to use it as a current output at all, that
>list would be empty
>> for this part (same with voltage range list) and the driver would
>refuse to put
>> it in that mode at all.
>>
>> So for your usecase you'd simply specify that all ranges are fine. 
>If someone blows
>> external kit up, it's their own fault as we had no way of knowing
>what was safe.
>>
>> Still, here definitely separate current and voltage channels!
>>
>> Jonathan
>>> This patch adds support for switching modes from userspace.
>>>
>>> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
>>> ---
>> As I stated in my last email in response to patch 1 I think
>continuing the
>> discussion on how to handle this here makes more sense...
>>> Register them as two independent channels. One for current and one
>for
>>>   voltage. In a sense the change of measurement type is just a
>different type
>>>   of front end mux like you see on many ADCs.
>> Hmm. Thinking more on this I'm worried that, as with multirange DACs,
>> it might be possible to switch this DAC into a mode that will
>actually
>> damage the hardware.  I note it is also a multirange part and that
>was
>> originaly controlled by platform data.
>>
>> I would thing we either need to restrict the choice to platform data
>only,
>> or add some concept of limiting the resulting ranges in platform
>data.
>>
>> You are in a fairly odd situation if you want to, at runtime switch a
>> given bit of circuitry from one mode to the other.
>>
>> What is your usecase?
>>
>> I really don't like the combined channel as it's either one or the
>other
>> at any given time, not some mixture of the two. Given the simple
>nature
>> of powerdown on channels on this device, here we can just do it by
>allowing
>> only the voltage or the current channel to be powered up at any given
>time.
>>
>> See below for various other comemnts.
>>
>> Lars, any thoughts on this?  You'd probably come across more of these
>> multirange parts than I have.
>>
>> Jonathan
>>
>>> This patch is not done yet :-) I would like to get some feedback of
>my work.
>>> I have tested this patch with an ad5755 and it works.
>>>
>>> So if you have any ideas on how I should progress please give me
>some feedback.
>>>
>>>   drivers/iio/dac/ad5755.c | 405
>+++++++++++++++++++++++++++++++++++++----------
>>>   1 file changed, 319 insertions(+), 86 deletions(-)
>>>
>>> diff --git a/drivers/iio/dac/ad5755.c b/drivers/iio/dac/ad5755.c
>>> index bfb350a..6e7ab3f 100644
>>> --- a/drivers/iio/dac/ad5755.c
>>> +++ b/drivers/iio/dac/ad5755.c
>>> @@ -63,6 +63,9 @@
>>>   #define AD5755_SLEW_RATE_SHIFT			3
>>>   #define AD5755_SLEW_ENABLE			BIT(12)
>>>   
>>> +#define AD5755_VOLTAGE_MODE			0
>>> +#define AD5755_CURRENT_MODE			1
>>> +
>>>   /**
>>>    * struct ad5755_chip_info - chip specific information
>>>    * @channel_template:	channel specification
>>> @@ -91,6 +94,8 @@ struct ad5755_state {
>>>   	unsigned int			ctrl[AD5755_NUM_CHANNELS];
>>>   	struct iio_chan_spec		channels[AD5755_NUM_CHANNELS];
>>>   
>>> +	struct ad5755_platform_data *pdata;
>>> +
>>>   	/*
>>>   	 * DMA (thus cache coherency maintenance) requires the
>>>   	 * transfer buffers to live in their own cache lines.
>>> @@ -109,6 +114,163 @@ enum ad5755_type {
>>>   	ID_AD5737,
>>>   };
>>>   
>>> +struct ad5755_ranges {
>>> +	enum ad5755_mode range;
>>> +	unsigned int scale;
>>> +	int offset;
>>> +};
>>> +
>>> +static const struct ad5755_ranges ad5755_range_def[] = {
>>> +	{
>>> +		.range = AD5755_MODE_VOLTAGE_0V_5V,
>>> +		.scale = 76293945,
>>> +		.offset = 0,
>>> +	}, {
>>> +		.range = AD5755_MODE_VOLTAGE_0V_10V,
>>> +		.scale = 152587890,
>>> +		.offset = 0,
>>> +	}, {
>>> +		.range = AD5755_MODE_VOLTAGE_PLUSMINUS_5V,
>>> +		.scale = 152587890,
>>> +		.offset = -(1 << (16 /*REALBITS*/ - 1)),
>>> +	}, {
>>> +		.range = AD5755_MODE_VOLTAGE_PLUSMINUS_10V,
>>> +		.scale = 305175781,
>>> +		.offset = -(1 << (16 /*REALBITS*/ - 1)),
>>> +	}, {
>>> +		.range = AD5755_MODE_CURRENT_4mA_20mA,
>>> +		.scale = 244140,
>>> +		.offset = 16384,
>>> +	}, {
>>> +		.range = AD5755_MODE_CURRENT_0mA_20mA,
>>> +		.scale = 305175,
>>> +		.offset = 0,
>>> +	}, {
>>> +		.range = AD5755_MODE_CURRENT_0mA_24mA,
>>> +		.scale = 366210,
>>> +		.offset = 0,
>>> +	}
>>> +};
>>> +
>>> +static inline enum ad5755_mode ad5755_get_chan_mode(struct
>ad5755_state *st,
>>> +						    const struct iio_chan_spec
>>> +						    *chan)
>>> +{
>>> +	return st->ctrl[chan->channel] & 7;
>>> +}
>>> +
>>> +static inline int ad5755_get_chan_scale(struct ad5755_state *st,
>>> +					const struct iio_chan_spec *chan)
>>> +{
>>> +	return ad5755_range_def[ad5755_get_chan_mode(st, chan)].scale;
>>> +}
>>> +
>>> +static inline int ad5755_get_chan_offset(struct ad5755_state *st,
>>> +					 const struct iio_chan_spec *chan)
>>> +{
>>> +	return ad5755_range_def[ad5755_get_chan_mode(st, chan)].offset;
>>> +}
>>> +
>>> +static bool ad5755_is_voltage_mode(enum ad5755_mode mode)
>>> +{
>>> +	switch (mode) {
>>> +	case AD5755_MODE_VOLTAGE_0V_5V:
>>> +	case AD5755_MODE_VOLTAGE_0V_10V:
>>> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
>>> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
>>> +		return true;
>>> +	default:
>>> +		return false;
>>> +	}
>>> +}
>>> +
>>> +static ssize_t ad5755_show_voltcur_modes(struct device *dev,
>>> +					 struct device_attribute *attr,
>>> +					 char *buf)
>>> +{
>>> +	return sprintf(buf, "voltage: 0\ncurrent: 1\n");
>> This is just not how it is done.  Value written must = value read
>> (assuming it succeeded and nothing has change it in the meantime).
>>
>> We have the IIO_ENUM stuff in iio.h to handle this common case of
>matching
>> strings to real values.
>>
>> My gut feeling is that, subject to be convinced that we want runtime
>> configuration of current vs voltage output (that it can be prevented
>> from causing hardware damange), that we want to have
>> separate channels for the current and voltage and handle this by
>effectively
>> controlling whether they are enabled or not.  Only allow either
>> the current or the voltage channel to power up at a given time
>> (internally this is presumably what the hardware is doing anyway!)
>>
>>> +}
>>> +
>>> +static ssize_t ad5755_scales(char *buf, bool voltage_mode)
>>> +{
>>> +	int i, len = 0;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
>>> +		if (ad5755_is_voltage_mode(i) != voltage_mode)
>>> +			continue;
>>> +		len += sprintf(buf + len, "0.%09u ", ad5755_range_def[i].scale);
>>> +	}
>>> +	len += sprintf(buf + len, "\n");
>>> +
>>> +	return len;
>>> +}
>>> +
>>> +static ssize_t ad5755_offset(char *buf, bool voltage_mode)
>>> +{
>>> +	int i, len = 0;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
>>> +		if (ad5755_is_voltage_mode(i) != voltage_mode)
>>> +			continue;
>>> +		len += sprintf(buf + len, "%d ", ad5755_range_def[i].offset);
>>> +	}
>>> +	len += sprintf(buf + len, "\n");
>>> +
>>> +	return len;
>>> +}
>>> +
>>> +static ssize_t ad5755_show_voltage_scales(struct device *dev,
>>> +					  struct device_attribute *attr,
>>> +					  char *buf)
>>> +{
>>> +	return ad5755_scales(buf, true);
>>> +}
>>> +
>>> +static ssize_t ad5755_show_voltage_offset(struct device *dev,
>>> +					  struct device_attribute *attr,
>>> +					  char *buf)
>>> +{
>>> +	return ad5755_offset(buf, true);
>>> +}
>>> +
>>> +static ssize_t ad5755_show_current_scales(struct device *dev,
>>> +					  struct device_attribute *attr,
>>> +					  char *buf)
>>> +{
>>> +	return ad5755_scales(buf, false);
>>> +}
>>> +
>>> +static ssize_t ad5755_show_current_offset(struct device *dev,
>>> +					  struct device_attribute *attr,
>>> +					  char *buf)
>>> +{
>>> +	return ad5755_offset(buf, false);
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(out_voltcur_modes_available, S_IRUGO,
>>> +		       ad5755_show_voltcur_modes, NULL, 0);
>>> +static IIO_DEVICE_ATTR(out_voltage_scale_available, S_IRUGO,
>>> +		       ad5755_show_voltage_scales, NULL, 0);
>>> +static IIO_DEVICE_ATTR(out_voltage_offset_available, S_IRUGO,
>>> +		       ad5755_show_voltage_offset, NULL, 0);
>>> +static IIO_DEVICE_ATTR(out_current_scale_available, S_IRUGO,
>>> +		       ad5755_show_current_scales, NULL, 0);
>>> +static IIO_DEVICE_ATTR(out_current_offset_available, S_IRUGO,
>>> +		       ad5755_show_current_offset, NULL, 0);
>>> +
>>> +static struct attribute *ad5755_attributes[] = {
>>> +	&iio_dev_attr_out_voltcur_modes_available.dev_attr.attr,
>>> +	&iio_dev_attr_out_voltage_scale_available.dev_attr.attr,
>>> +	&iio_dev_attr_out_voltage_offset_available.dev_attr.attr,
>>> +	&iio_dev_attr_out_current_scale_available.dev_attr.attr,
>>> +	&iio_dev_attr_out_current_offset_available.dev_attr.attr,
>>> +	NULL,
>>> +};
>>> +
>>> +static const struct attribute_group ad5755_attribute_group = {
>>> +	.attrs = ad5755_attributes,
>>> +};
>>> +
>>>   static int ad5755_write_unlocked(struct iio_dev *indio_dev,
>>>   	unsigned int reg, unsigned int val)
>>>   {
>>> @@ -226,31 +388,54 @@ out_unlock:
>>>   	return 0;
>>>   }
>>>   
>>> -static const int ad5755_min_max_table[][2] = {
>>> -	[AD5755_MODE_VOLTAGE_0V_5V] = { 0, 5000 },
>>> -	[AD5755_MODE_VOLTAGE_0V_10V] = { 0, 10000 },
>>> -	[AD5755_MODE_VOLTAGE_PLUSMINUS_5V] = { -5000, 5000 },
>>> -	[AD5755_MODE_VOLTAGE_PLUSMINUS_10V] = { -10000, 10000 },
>>> -	[AD5755_MODE_CURRENT_4mA_20mA] = { 4, 20 },
>>> -	[AD5755_MODE_CURRENT_0mA_20mA] = { 0, 20 },
>>> -	[AD5755_MODE_CURRENT_0mA_24mA] = { 0, 24 },
>>> -};
>>> +static bool ad5755_is_valid_mode(struct ad5755_state *st, enum
>ad5755_mode mode)
>>> +{
>>> +	switch (mode) {
>>> +	case AD5755_MODE_VOLTAGE_0V_5V:
>>> +	case AD5755_MODE_VOLTAGE_0V_10V:
>>> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
>>> +	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
>>> +		return st->chip_info->has_voltage_out;
>>> +	case AD5755_MODE_CURRENT_4mA_20mA:
>>> +	case AD5755_MODE_CURRENT_0mA_20mA:
>>> +	case AD5755_MODE_CURRENT_0mA_24mA:
>>> +		return true;
>>> +	default:
>>> +		return false;
>>> +	}
>>> +}
>>>   
>>> -static void ad5755_get_min_max(struct ad5755_state *st,
>>> -	struct iio_chan_spec const *chan, int *min, int *max)
>>> +static int ad5755_clear_dac(struct iio_dev *indio_dev, int channel)
>>>   {
>>> -	enum ad5755_mode mode = st->ctrl[chan->channel] & 7;
>>> -	*min = ad5755_min_max_table[mode][0];
>>> -	*max = ad5755_min_max_table[mode][1];
>>> +	int i = channel;
>>> +	int ret;
>>> +
>>> +	ret = ad5755_update_dac_ctrl(indio_dev, i, 0, UINT_MAX);
>>> +	return ret;
>>> +
>>>   }
>>>   
>>> -static inline int ad5755_get_offset(struct ad5755_state *st,
>>> -	struct iio_chan_spec const *chan)
>>> +static int ad5755_setup_dac(struct iio_dev *indio_dev, int channel)
>>>   {
>>> -	int min, max;
>>> +	int i = channel;
>>> +	int ret;
>>> +	struct ad5755_state *st = iio_priv(indio_dev);
>>> +	unsigned int val;
>>> +	struct ad5755_platform_data *pdata = st->pdata;
>>> +
>>> +	if (!ad5755_is_valid_mode(st, pdata->dac[i].mode))
>>> +		return -EINVAL;
>>> +
>>> +	val = 0;
>>> +	if (!pdata->dac[i].ext_current_sense_resistor)
>>> +		val |= AD5755_DAC_INT_CURRENT_SENSE_RESISTOR;
>>> +	if (pdata->dac[i].enable_voltage_overrange)
>>> +		val |= AD5755_DAC_VOLTAGE_OVERRANGE_EN;
>>> +	val |= pdata->dac[i].mode;
>>> +
>>> +	ret = ad5755_update_dac_ctrl(indio_dev, i, val, 0);
>>> +	return ret;
>>>   
>>> -	ad5755_get_min_max(st, chan, &min, &max);
>>> -	return (min * (1 << chan->scan_type.realbits)) / (max - min);
>>>   }
>>>   
>>>   static int ad5755_chan_reg_info(struct ad5755_state *st,
>>> @@ -294,18 +479,25 @@ static int ad5755_read_raw(struct iio_dev
>*indio_dev,
>>>   {
>>>   	struct ad5755_state *st = iio_priv(indio_dev);
>>>   	unsigned int reg, shift, offset;
>>> -	int min, max;
>>>   	int ret;
>>>   
>>>   	switch (info) {
>>>   	case IIO_CHAN_INFO_SCALE:
>>> -		ad5755_get_min_max(st, chan, &min, &max);
>>> -		*val = max - min;
>>> -		*val2 = chan->scan_type.realbits;
>>> -		return IIO_VAL_FRACTIONAL_LOG2;
>>> +		*val = 0;
>>> +		*val2 = ad5755_get_chan_scale(st, chan);
>>> +		return IIO_VAL_INT_PLUS_NANO;
>>>   	case IIO_CHAN_INFO_OFFSET:
>>> -		*val = ad5755_get_offset(st, chan);
>>> +		*val = ad5755_get_chan_offset(st, chan);
>>>   		return IIO_VAL_INT;
>>> +	case IIO_CHAN_INFO_MODE:
>>> +		if (ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode)) {
>>> +			*val = AD5755_VOLTAGE_MODE;
>>> +			return IIO_VAL_INT;
>>> +		} else {
>>> +			*val = AD5755_CURRENT_MODE;
>>> +			return IIO_VAL_INT;
>>> +		}
>>> +		break;
>>>   	default:
>>>   		ret = ad5755_chan_reg_info(st, chan, info, false,
>>>   						&reg, &shift, &offset);
>>> @@ -325,24 +517,100 @@ static int ad5755_read_raw(struct iio_dev
>*indio_dev,
>>>   }
>>>   
>>>   static int ad5755_write_raw(struct iio_dev *indio_dev,
>>> -	const struct iio_chan_spec *chan, int val, int val2, long info)
>>> +			    const struct iio_chan_spec *chan, int val, int val2,
>>> +			    long info)
>> This is just reformatting. Might be worthwhile, but should be in a
>separate
>> patch.
>>>   {
>>>   	struct ad5755_state *st = iio_priv(indio_dev);
>>> -	unsigned int shift, reg, offset;
>>> -	int ret;
>>> +	unsigned int shift, reg;
>>> +	bool is_voltage;
>>> +	int offset;
>>> +	unsigned int scale;
>>> +	int ret, i;
>>>   
>>> -	ret = ad5755_chan_reg_info(st, chan, info, true,
>>> -					&reg, &shift, &offset);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>> -	val <<= shift;
>>> -	val += offset;
>>> +	switch (info) {
>>> +	case IIO_CHAN_INFO_SCALE:
>>> +	case IIO_CHAN_INFO_OFFSET:
>>> +	case IIO_CHAN_INFO_MODE:
>>> +		if (!(bool) (st->pwr_down & (1 << chan->channel))) {
>>> +			printk
>>> +			    ("POWER DOWN off - Power down before shifting modes\n");
>>> +			return -EPERM;
>>> +		}
>>> +	}
>>>   
>>> -	if (val < 0 || val > 0xffff)
>>> +	switch (info) {
>>> +	case IIO_CHAN_INFO_SCALE:
>>> +		is_voltage =
>>> +		    ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode);
>>> +		offset =
>>> +		    ad5755_range_def[st->pdata->dac[chan->address].mode].offset;
>>> +		/* is new scale allowed */
>>> +		for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
>>> +			if (val2 == ad5755_range_def[i].scale &&
>>> +			    offset == ad5755_range_def[i].offset &&
>>> +			    is_voltage == ad5755_is_voltage_mode(i)) {
>>> +				st->pdata->dac[chan->address].mode = i;
>>> +				ad5755_clear_dac(indio_dev, chan->address);
>>> +				return ad5755_setup_dac(indio_dev, chan->address);
>>> +			}
>>> +		}
>>>   		return -EINVAL;
>>> +	case IIO_CHAN_INFO_OFFSET:
>>> +		is_voltage =
>>> +		    ad5755_is_voltage_mode(st->pdata->dac[chan->address].mode);
>>> +		scale =
>>> +		    ad5755_range_def[st->pdata->dac[chan->address].mode].scale;
>>> +		/* is new offset allowed */
>>> +		for (i = 0; i < ARRAY_SIZE(ad5755_range_def); i++) {
>>> +			if (val == ad5755_range_def[i].offset &&
>>> +			    scale == ad5755_range_def[i].scale &&
>>> +			    is_voltage == ad5755_is_voltage_mode(i)) {
>>> +				st->pdata->dac[chan->address].mode = i;
>>> +				ad5755_clear_dac(indio_dev, chan->address);
>>> +				return ad5755_setup_dac(indio_dev, chan->address);
>>> +			}
>>> +		}
>>> +		return -EINVAL;
>>> +	case IIO_CHAN_INFO_MODE:
>>> +		if (val2 != 0)
>>> +			return -EINVAL;
>>> +		if (val == AD5755_VOLTAGE_MODE)
>> This looks like magic values to me.
>> If we did end up with such an interface, you'd want to be using the
>> extended attribute stuff and the enum support it has to give
>> meaningful names to these.
>>
>>> +			st->pdata->dac[chan->address].mode = AD5755_MODE_VOLTAGE_0V_5V;
>>> +		else
>>> +			st->pdata->dac[chan->address].mode =
>AD5755_MODE_CURRENT_0mA_20mA;
>>> +		ad5755_clear_dac(indio_dev, chan->address);
>>> +
>>> +		return ad5755_setup_dac(indio_dev, chan->address);
>>> +		break;
>>> +	default:
>>> +		ret = ad5755_chan_reg_info(st, chan, info, true,
>>> +					   &reg, &shift, &offset);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		val <<= shift;
>>> +		val += offset;
>>> +
>>> +		if (val < 0 || val > 0xffff)
>>> +			return -EINVAL;
>>> +
>>> +		return ad5755_write(indio_dev, reg, val);
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static int ad5755_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;
>>> +	}
>>>   
>>> -	return ad5755_write(indio_dev, reg, val);
>>> +	return -EINVAL;
>>>   }
>>>   
>>>   static ssize_t ad5755_read_powerdown(struct iio_dev *indio_dev,
>uintptr_t priv,
>>> @@ -371,6 +639,8 @@ static ssize_t ad5755_write_powerdown(struct
>iio_dev *indio_dev, uintptr_t priv,
>>>   static const struct iio_info ad5755_info = {
>>>   	.read_raw = ad5755_read_raw,
>>>   	.write_raw = ad5755_write_raw,
>>> +	.write_raw_get_fmt = &ad5755_write_raw_get_fmt,
>>> +	.attrs = &ad5755_attribute_group,
>>>   	.driver_module = THIS_MODULE,
>>>   };
>>>   
>>> @@ -391,7 +661,8 @@ static const struct iio_chan_spec_ext_info
>ad5755_ext_info[] = {
>>>   		BIT(IIO_CHAN_INFO_SCALE) |			\
>>>   		BIT(IIO_CHAN_INFO_OFFSET) |			\
>>>   		BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
>>> -		BIT(IIO_CHAN_INFO_CALIBBIAS),			\
>>> +		BIT(IIO_CHAN_INFO_CALIBBIAS)  |			\
>>> +		BIT(IIO_CHAN_INFO_MODE),			\
>>>   	.scan_type = {						\
>>>   		.sign = 'u',					\
>>>   		.realbits = (_bits),				\
>>> @@ -424,27 +695,9 @@ static const struct ad5755_chip_info
>ad5755_chip_info_tbl[] = {
>>>   	},
>>>   };
>>>   
>>> -static bool ad5755_is_valid_mode(struct ad5755_state *st, enum
>ad5755_mode mode)
>>> -{
>>> -	switch (mode) {
>>> -	case AD5755_MODE_VOLTAGE_0V_5V:
>>> -	case AD5755_MODE_VOLTAGE_0V_10V:
>>> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
>>> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
>>> -		return st->chip_info->has_voltage_out;
>>> -	case AD5755_MODE_CURRENT_4mA_20mA:
>>> -	case AD5755_MODE_CURRENT_0mA_20mA:
>>> -	case AD5755_MODE_CURRENT_0mA_24mA:
>>> -		return true;
>>> -	default:
>>> -		return false;
>>> -	}
>>> -}
>>> -
>>>   static int ad5755_setup_pdata(struct iio_dev *indio_dev,
>>> -			      const struct ad5755_platform_data *pdata)
>>> +			      struct ad5755_platform_data *pdata)
>>>   {
>>> -	struct ad5755_state *st = iio_priv(indio_dev);
>>>   	unsigned int val;
>>>   	unsigned int i;
>>>   	int ret;
>>> @@ -479,39 +732,17 @@ static int ad5755_setup_pdata(struct iio_dev
>*indio_dev,
>>>   	}
>>>   
>>>   	for (i = 0; i < ARRAY_SIZE(pdata->dac); ++i) {
>>> -		if (!ad5755_is_valid_mode(st, pdata->dac[i].mode))
>>> -			return -EINVAL;
>>> -
>>> -		val = 0;
>>> -		if (!pdata->dac[i].ext_current_sense_resistor)
>>> -			val |= AD5755_DAC_INT_CURRENT_SENSE_RESISTOR;
>>> -		if (pdata->dac[i].enable_voltage_overrange)
>>> -			val |= AD5755_DAC_VOLTAGE_OVERRANGE_EN;
>>> -		val |= pdata->dac[i].mode;
>>> -
>>> -		ret = ad5755_update_dac_ctrl(indio_dev, i, val, 0);
>>> +		ret = ad5755_setup_dac(indio_dev, i);
>>>   		if (ret < 0)
>>>   			return ret;
>>> +
>>>   	}
>>>   
>>>   	return 0;
>>>   }
>>>   
>>> -static bool ad5755_is_voltage_mode(enum ad5755_mode mode)
>>> -{
>>> -	switch (mode) {
>>> -	case AD5755_MODE_VOLTAGE_0V_5V:
>>> -	case AD5755_MODE_VOLTAGE_0V_10V:
>>> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_5V:
>>> -	case AD5755_MODE_VOLTAGE_PLUSMINUS_10V:
>>> -		return true;
>>> -	default:
>>> -		return false;
>>> -	}
>>> -}
>>> -
>>>   static int ad5755_init_channels(struct iio_dev *indio_dev,
>>> -				const struct ad5755_platform_data *pdata)
>>> +				struct ad5755_platform_data *pdata)
>>>   {
>>>   	struct ad5755_state *st = iio_priv(indio_dev);
>>>   	struct iio_chan_spec *channels = st->channels;
>>> @@ -521,8 +752,8 @@ static int ad5755_init_channels(struct iio_dev
>*indio_dev,
>>>   		channels[i] = st->chip_info->channel_template;
>>>   		channels[i].channel = i;
>>>   		channels[i].address = i;
>>> -		if (pdata && ad5755_is_voltage_mode(pdata->dac[i].mode))
>>> -			channels[i].type = IIO_VOLTAGE;
>>> +		if (st->chip_info->has_voltage_out)
>>> +			channels[i].type = IIO_DUAL_VOLTCUR;
>>>   		else
>>>   			channels[i].type = IIO_CURRENT;
>>>   	}
>>> @@ -543,7 +774,7 @@ static int ad5755_init_channels(struct iio_dev
>*indio_dev,
>>>   		}, \
>>>   	}
>>>   
>>> -static const struct ad5755_platform_data ad5755_default_pdata = {
>>> +struct ad5755_platform_data ad5755_default_pdata = {
>>>   	.ext_dc_dc_compenstation_resistor = false,
>>>   	.dc_dc_phase = AD5755_DC_DC_PHASE_ALL_SAME_EDGE,
>>>   	.dc_dc_freq = AD5755_DC_DC_FREQ_410kHZ,
>>> @@ -559,7 +790,7 @@ static const struct ad5755_platform_data
>ad5755_default_pdata = {
>>>   static int ad5755_probe(struct spi_device *spi)
>>>   {
>>>   	enum ad5755_type type = spi_get_device_id(spi)->driver_data;
>>> -	const struct ad5755_platform_data *pdata =
>dev_get_platdata(&spi->dev);
>>> +	struct ad5755_platform_data *pdata = dev_get_platdata(&spi->dev);
>>>   	struct iio_dev *indio_dev;
>>>   	struct ad5755_state *st;
>>>   	int ret;
>>> @@ -586,6 +817,8 @@ static int ad5755_probe(struct spi_device *spi)
>>>   	if (!pdata)
>>>   		pdata = &ad5755_default_pdata;
>>>   
>>> +	st->pdata = pdata;
>>> +
>>>   	ret = ad5755_init_channels(indio_dev, pdata);
>>>   	if (ret)
>>>   		return ret;
>>>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [RFC 2/2] iio: ad5755: added support for switching between voltage and current output

[Lars-Peter Clausen]

On 01/22/2016 01:54 PM, Sean Nyekjaer wrote:
> DAC ad5755 have both support for voltage and current output, before the driver
> only had support for switching modes at compile time. Not very smart...
> 
> This patch adds support for switching modes from userspace.
> 
> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
> ---
> 
> This patch is not done yet :-) I would like to get some feedback of my work.
> I have tested this patch with an ad5755 and it works.
> 
> So if you have any ideas on how I should progress please give me some feedback.

Hi,

I'd like to better understand why this is necessary (or not). What is your
usecase where you need to support switching at runtime?

- Lars

Re: [RFC 2/2] iio: ad5755: added support for switching between voltage and current output

[Sean Nyekjær]

On 2016-01-25 17:00, Lars-Peter Clausen wrote:
> On 01/22/2016 01:54 PM, Sean Nyekjaer wrote:
>> DAC ad5755 have both support for voltage and current output, before the driver
>> only had support for switching modes at compile time. Not very smart...
>>
>> This patch adds support for switching modes from userspace.
>>
>> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
>> ---
>>
>> This patch is not done yet :-) I would like to get some feedback of my work.
>> I have tested this patch with an ad5755 and it works.
>>
>> So if you have any ideas on how I should progress please give me some feedback.
> Hi,
>
> I'd like to better understand why this is necessary (or not). What is your
> usecase where you need to support switching at runtime?
>
> - Lars
>
Hi Lars

Our customer is building an IO board, where they are going to loop some 
-15-15V or 0-24mA signals thru their board and manipulate them. We are 
using an ti-ads8688 for input and an ad5755 for output :-)
In our case it's necessary because our costumer does not know about the 
input signals therefore we have to give them the posibility to switch 
between the modes.
Yes we could just provide them with 2 voltage modes and 2 current mode 
or two different boardfiles, but they have declined that proposal :-(

/Sean

Re: [RFC 1/2] iio: core: added support for a single output that supports both current and voltage

[Sean Nyekjær]

Hi

How do you register two channels for one channel?
Please give a quick example

/Sean

On 2016-01-22 15:14, Jonathan Cameron wrote:
> Hi
>
> Quick gut feel response. Register two channels.
>
> Jonathan
>
> On 22 January 2016 12:54:55 GMT+00:00, Sean Nyekjaer 
> <sean.nyekjaer@prevas.dk> wrote:
>
>     Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
>     ---
>       drivers/iio/industrialio-core.c | 2 ++
>       include/linux/iio/iio.h         | 1 +
>       include/uapi/linux/iio/types.h  | 1 +
>       3 files changed, 4 insertions(+)
>
>     diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>     index fd01f34..5e1beea 100644
>     --- a/drivers/iio/industrialio-core.c
>     +++ b/drivers/iio/industrialio-core.c
>     @@ -77,6 +77,7 @@ static const char * const iio_chan_type_name_spec[] = {
>        [IIO_VELOCITY] = "velocity",
>        [IIO_CONCENTRATION] = "concentration",
>        [IIO_RESISTANCE] = "resistance",
>     + [IIO_DUAL_VOLTCUR] = "voltcur",
>       };
>       
>       static const char * const iio_modifier_names[] = {
>     @@ -146,6 +147,7 @@ static const char * const iio_chan_info_postfix[] = {
>        [IIO_CHAN_INFO_DEBOUNCE_TIME] = "debounce_time",
>        [IIO_CHAN_INFO_CALIBEMISSIVITY] = "calibemissivity",
>       
>     [IIO_CHAN_INFO_OVERSAMPLING_RATIO] = "oversampling_ratio",
>     + [IIO_CHAN_INFO_MODE] = "mode",
>       };
>       
>       /**
>     diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>     index b589411..ef4c7cd9 100644
>     --- a/include/linux/iio/iio.h
>     +++ b/include/linux/iio/iio.h
>     @@ -46,6 +46,7 @@ enum iio_chan_info_enum {
>        IIO_CHAN_INFO_DEBOUNCE_TIME,
>        IIO_CHAN_INFO_CALIBEMISSIVITY,
>        IIO_CHAN_INFO_OVERSAMPLING_RATIO,
>     + IIO_CHAN_INFO_MODE,
>       };
>       
>       enum iio_shared_by {
>     diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
>     index 7c63bd6..81e6a2a 100644
>     --- a/include/uapi/linux/iio/types.h
>     +++ b/include/uapi/linux/iio/types.h
>     @@ -37,6 +37,7 @@ enum iio_chan_type {
>        IIO_VELOCITY,
>        IIO_CONCENTRATION,
>        IIO_RESISTANCE,
>     + IIO_DUAL_VOLTCUR,
>       };
>       
>       enum iio_modifier {
>
>
> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity. 

Re: [RFC 1/2] iio: core: added support for a single output that supports both current and voltage

[Jonathan Cameron]

On 22 January 2016 14:37:33 GMT+00:00, "Sean Nyekjær" <sean.nyekjaer@prevas.dk> wrote:
>Hi
>
>How do you register two channels for one channel?

Register them as two independent channels. One for current and one for
 voltage. In a sense the change of measurement type is just a different type
 of front end mux like you see on many ADCs.


>Please give a quick example
Phone only so will get back to you tomorrow!

>
>/Sean
>
>On 2016-01-22 15:14, Jonathan Cameron wrote:
>> Hi
>>
>> Quick gut feel response. Register two channels.
>>
>> Jonathan
>>
>> On 22 January 2016 12:54:55 GMT+00:00, Sean Nyekjaer 
>> <sean.nyekjaer@prevas.dk> wrote:
>>
>>     Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
>>     ---
>>       drivers/iio/industrialio-core.c | 2 ++
>>       include/linux/iio/iio.h         | 1 +
>>       include/uapi/linux/iio/types.h  | 1 +
>>       3 files changed, 4 insertions(+)
>>
>>     diff --git a/drivers/iio/industrialio-core.c
>b/drivers/iio/industrialio-core.c
>>     index fd01f34..5e1beea 100644
>>     --- a/drivers/iio/industrialio-core.c
>>     +++ b/drivers/iio/industrialio-core.c
>>     @@ -77,6 +77,7 @@ static const char * const
>iio_chan_type_name_spec[] = {
>>        [IIO_VELOCITY] = "velocity",
>>        [IIO_CONCENTRATION] = "concentration",
>>        [IIO_RESISTANCE] = "resistance",
>>     + [IIO_DUAL_VOLTCUR] = "voltcur",
>>       };
>>       
>>       static const char * const iio_modifier_names[] = {
>>     @@ -146,6 +147,7 @@ static const char * const
>iio_chan_info_postfix[] = {
>>        [IIO_CHAN_INFO_DEBOUNCE_TIME] = "debounce_time",
>>        [IIO_CHAN_INFO_CALIBEMISSIVITY] = "calibemissivity",
>>       
>>     [IIO_CHAN_INFO_OVERSAMPLING_RATIO] = "oversampling_ratio",
>>     + [IIO_CHAN_INFO_MODE] = "mode",
>>       };
>>       
>>       /**
>>     diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>     index b589411..ef4c7cd9 100644
>>     --- a/include/linux/iio/iio.h
>>     +++ b/include/linux/iio/iio.h
>>     @@ -46,6 +46,7 @@ enum iio_chan_info_enum {
>>        IIO_CHAN_INFO_DEBOUNCE_TIME,
>>        IIO_CHAN_INFO_CALIBEMISSIVITY,
>>        IIO_CHAN_INFO_OVERSAMPLING_RATIO,
>>     + IIO_CHAN_INFO_MODE,
>>       };
>>       
>>       enum iio_shared_by {
>>     diff --git a/include/uapi/linux/iio/types.h
>b/include/uapi/linux/iio/types.h
>>     index 7c63bd6..81e6a2a 100644
>>     --- a/include/uapi/linux/iio/types.h
>>     +++ b/include/uapi/linux/iio/types.h
>>     @@ -37,6 +37,7 @@ enum iio_chan_type {
>>        IIO_VELOCITY,
>>        IIO_CONCENTRATION,
>>        IIO_RESISTANCE,
>>     + IIO_DUAL_VOLTCUR,
>>       };
>>       
>>       enum iio_modifier {
>>
>>
>> -- 
>> Sent from my Android device with K-9 Mail. Please excuse my brevity. 
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [RFC 1/2] iio: core: added support for a single output that supports both current and voltage

[Jonathan Cameron]

On 22/01/16 16:49, Jonathan Cameron wrote:
> 
> 
> On 22 January 2016 14:37:33 GMT+00:00, "Sean Nyekjær" <sean.nyekjaer@prevas.dk> wrote:
>> Hi
>>
>> How do you register two channels for one channel?
I got this a bit backwards reading it on the phone and thought we were dealing
with an ADC rather than a DAC (oops!) I'm going to move the rest of this
discussion into the reply to patch 2 as it will make more sense alongside
your usecase there.

<snip>
> 
> 
>> Please give a quick example
> Phone only so will get back to you tomorrow!
> 
>>
>> /Sean
>>
>> On 2016-01-22 15:14, Jonathan Cameron wrote:
>>> Hi
>>>
>>> Quick gut feel response. Register two channels.
>>>
>>> Jonathan
>>>
>>> On 22 January 2016 12:54:55 GMT+00:00, Sean Nyekjaer 
>>> <sean.nyekjaer@prevas.dk> wrote:
>>>
>>>     Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
>>>     ---
>>>       drivers/iio/industrialio-core.c | 2 ++
>>>       include/linux/iio/iio.h         | 1 +
>>>       include/uapi/linux/iio/types.h  | 1 +
>>>       3 files changed, 4 insertions(+)
>>>
>>>     diff --git a/drivers/iio/industrialio-core.c
>> b/drivers/iio/industrialio-core.c
>>>     index fd01f34..5e1beea 100644
>>>     --- a/drivers/iio/industrialio-core.c
>>>     +++ b/drivers/iio/industrialio-core.c
>>>     @@ -77,6 +77,7 @@ static const char * const
>> iio_chan_type_name_spec[] = {
>>>        [IIO_VELOCITY] = "velocity",
>>>        [IIO_CONCENTRATION] = "concentration",
>>>        [IIO_RESISTANCE] = "resistance",
>>>     + [IIO_DUAL_VOLTCUR] = "voltcur",
>>>       };
>>>       
>>>       static const char * const iio_modifier_names[] = {
>>>     @@ -146,6 +147,7 @@ static const char * const
>> iio_chan_info_postfix[] = {
>>>        [IIO_CHAN_INFO_DEBOUNCE_TIME] = "debounce_time",
>>>        [IIO_CHAN_INFO_CALIBEMISSIVITY] = "calibemissivity",
>>>       
>>>     [IIO_CHAN_INFO_OVERSAMPLING_RATIO] = "oversampling_ratio",
>>>     + [IIO_CHAN_INFO_MODE] = "mode",
>>>       };
>>>       
>>>       /**
>>>     diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>>     index b589411..ef4c7cd9 100644
>>>     --- a/include/linux/iio/iio.h
>>>     +++ b/include/linux/iio/iio.h
>>>     @@ -46,6 +46,7 @@ enum iio_chan_info_enum {
>>>        IIO_CHAN_INFO_DEBOUNCE_TIME,
>>>        IIO_CHAN_INFO_CALIBEMISSIVITY,
>>>        IIO_CHAN_INFO_OVERSAMPLING_RATIO,
>>>     + IIO_CHAN_INFO_MODE,
>>>       };
>>>       
>>>       enum iio_shared_by {
>>>     diff --git a/include/uapi/linux/iio/types.h
>> b/include/uapi/linux/iio/types.h
>>>     index 7c63bd6..81e6a2a 100644
>>>     --- a/include/uapi/linux/iio/types.h
>>>     +++ b/include/uapi/linux/iio/types.h
>>>     @@ -37,6 +37,7 @@ enum iio_chan_type {
>>>        IIO_VELOCITY,
>>>        IIO_CONCENTRATION,
>>>        IIO_RESISTANCE,
>>>     + IIO_DUAL_VOLTCUR,
>>>       };
>>>       
>>>       enum iio_modifier {
>>>
>>>
>>> -- 
>>> Sent from my Android device with K-9 Mail. Please excuse my brevity. 
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>