[PATCHv2 0/3] Add support for the Nuvoton NAU7802 ADC to the cfa10049

[PATCHv2 0/3] Add support for the Nuvoton NAU7802 ADC to the cfa10049

[Alexandre Belloni]

Hi,

Sorry for the delay, this is v2 of the patchset that adds support for the 3
Nuvoton NAU7802 ADCs present on the CFA-10049 board from Crystalfontz. The first
patch adds the driver, the next ones are adding them in the DT after switching
the second i2c bus to bitbanging following a lot of timeout issues we have on
that bus.

Since v1, I've taken into account the review from Jonathan Cameron, Lars-Peter
Clausen and Maxime Ripard:
 - channels are now statically allocated
 - no wrappers around i2c functions
 - sampling_freq is a shared attribute
 - setting gain is done through scale
 - cosmetics changes (s/idev/indio_dev/, blank lines before returns)
 - use of sign_extend32 to extend the sign
 - reading scale now returns IIO_VAL_FRACTIONAL_LOG2
 - no dt lookup for interrupts, this is done in of_i2c.c but that was mask due
 to a bug.

what I didn't address is setting the external reference voltage through a fixed
regulator and I kept the vldo attribute to set the internal voltage reference in
the DT as Jonathan seemed to be ok with that. If anybody is using a nau7802 with
an external reference, it will still be time to add the fixed regulator. It
shouldn't be difficult but I don't have any hardware to test.

Regards,

Alexandre Belloni (1):
  iio: Add Nuvoton NAU7802 ADC driver

Maxime Ripard (2):
  ARM: mxs: cfa10049: Switch bus i2c1 to bitbanging
  ARM: mxs: cfa10049: Add NAU7802 ADCs to the device tree

 .../bindings/iio/adc/nuvoton-nau7802.txt           |  17 +
 arch/arm/boot/dts/imx28-cfa10049.dts               | 126 +++--
 drivers/iio/adc/Kconfig                            |   9 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/nau7802.c                          | 603 +++++++++++++++++++++
 5 files changed, 713 insertions(+), 43 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
 create mode 100644 drivers/iio/adc/nau7802.c

-- 
1.8.1.2

[PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Alexandre Belloni]

The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
gain and sampling rates.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 .../bindings/iio/adc/nuvoton-nau7802.txt           |  17 +
 drivers/iio/adc/Kconfig                            |   9 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/nau7802.c                          | 603 +++++++++++++++++++++
 4 files changed, 630 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
 create mode 100644 drivers/iio/adc/nau7802.c

diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
new file mode 100644
index 0000000..9bc4218
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
@@ -0,0 +1,17 @@
+* Nuvoton NAU7802 Analog to Digital Converter (ADC)
+
+Required properties:
+  - compatible: Should be "nuvoton,nau7802"
+  - reg: Should contain the ADC I2C address
+
+Optional properties:
+  - nuvoton,vldo: Reference voltage in millivolts (integer)
+  - interrupts: IRQ line for the ADC. If not used the driver will use
+    polling.
+
+Example:
+adc2: nau7802@2a {
+	compatible = "nuvoton,nau7802";
+	reg = <0x2a>;
+	nuvoton,vldo = <3000>;
+};
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ab0767e6..d7f9ed8 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -133,6 +133,15 @@ config MAX1363
 	  max11646, max11647) Provides direct access via sysfs and buffered
 	  data via the iio dev interface.
 
+config NAU7802
+	tristate "Nuvoton NAU7802 ADC driver"
+	depends on I2C
+	help
+	  Say yes here to build support for Nuvoton NAU7802 ADC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called nau7802.
+
 config TI_ADC081C
 	tristate "Texas Instruments ADC081C021/027"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0a825be..d426081 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_NAU7802) += nau7802.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
 obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
