linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] staging: iio: ad7606: move driver out of staging
@ 2016-11-11  6:34 Eva Rachel Retuya
  2016-11-11  6:34 ` [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale Eva Rachel Retuya
  2016-11-11  6:34 ` [PATCH 2/2] staging: iio: ad7606: move out of staging Eva Rachel Retuya
  0 siblings, 2 replies; 19+ messages in thread
From: Eva Rachel Retuya @ 2016-11-11  6:34 UTC (permalink / raw)
  To: linux-iio, devel, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh,
	Eva Rachel Retuya

Address the last remaining TODO [1] for this driver and move it from staging
into mainline.

[1] https://marc.info/?l=linux-iio&m=147689684332118&w=2 

Eva Rachel Retuya (2):
  staging: iio: ad7606: replace range/range_available with corresponding
    scale
  staging: iio: ad7606: move out of staging

 drivers/iio/adc/Kconfig              |  34 +++
 drivers/iio/adc/Makefile             |   3 +
 drivers/iio/adc/ad7606.c             | 519 +++++++++++++++++++++++++++++++++
 drivers/iio/adc/ad7606.h             |  78 +++++
 drivers/iio/adc/ad7606_par.c         | 112 ++++++++
 drivers/iio/adc/ad7606_spi.c         |  78 +++++
 drivers/staging/iio/adc/Kconfig      |  34 ---
 drivers/staging/iio/adc/Makefile     |   4 -
 drivers/staging/iio/adc/ad7606.c     | 543 -----------------------------------
 drivers/staging/iio/adc/ad7606.h     |  78 -----
 drivers/staging/iio/adc/ad7606_par.c | 112 --------
 drivers/staging/iio/adc/ad7606_spi.c |  78 -----
 12 files changed, 824 insertions(+), 849 deletions(-)
 create mode 100644 drivers/iio/adc/ad7606.c
 create mode 100644 drivers/iio/adc/ad7606.h
 create mode 100644 drivers/iio/adc/ad7606_par.c
 create mode 100644 drivers/iio/adc/ad7606_spi.c
 delete mode 100644 drivers/staging/iio/adc/ad7606.c
 delete mode 100644 drivers/staging/iio/adc/ad7606.h
 delete mode 100644 drivers/staging/iio/adc/ad7606_par.c
 delete mode 100644 drivers/staging/iio/adc/ad7606_spi.c

