[PATCH 1/3] staging:iio:dac: Remove unused dac.h includes

[PATCH 1/3] staging:iio:dac: Remove unused dac.h includes

[Lars-Peter Clausen]

None of these drivers use anything from dac.h, so remove the include.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/dac/ad5064.c      |    1 -
 drivers/staging/iio/dac/ad5360.c      |    1 -
 drivers/staging/iio/dac/ad5380.c      |    2 --
 drivers/staging/iio/dac/ad5421.c      |    1 -
 drivers/staging/iio/dac/ad5446.c      |    1 -
 drivers/staging/iio/dac/ad5504.c      |    2 +-
 drivers/staging/iio/dac/ad5624r_spi.c |    2 +-
 drivers/staging/iio/dac/ad5686.c      |    1 -
 drivers/staging/iio/dac/ad5764.c      |    1 -
 drivers/staging/iio/dac/ad5791.c      |    2 +-
 10 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/iio/dac/ad5064.c b/drivers/staging/iio/dac/ad5064.c
index 2394f05..276af02 100644
--- a/drivers/staging/iio/dac/ad5064.c
+++ b/drivers/staging/iio/dac/ad5064.c
@@ -18,7 +18,6 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include "dac.h"
 
 #define AD5064_MAX_DAC_CHANNELS			8
 #define AD5064_MAX_VREFS			4
diff --git a/drivers/staging/iio/dac/ad5360.c b/drivers/staging/iio/dac/ad5360.c
index 26cac42..0304079 100644
--- a/drivers/staging/iio/dac/ad5360.c
+++ b/drivers/staging/iio/dac/ad5360.c
@@ -18,7 +18,6 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include "dac.h"
 
 #define AD5360_CMD(x)				((x) << 22)
 #define AD5360_ADDR(x)				((x) << 16)
diff --git a/drivers/staging/iio/dac/ad5380.c b/drivers/staging/iio/dac/ad5380.c
index 7a35cc6..5dfb445 100644
--- a/drivers/staging/iio/dac/ad5380.c
+++ b/drivers/staging/iio/dac/ad5380.c
@@ -20,8 +20,6 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include "dac.h"
-
 
 #define AD5380_REG_DATA(x)	(((x) << 2) | 3)
 #define AD5380_REG_OFFSET(x)	(((x) << 2) | 2)
diff --git a/drivers/staging/iio/dac/ad5421.c b/drivers/staging/iio/dac/ad5421.c
index ffbd4c2..ea2f83b 100644
--- a/drivers/staging/iio/dac/ad5421.c
+++ b/drivers/staging/iio/dac/ad5421.c
@@ -19,7 +19,6 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
-#include "dac.h"
 #include "ad5421.h"
 
 
diff --git a/drivers/staging/iio/dac/ad5446.c b/drivers/staging/iio/dac/ad5446.c
index b6b7eec..f769b74 100644
--- a/drivers/staging/iio/dac/ad5446.c
+++ b/drivers/staging/iio/dac/ad5446.c
@@ -20,7 +20,6 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include "dac.h"
 
 #include "ad5446.h"
 
diff --git a/drivers/staging/iio/dac/ad5504.c b/drivers/staging/iio/dac/ad5504.c
index 19e65e1..07b31f4 100644
--- a/drivers/staging/iio/dac/ad5504.c
+++ b/drivers/staging/iio/dac/ad5504.c
@@ -19,7 +19,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
-#include "dac.h"
+
 #include "ad5504.h"
 
 static int ad5504_spi_write(struct spi_device *spi, u8 addr, u16 val)