new file mode 100644
index 0000000..e1b6981
--- /dev/null
+++ b/drivers/iio/adc/nau7802.c
@@ -0,0 +1,603 @@
+/*
+ * Driver for the Nuvoton NAU7802 ADC
+ *
+ * Copyright 2013 Free Electrons
+ *
+ * Licensed under the GPLv2 or later.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/wait.h>
+#include <linux/log2.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define NAU7802_REG_PUCTRL	0x00
+#define NAU7802_PUCTRL_RR(x)		(x << 0)
+#define NAU7802_PUCTRL_RR_BIT		NAU7802_PUCTRL_RR(1)
+#define NAU7802_PUCTRL_PUD(x)		(x << 1)
+#define NAU7802_PUCTRL_PUD_BIT		NAU7802_PUCTRL_PUD(1)
+#define NAU7802_PUCTRL_PUA(x)		(x << 2)
+#define NAU7802_PUCTRL_PUA_BIT		NAU7802_PUCTRL_PUA(1)
+#define NAU7802_PUCTRL_PUR(x)		(x << 3)
+#define NAU7802_PUCTRL_PUR_BIT		NAU7802_PUCTRL_PUR(1)
+#define NAU7802_PUCTRL_CS(x)		(x << 4)
+#define NAU7802_PUCTRL_CS_BIT		NAU7802_PUCTRL_CS(1)
+#define NAU7802_PUCTRL_CR(x)		(x << 5)
+#define NAU7802_PUCTRL_CR_BIT		NAU7802_PUCTRL_CR(1)
+#define NAU7802_PUCTRL_AVDDS(x)		(x << 7)
+#define NAU7802_PUCTRL_AVDDS_BIT	NAU7802_PUCTRL_AVDDS(1)
+#define NAU7802_REG_CTRL1	0x01
+#define NAU7802_CTRL1_VLDO(x)		(x << 3)
+#define NAU7802_CTRL1_GAINS(x)		(x)
+#define NAU7802_CTRL1_GAINS_BITS	0x07
+#define NAU7802_REG_CTRL2	0x02
+#define NAU7802_CTRL2_CHS(x)		(x << 7)
+#define NAU7802_CTRL2_CRS(x)		(x << 4)
+#define NAU7802_SAMP_FREQ_320	0x07
+#define NAU7802_CTRL2_CHS_BIT		NAU7802_CTRL2_CHS(1)
+#define NAU7802_REG_ADC_B2	0x12
+#define NAU7802_REG_ADC_B1	0x13
+#define NAU7802_REG_ADC_B0	0x14
+#define NAU7802_REG_ADC_CTRL	0x15
+
+#define NAU7802_DEFAULT_CONVERSIONS 6
+
+struct nau7802_state {
+	struct i2c_client	*client;
+	s32			last_value;
+	struct mutex		lock;
+	struct mutex		data_lock;
+	u32			vref_mv;
+	u32			conversion_count;
+	u32			min_conversions;
+	u8			sample_rate;
+	u32			scale_avail[8];
+	struct completion	value_ok;
+};
+
+#define NAU7802_CHANNEL(chan) {					\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = (chan),					\
+	.scan_index = (chan),					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ)	\
+}
+
+static const struct iio_chan_spec nau7802_chan_array[] = {
+	NAU7802_CHANNEL(0),
+	NAU7802_CHANNEL(1),
+};
+
+static const u16 nau7802_sample_freq_avail[] = {10, 20, 40, 80,
+						10, 10, 10, 320};
+
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("10 40 80 320");
+
+static ssize_t nau7802_sysfs_set_min_conversions(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf,
+		size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct nau7802_state *st = iio_priv(indio_dev);
+	u32 val;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&st->lock);
+	st->min_conversions = val;
+	st->conversion_count = 0;
+	mutex_unlock(&st->lock);
+
+	return len;
+}
+
+static ssize_t nau7802_sysfs_get_min_conversions(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct nau7802_state *st = iio_priv(indio_dev);
+
+	return sprintf(buf, "%d\n", st->min_conversions);
+}
+
+static IIO_DEVICE_ATTR(min_conversions, S_IWUSR | S_IRUGO,
+		nau7802_sysfs_get_min_conversions,
+		nau7802_sysfs_set_min_conversions, 0);
+
+static struct attribute *nau7802_attributes[] = {
+	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+	&iio_dev_attr_min_conversions.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group nau7802_attribute_group = {
+	.attrs = nau7802_attributes,
+};
+
+static int nau7802_set_gain(struct nau7802_state *st, int gain)
+{
+	u8 data;
+	int ret;
+
+	mutex_lock(&st->lock);
+	st->conversion_count = 0;
+
+	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL1);
+	if (data < 0)
+		goto nau7802_sysfs_set_gain_out;
+	ret = i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL1,
+					(data & (~NAU7802_CTRL1_GAINS_BITS)) |
+					gain);
+
+nau7802_sysfs_set_gain_out:
+	mutex_unlock(&st->lock);
+
+	return ret ? ret : 0;
+}
+
+static int nau7802_read_conversion(struct nau7802_state *st)
+{
+	u8 data;
+
+	mutex_lock(&st->data_lock);
+	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_ADC_B2);
+	if (data < 0)
+		goto nau7802_read_conversion_out;
+	st->last_value = data << 16;
+
+	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_ADC_B1);
+	if (data < 0)
+		goto nau7802_read_conversion_out;
+	st->last_value |= data << 8;
+
+	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_ADC_B0);
+	if (data < 0)
+		goto nau7802_read_conversion_out;
+	st->last_value |= data;
+
+	st->last_value = sign_extend32(st->last_value, 23);
+
+nau7802_read_conversion_out:
+	mutex_unlock(&st->data_lock);
+
+	return data;
+}
+
+/*
+ * Conversions are synchronised on the rising edge of NAU7802_PUCTRL_CS_BIT
+ */
+static int nau7802_sync(struct nau7802_state *st)
+{
+	int ret;
+	u8 data;
+
+	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_PUCTRL);
+	if (data < 0)
+		return data;
+	ret = i2c_smbus_write_byte_data(st->client, NAU7802_REG_PUCTRL,
+				data | NAU7802_PUCTRL_CS_BIT);
+
+	return ret;
+}
+
+static irqreturn_t nau7802_eoc_trigger(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct nau7802_state *st = iio_priv(indio_dev);
+	u8 status;
+
+	status = i2c_smbus_read_byte_data(st->client, NAU7802_REG_PUCTRL);
+	if (status < 0)
+		return IRQ_HANDLED;
+
+	if (!(status & NAU7802_PUCTRL_CR_BIT))
+		return IRQ_NONE;
+
+	if (nau7802_read_conversion(st) < 0)
+		return IRQ_HANDLED;
+
+	/* Because there is actually only one ADC for both channels, we have to
+	 * wait for enough conversions to happen before getting a significant
+	 * value when changing channels and the values are far appart.
+	 */
+	if (st->conversion_count < st->min_conversions)
+		st->conversion_count++;
+	if (st->conversion_count >= st->min_conversions)
+		complete_all(&st->value_ok);
+
+	return IRQ_HANDLED;
+}
+
+static int nau7802_read_irq(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan,
+			int *val)
+{
+	struct nau7802_state *st = iio_priv(indio_dev);
+	int ret;
+
+	INIT_COMPLETION(st->value_ok);
+	enable_irq(st->client->irq);
+
+	nau7802_sync(st);
+
+	/* read registers to ensure we flush everything */
+	ret = nau7802_read_conversion(st);
+	if (ret < 0)
+		goto read_chan_info_failure;
+
+	/* Wait for a conversion to finish */
+	ret = wait_for_completion_interruptible_timeout(&st->value_ok,
+			msecs_to_jiffies(1000));
+	if (ret == 0)
+		ret = -ETIMEDOUT;
+
+	if (ret < 0)
+		goto read_chan_info_failure;
+
+	disable_irq(st->client->irq);
+
+	*val = st->last_value;
+
+	return IIO_VAL_INT;
+
+read_chan_info_failure:
+	disable_irq(st->client->irq);
+
+	return ret;
+}
+
+static int nau7802_read_poll(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan,
+			int *val)
+{
+	struct nau7802_state *st = iio_priv(indio_dev);
+	int ret;
+	u8 data;
+
+	nau7802_sync(st);
+
+	/* read registers to ensure we flush everything */
+	ret = nau7802_read_conversion(st);
+	if (ret < 0)
+		return ret;
+
+	/* Because there is actually only one ADC for both channels, we have to
+	 * wait for enough conversions to happen before getting a significant
+	 * value when changing channels and the values are far appart.
+	 */
+	do {
+		data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_PUCTRL);
+		if (data < 0)
+			return data;
+
+		while (!(data & NAU7802_PUCTRL_CR_BIT)) {
+			if (st->sample_rate != NAU7802_SAMP_FREQ_320)
+				msleep(20);
+			else
+				mdelay(4);
+			data = i2c_smbus_read_byte_data(st->client,
+							NAU7802_REG_PUCTRL);
+			if (data < 0)
+				return data;
+		}
+
+		nau7802_read_conversion(st);
+		if (ret < 0)
+			return ret;
+		if (st->conversion_count < st->min_conversions)
+			st->conversion_count++;
+	} while (st->conversion_count < st->min_conversions);
+
+	*val = st->last_value;
+
+	return IIO_VAL_INT;
+}
+
+static int nau7802_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct nau7802_state *st = iio_priv(indio_dev);
+	u8 data;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&st->lock);
+		/*
+		 * Select the channel to use
+		 *   - Channel 1 is value 0 in the CHS register
+		 *   - Channel 2 is value 1 in the CHS register
+		 */
+		data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL2);
+		if (data < 0)
+			return data;
+
+		if (((data & NAU7802_CTRL2_CHS_BIT) && !chan->channel) ||
+				(!(data & NAU7802_CTRL2_CHS_BIT) &&
+				 chan->channel)) {
+			st->conversion_count = 0;
+			ret = i2c_smbus_write_byte_data(st->client,
+					NAU7802_REG_CTRL2,
+					NAU7802_CTRL2_CHS(chan->channel) |
+					NAU7802_CTRL2_CRS(st->sample_rate));
+
+			if (ret < 0)
+				return ret;
+		}
+
+		if (st->client->irq)
+			ret = nau7802_read_irq(indio_dev, chan, val);
+		else
+			ret = nau7802_read_poll(indio_dev, chan, val);
+
+		mutex_unlock(&st->lock);
+		return ret;
+
+	case IIO_CHAN_INFO_SCALE:
+		data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL1);
+		if (data < 0)
+			return data;
+
+		/* we have 24 bits of signed data, that means 23 bits of data
+		 * plus the sign bit */
+		*val = st->vref_mv;
+		*val2 = 23 + (data & NAU7802_CTRL1_GAINS_BITS);
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val =  nau7802_sample_freq_avail[st->sample_rate];
+		*val2 = 0;
+		return IIO_VAL_INT;
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int nau7802_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct nau7802_state *st = iio_priv(indio_dev);
+	int i;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++)
+			if (val2 == st->scale_avail[i])
+				return nau7802_set_gain(st, i);
+
+		break;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		for (i = 0; i < ARRAY_SIZE(nau7802_sample_freq_avail); i++)
+			if (val == nau7802_sample_freq_avail[i]) {
+				mutex_lock(&st->lock);
+				st->sample_rate = i;
+				st->conversion_count = 0;
+				i2c_smbus_write_byte_data(st->client,
+					NAU7802_REG_CTRL2,
+					NAU7802_CTRL2_CRS(st->sample_rate));
+				mutex_unlock(&st->lock);
+				return 0;
+			}
+
+		break;
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int nau7802_write_raw_get_fmt(struct iio_dev *indio_dev,
+				     struct iio_chan_spec const *chan,
+				     long mask)
+{
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static const struct iio_info nau7802_info = {
+	.driver_module = THIS_MODULE,
+	.read_raw = &nau7802_read_raw,
+	.write_raw = &nau7802_write_raw,
+	.write_raw_get_fmt = nau7802_write_raw_get_fmt,
+	.attrs = &nau7802_attribute_group,
+};
+
+static int nau7802_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct nau7802_state *st;
+	struct device_node *np = client->dev.of_node;
+	int i, ret;
+	u8 data;
+	u32 tmp;
+
+	if (!client->dev.of_node) {
+		dev_err(&client->dev, "No device tree node available.\n");
+		return -EINVAL;
+	}
+
+	indio_dev = iio_device_alloc(sizeof(*st));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	i2c_set_clientdata(client, indio_dev);
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->name = dev_name(&client->dev);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &nau7802_info;
+
+	st->client = client;
+
+	/* Reset the device */
+	i2c_smbus_write_byte_data(st->client, NAU7802_REG_PUCTRL,
+				  NAU7802_PUCTRL_RR_BIT);
+
+	/* Enter normal operation mode */
+	i2c_smbus_write_byte_data(st->client, NAU7802_REG_PUCTRL,
+				  NAU7802_PUCTRL_PUD_BIT);
+
+	/*
+	 * After about 200 usecs, the device should be ready and then
+	 * the Power Up bit will be set to 1. If not, wait for it.
+	 */
+	udelay(210);
+	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_PUCTRL);
+	if (data < 0)
+		return -ENODEV;
+	if (!(data & NAU7802_PUCTRL_PUR_BIT))
+		return -ENODEV;
+
+	of_property_read_u32(np, "nuvoton,vldo", &tmp);
+	st->vref_mv = tmp;
+
+	/* Populate available ADC input ranges */
+	for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++)
+		st->scale_avail[i] = (((u64)st->vref_mv) * 1000000000ULL)
+					   >> (23 + i);
+
+	data = NAU7802_PUCTRL_PUD_BIT | NAU7802_PUCTRL_PUA_BIT |
+		NAU7802_PUCTRL_CS_BIT;
+	if (tmp >= 2400)
+		data |= NAU7802_PUCTRL_AVDDS_BIT;
+
+	i2c_smbus_write_byte_data(st->client, NAU7802_REG_PUCTRL, data);
+	i2c_smbus_write_byte_data(st->client, NAU7802_REG_ADC_CTRL, 0x30);
+
+	if (tmp >= 2400) {
+		data = NAU7802_CTRL1_VLDO((4500 - tmp) / 300);
+		i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL1, data);
+	}
+
+	st->min_conversions = NAU7802_DEFAULT_CONVERSIONS;
+
+	/*
+	 * The ADC fires continuously and we can't do anything about
+	 * it. So we need to have the IRQ disabled by default, and we
+	 * will enable them back when we will need them..
+	 */
+	if (client->irq) {
+		ret = request_threaded_irq(client->irq,
+				NULL,
+				nau7802_eoc_trigger,
+				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+				client->dev.driver->name,
+				indio_dev);
+		if (ret) {
+			/*
+			 * What may happen here is that our IRQ controller is
+			 * not able to get level interrupt but this is required
+			 * by this ADC as when going over 40 sample per second,
+			 * the interrupt line may stay high between conversions.
+			 * So, we continue no matter what but we switch to
+			 * polling mode.
+			 */
+			dev_info(&client->dev,
+				"Failed to allocate IRQ, using polling mode\n");
+			client->irq = 0;
+		} else
+			disable_irq(client->irq);
+	}
+
+	if (!client->irq) {
+		/*
+		 * We are polling, use the fastest sample rate by
+		 * default
+		 */
+		st->sample_rate = NAU7802_SAMP_FREQ_320;
+		i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL2,
+					  NAU7802_CTRL2_CRS(st->sample_rate));
+	}
+
+	/* Setup the ADC channels available on the board */
+	indio_dev->num_channels = 2;
+	indio_dev->channels = nau7802_chan_array;
+
+	init_completion(&st->value_ok);
+	mutex_init(&st->lock);
+	mutex_init(&st->data_lock);
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "Couldn't register the device.\n");
+		goto error_device_register;
+	}
+
+	return 0;
+
+error_device_register:
+	mutex_destroy(&st->lock);
+	mutex_destroy(&st->data_lock);
+	if (client->irq)
+		free_irq(client->irq, indio_dev);
+	iio_device_free(indio_dev);
+
+	return ret;
+}
+
+static int nau7802_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct nau7802_state *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	mutex_destroy(&st->lock);
+	mutex_destroy(&st->data_lock);
+	if (client->irq)
+		free_irq(client->irq, indio_dev);
+	iio_device_free(indio_dev);
+
+	return 0;
+}
+
+static const struct i2c_device_id nau7802_i2c_id[] = {
+	{ "nau7802", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, nau7802_i2c_id);
+
+static const struct of_device_id nau7802_dt_ids[] = {
+	{ .compatible = "nuvoton,nau7802" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, nau7802_dt_ids);
+
+static struct i2c_driver nau7802_driver = {
+	.probe = nau7802_probe,
+	.remove = nau7802_remove,
+	.id_table = nau7802_i2c_id,
+	.driver = {
+		   .name = "nau7802",
+		   .of_match_table = of_match_ptr(nau7802_dt_ids),
+	},
+};
+
+module_i2c_driver(nau7802_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Nuvoton NAU7802 ADC Driver");
+MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
+MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@free-electrons.com>");
-- 
1.8.1.2

[PATCHv2 2/3] ARM: mxs: cfa10049: Switch bus i2c1 to bitbanging

[Alexandre Belloni]

From: Maxime Ripard <maxime.ripard@free-electrons.com>

The ADCs connected to this bus have been experiencing some timeout
issues when using the iMX28 i2c controller. Switching back to bitbanging
solves this.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 arch/arm/boot/dts/imx28-cfa10049.dts | 108 +++++++++++++++++++++--------------
 1 file changed, 65 insertions(+), 43 deletions(-)

diff --git a/arch/arm/boot/dts/imx28-cfa10049.dts b/arch/arm/boot/dts/imx28-cfa10049.dts
index 05ae549..d3758c2 100644
--- a/arch/arm/boot/dts/imx28-cfa10049.dts
+++ b/arch/arm/boot/dts/imx28-cfa10049.dts
@@ -139,6 +139,17 @@
 					fsl,pull-up = <0>; /* 0 will enable the keeper */
 				};
 
+				i2c1_pins_cfa10049: i2c1@0 {
+					reg = <0>;
+					fsl,pinmux-ids = <
+						0x3103 /* MX28_PAD_PWM0__GPIO */
+						0x3113 /* MX28_PAD_PWM1__I2C1_SDA */
+					>;
+					fsl,drive-strength = <1>;
+					fsl,voltage = <1>;
+					fsl,pull-up = <1>;
+				};
+
 				fiq_pins_cfa10049: fiq@0 {
 					reg = <0>;
 					fsl,pinmux-ids = <
@@ -199,49 +210,6 @@
 				status = "okay";
 			};
 
-			i2c1: i2c@8005a000 {
-				pinctrl-names = "default";
-				pinctrl-0 = <&i2c1_pins_a>;
-				status = "okay";
-			};
-
-			i2cmux {
-				compatible = "i2c-mux-gpio";
-				#address-cells = <1>;
-				#size-cells = <0>;
-				mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
-				i2c-parent = <&i2c1>;
-
-				i2c@0 {
-					reg = <0>;
-				};
-
-				i2c@1 {
-					reg = <1>;
-				};
-
-				i2c@2 {
-					reg = <2>;
-				};
-
-				i2c@3 {
-					reg = <3>;
-					#address-cells = <1>;
-					#size-cells = <0>;
-
-					pca9555: pca9555@20 {
-						compatible = "nxp,pca9555";
-						interrupt-parent = <&gpio2>;
-						interrupts = <19 0x2>;
-						gpio-controller;
-						#gpio-cells = <2>;
-						interrupt-controller;
-						#interrupt-cells = <2>;
-						reg = <0x20>;
-					};
-				};
-			};
-
 			usbphy1: usbphy@8007e000 {
 				status = "okay";
 			};
@@ -366,6 +334,60 @@
 		rotary-encoder,relative-axis;
 	};
 
+	i2c1gpio: i2c@0 {
+		compatible = "i2c-gpio";
+		pinctrl-0 = <&i2c1_pins_cfa10049>;
+		pinctrl-names = "default";
+		gpios = <
+			&gpio3 17 0 /* sda */
+			&gpio3 16 0 /* scl */
+			 >;
+		i2c-gpio,delay-us = <2>;	/* ~100 kHz */
+	};
+
+	i2cmux {
+		compatible = "i2c-mux-gpio";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		mux-gpios = <&gpio1 22 0 &gpio1 23 0>;
+		i2c-parent = <&i2c1gpio>;
+
+		i2c@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c@1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c@2 {
+			reg = <2>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c@3 {
+			reg = <3>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			pca9555: pca9555@20 {
+				compatible = "nxp,pca9555";
+				interrupt-parent = <&gpio2>;
+				interrupts = <19 0x2>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				reg = <0x20>;
+			};
+		};
+	};
+
 	backlight {
 		compatible = "pwm-backlight";
 		pwms = <&pwm 3 5000000>;
-- 
1.8.1.2

[PATCHv2 3/3] ARM: mxs: cfa10049: Add NAU7802 ADCs to the device tree

[Alexandre Belloni]

From: Maxime Ripard <maxime.ripard@free-electrons.com>

Since these ADCs share the same non-configurable address on the I2C bus,
they have to be put behind a muxer.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/imx28-cfa10049.dts | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/imx28-cfa10049.dts b/arch/arm/boot/dts/imx28-cfa10049.dts
index d3758c2..efc4213 100644
--- a/arch/arm/boot/dts/imx28-cfa10049.dts
+++ b/arch/arm/boot/dts/imx28-cfa10049.dts
@@ -356,18 +356,36 @@
 			reg = <0>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+
+			adc0: nau7802@2a {
+				compatible = "nuvoton,nau7802";
+				reg = <0x2a>;
+				nuvoton,vldo = <3000>;
+			};
 		};
 
 		i2c@1 {
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+
+			adc1: nau7802@2a {
+				compatible = "nuvoton,nau7802";
+				reg = <0x2a>;
+				nuvoton,vldo = <3000>;
+			};
 		};
 
 		i2c@2 {
 			reg = <2>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+
+			adc2: nau7802@2a {
+				compatible = "nuvoton,nau7802";
+				reg = <0x2a>;
+				nuvoton,vldo = <3000>;
+			};
 		};
 
 		i2c@3 {
-- 
1.8.1.2

Re: [PATCHv2 2/3] ARM: mxs: cfa10049: Switch bus i2c1 to bitbanging

[Fabio Estevam]

On Thu, Jun 20, 2013 at 3:57 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> From: Maxime Ripard <maxime.ripard@free-electrons.com>
>
> The ADCs connected to this bus have been experiencing some timeout
> issues when using the iMX28 i2c controller. Switching back to bitbanging
> solves this.

Could you please investigate this timeout issue?

The I2C timeouts I was having on mx28 when trying to access sgtl5000
codec, was due to the lack
of codec clock.

After I provided the codec clock I am able to use I2C without issues
and just sent a patch for it.

Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Jonathan Cameron]

On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
> gain and sampling rates.
>
Sorry, somewhat low on time today so only a quick review.

1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
   a little vague?  Not that I have a better idea!

2) A lot of i2c calls could do with error handling.


> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  .../bindings/iio/adc/nuvoton-nau7802.txt           |  17 +
>  drivers/iio/adc/Kconfig                            |   9 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/nau7802.c                          | 603 +++++++++++++++++++++
>  4 files changed, 630 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>  create mode 100644 drivers/iio/adc/nau7802.c
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
> new file mode 100644
> index 0000000..9bc4218
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
> @@ -0,0 +1,17 @@
> +* Nuvoton NAU7802 Analog to Digital Converter (ADC)
> +
> +Required properties:
> +  - compatible: Should be "nuvoton,nau7802"
> +  - reg: Should contain the ADC I2C address
> +
> +Optional properties:
> +  - nuvoton,vldo: Reference voltage in millivolts (integer)
> +  - interrupts: IRQ line for the ADC. If not used the driver will use
> +    polling.
> +
> +Example:
> +adc2: nau7802@2a {
> +	compatible = "nuvoton,nau7802";
> +	reg = <0x2a>;
> +	nuvoton,vldo = <3000>;
> +};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index ab0767e6..d7f9ed8 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -133,6 +133,15 @@ config MAX1363
>  	  max11646, max11647) Provides direct access via sysfs and buffered
>  	  data via the iio dev interface.
>
> +config NAU7802
> +	tristate "Nuvoton NAU7802 ADC driver"
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Nuvoton NAU7802 ADC.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called nau7802.
> +
>  config TI_ADC081C
>  	tristate "Texas Instruments ADC081C021/027"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 0a825be..d426081 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_AT91_ADC) += at91_adc.o
>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> +obj-$(CONFIG_NAU7802) += nau7802.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>  obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
> diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
> new file mode 100644
> index 0000000..e1b6981
> --- /dev/null
> +++ b/drivers/iio/adc/nau7802.c
> @@ -0,0 +1,603 @@
> +/*
> + * Driver for the Nuvoton NAU7802 ADC
> + *
> + * Copyright 2013 Free Electrons
> + *
> + * Licensed under the GPLv2 or later.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/wait.h>
> +#include <linux/log2.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define NAU7802_REG_PUCTRL	0x00
> +#define NAU7802_PUCTRL_RR(x)		(x << 0)
> +#define NAU7802_PUCTRL_RR_BIT		NAU7802_PUCTRL_RR(1)
> +#define NAU7802_PUCTRL_PUD(x)		(x << 1)
> +#define NAU7802_PUCTRL_PUD_BIT		NAU7802_PUCTRL_PUD(1)
> +#define NAU7802_PUCTRL_PUA(x)		(x << 2)
> +#define NAU7802_PUCTRL_PUA_BIT		NAU7802_PUCTRL_PUA(1)
> +#define NAU7802_PUCTRL_PUR(x)		(x << 3)
> +#define NAU7802_PUCTRL_PUR_BIT		NAU7802_PUCTRL_PUR(1)
> +#define NAU7802_PUCTRL_CS(x)		(x << 4)
> +#define NAU7802_PUCTRL_CS_BIT		NAU7802_PUCTRL_CS(1)
> +#define NAU7802_PUCTRL_CR(x)		(x << 5)
> +#define NAU7802_PUCTRL_CR_BIT		NAU7802_PUCTRL_CR(1)
> +#define NAU7802_PUCTRL_AVDDS(x)		(x << 7)
> +#define NAU7802_PUCTRL_AVDDS_BIT	NAU7802_PUCTRL_AVDDS(1)
> +#define NAU7802_REG_CTRL1	0x01
> +#define NAU7802_CTRL1_VLDO(x)		(x << 3)
> +#define NAU7802_CTRL1_GAINS(x)		(x)
> +#define NAU7802_CTRL1_GAINS_BITS	0x07
> +#define NAU7802_REG_CTRL2	0x02
> +#define NAU7802_CTRL2_CHS(x)		(x << 7)
> +#define NAU7802_CTRL2_CRS(x)		(x << 4)
> +#define NAU7802_SAMP_FREQ_320	0x07
> +#define NAU7802_CTRL2_CHS_BIT		NAU7802_CTRL2_CHS(1)
> +#define NAU7802_REG_ADC_B2	0x12
> +#define NAU7802_REG_ADC_B1	0x13
> +#define NAU7802_REG_ADC_B0	0x14
> +#define NAU7802_REG_ADC_CTRL	0x15
> +
> +#define NAU7802_DEFAULT_CONVERSIONS 6
> +
> +struct nau7802_state {
> +	struct i2c_client	*client;
> +	s32			last_value;
> +	struct mutex		lock;
> +	struct mutex		data_lock;
> +	u32			vref_mv;
> +	u32			conversion_count;
> +	u32			min_conversions;
> +	u8			sample_rate;
> +	u32			scale_avail[8];
> +	struct completion	value_ok;
> +};
> +
> +#define NAU7802_CHANNEL(chan) {					\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = (chan),					\
> +	.scan_index = (chan),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ)	\
> +}
> +
> +static const struct iio_chan_spec nau7802_chan_array[] = {
> +	NAU7802_CHANNEL(0),
> +	NAU7802_CHANNEL(1),
> +};
> +
> +static const u16 nau7802_sample_freq_avail[] = {10, 20, 40, 80,
> +						10, 10, 10, 320};
> +
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("10 40 80 320");
> +
> +static ssize_t nau7802_sysfs_set_min_conversions(struct device *dev,
> +		struct device_attribute *attr,
> +		const char *buf,
> +		size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct nau7802_state *st = iio_priv(indio_dev);
> +	u32 val;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &val);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&st->lock);
> +	st->min_conversions = val;
> +	st->conversion_count = 0;
> +	mutex_unlock(&st->lock);
> +
> +	return len;
> +}
> +
> +static ssize_t nau7802_sysfs_get_min_conversions(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct nau7802_state *st = iio_priv(indio_dev);
> +
> +	return sprintf(buf, "%d\n", st->min_conversions);
> +}
> +
> +static IIO_DEVICE_ATTR(min_conversions, S_IWUSR | S_IRUGO,
> +		nau7802_sysfs_get_min_conversions,
> +		nau7802_sysfs_set_min_conversions, 0);
> +
> +static struct attribute *nau7802_attributes[] = {
> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> +	&iio_dev_attr_min_conversions.dev_attr.attr,
A new userspace abi element so it needs documented under
Documentaiton/ABI/testing/
> +	NULL
> +};
> +
> +static const struct attribute_group nau7802_attribute_group = {
> +	.attrs = nau7802_attributes,
> +};
> +
> +static int nau7802_set_gain(struct nau7802_state *st, int gain)
> +{
> +	u8 data;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	st->conversion_count = 0;
> +
> +	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL1);
> +	if (data < 0)
> +		goto nau7802_sysfs_set_gain_out;
> +	ret = i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL1,
> +					(data & (~NAU7802_CTRL1_GAINS_BITS)) |
> +					gain);
> +
> +nau7802_sysfs_set_gain_out:
> +	mutex_unlock(&st->lock);
> +
> +	return ret ? ret : 0;
> +}
> +
> +static int nau7802_read_conversion(struct nau7802_state *st)
> +{
> +	u8 data;
> +
> +	mutex_lock(&st->data_lock);
> +	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_ADC_B2);
> +	if (data < 0)
> +		goto nau7802_read_conversion_out;
> +	st->last_value = data << 16;
> +
> +	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_ADC_B1);
> +	if (data < 0)
> +		goto nau7802_read_conversion_out;
> +	st->last_value |= data << 8;
> +
> +	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_ADC_B0);
> +	if (data < 0)
> +		goto nau7802_read_conversion_out;
> +	st->last_value |= data;
> +
> +	st->last_value = sign_extend32(st->last_value, 23);
> +
> +nau7802_read_conversion_out:
> +	mutex_unlock(&st->data_lock);
> +
> +	return data;
> +}
> +
> +/*
> + * Conversions are synchronised on the rising edge of NAU7802_PUCTRL_CS_BIT
> + */
> +static int nau7802_sync(struct nau7802_state *st)
> +{
> +	int ret;
> +	u8 data;
> +
> +	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_PUCTRL);
> +	if (data < 0)
> +		return data;
> +	ret = i2c_smbus_write_byte_data(st->client, NAU7802_REG_PUCTRL,
> +				data | NAU7802_PUCTRL_CS_BIT);
> +
> +	return ret;
> +}
> +
> +static irqreturn_t nau7802_eoc_trigger(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct nau7802_state *st = iio_priv(indio_dev);
> +	u8 status;
> +
> +	status = i2c_smbus_read_byte_data(st->client, NAU7802_REG_PUCTRL);
> +	if (status < 0)
> +		return IRQ_HANDLED;
> +
> +	if (!(status & NAU7802_PUCTRL_CR_BIT))
> +		return IRQ_NONE;
> +
> +	if (nau7802_read_conversion(st) < 0)
> +		return IRQ_HANDLED;
> +
> +	/* Because there is actually only one ADC for both channels, we have to
> +	 * wait for enough conversions to happen before getting a significant
> +	 * value when changing channels and the values are far appart.
> +	 */
> +	if (st->conversion_count < st->min_conversions)
> +		st->conversion_count++;
> +	if (st->conversion_count >= st->min_conversions)
> +		complete_all(&st->value_ok);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int nau7802_read_irq(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int *val)
> +{
> +	struct nau7802_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	INIT_COMPLETION(st->value_ok);
> +	enable_irq(st->client->irq);
> +
> +	nau7802_sync(st);
> +
> +	/* read registers to ensure we flush everything */
> +	ret = nau7802_read_conversion(st);
> +	if (ret < 0)
> +		goto read_chan_info_failure;
> +
> +	/* Wait for a conversion to finish */
> +	ret = wait_for_completion_interruptible_timeout(&st->value_ok,
> +			msecs_to_jiffies(1000));
> +	if (ret == 0)
> +		ret = -ETIMEDOUT;
> +
> +	if (ret < 0)
> +		goto read_chan_info_failure;
> +
> +	disable_irq(st->client->irq);
> +
> +	*val = st->last_value;
> +
> +	return IIO_VAL_INT;
> +
> +read_chan_info_failure:
> +	disable_irq(st->client->irq);
> +
> +	return ret;
> +}
> +
> +static int nau7802_read_poll(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int *val)
> +{
> +	struct nau7802_state *st = iio_priv(indio_dev);
> +	int ret;
> +	u8 data;
> +
> +	nau7802_sync(st);
> +
> +	/* read registers to ensure we flush everything */
> +	ret = nau7802_read_conversion(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Because there is actually only one ADC for both channels, we have to
> +	 * wait for enough conversions to happen before getting a significant
> +	 * value when changing channels and the values are far appart.
apart
> +	 */
> +	do {
> +		data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_PUCTRL);
> +		if (data < 0)
> +			return data;
> +
> +		while (!(data & NAU7802_PUCTRL_CR_BIT)) {
> +			if (st->sample_rate != NAU7802_SAMP_FREQ_320)
> +				msleep(20);
> +			else
> +				mdelay(4);
> +			data = i2c_smbus_read_byte_data(st->client,
> +							NAU7802_REG_PUCTRL);
> +			if (data < 0)
> +				return data;
> +		}
> +
> +		nau7802_read_conversion(st);
> +		if (ret < 0)
> +			return ret;
> +		if (st->conversion_count < st->min_conversions)
> +			st->conversion_count++;
> +	} while (st->conversion_count < st->min_conversions);
> +
> +	*val = st->last_value;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int nau7802_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct nau7802_state *st = iio_priv(indio_dev);
> +	u8 data;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&st->lock);
> +		/*
> +		 * Select the channel to use
> +		 *   - Channel 1 is value 0 in the CHS register
> +		 *   - Channel 2 is value 1 in the CHS register
> +		 */
> +		data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL2);
> +		if (data < 0)
> +			return data;
> +
> +		if (((data & NAU7802_CTRL2_CHS_BIT) && !chan->channel) ||
> +				(!(data & NAU7802_CTRL2_CHS_BIT) &&
> +				 chan->channel)) {
> +			st->conversion_count = 0;
> +			ret = i2c_smbus_write_byte_data(st->client,
> +					NAU7802_REG_CTRL2,
> +					NAU7802_CTRL2_CHS(chan->channel) |
> +					NAU7802_CTRL2_CRS(st->sample_rate));
> +
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		if (st->client->irq)
> +			ret = nau7802_read_irq(indio_dev, chan, val);
> +		else
> +			ret = nau7802_read_poll(indio_dev, chan, val);
> +
> +		mutex_unlock(&st->lock);
> +		return ret;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL1);
> +		if (data < 0)
> +			return data;
> +
> +		/* we have 24 bits of signed data, that means 23 bits of data
> +		 * plus the sign bit */
> +		*val = st->vref_mv;
> +		*val2 = 23 + (data & NAU7802_CTRL1_GAINS_BITS);
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val =  nau7802_sample_freq_avail[st->sample_rate];
> +		*val2 = 0;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int nau7802_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct nau7802_state *st = iio_priv(indio_dev);
> +	int i;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++)
> +			if (val2 == st->scale_avail[i])
> +				return nau7802_set_gain(st, i);
> +
> +		break;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		for (i = 0; i < ARRAY_SIZE(nau7802_sample_freq_avail); i++)
> +			if (val == nau7802_sample_freq_avail[i]) {
> +				mutex_lock(&st->lock);
> +				st->sample_rate = i;
> +				st->conversion_count = 0;
Error handling for the i2c call?
> +				i2c_smbus_write_byte_data(st->client,
> +					NAU7802_REG_CTRL2,
> +					NAU7802_CTRL2_CRS(st->sample_rate));
> +				mutex_unlock(&st->lock);
> +				return 0;
> +			}
> +
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int nau7802_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				     struct iio_chan_spec const *chan,
> +				     long mask)
> +{
> +	return IIO_VAL_INT_PLUS_NANO;
> +}
> +
> +static const struct iio_info nau7802_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = &nau7802_read_raw,
> +	.write_raw = &nau7802_write_raw,
> +	.write_raw_get_fmt = nau7802_write_raw_get_fmt,
> +	.attrs = &nau7802_attribute_group,
> +};
> +
> +static int nau7802_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct iio_dev *indio_dev;
> +	struct nau7802_state *st;
> +	struct device_node *np = client->dev.of_node;
> +	int i, ret;
> +	u8 data;
> +	u32 tmp;
> +
> +	if (!client->dev.of_node) {
> +		dev_err(&client->dev, "No device tree node available.\n");
> +		return -EINVAL;
> +	}
> +
> +	indio_dev = iio_device_alloc(sizeof(*st));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->name = dev_name(&client->dev);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &nau7802_info;
> +
> +	st->client = client;
> +
> +	/* Reset the device */
As a general rule we'd expect any errors returned by these functions
to be checked and handled.
> +	i2c_smbus_write_byte_data(st->client, NAU7802_REG_PUCTRL,
> +				  NAU7802_PUCTRL_RR_BIT);
> +
> +	/* Enter normal operation mode */
> +	i2c_smbus_write_byte_data(st->client, NAU7802_REG_PUCTRL,
> +				  NAU7802_PUCTRL_PUD_BIT);
> +
> +	/*
> +	 * After about 200 usecs, the device should be ready and then
> +	 * the Power Up bit will be set to 1. If not, wait for it.
> +	 */
> +	udelay(210);
> +	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_PUCTRL);
> +	if (data < 0)
> +		return -ENODEV;
> +	if (!(data & NAU7802_PUCTRL_PUR_BIT))
> +		return -ENODEV;
> +
> +	of_property_read_u32(np, "nuvoton,vldo", &tmp);
> +	st->vref_mv = tmp;
> +
> +	/* Populate available ADC input ranges */
> +	for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++)
> +		st->scale_avail[i] = (((u64)st->vref_mv) * 1000000000ULL)
> +					   >> (23 + i);
> +
> +	data = NAU7802_PUCTRL_PUD_BIT | NAU7802_PUCTRL_PUA_BIT |
> +		NAU7802_PUCTRL_CS_BIT;
> +	if (tmp >= 2400)
> +		data |= NAU7802_PUCTRL_AVDDS_BIT;
> +
> +	i2c_smbus_write_byte_data(st->client, NAU7802_REG_PUCTRL, data);
> +	i2c_smbus_write_byte_data(st->client, NAU7802_REG_ADC_CTRL, 0x30);
> +
> +	if (tmp >= 2400) {
> +		data = NAU7802_CTRL1_VLDO((4500 - tmp) / 300);
> +		i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL1, data);
> +	}
> +
> +	st->min_conversions = NAU7802_DEFAULT_CONVERSIONS;
> +
> +	/*
> +	 * The ADC fires continuously and we can't do anything about
> +	 * it. So we need to have the IRQ disabled by default, and we
> +	 * will enable them back when we will need them..
> +	 */
> +	if (client->irq) {
> +		ret = request_threaded_irq(client->irq,
> +				NULL,
> +				nau7802_eoc_trigger,
> +				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +				client->dev.driver->name,
> +				indio_dev);
> +		if (ret) {
> +			/*
> +			 * What may happen here is that our IRQ controller is
> +			 * not able to get level interrupt but this is required
> +			 * by this ADC as when going over 40 sample per second,
> +			 * the interrupt line may stay high between conversions.
> +			 * So, we continue no matter what but we switch to
> +			 * polling mode.
> +			 */
> +			dev_info(&client->dev,
> +				"Failed to allocate IRQ, using polling mode\n");
> +			client->irq = 0;
> +		} else
> +			disable_irq(client->irq);
> +	}
> +
> +	if (!client->irq) {
> +		/*
> +		 * We are polling, use the fastest sample rate by
> +		 * default
> +		 */
> +		st->sample_rate = NAU7802_SAMP_FREQ_320;
> +		i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL2,
> +					  NAU7802_CTRL2_CRS(st->sample_rate));
> +	}
> +
> +	/* Setup the ADC channels available on the board */
> +	indio_dev->num_channels = 2;
> +	indio_dev->channels = nau7802_chan_array;
> +
> +	init_completion(&st->value_ok);
> +	mutex_init(&st->lock);
> +	mutex_init(&st->data_lock);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Couldn't register the device.\n");
> +		goto error_device_register;
> +	}
> +
> +	return 0;
> +
> +error_device_register:
> +	mutex_destroy(&st->lock);
> +	mutex_destroy(&st->data_lock);
> +	if (client->irq)
> +		free_irq(client->irq, indio_dev);
> +	iio_device_free(indio_dev);
> +
> +	return ret;
> +}
> +
> +static int nau7802_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct nau7802_state *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	mutex_destroy(&st->lock);
> +	mutex_destroy(&st->data_lock);
> +	if (client->irq)
> +		free_irq(client->irq, indio_dev);
> +	iio_device_free(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id nau7802_i2c_id[] = {
> +	{ "nau7802", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, nau7802_i2c_id);
> +
> +static const struct of_device_id nau7802_dt_ids[] = {
> +	{ .compatible = "nuvoton,nau7802" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, nau7802_dt_ids);
> +
> +static struct i2c_driver nau7802_driver = {
> +	.probe = nau7802_probe,
> +	.remove = nau7802_remove,
> +	.id_table = nau7802_i2c_id,
> +	.driver = {
> +		   .name = "nau7802",
> +		   .of_match_table = of_match_ptr(nau7802_dt_ids),
> +	},
> +};
> +
> +module_i2c_driver(nau7802_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Nuvoton NAU7802 ADC Driver");
> +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
> +MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@free-electrons.com>");
>

Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Lars-Peter Clausen]

On 06/22/2013 01:55 PM, Jonathan Cameron wrote:
> On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
>> gain and sampling rates.
>>
> Sorry, somewhat low on time today so only a quick review.
> 
> 1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
>    a little vague?  Not that I have a better idea!

I really don't like the name min_conversions either. Isn't this effectively
a decimation filter?

- Lars

Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Alexandre Belloni]

On 22/06/2013 14:02, Lars-Peter Clausen wrote:
> On 06/22/2013 01:55 PM, Jonathan Cameron wrote:
>> On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
>>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
>>> gain and sampling rates.
>>>
>> Sorry, somewhat low on time today so only a quick review.
>>
>> 1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
>>    a little vague?  Not that I have a better idea!
> I really don't like the name min_conversions either. Isn't this effectively
> a decimation filter?

Yeah, it could be seen like that but it is only relevant and only
happens when switching between channels. I'm open to any ideas.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Lars-Peter Clausen]

On 06/22/2013 03:07 PM, Alexandre Belloni wrote:
> On 22/06/2013 14:02, Lars-Peter Clausen wrote:
>> On 06/22/2013 01:55 PM, Jonathan Cameron wrote:
>>> On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
>>>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
>>>> gain and sampling rates.
>>>>
>>> Sorry, somewhat low on time today so only a quick review.
>>>
>>> 1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
>>>    a little vague?  Not that I have a better idea!
>> I really don't like the name min_conversions either. Isn't this effectively
>> a decimation filter?
> 
> Yeah, it could be seen like that but it is only relevant and only
> happens when switching between channels. I'm open to any ideas.
> 

I see. Is there anything about this in the datasheet on how many conversions
you usually need? Is this really something you need to change at runtime or
does moving this to platform data work?

- Lars

Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Alexandre Belloni]

On 22/06/2013 15:20, Lars-Peter Clausen wrote:
> On 06/22/2013 03:07 PM, Alexandre Belloni wrote:
>> On 22/06/2013 14:02, Lars-Peter Clausen wrote:
>>> On 06/22/2013 01:55 PM, Jonathan Cameron wrote:
>>>> On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
>>>>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
>>>>> gain and sampling rates.
>>>>>
>>>> Sorry, somewhat low on time today so only a quick review.
>>>>
>>>> 1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
>>>>    a little vague?  Not that I have a better idea!
>>> I really don't like the name min_conversions either. Isn't this effectively
>>> a decimation filter?
>> Yeah, it could be seen like that but it is only relevant and only
>> happens when switching between channels. I'm open to any ideas.
>>
> I see. Is there anything about this in the datasheet on how many conversions
> you usually need? Is this really something you need to change at runtime or
> does moving this to platform data work?
>
>

There is actually nothing in the datasheet. The default value (6
conversions) was found experimentally. What I did was saturating the ADC
with the higher value on one channel and the lower value on the other
one and I tried to find when reading both channel sequentially was
resulting in a correct value.

You may not need to change it at runtime. And that value mainly depend
on the precision versus speed balance you want to achieve. If you know
that the values on both channels will not be to far apart, then you may
not need to wait at all.

Would you think that is something I should hide in the DT ? Or maybe I
can drop that knob for now and see if it is needed in the future.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Lars-Peter Clausen]

