[PATCH 1/5] staging:iio: Add extended IIO channel info

[PATCH 1/5] staging:iio: Add extended IIO channel info

[Lars-Peter Clausen]

Sometimes devices have per channel properties which either do not map nicely to
the current channel info scheme (e.g. string properties) or are very device
specific, so it does not make sense to add generic support for them.

Currently drivers define these attributes by hand for each channel. Depending on
the number of channels this can amount to quite a few lines of boilerplate code.
Especially if a driver supports multiple variations of a chip with different
numbers of channels. In this case it becomes necessary to have a individual
attribute list per chip variation and also a individual iio_info struct.

This patch introduces a new scheme for handling such per channel attributes
called extended channel info attributes. A extended channel info attribute
consist of a name, a flag whether it is shared and read and write callbacks.
The read and write callbacks are similar to the {read,write}_raw callbacks and
take a IIO device and a channel as their first parameters, but instead of
pre-parsed integer values they directly get passed the raw string value, which
has been written to the sysfs file.

It is possible to assign a list of extended channel info attributes to a
channel. For each extended channel info attribute the IIO core will create a new
sysfs attribute conforming to the IIO channel naming spec for the channels type,
similar as for normal info attributes. Read and write access to this sysfs
attribute will be redirected to the extended channel info attributes read and
write callbacks.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/iio.h               |   23 ++++++++++++
 drivers/staging/iio/industrialio-core.c |   61 ++++++++++++++++++++++++++++---
 2 files changed, 78 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
index be6ced3..2a0cfbb 100644
--- a/drivers/staging/iio/iio.h
+++ b/drivers/staging/iio/iio.h
@@ -88,6 +88,25 @@ enum iio_endian {
 	IIO_LE,
 };
 