diff --git a/drivers/staging/iio/dac/ad5624r_spi.c b/drivers/staging/iio/dac/ad5624r_spi.c
index 2ce0544..ef6a320 100644
--- a/drivers/staging/iio/dac/ad5624r_spi.c
+++ b/drivers/staging/iio/dac/ad5624r_spi.c
@@ -18,7 +18,7 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include "dac.h"
+
 #include "ad5624r.h"
 
 static int ad5624r_spi_write(struct spi_device *spi,
diff --git a/drivers/staging/iio/dac/ad5686.c b/drivers/staging/iio/dac/ad5686.c
index a3881d7..cf833c9 100644
--- a/drivers/staging/iio/dac/ad5686.c
+++ b/drivers/staging/iio/dac/ad5686.c
@@ -18,7 +18,6 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include "dac.h"
 
 #define AD5686_DAC_CHANNELS			4
 
diff --git a/drivers/staging/iio/dac/ad5764.c b/drivers/staging/iio/dac/ad5764.c
index 03dbd93..ffce304 100644
--- a/drivers/staging/iio/dac/ad5764.c
+++ b/drivers/staging/iio/dac/ad5764.c
@@ -18,7 +18,6 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include "dac.h"
 
 #define AD5764_REG_SF_NOP			0x0
 #define AD5764_REG_SF_CONFIG			0x1
diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
index ad33725..4e955ed 100644
--- a/drivers/staging/iio/dac/ad5791.c
+++ b/drivers/staging/iio/dac/ad5791.c
@@ -19,7 +19,7 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include "dac.h"
+
 #include "ad5791.h"
 
 static int ad5791_spi_write(struct spi_device *spi, u8 addr, u32 val)
-- 
1.7.10

[PATCH 2/3] staging:iio:dac:max517: Convert to channel spec

[Lars-Peter Clausen]

Convert the max517 driver to channel spec. As part of the conversion the
"out_voltage_1&2_raw" property, which updates both channel 1 and 2
simultaneously with the same value, is lost, since this is not really covered by
the IIO spec.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Roland Stigge <stigge@antcom.de>
---
 drivers/staging/iio/dac/max517.c |  145 ++++++++++++++------------------------
 1 file changed, 53 insertions(+), 92 deletions(-)

diff --git a/drivers/staging/iio/dac/max517.c b/drivers/staging/iio/dac/max517.c
index 65d6349..ec8c88c 100644
--- a/drivers/staging/iio/dac/max517.c
+++ b/drivers/staging/iio/dac/max517.c
@@ -27,7 +27,6 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-#include "dac.h"
 
 #include "max517.h"
 
@@ -55,22 +54,14 @@ struct max517_data {
  *          bit 1: channel 2
  * (this way, it's possible to set both channels at once)
  */
-static ssize_t max517_set_value(struct device *dev,
-				 struct device_attribute *attr,
-				 const char *buf, size_t count, int channel)
+static ssize_t max517_set_value(struct iio_dev *indio_dev,
+	long val, int channel)
 {
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct max517_data *data = iio_priv(indio_dev);
 	struct i2c_client *client = data->client;
 	u8 outbuf[4]; /* 1x or 2x command + value */
 	int outbuf_size = 0;
 	int res;
-	long val;
-
-	res = strict_strtol(buf, 10, &val);
-
-	if (res)
-		return res;
 
 	if (val < 0 || val > 255)
 		return -EINVAL;
@@ -95,89 +86,46 @@ static ssize_t max517_set_value(struct device *dev,
 	if (res < 0)
 		return res;
 
-	return count;
-}
-
-static ssize_t max517_set_value_1(struct device *dev,
-				 struct device_attribute *attr,
-				 const char *buf, size_t count)
-{
-	return max517_set_value(dev, attr, buf, count, 1);
-}
-static IIO_DEV_ATTR_OUT_RAW(1, max517_set_value_1, 0);
-
-static ssize_t max517_set_value_2(struct device *dev,
-				 struct device_attribute *attr,
-				 const char *buf, size_t count)
-{
-	return max517_set_value(dev, attr, buf, count, 2);
-}
-static IIO_DEV_ATTR_OUT_RAW(2, max517_set_value_2, 1);
-
-static ssize_t max517_set_value_both(struct device *dev,
-				 struct device_attribute *attr,
-				 const char *buf, size_t count)
-{
-	return max517_set_value(dev, attr, buf, count, 3);
+	return 0;
 }
-static IIO_DEVICE_ATTR_NAMED(out_voltage1and2_raw,
-			     out_voltage1&2_raw, S_IWUSR, NULL,
-			     max517_set_value_both, -1);
 
-static ssize_t max517_show_scale(struct device *dev,
-				struct device_attribute *attr,
-				char *buf, int channel)
+static int max517_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long m)
 {
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct max517_data *data = iio_priv(indio_dev);
-	/* Corresponds to Vref / 2^(bits) */
-	unsigned int scale_uv = (data->vref_mv[channel - 1] * 1000) >> 8;
-
-	return sprintf(buf, "%d.%03d\n", scale_uv / 1000, scale_uv % 1000);
+	int scale_uv;
+
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		/* Corresponds to Vref / 2^(bits) */
+		scale_uv = (data->vref_mv[chan->channel] * 1000) >> 8;
+		*val =  scale_uv / 1000000;
+		*val2 = scale_uv % 1000000;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		break;
+	}
+	return -EINVAL;
 }
 
-static ssize_t max517_show_scale1(struct device *dev,
-				struct device_attribute *attr,
-				char *buf)
+static int max517_write_raw(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, int val, int val2, long mask)
 {
-	return max517_show_scale(dev, attr, buf, 1);
-}
-static IIO_DEVICE_ATTR(out_voltage1_scale, S_IRUGO,
-		       max517_show_scale1, NULL, 0);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = max517_set_value(indio_dev, val, chan->channel);
+		break;
+	default:
+		ret = -EINVAL;
+	}
 
-static ssize_t max517_show_scale2(struct device *dev,
-				struct device_attribute *attr,
-				char *buf)
-{
-	return max517_show_scale(dev, attr, buf, 2);
+	return ret;
 }
-static IIO_DEVICE_ATTR(out_voltage2_scale, S_IRUGO,
-		       max517_show_scale2, NULL, 0);
-
-/* On MAX517 variant, we have one output */
-static struct attribute *max517_attributes[] = {
-	&iio_dev_attr_out_voltage1_raw.dev_attr.attr,
-	&iio_dev_attr_out_voltage1_scale.dev_attr.attr,
-	NULL
-};
-
-static struct attribute_group max517_attribute_group = {
-	.attrs = max517_attributes,
-};
-
-/* On MAX518 and MAX519 variant, we have two outputs */
-static struct attribute *max518_attributes[] = {
-	&iio_dev_attr_out_voltage1_raw.dev_attr.attr,
-	&iio_dev_attr_out_voltage1_scale.dev_attr.attr,
-	&iio_dev_attr_out_voltage2_raw.dev_attr.attr,
-	&iio_dev_attr_out_voltage2_scale.dev_attr.attr,
-	&iio_dev_attr_out_voltage1and2_raw.dev_attr.attr,
-	NULL
-};
-
-static struct attribute_group max518_attribute_group = {
-	.attrs = max518_attributes,
-};
 
 #ifdef CONFIG_PM_SLEEP
 static int max517_suspend(struct device *dev)
@@ -201,13 +149,24 @@ static SIMPLE_DEV_PM_OPS(max517_pm_ops, max517_suspend, max517_resume);
 #endif
 
 static const struct iio_info max517_info = {
-	.attrs = &max517_attribute_group,
+	.read_raw = max517_read_raw,
+	.write_raw = max517_write_raw,
 	.driver_module = THIS_MODULE,
 };
 
-static const struct iio_info max518_info = {
-	.attrs = &max518_attribute_group,
-	.driver_module = THIS_MODULE,
+#define MAX517_CHANNEL(chan) {				\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.output = 1,						\
+	.channel = (chan),					\
+	.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |		\
+	IIO_CHAN_INFO_SCALE_SEPARATE_BIT,			\
+	.scan_type = IIO_ST('u', 8, 8, 0),	\
+}
+
+static const struct iio_chan_spec max517_channels[] = {
+	MAX517_CHANNEL(0),
+	MAX517_CHANNEL(1)
 };
 
 static int max517_probe(struct i2c_client *client,
@@ -230,12 +189,14 @@ static int max517_probe(struct i2c_client *client,
 	/* establish that the iio_dev is a child of the i2c device */
 	indio_dev->dev.parent = &client->dev;
 
-	/* reduced attribute set for MAX517 */
+	/* reduced channel set for MAX517 */
 	if (id->driver_data == ID_MAX517)
-		indio_dev->info = &max517_info;
+		indio_dev->num_channels = 1;
 	else
-		indio_dev->info = &max518_info;
+		indio_dev->num_channels = 2;
+	indio_dev->channels = max517_channels;
 	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &max517_info;
 
 	/*
 	 * Reference voltage on MAX518 and default is 5V, else take vref_mv
-- 
1.7.10

Re: [PATCH 2/3] staging:iio:dac:max517: Convert to channel spec

[Jonathan Cameron]

On 05/11/2012 06:04 PM, Lars-Peter Clausen wrote:
> Convert the max517 driver to channel spec. As part of the conversion the
> "out_voltage_1&2_raw" property, which updates both channel 1 and 2
> simultaneously with the same value, is lost, since this is not really covered by
> the IIO spec.
We'll have to introduce a buffered option for output at somepoint to
cover the equivalent of that out_voltage_1&2_raw attribute.

Roland, how vital was that to you?
We also have a channel rename in here I think, 1->0 and 2->1. Description
of the patch should probably note that.

Couple of trivial points inline. Nothing to stop me acking,
but obviously you should get an ack from Roland as well.

Roland, making these changes will make it easy to take this
out of staging as it is abi compliant after this patch.

> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> Cc: Roland Stigge <stigge@antcom.de>
> ---
>  drivers/staging/iio/dac/max517.c |  145 ++++++++++++++------------------------
>  1 file changed, 53 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/staging/iio/dac/max517.c b/drivers/staging/iio/dac/max517.c
> index 65d6349..ec8c88c 100644
> --- a/drivers/staging/iio/dac/max517.c
> +++ b/drivers/staging/iio/dac/max517.c
> @@ -27,7 +27,6 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include "dac.h"
>  
>  #include "max517.h"
>  
> @@ -55,22 +54,14 @@ struct max517_data {
>   *          bit 1: channel 2
>   * (this way, it's possible to set both channels at once)
>   */
> -static ssize_t max517_set_value(struct device *dev,
> -				 struct device_attribute *attr,
> -				 const char *buf, size_t count, int channel)
given where this is used, returning int would be preferable...
It gets stuck in an int anyway...
> +static ssize_t max517_set_value(struct iio_dev *indio_dev,
> +	long val, int channel)
>  {
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct max517_data *data = iio_priv(indio_dev);
>  	struct i2c_client *client = data->client;
>  	u8 outbuf[4]; /* 1x or 2x command + value */
>  	int outbuf_size = 0;
>  	int res;
> -	long val;
> -
> -	res = strict_strtol(buf, 10, &val);
> -
> -	if (res)
> -		return res;
>  
>  	if (val < 0 || val > 255)
>  		return -EINVAL;
> @@ -95,89 +86,46 @@ static ssize_t max517_set_value(struct device *dev,
>  	if (res < 0)
>  		return res;
>  
> -	return count;
> -}
> -
> -static ssize_t max517_set_value_1(struct device *dev,
> -				 struct device_attribute *attr,
> -				 const char *buf, size_t count)
> -{
> -	return max517_set_value(dev, attr, buf, count, 1);
> -}
> -static IIO_DEV_ATTR_OUT_RAW(1, max517_set_value_1, 0);
> -
> -static ssize_t max517_set_value_2(struct device *dev,
> -				 struct device_attribute *attr,
> -				 const char *buf, size_t count)
> -{
> -	return max517_set_value(dev, attr, buf, count, 2);
> -}
> -static IIO_DEV_ATTR_OUT_RAW(2, max517_set_value_2, 1);
> -
> -static ssize_t max517_set_value_both(struct device *dev,
> -				 struct device_attribute *attr,
> -				 const char *buf, size_t count)
> -{
> -	return max517_set_value(dev, attr, buf, count, 3);
> +	return 0;
>  }
> -static IIO_DEVICE_ATTR_NAMED(out_voltage1and2_raw,
> -			     out_voltage1&2_raw, S_IWUSR, NULL,
> -			     max517_set_value_both, -1);
>  
> -static ssize_t max517_show_scale(struct device *dev,
> -				struct device_attribute *attr,
> -				char *buf, int channel)
> +static int max517_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long m)
>  {
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct max517_data *data = iio_priv(indio_dev);
> -	/* Corresponds to Vref / 2^(bits) */
> -	unsigned int scale_uv = (data->vref_mv[channel - 1] * 1000) >> 8;
> -
> -	return sprintf(buf, "%d.%03d\n", scale_uv / 1000, scale_uv % 1000);
> +	int scale_uv;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		/* Corresponds to Vref / 2^(bits) */
> +		scale_uv = (data->vref_mv[chan->channel] * 1000) >> 8;
> +		*val =  scale_uv / 1000000;
> +		*val2 = scale_uv % 1000000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		break;
> +	}
> +	return -EINVAL;
>  }
>  
> -static ssize_t max517_show_scale1(struct device *dev,
> -				struct device_attribute *attr,
> -				char *buf)
> +static int max517_write_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, int val, int val2, long mask)
>  {
> -	return max517_show_scale(dev, attr, buf, 1);
> -}
> -static IIO_DEVICE_ATTR(out_voltage1_scale, S_IRUGO,
> -		       max517_show_scale1, NULL, 0);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = max517_set_value(indio_dev, val, chan->channel);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
>  
> -static ssize_t max517_show_scale2(struct device *dev,
> -				struct device_attribute *attr,
> -				char *buf)
> -{
> -	return max517_show_scale(dev, attr, buf, 2);
> +	return ret;
>  }
> -static IIO_DEVICE_ATTR(out_voltage2_scale, S_IRUGO,
> -		       max517_show_scale2, NULL, 0);
> -
> -/* On MAX517 variant, we have one output */
> -static struct attribute *max517_attributes[] = {
> -	&iio_dev_attr_out_voltage1_raw.dev_attr.attr,
> -	&iio_dev_attr_out_voltage1_scale.dev_attr.attr,
> -	NULL
> -};
> -
> -static struct attribute_group max517_attribute_group = {
> -	.attrs = max517_attributes,
> -};
> -
> -/* On MAX518 and MAX519 variant, we have two outputs */
> -static struct attribute *max518_attributes[] = {
> -	&iio_dev_attr_out_voltage1_raw.dev_attr.attr,
> -	&iio_dev_attr_out_voltage1_scale.dev_attr.attr,
> -	&iio_dev_attr_out_voltage2_raw.dev_attr.attr,
> -	&iio_dev_attr_out_voltage2_scale.dev_attr.attr,
> -	&iio_dev_attr_out_voltage1and2_raw.dev_attr.attr,
> -	NULL
> -};
> -
> -static struct attribute_group max518_attribute_group = {
> -	.attrs = max518_attributes,
> -};
>  
>  #ifdef CONFIG_PM_SLEEP
>  static int max517_suspend(struct device *dev)
> @@ -201,13 +149,24 @@ static SIMPLE_DEV_PM_OPS(max517_pm_ops, max517_suspend, max517_resume);
>  #endif
>  
>  static const struct iio_info max517_info = {
> -	.attrs = &max517_attribute_group,
> +	.read_raw = max517_read_raw,
> +	.write_raw = max517_write_raw,
>  	.driver_module = THIS_MODULE,
>  };
>  
> -static const struct iio_info max518_info = {
> -	.attrs = &max518_attribute_group,
> -	.driver_module = THIS_MODULE,
> +#define MAX517_CHANNEL(chan) {				\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.output = 1,						\
> +	.channel = (chan),					\
> +	.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |		\
> +	IIO_CHAN_INFO_SCALE_SEPARATE_BIT,			\
I suppose the scan_type acts as kind of documentation, but
it's not really relevant or necessary here... I'd scrap it.
> +	.scan_type = IIO_ST('u', 8, 8, 0),	\
> +}
> +
> +static const struct iio_chan_spec max517_channels[] = {
> +	MAX517_CHANNEL(0),
> +	MAX517_CHANNEL(1)
>  };
>  
>  static int max517_probe(struct i2c_client *client,
> @@ -230,12 +189,14 @@ static int max517_probe(struct i2c_client *client,
>  	/* establish that the iio_dev is a child of the i2c device */
>  	indio_dev->dev.parent = &client->dev;
>  
> -	/* reduced attribute set for MAX517 */
> +	/* reduced channel set for MAX517 */
>  	if (id->driver_data == ID_MAX517)
> -		indio_dev->info = &max517_info;
> +		indio_dev->num_channels = 1;
>  	else
> -		indio_dev->info = &max518_info;
> +		indio_dev->num_channels = 2;
> +	indio_dev->channels = max517_channels;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &max517_info;
>  
>  	/*
>  	 * Reference voltage on MAX518 and default is 5V, else take vref_mv

Re: [PATCH 2/3] staging:iio:dac:max517: Convert to channel spec

[Roland Stigge]

Hi,

On 12/05/12 20:51, Jonathan Cameron wrote:
> On 05/11/2012 06:04 PM, Lars-Peter Clausen wrote:
>> Convert the max517 driver to channel spec. As part of the conversion the
>> "out_voltage_1&2_raw" property, which updates both channel 1 and 2
>> simultaneously with the same value, is lost, since this is not really covered by
>> the IIO spec.
> We'll have to introduce a buffered option for output at somepoint to
> cover the equivalent of that out_voltage_1&2_raw attribute.
> 
> Roland, how vital was that to you?

I'm generally fine with the proposed changes and can ack it. However, we
should provide a replacement for the old "1&2" attribute. It supports a
certain hardware feature of this chip. No need to sacrifice it.

Thanks,

Roland

Re: [PATCH 2/3] staging:iio:dac:max517: Convert to channel spec

[Lars-Peter Clausen]

On 05/12/2012 11:44 PM, Roland Stigge wrote:
> Hi,
> 
> On 12/05/12 20:51, Jonathan Cameron wrote:
>> On 05/11/2012 06:04 PM, Lars-Peter Clausen wrote:
>>> Convert the max517 driver to channel spec. As part of the conversion the
>>> "out_voltage_1&2_raw" property, which updates both channel 1 and 2
>>> simultaneously with the same value, is lost, since this is not really covered by
>>> the IIO spec.
>> We'll have to introduce a buffered option for output at somepoint to
>> cover the equivalent of that out_voltage_1&2_raw attribute.
>>
>> Roland, how vital was that to you?
> 
> I'm generally fine with the proposed changes and can ack it. However, we
> should provide a replacement for the old "1&2" attribute. It supports a
> certain hardware feature of this chip. No need to sacrifice it.

I don't think it makes sense to keep it in it's current form. First of all this
isn't really covered by the IIO spec to have one attribute which changes two
(or more) output at once and secondly it is artificially limited to setting the
same value on both outputs, while the chip doesn't have this limitations and
you can set different values on both outputs at once. So I'm not so sure how
useful it even is in it's current form.

The proper solution for this is to implement buffered support for output
devices and I do have some experimental code for that. But I'd like to move the
DACs out of staging. And this driver is sort of the main blocker right now. If
you insist on keeping the "1&2" attribute, I guess it's best to just leave the
max517 in staging for now, while we move the other DAC drivers out.

- Lars

Re: [PATCH 2/3] staging:iio:dac:max517: Convert to channel spec

[Roland Stigge]

Hi,

On 13/05/12 09:25, Lars-Peter Clausen wrote:
>> I'm generally fine with the proposed changes and can ack it. However, we
>> should provide a replacement for the old "1&2" attribute. It supports a
>> certain hardware feature of this chip. No need to sacrifice it.
> 
> I don't think it makes sense to keep it in it's current form. First of all this
> isn't really covered by the IIO spec to have one attribute which changes two
> (or more) output at once and secondly it is artificially limited to setting the
> same value on both outputs, while the chip doesn't have this limitations and
> you can set different values on both outputs at once.

Please consider this as a special feature of the chip (setting the
outputs timely at once), not a "limitation", since you can also set them
individually.

However, I'm not sure how many people are actually using this feature.
So feel free to patch as proposed and go forward moving the driver out
of staging.

> The proper solution for this is to implement buffered support for output
> devices and I do have some experimental code for that.

Good! When you already have code, please provide, so I can test it.
Maybe we can re-integrate it into the "new" driver later on.

Thanks for working on this,

Roland

Re: [PATCH 2/3] staging:iio:dac:max517: Convert to channel spec

[Lars-Peter Clausen]

On 05/13/2012 11:06 AM, Roland Stigge wrote:
> Hi,
> 
> On 13/05/12 09:25, Lars-Peter Clausen wrote:
>>> I'm generally fine with the proposed changes and can ack it. However, we
>>> should provide a replacement for the old "1&2" attribute. It supports a
>>> certain hardware feature of this chip. No need to sacrifice it.
>>
>> I don't think it makes sense to keep it in it's current form. First of all this
>> isn't really covered by the IIO spec to have one attribute which changes two
>> (or more) output at once and secondly it is artificially limited to setting the
>> same value on both outputs, while the chip doesn't have this limitations and
>> you can set different values on both outputs at once.
> 
> Please consider this as a special feature of the chip (setting the
> outputs timely at once), not a "limitation", since you can also set them
> individually.
> 

What I meant was that it is not a limitation of the chip, but the driver only
allows limited use of that feature. You can only set the same value on both
outputs simultaneously, which I think has not so many practical use-cases.
You'd rather want to set two different values on output 1 and 2.

- Lars

Re: [PATCH 2/3] staging:iio:dac:max517: Convert to channel spec

[Roland Stigge]

On 13/05/12 11:20, Lars-Peter Clausen wrote:
[regarding the "1&2" output]
> You can only set the same value on both
> outputs simultaneously, which I think has not so many practical use-cases.

_If_ you want to switch outputs "simultaneously", it's hard to do with
the max51x chips due to relatively slow I2C communication. Don't have
timing diagrams right now how close you get if you switch the two
outputs directly one after another, though.

But right, it's not highest priority...

> You'd rather want to set two different values on output 1 and 2.

We have and will have attributes for those anyway, don't we?

Roland

Re: [PATCH 2/3] staging:iio:dac:max517: Convert to channel spec

[Lars-Peter Clausen]

On 05/13/2012 11:41 AM, Roland Stigge wrote:
> On 13/05/12 11:20, Lars-Peter Clausen wrote:
> [regarding the "1&2" output]
>> You can only set the same value on both
>> outputs simultaneously, which I think has not so many practical use-cases.
> 
> _If_ you want to switch outputs "simultaneously", it's hard to do with
> the max51x chips due to relatively slow I2C communication. Don't have
> timing diagrams right now how close you get if you switch the two
> outputs directly one after another, though.

Hm, I thought the whole purpose of the "1&2" attribute was to update them
simultaneously. According to the datasheet the chip will latch new values on a
stop condition. So if you write both channels in one transfer they will be
updated simultaneously. If you don't care about that you might as well just
update the channels by writing to their respective attribute.

> 
> But right, it's not highest priority...
> 
>> You'd rather want to set two different values on output 1 and 2.
> 
> We have and will have attributes for those anyway, don't we?
> 

Yes, sure. But again I meant simultaneously, which will be possible with buffer
output support.

- Lars

Re: [PATCH 2/3] staging:iio:dac:max517: Convert to channel spec

[Lars-Peter Clausen]

On 05/12/2012 08:51 PM, Jonathan Cameron wrote:
> [...]
>> -static const struct iio_info max518_info = {
>> -	.attrs = &max518_attribute_group,
>> -	.driver_module = THIS_MODULE,
>> +#define MAX517_CHANNEL(chan) {				\
>> +	.type = IIO_VOLTAGE,					\
>> +	.indexed = 1,						\
>> +	.output = 1,						\
>> +	.channel = (chan),					\
>> +	.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |		\
>> +	IIO_CHAN_INFO_SCALE_SEPARATE_BIT,			\
> I suppose the scan_type acts as kind of documentation, but
> it's not really relevant or necessary here... I'd scrap it.

I'd like to keep it, because I think it will be used in the future. E.g. for
buffer output, but also some other things might make use of it.

>> +	.scan_type = IIO_ST('u', 8, 8, 0),	\
>> +}

Re: [PATCH 2/3] staging:iio:dac:max517: Convert to channel spec

[Jonathan Cameron]

On 5/14/2012 3:31 PM, Lars-Peter Clausen wrote:
> On 05/12/2012 08:51 PM, Jonathan Cameron wrote:
>> [...]
>>> -static const struct iio_info max518_info = {
>>> -	.attrs =&max518_attribute_group,
>>> -	.driver_module = THIS_MODULE,
>>> +#define MAX517_CHANNEL(chan) {				\
>>> +	.type = IIO_VOLTAGE,					\
>>> +	.indexed = 1,						\
>>> +	.output = 1,						\
>>> +	.channel = (chan),					\
>>> +	.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |		\
>>> +	IIO_CHAN_INFO_SCALE_SEPARATE_BIT,			\
>> I suppose the scan_type acts as kind of documentation, but
>> it's not really relevant or necessary here... I'd scrap it.
>
> I'd like to keep it, because I think it will be used in the future. E.g. for
> buffer output, but also some other things might make use of it.
I'll let it go then...
>
>>> +	.scan_type = IIO_ST('u', 8, 8, 0),	\
>>> +}
>

[PATCH 3/3] staging:iio:dac: Remove dac.h

[Lars-Peter Clausen]

Now that all drivers have been converted to channel spec, we can finally remove
the dac.h file since it is no longer used.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/dac/dac.h |    6 ------
 1 file changed, 6 deletions(-)
 delete mode 100644 drivers/staging/iio/dac/dac.h

diff --git a/drivers/staging/iio/dac/dac.h b/drivers/staging/iio/dac/dac.h
deleted file mode 100644
index 0754d71..0000000
--- a/drivers/staging/iio/dac/dac.h
+++ /dev/null
@@ -1,6 +0,0 @@
-/*
- * dac.h - sysfs attributes associated with DACs
- */
-
-#define IIO_DEV_ATTR_OUT_RAW(_num, _store, _addr)				\
-	IIO_DEVICE_ATTR(out_voltage##_num##_raw, S_IWUSR, NULL, _store, _addr)
-- 
1.7.10

Re: [PATCH 3/3] staging:iio:dac: Remove dac.h

[Jonathan Cameron]

On 05/11/2012 06:04 PM, Lars-Peter Clausen wrote:
> Now that all drivers have been converted to channel spec, we can finally remove
> the dac.h file since it is no longer used.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/staging/iio/dac/dac.h |    6 ------
>  1 file changed, 6 deletions(-)
>  delete mode 100644 drivers/staging/iio/dac/dac.h
> 
> diff --git a/drivers/staging/iio/dac/dac.h b/drivers/staging/iio/dac/dac.h
> deleted file mode 100644
> index 0754d71..0000000
> --- a/drivers/staging/iio/dac/dac.h
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -/*
> - * dac.h - sysfs attributes associated with DACs
> - */
> -
> -#define IIO_DEV_ATTR_OUT_RAW(_num, _store, _addr)				\
> -	IIO_DEVICE_ATTR(out_voltage##_num##_raw, S_IWUSR, NULL, _store, _addr)

Re: [PATCH 1/3] staging:iio:dac: Remove unused dac.h includes

[Jonathan Cameron]

On 05/11/2012 06:04 PM, Lars-Peter Clausen wrote:
> None of these drivers use anything from dac.h, so remove the include.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/staging/iio/dac/ad5064.c      |    1 -
>  drivers/staging/iio/dac/ad5360.c      |    1 -
>  drivers/staging/iio/dac/ad5380.c      |    2 --
>  drivers/staging/iio/dac/ad5421.c      |    1 -
>  drivers/staging/iio/dac/ad5446.c      |    1 -
>  drivers/staging/iio/dac/ad5504.c      |    2 +-
>  drivers/staging/iio/dac/ad5624r_spi.c |    2 +-
>  drivers/staging/iio/dac/ad5686.c      |    1 -
>  drivers/staging/iio/dac/ad5764.c      |    1 -
>  drivers/staging/iio/dac/ad5791.c      |    2 +-
>  10 files changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/iio/dac/ad5064.c b/drivers/staging/iio/dac/ad5064.c
> index 2394f05..276af02 100644
> --- a/drivers/staging/iio/dac/ad5064.c
> +++ b/drivers/staging/iio/dac/ad5064.c
> @@ -18,7 +18,6 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include "dac.h"
>  
>  #define AD5064_MAX_DAC_CHANNELS			8
>  #define AD5064_MAX_VREFS			4
> diff --git a/drivers/staging/iio/dac/ad5360.c b/drivers/staging/iio/dac/ad5360.c
> index 26cac42..0304079 100644
> --- a/drivers/staging/iio/dac/ad5360.c
> +++ b/drivers/staging/iio/dac/ad5360.c
> @@ -18,7 +18,6 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include "dac.h"
>  
>  #define AD5360_CMD(x)				((x) << 22)
>  #define AD5360_ADDR(x)				((x) << 16)
> diff --git a/drivers/staging/iio/dac/ad5380.c b/drivers/staging/iio/dac/ad5380.c
> index 7a35cc6..5dfb445 100644
> --- a/drivers/staging/iio/dac/ad5380.c
> +++ b/drivers/staging/iio/dac/ad5380.c
> @@ -20,8 +20,6 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include "dac.h"
> -
>  
>  #define AD5380_REG_DATA(x)	(((x) << 2) | 3)
>  #define AD5380_REG_OFFSET(x)	(((x) << 2) | 2)
> diff --git a/drivers/staging/iio/dac/ad5421.c b/drivers/staging/iio/dac/ad5421.c
> index ffbd4c2..ea2f83b 100644
> --- a/drivers/staging/iio/dac/ad5421.c
> +++ b/drivers/staging/iio/dac/ad5421.c
> @@ -19,7 +19,6 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/events.h>
> -#include "dac.h"
>  #include "ad5421.h"
>  
>  
> diff --git a/drivers/staging/iio/dac/ad5446.c b/drivers/staging/iio/dac/ad5446.c
> index b6b7eec..f769b74 100644
> --- a/drivers/staging/iio/dac/ad5446.c
> +++ b/drivers/staging/iio/dac/ad5446.c
> @@ -20,7 +20,6 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include "dac.h"
>  
>  #include "ad5446.h"
>  
> diff --git a/drivers/staging/iio/dac/ad5504.c b/drivers/staging/iio/dac/ad5504.c
> index 19e65e1..07b31f4 100644
> --- a/drivers/staging/iio/dac/ad5504.c
> +++ b/drivers/staging/iio/dac/ad5504.c
> @@ -19,7 +19,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/events.h>
> -#include "dac.h"
> +
>  #include "ad5504.h"
>  
>  static int ad5504_spi_write(struct spi_device *spi, u8 addr, u16 val)
> diff --git a/drivers/staging/iio/dac/ad5624r_spi.c b/drivers/staging/iio/dac/ad5624r_spi.c
> index 2ce0544..ef6a320 100644
> --- a/drivers/staging/iio/dac/ad5624r_spi.c
> +++ b/drivers/staging/iio/dac/ad5624r_spi.c
> @@ -18,7 +18,7 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include "dac.h"
> +
>  #include "ad5624r.h"
>  
>  static int ad5624r_spi_write(struct spi_device *spi,
> diff --git a/drivers/staging/iio/dac/ad5686.c b/drivers/staging/iio/dac/ad5686.c
> index a3881d7..cf833c9 100644
> --- a/drivers/staging/iio/dac/ad5686.c
> +++ b/drivers/staging/iio/dac/ad5686.c
> @@ -18,7 +18,6 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include "dac.h"
>  
>  #define AD5686_DAC_CHANNELS			4
>  
> diff --git a/drivers/staging/iio/dac/ad5764.c b/drivers/staging/iio/dac/ad5764.c
> index 03dbd93..ffce304 100644
> --- a/drivers/staging/iio/dac/ad5764.c
> +++ b/drivers/staging/iio/dac/ad5764.c
> @@ -18,7 +18,6 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include "dac.h"
>  
>  #define AD5764_REG_SF_NOP			0x0
>  #define AD5764_REG_SF_CONFIG			0x1
> diff --git a/drivers/staging/iio/dac/ad5791.c b/drivers/staging/iio/dac/ad5791.c
> index ad33725..4e955ed 100644
> --- a/drivers/staging/iio/dac/ad5791.c
> +++ b/drivers/staging/iio/dac/ad5791.c
> @@ -19,7 +19,7 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> -#include "dac.h"
> +
>  #include "ad5791.h"
>  
>  static int ad5791_spi_write(struct spi_device *spi, u8 addr, u32 val)