On 06/22/2013 03:28 PM, Alexandre Belloni wrote:
> On 22/06/2013 15:20, Lars-Peter Clausen wrote:
>> On 06/22/2013 03:07 PM, Alexandre Belloni wrote:
>>> On 22/06/2013 14:02, Lars-Peter Clausen wrote:
>>>> On 06/22/2013 01:55 PM, Jonathan Cameron wrote:
>>>>> On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
>>>>>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
>>>>>> gain and sampling rates.
>>>>>>
>>>>> Sorry, somewhat low on time today so only a quick review.
>>>>>
>>>>> 1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
>>>>>    a little vague?  Not that I have a better idea!
>>>> I really don't like the name min_conversions either. Isn't this effectively
>>>> a decimation filter?
>>> Yeah, it could be seen like that but it is only relevant and only
>>> happens when switching between channels. I'm open to any ideas.
>>>
>> I see. Is there anything about this in the datasheet on how many conversions
>> you usually need? Is this really something you need to change at runtime or
>> does moving this to platform data work?
>>
>>
> 
> There is actually nothing in the datasheet. The default value (6
> conversions) was found experimentally. What I did was saturating the ADC
> with the higher value on one channel and the lower value on the other
> one and I tried to find when reading both channel sequentially was
> resulting in a correct value.
> 
> You may not need to change it at runtime. And that value mainly depend
> on the precision versus speed balance you want to achieve. If you know
> that the values on both channels will not be to far apart, then you may
> not need to wait at all.
> 
> Would you think that is something I should hide in the DT ? Or maybe I
> can drop that knob for now and see if it is needed in the future.
> 