-- 
2.7.4

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-11  6:34 [PATCH 0/2] staging: iio: ad7606: move driver out of staging Eva Rachel Retuya
@ 2016-11-11  6:34 ` Eva Rachel Retuya
  2016-11-11 14:18   ` Lars-Peter Clausen
  2016-11-11  6:34 ` [PATCH 2/2] staging: iio: ad7606: move out of staging Eva Rachel Retuya
  1 sibling, 1 reply; 19+ messages in thread
From: Eva Rachel Retuya @ 2016-11-11  6:34 UTC (permalink / raw)
  To: linux-iio, devel, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh,
	Eva Rachel Retuya

Eliminate the non-standard attribute in_voltage_range and move its
functionality under the scale attribute. read_raw() has been taken care
of previously so only write_raw() is handled here.

Additionally, rename the attribute in_voltage_range_available into
in_voltage_scale_available.

Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
 drivers/staging/iio/adc/ad7606.c | 56 ++++++++++++----------------------------
 1 file changed, 16 insertions(+), 40 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c
index 4531908..cceb18c 100644
--- a/drivers/staging/iio/adc/ad7606.c
+++ b/drivers/staging/iio/adc/ad7606.c
@@ -161,42 +161,7 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
-static ssize_t ad7606_show_range(struct device *dev,
-				 struct device_attribute *attr, char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ad7606_state *st = iio_priv(indio_dev);
-
-	return sprintf(buf, "%u\n", st->range);
-}
-
-static ssize_t ad7606_store_range(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t count)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ad7606_state *st = iio_priv(indio_dev);
-	unsigned long lval;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &lval);
-	if (ret)
-		return ret;
-
-	if (!(lval == 5000 || lval == 10000))
-		return -EINVAL;
-
-	mutex_lock(&indio_dev->mlock);
-	gpiod_set_value(st->gpio_range, lval == 10000);
-	st->range = lval;
-	mutex_unlock(&indio_dev->mlock);
-
-	return count;
-}
-
-static IIO_DEVICE_ATTR(in_voltage_range, S_IRUGO | S_IWUSR,
-		       ad7606_show_range, ad7606_store_range, 0);
-static IIO_CONST_ATTR(in_voltage_range_available, "5000 10000");
+static IIO_CONST_ATTR(in_voltage_scale_available, "5000 10000");
 
 static int ad7606_oversampling_get_index(unsigned int val)
 {
@@ -221,6 +186,19 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 	int ret;
 
 	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val2)
+			return -EINVAL;
+
+		if (!(val == 5000 || val == 10000))
+			return -EINVAL;
+
+		mutex_lock(&indio_dev->mlock);
+		gpiod_set_value(st->gpio_range, val == 10000);
+		st->range = val;
+		mutex_unlock(&indio_dev->mlock);
+
+		return 0;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		if (val2)
 			return -EINVAL;
@@ -247,8 +225,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 static IIO_CONST_ATTR(oversampling_ratio_available, "1 2 4 8 16 32 64");
 
 static struct attribute *ad7606_attributes_os_and_range[] = {
-	&iio_dev_attr_in_voltage_range.dev_attr.attr,
-	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
+	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
 	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
 	NULL,
 };
@@ -267,8 +244,7 @@ static const struct attribute_group ad7606_attribute_group_os = {
 };
 
 static struct attribute *ad7606_attributes_range[] = {
-	&iio_dev_attr_in_voltage_range.dev_attr.attr,
-	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
+	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
 	NULL,
 };
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/2] staging: iio: ad7606: move out of staging
  2016-11-11  6:34 [PATCH 0/2] staging: iio: ad7606: move driver out of staging Eva Rachel Retuya
  2016-11-11  6:34 ` [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale Eva Rachel Retuya
@ 2016-11-11  6:34 ` Eva Rachel Retuya
  2016-11-11 14:22   ` Lars-Peter Clausen
  2016-11-11 16:10   ` kbuild test robot
  1 sibling, 2 replies; 19+ messages in thread
From: Eva Rachel Retuya @ 2016-11-11  6:34 UTC (permalink / raw)
  To: linux-iio, devel, linux-kernel
  Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh,
	Eva Rachel Retuya

Move the ad7606 driver from staging/iio/adc to iio/adc. Also, update the
corresponding Makefile and Kconfig associated with the change.

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
 drivers/iio/adc/Kconfig              |  34 +++
 drivers/iio/adc/Makefile             |   3 +
 drivers/iio/adc/ad7606.c             | 519 +++++++++++++++++++++++++++++++++++
 drivers/iio/adc/ad7606.h             |  78 ++++++
 drivers/iio/adc/ad7606_par.c         | 112 ++++++++
 drivers/iio/adc/ad7606_spi.c         |  78 ++++++
 drivers/staging/iio/adc/Kconfig      |  34 ---
 drivers/staging/iio/adc/Makefile     |   4 -
 drivers/staging/iio/adc/ad7606.c     | 519 -----------------------------------
 drivers/staging/iio/adc/ad7606.h     |  78 ------
 drivers/staging/iio/adc/ad7606_par.c | 112 --------
 drivers/staging/iio/adc/ad7606_spi.c |  78 ------
 12 files changed, 824 insertions(+), 825 deletions(-)
 create mode 100644 drivers/iio/adc/ad7606.c
 create mode 100644 drivers/iio/adc/ad7606.h
 create mode 100644 drivers/iio/adc/ad7606_par.c
 create mode 100644 drivers/iio/adc/ad7606_spi.c
 delete mode 100644 drivers/staging/iio/adc/ad7606.c
 delete mode 100644 drivers/staging/iio/adc/ad7606.h
 delete mode 100644 drivers/staging/iio/adc/ad7606_par.c
 delete mode 100644 drivers/staging/iio/adc/ad7606_spi.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index c3fe98d..742db8f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -58,6 +58,40 @@ config AD7476
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad7476.
 
+config AD7606
+	tristate "Analog Devices AD7606 ADC driver"
+	depends on GPIOLIB || COMPILE_TEST
+	depends on HAS_IOMEM
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for Analog Devices:
+	  ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC).
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7606.
+
+config AD7606_IFACE_PARALLEL
+	tristate "parallel interface support"
+	depends on AD7606
+	help
+	  Say yes here to include parallel interface support on the AD7606
+	  ADC driver.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7606_parallel.
+
+config AD7606_IFACE_SPI
+	tristate "spi interface support"
+	depends on AD7606
+	depends on SPI
+	help
+	  Say yes here to include parallel interface support on the AD7606
+	  ADC driver.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7606_spi.
+
 config AD7766
 	tristate "Analog Devices AD7766/AD7767 ADC driver"
 	depends on SPI_MASTER
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 96894b3..d06d3a9 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -9,6 +9,9 @@ obj-$(CONFIG_AD7291) += ad7291.o
 obj-$(CONFIG_AD7298) += ad7298.o
 obj-$(CONFIG_AD7923) += ad7923.o
 obj-$(CONFIG_AD7476) += ad7476.o
+obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
+obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
+obj-$(CONFIG_AD7606) += ad7606.o
 obj-$(CONFIG_AD7766) += ad7766.o
 obj-$(CONFIG_AD7791) += ad7791.o
 obj-$(CONFIG_AD7793) += ad7793.o
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
new file mode 100644
index 0000000..cceb18c
--- /dev/null
+++ b/drivers/iio/adc/ad7606.c
@@ -0,0 +1,519 @@
+/*
+ * AD7606 SPI ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/regulator/consumer.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
+#include <linux/sched.h>
+#include <linux/module.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#include "ad7606.h"
+
+static int ad7606_reset(struct ad7606_state *st)
+{
+	if (st->gpio_reset) {
+		gpiod_set_value(st->gpio_reset, 1);
+		ndelay(100); /* t_reset >= 100ns */
+		gpiod_set_value(st->gpio_reset, 0);
+		return 0;
+	}
+
+	return -ENODEV;
+}
+
+static int ad7606_read_samples(struct ad7606_state *st)
+{
+	unsigned int num = st->chip_info->num_channels;
+	u16 *data = st->data;
+	int ret;
+
+	/*
+	 * The frstdata signal is set to high while and after reading the sample
+	 * of the first channel and low for all other channels. This can be used
+	 * to check that the incoming data is correctly aligned. During normal
+	 * operation the data should never become unaligned, but some glitch or
+	 * electrostatic discharge might cause an extra read or clock cycle.
+	 * Monitoring the frstdata signal allows to recover from such failure
+	 * situations.
+	 */
+
+	if (st->gpio_frstdata) {
+		ret = st->bops->read_block(st->dev, 1, data);
+		if (ret)
+			return ret;
+
+		if (!gpiod_get_value(st->gpio_frstdata)) {
+			ad7606_reset(st);
+			return -EIO;
+		}
+
+		data++;
+		num--;
+	}
+
+	return st->bops->read_block(st->dev, num, data);
+}
+
+static irqreturn_t ad7606_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct ad7606_state *st = iio_priv(pf->indio_dev);
+
+	gpiod_set_value(st->gpio_convst, 1);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * ad7606_poll_bh_to_ring() bh of trigger launched polling to ring buffer
+ * @work_s:	the work struct through which this was scheduled
+ *
+ * Currently there is no option in this driver to disable the saving of
+ * timestamps within the ring.
+ * I think the one copy of this at a time was to avoid problems if the
+ * trigger was set far too high and the reads then locked up the computer.
+ **/
+static void ad7606_poll_bh_to_ring(struct work_struct *work_s)
+{
+	struct ad7606_state *st = container_of(work_s, struct ad7606_state,
+						poll_work);
+	struct iio_dev *indio_dev = iio_priv_to_dev(st);
+	int ret;
+
+	ret = ad7606_read_samples(st);
+	if (ret == 0)
+		iio_push_to_buffers_with_timestamp(indio_dev, st->data,
+						   iio_get_time_ns(indio_dev));
+
+	gpiod_set_value(st->gpio_convst, 0);
+	iio_trigger_notify_done(indio_dev->trig);
+}
+
+static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
+{
+	struct ad7606_state *st = iio_priv(indio_dev);
+	int ret;
+
+	st->done = false;
+	gpiod_set_value(st->gpio_convst, 1);
+
+	ret = wait_event_interruptible(st->wq_data_avail, st->done);
+	if (ret)
+		goto error_ret;
+
+	ret = ad7606_read_samples(st);
+	if (ret == 0)
+		ret = st->data[ch];
+
+error_ret:
+	gpiod_set_value(st->gpio_convst, 0);
+
+	return ret;
+}
+
+static int ad7606_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long m)
+{
+	int ret;
+	struct ad7606_state *st = iio_priv(indio_dev);
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7606_scan_direct(indio_dev, chan->address);
+		iio_device_release_direct_mode(indio_dev);
+
+		if (ret < 0)
+			return ret;
+		*val = (short)ret;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->range * 2;
+		*val2 = st->chip_info->channels[0].scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*val = st->oversampling;
+		return IIO_VAL_INT;
+	}
+	return -EINVAL;
+}
+
+static IIO_CONST_ATTR(in_voltage_scale_available, "5000 10000");
+
+static int ad7606_oversampling_get_index(unsigned int val)
+{
+	unsigned char supported[] = {1, 2, 4, 8, 16, 32, 64};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(supported); i++)
+		if (val == supported[i])
+			return i;
+
+	return -EINVAL;
+}
+
+static int ad7606_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long mask)
+{
+	struct ad7606_state *st = iio_priv(indio_dev);
+	int values[3];
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val2)
+			return -EINVAL;
+
+		if (!(val == 5000 || val == 10000))
+			return -EINVAL;
+
+		mutex_lock(&indio_dev->mlock);
+		gpiod_set_value(st->gpio_range, val == 10000);
+		st->range = val;
+		mutex_unlock(&indio_dev->mlock);
+
+		return 0;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		if (val2)
+			return -EINVAL;
+		ret = ad7606_oversampling_get_index(val);
+		if (ret < 0)
+			return ret;
+
+		values[0] = (ret >> 0) & 1;
+		values[1] = (ret >> 1) & 1;
+		values[2] = (ret >> 2) & 1;
+
+		mutex_lock(&indio_dev->mlock);
+		gpiod_set_array_value(ARRAY_SIZE(values), st->gpio_os->desc,
+				      values);
+		st->oversampling = val;
+		mutex_unlock(&indio_dev->mlock);
+
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static IIO_CONST_ATTR(oversampling_ratio_available, "1 2 4 8 16 32 64");
+
+static struct attribute *ad7606_attributes_os_and_range[] = {
+	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
+	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ad7606_attribute_group_os_and_range = {
+	.attrs = ad7606_attributes_os_and_range,
+};
+
+static struct attribute *ad7606_attributes_os[] = {
+	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ad7606_attribute_group_os = {
+	.attrs = ad7606_attributes_os,
+};
+
+static struct attribute *ad7606_attributes_range[] = {
+	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ad7606_attribute_group_range = {
+	.attrs = ad7606_attributes_range,
+};
+
+#define AD7606_CHANNEL(num)					\
+	{							\
+		.type = IIO_VOLTAGE,				\
+		.indexed = 1,					\
+		.channel = num,					\
+		.address = num,					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
+		.info_mask_shared_by_all =			\
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
+		.scan_index = num,				\
+		.scan_type = {					\
+			.sign = 's',				\
+			.realbits = 16,				\
+			.storagebits = 16,			\
+			.endianness = IIO_CPU,			\
+		},						\
+	}
+
+static const struct iio_chan_spec ad7606_channels[] = {
+	IIO_CHAN_SOFT_TIMESTAMP(8),
+	AD7606_CHANNEL(0),
+	AD7606_CHANNEL(1),
+	AD7606_CHANNEL(2),
+	AD7606_CHANNEL(3),
+	AD7606_CHANNEL(4),
+	AD7606_CHANNEL(5),
+	AD7606_CHANNEL(6),
+	AD7606_CHANNEL(7),
+};
+
+static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
+	/*
+	 * More devices added in future
+	 */
+	[ID_AD7606_8] = {
+		.channels = ad7606_channels,
+		.num_channels = 9,
+	},
+	[ID_AD7606_6] = {
+		.channels = ad7606_channels,
+		.num_channels = 7,
+	},
+	[ID_AD7606_4] = {
+		.channels = ad7606_channels,
+		.num_channels = 5,
+	},
+};
+
+static int ad7606_request_gpios(struct ad7606_state *st)
+{
+	struct device *dev = st->dev;
+
+	st->gpio_convst = devm_gpiod_get(dev, "conversion-start",
+					 GPIOD_OUT_LOW);
+	if (IS_ERR(st->gpio_convst))
+		return PTR_ERR(st->gpio_convst);
+
+	st->gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(st->gpio_reset))
+		return PTR_ERR(st->gpio_reset);
+
+	st->gpio_range = devm_gpiod_get_optional(dev, "range", GPIOD_OUT_LOW);
+	if (IS_ERR(st->gpio_range))
+		return PTR_ERR(st->gpio_range);
+
+	st->gpio_standby = devm_gpiod_get_optional(dev, "standby",
+						   GPIOD_OUT_HIGH);
+	if (IS_ERR(st->gpio_standby))
+		return PTR_ERR(st->gpio_standby);
+
+	st->gpio_frstdata = devm_gpiod_get_optional(dev, "first-data",
+						    GPIOD_IN);
+	if (IS_ERR(st->gpio_frstdata))
+		return PTR_ERR(st->gpio_frstdata);
+
+	st->gpio_os = devm_gpiod_get_array_optional(dev, "oversampling-ratio",
+			GPIOD_OUT_LOW);
+	return PTR_ERR_OR_ZERO(st->gpio_os);
+}
+
+/**
+ *  Interrupt handler
+ */
+static irqreturn_t ad7606_interrupt(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = dev_id;
+	struct ad7606_state *st = iio_priv(indio_dev);
+
+	if (iio_buffer_enabled(indio_dev)) {
+		schedule_work(&st->poll_work);
+	} else {
+		st->done = true;
+		wake_up_interruptible(&st->wq_data_avail);
+	}
+
+	return IRQ_HANDLED;
+};
+
+static const struct iio_info ad7606_info_no_os_or_range = {
+	.driver_module = THIS_MODULE,
+	.read_raw = &ad7606_read_raw,
+};
+
+static const struct iio_info ad7606_info_os_and_range = {
+	.driver_module = THIS_MODULE,
+	.read_raw = &ad7606_read_raw,
+	.write_raw = &ad7606_write_raw,
+	.attrs = &ad7606_attribute_group_os_and_range,
+};
+
+static const struct iio_info ad7606_info_os = {
+	.driver_module = THIS_MODULE,
+	.read_raw = &ad7606_read_raw,
+	.write_raw = &ad7606_write_raw,
+	.attrs = &ad7606_attribute_group_os,
+};
+
+static const struct iio_info ad7606_info_range = {
+	.driver_module = THIS_MODULE,
+	.read_raw = &ad7606_read_raw,
+	.attrs = &ad7606_attribute_group_range,
+};
+
+int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
+		 const char *name, unsigned int id,
+		 const struct ad7606_bus_ops *bops)
+{
+	struct ad7606_state *st;
+	int ret;
+	struct iio_dev *indio_dev;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	st->dev = dev;
+	st->bops = bops;
+	st->base_address = base_address;
+	st->range = 5000;
+	st->oversampling = 1;
+	INIT_WORK(&st->poll_work, &ad7606_poll_bh_to_ring);
+
+	st->reg = devm_regulator_get(dev, "avcc");
+	if (IS_ERR(st->reg))
+		return PTR_ERR(st->reg);
+
+	ret = regulator_enable(st->reg);
+	if (ret) {
+		dev_err(dev, "Failed to enable specified AVcc supply\n");
+		return ret;
+	}
+
+	ret = ad7606_request_gpios(st);
+	if (ret)
+		goto error_disable_reg;
+
+	st->chip_info = &ad7606_chip_info_tbl[id];
+
+	indio_dev->dev.parent = dev;
+	if (st->gpio_os) {
+		if (st->gpio_range)
+			indio_dev->info = &ad7606_info_os_and_range;
+		else
+			indio_dev->info = &ad7606_info_os;
+	} else {
+		if (st->gpio_range)
+			indio_dev->info = &ad7606_info_range;
+		else
+			indio_dev->info = &ad7606_info_no_os_or_range;
+	}
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->name = name;
+	indio_dev->channels = st->chip_info->channels;
+	indio_dev->num_channels = st->chip_info->num_channels;
+
+	init_waitqueue_head(&st->wq_data_avail);
+
+	ret = ad7606_reset(st);
+	if (ret)
+		dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n");
+
+	ret = request_irq(irq, ad7606_interrupt, IRQF_TRIGGER_FALLING, name,
+			  indio_dev);
+	if (ret)
+		goto error_disable_reg;
+
+	ret = iio_triggered_buffer_setup(indio_dev, &ad7606_trigger_handler,
+					 NULL, NULL);
+	if (ret)
+		goto error_free_irq;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto error_unregister_ring;
+
+	dev_set_drvdata(dev, indio_dev);
+
+	return 0;
+error_unregister_ring:
+	iio_triggered_buffer_cleanup(indio_dev);
+
+error_free_irq:
+	free_irq(irq, indio_dev);
+
+error_disable_reg:
+	regulator_disable(st->reg);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ad7606_probe);
+
+int ad7606_remove(struct device *dev, int irq)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad7606_state *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	free_irq(irq, indio_dev);
+	regulator_disable(st->reg);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ad7606_remove);
+
+#ifdef CONFIG_PM_SLEEP
+
+static int ad7606_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad7606_state *st = iio_priv(indio_dev);
+
+	if (st->gpio_standby) {
+		gpiod_set_value(st->gpio_range, 1);
+		gpiod_set_value(st->gpio_standby, 0);
+	}
+
+	return 0;
+}
+
+static int ad7606_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad7606_state *st = iio_priv(indio_dev);
+
+	if (st->gpio_standby) {
+		gpiod_set_value(st->gpio_range, st->range == 10000);
+		gpiod_set_value(st->gpio_standby, 1);
+		ad7606_reset(st);
+	}
+
+	return 0;
+}
+
+SIMPLE_DEV_PM_OPS(ad7606_pm_ops, ad7606_suspend, ad7606_resume);
+EXPORT_SYMBOL_GPL(ad7606_pm_ops);
+
+#endif
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
new file mode 100644
index 0000000..746f955
--- /dev/null
+++ b/drivers/iio/adc/ad7606.h
@@ -0,0 +1,78 @@
+/*
+ * AD7606 ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ */
+
+#ifndef IIO_ADC_AD7606_H_
+#define IIO_ADC_AD7606_H_
+
+/**
+ * struct ad7606_chip_info - chip specific information
+ * @name:		identification string for chip
+ * @channels:		channel specification
+ * @num_channels:	number of channels
+ */
+
+struct ad7606_chip_info {
+	const struct iio_chan_spec	*channels;
+	unsigned int			num_channels;
+};
+
+/**
+ * struct ad7606_state - driver instance specific data
+ */
+
+struct ad7606_state {
+	struct device			*dev;
+	const struct ad7606_chip_info	*chip_info;
+	struct regulator		*reg;
+	struct work_struct		poll_work;
+	wait_queue_head_t		wq_data_avail;
+	const struct ad7606_bus_ops	*bops;
+	unsigned int			range;
+	unsigned int			oversampling;
+	bool				done;
+	void __iomem			*base_address;
+
+	struct gpio_desc		*gpio_convst;
+	struct gpio_desc		*gpio_reset;
+	struct gpio_desc		*gpio_range;
+	struct gpio_desc		*gpio_standby;
+	struct gpio_desc		*gpio_frstdata;
+	struct gpio_descs		*gpio_os;
+
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 * 8 * 16-bit samples + 64-bit timestamp
+	 */
+	unsigned short			data[12] ____cacheline_aligned;
+};
+
+struct ad7606_bus_ops {
+	/* more methods added in future? */
+	int (*read_block)(struct device *, int, void *);
+};
+
+int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
+		 const char *name, unsigned int id,
+		 const struct ad7606_bus_ops *bops);
+int ad7606_remove(struct device *dev, int irq);
+
+enum ad7606_supported_device_ids {
+	ID_AD7606_8,
+	ID_AD7606_6,
+	ID_AD7606_4
+};
+
+#ifdef CONFIG_PM_SLEEP
+extern const struct dev_pm_ops ad7606_pm_ops;
+#define AD7606_PM_OPS (&ad7606_pm_ops)
+#else
+#define AD7606_PM_OPS NULL
+#endif
+
+#endif /* IIO_ADC_AD7606_H_ */
diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
new file mode 100644
index 0000000..cd6c410c
--- /dev/null
+++ b/drivers/iio/adc/ad7606_par.c
@@ -0,0 +1,112 @@
+/*
+ * AD7606 Parallel Interface ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/err.h>
+#include <linux/io.h>
+
+#include <linux/iio/iio.h>
+#include "ad7606.h"
+
+static int ad7606_par16_read_block(struct device *dev,
+				   int count, void *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct ad7606_state *st = iio_priv(indio_dev);
+
+	insw((unsigned long)st->base_address, buf, count);
+
+	return 0;
+}
+
+static const struct ad7606_bus_ops ad7606_par16_bops = {
+	.read_block	= ad7606_par16_read_block,
+};
+
+static int ad7606_par8_read_block(struct device *dev,
+				  int count, void *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct ad7606_state *st = iio_priv(indio_dev);
+
+	insb((unsigned long)st->base_address, buf, count * 2);
+
+	return 0;
+}
+
+static const struct ad7606_bus_ops ad7606_par8_bops = {
+	.read_block	= ad7606_par8_read_block,
+};
+
+static int ad7606_par_probe(struct platform_device *pdev)
+{
+	const struct platform_device_id *id = platform_get_device_id(pdev);
+	struct resource *res;
+	void __iomem *addr;
+	resource_size_t remap_size;
+	int irq;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "no irq\n");
+		return -ENODEV;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	addr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(addr))
+		return PTR_ERR(addr);
+
+	remap_size = resource_size(res);
+
+	return ad7606_probe(&pdev->dev, irq, addr,
+			    id->name, id->driver_data,
+			    remap_size > 1 ? &ad7606_par16_bops :
+			    &ad7606_par8_bops);
+}
+
+static int ad7606_par_remove(struct platform_device *pdev)
+{
+	return ad7606_remove(&pdev->dev, platform_get_irq(pdev, 0));
+}
+
+static const struct platform_device_id ad7606_driver_ids[] = {
+	{
+		.name		= "ad7606-8",
+		.driver_data	= ID_AD7606_8,
+	}, {
+		.name		= "ad7606-6",
+		.driver_data	= ID_AD7606_6,
+	}, {
+		.name		= "ad7606-4",
+		.driver_data	= ID_AD7606_4,
+	},
+	{ }
+};
+
+MODULE_DEVICE_TABLE(platform, ad7606_driver_ids);
+
+static struct platform_driver ad7606_driver = {
+	.probe = ad7606_par_probe,
+	.remove	= ad7606_par_remove,
+	.id_table = ad7606_driver_ids,
+	.driver = {
+		.name	 = "ad7606",
+		.pm	 = AD7606_PM_OPS,
+	},
+};
+
+module_platform_driver(ad7606_driver);
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
new file mode 100644
index 0000000..c9b1f266
--- /dev/null
+++ b/drivers/iio/adc/ad7606_spi.c
@@ -0,0 +1,78 @@
+/*
+ * AD7606 SPI ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+#include <linux/err.h>
+
+#include <linux/iio/iio.h>
+#include "ad7606.h"
+
+#define MAX_SPI_FREQ_HZ		23500000	/* VDRIVE above 4.75 V */
+
+static int ad7606_spi_read_block(struct device *dev,
+				 int count, void *buf)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	int i, ret;
+	unsigned short *data = buf;
+	__be16 *bdata = buf;
+
+	ret = spi_read(spi, buf, count * 2);
+	if (ret < 0) {
+		dev_err(&spi->dev, "SPI read error\n");
+		return ret;
+	}
+
+	for (i = 0; i < count; i++)
+		data[i] = be16_to_cpu(bdata[i]);
+
+	return 0;
+}
+
+static const struct ad7606_bus_ops ad7606_spi_bops = {
+	.read_block	= ad7606_spi_read_block,
+};
+
+static int ad7606_spi_probe(struct spi_device *spi)
+{
+	const struct spi_device_id *id = spi_get_device_id(spi);
+
+	return ad7606_probe(&spi->dev, spi->irq, NULL,
+			    id->name, id->driver_data,
+			    &ad7606_spi_bops);
+}
+
+static int ad7606_spi_remove(struct spi_device *spi)
+{
+	return ad7606_remove(&spi->dev, spi->irq);
+}
+
+static const struct spi_device_id ad7606_id[] = {
+	{"ad7606-8", ID_AD7606_8},
+	{"ad7606-6", ID_AD7606_6},
+	{"ad7606-4", ID_AD7606_4},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ad7606_id);
+
+static struct spi_driver ad7606_driver = {
+	.driver = {
+		.name = "ad7606",
+		.pm = AD7606_PM_OPS,
+	},
+	.probe = ad7606_spi_probe,
+	.remove = ad7606_spi_remove,
+	.id_table = ad7606_id,
+};
+module_spi_driver(ad7606_driver);
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
index deff899..995867b 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -3,40 +3,6 @@
 #
 menu "Analog to digital converters"
 
-config AD7606
-	tristate "Analog Devices AD7606 ADC driver"
-	depends on GPIOLIB || COMPILE_TEST
-	depends on HAS_IOMEM
-	select IIO_BUFFER
-	select IIO_TRIGGERED_BUFFER
-	help
-	  Say yes here to build support for Analog Devices:
-	  ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC).
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called ad7606.
-
-config AD7606_IFACE_PARALLEL
-	tristate "parallel interface support"
-	depends on AD7606
-	help
-	  Say yes here to include parallel interface support on the AD7606
-	  ADC driver.
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called ad7606_parallel.
-
-config AD7606_IFACE_SPI
-	tristate "spi interface support"
-	depends on AD7606
-	depends on SPI
-	help
-	  Say yes here to include parallel interface support on the AD7606
-	  ADC driver.
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called ad7606_spi.
-
 config AD7780
 	tristate "Analog Devices AD7780 and similar ADCs driver"
 	depends on SPI
diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
index ac09485..549032b 100644
--- a/drivers/staging/iio/adc/Makefile
+++ b/drivers/staging/iio/adc/Makefile
@@ -2,10 +2,6 @@
 # Makefile for industrial I/O ADC drivers
 #
 
-obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
-obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
-obj-$(CONFIG_AD7606) += ad7606.o
-
 obj-$(CONFIG_AD7780) += ad7780.o
 obj-$(CONFIG_AD7816) += ad7816.o
 obj-$(CONFIG_AD7192) += ad7192.o
diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c
deleted file mode 100644
index cceb18c..0000000
--- a/drivers/staging/iio/adc/ad7606.c
+++ /dev/null
@@ -1,519 +0,0 @@
-/*
- * AD7606 SPI ADC driver
- *
- * Copyright 2011 Analog Devices Inc.
- *
- * Licensed under the GPL-2.
- */
-
-#include <linux/interrupt.h>
-#include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/sysfs.h>
-#include <linux/regulator/consumer.h>
-#include <linux/err.h>
-#include <linux/gpio/consumer.h>
-#include <linux/delay.h>
-#include <linux/sched.h>
-#include <linux/module.h>
-
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-#include <linux/iio/buffer.h>
-#include <linux/iio/trigger_consumer.h>
-#include <linux/iio/triggered_buffer.h>
-
-#include "ad7606.h"
-
-static int ad7606_reset(struct ad7606_state *st)
-{
-	if (st->gpio_reset) {
-		gpiod_set_value(st->gpio_reset, 1);
-		ndelay(100); /* t_reset >= 100ns */
-		gpiod_set_value(st->gpio_reset, 0);
-		return 0;
-	}
-
-	return -ENODEV;
-}
-
-static int ad7606_read_samples(struct ad7606_state *st)
-{
-	unsigned int num = st->chip_info->num_channels;
-	u16 *data = st->data;
-	int ret;
-
-	/*
-	 * The frstdata signal is set to high while and after reading the sample
-	 * of the first channel and low for all other channels. This can be used
-	 * to check that the incoming data is correctly aligned. During normal
-	 * operation the data should never become unaligned, but some glitch or
-	 * electrostatic discharge might cause an extra read or clock cycle.
-	 * Monitoring the frstdata signal allows to recover from such failure
-	 * situations.
-	 */
-
-	if (st->gpio_frstdata) {
-		ret = st->bops->read_block(st->dev, 1, data);
-		if (ret)
-			return ret;
-
-		if (!gpiod_get_value(st->gpio_frstdata)) {
-			ad7606_reset(st);
-			return -EIO;
-		}
-
-		data++;
-		num--;
-	}
-
-	return st->bops->read_block(st->dev, num, data);
-}
-
-static irqreturn_t ad7606_trigger_handler(int irq, void *p)
-{
-	struct iio_poll_func *pf = p;
-	struct ad7606_state *st = iio_priv(pf->indio_dev);
-
-	gpiod_set_value(st->gpio_convst, 1);
-
-	return IRQ_HANDLED;
-}
-
-/**
- * ad7606_poll_bh_to_ring() bh of trigger launched polling to ring buffer
- * @work_s:	the work struct through which this was scheduled
- *
- * Currently there is no option in this driver to disable the saving of
- * timestamps within the ring.
- * I think the one copy of this at a time was to avoid problems if the
- * trigger was set far too high and the reads then locked up the computer.
- **/
-static void ad7606_poll_bh_to_ring(struct work_struct *work_s)
-{
-	struct ad7606_state *st = container_of(work_s, struct ad7606_state,
-						poll_work);
-	struct iio_dev *indio_dev = iio_priv_to_dev(st);
-	int ret;
-
-	ret = ad7606_read_samples(st);
-	if (ret == 0)
-		iio_push_to_buffers_with_timestamp(indio_dev, st->data,
-						   iio_get_time_ns(indio_dev));
-
-	gpiod_set_value(st->gpio_convst, 0);
-	iio_trigger_notify_done(indio_dev->trig);
-}
-
-static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
-{
-	struct ad7606_state *st = iio_priv(indio_dev);
-	int ret;
-
-	st->done = false;
-	gpiod_set_value(st->gpio_convst, 1);
-
-	ret = wait_event_interruptible(st->wq_data_avail, st->done);
-	if (ret)
-		goto error_ret;
-
-	ret = ad7606_read_samples(st);
-	if (ret == 0)
-		ret = st->data[ch];
-
-error_ret:
-	gpiod_set_value(st->gpio_convst, 0);
-
-	return ret;
-}
-
-static int ad7606_read_raw(struct iio_dev *indio_dev,
-			   struct iio_chan_spec const *chan,
-			   int *val,
-			   int *val2,
-			   long m)
-{
-	int ret;
-	struct ad7606_state *st = iio_priv(indio_dev);
-
-	switch (m) {
-	case IIO_CHAN_INFO_RAW:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
-
-		ret = ad7606_scan_direct(indio_dev, chan->address);
-		iio_device_release_direct_mode(indio_dev);
-
-		if (ret < 0)
-			return ret;
-		*val = (short)ret;
-		return IIO_VAL_INT;
-	case IIO_CHAN_INFO_SCALE:
-		*val = st->range * 2;
-		*val2 = st->chip_info->channels[0].scan_type.realbits;
-		return IIO_VAL_FRACTIONAL_LOG2;
-	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		*val = st->oversampling;
-		return IIO_VAL_INT;
-	}
-	return -EINVAL;
-}
-
-static IIO_CONST_ATTR(in_voltage_scale_available, "5000 10000");
-
-static int ad7606_oversampling_get_index(unsigned int val)
-{
-	unsigned char supported[] = {1, 2, 4, 8, 16, 32, 64};
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(supported); i++)
-		if (val == supported[i])
-			return i;
-
-	return -EINVAL;
-}
-
-static int ad7606_write_raw(struct iio_dev *indio_dev,
-			    struct iio_chan_spec const *chan,
-			    int val,
-			    int val2,
-			    long mask)
-{
-	struct ad7606_state *st = iio_priv(indio_dev);
-	int values[3];
-	int ret;
-
-	switch (mask) {
-	case IIO_CHAN_INFO_SCALE:
-		if (val2)
-			return -EINVAL;
-
-		if (!(val == 5000 || val == 10000))
-			return -EINVAL;
-
-		mutex_lock(&indio_dev->mlock);
-		gpiod_set_value(st->gpio_range, val == 10000);
-		st->range = val;
-		mutex_unlock(&indio_dev->mlock);
-
-		return 0;
-	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		if (val2)
-			return -EINVAL;
-		ret = ad7606_oversampling_get_index(val);
-		if (ret < 0)
-			return ret;
-
-		values[0] = (ret >> 0) & 1;
-		values[1] = (ret >> 1) & 1;
-		values[2] = (ret >> 2) & 1;
-
-		mutex_lock(&indio_dev->mlock);
-		gpiod_set_array_value(ARRAY_SIZE(values), st->gpio_os->desc,
-				      values);
-		st->oversampling = val;
-		mutex_unlock(&indio_dev->mlock);
-
-		return 0;
-	default:
-		return -EINVAL;
-	}
-}
-
-static IIO_CONST_ATTR(oversampling_ratio_available, "1 2 4 8 16 32 64");
-
-static struct attribute *ad7606_attributes_os_and_range[] = {
-	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
-	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
-	NULL,
-};
-
-static const struct attribute_group ad7606_attribute_group_os_and_range = {
-	.attrs = ad7606_attributes_os_and_range,
-};
-
-static struct attribute *ad7606_attributes_os[] = {
-	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
-	NULL,
-};
-
-static const struct attribute_group ad7606_attribute_group_os = {
-	.attrs = ad7606_attributes_os,
-};
-
-static struct attribute *ad7606_attributes_range[] = {
-	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
-	NULL,
-};
-
-static const struct attribute_group ad7606_attribute_group_range = {
-	.attrs = ad7606_attributes_range,
-};
-
-#define AD7606_CHANNEL(num)					\
-	{							\
-		.type = IIO_VOLTAGE,				\
-		.indexed = 1,					\
-		.channel = num,					\
-		.address = num,					\
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
-		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
-		.info_mask_shared_by_all =			\
-			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
-		.scan_index = num,				\
-		.scan_type = {					\
-			.sign = 's',				\
-			.realbits = 16,				\
-			.storagebits = 16,			\
-			.endianness = IIO_CPU,			\
-		},						\
-	}
-
-static const struct iio_chan_spec ad7606_channels[] = {
-	IIO_CHAN_SOFT_TIMESTAMP(8),
-	AD7606_CHANNEL(0),
-	AD7606_CHANNEL(1),
-	AD7606_CHANNEL(2),
-	AD7606_CHANNEL(3),
-	AD7606_CHANNEL(4),
-	AD7606_CHANNEL(5),
-	AD7606_CHANNEL(6),
-	AD7606_CHANNEL(7),
-};
-
-static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
-	/*
-	 * More devices added in future
-	 */
-	[ID_AD7606_8] = {
-		.channels = ad7606_channels,
-		.num_channels = 9,
-	},
-	[ID_AD7606_6] = {
-		.channels = ad7606_channels,
-		.num_channels = 7,
-	},
-	[ID_AD7606_4] = {
-		.channels = ad7606_channels,
-		.num_channels = 5,
-	},
-};
-
-static int ad7606_request_gpios(struct ad7606_state *st)
-{
-	struct device *dev = st->dev;
-
-	st->gpio_convst = devm_gpiod_get(dev, "conversion-start",
-					 GPIOD_OUT_LOW);
-	if (IS_ERR(st->gpio_convst))
-		return PTR_ERR(st->gpio_convst);
-
-	st->gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(st->gpio_reset))
-		return PTR_ERR(st->gpio_reset);
-
-	st->gpio_range = devm_gpiod_get_optional(dev, "range", GPIOD_OUT_LOW);
-	if (IS_ERR(st->gpio_range))
-		return PTR_ERR(st->gpio_range);
-
-	st->gpio_standby = devm_gpiod_get_optional(dev, "standby",
-						   GPIOD_OUT_HIGH);
-	if (IS_ERR(st->gpio_standby))
-		return PTR_ERR(st->gpio_standby);
-
-	st->gpio_frstdata = devm_gpiod_get_optional(dev, "first-data",
-						    GPIOD_IN);
-	if (IS_ERR(st->gpio_frstdata))
-		return PTR_ERR(st->gpio_frstdata);
-
-	st->gpio_os = devm_gpiod_get_array_optional(dev, "oversampling-ratio",
-			GPIOD_OUT_LOW);
-	return PTR_ERR_OR_ZERO(st->gpio_os);
-}
-
-/**
- *  Interrupt handler
- */
-static irqreturn_t ad7606_interrupt(int irq, void *dev_id)
-{
-	struct iio_dev *indio_dev = dev_id;
-	struct ad7606_state *st = iio_priv(indio_dev);
-
-	if (iio_buffer_enabled(indio_dev)) {
-		schedule_work(&st->poll_work);
-	} else {
-		st->done = true;
-		wake_up_interruptible(&st->wq_data_avail);
-	}
-
-	return IRQ_HANDLED;
-};
-
-static const struct iio_info ad7606_info_no_os_or_range = {
-	.driver_module = THIS_MODULE,
-	.read_raw = &ad7606_read_raw,
-};
-
-static const struct iio_info ad7606_info_os_and_range = {
-	.driver_module = THIS_MODULE,
-	.read_raw = &ad7606_read_raw,
-	.write_raw = &ad7606_write_raw,
-	.attrs = &ad7606_attribute_group_os_and_range,
-};
-
-static const struct iio_info ad7606_info_os = {
-	.driver_module = THIS_MODULE,
-	.read_raw = &ad7606_read_raw,
-	.write_raw = &ad7606_write_raw,
-	.attrs = &ad7606_attribute_group_os,
-};
-
-static const struct iio_info ad7606_info_range = {
-	.driver_module = THIS_MODULE,
-	.read_raw = &ad7606_read_raw,
-	.attrs = &ad7606_attribute_group_range,
-};
-
-int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
-		 const char *name, unsigned int id,
-		 const struct ad7606_bus_ops *bops)
-{
-	struct ad7606_state *st;
-	int ret;
-	struct iio_dev *indio_dev;
-
-	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
-	if (!indio_dev)
-		return -ENOMEM;
-
-	st = iio_priv(indio_dev);
-
-	st->dev = dev;
-	st->bops = bops;
-	st->base_address = base_address;
-	st->range = 5000;
-	st->oversampling = 1;
-	INIT_WORK(&st->poll_work, &ad7606_poll_bh_to_ring);
-
-	st->reg = devm_regulator_get(dev, "avcc");
-	if (IS_ERR(st->reg))
-		return PTR_ERR(st->reg);
-
-	ret = regulator_enable(st->reg);
-	if (ret) {
-		dev_err(dev, "Failed to enable specified AVcc supply\n");
-		return ret;
-	}
-
-	ret = ad7606_request_gpios(st);
-	if (ret)
-		goto error_disable_reg;
-
-	st->chip_info = &ad7606_chip_info_tbl[id];
-
-	indio_dev->dev.parent = dev;
-	if (st->gpio_os) {
-		if (st->gpio_range)
-			indio_dev->info = &ad7606_info_os_and_range;
-		else
-			indio_dev->info = &ad7606_info_os;
-	} else {
-		if (st->gpio_range)
-			indio_dev->info = &ad7606_info_range;
-		else
-			indio_dev->info = &ad7606_info_no_os_or_range;
-	}
-	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->name = name;
-	indio_dev->channels = st->chip_info->channels;
-	indio_dev->num_channels = st->chip_info->num_channels;
-
-	init_waitqueue_head(&st->wq_data_avail);
-
-	ret = ad7606_reset(st);
-	if (ret)
-		dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n");
-
-	ret = request_irq(irq, ad7606_interrupt, IRQF_TRIGGER_FALLING, name,
-			  indio_dev);
-	if (ret)
-		goto error_disable_reg;
-
-	ret = iio_triggered_buffer_setup(indio_dev, &ad7606_trigger_handler,
-					 NULL, NULL);
-	if (ret)
-		goto error_free_irq;
-
-	ret = iio_device_register(indio_dev);
-	if (ret)
-		goto error_unregister_ring;
-
-	dev_set_drvdata(dev, indio_dev);
-
-	return 0;
-error_unregister_ring:
-	iio_triggered_buffer_cleanup(indio_dev);
-
-error_free_irq:
-	free_irq(irq, indio_dev);
-
-error_disable_reg:
-	regulator_disable(st->reg);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(ad7606_probe);
-
-int ad7606_remove(struct device *dev, int irq)
-{
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
-	struct ad7606_state *st = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-	iio_triggered_buffer_cleanup(indio_dev);
-
-	free_irq(irq, indio_dev);
-	regulator_disable(st->reg);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(ad7606_remove);
-
-#ifdef CONFIG_PM_SLEEP
-
-static int ad7606_suspend(struct device *dev)
-{
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
-	struct ad7606_state *st = iio_priv(indio_dev);
-
-	if (st->gpio_standby) {
-		gpiod_set_value(st->gpio_range, 1);
-		gpiod_set_value(st->gpio_standby, 0);
-	}
-
-	return 0;
-}
-
-static int ad7606_resume(struct device *dev)
-{
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
-	struct ad7606_state *st = iio_priv(indio_dev);
-
-	if (st->gpio_standby) {
-		gpiod_set_value(st->gpio_range, st->range == 10000);
-		gpiod_set_value(st->gpio_standby, 1);
-		ad7606_reset(st);
-	}
-
-	return 0;
-}
-
-SIMPLE_DEV_PM_OPS(ad7606_pm_ops, ad7606_suspend, ad7606_resume);
-EXPORT_SYMBOL_GPL(ad7606_pm_ops);
-
-#endif
-
-MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
-MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
-MODULE_LICENSE("GPL v2");
diff --git a/drivers/staging/iio/adc/ad7606.h b/drivers/staging/iio/adc/ad7606.h
deleted file mode 100644
index 746f955..0000000
--- a/drivers/staging/iio/adc/ad7606.h
+++ /dev/null
@@ -1,78 +0,0 @@
-/*
- * AD7606 ADC driver
- *
- * Copyright 2011 Analog Devices Inc.
- *
- * Licensed under the GPL-2.
- */
-
-#ifndef IIO_ADC_AD7606_H_
-#define IIO_ADC_AD7606_H_
-
-/**
- * struct ad7606_chip_info - chip specific information
- * @name:		identification string for chip
- * @channels:		channel specification
- * @num_channels:	number of channels
- */
-
-struct ad7606_chip_info {
-	const struct iio_chan_spec	*channels;
-	unsigned int			num_channels;
-};
-
-/**
- * struct ad7606_state - driver instance specific data
- */
-
-struct ad7606_state {
-	struct device			*dev;
-	const struct ad7606_chip_info	*chip_info;
-	struct regulator		*reg;
-	struct work_struct		poll_work;
-	wait_queue_head_t		wq_data_avail;
-	const struct ad7606_bus_ops	*bops;
-	unsigned int			range;
-	unsigned int			oversampling;
-	bool				done;
-	void __iomem			*base_address;
-
-	struct gpio_desc		*gpio_convst;
-	struct gpio_desc		*gpio_reset;
-	struct gpio_desc		*gpio_range;
-	struct gpio_desc		*gpio_standby;
-	struct gpio_desc		*gpio_frstdata;
-	struct gpio_descs		*gpio_os;
-
-	/*
-	 * DMA (thus cache coherency maintenance) requires the
-	 * transfer buffers to live in their own cache lines.
-	 * 8 * 16-bit samples + 64-bit timestamp
-	 */
-	unsigned short			data[12] ____cacheline_aligned;
-};
-
-struct ad7606_bus_ops {
-	/* more methods added in future? */
-	int (*read_block)(struct device *, int, void *);
-};
-
-int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
-		 const char *name, unsigned int id,
-		 const struct ad7606_bus_ops *bops);
-int ad7606_remove(struct device *dev, int irq);
-
-enum ad7606_supported_device_ids {
-	ID_AD7606_8,
-	ID_AD7606_6,
-	ID_AD7606_4
-};
-
-#ifdef CONFIG_PM_SLEEP
-extern const struct dev_pm_ops ad7606_pm_ops;
-#define AD7606_PM_OPS (&ad7606_pm_ops)
-#else
-#define AD7606_PM_OPS NULL
-#endif
-
-#endif /* IIO_ADC_AD7606_H_ */
diff --git a/drivers/staging/iio/adc/ad7606_par.c b/drivers/staging/iio/adc/ad7606_par.c
deleted file mode 100644
index cd6c410c..0000000
--- a/drivers/staging/iio/adc/ad7606_par.c
+++ /dev/null
@@ -1,112 +0,0 @@
-/*
- * AD7606 Parallel Interface ADC driver
- *
- * Copyright 2011 Analog Devices Inc.
- *
- * Licensed under the GPL-2.
- */
-
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/types.h>
-#include <linux/err.h>
-#include <linux/io.h>
-
-#include <linux/iio/iio.h>
-#include "ad7606.h"
-
-static int ad7606_par16_read_block(struct device *dev,
-				   int count, void *buf)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
-	struct ad7606_state *st = iio_priv(indio_dev);
-
-	insw((unsigned long)st->base_address, buf, count);
-
-	return 0;
-}
-
-static const struct ad7606_bus_ops ad7606_par16_bops = {
-	.read_block	= ad7606_par16_read_block,
-};
-
-static int ad7606_par8_read_block(struct device *dev,
-				  int count, void *buf)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
-	struct ad7606_state *st = iio_priv(indio_dev);
-
-	insb((unsigned long)st->base_address, buf, count * 2);
-
-	return 0;
-}
-
-static const struct ad7606_bus_ops ad7606_par8_bops = {
-	.read_block	= ad7606_par8_read_block,
-};
-
-static int ad7606_par_probe(struct platform_device *pdev)
-{
-	const struct platform_device_id *id = platform_get_device_id(pdev);
-	struct resource *res;
-	void __iomem *addr;
-	resource_size_t remap_size;
-	int irq;
-
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		dev_err(&pdev->dev, "no irq\n");
-		return -ENODEV;
-	}
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	addr = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(addr))
-		return PTR_ERR(addr);
-
-	remap_size = resource_size(res);
-
-	return ad7606_probe(&pdev->dev, irq, addr,
-			    id->name, id->driver_data,
-			    remap_size > 1 ? &ad7606_par16_bops :
-			    &ad7606_par8_bops);
-}
-
-static int ad7606_par_remove(struct platform_device *pdev)
-{
-	return ad7606_remove(&pdev->dev, platform_get_irq(pdev, 0));
-}
-
-static const struct platform_device_id ad7606_driver_ids[] = {
-	{
-		.name		= "ad7606-8",
-		.driver_data	= ID_AD7606_8,
-	}, {
-		.name		= "ad7606-6",
-		.driver_data	= ID_AD7606_6,
-	}, {
-		.name		= "ad7606-4",
-		.driver_data	= ID_AD7606_4,
-	},
-	{ }
-};
-
-MODULE_DEVICE_TABLE(platform, ad7606_driver_ids);
-
-static struct platform_driver ad7606_driver = {
-	.probe = ad7606_par_probe,
-	.remove	= ad7606_par_remove,
-	.id_table = ad7606_driver_ids,
-	.driver = {
-		.name	 = "ad7606",
-		.pm	 = AD7606_PM_OPS,
-	},
-};
-
-module_platform_driver(ad7606_driver);
-
-MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
-MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
-MODULE_LICENSE("GPL v2");
diff --git a/drivers/staging/iio/adc/ad7606_spi.c b/drivers/staging/iio/adc/ad7606_spi.c
deleted file mode 100644
index c9b1f266..0000000
--- a/drivers/staging/iio/adc/ad7606_spi.c
+++ /dev/null
@@ -1,78 +0,0 @@
-/*
- * AD7606 SPI ADC driver
- *
- * Copyright 2011 Analog Devices Inc.
- *
- * Licensed under the GPL-2.
- */
-
-#include <linux/module.h>
-#include <linux/spi/spi.h>
-#include <linux/types.h>
-#include <linux/err.h>
-
-#include <linux/iio/iio.h>
-#include "ad7606.h"
-
-#define MAX_SPI_FREQ_HZ		23500000	/* VDRIVE above 4.75 V */
-
-static int ad7606_spi_read_block(struct device *dev,
-				 int count, void *buf)
-{
-	struct spi_device *spi = to_spi_device(dev);
-	int i, ret;
-	unsigned short *data = buf;
-	__be16 *bdata = buf;
-
-	ret = spi_read(spi, buf, count * 2);
-	if (ret < 0) {
-		dev_err(&spi->dev, "SPI read error\n");
-		return ret;
-	}
-
-	for (i = 0; i < count; i++)
-		data[i] = be16_to_cpu(bdata[i]);
-
-	return 0;
-}
-
-static const struct ad7606_bus_ops ad7606_spi_bops = {
-	.read_block	= ad7606_spi_read_block,
-};
-
-static int ad7606_spi_probe(struct spi_device *spi)
-{
-	const struct spi_device_id *id = spi_get_device_id(spi);
-
-	return ad7606_probe(&spi->dev, spi->irq, NULL,
-			    id->name, id->driver_data,
-			    &ad7606_spi_bops);
-}
-
-static int ad7606_spi_remove(struct spi_device *spi)
-{
-	return ad7606_remove(&spi->dev, spi->irq);
-}
-
-static const struct spi_device_id ad7606_id[] = {
-	{"ad7606-8", ID_AD7606_8},
-	{"ad7606-6", ID_AD7606_6},
-	{"ad7606-4", ID_AD7606_4},
-	{}
-};
-MODULE_DEVICE_TABLE(spi, ad7606_id);
-
-static struct spi_driver ad7606_driver = {
-	.driver = {
-		.name = "ad7606",
-		.pm = AD7606_PM_OPS,
-	},
-	.probe = ad7606_spi_probe,
-	.remove = ad7606_spi_remove,
-	.id_table = ad7606_id,
-};
-module_spi_driver(ad7606_driver);
-
-MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
-MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
-MODULE_LICENSE("GPL v2");
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-11  6:34 ` [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale Eva Rachel Retuya
@ 2016-11-11 14:18   ` Lars-Peter Clausen
  2016-11-12 14:22     ` Eva Rachel Retuya
  2016-11-12 14:24     ` Jonathan Cameron
  0 siblings, 2 replies; 19+ messages in thread
From: Lars-Peter Clausen @ 2016-11-11 14:18 UTC (permalink / raw)
  To: Eva Rachel Retuya, linux-iio, devel, linux-kernel
  Cc: Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh

On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
> Eliminate the non-standard attribute in_voltage_range and move its
> functionality under the scale attribute. read_raw() has been taken care
> of previously so only write_raw() is handled here.
> 
> Additionally, rename the attribute in_voltage_range_available into
> in_voltage_scale_available.
> 
> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>

Hi,

Thanks for the patch. Unfortunately this is not quite this straight forward.

The scale is what you multiply the raw with to get the value in the standard
IIO unit. Range as implemented in this driver is the maximum output voltage.

To get the scale we need to look at the transfer function of the ADC [1].
The transfer function tells us that 1 LSB is 305uV for the 10V range and
152uV for the 5V range.

More specifically this is $RANGE*2/2**16 (times two since the ADC is bipolar).

Since the default unit for IIO is mV for voltages we need to multiply this
by 1000.

The other thing we need to handle is the case where the RANGE pin is not
connected to a GPIO but either hardwired to 1 or 0. Which we need to handle
somehow.

[1]
http://www.analog.com/media/en/technical-documentation/data-sheets/AD7606_7606-6_7606-4.pdf#page=23
(right bottom)

> ---
>  drivers/staging/iio/adc/ad7606.c | 56 ++++++++++++----------------------------
>  1 file changed, 16 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c
> index 4531908..cceb18c 100644
> --- a/drivers/staging/iio/adc/ad7606.c
> +++ b/drivers/staging/iio/adc/ad7606.c
> @@ -161,42 +161,7 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> -static ssize_t ad7606_show_range(struct device *dev,
> -				 struct device_attribute *attr, char *buf)
> -{
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct ad7606_state *st = iio_priv(indio_dev);
> -
> -	return sprintf(buf, "%u\n", st->range);
> -}
> -
> -static ssize_t ad7606_store_range(struct device *dev,
> -				  struct device_attribute *attr,
> -				  const char *buf, size_t count)
> -{
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct ad7606_state *st = iio_priv(indio_dev);
> -	unsigned long lval;
> -	int ret;
> -
> -	ret = kstrtoul(buf, 10, &lval);
> -	if (ret)
> -		return ret;
> -
> -	if (!(lval == 5000 || lval == 10000))
> -		return -EINVAL;
> -
> -	mutex_lock(&indio_dev->mlock);
> -	gpiod_set_value(st->gpio_range, lval == 10000);
> -	st->range = lval;
> -	mutex_unlock(&indio_dev->mlock);
> -
> -	return count;
> -}
> -
> -static IIO_DEVICE_ATTR(in_voltage_range, S_IRUGO | S_IWUSR,
> -		       ad7606_show_range, ad7606_store_range, 0);
> -static IIO_CONST_ATTR(in_voltage_range_available, "5000 10000");
> +static IIO_CONST_ATTR(in_voltage_scale_available, "5000 10000");
>  
>  static int ad7606_oversampling_get_index(unsigned int val)
>  {
> @@ -221,6 +186,19 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  	int ret;
>  
>  	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val2)
> +			return -EINVAL;
> +
> +		if (!(val == 5000 || val == 10000))
> +			return -EINVAL;
> +
> +		mutex_lock(&indio_dev->mlock);
> +		gpiod_set_value(st->gpio_range, val == 10000);
> +		st->range = val;
> +		mutex_unlock(&indio_dev->mlock);
> +
> +		return 0;
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		if (val2)
>  			return -EINVAL;
> @@ -247,8 +225,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  static IIO_CONST_ATTR(oversampling_ratio_available, "1 2 4 8 16 32 64");
>  
>  static struct attribute *ad7606_attributes_os_and_range[] = {
> -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
> -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
> +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
>  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
>  	NULL,
>  };
> @@ -267,8 +244,7 @@ static const struct attribute_group ad7606_attribute_group_os = {
>  };
>  
>  static struct attribute *ad7606_attributes_range[] = {
> -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
> -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
> +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
>  	NULL,
>  };
>  
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] staging: iio: ad7606: move out of staging
  2016-11-11  6:34 ` [PATCH 2/2] staging: iio: ad7606: move out of staging Eva Rachel Retuya
@ 2016-11-11 14:22   ` Lars-Peter Clausen
  2016-11-12 14:26     ` Jonathan Cameron
  2016-11-11 16:10   ` kbuild test robot
  1 sibling, 1 reply; 19+ messages in thread
From: Lars-Peter Clausen @ 2016-11-11 14:22 UTC (permalink / raw)
  To: Eva Rachel Retuya, linux-iio, devel, linux-kernel
  Cc: Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh

On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
> Move the ad7606 driver from staging/iio/adc to iio/adc. Also, update the
> corresponding Makefile and Kconfig associated with the change.

This is obviously OK, but when you generate a patch that moves files use
`git format-patch -M ...`. This will generate a more compact patch.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] staging: iio: ad7606: move out of staging
  2016-11-11  6:34 ` [PATCH 2/2] staging: iio: ad7606: move out of staging Eva Rachel Retuya
  2016-11-11 14:22   ` Lars-Peter Clausen
@ 2016-11-11 16:10   ` kbuild test robot
  1 sibling, 0 replies; 19+ messages in thread
From: kbuild test robot @ 2016-11-11 16:10 UTC (permalink / raw)
  To: Eva Rachel Retuya
  Cc: kbuild-all, linux-iio, devel, linux-kernel, lars,
	Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh,
	Eva Rachel Retuya

[-- Attachment #1: Type: text/plain, Size: 4393 bytes --]

Hi Eva,

[auto build test WARNING on iio/togreg]
[also build test WARNING on next-20161111]
[cannot apply to v4.9-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eva-Rachel-Retuya/staging-iio-ad7606-move-driver-out-of-staging/20161111-143836
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All warnings (new ones prefixed by >>):

   drivers/iio/adc/ad7606_par.c: In function 'ad7606_par16_read_block':
>> drivers/iio/adc/ad7606_par.c:23:23: warning: unused variable 'st' [-Wunused-variable]
     struct ad7606_state *st = iio_priv(indio_dev);
                          ^~
   drivers/iio/adc/ad7606_par.c: In function 'ad7606_par8_read_block':
   drivers/iio/adc/ad7606_par.c:39:23: warning: unused variable 'st' [-Wunused-variable]
     struct ad7606_state *st = iio_priv(indio_dev);
                          ^~

vim +/st +23 drivers/iio/adc/ad7606_par.c

b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22   7   */
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22   8  
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22   9  #include <linux/module.h>
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  10  #include <linux/platform_device.h>
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  11  #include <linux/types.h>
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  12  #include <linux/err.h>
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  13  #include <linux/io.h>
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  14  
06458e27 drivers/staging/iio/adc/ad7606_par.c Jonathan Cameron  2012-04-25  15  #include <linux/iio/iio.h>
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  16  #include "ad7606.h"
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  17  
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  18  static int ad7606_par16_read_block(struct device *dev,
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  19  				   int count, void *buf)
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  20  {
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  21  	struct platform_device *pdev = to_platform_device(dev);
e61181d0 drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-05-18  22  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
e61181d0 drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-05-18 @23  	struct ad7606_state *st = iio_priv(indio_dev);
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  24  
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  25  	insw((unsigned long)st->base_address, buf, count);
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  26  
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  27  	return 0;
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  28  }
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  29  
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  30  static const struct ad7606_bus_ops ad7606_par16_bops = {
b9618c0c drivers/staging/iio/adc/ad7606_par.c Michael Hennerich 2011-02-22  31  	.read_block	= ad7606_par16_read_block,

:::::: The code at line 23 was first introduced by commit
:::::: e61181d0a3e6788d57de9c1ae305d1c6f5fabade staging:iio:adc:ad7606: Use private data space from iio_allocate_device

:::::: TO: Michael Hennerich <michael.hennerich@analog.com>
:::::: CC: Greg Kroah-Hartman <gregkh@suse.de>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 42250 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-11 14:18   ` Lars-Peter Clausen
@ 2016-11-12 14:22     ` Eva Rachel Retuya
  2016-11-14 10:25       ` Lars-Peter Clausen
  2016-11-12 14:24     ` Jonathan Cameron
  1 sibling, 1 reply; 19+ messages in thread
From: Eva Rachel Retuya @ 2016-11-12 14:22 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: linux-iio, linux-kernel, Michael.Hennerich, jic23, knaack.h,
	pmeerw, gregkh

Hello, 

Thanks for explaining it. Now I understand better why read_raw is
formatted in that manner. I have some questions in-line:

On Fri, Nov 11, 2016 at 03:18:37PM +0100, Lars-Peter Clausen wrote:
> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
> > Eliminate the non-standard attribute in_voltage_range and move its
> > functionality under the scale attribute. read_raw() has been taken care
> > of previously so only write_raw() is handled here.
> > 
> > Additionally, rename the attribute in_voltage_range_available into
> > in_voltage_scale_available.
> > 
> > Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> 
> Hi,
> 
> Thanks for the patch. Unfortunately this is not quite this straight forward.
> 
> The scale is what you multiply the raw with to get the value in the standard
> IIO unit. Range as implemented in this driver is the maximum output voltage.
> 
> To get the scale we need to look at the transfer function of the ADC [1].
> The transfer function tells us that 1 LSB is 305uV for the 10V range and
> 152uV for the 5V range.

Instead of 10000 and 5000, these will be the values for the
scale_available attribute?

> 
> More specifically this is $RANGE*2/2**16 (times two since the ADC is bipolar).
> 
> Since the default unit for IIO is mV for voltages we need to multiply this
> by 1000.
> 
> The other thing we need to handle is the case where the RANGE pin is not
> connected to a GPIO but either hardwired to 1 or 0. Which we need to handle
> somehow.

Will this be on the same patch or a separate one? Can you please give a
hint on how to check if it is hardwired?

Eva

> 
> [1]
> http://www.analog.com/media/en/technical-documentation/data-sheets/AD7606_7606-6_7606-4.pdf#page=23
> (right bottom)
> 
> > ---
> >  drivers/staging/iio/adc/ad7606.c | 56 ++++++++++++----------------------------
> >  1 file changed, 16 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c
> > index 4531908..cceb18c 100644
> > --- a/drivers/staging/iio/adc/ad7606.c
> > +++ b/drivers/staging/iio/adc/ad7606.c
> > @@ -161,42 +161,7 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
> >  	return -EINVAL;
> >  }
> >  
> > -static ssize_t ad7606_show_range(struct device *dev,
> > -				 struct device_attribute *attr, char *buf)
> > -{
> > -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > -	struct ad7606_state *st = iio_priv(indio_dev);
> > -
> > -	return sprintf(buf, "%u\n", st->range);
> > -}
> > -
> > -static ssize_t ad7606_store_range(struct device *dev,
> > -				  struct device_attribute *attr,
> > -				  const char *buf, size_t count)
> > -{
> > -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > -	struct ad7606_state *st = iio_priv(indio_dev);
> > -	unsigned long lval;
> > -	int ret;
> > -
> > -	ret = kstrtoul(buf, 10, &lval);
> > -	if (ret)
> > -		return ret;
> > -
> > -	if (!(lval == 5000 || lval == 10000))
> > -		return -EINVAL;
> > -
> > -	mutex_lock(&indio_dev->mlock);
> > -	gpiod_set_value(st->gpio_range, lval == 10000);
> > -	st->range = lval;
> > -	mutex_unlock(&indio_dev->mlock);
> > -
> > -	return count;
> > -}
> > -
> > -static IIO_DEVICE_ATTR(in_voltage_range, S_IRUGO | S_IWUSR,
> > -		       ad7606_show_range, ad7606_store_range, 0);
> > -static IIO_CONST_ATTR(in_voltage_range_available, "5000 10000");
> > +static IIO_CONST_ATTR(in_voltage_scale_available, "5000 10000");
> >  
> >  static int ad7606_oversampling_get_index(unsigned int val)
> >  {
> > @@ -221,6 +186,19 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> >  	int ret;
> >  
> >  	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		if (val2)
> > +			return -EINVAL;
> > +
> > +		if (!(val == 5000 || val == 10000))
> > +			return -EINVAL;
> > +
> > +		mutex_lock(&indio_dev->mlock);
> > +		gpiod_set_value(st->gpio_range, val == 10000);
> > +		st->range = val;
> > +		mutex_unlock(&indio_dev->mlock);
> > +
> > +		return 0;
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> >  		if (val2)
> >  			return -EINVAL;
> > @@ -247,8 +225,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> >  static IIO_CONST_ATTR(oversampling_ratio_available, "1 2 4 8 16 32 64");
> >  
> >  static struct attribute *ad7606_attributes_os_and_range[] = {
> > -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
> > -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
> > +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
> >  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
> >  	NULL,
> >  };
> > @@ -267,8 +244,7 @@ static const struct attribute_group ad7606_attribute_group_os = {
> >  };
> >  
> >  static struct attribute *ad7606_attributes_range[] = {
> > -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
> > -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
> > +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
> >  	NULL,
> >  };
> >  
> > 
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-11 14:18   ` Lars-Peter Clausen
  2016-11-12 14:22     ` Eva Rachel Retuya
@ 2016-11-12 14:24     ` Jonathan Cameron
  2016-11-14 10:30       ` Lars-Peter Clausen
  2016-11-14 16:58       ` Linus Walleij
  1 sibling, 2 replies; 19+ messages in thread
From: Jonathan Cameron @ 2016-11-12 14:24 UTC (permalink / raw)
  To: Lars-Peter Clausen, Eva Rachel Retuya, linux-iio, linux-kernel
  Cc: Michael.Hennerich, knaack.h, pmeerw, gregkh, Linus Walleij

On 11/11/16 14:18, Lars-Peter Clausen wrote:
> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>> Eliminate the non-standard attribute in_voltage_range and move its
>> functionality under the scale attribute. read_raw() has been taken care
>> of previously so only write_raw() is handled here.
>>
>> Additionally, rename the attribute in_voltage_range_available into
>> in_voltage_scale_available.
>>
>> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> 
> Hi,
> 
> Thanks for the patch. Unfortunately this is not quite this straight forward.
> 
> The scale is what you multiply the raw with to get the value in the standard
> IIO unit. Range as implemented in this driver is the maximum output voltage.
> 
> To get the scale we need to look at the transfer function of the ADC [1].
> The transfer function tells us that 1 LSB is 305uV for the 10V range and
> 152uV for the 5V range.
> 
> More specifically this is $RANGE*2/2**16 (times two since the ADC is bipolar).
> 
> Since the default unit for IIO is mV for voltages we need to multiply this
> by 1000.
> 
> The other thing we need to handle is the case where the RANGE pin is not
> connected to a GPIO but either hardwired to 1 or 0. Which we need to handle
> somehow.
Is it just me who thought, we need a fixed GPI like a fixed regulator?
Would allow this sort of fixed wiring to be simply defined.

Linus, worth exploring?

I doubt this will be the last case of this particular problem
(not exactly unusual to hard wire control lines like these as which range
makes sense is often a feature of the device).

Would be a pain to have to add code to every driver to cover the fixed
case.
> 
> [1]
> http://www.analog.com/media/en/technical-documentation/data-sheets/AD7606_7606-6_7606-4.pdf#page=23
> (right bottom)
> 
>> ---
>>  drivers/staging/iio/adc/ad7606.c | 56 ++++++++++++----------------------------
>>  1 file changed, 16 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c
>> index 4531908..cceb18c 100644
>> --- a/drivers/staging/iio/adc/ad7606.c
>> +++ b/drivers/staging/iio/adc/ad7606.c
>> @@ -161,42 +161,7 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>>  	return -EINVAL;
>>  }
>>  
>> -static ssize_t ad7606_show_range(struct device *dev,
>> -				 struct device_attribute *attr, char *buf)
>> -{
>> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> -	struct ad7606_state *st = iio_priv(indio_dev);
>> -
>> -	return sprintf(buf, "%u\n", st->range);
>> -}
>> -
>> -static ssize_t ad7606_store_range(struct device *dev,
>> -				  struct device_attribute *attr,
>> -				  const char *buf, size_t count)
>> -{
>> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> -	struct ad7606_state *st = iio_priv(indio_dev);
>> -	unsigned long lval;
>> -	int ret;
>> -
>> -	ret = kstrtoul(buf, 10, &lval);
>> -	if (ret)
>> -		return ret;
>> -
>> -	if (!(lval == 5000 || lval == 10000))
>> -		return -EINVAL;
>> -
>> -	mutex_lock(&indio_dev->mlock);
>> -	gpiod_set_value(st->gpio_range, lval == 10000);
>> -	st->range = lval;
>> -	mutex_unlock(&indio_dev->mlock);
>> -
>> -	return count;
>> -}
>> -
>> -static IIO_DEVICE_ATTR(in_voltage_range, S_IRUGO | S_IWUSR,
>> -		       ad7606_show_range, ad7606_store_range, 0);
>> -static IIO_CONST_ATTR(in_voltage_range_available, "5000 10000");
>> +static IIO_CONST_ATTR(in_voltage_scale_available, "5000 10000");
>>  
>>  static int ad7606_oversampling_get_index(unsigned int val)
>>  {
>> @@ -221,6 +186,19 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>>  	int ret;
>>  
>>  	switch (mask) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		if (val2)
>> +			return -EINVAL;
>> +
>> +		if (!(val == 5000 || val == 10000))
>> +			return -EINVAL;
>> +
>> +		mutex_lock(&indio_dev->mlock);
>> +		gpiod_set_value(st->gpio_range, val == 10000);
>> +		st->range = val;
>> +		mutex_unlock(&indio_dev->mlock);
>> +
>> +		return 0;
>>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>  		if (val2)
>>  			return -EINVAL;
>> @@ -247,8 +225,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>>  static IIO_CONST_ATTR(oversampling_ratio_available, "1 2 4 8 16 32 64");
>>  
>>  static struct attribute *ad7606_attributes_os_and_range[] = {
>> -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
>> -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
>> +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
>>  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
>>  	NULL,
>>  };
>> @@ -267,8 +244,7 @@ static const struct attribute_group ad7606_attribute_group_os = {
>>  };
>>  
>>  static struct attribute *ad7606_attributes_range[] = {
>> -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
>> -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
>> +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
>>  	NULL,
>>  };
>>  
>>
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] staging: iio: ad7606: move out of staging
  2016-11-11 14:22   ` Lars-Peter Clausen
@ 2016-11-12 14:26     ` Jonathan Cameron
  2016-11-12 14:32       ` Eva Rachel Retuya
  0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Cameron @ 2016-11-12 14:26 UTC (permalink / raw)
  To: Lars-Peter Clausen, Eva Rachel Retuya, linux-iio, linux-kernel
  Cc: Michael.Hennerich, knaack.h, pmeerw, gregkh

On 11/11/16 14:22, Lars-Peter Clausen wrote:
> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>> Move the ad7606 driver from staging/iio/adc to iio/adc. Also, update the
>> corresponding Makefile and Kconfig associated with the change.
> 
> This is obviously OK, but when you generate a patch that moves files use
> `git format-patch -M ...`. This will generate a more compact patch.
> 
I tend to make an exception for staging graduation patches.
The mere posting of the whole driver by not using the move detection
makes for really easy review of whether we have forgotten something
else that needs cleanup before the graduation.

Most of the time -M is much more sensible!

Jonathan

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] staging: iio: ad7606: move out of staging
  2016-11-12 14:26     ` Jonathan Cameron
@ 2016-11-12 14:32       ` Eva Rachel Retuya
  2016-11-12 14:42         ` Jonathan Cameron
  0 siblings, 1 reply; 19+ messages in thread
From: Eva Rachel Retuya @ 2016-11-12 14:32 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, linux-iio, linux-kernel, Michael.Hennerich,
	knaack.h, pmeerw, gregkh

On Sat, Nov 12, 2016 at 02:26:51PM +0000, Jonathan Cameron wrote:
> On 11/11/16 14:22, Lars-Peter Clausen wrote:
> > On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
> >> Move the ad7606 driver from staging/iio/adc to iio/adc. Also, update the
> >> corresponding Makefile and Kconfig associated with the change.
> > 
> > This is obviously OK, but when you generate a patch that moves files use
> > `git format-patch -M ...`. This will generate a more compact patch.
> > 
> I tend to make an exception for staging graduation patches.
> The mere posting of the whole driver by not using the move detection
> makes for really easy review of whether we have forgotten something
> else that needs cleanup before the graduation.
> 
> Most of the time -M is much more sensible!
> 
> Jonathan

Should I use this -M on version 2 or still go with the previous method?

Thanks,
Eva

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] staging: iio: ad7606: move out of staging
  2016-11-12 14:32       ` Eva Rachel Retuya
@ 2016-11-12 14:42         ` Jonathan Cameron
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2016-11-12 14:42 UTC (permalink / raw)
  To: Lars-Peter Clausen, linux-iio, linux-kernel, Michael.Hennerich,
	knaack.h, pmeerw, gregkh

On 12/11/16 14:32, Eva Rachel Retuya wrote:
> On Sat, Nov 12, 2016 at 02:26:51PM +0000, Jonathan Cameron wrote:
>> On 11/11/16 14:22, Lars-Peter Clausen wrote:
>>> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>>>> Move the ad7606 driver from staging/iio/adc to iio/adc. Also, update the
>>>> corresponding Makefile and Kconfig associated with the change.
>>>
>>> This is obviously OK, but when you generate a patch that moves files use
>>> `git format-patch -M ...`. This will generate a more compact patch.
>>>
>> I tend to make an exception for staging graduation patches.
>> The mere posting of the whole driver by not using the move detection
>> makes for really easy review of whether we have forgotten something
>> else that needs cleanup before the graduation.
>>
>> Most of the time -M is much more sensible!
>>
>> Jonathan
> 
> Should I use this -M on version 2 or still go with the previous method?
> 
> Thanks,
> Eva
> 
Go without the no -M option and face the ire of Lars ;)

J

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-12 14:22     ` Eva Rachel Retuya
@ 2016-11-14 10:25       ` Lars-Peter Clausen
  0 siblings, 0 replies; 19+ messages in thread
From: Lars-Peter Clausen @ 2016-11-14 10:25 UTC (permalink / raw)
  To: linux-iio, linux-kernel, Michael.Hennerich, jic23, knaack.h,
	pmeerw, gregkh

On 11/12/2016 03:22 PM, Eva Rachel Retuya wrote:
> Hello, 
> 
> Thanks for explaining it. Now I understand better why read_raw is
> formatted in that manner. I have some questions in-line:
> 
> On Fri, Nov 11, 2016 at 03:18:37PM +0100, Lars-Peter Clausen wrote:
>> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>>> Eliminate the non-standard attribute in_voltage_range and move its
>>> functionality under the scale attribute. read_raw() has been taken care
>>> of previously so only write_raw() is handled here.
>>>
>>> Additionally, rename the attribute in_voltage_range_available into
>>> in_voltage_scale_available.
>>>
>>> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
>>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
>>
>> Hi,
>>
>> Thanks for the patch. Unfortunately this is not quite this straight forward.
>>
>> The scale is what you multiply the raw with to get the value in the standard
>> IIO unit. Range as implemented in this driver is the maximum output voltage.
>>
>> To get the scale we need to look at the transfer function of the ADC [1].
>> The transfer function tells us that 1 LSB is 305uV for the 10V range and
>> 152uV for the 5V range.
> 
> Instead of 10000 and 5000, these will be the values for the
> scale_available attribute?

Yes. But these are slightly truncated values, we should try to minimize the
rounding error introduced by those scales by making them as accurate as
possible. The exact values are 5/2**16 and 2.5/2**16. Neither of which we
can express accurately. But we should use at least 6 digits and also round
to the closest rather than truncating.

> 
>>
>> More specifically this is $RANGE*2/2**16 (times two since the ADC is bipolar).
>>
>> Since the default unit for IIO is mV for voltages we need to multiply this
>> by 1000.
>>
>> The other thing we need to handle is the case where the RANGE pin is not
>> connected to a GPIO but either hardwired to 1 or 0. Which we need to handle
>> somehow.
> 
> Will this be on the same patch or a separate one? Can you please give a
> hint on how to check if it is hardwired?

The current driver checks if the GPIO is set and if it is not it does not
register the range attribute. As a first step we could mimic this behavior,
but long term we should find a way to discover the setting of the hardwired
pin and report the scale accordingly.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-12 14:24     ` Jonathan Cameron
@ 2016-11-14 10:30       ` Lars-Peter Clausen
  2016-11-14 17:26         ` Jonathan Cameron
  2016-11-14 16:58       ` Linus Walleij
  1 sibling, 1 reply; 19+ messages in thread
From: Lars-Peter Clausen @ 2016-11-14 10:30 UTC (permalink / raw)
  To: Jonathan Cameron, Eva Rachel Retuya, linux-iio, linux-kernel
  Cc: Michael.Hennerich, knaack.h, pmeerw, gregkh, Linus Walleij

On 11/12/2016 03:24 PM, Jonathan Cameron wrote:
> On 11/11/16 14:18, Lars-Peter Clausen wrote:
>> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>>> Eliminate the non-standard attribute in_voltage_range and move its
>>> functionality under the scale attribute. read_raw() has been taken care
>>> of previously so only write_raw() is handled here.
>>>
>>> Additionally, rename the attribute in_voltage_range_available into
>>> in_voltage_scale_available.
>>>
>>> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
>>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
>>
>> Hi,
>>
>> Thanks for the patch. Unfortunately this is not quite this straight forward.
>>
>> The scale is what you multiply the raw with to get the value in the standard
>> IIO unit. Range as implemented in this driver is the maximum output voltage.
>>
>> To get the scale we need to look at the transfer function of the ADC [1].
>> The transfer function tells us that 1 LSB is 305uV for the 10V range and
>> 152uV for the 5V range.
>>
>> More specifically this is $RANGE*2/2**16 (times two since the ADC is bipolar).
>>
>> Since the default unit for IIO is mV for voltages we need to multiply this
>> by 1000.
>>
>> The other thing we need to handle is the case where the RANGE pin is not
>> connected to a GPIO but either hardwired to 1 or 0. Which we need to handle
>> somehow.
> Is it just me who thought, we need a fixed GPI like a fixed regulator?
> Would allow this sort of fixed wiring to be simply defined.
> 
> Linus, worth exploring?
> 
> I doubt this will be the last case of this particular problem
> (not exactly unusual to hard wire control lines like these as which range
> makes sense is often a feature of the device).
> 
> Would be a pain to have to add code to every driver to cover the fixed
> case.

We still have to add code to every driver to cover the fixed case since the
mode of operation is inherently different. But it would be nice to have a
coherent way of doing so with a standardized interface rather than having
every device come up with its own code and bindings.

This could either be handled directly by the GPIO API or by a small set of
helper functions on top of it.

I think the most important part for now is to agree on a standard binding to
describe hardwired GPIOs. We can still rework the kernel API later on, but
the DT bindings will be set in stone.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-12 14:24     ` Jonathan Cameron
  2016-11-14 10:30       ` Lars-Peter Clausen
@ 2016-11-14 16:58       ` Linus Walleij
  2016-11-14 18:53         ` Lars-Peter Clausen
  1 sibling, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2016-11-14 16:58 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, Eva Rachel Retuya, linux-iio, linux-kernel,
	Michael Hennerich, Hartmut Knaack, Peter Meerwald, Greg KH

On Sat, Nov 12, 2016 at 3:24 PM, Jonathan Cameron <jic23@kernel.org> wrote:

> Is it just me who thought, we need a fixed GPI like a fixed regulator?
> Would allow this sort of fixed wiring to be simply defined.
>
> Linus, worth exploring?

So if fixed regulator is for a voltage provider, this would be
pretty much the inverse: deciding for a voltage range by switching
a GPIO.

No previous experience with that, it should be outside of the
GPIO subsystem for sure...

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-14 10:30       ` Lars-Peter Clausen
@ 2016-11-14 17:26         ` Jonathan Cameron
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2016-11-14 17:26 UTC (permalink / raw)
  To: Lars-Peter Clausen, Jonathan Cameron, Eva Rachel Retuya,
	linux-iio, linux-kernel
  Cc: Michael.Hennerich, knaack.h, pmeerw, gregkh, Linus Walleij



On 14 November 2016 10:30:50 GMT+00:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>On 11/12/2016 03:24 PM, Jonathan Cameron wrote:
>> On 11/11/16 14:18, Lars-Peter Clausen wrote:
>>> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>>>> Eliminate the non-standard attribute in_voltage_range and move its
>>>> functionality under the scale attribute. read_raw() has been taken
>care
>>>> of previously so only write_raw() is handled here.
>>>>
>>>> Additionally, rename the attribute in_voltage_range_available into
>>>> in_voltage_scale_available.
>>>>
>>>> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
>>>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
>>>
>>> Hi,
>>>
>>> Thanks for the patch. Unfortunately this is not quite this straight
>forward.
>>>
>>> The scale is what you multiply the raw with to get the value in the
>standard
>>> IIO unit. Range as implemented in this driver is the maximum output
>voltage.
>>>
>>> To get the scale we need to look at the transfer function of the ADC
>[1].
>>> The transfer function tells us that 1 LSB is 305uV for the 10V range
>and
>>> 152uV for the 5V range.
>>>
>>> More specifically this is $RANGE*2/2**16 (times two since the ADC is
>bipolar).
>>>
>>> Since the default unit for IIO is mV for voltages we need to
>multiply this
>>> by 1000.
>>>
>>> The other thing we need to handle is the case where the RANGE pin is
>not
>>> connected to a GPIO but either hardwired to 1 or 0. Which we need to
>handle
>>> somehow.
>> Is it just me who thought, we need a fixed GPI like a fixed
>regulator?
>> Would allow this sort of fixed wiring to be simply defined.
>> 
>> Linus, worth exploring?
>> 
>> I doubt this will be the last case of this particular problem
>> (not exactly unusual to hard wire control lines like these as which
>range
>> makes sense is often a feature of the device).
>> 
>> Would be a pain to have to add code to every driver to cover the
>fixed
>> case.
>
>We still have to add code to every driver to cover the fixed case since
>the
>mode of operation is inherently different. But it would be nice to have
>a
>coherent way of doing so with a standardized interface rather than
>having
>every device come up with its own code and bindings.
Agreed. Not totally obvious how to do it but definitely don't want to work around it by custom bindings all over the place.

A simple is_gpo_fixed or similar would get the driver the info it needs. Could do it with errors 
but that would perhaps be ugly!
The other flags related to type of input/output can be emulated so there doesn't seem to be anything similar...
>
>This could either be handled directly by the GPIO API or by a small set
>of
>helper functions on top of it.
>
>I think the most important part for now is to agree on a standard
>binding to
>describe hardwired GPIOs. We can still rework the kernel API later on,
>but
>the DT bindings will be set in stone.
Agreed. Some magic gpio description or a fake gpio chip similar to fixed regs?

Jonathan
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-14 16:58       ` Linus Walleij
@ 2016-11-14 18:53         ` Lars-Peter Clausen
  2016-11-14 22:15           ` Jonathan Cameron
  2016-11-14 23:12           ` Linus Walleij
  0 siblings, 2 replies; 19+ messages in thread
From: Lars-Peter Clausen @ 2016-11-14 18:53 UTC (permalink / raw)
  To: Linus Walleij, Jonathan Cameron
  Cc: Eva Rachel Retuya, linux-iio, linux-kernel, Michael Hennerich,
	Hartmut Knaack, Peter Meerwald, Greg KH

On 11/14/2016 05:58 PM, Linus Walleij wrote:
> On Sat, Nov 12, 2016 at 3:24 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> 
>> Is it just me who thought, we need a fixed GPI like a fixed regulator?
>> Would allow this sort of fixed wiring to be simply defined.
>>
>> Linus, worth exploring?
> 
> So if fixed regulator is for a voltage provider, this would be
> pretty much the inverse: deciding for a voltage range by switching
> a GPIO.

It's about figuring out the setting of a "GPIO" that can't be changed from
software.

Devices sometimes, instead of a configuration bus like I2C or SPI, use
simple input pins, that can either be set to high or low, to allow software
the state of the device. The GPIO API is typically used to configure these pins.

This works fine as long as the pin is connected to a GPIO. But sometimes the
system designer decides that a settings does not need to be configurable, in
this case the pin will be tied to logic low or high directly on the PCB
without any GPIO controller being involved.

Sometimes a driver wants to know how the pin is wired up so it can report to
userspace this part runs in the following mode and the mode can't be
changed. In a sense it is like a reverse GPIO hog.

Considering that this is a common usecase the question was how this can be
implemented in a driver independent way to avoid code duplication and
slightly different variations of what is effectively the same DT/ACPI binding.

E.g. lets say for a configurable pin you use

	range-gpio = <&gpio ...>;

and for a static pin

	range-gpio-fixed = <1>;

Or something similar.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-14 18:53         ` Lars-Peter Clausen
@ 2016-11-14 22:15           ` Jonathan Cameron
  2016-11-14 23:12           ` Linus Walleij
  1 sibling, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2016-11-14 22:15 UTC (permalink / raw)
  To: Lars-Peter Clausen, Linus Walleij, Jonathan Cameron
  Cc: Eva Rachel Retuya, linux-iio, linux-kernel, Michael Hennerich,
	Hartmut Knaack, Peter Meerwald, Greg KH



On 14 November 2016 18:53:28 GMT+00:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>On 11/14/2016 05:58 PM, Linus Walleij wrote:
>> On Sat, Nov 12, 2016 at 3:24 PM, Jonathan Cameron <jic23@kernel.org>
>wrote:
>> 
>>> Is it just me who thought, we need a fixed GPI like a fixed
>regulator?
Probably didn't help clarity that I described it as an input pin whereas it's kind of like having an 
output pin whose state you can't change...

>>> Would allow this sort of fixed wiring to be simply defined.
>>>
>>> Linus, worth exploring?
>> 
>> So if fixed regulator is for a voltage provider, this would be
>> pretty much the inverse: deciding for a voltage range by switching
>> a GPIO.
>
>It's about figuring out the setting of a "GPIO" that can't be changed
>from
>software.
>
>Devices sometimes, instead of a configuration bus like I2C or SPI, use
>simple input pins, that can either be set to high or low, to allow
>software
>the state of the device. The GPIO API is typically used to configure
>these pins.
>
>This works fine as long as the pin is connected to a GPIO. But
>sometimes the
>system designer decides that a settings does not need to be
>configurable, in
>this case the pin will be tied to logic low or high directly on the PCB
>without any GPIO controller being involved.
>
>Sometimes a driver wants to know how the pin is wired up so it can
>report to
>userspace this part runs in the following mode and the mode can't be
>changed. In a sense it is like a reverse GPIO hog.
>
>Considering that this is a common usecase the question was how this can
>be
>implemented in a driver independent way to avoid code duplication and
>slightly different variations of what is effectively the same DT/ACPI
>binding.
>
>E.g. lets say for a configurable pin you use
>
>	range-gpio = <&gpio ...>;
>
>and for a static pin
>
>	range-gpio-fixed = <1>;
>
>Or something similar.
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-14 18:53         ` Lars-Peter Clausen
  2016-11-14 22:15           ` Jonathan Cameron
@ 2016-11-14 23:12           ` Linus Walleij
  2016-11-19 12:32             ` Jonathan Cameron
  1 sibling, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2016-11-14 23:12 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Jonathan Cameron, Eva Rachel Retuya, linux-iio, linux-kernel,
	Michael Hennerich, Hartmut Knaack, Peter Meerwald, Greg KH

On Mon, Nov 14, 2016 at 7:53 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:

> It's about figuring out the setting of a "GPIO" that can't be changed from
> software.
>
> Devices sometimes, instead of a configuration bus like I2C or SPI, use
> simple input pins, that can either be set to high or low, to allow software
> the state of the device. The GPIO API is typically used to configure these pins.
>
> This works fine as long as the pin is connected to a GPIO. But sometimes the
> system designer decides that a settings does not need to be configurable, in
> this case the pin will be tied to logic low or high directly on the PCB
> without any GPIO controller being involved.
>
> Sometimes a driver wants to know how the pin is wired up so it can report to
> userspace this part runs in the following mode and the mode can't be
> changed. In a sense it is like a reverse GPIO hog.
>
> Considering that this is a common usecase the question was how this can be
> implemented in a driver independent way to avoid code duplication and
> slightly different variations of what is effectively the same DT/ACPI binding.
>
> E.g. lets say for a configurable pin you use
>
>         range-gpio = <&gpio ...>;
>
> and for a static pin
>
>         range-gpio-fixed = <1>;
>
> Or something similar.

Aha I understand.

Usually I feel we need not shoehorn stuff into GPIO because it is convenient,
it might be best to leave the GPIO optional and if it is not there, look for
a custom attribute that represents the "hogging" to 0/1. I think trying
to extend GPIO bindings to cover it is overgeneralization, instead go
for a local binding for this kind of devices.

But mainly it is a question to the DT bindings maintainers.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale
  2016-11-14 23:12           ` Linus Walleij
@ 2016-11-19 12:32             ` Jonathan Cameron
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Cameron @ 2016-11-19 12:32 UTC (permalink / raw)
  To: Linus Walleij, Lars-Peter Clausen
  Cc: Eva Rachel Retuya, linux-iio, linux-kernel, Michael Hennerich,
	Hartmut Knaack, Peter Meerwald, Greg KH, devicetree, Rob Herring,
	Mark Rutland

On 14/11/16 23:12, Linus Walleij wrote:
> On Mon, Nov 14, 2016 at 7:53 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> 
>> It's about figuring out the setting of a "GPIO" that can't be changed from
>> software.
>>
>> Devices sometimes, instead of a configuration bus like I2C or SPI, use
>> simple input pins, that can either be set to high or low, to allow software
>> the state of the device. The GPIO API is typically used to configure these pins.
>>
>> This works fine as long as the pin is connected to a GPIO. But sometimes the
>> system designer decides that a settings does not need to be configurable, in
>> this case the pin will be tied to logic low or high directly on the PCB
>> without any GPIO controller being involved.
>>
>> Sometimes a driver wants to know how the pin is wired up so it can report to
>> userspace this part runs in the following mode and the mode can't be
>> changed. In a sense it is like a reverse GPIO hog.
>>
>> Considering that this is a common usecase the question was how this can be
>> implemented in a driver independent way to avoid code duplication and
>> slightly different variations of what is effectively the same DT/ACPI binding.
>>
>> E.g. lets say for a configurable pin you use
>>
>>         range-gpio = <&gpio ...>;
>>
>> and for a static pin
>>
>>         range-gpio-fixed = <1>;
>>
>> Or something similar.
> 
> Aha I understand.
> 
> Usually I feel we need not shoehorn stuff into GPIO because it is convenient,
> it might be best to leave the GPIO optional and if it is not there, look for
> a custom attribute that represents the "hogging" to 0/1. I think trying
> to extend GPIO bindings to cover it is overgeneralization, instead go
> for a local binding for this kind of devices.
> 
> But mainly it is a question to the DT bindings maintainers.
That's a reasonable approach, but I'd certainly like to see a generic binding
to describe it.  It's a pretty common situation.

Seems more likely we'll get a device tree maintainer response if we cc them ;)

So Mark, Rob and thoughts on this?

Thanks,

Jonathan
> 
> Yours,
> Linus Walleij
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2016-11-19 12:32 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-11  6:34 [PATCH 0/2] staging: iio: ad7606: move driver out of staging Eva Rachel Retuya
2016-11-11  6:34 ` [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale Eva Rachel Retuya
2016-11-11 14:18   ` Lars-Peter Clausen
2016-11-12 14:22     ` Eva Rachel Retuya
2016-11-14 10:25       ` Lars-Peter Clausen
2016-11-12 14:24     ` Jonathan Cameron
2016-11-14 10:30       ` Lars-Peter Clausen
2016-11-14 17:26         ` Jonathan Cameron
2016-11-14 16:58       ` Linus Walleij
2016-11-14 18:53         ` Lars-Peter Clausen
2016-11-14 22:15           ` Jonathan Cameron
2016-11-14 23:12           ` Linus Walleij
2016-11-19 12:32             ` Jonathan Cameron
2016-11-11  6:34 ` [PATCH 2/2] staging: iio: ad7606: move out of staging Eva Rachel Retuya
2016-11-11 14:22   ` Lars-Peter Clausen
2016-11-12 14:26     ` Jonathan Cameron
2016-11-12 14:32       ` Eva Rachel Retuya
2016-11-12 14:42         ` Jonathan Cameron
2016-11-11 16:10   ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).