+struct iio_chan_spec;
+struct iio_dev;
+
+/**
+ * struct iio_chan_spec_ext_info - Extended channel info attribute
+ * @name:	Info attribute name
+ * @shared:	Whether this attribute is shared between all channels.
+ * @read:	Read callback for this info attribute, may be NULL.
+ * @write:	Write callback for this info attribute, may be NULL.
+ */
+struct iio_chan_spec_ext_info {
+	const char *name;
+	bool shared;
+	ssize_t (*read)(struct iio_dev *, struct iio_chan_spec const *,
+			char *buf);
+	ssize_t (*write)(struct iio_dev *, struct iio_chan_spec const *,
+			const char *buf, size_t len);
+};
+
 /**
  * struct iio_chan_spec - specification of a single channel
  * @type:		What type of measurement is the channel making.
@@ -107,6 +126,9 @@ enum iio_endian {
  * @info_mask:		What information is to be exported about this channel.
  *			This includes calibbias, scale etc.
  * @event_mask:	What events can this channel produce.
+ * @ext_info:		List of extended info attributes for this channel.
+ *			The list is NULL terminated, the last element should
+ *			have it's name field set to NULL.
  * @extend_name:	Allows labeling of channel attributes with an
  *			informative name. Note this has no effect codes etc,
  *			unlike modifiers.
@@ -141,6 +163,7 @@ struct iio_chan_spec {
 	} scan_type;
 	long			info_mask;
 	long			event_mask;
+	const struct iio_chan_spec_ext_info *ext_info;
 	char			*extend_name;
 	const char		*datasheet_name;
 	unsigned		processed_val:1;
diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
index e4824fe..a02b6ec 100644
--- a/drivers/staging/iio/industrialio-core.c
+++ b/drivers/staging/iio/industrialio-core.c
@@ -144,6 +144,33 @@ static void __exit iio_exit(void)
 	bus_unregister(&iio_bus_type);
 }
 
+static ssize_t iio_read_channel_ext_info(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	const struct iio_chan_spec_ext_info *ext_info;
+
+	ext_info = &this_attr->c->ext_info[this_attr->address];
+
+	return ext_info->read(indio_dev, this_attr->c, buf);
+}
+
+static ssize_t iio_write_channel_ext_info(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf,
+					 size_t len)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	const struct iio_chan_spec_ext_info *ext_info;
+
+	ext_info = &this_attr->c->ext_info[this_attr->address];
+
+	return ext_info->write(indio_dev, this_attr->c, buf, len);
+}
+
 static ssize_t iio_read_channel_info(struct device *dev,
 				     struct device_attribute *attr,
 				     char *buf)
@@ -401,10 +428,13 @@ int __iio_add_chan_devattr(const char *postfix,
 	list_for_each_entry(t, attr_list, l)
 		if (strcmp(t->dev_attr.attr.name,
 			   iio_attr->dev_attr.attr.name) == 0) {
-			if (!generic)
+			if (!generic) {
 				dev_err(dev, "tried to double register : %s\n",
 					t->dev_attr.attr.name);
-			ret = -EBUSY;
+				ret = -EBUSY;
+			} else {
+				ret = 0;
+			}
 			goto error_device_attr_deinit;
 		}
 	list_add(&iio_attr->l, attr_list);
@@ -423,6 +453,7 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
 					struct iio_chan_spec const *chan)
 {
 	int ret, i, attrcount = 0;
+	const struct iio_chan_spec_ext_info *ext_info;
 
 	if (chan->channel < 0)
 		return 0;
@@ -449,14 +480,32 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
 					     !(i%2),
 					     &indio_dev->dev,
 					     &indio_dev->channel_attr_list);
-		if (ret == -EBUSY && (i%2 == 0)) {
-			ret = 0;
-			continue;
-		}
 		if (ret < 0)
 			goto error_ret;
 		attrcount++;
 	}
+
+	if (chan->ext_info) {
+		unsigned int i = 0;
+		for (ext_info = chan->ext_info; ext_info->name; ext_info++) {
+			ret = __iio_add_chan_devattr(ext_info->name,
+					chan,
+					ext_info->read ?
+					    &iio_read_channel_ext_info : NULL,
+					ext_info->write ?
+					    &iio_write_channel_ext_info : NULL,
+					i,
+					ext_info->shared,
+					&indio_dev->dev,
+					&indio_dev->channel_attr_list);
+			if (ret)
+				goto error_ret;
+			i++;
+		}
+
+		attrcount += i;
+	}
+
 	ret = attrcount;
 error_ret:
 	return ret;
-- 
1.7.9

[PATCH 2/5] staging:iio:dac:ad5064: Convert to extended channel info attributes

[Lars-Peter Clausen]

Use extended channel info attributes for the powerdown and powerdown_mode
attributes.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/dac/ad5064.c |   92 ++++++++++++++++----------------------
 1 files changed, 39 insertions(+), 53 deletions(-)

diff --git a/drivers/staging/iio/dac/ad5064.c b/drivers/staging/iio/dac/ad5064.c
index 049a855..202f927 100644
--- a/drivers/staging/iio/dac/ad5064.c
+++ b/drivers/staging/iio/dac/ad5064.c
@@ -86,6 +86,29 @@ enum ad5064_type {
 	ID_AD5064_1,
 };
 
+static ssize_t ad5064_read_powerdown_mode(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, char *buf);
+static ssize_t ad5064_write_powerdown_mode(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, const char *buf, size_t len);
+static ssize_t ad5064_read_dac_powerdown(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, char *buf);
+static ssize_t ad5064_write_dac_powerdown(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, const char *buf, size_t len);
+
+static struct iio_chan_spec_ext_info ad5064_ext_info[] = {
+	{
+		.name = "powerdown",
+		.read = ad5064_read_dac_powerdown,
+		.write = ad5064_write_dac_powerdown,
+	},
+	{
+		.name = "powerdown_mode",
+		.read = ad5064_read_powerdown_mode,
+		.write = ad5064_write_powerdown_mode,
+	},
+	{ },
+};
+
 #define AD5064_CHANNEL(chan, bits) {				\
 	.type = IIO_VOLTAGE,					\
 	.indexed = 1,						\
@@ -93,7 +116,8 @@ enum ad5064_type {
 	.channel = (chan),					\
 	.info_mask = IIO_CHAN_INFO_SCALE_SEPARATE_BIT,	\
 	.address = AD5064_ADDR_DAC(chan),			\
-	.scan_type = IIO_ST('u', (bits), 16, 20 - (bits))	\
+	.scan_type = IIO_ST('u', (bits), 16, 20 - (bits)),	\
+	.ext_info = ad5064_ext_info,				\
 }
 
 static const struct ad5064_chip_info ad5064_chip_info_tbl[] = {
@@ -160,22 +184,18 @@ static const char ad5064_powerdown_modes[][15] = {
 	[AD5064_LDAC_PWRDN_3STATE]	= "three_state",
 };
 
-static ssize_t ad5064_read_powerdown_mode(struct device *dev,
-	struct device_attribute *attr, char *buf)
+static ssize_t ad5064_read_powerdown_mode(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, char *buf)
 {
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct ad5064_state *st = iio_priv(indio_dev);
 
 	return sprintf(buf, "%s\n",
-		ad5064_powerdown_modes[st->pwr_down_mode[this_attr->address]]);
+		ad5064_powerdown_modes[st->pwr_down_mode[chan->channel]]);
 }
 
-static ssize_t ad5064_write_powerdown_mode(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t len)
+static ssize_t ad5064_write_powerdown_mode(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, const char *buf, size_t len)
 {
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct ad5064_state *st = iio_priv(indio_dev);
 	unsigned int mode, i;
 	int ret;
@@ -192,31 +212,26 @@ static ssize_t ad5064_write_powerdown_mode(struct device *dev,
 		return  -EINVAL;
 
 	mutex_lock(&indio_dev->mlock);
-	st->pwr_down_mode[this_attr->address] = mode;
+	st->pwr_down_mode[chan->channel] = mode;
 
-	ret = ad5064_sync_powerdown_mode(st, this_attr->address);
+	ret = ad5064_sync_powerdown_mode(st, chan->channel);
 	mutex_unlock(&indio_dev->mlock);
 
 	return ret ? ret : len;
 }
 
-static ssize_t ad5064_read_dac_powerdown(struct device *dev,
-					   struct device_attribute *attr,
-					   char *buf)
+static ssize_t ad5064_read_dac_powerdown(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, char *buf)
 {
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct ad5064_state *st = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
-	return sprintf(buf, "%d\n", st->pwr_down[this_attr->address]);
+	return sprintf(buf, "%d\n", st->pwr_down[chan->channel]);
 }
 
-static ssize_t ad5064_write_dac_powerdown(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t len)
+static ssize_t ad5064_write_dac_powerdown(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, const char *buf, size_t len)
 {
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct ad5064_state *st = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 	bool pwr_down;
 	int ret;
 
@@ -225,9 +240,9 @@ static ssize_t ad5064_write_dac_powerdown(struct device *dev,
 		return ret;
 
 	mutex_lock(&indio_dev->mlock);
-	st->pwr_down[this_attr->address] = pwr_down;
+	st->pwr_down[chan->channel] = pwr_down;
 
-	ret = ad5064_sync_powerdown_mode(st, this_attr->address);
+	ret = ad5064_sync_powerdown_mode(st, chan->channel);
 	mutex_unlock(&indio_dev->mlock);
 	return ret ? ret : len;
 }
@@ -235,36 +250,7 @@ static ssize_t ad5064_write_dac_powerdown(struct device *dev,
 static IIO_CONST_ATTR(out_voltage_powerdown_mode_available,
 			"1kohm_to_gnd 100kohm_to_gnd three_state");
 
-#define IIO_DEV_ATTR_DAC_POWERDOWN_MODE(_chan) \
-	IIO_DEVICE_ATTR(out_voltage##_chan##_powerdown_mode, \
-			S_IRUGO | S_IWUSR, \
-			ad5064_read_powerdown_mode, \
-			ad5064_write_powerdown_mode, _chan);
-
-#define IIO_DEV_ATTR_DAC_POWERDOWN(_chan)				\
-	IIO_DEVICE_ATTR(out_voltage##_chan##_powerdown,			\
-			S_IRUGO | S_IWUSR,				\
-			ad5064_read_dac_powerdown,			\
-			ad5064_write_dac_powerdown, _chan)
-
-static IIO_DEV_ATTR_DAC_POWERDOWN(0);
-static IIO_DEV_ATTR_DAC_POWERDOWN_MODE(0);
-static IIO_DEV_ATTR_DAC_POWERDOWN(1);
-static IIO_DEV_ATTR_DAC_POWERDOWN_MODE(1);
-static IIO_DEV_ATTR_DAC_POWERDOWN(2);
-static IIO_DEV_ATTR_DAC_POWERDOWN_MODE(2);
-static IIO_DEV_ATTR_DAC_POWERDOWN(3);
-static IIO_DEV_ATTR_DAC_POWERDOWN_MODE(3);
-
 static struct attribute *ad5064_attributes[] = {
-	&iio_dev_attr_out_voltage0_powerdown.dev_attr.attr,
-	&iio_dev_attr_out_voltage1_powerdown.dev_attr.attr,
-	&iio_dev_attr_out_voltage2_powerdown.dev_attr.attr,
-	&iio_dev_attr_out_voltage3_powerdown.dev_attr.attr,
-	&iio_dev_attr_out_voltage0_powerdown_mode.dev_attr.attr,
-	&iio_dev_attr_out_voltage1_powerdown_mode.dev_attr.attr,
-	&iio_dev_attr_out_voltage2_powerdown_mode.dev_attr.attr,
-	&iio_dev_attr_out_voltage3_powerdown_mode.dev_attr.attr,
 	&iio_const_attr_out_voltage_powerdown_mode_available.dev_attr.attr,
 	NULL,
 };
-- 
1.7.9

[PATCH 3/5] staging:iio:dac:ad5064: Add AD5628/AD5648/AD5668 support

[Lars-Peter Clausen]

The AD5628/AD5648/AD5668 are similar to the AD5024/AD5044/AD5064. They have an
internal reference voltage and 8 instead of 4 DAC channels.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/dac/Kconfig  |    4 +-
 drivers/staging/iio/dac/ad5064.c |  180 +++++++++++++++++++++++++++++---------
 2 files changed, 139 insertions(+), 45 deletions(-)

diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
index 13e2797..f86179c 100644
--- a/drivers/staging/iio/dac/Kconfig
+++ b/drivers/staging/iio/dac/Kconfig
@@ -4,11 +4,11 @@
 menu "Digital to analog converters"
 
 config AD5064
-	tristate "Analog Devices AD5064/64-1/44/24 DAC driver"
+	tristate "Analog Devices AD5064/64-1/44/24, AD5628/48/68 DAC driver"
 	depends on SPI
 	help
 	  Say yes here to build support for Analog Devices AD5064, AD5064-1,
-	  AD5044, AD5024 Digital to Analog Converter.
+	  AD5044, AD5024, AD5628, AD5648, AD5668 Digital to Analog Converter.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad5064.
diff --git a/drivers/staging/iio/dac/ad5064.c b/drivers/staging/iio/dac/ad5064.c
index 202f927..8cf8ca3 100644
--- a/drivers/staging/iio/dac/ad5064.c
+++ b/drivers/staging/iio/dac/ad5064.c
@@ -1,5 +1,6 @@
 /*
- * AD5064, AD5064-1, AD5044, AD5024 Digital to analog converters  driver
+ * AD5064, AD5064-1, AD5044, AD5024, AD5628, AD5648, AD5668
+ * Digital to analog converters driver
  *
  * Copyright 2011 Analog Devices Inc.
  *
@@ -19,7 +20,8 @@
 #include "../sysfs.h"
 #include "dac.h"
 
-#define AD5064_DAC_CHANNELS			4
+#define AD5064_MAX_DAC_CHANNELS			8
+#define AD5064_MAX_VREFS			4
 
 #define AD5064_ADDR(x)				((x) << 20)
 #define AD5064_CMD(x)				((x) << 24)
@@ -35,7 +37,10 @@
 #define AD5064_CMD_CLEAR			0x5
 #define AD5064_CMD_LDAC_MASK			0x6
 #define AD5064_CMD_RESET			0x7
-#define AD5064_CMD_DAISY_CHAIN_ENABLE		0x8
+#define AD5064_CMD_CONFIG			0x8
+
+#define AD5064_CONFIG_DAISY_CHAIN_ENABLE	BIT(1)
+#define AD5064_CONFIG_INT_VREF_ENABLE		BIT(0)
 
 #define AD5064_LDAC_PWRDN_NONE			0x0
 #define AD5064_LDAC_PWRDN_1K			0x1
@@ -45,12 +50,17 @@
 /**
  * struct ad5064_chip_info - chip specific information
  * @shared_vref:	whether the vref supply is shared between channels
+ * @internal_vref:	internal reference voltage. 0 if the chip has no internal
+ *			vref.
  * @channel:		channel specification
-*/
+ * @num_channels:	number of channels
+ */
 
 struct ad5064_chip_info {
 	bool shared_vref;
-	struct iio_chan_spec channel[AD5064_DAC_CHANNELS];
+	unsigned long internal_vref;
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
 };
 
 /**
@@ -61,16 +71,19 @@ struct ad5064_chip_info {
  * @pwr_down:		whether channel is powered down
  * @pwr_down_mode:	channel's current power down mode
  * @dac_cache:		current DAC raw value (chip does not support readback)
+ * @use_internal_vref:	set to true if the internal reference voltage should be
+ *			used.
  * @data:		spi transfer buffers
  */
 
 struct ad5064_state {
 	struct spi_device		*spi;
 	const struct ad5064_chip_info	*chip_info;
-	struct regulator_bulk_data	vref_reg[AD5064_DAC_CHANNELS];
-	bool				pwr_down[AD5064_DAC_CHANNELS];
-	u8				pwr_down_mode[AD5064_DAC_CHANNELS];
-	unsigned int			dac_cache[AD5064_DAC_CHANNELS];
+	struct regulator_bulk_data	vref_reg[AD5064_MAX_VREFS];
+	bool				pwr_down[AD5064_MAX_DAC_CHANNELS];
+	u8				pwr_down_mode[AD5064_MAX_DAC_CHANNELS];
+	unsigned int			dac_cache[AD5064_MAX_DAC_CHANNELS];
+	bool				use_internal_vref;
 
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
@@ -84,6 +97,12 @@ enum ad5064_type {
 	ID_AD5044,
 	ID_AD5064,
 	ID_AD5064_1,
+	ID_AD5628_1,
+	ID_AD5628_2,
+	ID_AD5648_1,
+	ID_AD5648_2,
+	ID_AD5668_1,
+	ID_AD5668_2,
 };
 
 static ssize_t ad5064_read_powerdown_mode(struct iio_dev *indio_dev,
@@ -120,34 +139,78 @@ static struct iio_chan_spec_ext_info ad5064_ext_info[] = {
 	.ext_info = ad5064_ext_info,				\
 }
 
+#define DECLARE_AD5064_CHANNELS(name, bits) \
+const struct iio_chan_spec name[] = { \
+	AD5064_CHANNEL(0, bits), \
+	AD5064_CHANNEL(1, bits), \
+	AD5064_CHANNEL(2, bits), \
+	AD5064_CHANNEL(3, bits), \
+	AD5064_CHANNEL(4, bits), \
+	AD5064_CHANNEL(5, bits), \
+	AD5064_CHANNEL(6, bits), \
+	AD5064_CHANNEL(7, bits), \
+}
+
+static DECLARE_AD5064_CHANNELS(ad5024_channels, 12);
+static DECLARE_AD5064_CHANNELS(ad5044_channels, 14);
+static DECLARE_AD5064_CHANNELS(ad5064_channels, 16);
+
 static const struct ad5064_chip_info ad5064_chip_info_tbl[] = {
 	[ID_AD5024] = {
 		.shared_vref = false,
-		.channel[0] = AD5064_CHANNEL(0, 12),
-		.channel[1] = AD5064_CHANNEL(1, 12),
-		.channel[2] = AD5064_CHANNEL(2, 12),
-		.channel[3] = AD5064_CHANNEL(3, 12),
+		.channels = ad5024_channels,
+		.num_channels = 4,
 	},
 	[ID_AD5044] = {
 		.shared_vref = false,
-		.channel[0] = AD5064_CHANNEL(0, 14),
-		.channel[1] = AD5064_CHANNEL(1, 14),
-		.channel[2] = AD5064_CHANNEL(2, 14),
-		.channel[3] = AD5064_CHANNEL(3, 14),
+		.channels = ad5044_channels,
+		.num_channels = 4,
 	},
 	[ID_AD5064] = {
 		.shared_vref = false,
-		.channel[0] = AD5064_CHANNEL(0, 16),
-		.channel[1] = AD5064_CHANNEL(1, 16),
-		.channel[2] = AD5064_CHANNEL(2, 16),
-		.channel[3] = AD5064_CHANNEL(3, 16),
+		.channels = ad5064_channels,
+		.num_channels = 4,
 	},
 	[ID_AD5064_1] = {
 		.shared_vref = true,
-		.channel[0] = AD5064_CHANNEL(0, 16),
-		.channel[1] = AD5064_CHANNEL(1, 16),
-		.channel[2] = AD5064_CHANNEL(2, 16),
-		.channel[3] = AD5064_CHANNEL(3, 16),
+		.channels = ad5064_channels,
+		.num_channels = 4,
+	},
+	[ID_AD5628_1] = {
+		.shared_vref = true,
+		.internal_vref = 2500000,
+		.channels = ad5024_channels,
+		.num_channels = 8,
+	},
+	[ID_AD5628_2] = {
+		.shared_vref = true,
+		.internal_vref = 5000000,
+		.channels = ad5024_channels,
+		.num_channels = 8,
+	},
+	[ID_AD5648_1] = {
+		.shared_vref = true,
+		.internal_vref = 2500000,
+		.channels = ad5044_channels,
+		.num_channels = 8,
+	},
+	[ID_AD5648_2] = {
+		.shared_vref = true,
+		.internal_vref = 5000000,
+		.channels = ad5044_channels,
+		.num_channels = 8,
+	},
+	[ID_AD5668_1] = {
+		.shared_vref = true,
+		.internal_vref = 2500000,
+		.channels = ad5064_channels,
+		.num_channels = 8,
+	},
+	[ID_AD5668_2] = {
+		.shared_vref = true,
+		.internal_vref = 5000000,
+		.channels = ad5064_channels,
+		.num_channels = 8,
 	},
 };
 
@@ -259,6 +322,18 @@ static const struct attribute_group ad5064_attribute_group = {
 	.attrs = ad5064_attributes,
 };
 
+static int ad5064_get_vref(struct ad5064_state *st,
+	struct iio_chan_spec const *chan)
+{
+	unsigned int i;
+
+	if (st->use_internal_vref)
+		return st->chip_info->internal_vref;
+
+	i = st->chip_info->shared_vref ? 0 : chan->channel;
+	return regulator_get_voltage(st->vref_reg[i].consumer);
+}
+
 static int ad5064_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val,
@@ -266,7 +341,6 @@ static int ad5064_read_raw(struct iio_dev *indio_dev,
 			   long m)
 {
 	struct ad5064_state *st = iio_priv(indio_dev);
-	unsigned int vref;
 	int scale_uv;
 
 	switch (m) {
@@ -274,8 +348,7 @@ static int ad5064_read_raw(struct iio_dev *indio_dev,
 		*val = st->dac_cache[chan->channel];
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		vref = st->chip_info->shared_vref ? 0 : chan->channel;
-		scale_uv = regulator_get_voltage(st->vref_reg[vref].consumer);
+		scale_uv = ad5064_get_vref(st, chan);
 		if (scale_uv < 0)
 			return scale_uv;
 
@@ -323,7 +396,7 @@ static const struct iio_info ad5064_info = {
 
 static inline unsigned int ad5064_num_vref(struct ad5064_state *st)
 {
-	return st->chip_info->shared_vref ? 1 : AD5064_DAC_CHANNELS;
+	return st->chip_info->shared_vref ? 1 : st->chip_info->num_channels;
 }
 
 static const char * const ad5064_vref_names[] = {
@@ -362,14 +435,24 @@ static int __devinit ad5064_probe(struct spi_device *spi)
 
 	ret = regulator_bulk_get(&st->spi->dev, ad5064_num_vref(st),
 		st->vref_reg);
-	if (ret)
-		goto error_free;
-
-	ret = regulator_bulk_enable(ad5064_num_vref(st), st->vref_reg);
-	if (ret)
-		goto error_free_reg;
+	if (ret) {
+		if (!st->chip_info->internal_vref)
+			goto error_free;
+		st->use_internal_vref = true;
+		ret = ad5064_spi_write(st, AD5064_CMD_CONFIG, 0,
+			AD5064_CONFIG_INT_VREF_ENABLE, 0);
+		if (ret) {
+			dev_err(&spi->dev, "Failed to enable internal vref: %d\n",
+				ret);
+			goto error_free;
+		}
+	} else {
+		ret = regulator_bulk_enable(ad5064_num_vref(st), st->vref_reg);
+		if (ret)
+			goto error_free_reg;
+	}
 
-	for (i = 0; i < AD5064_DAC_CHANNELS; ++i) {
+	for (i = 0; i < st->chip_info->num_channels; ++i) {
 		st->pwr_down_mode[i] = AD5064_LDAC_PWRDN_1K;
 		st->dac_cache[i] = 0x8000;
 	}
@@ -378,8 +461,8 @@ static int __devinit ad5064_probe(struct spi_device *spi)
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->info = &ad5064_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = st->chip_info->channel;
-	indio_dev->num_channels = AD5064_DAC_CHANNELS;
+	indio_dev->channels = st->chip_info->channels;
+	indio_dev->num_channels = st->chip_info->num_channels;
 
 	ret = iio_device_register(indio_dev);
 	if (ret)
@@ -388,9 +471,11 @@ static int __devinit ad5064_probe(struct spi_device *spi)
 	return 0;
 
 error_disable_reg:
-	regulator_bulk_disable(ad5064_num_vref(st), st->vref_reg);
+	if (!st->use_internal_vref)
+		regulator_bulk_disable(ad5064_num_vref(st), st->vref_reg);
 error_free_reg:
-	regulator_bulk_free(ad5064_num_vref(st), st->vref_reg);
+	if (!st->use_internal_vref)
+		regulator_bulk_free(ad5064_num_vref(st), st->vref_reg);
 error_free:
 	iio_free_device(indio_dev);
 
@@ -405,8 +490,10 @@ static int __devexit ad5064_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 
-	regulator_bulk_disable(ad5064_num_vref(st), st->vref_reg);
-	regulator_bulk_free(ad5064_num_vref(st), st->vref_reg);
+	if (!st->use_internal_vref) {
+		regulator_bulk_disable(ad5064_num_vref(st), st->vref_reg);
+		regulator_bulk_free(ad5064_num_vref(st), st->vref_reg);
+	}
 
 	iio_free_device(indio_dev);
 
@@ -418,6 +505,13 @@ static const struct spi_device_id ad5064_id[] = {
 	{"ad5044", ID_AD5044},
 	{"ad5064", ID_AD5064},
 	{"ad5064-1", ID_AD5064_1},
+	{"ad5628-1", ID_AD5628_1},
+	{"ad5628-2", ID_AD5628_2},
+	{"ad5648-1", ID_AD5648_1},
+	{"ad5648-2", ID_AD5648_2},
+	{"ad5668-1", ID_AD5668_1},
+	{"ad5668-2", ID_AD5668_2},
+	{"ad5668-3", ID_AD5668_2}, /* similar enough to ad5668-2 */
 	{}
 };
 MODULE_DEVICE_TABLE(spi, ad5064_id);
@@ -434,5 +528,5 @@ static struct spi_driver ad5064_driver = {
 module_spi_driver(ad5064_driver);
 
 MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
-MODULE_DESCRIPTION("Analog Devices AD5064/64-1/44/24 DAC");
+MODULE_DESCRIPTION("Analog Devices AD5064/64-1/44/24, AD5628/48/68 DAC");
 MODULE_LICENSE("GPL v2");
-- 
1.7.9

[PATCH 4/5] staging:iio:dac:ad5064: Add AD5666 support

[Lars-Peter Clausen]

The AD5666 is similar to the ad5064-1, but has a internal reference voltage.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/dac/Kconfig  |    6 +++---
 drivers/staging/iio/dac/ad5064.c |   20 ++++++++++++++++++--
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
index f86179c..4d10537 100644
--- a/drivers/staging/iio/dac/Kconfig
+++ b/drivers/staging/iio/dac/Kconfig
@@ -4,11 +4,11 @@
 menu "Digital to analog converters"
 
 config AD5064
-	tristate "Analog Devices AD5064/64-1/44/24, AD5628/48/68 DAC driver"
+	tristate "Analog Devices AD5064/64-1/44/24, AD5628/48/68, AD5666 DAC driver"
 	depends on SPI
 	help
-	  Say yes here to build support for Analog Devices AD5064, AD5064-1,
-	  AD5044, AD5024, AD5628, AD5648, AD5668 Digital to Analog Converter.
+	  Say yes here to build support for Analog Devices AD5064, AD5064-1, AD5044,
+	  AD5024, AD5628, AD5648, AD5666, AD5668 Digital to Analog Converter.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad5064.
diff --git a/drivers/staging/iio/dac/ad5064.c b/drivers/staging/iio/dac/ad5064.c
index 8cf8ca3..2457182 100644
--- a/drivers/staging/iio/dac/ad5064.c
+++ b/drivers/staging/iio/dac/ad5064.c
@@ -1,5 +1,5 @@
 /*
- * AD5064, AD5064-1, AD5044, AD5024, AD5628, AD5648, AD5668
+ * AD5064, AD5064-1, AD5044, AD5024, AD5628, AD5648, AD5666, AD5668
  * Digital to analog converters driver
  *
  * Copyright 2011 Analog Devices Inc.
@@ -103,6 +103,8 @@ enum ad5064_type {
 	ID_AD5648_2,
 	ID_AD5668_1,
 	ID_AD5668_2,
+	ID_AD5666_1,
+	ID_AD5666_2,
 };
 
 static ssize_t ad5064_read_powerdown_mode(struct iio_dev *indio_dev,
@@ -200,6 +202,18 @@ static const struct ad5064_chip_info ad5064_chip_info_tbl[] = {
 		.channels = ad5044_channels,
 		.num_channels = 8,
 	},
+	[ID_AD5666_1] = {
+		.shared_vref = true,
+		.internal_vref = 2500000,
+		.channels = ad5064_channels,
+		.num_channels = 4,
+	},
+	[ID_AD5666_2] = {
+		.shared_vref = true,
+		.internal_vref = 5000000,
+		.channels = ad5064_channels,
+		.num_channels = 4,
+	},
 	[ID_AD5668_1] = {
 		.shared_vref = true,
 		.internal_vref = 2500000,
@@ -509,6 +523,8 @@ static const struct spi_device_id ad5064_id[] = {
 	{"ad5628-2", ID_AD5628_2},
 	{"ad5648-1", ID_AD5648_1},
 	{"ad5648-2", ID_AD5648_2},
+	{"ad5666-1", ID_AD5666_1},
+	{"ad5666-2", ID_AD5666_2},
 	{"ad5668-1", ID_AD5668_1},
 	{"ad5668-2", ID_AD5668_2},
 	{"ad5668-3", ID_AD5668_2}, /* similar enough to ad5668-2 */
@@ -528,5 +544,5 @@ static struct spi_driver ad5064_driver = {
 module_spi_driver(ad5064_driver);
 
 MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
-MODULE_DESCRIPTION("Analog Devices AD5064/64-1/44/24, AD5628/48/68 DAC");
+MODULE_DESCRIPTION("Analog Devices AD5064/64-1/44/24, AD5628/48/68, AD5666 DAC");
 MODULE_LICENSE("GPL v2");
-- 
1.7.9

[PATCH 5/5] staging:iio:dac:ad5064: Add AD5025/AD5045/AD5065 support

[Lars-Peter Clausen]

The AD5025/AD5045/AD5065 are identical to the AD5024/AD5044/AD5064 excpet that
they have 2 instead of 4 DAC channels.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/dac/Kconfig  |    7 ++++---
 drivers/staging/iio/dac/ad5064.c |   27 ++++++++++++++++++++++++---
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
index 4d10537..40ab599 100644
--- a/drivers/staging/iio/dac/Kconfig
+++ b/drivers/staging/iio/dac/Kconfig
@@ -4,11 +4,12 @@
 menu "Digital to analog converters"
 
 config AD5064
-	tristate "Analog Devices AD5064/64-1/44/24, AD5628/48/68, AD5666 DAC driver"
+	tristate "Analog Devices AD5064/64-1/65/44/45/24/25, AD5628/48/68, AD5666 DAC driver"
 	depends on SPI
 	help
-	  Say yes here to build support for Analog Devices AD5064, AD5064-1, AD5044,
-	  AD5024, AD5628, AD5648, AD5666, AD5668 Digital to Analog Converter.
+	  Say yes here to build support for Analog Devices AD5024, AD5025, AD5044,
+	  AD5045, AD5064, AD5064-1, AD5065, AD5628, AD5648, AD5666, AD5668 Digital
+	  to Analog Converter.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad5064.
diff --git a/drivers/staging/iio/dac/ad5064.c b/drivers/staging/iio/dac/ad5064.c
index 2457182..65a9128 100644
--- a/drivers/staging/iio/dac/ad5064.c
+++ b/drivers/staging/iio/dac/ad5064.c
@@ -1,6 +1,6 @@
 /*
- * AD5064, AD5064-1, AD5044, AD5024, AD5628, AD5648, AD5666, AD5668
- * Digital to analog converters driver
+ * AD5024, AD5025, AD5044, AD5045, AD5064, AD5064-1, AD5065, AD5628, AD5648,
+ * AD5666, AD5668 Digital to analog converters driver
  *
  * Copyright 2011 Analog Devices Inc.
  *
@@ -94,9 +94,12 @@ struct ad5064_state {
 
 enum ad5064_type {
 	ID_AD5024,
+	ID_AD5025,
 	ID_AD5044,
+	ID_AD5045,
 	ID_AD5064,
 	ID_AD5064_1,
+	ID_AD5065,
 	ID_AD5628_1,
 	ID_AD5628_2,
 	ID_AD5648_1,
@@ -163,16 +166,31 @@ static const struct ad5064_chip_info ad5064_chip_info_tbl[] = {
 		.channels = ad5024_channels,
 		.num_channels = 4,
 	},
+	[ID_AD5025] = {
+		.shared_vref = false,
+		.channels = ad5024_channels,
+		.num_channels = 2,
+	},
 	[ID_AD5044] = {
 		.shared_vref = false,
 		.channels = ad5044_channels,
 		.num_channels = 4,
 	},
+	[ID_AD5045] = {
+		.shared_vref = false,
+		.channels = ad5044_channels,
+		.num_channels = 2,
+	},
 	[ID_AD5064] = {
 		.shared_vref = false,
 		.channels = ad5064_channels,
 		.num_channels = 4,
 	},
+	[ID_AD5065] = {
+		.shared_vref = false,
+		.channels = ad5064_channels,
+		.num_channels = 2,
+	},
 	[ID_AD5064_1] = {
 		.shared_vref = true,
 		.channels = ad5064_channels,
@@ -516,8 +534,11 @@ static int __devexit ad5064_remove(struct spi_device *spi)
 
 static const struct spi_device_id ad5064_id[] = {
 	{"ad5024", ID_AD5024},
+	{"ad5025", ID_AD5025},
 	{"ad5044", ID_AD5044},
+	{"ad5045", ID_AD5045},
 	{"ad5064", ID_AD5064},
+	{"ad5065", ID_AD5065},
 	{"ad5064-1", ID_AD5064_1},
 	{"ad5628-1", ID_AD5628_1},
 	{"ad5628-2", ID_AD5628_2},
@@ -544,5 +565,5 @@ static struct spi_driver ad5064_driver = {
 module_spi_driver(ad5064_driver);
 
 MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
-MODULE_DESCRIPTION("Analog Devices AD5064/64-1/44/24, AD5628/48/68, AD5666 DAC");
+MODULE_DESCRIPTION("Analog Devices AD5024/25/44/45/64/64-1/65, AD5628/48/68, AD5666 DAC");
 MODULE_LICENSE("GPL v2");
-- 
1.7.9

Re: [PATCH 1/5] staging:iio: Add extended IIO channel info

[Jonathan Cameron]

Fairly busy day so just some initial comments.  I'll think about this 
some more when I get
a chance...
> Sometimes devices have per channel properties which either do not map nicely to
> the current channel info scheme (e.g. string properties) or are very device
> specific, so it does not make sense to add generic support for them.
For the second class is it so bad to just put them in via attrs?  The 
first I agree
entirely should be supported in a fashion similar to this.  What you 
have here is
going to involve a fairly similar amount of boiler plate.
>
> Currently drivers define these attributes by hand for each channel. Depending on
> the number of channels this can amount to quite a few lines of boilerplate code.
> Especially if a driver supports multiple variations of a chip with different
> numbers of channels. In this case it becomes necessary to have a individual
> attribute list per chip variation and also a individual iio_info struct.
>
> This patch introduces a new scheme for handling such per channel attributes
> called extended channel info attributes. A extended channel info attribute
> consist of a name, a flag whether it is shared and read and write callbacks.
> The read and write callbacks are similar to the {read,write}_raw callbacks and
> take a IIO device and a channel as their first parameters, but instead of
> pre-parsed integer values they directly get passed the raw string value, which
> has been written to the sysfs file.
>
> It is possible to assign a list of extended channel info attributes to a
> channel. For each extended channel info attribute the IIO core will create a new
> sysfs attribute conforming to the IIO channel naming spec for the channels type,
> similar as for normal info attributes. Read and write access to this sysfs
> attribute will be redirected to the extended channel info attributes read and
> write callbacks.
My questions with this are about how it will interact with in kernel 
users. It is definitely
worth having a string type iio_info element.
I wonder if we want to allow free naming?  Could we define an enum to cover
'string' type iio_info elements?
>
> Signed-off-by: Lars-Peter Clausen<lars@metafoo.de>
> ---
>   drivers/staging/iio/iio.h               |   23 ++++++++++++
>   drivers/staging/iio/industrialio-core.c |   61 ++++++++++++++++++++++++++++---
>   2 files changed, 78 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
> index be6ced3..2a0cfbb 100644
> --- a/drivers/staging/iio/iio.h
> +++ b/drivers/staging/iio/iio.h
> @@ -88,6 +88,25 @@ enum iio_endian {
>   	IIO_LE,
>   };
>
> +struct iio_chan_spec;
> +struct iio_dev;
> +
> +/**
> + * struct iio_chan_spec_ext_info - Extended channel info attribute
> + * @name:	Info attribute name
> + * @shared:	Whether this attribute is shared between all channels.
> + * @read:	Read callback for this info attribute, may be NULL.
> + * @write:	Write callback for this info attribute, may be NULL.
> + */
> +struct iio_chan_spec_ext_info {
> +	const char *name;
> +	bool shared;
> +	ssize_t (*read)(struct iio_dev *, struct iio_chan_spec const *,
> +			char *buf);
> +	ssize_t (*write)(struct iio_dev *, struct iio_chan_spec const *,
> +			const char *buf, size_t len);
> +};
Is it worth making the callbacks also take the const char *name from 
above in the structure or
define some sort of 'private' integer in here. I'm just thinking of 
aiding reuse of the
callbacks
> +
>   /**
>    * struct iio_chan_spec - specification of a single channel
>    * @type:		What type of measurement is the channel making.
> @@ -107,6 +126,9 @@ enum iio_endian {
>    * @info_mask:		What information is to be exported about this channel.
>    *			This includes calibbias, scale etc.
>    * @event_mask:	What events can this channel produce.
> + * @ext_info:		List of extended info attributes for this channel.
> + *			The list is NULL terminated, the last element should
> + *			have it's name field set to NULL.
It's not a list, it's an array.
>    * @extend_name:	Allows labeling of channel attributes with an
>    *			informative name. Note this has no effect codes etc,
>    *			unlike modifiers.
> @@ -141,6 +163,7 @@ struct iio_chan_spec {
>   	} scan_type;
>   	long			info_mask;
>   	long			event_mask;
> +	const struct iio_chan_spec_ext_info *ext_info;
>   	char			*extend_name;
>   	const char		*datasheet_name;
>   	unsigned		processed_val:1;
> diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
> index e4824fe..a02b6ec 100644
> --- a/drivers/staging/iio/industrialio-core.c
> +++ b/drivers/staging/iio/industrialio-core.c
> @@ -144,6 +144,33 @@ static void __exit iio_exit(void)
>   	bus_unregister(&iio_bus_type);
>   }
>
> +static ssize_t iio_read_channel_ext_info(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	const struct iio_chan_spec_ext_info *ext_info;
> +
> +	ext_info =&this_attr->c->ext_info[this_attr->address];
> +
> +	return ext_info->read(indio_dev, this_attr->c, buf);
> +}
> +
> +static ssize_t iio_write_channel_ext_info(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf,
> +					 size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	const struct iio_chan_spec_ext_info *ext_info;
> +
> +	ext_info =&this_attr->c->ext_info[this_attr->address];
> +
> +	return ext_info->write(indio_dev, this_attr->c, buf, len);
> +}
> +
>   static ssize_t iio_read_channel_info(struct device *dev,
>   				     struct device_attribute *attr,
>   				     char *buf)
> @@ -401,10 +428,13 @@ int __iio_add_chan_devattr(const char *postfix,
>   	list_for_each_entry(t, attr_list, l)
>   		if (strcmp(t->dev_attr.attr.name,
>   			   iio_attr->dev_attr.attr.name) == 0) {
> -			if (!generic)
> +			if (!generic) {
>   				dev_err(dev, "tried to double register : %s\n",
>   					t->dev_attr.attr.name);
> -			ret = -EBUSY;
> +				ret = -EBUSY;
> +			} else {
> +				ret = 0;
> +			}
I would roll this and the next one into a separate precursor patch 
given. It's necessary
for the change you have here, but nice and easily separated.
>   			goto error_device_attr_deinit;
>   		}
>   	list_add(&iio_attr->l, attr_list);
> @@ -423,6 +453,7 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
>   					struct iio_chan_spec const *chan)
>   {
>   	int ret, i, attrcount = 0;
> +	const struct iio_chan_spec_ext_info *ext_info;
>
>   	if (chan->channel<  0)
>   		return 0;
> @@ -449,14 +480,32 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
>   					     !(i%2),
>   					&indio_dev->dev,
>   					&indio_dev->channel_attr_list);
> -		if (ret == -EBUSY&&  (i%2 == 0)) {
> -			ret = 0;
> -			continue;
> -		}

>   		if (ret<  0)
>   			goto error_ret;
>   		attrcount++;
>   	}
> +
> +	if (chan->ext_info) {
> +		unsigned int i = 0;
> +		for (ext_info = chan->ext_info; ext_info->name; ext_info++) {
> +			ret = __iio_add_chan_devattr(ext_info->name,
> +					chan,
> +					ext_info->read ?
> +					&iio_read_channel_ext_info : NULL,
> +					ext_info->write ?
> +					&iio_write_channel_ext_info : NULL,
> +					i,
> +					ext_info->shared,
> +					&indio_dev->dev,
> +					&indio_dev->channel_attr_list);
> +			if (ret)
> +				goto error_ret;
> +			i++;
> +		}
> +
> +		attrcount += i;
> +	}
> +
>   	ret = attrcount;
>   error_ret:
>   	return ret;

Re: [PATCH 4/5] staging:iio:dac:ad5064: Add AD5666 support

[Jonathan Cameron]

On 2/16/2012 10:49 AM, Lars-Peter Clausen wrote:
> The AD5666 is similar to the ad5064-1, but has a internal reference voltage.
This one is easy :)
>
> Signed-off-by: Lars-Peter Clausen<lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>   drivers/staging/iio/dac/Kconfig  |    6 +++---
>   drivers/staging/iio/dac/ad5064.c |   20 ++++++++++++++++++--
>   2 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
> index f86179c..4d10537 100644
> --- a/drivers/staging/iio/dac/Kconfig
> +++ b/drivers/staging/iio/dac/Kconfig
> @@ -4,11 +4,11 @@
>   menu "Digital to analog converters"
>
>   config AD5064
> -	tristate "Analog Devices AD5064/64-1/44/24, AD5628/48/68 DAC driver"
> +	tristate "Analog Devices AD5064/64-1/44/24, AD5628/48/68, AD5666 DAC driver"
>   	depends on SPI
>   	help
> -	  Say yes here to build support for Analog Devices AD5064, AD5064-1,
> -	  AD5044, AD5024, AD5628, AD5648, AD5668 Digital to Analog Converter.
> +	  Say yes here to build support for Analog Devices AD5064, AD5064-1, AD5044,
> +	  AD5024, AD5628, AD5648, AD5666, AD5668 Digital to Analog Converter.
>
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called ad5064.
> diff --git a/drivers/staging/iio/dac/ad5064.c b/drivers/staging/iio/dac/ad5064.c
> index 8cf8ca3..2457182 100644
> --- a/drivers/staging/iio/dac/ad5064.c
> +++ b/drivers/staging/iio/dac/ad5064.c
> @@ -1,5 +1,5 @@
>   /*
> - * AD5064, AD5064-1, AD5044, AD5024, AD5628, AD5648, AD5668
> + * AD5064, AD5064-1, AD5044, AD5024, AD5628, AD5648, AD5666, AD5668
>    * Digital to analog converters driver
>    *
>    * Copyright 2011 Analog Devices Inc.
> @@ -103,6 +103,8 @@ enum ad5064_type {
>   	ID_AD5648_2,
>   	ID_AD5668_1,
>   	ID_AD5668_2,
> +	ID_AD5666_1,
> +	ID_AD5666_2,
>   };
>
>   static ssize_t ad5064_read_powerdown_mode(struct iio_dev *indio_dev,
> @@ -200,6 +202,18 @@ static const struct ad5064_chip_info ad5064_chip_info_tbl[] = {
>   		.channels = ad5044_channels,
>   		.num_channels = 8,
>   	},
> +	[ID_AD5666_1] = {
> +		.shared_vref = true,
> +		.internal_vref = 2500000,
> +		.channels = ad5064_channels,
> +		.num_channels = 4,
> +	},
> +	[ID_AD5666_2] = {
> +		.shared_vref = true,
> +		.internal_vref = 5000000,
> +		.channels = ad5064_channels,
> +		.num_channels = 4,
> +	},
>   	[ID_AD5668_1] = {
>   		.shared_vref = true,
>   		.internal_vref = 2500000,
> @@ -509,6 +523,8 @@ static const struct spi_device_id ad5064_id[] = {
>   	{"ad5628-2", ID_AD5628_2},
>   	{"ad5648-1", ID_AD5648_1},
>   	{"ad5648-2", ID_AD5648_2},
> +	{"ad5666-1", ID_AD5666_1},
> +	{"ad5666-2", ID_AD5666_2},
>   	{"ad5668-1", ID_AD5668_1},
>   	{"ad5668-2", ID_AD5668_2},
>   	{"ad5668-3", ID_AD5668_2}, /* similar enough to ad5668-2 */
> @@ -528,5 +544,5 @@ static struct spi_driver ad5064_driver = {
>   module_spi_driver(ad5064_driver);
>
>   MODULE_AUTHOR("Lars-Peter Clausen<lars@metafoo.de>");
> -MODULE_DESCRIPTION("Analog Devices AD5064/64-1/44/24, AD5628/48/68 DAC");
> +MODULE_DESCRIPTION("Analog Devices AD5064/64-1/44/24, AD5628/48/68, AD5666 DAC");
>   MODULE_LICENSE("GPL v2");

Re: [PATCH 5/5] staging:iio:dac:ad5064: Add AD5025/AD5045/AD5065 support

[Jonathan Cameron]

On 2/16/2012 10:49 AM, Lars-Peter Clausen wrote:
> The AD5025/AD5045/AD5065 are identical to the AD5024/AD5044/AD5064 excpet that
except
> they have 2 instead of 4 DAC channels.
>
> Signed-off-by: Lars-Peter Clausen<lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>   drivers/staging/iio/dac/Kconfig  |    7 ++++---
>   drivers/staging/iio/dac/ad5064.c |   27 ++++++++++++++++++++++++---
>   2 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
> index 4d10537..40ab599 100644
> --- a/drivers/staging/iio/dac/Kconfig
> +++ b/drivers/staging/iio/dac/Kconfig
> @@ -4,11 +4,12 @@
>   menu "Digital to analog converters"
>
>   config AD5064
> -	tristate "Analog Devices AD5064/64-1/44/24, AD5628/48/68, AD5666 DAC driver"
> +	tristate "Analog Devices AD5064/64-1/65/44/45/24/25, AD5628/48/68, AD5666 DAC driver"
>   	depends on SPI
>   	help
> -	  Say yes here to build support for Analog Devices AD5064, AD5064-1, AD5044,
> -	  AD5024, AD5628, AD5648, AD5666, AD5668 Digital to Analog Converter.
> +	  Say yes here to build support for Analog Devices AD5024, AD5025, AD5044,
> +	  AD5045, AD5064, AD5064-1, AD5065, AD5628, AD5648, AD5666, AD5668 Digital
> +	  to Analog Converter.
>
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called ad5064.
> diff --git a/drivers/staging/iio/dac/ad5064.c b/drivers/staging/iio/dac/ad5064.c
> index 2457182..65a9128 100644
> --- a/drivers/staging/iio/dac/ad5064.c
> +++ b/drivers/staging/iio/dac/ad5064.c
> @@ -1,6 +1,6 @@
>   /*
> - * AD5064, AD5064-1, AD5044, AD5024, AD5628, AD5648, AD5666, AD5668
> - * Digital to analog converters driver
> + * AD5024, AD5025, AD5044, AD5045, AD5064, AD5064-1, AD5065, AD5628, AD5648,
> + * AD5666, AD5668 Digital to analog converters driver
>    *
>    * Copyright 2011 Analog Devices Inc.
>    *
> @@ -94,9 +94,12 @@ struct ad5064_state {
>
>   enum ad5064_type {
>   	ID_AD5024,
> +	ID_AD5025,
>   	ID_AD5044,
> +	ID_AD5045,
>   	ID_AD5064,
>   	ID_AD5064_1,
> +	ID_AD5065,
>   	ID_AD5628_1,
>   	ID_AD5628_2,
>   	ID_AD5648_1,
> @@ -163,16 +166,31 @@ static const struct ad5064_chip_info ad5064_chip_info_tbl[] = {
>   		.channels = ad5024_channels,
>   		.num_channels = 4,
>   	},
> +	[ID_AD5025] = {
> +		.shared_vref = false,
> +		.channels = ad5024_channels,
> +		.num_channels = 2,
> +	},
>   	[ID_AD5044] = {
>   		.shared_vref = false,
>   		.channels = ad5044_channels,
>   		.num_channels = 4,
>   	},
> +	[ID_AD5045] = {
> +		.shared_vref = false,
> +		.channels = ad5044_channels,
> +		.num_channels = 2,
> +	},
>   	[ID_AD5064] = {
>   		.shared_vref = false,
>   		.channels = ad5064_channels,
>   		.num_channels = 4,
>   	},
> +	[ID_AD5065] = {
> +		.shared_vref = false,
> +		.channels = ad5064_channels,
> +		.num_channels = 2,
> +	},
>   	[ID_AD5064_1] = {
>   		.shared_vref = true,
>   		.channels = ad5064_channels,
> @@ -516,8 +534,11 @@ static int __devexit ad5064_remove(struct spi_device *spi)
>
>   static const struct spi_device_id ad5064_id[] = {
>   	{"ad5024", ID_AD5024},
> +	{"ad5025", ID_AD5025},
>   	{"ad5044", ID_AD5044},
> +	{"ad5045", ID_AD5045},
>   	{"ad5064", ID_AD5064},
> +	{"ad5065", ID_AD5065},
>   	{"ad5064-1", ID_AD5064_1},
>   	{"ad5628-1", ID_AD5628_1},
>   	{"ad5628-2", ID_AD5628_2},
> @@ -544,5 +565,5 @@ static struct spi_driver ad5064_driver = {
>   module_spi_driver(ad5064_driver);
>
>   MODULE_AUTHOR("Lars-Peter Clausen<lars@metafoo.de>");
> -MODULE_DESCRIPTION("Analog Devices AD5064/64-1/44/24, AD5628/48/68, AD5666 DAC");
> +MODULE_DESCRIPTION("Analog Devices AD5024/25/44/45/64/64-1/65, AD5628/48/68, AD5666 DAC");
>   MODULE_LICENSE("GPL v2");

Re: [PATCH 1/5] staging:iio: Add extended IIO channel info

[Lars-Peter Clausen]

On 02/16/2012 03:35 PM, Jonathan Cameron wrote:
> Fairly busy day so just some initial comments.  I'll think about this some
> more when I get
> a chance...
>> Sometimes devices have per channel properties which either do not map
>> nicely to
>> the current channel info scheme (e.g. string properties) or are very device
>> specific, so it does not make sense to add generic support for them.
> For the second class is it so bad to just put them in via attrs?  The first
> I agree
> entirely should be supported in a fashion similar to this.  What you have
> here is
> going to involve a fairly similar amount of boiler plate.

With this you only need to specify the extended attributes once and can let
each channel use the same set. Without it you need to create a attribute for
each channel manually. Patch 2 in this series shows this quite nicely.

And as I wrote having support for similar chips with different channel
numbers makes this even worse.

>>
>> Currently drivers define these attributes by hand for each channel.
>> Depending on
>> the number of channels this can amount to quite a few lines of boilerplate
>> code.
>> Especially if a driver supports multiple variations of a chip with different
>> numbers of channels. In this case it becomes necessary to have a individual
>> attribute list per chip variation and also a individual iio_info struct.
>>
>> This patch introduces a new scheme for handling such per channel attributes
>> called extended channel info attributes. A extended channel info attribute
>> consist of a name, a flag whether it is shared and read and write callbacks.
>> The read and write callbacks are similar to the {read,write}_raw callbacks
>> and
>> take a IIO device and a channel as their first parameters, but instead of
>> pre-parsed integer values they directly get passed the raw string value,
>> which
>> has been written to the sysfs file.
>>
>> It is possible to assign a list of extended channel info attributes to a
>> channel. For each extended channel info attribute the IIO core will create
>> a new
>> sysfs attribute conforming to the IIO channel naming spec for the channels
>> type,
>> similar as for normal info attributes. Read and write access to this sysfs
>> attribute will be redirected to the extended channel info attributes read and
>> write callbacks.
> My questions with this are about how it will interact with in kernel users.
> It is definitely
> worth having a string type iio_info element.
> I wonder if we want to allow free naming?  Could we define an enum to cover
> 'string' type iio_info elements?

It is not intended to be used by in kernel users, it is just meant as a
replacement for the handcrafted per channel sysfs attributes.

>>
>> Signed-off-by: Lars-Peter Clausen<lars@metafoo.de>
>> ---
>>   drivers/staging/iio/iio.h               |   23 ++++++++++++
>>   drivers/staging/iio/industrialio-core.c |   61
>> ++++++++++++++++++++++++++++---
>>   2 files changed, 78 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
>> index be6ced3..2a0cfbb 100644
>> --- a/drivers/staging/iio/iio.h
>> +++ b/drivers/staging/iio/iio.h
>> @@ -88,6 +88,25 @@ enum iio_endian {
>>       IIO_LE,
>>   };
>>
>> +struct iio_chan_spec;
>> +struct iio_dev;
>> +
>> +/**
>> + * struct iio_chan_spec_ext_info - Extended channel info attribute
>> + * @name:    Info attribute name
>> + * @shared:    Whether this attribute is shared between all channels.
>> + * @read:    Read callback for this info attribute, may be NULL.
>> + * @write:    Write callback for this info attribute, may be NULL.
>> + */
>> +struct iio_chan_spec_ext_info {
>> +    const char *name;
>> +    bool shared;
>> +    ssize_t (*read)(struct iio_dev *, struct iio_chan_spec const *,
>> +            char *buf);
>> +    ssize_t (*write)(struct iio_dev *, struct iio_chan_spec const *,
>> +            const char *buf, size_t len);
>> +};
> Is it worth making the callbacks also take the const char *name from above
> in the structure or
> define some sort of 'private' integer in here. I'm just thinking of aiding
> reuse of the
> callbacks

Yes, I've thought about it and decided against it, since we don't have a
user for this right now. So chances are that I'd get the interface wrong and
we need to modify it once we have real users anyway. Also coccinelle makes
it quite trivial to refactor a function or callback to take an additional
parameter.

Re: [PATCH 1/5] staging:iio: Add extended IIO channel info

[Jonathan Cameron]

On 2/16/2012 3:02 PM, Lars-Peter Clausen wrote:
> On 02/16/2012 03:35 PM, Jonathan Cameron wrote:
>> Fairly busy day so just some initial comments.  I'll think about this some
>> more when I get
>> a chance...
>>> Sometimes devices have per channel properties which either do not map
>>> nicely to
>>> the current channel info scheme (e.g. string properties) or are very device
>>> specific, so it does not make sense to add generic support for them.
>> For the second class is it so bad to just put them in via attrs?  The first
>> I agree
>> entirely should be supported in a fashion similar to this.  What you have
>> here is
>> going to involve a fairly similar amount of boiler plate.
> With this you only need to specify the extended attributes once and can let
> each channel use the same set. Without it you need to create a attribute for
> each channel manually. Patch 2 in this series shows this quite nicely.
>
> And as I wrote having support for similar chips with different channel
> numbers makes this even worse.
Fair enough.
>
>>> Currently drivers define these attributes by hand for each channel.
>>> Depending on
>>> the number of channels this can amount to quite a few lines of boilerplate
>>> code.
>>> Especially if a driver supports multiple variations of a chip with different
>>> numbers of channels. In this case it becomes necessary to have a individual
>>> attribute list per chip variation and also a individual iio_info struct.
>>>
>>> This patch introduces a new scheme for handling such per channel attributes
>>> called extended channel info attributes. A extended channel info attribute
>>> consist of a name, a flag whether it is shared and read and write callbacks.
>>> The read and write callbacks are similar to the {read,write}_raw callbacks
>>> and
>>> take a IIO device and a channel as their first parameters, but instead of
>>> pre-parsed integer values they directly get passed the raw string value,
>>> which
>>> has been written to the sysfs file.
>>>
>>> It is possible to assign a list of extended channel info attributes to a
>>> channel. For each extended channel info attribute the IIO core will create
>>> a new
>>> sysfs attribute conforming to the IIO channel naming spec for the channels
>>> type,
>>> similar as for normal info attributes. Read and write access to this sysfs
>>> attribute will be redirected to the extended channel info attributes read and
>>> write callbacks.
>> My questions with this are about how it will interact with in kernel users.
>> It is definitely
>> worth having a string type iio_info element.
>> I wonder if we want to allow free naming?  Could we define an enum to cover
>> 'string' type iio_info elements?
> It is not intended to be used by in kernel users, it is just meant as a
> replacement for the handcrafted per channel sysfs attributes.
Agreed that for some of these parameters such a use wouldn't make much 
sense, but
it seems likely that something will come along at some point where it 
does so we probably
want this at the back of our minds...
>
>>> Signed-off-by: Lars-Peter Clausen<lars@metafoo.de>
>>> ---
>>>    drivers/staging/iio/iio.h               |   23 ++++++++++++
>>>    drivers/staging/iio/industrialio-core.c |   61
>>> ++++++++++++++++++++++++++++---
>>>    2 files changed, 78 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
>>> index be6ced3..2a0cfbb 100644
>>> --- a/drivers/staging/iio/iio.h
>>> +++ b/drivers/staging/iio/iio.h
>>> @@ -88,6 +88,25 @@ enum iio_endian {
>>>        IIO_LE,
>>>    };
>>>
>>> +struct iio_chan_spec;
>>> +struct iio_dev;
>>> +
>>> +/**
>>> + * struct iio_chan_spec_ext_info - Extended channel info attribute
>>> + * @name:    Info attribute name
>>> + * @shared:    Whether this attribute is shared between all channels.
>>> + * @read:    Read callback for this info attribute, may be NULL.
>>> + * @write:    Write callback for this info attribute, may be NULL.
>>> + */
>>> +struct iio_chan_spec_ext_info {
>>> +    const char *name;
>>> +    bool shared;
>>> +    ssize_t (*read)(struct iio_dev *, struct iio_chan_spec const *,
>>> +            char *buf);
>>> +    ssize_t (*write)(struct iio_dev *, struct iio_chan_spec const *,
>>> +            const char *buf, size_t len);
>>> +};
>> Is it worth making the callbacks also take the const char *name from above
>> in the structure or
>> define some sort of 'private' integer in here. I'm just thinking of aiding
>> reuse of the
>> callbacks
> Yes, I've thought about it and decided against it, since we don't have a
> user for this right now. So chances are that I'd get the interface wrong and
> we need to modify it once we have real users anyway. Also coccinelle makes
> it quite trivial to refactor a function or callback to take an additional
> parameter.
All right, then I agree that this approach is fine for now.  We'll keep 
it in mind for
possibly needing updating as it gets more users.

Acked-by: Jonathan Cameron <jic23@kernel.org>  Preferably with the bits 
suggested split
out into a precursor patch.
> domo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] staging:iio:dac:ad5064: Convert to extended channel info attributes

[Jonathan Cameron]

On 2/16/2012 10:49 AM, Lars-Peter Clausen wrote:
> Use extended channel info attributes for the powerdown and powerdown_mode
> attributes.
You could even take the power_down_mode_available in as a shared
iio_chan_spec_ext_info?  Nice as it would get rid of the attrs group 
entirely.

Up to you and fine with me either way.
>
> Signed-off-by: Lars-Peter Clausen<lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>   drivers/staging/iio/dac/ad5064.c |   92 ++++++++++++++++----------------------
>   1 files changed, 39 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/staging/iio/dac/ad5064.c b/drivers/staging/iio/dac/ad5064.c
> index 049a855..202f927 100644
> --- a/drivers/staging/iio/dac/ad5064.c
> +++ b/drivers/staging/iio/dac/ad5064.c
> @@ -86,6 +86,29 @@ enum ad5064_type {
>   	ID_AD5064_1,
>   };
>
> +static ssize_t ad5064_read_powerdown_mode(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, char *buf);
> +static ssize_t ad5064_write_powerdown_mode(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, const char *buf, size_t len);
> +static ssize_t ad5064_read_dac_powerdown(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, char *buf);
> +static ssize_t ad5064_write_dac_powerdown(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, const char *buf, size_t len);
> +
> +static struct iio_chan_spec_ext_info ad5064_ext_info[] = {
> +	{
> +		.name = "powerdown",
> +		.read = ad5064_read_dac_powerdown,
> +		.write = ad5064_write_dac_powerdown,
> +	},
> +	{
> +		.name = "powerdown_mode",
> +		.read = ad5064_read_powerdown_mode,
> +		.write = ad5064_write_powerdown_mode,
> +	},
> +	{ },
> +};
> +
>   #define AD5064_CHANNEL(chan, bits) {				\
>   	.type = IIO_VOLTAGE,					\
>   	.indexed = 1,						\
> @@ -93,7 +116,8 @@ enum ad5064_type {
>   	.channel = (chan),					\
>   	.info_mask = IIO_CHAN_INFO_SCALE_SEPARATE_BIT,	\
>   	.address = AD5064_ADDR_DAC(chan),			\
> -	.scan_type = IIO_ST('u', (bits), 16, 20 - (bits))	\
> +	.scan_type = IIO_ST('u', (bits), 16, 20 - (bits)),	\
> +	.ext_info = ad5064_ext_info,				\
>   }
>
>   static const struct ad5064_chip_info ad5064_chip_info_tbl[] = {
> @@ -160,22 +184,18 @@ static const char ad5064_powerdown_modes[][15] = {
>   	[AD5064_LDAC_PWRDN_3STATE]	= "three_state",
>   };
>
> -static ssize_t ad5064_read_powerdown_mode(struct device *dev,
> -	struct device_attribute *attr, char *buf)
> +static ssize_t ad5064_read_powerdown_mode(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, char *buf)
>   {
> -	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>   	struct ad5064_state *st = iio_priv(indio_dev);
>
>   	return sprintf(buf, "%s\n",
> -		ad5064_powerdown_modes[st->pwr_down_mode[this_attr->address]]);
> +		ad5064_powerdown_modes[st->pwr_down_mode[chan->channel]]);
>   }
>
> -static ssize_t ad5064_write_powerdown_mode(struct device *dev,
> -	struct device_attribute *attr, const char *buf, size_t len)
> +static ssize_t ad5064_write_powerdown_mode(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, const char *buf, size_t len)
>   {
> -	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>   	struct ad5064_state *st = iio_priv(indio_dev);
>   	unsigned int mode, i;
>   	int ret;
> @@ -192,31 +212,26 @@ static ssize_t ad5064_write_powerdown_mode(struct device *dev,
>   		return  -EINVAL;
>
>   	mutex_lock(&indio_dev->mlock);
> -	st->pwr_down_mode[this_attr->address] = mode;
> +	st->pwr_down_mode[chan->channel] = mode;
>
> -	ret = ad5064_sync_powerdown_mode(st, this_attr->address);
> +	ret = ad5064_sync_powerdown_mode(st, chan->channel);
>   	mutex_unlock(&indio_dev->mlock);
>
>   	return ret ? ret : len;
>   }
>
> -static ssize_t ad5064_read_dac_powerdown(struct device *dev,
> -					   struct device_attribute *attr,
> -					   char *buf)
> +static ssize_t ad5064_read_dac_powerdown(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, char *buf)
>   {
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>   	struct ad5064_state *st = iio_priv(indio_dev);
> -	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>
> -	return sprintf(buf, "%d\n", st->pwr_down[this_attr->address]);
> +	return sprintf(buf, "%d\n", st->pwr_down[chan->channel]);
>   }
>
> -static ssize_t ad5064_write_dac_powerdown(struct device *dev,
> -	struct device_attribute *attr, const char *buf, size_t len)
> +static ssize_t ad5064_write_dac_powerdown(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, const char *buf, size_t len)
>   {
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>   	struct ad5064_state *st = iio_priv(indio_dev);
> -	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>   	bool pwr_down;
>   	int ret;
>
> @@ -225,9 +240,9 @@ static ssize_t ad5064_write_dac_powerdown(struct device *dev,
>   		return ret;
>
>   	mutex_lock(&indio_dev->mlock);
> -	st->pwr_down[this_attr->address] = pwr_down;
> +	st->pwr_down[chan->channel] = pwr_down;
>
> -	ret = ad5064_sync_powerdown_mode(st, this_attr->address);
> +	ret = ad5064_sync_powerdown_mode(st, chan->channel);
>   	mutex_unlock(&indio_dev->mlock);
>   	return ret ? ret : len;
>   }
> @@ -235,36 +250,7 @@ static ssize_t ad5064_write_dac_powerdown(struct device *dev,
>   static IIO_CONST_ATTR(out_voltage_powerdown_mode_available,
>   			"1kohm_to_gnd 100kohm_to_gnd three_state");
>
> -#define IIO_DEV_ATTR_DAC_POWERDOWN_MODE(_chan) \
> -	IIO_DEVICE_ATTR(out_voltage##_chan##_powerdown_mode, \
> -			S_IRUGO | S_IWUSR, \
> -			ad5064_read_powerdown_mode, \
> -			ad5064_write_powerdown_mode, _chan);
> -
> -#define IIO_DEV_ATTR_DAC_POWERDOWN(_chan)				\
> -	IIO_DEVICE_ATTR(out_voltage##_chan##_powerdown,			\
> -			S_IRUGO | S_IWUSR,				\
> -			ad5064_read_dac_powerdown,			\
> -			ad5064_write_dac_powerdown, _chan)
> -
> -static IIO_DEV_ATTR_DAC_POWERDOWN(0);
> -static IIO_DEV_ATTR_DAC_POWERDOWN_MODE(0);
> -static IIO_DEV_ATTR_DAC_POWERDOWN(1);
> -static IIO_DEV_ATTR_DAC_POWERDOWN_MODE(1);
> -static IIO_DEV_ATTR_DAC_POWERDOWN(2);
> -static IIO_DEV_ATTR_DAC_POWERDOWN_MODE(2);
> -static IIO_DEV_ATTR_DAC_POWERDOWN(3);
> -static IIO_DEV_ATTR_DAC_POWERDOWN_MODE(3);
> -
>   static struct attribute *ad5064_attributes[] = {
> -	&iio_dev_attr_out_voltage0_powerdown.dev_attr.attr,
> -	&iio_dev_attr_out_voltage1_powerdown.dev_attr.attr,
> -	&iio_dev_attr_out_voltage2_powerdown.dev_attr.attr,
> -	&iio_dev_attr_out_voltage3_powerdown.dev_attr.attr,
> -	&iio_dev_attr_out_voltage0_powerdown_mode.dev_attr.attr,
> -	&iio_dev_attr_out_voltage1_powerdown_mode.dev_attr.attr,
> -	&iio_dev_attr_out_voltage2_powerdown_mode.dev_attr.attr,
> -	&iio_dev_attr_out_voltage3_powerdown_mode.dev_attr.attr,
>   	&iio_const_attr_out_voltage_powerdown_mode_available.dev_attr.attr,
>   	NULL,
>   };

Re: [PATCH 3/5] staging:iio:dac:ad5064: Add AD5628/AD5648/AD5668 support

[Jonathan Cameron]

On 2/16/2012 10:49 AM, Lars-Peter Clausen wrote:
> The AD5628/AD5648/AD5668 are similar to the AD5024/AD5044/AD5064. They have an
> internal reference voltage and 8 instead of 4 DAC channels.
I'd marginally have preferred this set as a cleanup then the extension 
to cover the 8 channel
parts but it's fine like this.
> Signed-off-by: Lars-Peter Clausen<lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>   drivers/staging/iio/dac/Kconfig  |    4 +-
>   drivers/staging/iio/dac/ad5064.c |  180 +++++++++++++++++++++++++++++---------
>   2 files changed, 139 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
> index 13e2797..f86179c 100644
> --- a/drivers/staging/iio/dac/Kconfig
> +++ b/drivers/staging/iio/dac/Kconfig
> @@ -4,11 +4,11 @@
>   menu "Digital to analog converters"
>
>   config AD5064
> -	tristate "Analog Devices AD5064/64-1/44/24 DAC driver"
> +	tristate "Analog Devices AD5064/64-1/44/24, AD5628/48/68 DAC driver"
>   	depends on SPI
>   	help
>   	  Say yes here to build support for Analog Devices AD5064, AD5064-1,
> -	  AD5044, AD5024 Digital to Analog Converter.
> +	  AD5044, AD5024, AD5628, AD5648, AD5668 Digital to Analog Converter.
>
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called ad5064.
> diff --git a/drivers/staging/iio/dac/ad5064.c b/drivers/staging/iio/dac/ad5064.c
> index 202f927..8cf8ca3 100644
> --- a/drivers/staging/iio/dac/ad5064.c
> +++ b/drivers/staging/iio/dac/ad5064.c
> @@ -1,5 +1,6 @@
>   /*
> - * AD5064, AD5064-1, AD5044, AD5024 Digital to analog converters  driver
> + * AD5064, AD5064-1, AD5044, AD5024, AD5628, AD5648, AD5668
> + * Digital to analog converters driver
>    *
>    * Copyright 2011 Analog Devices Inc.
>    *
> @@ -19,7 +20,8 @@
>   #include "../sysfs.h"
>   #include "dac.h"
>
> -#define AD5064_DAC_CHANNELS			4
> +#define AD5064_MAX_DAC_CHANNELS			8
> +#define AD5064_MAX_VREFS			4
>
>   #define AD5064_ADDR(x)				((x)<<  20)
>   #define AD5064_CMD(x)				((x)<<  24)
> @@ -35,7 +37,10 @@
>   #define AD5064_CMD_CLEAR			0x5
>   #define AD5064_CMD_LDAC_MASK			0x6
>   #define AD5064_CMD_RESET			0x7
> -#define AD5064_CMD_DAISY_CHAIN_ENABLE		0x8
> +#define AD5064_CMD_CONFIG			0x8
> +
> +#define AD5064_CONFIG_DAISY_CHAIN_ENABLE	BIT(1)
> +#define AD5064_CONFIG_INT_VREF_ENABLE		BIT(0)
>
>   #define AD5064_LDAC_PWRDN_NONE			0x0
>   #define AD5064_LDAC_PWRDN_1K			0x1
> @@ -45,12 +50,17 @@
>   /**
>    * struct ad5064_chip_info - chip specific information
>    * @shared_vref:	whether the vref supply is shared between channels
> + * @internal_vref:	internal reference voltage. 0 if the chip has no internal
> + *			vref.
>    * @channel:		channel specification
> -*/
> + * @num_channels:	number of channels
> + */
>
>   struct ad5064_chip_info {
>   	bool shared_vref;
> -	struct iio_chan_spec channel[AD5064_DAC_CHANNELS];
> +	unsigned long internal_vref;
> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;
>   };
>
>   /**
> @@ -61,16 +71,19 @@ struct ad5064_chip_info {
>    * @pwr_down:		whether channel is powered down
>    * @pwr_down_mode:	channel's current power down mode
>    * @dac_cache:		current DAC raw value (chip does not support readback)
> + * @use_internal_vref:	set to true if the internal reference voltage should be
> + *			used.
>    * @data:		spi transfer buffers
>    */
>
>   struct ad5064_state {
>   	struct spi_device		*spi;
>   	const struct ad5064_chip_info	*chip_info;
> -	struct regulator_bulk_data	vref_reg[AD5064_DAC_CHANNELS];
> -	bool				pwr_down[AD5064_DAC_CHANNELS];
> -	u8				pwr_down_mode[AD5064_DAC_CHANNELS];
> -	unsigned int			dac_cache[AD5064_DAC_CHANNELS];
> +	struct regulator_bulk_data	vref_reg[AD5064_MAX_VREFS];
> +	bool				pwr_down[AD5064_MAX_DAC_CHANNELS];
> +	u8				pwr_down_mode[AD5064_MAX_DAC_CHANNELS];
> +	unsigned int			dac_cache[AD5064_MAX_DAC_CHANNELS];
> +	bool				use_internal_vref;
>
>   	/*
>   	 * DMA (thus cache coherency maintenance) requires the
> @@ -84,6 +97,12 @@ enum ad5064_type {
>   	ID_AD5044,
>   	ID_AD5064,
>   	ID_AD5064_1,
> +	ID_AD5628_1,
> +	ID_AD5628_2,
> +	ID_AD5648_1,
> +	ID_AD5648_2,
> +	ID_AD5668_1,
> +	ID_AD5668_2,
>   };
>
>   static ssize_t ad5064_read_powerdown_mode(struct iio_dev *indio_dev,
> @@ -120,34 +139,78 @@ static struct iio_chan_spec_ext_info ad5064_ext_info[] = {
>   	.ext_info = ad5064_ext_info,				\
>   }
>
> +#define DECLARE_AD5064_CHANNELS(name, bits) \
> +const struct iio_chan_spec name[] = { \
> +	AD5064_CHANNEL(0, bits), \
> +	AD5064_CHANNEL(1, bits), \
> +	AD5064_CHANNEL(2, bits), \
> +	AD5064_CHANNEL(3, bits), \
> +	AD5064_CHANNEL(4, bits), \
> +	AD5064_CHANNEL(5, bits), \
> +	AD5064_CHANNEL(6, bits), \
> +	AD5064_CHANNEL(7, bits), \
> +}
> +
Could make this a neater set by breaking out this tidy up first
(then extend it to 8 channels).   Pretty straight forward though
so doesn't really matter.
> +static DECLARE_AD5064_CHANNELS(ad5024_channels, 12);
> +static DECLARE_AD5064_CHANNELS(ad5044_channels, 14);
> +static DECLARE_AD5064_CHANNELS(ad5064_channels, 16);
> +
>   static const struct ad5064_chip_info ad5064_chip_info_tbl[] = {
>   	[ID_AD5024] = {
>   		.shared_vref = false,
> -		.channel[0] = AD5064_CHANNEL(0, 12),
> -		.channel[1] = AD5064_CHANNEL(1, 12),
> -		.channel[2] = AD5064_CHANNEL(2, 12),
> -		.channel[3] = AD5064_CHANNEL(3, 12),
> +		.channels = ad5024_channels,
> +		.num_channels = 4,
>   	},
>   	[ID_AD5044] = {
>   		.shared_vref = false,
> -		.channel[0] = AD5064_CHANNEL(0, 14),
> -		.channel[1] = AD5064_CHANNEL(1, 14),
> -		.channel[2] = AD5064_CHANNEL(2, 14),
> -		.channel[3] = AD5064_CHANNEL(3, 14),
> +		.channels = ad5044_channels,
> +		.num_channels = 4,
>   	},
>   	[ID_AD5064] = {
>   		.shared_vref = false,
> -		.channel[0] = AD5064_CHANNEL(0, 16),
> -		.channel[1] = AD5064_CHANNEL(1, 16),
> -		.channel[2] = AD5064_CHANNEL(2, 16),
> -		.channel[3] = AD5064_CHANNEL(3, 16),
> +		.channels = ad5064_channels,
> +		.num_channels = 4,
>   	},
>   	[ID_AD5064_1] = {
>   		.shared_vref = true,
> -		.channel[0] = AD5064_CHANNEL(0, 16),
> -		.channel[1] = AD5064_CHANNEL(1, 16),
> -		.channel[2] = AD5064_CHANNEL(2, 16),
> -		.channel[3] = AD5064_CHANNEL(3, 16),
> +		.channels = ad5064_channels,
> +		.num_channels = 4,
> +	},
> +	[ID_AD5628_1] = {
> +		.shared_vref = true,
> +		.internal_vref = 2500000,
> +		.channels = ad5024_channels,
> +		.num_channels = 8,
> +	},
> +	[ID_AD5628_2] = {
> +		.shared_vref = true,
> +		.internal_vref = 5000000,
> +		.channels = ad5024_channels,
> +		.num_channels = 8,
> +	},
> +	[ID_AD5648_1] = {
> +		.shared_vref = true,
> +		.internal_vref = 2500000,
> +		.channels = ad5044_channels,
> +		.num_channels = 8,
> +	},
> +	[ID_AD5648_2] = {
> +		.shared_vref = true,
> +		.internal_vref = 5000000,
> +		.channels = ad5044_channels,
> +		.num_channels = 8,
> +	},
> +	[ID_AD5668_1] = {
> +		.shared_vref = true,
> +		.internal_vref = 2500000,
> +		.channels = ad5064_channels,
> +		.num_channels = 8,
> +	},
> +	[ID_AD5668_2] = {
> +		.shared_vref = true,
> +		.internal_vref = 5000000,
> +		.channels = ad5064_channels,
> +		.num_channels = 8,
>   	},
>   };
>
> @@ -259,6 +322,18 @@ static const struct attribute_group ad5064_attribute_group = {
>   	.attrs = ad5064_attributes,
>   };
>
> +static int ad5064_get_vref(struct ad5064_state *st,
> +	struct iio_chan_spec const *chan)
> +{
> +	unsigned int i;
> +
> +	if (st->use_internal_vref)
> +		return st->chip_info->internal_vref;
> +
> +	i = st->chip_info->shared_vref ? 0 : chan->channel;
> +	return regulator_get_voltage(st->vref_reg[i].consumer);
> +}
> +
>   static int ad5064_read_raw(struct iio_dev *indio_dev,
>   			   struct iio_chan_spec const *chan,
>   			   int *val,
> @@ -266,7 +341,6 @@ static int ad5064_read_raw(struct iio_dev *indio_dev,
>   			   long m)
>   {
>   	struct ad5064_state *st = iio_priv(indio_dev);
> -	unsigned int vref;
>   	int scale_uv;
>
>   	switch (m) {
> @@ -274,8 +348,7 @@ static int ad5064_read_raw(struct iio_dev *indio_dev,
>   		*val = st->dac_cache[chan->channel];
>   		return IIO_VAL_INT;
>   	case IIO_CHAN_INFO_SCALE:
> -		vref = st->chip_info->shared_vref ? 0 : chan->channel;
> -		scale_uv = regulator_get_voltage(st->vref_reg[vref].consumer);
> +		scale_uv = ad5064_get_vref(st, chan);
>   		if (scale_uv<  0)
>   			return scale_uv;
>
> @@ -323,7 +396,7 @@ static const struct iio_info ad5064_info = {
>
>   static inline unsigned int ad5064_num_vref(struct ad5064_state *st)
>   {
> -	return st->chip_info->shared_vref ? 1 : AD5064_DAC_CHANNELS;
> +	return st->chip_info->shared_vref ? 1 : st->chip_info->num_channels;
>   }
>
>   static const char * const ad5064_vref_names[] = {
> @@ -362,14 +435,24 @@ static int __devinit ad5064_probe(struct spi_device *spi)
>
>   	ret = regulator_bulk_get(&st->spi->dev, ad5064_num_vref(st),
>   		st->vref_reg);
> -	if (ret)
> -		goto error_free;
> -
> -	ret = regulator_bulk_enable(ad5064_num_vref(st), st->vref_reg);
> -	if (ret)
> -		goto error_free_reg;
> +	if (ret) {
> +		if (!st->chip_info->internal_vref)
> +			goto error_free;
> +		st->use_internal_vref = true;
> +		ret = ad5064_spi_write(st, AD5064_CMD_CONFIG, 0,
> +			AD5064_CONFIG_INT_VREF_ENABLE, 0);
> +		if (ret) {
> +			dev_err(&spi->dev, "Failed to enable internal vref: %d\n",
> +				ret);
> +			goto error_free;
> +		}
> +	} else {
> +		ret = regulator_bulk_enable(ad5064_num_vref(st), st->vref_reg);
> +		if (ret)
> +			goto error_free_reg;
> +	}
>
> -	for (i = 0; i<  AD5064_DAC_CHANNELS; ++i) {
> +	for (i = 0; i<  st->chip_info->num_channels; ++i) {
>   		st->pwr_down_mode[i] = AD5064_LDAC_PWRDN_1K;
>   		st->dac_cache[i] = 0x8000;
>   	}
> @@ -378,8 +461,8 @@ static int __devinit ad5064_probe(struct spi_device *spi)
>   	indio_dev->name = spi_get_device_id(spi)->name;
>   	indio_dev->info =&ad5064_info;
>   	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->channels = st->chip_info->channel;
> -	indio_dev->num_channels = AD5064_DAC_CHANNELS;
> +	indio_dev->channels = st->chip_info->channels;
> +	indio_dev->num_channels = st->chip_info->num_channels;
>
>   	ret = iio_device_register(indio_dev);
>   	if (ret)
> @@ -388,9 +471,11 @@ static int __devinit ad5064_probe(struct spi_device *spi)
>   	return 0;
>
>   error_disable_reg:
> -	regulator_bulk_disable(ad5064_num_vref(st), st->vref_reg);
> +	if (!st->use_internal_vref)
> +		regulator_bulk_disable(ad5064_num_vref(st), st->vref_reg);
>   error_free_reg:
> -	regulator_bulk_free(ad5064_num_vref(st), st->vref_reg);
> +	if (!st->use_internal_vref)
> +		regulator_bulk_free(ad5064_num_vref(st), st->vref_reg);
>   error_free:
>   	iio_free_device(indio_dev);
>
> @@ -405,8 +490,10 @@ static int __devexit ad5064_remove(struct spi_device *spi)
>
>   	iio_device_unregister(indio_dev);
>
> -	regulator_bulk_disable(ad5064_num_vref(st), st->vref_reg);
> -	regulator_bulk_free(ad5064_num_vref(st), st->vref_reg);
> +	if (!st->use_internal_vref) {
> +		regulator_bulk_disable(ad5064_num_vref(st), st->vref_reg);
> +		regulator_bulk_free(ad5064_num_vref(st), st->vref_reg);
> +	}
>
>   	iio_free_device(indio_dev);
>
> @@ -418,6 +505,13 @@ static const struct spi_device_id ad5064_id[] = {
>   	{"ad5044", ID_AD5044},
>   	{"ad5064", ID_AD5064},
>   	{"ad5064-1", ID_AD5064_1},
> +	{"ad5628-1", ID_AD5628_1},
> +	{"ad5628-2", ID_AD5628_2},
> +	{"ad5648-1", ID_AD5648_1},
> +	{"ad5648-2", ID_AD5648_2},
> +	{"ad5668-1", ID_AD5668_1},
> +	{"ad5668-2", ID_AD5668_2},
> +	{"ad5668-3", ID_AD5668_2}, /* similar enough to ad5668-2 */
>   	{}
>   };
>   MODULE_DEVICE_TABLE(spi, ad5064_id);
> @@ -434,5 +528,5 @@ static struct spi_driver ad5064_driver = {
>   module_spi_driver(ad5064_driver);
>
>   MODULE_AUTHOR("Lars-Peter Clausen<lars@metafoo.de>");
> -MODULE_DESCRIPTION("Analog Devices AD5064/64-1/44/24 DAC");
> +MODULE_DESCRIPTION("Analog Devices AD5064/64-1/44/24, AD5628/48/68 DAC");
>   MODULE_LICENSE("GPL v2");

Re: [PATCH 1/5] staging:iio: Add extended IIO channel info

[Lars-Peter Clausen]

On 02/16/2012 04:04 PM, Jonathan Cameron wrote:
> On 2/16/2012 3:02 PM, Lars-Peter Clausen wrote:
>> On 02/16/2012 03:35 PM, Jonathan Cameron wrote:
>>> Fairly busy day so just some initial comments.  I'll think about this some
>>> more when I get
>>> a chance...
>>>> Sometimes devices have per channel properties which either do not map
>>>> nicely to
>>>> the current channel info scheme (e.g. string properties) or are very device
>>>> specific, so it does not make sense to add generic support for them.
>>> For the second class is it so bad to just put them in via attrs?  The first
>>> I agree
>>> entirely should be supported in a fashion similar to this.  What you have
>>> here is
>>> going to involve a fairly similar amount of boiler plate.
>> With this you only need to specify the extended attributes once and can let
>> each channel use the same set. Without it you need to create a attribute for
>> each channel manually. Patch 2 in this series shows this quite nicely.
>>
>> And as I wrote having support for similar chips with different channel
>> numbers makes this even worse.
> Fair enough.
>>
>>>> Currently drivers define these attributes by hand for each channel.
>>>> Depending on
>>>> the number of channels this can amount to quite a few lines of boilerplate
>>>> code.
>>>> Especially if a driver supports multiple variations of a chip with
>>>> different
>>>> numbers of channels. In this case it becomes necessary to have a individual
>>>> attribute list per chip variation and also a individual iio_info struct.
>>>>
>>>> This patch introduces a new scheme for handling such per channel attributes
>>>> called extended channel info attributes. A extended channel info attribute
>>>> consist of a name, a flag whether it is shared and read and write
>>>> callbacks.
>>>> The read and write callbacks are similar to the {read,write}_raw callbacks
>>>> and
>>>> take a IIO device and a channel as their first parameters, but instead of
>>>> pre-parsed integer values they directly get passed the raw string value,
>>>> which
>>>> has been written to the sysfs file.
>>>>
>>>> It is possible to assign a list of extended channel info attributes to a
>>>> channel. For each extended channel info attribute the IIO core will create
>>>> a new
>>>> sysfs attribute conforming to the IIO channel naming spec for the channels
>>>> type,
>>>> similar as for normal info attributes. Read and write access to this sysfs
>>>> attribute will be redirected to the extended channel info attributes
>>>> read and
>>>> write callbacks.
>>> My questions with this are about how it will interact with in kernel users.
>>> It is definitely
>>> worth having a string type iio_info element.
>>> I wonder if we want to allow free naming?  Could we define an enum to cover
>>> 'string' type iio_info elements?
>> It is not intended to be used by in kernel users, it is just meant as a
>> replacement for the handcrafted per channel sysfs attributes.
> Agreed that for some of these parameters such a use wouldn't make much
> sense, but
> it seems likely that something will come along at some point where it does
> so we probably
> want this at the back of our minds...

Yes, I totally agree. E.g. for power management we definitely want in kernel
API support at some point. But I just don't know how the API for this should
look like right now.

>>
>>>> Signed-off-by: Lars-Peter Clausen<lars@metafoo.de>
>>>> ---
>>>>    drivers/staging/iio/iio.h               |   23 ++++++++++++
>>>>    drivers/staging/iio/industrialio-core.c |   61
>>>> ++++++++++++++++++++++++++++---
>>>>    2 files changed, 78 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
>>>> index be6ced3..2a0cfbb 100644
>>>> --- a/drivers/staging/iio/iio.h
>>>> +++ b/drivers/staging/iio/iio.h
>>>> @@ -88,6 +88,25 @@ enum iio_endian {
>>>>        IIO_LE,
>>>>    };
>>>>
>>>> +struct iio_chan_spec;
>>>> +struct iio_dev;
>>>> +
>>>> +/**
>>>> + * struct iio_chan_spec_ext_info - Extended channel info attribute
>>>> + * @name:    Info attribute name
>>>> + * @shared:    Whether this attribute is shared between all channels.
>>>> + * @read:    Read callback for this info attribute, may be NULL.
>>>> + * @write:    Write callback for this info attribute, may be NULL.
>>>> + */
>>>> +struct iio_chan_spec_ext_info {
>>>> +    const char *name;
>>>> +    bool shared;
>>>> +    ssize_t (*read)(struct iio_dev *, struct iio_chan_spec const *,
>>>> +            char *buf);
>>>> +    ssize_t (*write)(struct iio_dev *, struct iio_chan_spec const *,
>>>> +            const char *buf, size_t len);
>>>> +};
>>> Is it worth making the callbacks also take the const char *name from above
>>> in the structure or
>>> define some sort of 'private' integer in here. I'm just thinking of aiding
>>> reuse of the
>>> callbacks
>> Yes, I've thought about it and decided against it, since we don't have a
>> user for this right now. So chances are that I'd get the interface wrong and
>> we need to modify it once we have real users anyway. Also coccinelle makes
>> it quite trivial to refactor a function or callback to take an additional
>> parameter.
> All right, then I agree that this approach is fine for now.  We'll keep it
> in mind for
> possibly needing updating as it gets more users.
> 
> Acked-by: Jonathan Cameron <jic23@kernel.org>  Preferably with the bits
> suggested split
> out into a precursor patch.

Ok, thanks.