It is always a good idea to be conservative when introducing new ABI, so if
you think we can get away with hardcoding this in the driver I think that's
a good idea.

- Lars

Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Jonathan Cameron]

On 06/23/2013 02:54 PM, Lars-Peter Clausen wrote:
> On 06/22/2013 03:28 PM, Alexandre Belloni wrote:
>> On 22/06/2013 15:20, Lars-Peter Clausen wrote:
>>> On 06/22/2013 03:07 PM, Alexandre Belloni wrote:
>>>> On 22/06/2013 14:02, Lars-Peter Clausen wrote:
>>>>> On 06/22/2013 01:55 PM, Jonathan Cameron wrote:
>>>>>> On 06/20/2013 07:57 PM, Alexandre Belloni wrote:
>>>>>>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
>>>>>>> gain and sampling rates.
>>>>>>>
>>>>>> Sorry, somewhat low on time today so only a quick review.
>>>>>>
>>>>>> 1) Missing userspace ABI documentation.  Also, perhaps min_conversions is
>>>>>>    a little vague?  Not that I have a better idea!
>>>>> I really don't like the name min_conversions either. Isn't this effectively
>>>>> a decimation filter?
>>>> Yeah, it could be seen like that but it is only relevant and only
>>>> happens when switching between channels. I'm open to any ideas.
>>>>
>>> I see. Is there anything about this in the datasheet on how many conversions
>>> you usually need? Is this really something you need to change at runtime or
>>> does moving this to platform data work?
>>>
>>>
>>
>> There is actually nothing in the datasheet. The default value (6
>> conversions) was found experimentally. What I did was saturating the ADC
>> with the higher value on one channel and the lower value on the other
>> one and I tried to find when reading both channel sequentially was
>> resulting in a correct value.
>>
>> You may not need to change it at runtime. And that value mainly depend
>> on the precision versus speed balance you want to achieve. If you know
>> that the values on both channels will not be to far apart, then you may
>> not need to wait at all.
>>
>> Would you think that is something I should hide in the DT ? Or maybe I
>> can drop that knob for now and see if it is needed in the future.
>>
> 
> It is always a good idea to be conservative when introducing new ABI, so if
> you think we can get away with hardcoding this in the driver I think that's
> a good idea.
> 
I agree entirely.  We can always make it controllable later as long
as we keep the default the same as the value you hard code in now.

Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Lars-Peter Clausen]

On 06/20/2013 08:57 PM, Alexandre Belloni wrote:
> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
> gain and sampling rates.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  .../bindings/iio/adc/nuvoton-nau7802.txt           |  17 +
>  drivers/iio/adc/Kconfig                            |   9 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/nau7802.c                          | 603 +++++++++++++++++++++
>  4 files changed, 630 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>  create mode 100644 drivers/iio/adc/nau7802.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
> new file mode 100644
> index 0000000..9bc4218
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
> @@ -0,0 +1,17 @@
> +* Nuvoton NAU7802 Analog to Digital Converter (ADC)use
> +
> +Required properties:
> +  - compatible: Should be "nuvoton,nau7802"
> +  - reg: Should contain the ADC I2C address
> +
> +Optional properties:
> +  - nuvoton,vldo: Reference voltage in millivolts (integer)
> +  - interrupts: IRQ line for the ADC. If not used the driver will use
> +    polling.
> +
> +Example:
> +adc2: nau7802@2a {
> +	compatible = "nuvoton,nau7802";
> +	reg = <0x2a>;
> +	nuvoton,vldo = <3000>;

We usually use the regulator framework for specifying the reference voltage.

> +};
[...]
> diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
> new file mode 100644
> index 0000000..e1b6981
> --- /dev/null
> +++ b/drivers/iio/adc/nau7802.c
> @@ -0,0 +1,603 @@
[...]
> +static int nau7802_set_gain(struct nau7802_state *st, int gain)
> +{
> +	u8 data;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	st->conversion_count = 0;
> +
> +	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL1);
> +	if (data < 0)
> +		goto nau7802_sysfs_set_gain_out;

ret will be uninitialized if the goto above is taken

> +	ret = i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL1,
> +					(data & (~NAU7802_CTRL1_GAINS_BITS)) |
> +					gain);
> +
> +nau7802_sysfs_set_gain_out:
> +	mutex_unlock(&st->lock);
> +
> +	return ret ? ret : 0;
> +}
[...]
> +static int nau7802_read_irq(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int *val)
> +{
> +	struct nau7802_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	INIT_COMPLETION(st->value_ok);
> +	enable_irq(st->client->irq);

Is it really necessary to enable/disable the IRQ or could you keep it
enabled all the time?

> +
> +	nau7802_sync(st);
> +
> +	/* read registers to ensure we flush everything */
> +	ret = nau7802_read_conversion(st);
> +	if (ret < 0)
> +		goto read_chan_info_failure;
> +
> +	/* Wait for a conversion to finish */
> +	ret = wait_for_completion_interruptible_timeout(&st->value_ok,
> +			msecs_to_jiffies(1000));
> +	if (ret == 0)
> +		ret = -ETIMEDOUT;
> +
> +	if (ret < 0)
> +		goto read_chan_info_failure;
> +
> +	disable_irq(st->client->irq);
> +
> +	*val = st->last_value;
> +
> +	return IIO_VAL_INT;
> +
> +read_chan_info_failure:
> +	disable_irq(st->client->irq);
> +
> +	return ret;
> +}
[...]

[...]
> +static int nau7802_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct iio_dev *indio_dev;
> +	struct nau7802_state *st;
> +	struct device_node *np = client->dev.of_node;
> +	int i, ret;
> +	u8 data;
> +	u32 tmp;
> +
> +	if (!client->dev.of_node) {
> +		dev_err(&client->dev, "No device tree node available.\n");
> +		return -EINVAL;
> +	}

Except for getting the vref the is no direct dependency on devicetree, if
you switch to the regulator framework for the vref this check can be removed.

[...]
> +	/* Setup the ADC channels available on the board */
> +	indio_dev->num_channels = 2;

ARRAY_SIZE(nau7802_chan_array)

> +	indio_dev->channels = nau7802_chan_array;
> +
> +	init_completion(&st->value_ok);

You need to initialize the completion before requesting the IRQ handler.

[...]
> +}

Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Alexandre Belloni]

On 24/06/2013 08:41, Lars-Peter Clausen wrote:
> On 06/20/2013 08:57 PM, Alexandre Belloni wrote:
>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
>> gain and sampling rates.
>>
>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> ---
>>  .../bindings/iio/adc/nuvoton-nau7802.txt           |  17 +
>>  drivers/iio/adc/Kconfig                            |   9 +
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/nau7802.c                          | 603 +++++++++++++++++++++
>>  4 files changed, 630 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>>  create mode 100644 drivers/iio/adc/nau7802.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>> new file mode 100644
>> index 0000000..9bc4218
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>> @@ -0,0 +1,17 @@
>> +* Nuvoton NAU7802 Analog to Digital Converter (ADC)use
>> +
>> +Required properties:
>> +  - compatible: Should be "nuvoton,nau7802"
>> +  - reg: Should contain the ADC I2C address
>> +
>> +Optional properties:
>> +  - nuvoton,vldo: Reference voltage in millivolts (integer)
>> +  - interrupts: IRQ line for the ADC. If not used the driver will use
>> +    polling.
>> +
>> +Example:
>> +adc2: nau7802@2a {
>> +	compatible = "nuvoton,nau7802";
>> +	reg = <0x2a>;
>> +	nuvoton,vldo = <3000>;
> We usually use the regulator framework for specifying the reference voltage.

I followed what Jonathan said in his review of my first patch. Do we
want to use the regulator framework to set the internal reference
voltage of the ADC ? I agree that if you supply an external voltage, it
will be necessary to use the regulator framework. Unfortunately, I can't
test that here.

>> +};
> [...]
>> diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
>> new file mode 100644
>> index 0000000..e1b6981
>> --- /dev/null
>> +++ b/drivers/iio/adc/nau7802.c
>> @@ -0,0 +1,603 @@
> [...]
>> +static int nau7802_set_gain(struct nau7802_state *st, int gain)
>> +{
>> +	u8 data;
>> +	int ret;
>> +
>> +	mutex_lock(&st->lock);
>> +	st->conversion_count = 0;
>> +
>> +	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL1);
>> +	if (data < 0)
>> +		goto nau7802_sysfs_set_gain_out;
> ret will be uninitialized if the goto above is taken

Right, bigger issue, data is u8 so it will never be negative. I'm fixing
that !


>> +	ret = i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL1,
>> +					(data & (~NAU7802_CTRL1_GAINS_BITS)) |
>> +					gain);
>> +
>> +nau7802_sysfs_set_gain_out:
>> +	mutex_unlock(&st->lock);
>> +
>> +	return ret ? ret : 0;
>> +}
> [...]
>> +static int nau7802_read_irq(struct iio_dev *indio_dev,
>> +			struct iio_chan_spec const *chan,
>> +			int *val)
>> +{
>> +	struct nau7802_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	INIT_COMPLETION(st->value_ok);
>> +	enable_irq(st->client->irq);
> Is it really necessary to enable/disable the IRQ or could you keep it
> enabled all the time?

Fact is that the ADC doesn't really care if you are reading data or not
so you will probably endd up in a situation were you will get 320 IRQ
per second but not caring about the result. We have 3 ADCs on the board.
so that amounts to 960 IRQ per second when we are only reading like once
par second !


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Lars-Peter Clausen]

On 06/24/2013 12:37 PM, Alexandre Belloni wrote:
> On 24/06/2013 08:41, Lars-Peter Clausen wrote:
>> On 06/20/2013 08:57 PM, Alexandre Belloni wrote:
>>> The Nuvoton NAU7802 ADC is a 24-bit 2-channels I2C ADC, with adjustable
>>> gain and sampling rates.
>>>
>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> ---
>>>  .../bindings/iio/adc/nuvoton-nau7802.txt           |  17 +
>>>  drivers/iio/adc/Kconfig                            |   9 +
>>>  drivers/iio/adc/Makefile                           |   1 +
>>>  drivers/iio/adc/nau7802.c                          | 603 +++++++++++++++++++++
>>>  4 files changed, 630 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>>>  create mode 100644 drivers/iio/adc/nau7802.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>>> new file mode 100644
>>> index 0000000..9bc4218
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/nuvoton-nau7802.txt
>>> @@ -0,0 +1,17 @@
>>> +* Nuvoton NAU7802 Analog to Digital Converter (ADC)use
>>> +
>>> +Required properties:
>>> +  - compatible: Should be "nuvoton,nau7802"
>>> +  - reg: Should contain the ADC I2C address
>>> +
>>> +Optional properties:
>>> +  - nuvoton,vldo: Reference voltage in millivolts (integer)
>>> +  - interrupts: IRQ line for the ADC. If not used the driver will use
>>> +    polling.
>>> +
>>> +Example:
>>> +adc2: nau7802@2a {
>>> +	compatible = "nuvoton,nau7802";
>>> +	reg = <0x2a>;
>>> +	nuvoton,vldo = <3000>;
>> We usually use the regulator framework for specifying the reference voltage.
> 
> I followed what Jonathan said in his review of my first patch. Do we
> want to use the regulator framework to set the internal reference
> voltage of the ADC ? I agree that if you supply an external voltage, it
> will be necessary to use the regulator framework. Unfortunately, I can't
> test that here.
>

Ah, ok I missed that it is an internally generated voltage. It might makes
sense to add that to the properties documentation.

I guess ideally you'd also register a regulator for the internal regulator
and then use that. But I think that will unnecessarily complicate the code,
so I guess the current solution is fine.

There is one bug in probe though, if nuvoton,vldo is not set tmp will remain
uninitialized.


>>> +};
>> [...]
>>> diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
>>> new file mode 100644
>>> index 0000000..e1b6981
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/nau7802.c
>>> @@ -0,0 +1,603 @@
>> [...]
>>> +static int nau7802_set_gain(struct nau7802_state *st, int gain)
>>> +{
>>> +	u8 data;
>>> +	int ret;
>>> +
>>> +	mutex_lock(&st->lock);
>>> +	st->conversion_count = 0;
>>> +
>>> +	data = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL1);
>>> +	if (data < 0)
>>> +		goto nau7802_sysfs_set_gain_out;
>> ret will be uninitialized if the goto above is taken
> 
> Right, bigger issue, data is u8 so it will never be negative. I'm fixing
> that !
> 
> 
>>> +	ret = i2c_smbus_write_byte_data(st->client, NAU7802_REG_CTRL1,
>>> +					(data & (~NAU7802_CTRL1_GAINS_BITS)) |
>>> +					gain);
>>> +
>>> +nau7802_sysfs_set_gain_out:
>>> +	mutex_unlock(&st->lock);
>>> +
>>> +	return ret ? ret : 0;
>>> +}
>> [...]
>>> +static int nau7802_read_irq(struct iio_dev *indio_dev,
>>> +			struct iio_chan_spec const *chan,
>>> +			int *val)
>>> +{
>>> +	struct nau7802_state *st = iio_priv(indio_dev);
>>> +	int ret;
>>> +
>>> +	INIT_COMPLETION(st->value_ok);
>>> +	enable_irq(st->client->irq);
>> Is it really necessary to enable/disable the IRQ or could you keep it
>> enabled all the time?
> 
> Fact is that the ADC doesn't really care if you are reading data or not
> so you will probably endd up in a situation were you will get 320 IRQ
> per second but not caring about the result. We have 3 ADCs on the board.
> so that amounts to 960 IRQ per second when we are only reading like once
> par second !

Ah, ok, makes sense.

Re: [PATCHv2 1/3] iio: Add Nuvoton NAU7802 ADC driver

[Alexandre Belloni]

On 24/06/2013 18:41, Lars-Peter Clausen wrote:
> Ah, ok, makes sense.
>

I just sent v3, thanks for your reviews !

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com