Skip to contentSkip to conversation

Add support for adc101c* and adc121c*

PATCH Superseded Crestez Dan Leonard 6 participants 12 messages Mar 31, 2016 – Apr 5, 2016 IIO

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

cover Add support for adc101c* and adc121c*

This adds support for adc101c* and adc121c* using the ti-adc081c driver.

I send just the first patch earlier but got no response. The first patch now no longer sets .data in of_device_id because that was not actually used.

The second patch adds buffer support based on external trigger.

Crestez Dan Leonard (2):
ti-adc081c: Add support for adc101c* and adc121c*
ti-adc081c: Initial triggered buffer support

 drivers/iio/adc/Kconfig      |   6 +--
 drivers/iio/adc/ti-adc081c.c | 116 ++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 106 insertions(+), 16 deletions(-)
Signature · 1 line
2.8.0.rc3
#1 lore raw

PATCH 1/2 ti-adc081c: Add support for adc101c* and adc121c*

These chips has an almost identical interface but a deal with a different number of value bits. Datasheet links for comparison:

* http://www.ti.com/lit/ds/symlink/adc081c021.pdf
* http://www.ti.com/lit/ds/symlink/adc101c021.pdf
* http://www.ti.com/lit/ds/symlink/adc121c021.pdf

---
 drivers/iio/adc/Kconfig      |  6 +++---
 drivers/iio/adc/ti-adc081c.c | 31 +++++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 7 deletions(-)
drivers/iio/adc/Kconfig +3 -3
index 9ddcd5d..a2d0db5 100644
@@ -378,11 +378,11 @@ config ROCKCHIP_SARADC
 	  module will be called rockchip_saradc.
 
 config TI_ADC081C
-	tristate "Texas Instruments ADC081C021/027"
+	tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
 	depends on I2C
 	help
-	  If you say yes here you get support for Texas Instruments ADC081C021
-	  and ADC081C027 ADC chips.
+	  If you say yes here you get support for Texas Instruments ADC081C,
+	  ADC101C and ADC121C ADC chips.
 
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-adc081c.
drivers/iio/adc/ti-adc081c.c +27 -4
index ecbc121..9b2f26f 100644
@@ -1,9 +1,21 @@
 /*
+ * TI ADC081C/ADC101C/ADC121C 8/10/12-bit ADC driver
+ *
  * Copyright (C) 2012 Avionic Design GmbH
+ * Copyright (C) 2016 Intel
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
+ *
+ * Datasheets:
+ * 	http://www.ti.com/lit/ds/symlink/adc081c021.pdf
+ * 	http://www.ti.com/lit/ds/symlink/adc101c021.pdf
+ *	http://www.ti.com/lit/ds/symlink/adc121c021.pdf
+ *
+ * The devices have a very similar interface and differ mostly in the number of
+ * bits handled. For the 8-bit and 10-bit models the least-significant 4 or 2
+ * bits of value registers are reserved.
  */
 
 #include <linux/err.h>
@@ -17,6 +29,9 @@
 struct adc081c {
 	struct i2c_client *i2c;
 	struct regulator *ref;
+
+	/* 8, 10 or 12 */
+	int bits;
 };
 
 #define REG_CONV_RES 0x00
@@ -34,7 +49,7 @@ static int adc081c_read_raw(struct iio_dev *iio,
 		if (err < 0)
 			return err;
 
-		*value = (err >> 4) & 0xff;
+		*value = (err & 0xFFF) >> (12 - adc->bits);
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
@@ -43,7 +58,7 @@ static int adc081c_read_raw(struct iio_dev *iio,
 			return err;
 
 		*value = err / 1000;
-		*shift = 8;
+		*shift = adc->bits;
 
 		return IIO_VAL_FRACTIONAL_LOG2;
 
@@ -72,6 +87,9 @@ static int adc081c_probe(struct i2c_client *client,
 	struct adc081c *adc;
 	int err;
 
+	if (id->driver_data != 8 && id->driver_data != 10 && id->driver_data != 12)
+		return -EINVAL;
+
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -EOPNOTSUPP;
 
@@ -81,6 +99,7 @@ static int adc081c_probe(struct i2c_client *client,
 
 	adc = iio_priv(iio);
 	adc->i2c = client;
+	adc->bits = id->driver_data;
 
 	adc->ref = devm_regulator_get(&client->dev, "vref");
 	if (IS_ERR(adc->ref))
@@ -124,7 +143,9 @@ static int adc081c_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id adc081c_id[] = {
-	{ "adc081c", 0 },
+	{ "adc081c",  8 },
+	{ "adc101c", 10 },
+	{ "adc121c", 12 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adc081c_id);
@@ -132,6 +153,8 @@ MODULE_DEVICE_TABLE(i2c, adc081c_id);
 #ifdef CONFIG_OF
 static const struct of_device_id adc081c_of_match[] = {
 	{ .compatible = "ti,adc081c" },
+	{ .compatible = "ti,adc101c" },
+	{ .compatible = "ti,adc121c" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adc081c_of_match);
@@ -149,5 +172,5 @@ static struct i2c_driver adc081c_driver = {
 module_i2c_driver(adc081c_driver);
 
 MODULE_AUTHOR("Thierry Reding <[email protected]>");
-MODULE_DESCRIPTION("Texas Instruments ADC081C021/027 driver");
+MODULE_DESCRIPTION("Texas Instruments ADC081C/ADC101C/ADC121C driver");
 MODULE_LICENSE("GPL v2");
Signature · 1 line
2.8.0.rc3
#2 lore raw

Re: [PATCH 1/2] ti-adc081c: Add support for adc101c* and adc121c*

Quoted 3 lines

These chips has an almost identical interface but a deal with a different number of value bits. Datasheet links for comparison:

comments below

Quoted 84 lines

* http://www.ti.com/lit/ds/symlink/adc081c021.pdf
* http://www.ti.com/lit/ds/symlink/adc101c021.pdf
* http://www.ti.com/lit/ds/symlink/adc121c021.pdf

---
 drivers/iio/adc/Kconfig      |  6 +++---
 drivers/iio/adc/ti-adc081c.c | 31 +++++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 7 deletions(-)
drivers/iio/adc/Kconfig +3 -3
index 9ddcd5d..a2d0db5 100644
@@ -378,11 +378,11 @@ config ROCKCHIP_SARADC
 	  module will be called rockchip_saradc.
 
 config TI_ADC081C
-	tristate "Texas Instruments ADC081C021/027"
+	tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
 	depends on I2C
 	help
-	  If you say yes here you get support for Texas Instruments ADC081C021
-	  and ADC081C027 ADC chips.
+	  If you say yes here you get support for Texas Instruments ADC081C,
+	  ADC101C and ADC121C ADC chips.
 
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-adc081c.
drivers/iio/adc/ti-adc081c.c +17 -2
index ecbc121..9b2f26f 100644
@@ -1,9 +1,21 @@
 /*
+ * TI ADC081C/ADC101C/ADC121C 8/10/12-bit ADC driver
+ *
  * Copyright (C) 2012 Avionic Design GmbH
+ * Copyright (C) 2016 Intel
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
+ *
+ * Datasheets:
+ * 	http://www.ti.com/lit/ds/symlink/adc081c021.pdf
+ * 	http://www.ti.com/lit/ds/symlink/adc101c021.pdf
+ *	http://www.ti.com/lit/ds/symlink/adc121c021.pdf
+ *
+ * The devices have a very similar interface and differ mostly in the number of
+ * bits handled. For the 8-bit and 10-bit models the least-significant 4 or 2
+ * bits of value registers are reserved.
  */
 
 #include <linux/err.h>
@@ -17,6 +29,9 @@
 struct adc081c {
 	struct i2c_client *i2c;
 	struct regulator *ref;
+
+	/* 8, 10 or 12 */
+	int bits;
 };
 
 #define REG_CONV_RES 0x00
@@ -34,7 +49,7 @@ static int adc081c_read_raw(struct iio_dev *iio,
 		if (err < 0)
 			return err;
 
-		*value = (err >> 4) & 0xff;
+		*value = (err & 0xFFF) >> (12 - adc->bits);
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
@@ -43,7 +58,7 @@ static int adc081c_read_raw(struct iio_dev *iio,
 			return err;
 
 		*value = err / 1000;
-		*shift = 8;
+		*shift = adc->bits;
 
 		return IIO_VAL_FRACTIONAL_LOG2;
 

drivers/iio/adc/ti-adc081c.c static int adc081c_probe(struct i2c_client *client, · line 90

Quoted 4 lines
drivers/iio/adc/ti-adc081c.c +1 -0
@@ -72,6 +87,9 @@ static int adc081c_probe(struct i2c_client *client,
 	struct adc081c *adc;
 	int err;
 
+	if (id->driver_data != 8 && id->driver_data != 10 && id->driver_data != 12)

often the id/driverdata is used to point to a table of chip-specific configuration data rather than using driver_data to store the data itself

the second approach does not scale when more configuration data needs to be passed (just an observation)

Quoted 41 lines
drivers/iio/adc/ti-adc081c.c +9 -2
+		return -EINVAL;
+
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -EOPNOTSUPP;
 
@@ -81,6 +99,7 @@ static int adc081c_probe(struct i2c_client *client,
 
 	adc = iio_priv(iio);
 	adc->i2c = client;
+	adc->bits = id->driver_data;
 
 	adc->ref = devm_regulator_get(&client->dev, "vref");
 	if (IS_ERR(adc->ref))
@@ -124,7 +143,9 @@ static int adc081c_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id adc081c_id[] = {
-	{ "adc081c", 0 },
+	{ "adc081c",  8 },
+	{ "adc101c", 10 },
+	{ "adc121c", 12 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adc081c_id);
@@ -132,6 +153,8 @@ MODULE_DEVICE_TABLE(i2c, adc081c_id);
 #ifdef CONFIG_OF
 static const struct of_device_id adc081c_of_match[] = {
 	{ .compatible = "ti,adc081c" },
+	{ .compatible = "ti,adc101c" },
+	{ .compatible = "ti,adc121c" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adc081c_of_match);
@@ -149,5 +172,5 @@ static struct i2c_driver adc081c_driver = {
 module_i2c_driver(adc081c_driver);
 
 MODULE_AUTHOR("Thierry Reding <[email protected]>");
-MODULE_DESCRIPTION("Texas Instruments ADC081C021/027 driver");
+MODULE_DESCRIPTION("Texas Instruments ADC081C/ADC101C/ADC121C driver");
 MODULE_LICENSE("GPL v2");
Signature · 2 lines
Peter Meerwald-Stadler
+43-664-2444418 (mobile)
maintainer
#3 lore raw

Re: [PATCH 1/2] ti-adc081c: Add support for adc101c* and adc121c*

In reply to Crestez Dan Leonard
Quoted 8 lines · Crestez Dan Leonard

These chips has an almost identical interface but a deal with a different number of value bits. Datasheet links for comparison:

* http://www.ti.com/lit/ds/symlink/adc081c021.pdf
* http://www.ti.com/lit/ds/symlink/adc101c021.pdf
* http://www.ti.com/lit/ds/symlink/adc121c021.pdf

Just one query inline.

J

Quoted 79 lines
---
 drivers/iio/adc/Kconfig      |  6 +++---
 drivers/iio/adc/ti-adc081c.c | 31 +++++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 7 deletions(-)
drivers/iio/adc/Kconfig +3 -3
index 9ddcd5d..a2d0db5 100644
@@ -378,11 +378,11 @@ config ROCKCHIP_SARADC
 	  module will be called rockchip_saradc.
 
 config TI_ADC081C
-	tristate "Texas Instruments ADC081C021/027"
+	tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
 	depends on I2C
 	help
-	  If you say yes here you get support for Texas Instruments ADC081C021
-	  and ADC081C027 ADC chips.
+	  If you say yes here you get support for Texas Instruments ADC081C,
+	  ADC101C and ADC121C ADC chips.
 
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-adc081c.
drivers/iio/adc/ti-adc081c.c +17 -2
index ecbc121..9b2f26f 100644
@@ -1,9 +1,21 @@
 /*
+ * TI ADC081C/ADC101C/ADC121C 8/10/12-bit ADC driver
+ *
  * Copyright (C) 2012 Avionic Design GmbH
+ * Copyright (C) 2016 Intel
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
+ *
+ * Datasheets:
+ * 	http://www.ti.com/lit/ds/symlink/adc081c021.pdf
+ * 	http://www.ti.com/lit/ds/symlink/adc101c021.pdf
+ *	http://www.ti.com/lit/ds/symlink/adc121c021.pdf
+ *
+ * The devices have a very similar interface and differ mostly in the number of
+ * bits handled. For the 8-bit and 10-bit models the least-significant 4 or 2
+ * bits of value registers are reserved.
  */
 
 #include <linux/err.h>
@@ -17,6 +29,9 @@
 struct adc081c {
 	struct i2c_client *i2c;
 	struct regulator *ref;
+
+	/* 8, 10 or 12 */
+	int bits;
 };
 
 #define REG_CONV_RES 0x00
@@ -34,7 +49,7 @@ static int adc081c_read_raw(struct iio_dev *iio,
 		if (err < 0)
 			return err;
 
-		*value = (err >> 4) & 0xff;
+		*value = (err & 0xFFF) >> (12 - adc->bits);
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
@@ -43,7 +58,7 @@ static int adc081c_read_raw(struct iio_dev *iio,
 			return err;
 
 		*value = err / 1000;
-		*shift = 8;
+		*shift = adc->bits;
 
 		return IIO_VAL_FRACTIONAL_LOG2;
 

drivers/iio/adc/ti-adc081c.c static int adc081c_probe(struct i2c_client *client, · line 91

Quoted 5 lines
drivers/iio/adc/ti-adc081c.c +2 -0
@@ -72,6 +87,9 @@ static int adc081c_probe(struct i2c_client *client,
 	struct adc081c *adc;
 	int err;
 
+	if (id->driver_data != 8 && id->driver_data != 10 && id->driver_data != 12)
+		return -EINVAL;

How could this condition occur? The driver won't be probed if we don't have a match on one of the items in the id table and they all obey this rull?

On Peter's comment, it would be future proofing to use an index into a information structure array, but I'd prefer that to be added when needed and am reasonably happy with how you have it here.

Quoted 40 lines
drivers/iio/adc/ti-adc081c.c +8 -2
+
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -EOPNOTSUPP;
 
@@ -81,6 +99,7 @@ static int adc081c_probe(struct i2c_client *client,
 
 	adc = iio_priv(iio);
 	adc->i2c = client;
+	adc->bits = id->driver_data;
 
 	adc->ref = devm_regulator_get(&client->dev, "vref");
 	if (IS_ERR(adc->ref))
@@ -124,7 +143,9 @@ static int adc081c_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id adc081c_id[] = {
-	{ "adc081c", 0 },
+	{ "adc081c",  8 },
+	{ "adc101c", 10 },
+	{ "adc121c", 12 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adc081c_id);
@@ -132,6 +153,8 @@ MODULE_DEVICE_TABLE(i2c, adc081c_id);
 #ifdef CONFIG_OF
 static const struct of_device_id adc081c_of_match[] = {
 	{ .compatible = "ti,adc081c" },
+	{ .compatible = "ti,adc101c" },
+	{ .compatible = "ti,adc121c" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adc081c_of_match);
@@ -149,5 +172,5 @@ static struct i2c_driver adc081c_driver = {
 module_i2c_driver(adc081c_driver);
 
 MODULE_AUTHOR("Thierry Reding <[email protected]>");
-MODULE_DESCRIPTION("Texas Instruments ADC081C021/027 driver");
+MODULE_DESCRIPTION("Texas Instruments ADC081C/ADC101C/ADC121C driver");
 MODULE_LICENSE("GPL v2");
#4 lore raw

PATCH 2/2 ti-adc081c: Initial triggered buffer support

In reply to Crestez Dan Leonard

Using this requires software triggers like CONFIG_IIO_HRTIMER_TRIGGER.

The device can be configured to do internal periodic sampling but does not appear to offer some sort of interrupt on data ready. It only offers interrupts on values out of a specific range.

---
 drivers/iio/adc/ti-adc081c.c | 99 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 83 insertions(+), 16 deletions(-)
drivers/iio/adc/ti-adc081c.c +83 -16
index 9b2f26f..040e2aa 100644
@@ -24,6 +24,9 @@
 #include <linux/of.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/regulator/consumer.h>
 
 struct adc081c {
@@ -69,27 +72,83 @@ static int adc081c_read_raw(struct iio_dev *iio,
 	return -EINVAL;
 }
 
-static const struct iio_chan_spec adc081c_channel = {
-	.type = IIO_VOLTAGE,
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-};
+static irqreturn_t adc081c_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adc081c *data = iio_priv(indio_dev);
+	s64 ts;
+	u16 buf[8];
+	int ret;
+
+	/* Otherwise iio_push_to_buffers will corrupt the stack. */
+	if (indio_dev->scan_bytes > sizeof(buf)) {
+		dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
+				indio_dev->scan_bytes, (int)sizeof(buf));
+		goto out;
+	}
+
+	ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
+	ts = iio_get_time_ns();
+	if (ret < 0)
+		goto out;
+	buf[0] = ret;
+	iio_push_to_buffers_with_timestamp(indio_dev, buf, ts);
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
 
 static const struct iio_info adc081c_info = {
 	.read_raw = adc081c_read_raw,
 	.driver_module = THIS_MODULE,
 };
 
+struct adcxx1c_model {
+	int bits;
+	const struct iio_chan_spec* channels;
+};
+
+#define ADCxx1C_CHAN(_bits) {					\
+	.type = IIO_VOLTAGE,					\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = (_bits),				\
+		.storagebits = 16,				\
+		.shift = 12 - (_bits),				\
+		.endianness = IIO_CPU,				\
+	},							\
+}
+
+#define DEFINE_ADCxx1C_MODEL(_name, _bits)				\
+	static const struct iio_chan_spec _name ## _channels[] = {	\
+		ADCxx1C_CHAN((_bits)),					\
+		IIO_CHAN_SOFT_TIMESTAMP(1),				\
+	};								\
+	static const struct adcxx1c_model _name ## _model = {		\
+		.bits = (_bits),					\
+		.channels = _name ## _channels,				\
+	}
+
+DEFINE_ADCxx1C_MODEL(adc081c,  8);
+DEFINE_ADCxx1C_MODEL(adc101c, 10);
+DEFINE_ADCxx1C_MODEL(adc121c, 12);
+
+struct adcxx1c_info {
+	int bits;
+	const struct adc081c_channels* channels;
+};
+
 static int adc081c_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct iio_dev *iio;
 	struct adc081c *adc;
+	struct adcxx1c_model *model = (struct adcxx1c_model*)id->driver_data;
 	int err;
 
-	if (id->driver_data != 8 && id->driver_data != 10 && id->driver_data != 12)
-		return -EINVAL;
-
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -EOPNOTSUPP;
 
@@ -99,7 +158,7 @@ static int adc081c_probe(struct i2c_client *client,
 
 	adc = iio_priv(iio);
 	adc->i2c = client;
-	adc->bits = id->driver_data;
+	adc->bits = model->bits;
 
 	adc->ref = devm_regulator_get(&client->dev, "vref");
 	if (IS_ERR(adc->ref))
@@ -114,18 +173,26 @@ static int adc081c_probe(struct i2c_client *client,
 	iio->modes = INDIO_DIRECT_MODE;
 	iio->info = &adc081c_info;
 
-	iio->channels = &adc081c_channel;
-	iio->num_channels = 1;
+	iio->channels = model->channels;
+	iio->num_channels = 2;
+
+	err = iio_triggered_buffer_setup(iio, NULL, adc081c_trigger_handler, NULL);
+	if (err < 0) {
+		dev_err(&client->dev, "iio triggered buffer setup failed\n");
+		goto err_regulator_disable;
+	}
 
 	err = iio_device_register(iio);
 	if (err < 0)
-		goto regulator_disable;
+		goto err_buffer_cleanup;
 
 	i2c_set_clientdata(client, iio);
 
 	return 0;
 
-regulator_disable:
+err_buffer_cleanup:
+	iio_triggered_buffer_cleanup(iio);
+err_regulator_disable:
 	regulator_disable(adc->ref);
 
 	return err;
@@ -143,9 +210,9 @@ static int adc081c_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id adc081c_id[] = {
-	{ "adc081c",  8 },
-	{ "adc101c", 10 },
-	{ "adc121c", 12 },
+	{ "adc081c", (long)&adc081c_model },
+	{ "adc101c", (long)&adc101c_model },
+	{ "adc121c", (long)&adc121c_model },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adc081c_id);
Signature · 1 line
2.8.0.rc3
#5 lore raw

Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support

Quoted 1 line · Crestez Dan Leonard

Using this requires software triggers like CONFIG_IIO_HRTIMER_TRIGGER.

Then we are missing DEPENDS in Kconfig...

Quoted 176 lines

The device can be configured to do internal periodic sampling but does not appear to offer some sort of interrupt on data ready. It only offers interrupts on values out of a specific range.

---
 drivers/iio/adc/ti-adc081c.c | 99 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 83 insertions(+), 16 deletions(-)
drivers/iio/adc/ti-adc081c.c +83 -17
index 9b2f26f..040e2aa 100644
@@ -24,6 +24,9 @@
 #include <linux/of.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/regulator/consumer.h>
 
 struct adc081c {
@@ -69,27 +72,83 @@ static int adc081c_read_raw(struct iio_dev *iio,
        return -EINVAL;
 }
 
-static const struct iio_chan_spec adc081c_channel = {
-       .type = IIO_VOLTAGE,
-       .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
-       .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-};
+static irqreturn_t adc081c_trigger_handler(int irq, void *p)
+{
+       struct iio_poll_func *pf = p;
+       struct iio_dev *indio_dev = pf->indio_dev;
+       struct adc081c *data = iio_priv(indio_dev);
+       s64 ts;
+       u16 buf[8];
+       int ret;
+
+       /* Otherwise iio_push_to_buffers will corrupt the stack. */
+       if (indio_dev->scan_bytes > sizeof(buf)) {
+               dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
+                               indio_dev->scan_bytes, (int)sizeof(buf));
+               goto out;
+       }
+
+       ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
+       ts = iio_get_time_ns();
+       if (ret < 0)
+               goto out;
+       buf[0] = ret;
+       iio_push_to_buffers_with_timestamp(indio_dev, buf, ts);
+out:
+       iio_trigger_notify_done(indio_dev->trig);
+       return IRQ_HANDLED;
+}
 
 static const struct iio_info adc081c_info = {
        .read_raw = adc081c_read_raw,
        .driver_module = THIS_MODULE,
 };
 
+struct adcxx1c_model {
+       int bits;
+       const struct iio_chan_spec* channels;
+};
+
+#define ADCxx1C_CHAN(_bits) {                                  \
+       .type = IIO_VOLTAGE,                                    \
+       .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
+       .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
+       .scan_type = {                                          \
+               .sign = 'u',                                    \
+               .realbits = (_bits),                            \
+               .storagebits = 16,                              \
+               .shift = 12 - (_bits),                          \
+               .endianness = IIO_CPU,                          \
+       },                                                      \
+}
+
+#define DEFINE_ADCxx1C_MODEL(_name, _bits)                             \
+       static const struct iio_chan_spec _name ## _channels[] = {      \
+               ADCxx1C_CHAN((_bits)),                                  \
+               IIO_CHAN_SOFT_TIMESTAMP(1),                             \
+       };                                                              \
+       static const struct adcxx1c_model _name ## _model = {           \
+               .bits = (_bits),                                        \
+               .channels = _name ## _channels,                         \
+       }
+
+DEFINE_ADCxx1C_MODEL(adc081c,  8);
+DEFINE_ADCxx1C_MODEL(adc101c, 10);
+DEFINE_ADCxx1C_MODEL(adc121c, 12);
+
+struct adcxx1c_info {
+       int bits;
+       const struct adc081c_channels* channels;
+};
+
 static int adc081c_probe(struct i2c_client *client,
                         const struct i2c_device_id *id)
 {
        struct iio_dev *iio;
        struct adc081c *adc;
+       struct adcxx1c_model *model = (struct adcxx1c_model*)id->driver_data;
        int err;
 
-       if (id->driver_data != 8 && id->driver_data != 10 && id->driver_data != 12)
-               return -EINVAL;
-
        if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
                return -EOPNOTSUPP;
 
@@ -99,7 +158,7 @@ static int adc081c_probe(struct i2c_client *client,
 
        adc = iio_priv(iio);
        adc->i2c = client;
-       adc->bits = id->driver_data;
+       adc->bits = model->bits;
 
        adc->ref = devm_regulator_get(&client->dev, "vref");
        if (IS_ERR(adc->ref))
@@ -114,18 +173,26 @@ static int adc081c_probe(struct i2c_client *client,
        iio->modes = INDIO_DIRECT_MODE;
        iio->info = &adc081c_info;
 
-       iio->channels = &adc081c_channel;
-       iio->num_channels = 1;
+       iio->channels = model->channels;
+       iio->num_channels = 2;
+
+       err = iio_triggered_buffer_setup(iio, NULL, adc081c_trigger_handler, NULL);
+       if (err < 0) {
+               dev_err(&client->dev, "iio triggered buffer setup failed\n");
+               goto err_regulator_disable;
+       }
 
        err = iio_device_register(iio);
        if (err < 0)
-               goto regulator_disable;
+               goto err_buffer_cleanup;
 
        i2c_set_clientdata(client, iio);
 
        return 0;
 
-regulator_disable:
+err_buffer_cleanup:
+       iio_triggered_buffer_cleanup(iio);
+err_regulator_disable:
        regulator_disable(adc->ref);
 
        return err;
@@ -143,9 +210,9 @@ static int adc081c_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id adc081c_id[] = {
-       { "adc081c",  8 },
-       { "adc101c", 10 },
-       { "adc121c", 12 },
+       { "adc081c", (long)&adc081c_model },
+       { "adc101c", (long)&adc101c_model },
+       { "adc121c", (long)&adc121c_model },
        { }
 };
 MODULE_DEVICE_TABLE(i2c, adc081c_id);
--

2.8.0.rc3

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

#6 lore raw

Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support

Quoted 4 lines · Crt Mori
Quoted 1 line · Crestez Dan Leonard

Using this requires software triggers like CONFIG_IIO_HRTIMER_TRIGGER.

Then we are missing DEPENDS in Kconfig...

The device could be used with any generic trigger. The device driver shouldn't make the choice here which one it requires and which one not.

- Lars

#7 lore raw

Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support

In reply to Crestez Dan Leonard
Quoted 27 lines

Using this requires software triggers like CONFIG_IIO_HRTIMER_TRIGGER.

The device can be configured to do internal periodic sampling but does not appear to offer some sort of interrupt on data ready. It only offers interrupts on values out of a specific range.

---
 drivers/iio/adc/ti-adc081c.c | 99 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 83 insertions(+), 16 deletions(-)
drivers/iio/adc/ti-adc081c.c +3 -0
index 9b2f26f..040e2aa 100644
@@ -24,6 +24,9 @@
 #include <linux/of.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/regulator/consumer.h>
 
 struct adc081c {

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 76

Quoted 8 lines
drivers/iio/adc/ti-adc081c.c +0 -5
@@ -69,27 +72,83 @@ static int adc081c_read_raw(struct iio_dev *iio,
 	return -EINVAL;
 }
 
-static const struct iio_chan_spec adc081c_channel = {
-	.type = IIO_VOLTAGE,
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-};

the patch would look cleaner/shorter if adc081c_channel won't get moved around

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 81

Quoted 7 lines
drivers/iio/adc/ti-adc081c.c +7 -0
+static irqreturn_t adc081c_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adc081c *data = iio_priv(indio_dev);
+	s64 ts;
+	u16 buf[8];

comment: 2 bytes data + 6 bytes padding + 8 bytes timestamp

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 87

Quoted 6 lines
drivers/iio/adc/ti-adc081c.c +6 -0
+	int ret;
+
+	/* Otherwise iio_push_to_buffers will corrupt the stack. */
+	if (indio_dev->scan_bytes > sizeof(buf)) {
+		dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
+				indio_dev->scan_bytes, (int)sizeof(buf));

rather than casting sizeof(buf), use the correct printf length modifier, i.e. %z

not sure if this check is needed

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 91

Quoted 4 lines
drivers/iio/adc/ti-adc081c.c +4 -0
+		goto out;
+	}
+
+	ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);

REG_CONV_RES should be called ADC081C_REG_CONV_RES, but that's a separate issue

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 92

Quoted 1 line
drivers/iio/adc/ti-adc081c.c +1 -0
+	ts = iio_get_time_ns();

why is the timestamp taken here?, seems strange
often this is done together with iio_push_to_buffers_with_timestamp()

Quoted 75 lines
drivers/iio/adc/ti-adc081c.c +47 -4
+	if (ret < 0)
+		goto out;
+	buf[0] = ret;
+	iio_push_to_buffers_with_timestamp(indio_dev, buf, ts);
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
 
 static const struct iio_info adc081c_info = {
 	.read_raw = adc081c_read_raw,
 	.driver_module = THIS_MODULE,
 };
 
+struct adcxx1c_model {
+	int bits;
+	const struct iio_chan_spec* channels;
+};
+
+#define ADCxx1C_CHAN(_bits) {					\
+	.type = IIO_VOLTAGE,					\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = (_bits),				\
+		.storagebits = 16,				\
+		.shift = 12 - (_bits),				\
+		.endianness = IIO_CPU,				\
+	},							\
+}
+
+#define DEFINE_ADCxx1C_MODEL(_name, _bits)				\
+	static const struct iio_chan_spec _name ## _channels[] = {	\
+		ADCxx1C_CHAN((_bits)),					\
+		IIO_CHAN_SOFT_TIMESTAMP(1),				\
+	};								\
+	static const struct adcxx1c_model _name ## _model = {		\
+		.bits = (_bits),					\
+		.channels = _name ## _channels,				\
+	}
+
+DEFINE_ADCxx1C_MODEL(adc081c,  8);
+DEFINE_ADCxx1C_MODEL(adc101c, 10);
+DEFINE_ADCxx1C_MODEL(adc121c, 12);
+
+struct adcxx1c_info {
+	int bits;
+	const struct adc081c_channels* channels;
+};
+
 static int adc081c_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct iio_dev *iio;
 	struct adc081c *adc;
+	struct adcxx1c_model *model = (struct adcxx1c_model*)id->driver_data;
 	int err;
 
-	if (id->driver_data != 8 && id->driver_data != 10 && id->driver_data != 12)
-		return -EINVAL;
-
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -EOPNOTSUPP;
 
@@ -99,7 +158,7 @@ static int adc081c_probe(struct i2c_client *client,
 
 	adc = iio_priv(iio);
 	adc->i2c = client;
-	adc->bits = id->driver_data;
+	adc->bits = model->bits;
 
 	adc->ref = devm_regulator_get(&client->dev, "vref");
 	if (IS_ERR(adc->ref))

drivers/iio/adc/ti-adc081c.c static int adc081c_probe(struct i2c_client *client, · line 177

Quoted 7 lines
drivers/iio/adc/ti-adc081c.c +2 -2
@@ -114,18 +173,26 @@ static int adc081c_probe(struct i2c_client *client,
 	iio->modes = INDIO_DIRECT_MODE;
 	iio->info = &adc081c_info;
 
-	iio->channels = &adc081c_channel;
-	iio->num_channels = 1;
+	iio->channels = model->channels;
+	iio->num_channels = 2;

the number of channels could go into the adcxx1c_info struct

Quoted 24 lines
drivers/iio/adc/ti-adc081c.c +10 -2
+
+	err = iio_triggered_buffer_setup(iio, NULL, adc081c_trigger_handler, NULL);
+	if (err < 0) {
+		dev_err(&client->dev, "iio triggered buffer setup failed\n");
+		goto err_regulator_disable;
+	}
 
 	err = iio_device_register(iio);
 	if (err < 0)
-		goto regulator_disable;
+		goto err_buffer_cleanup;
 
 	i2c_set_clientdata(client, iio);
 
 	return 0;
 
-regulator_disable:
+err_buffer_cleanup:
+	iio_triggered_buffer_cleanup(iio);
+err_regulator_disable:
 	regulator_disable(adc->ref);
 
 	return err;

drivers/iio/adc/ti-adc081c.c static int adc081c_remove(struct i2c_client *client) · line 210

Quoted 1 line
drivers/iio/adc/ti-adc081c.c +0 -0
@@ -143,9 +210,9 @@ static int adc081c_remove(struct i2c_client *client)
 }

iio_triggered_buffer_cleanup() in _remove()?

drivers/iio/adc/ti-adc081c.c static int adc081c_remove(struct i2c_client *client) · line 213

Quoted 5 lines
drivers/iio/adc/ti-adc081c.c +1 -3
 static const struct i2c_device_id adc081c_id[] = {
-	{ "adc081c",  8 },
-	{ "adc101c", 10 },
-	{ "adc121c", 12 },
+	{ "adc081c", (long)&adc081c_model },

often an enum is used instead of a pointer

Quoted 6 lines
drivers/iio/adc/ti-adc081c.c +2 -0
+	{ "adc101c", (long)&adc101c_model },
+	{ "adc121c", (long)&adc121c_model },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adc081c_id);
 
Signature · 2 lines
Peter Meerwald-Stadler
+43-664-2444418 (mobile)
#8 lore raw

Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support

Quoted 8 lines · Peter Meerwald-Stadler

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 76

Quoted 5 lines
drivers/iio/adc/ti-adc081c.c +0 -5
-static const struct iio_chan_spec adc081c_channel = {
-	.type = IIO_VOLTAGE,
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-};

the patch would look cleaner/shorter if adc081c_channel won't get moved around

It was not moved around, it is now defined by a macro. Buffer support requires defining scan_type which contains a different number of bits.

The macros are now after iio_info adc081c_info instead of before, I can move that around. I also noticed that I declared both a struct adcxx1c_model and an unused struct adcxx1c_info. I will remove that.

The first patch in the series doesn't use any per-model data and just stores the number of bits in driver_data. I can change the series to introduce adcxx1c_model in the first patch.

Quoted 20 lines

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 81

Quoted 7 lines
drivers/iio/adc/ti-adc081c.c +7 -0
+static irqreturn_t adc081c_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adc081c *data = iio_priv(indio_dev);
+	s64 ts;
+	u16 buf[8];

comment: 2 bytes data + 6 bytes padding + 8 bytes timestamp

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 87

Quoted 6 lines
drivers/iio/adc/ti-adc081c.c +6 -0
+	int ret;
+
+	/* Otherwise iio_push_to_buffers will corrupt the stack. */
+	if (indio_dev->scan_bytes > sizeof(buf)) {
+		dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
+				indio_dev->scan_bytes, (int)sizeof(buf));

rather than casting sizeof(buf), use the correct printf length modifier, i.e. %z

Quoted 1 line

not sure if this check is needed

I guess it's not needed. My first version defined the buffer incorrectly and caused a messy crash. Calculating manual buffer alignments seems very fragile.

It seems that C99 variable-length-arrays work fine, something like:

	u16 buf[indio_dev->scan_bytes / 2];

Would that be acceptable? It compiles without warnings and there some other places in the kernel where VLAs are used.

Quoted 4 lines

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 91

Quoted 1 line
drivers/iio/adc/ti-adc081c.c +1 -0
+	ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);

REG_CONV_RES should be called ADC081C_REG_CONV_RES, but that's a separate issue

Yes, but that would be an entirely unrelated renaming.

Quoted 5 lines

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 92

Quoted 2 lines
drivers/iio/adc/ti-adc081c.c +1 -0
+	ts = iio_get_time_ns();

why is the timestamp taken here?, seems strange
often this is done together with iio_push_to_buffers_with_timestamp()

I wanted to keep it as close to the read as possible. In this case it doesn't matter.

Quoted 6 lines

drivers/iio/adc/ti-adc081c.c static int adc081c_probe(struct i2c_client *client, · line 177

Quoted 4 lines
drivers/iio/adc/ti-adc081c.c +2 -2
-	iio->channels = &adc081c_channel;
-	iio->num_channels = 1;
+	iio->channels = model->channels;
+	iio->num_channels = 2;

the number of channels could go into the adcxx1c_info struct

But it's a constant, it does not vary between devices. I could make an ADC081C_NUM_CHANNELS define.

--
Regards,
Leonard

maintainer
#9 lore raw

Re: [PATCH 2/2] ti-adc081c: Initial triggered buffer support

In reply to Peter Meerwald-Stadler
Quoted 6 lines · Peter Meerwald-Stadler
Quoted 6 lines

Using this requires software triggers like CONFIG_IIO_HRTIMER_TRIGGER.

The device can be configured to do internal periodic sampling but does not appear to offer some sort of interrupt on data ready. It only offers interrupts on values out of a specific range.

This isn't uncommon on chips that have some 'alarm' type capability. max1363 would be another example.

Quoted 2 lines
Quoted 2 lines

Mostly good, but a few little points inline to add to Peter's.

Jonathan

Quoted 61 lines
Quoted 19 lines
---
 drivers/iio/adc/ti-adc081c.c | 99 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 83 insertions(+), 16 deletions(-)
drivers/iio/adc/ti-adc081c.c +3 -0
index 9b2f26f..040e2aa 100644
@@ -24,6 +24,9 @@
 #include <linux/of.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/regulator/consumer.h>
 
 struct adc081c {

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 76

Quoted 8 lines
drivers/iio/adc/ti-adc081c.c +0 -5
@@ -69,27 +72,83 @@ static int adc081c_read_raw(struct iio_dev *iio,
 	return -EINVAL;
 }
 
-static const struct iio_chan_spec adc081c_channel = {
-	.type = IIO_VOLTAGE,
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-};

the patch would look cleaner/shorter if adc081c_channel won't get moved around

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 81

Quoted 7 lines
drivers/iio/adc/ti-adc081c.c +7 -0
+static irqreturn_t adc081c_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adc081c *data = iio_priv(indio_dev);
+	s64 ts;
+	u16 buf[8];

comment: 2 bytes data + 6 bytes padding + 8 bytes timestamp

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 87

Quoted 6 lines
drivers/iio/adc/ti-adc081c.c +6 -0
+	int ret;
+
+	/* Otherwise iio_push_to_buffers will corrupt the stack. */
+	if (indio_dev->scan_bytes > sizeof(buf)) {
+		dev_crit_once(&indio_dev->dev, "Bad iio_scan_bytes=%d > %d\n",
+				indio_dev->scan_bytes, (int)sizeof(buf));

rather than casting sizeof(buf), use the correct printf length modifier, i.e. %z

not sure if this check is needed

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 91

Quoted 4 lines
drivers/iio/adc/ti-adc081c.c +4 -0
+		goto out;
+	}
+
+	ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);

REG_CONV_RES should be called ADC081C_REG_CONV_RES, but that's a separate issue

Quoted 4 lines

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 92

Quoted 1 line
drivers/iio/adc/ti-adc081c.c +1 -0
+	ts = iio_get_time_ns();

why is the timestamp taken here?, seems strange
often this is done together with iio_push_to_buffers_with_timestamp()

Depends on where the trigger is coming from - here this is the correct option as it is the best guess on when the reading was actually taken, however the local variable is pointless, just roll this into the place it is used below.

Quoted 32 lines
Quoted 32 lines
drivers/iio/adc/ti-adc081c.c +25 -0
+	if (ret < 0)
+		goto out;
+	buf[0] = ret;
+	iio_push_to_buffers_with_timestamp(indio_dev, buf, ts);
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
 
 static const struct iio_info adc081c_info = {
 	.read_raw = adc081c_read_raw,
 	.driver_module = THIS_MODULE,
 };
 
+struct adcxx1c_model {
+	int bits;
+	const struct iio_chan_spec* channels;
+};
+
+#define ADCxx1C_CHAN(_bits) {					\
+	.type = IIO_VOLTAGE,					\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = (_bits),				\
+		.storagebits = 16,				\
+		.shift = 12 - (_bits),				\
+		.endianness = IIO_CPU,				\
+	},							\
+}
Quoted 8 lines

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 131

Quoted 8 lines
drivers/iio/adc/ti-adc081c.c +8 -0
+
+#define DEFINE_ADCxx1C_MODEL(_name, _bits)				\
+	static const struct iio_chan_spec _name ## _channels[] = {	\
+		ADCxx1C_CHAN((_bits)),					\
+		IIO_CHAN_SOFT_TIMESTAMP(1),				\
+	};								\
+	static const struct adcxx1c_model _name ## _model = {		\
+		.bits = (_bits),					\

Worth replicating bits? I would imagine that everywhere it is needed the channel pointer is also available, complete with this information.

Quoted 20 lines
Quoted 20 lines
drivers/iio/adc/ti-adc081c.c +13 -0
+		.channels = _name ## _channels,				\
+	}
+
+DEFINE_ADCxx1C_MODEL(adc081c,  8);
+DEFINE_ADCxx1C_MODEL(adc101c, 10);
+DEFINE_ADCxx1C_MODEL(adc121c, 12);
+
+struct adcxx1c_info {
+	int bits;
+	const struct adc081c_channels* channels;
+};
+
 static int adc081c_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct iio_dev *iio;
 	struct adc081c *adc;
+	struct adcxx1c_model *model = (struct adcxx1c_model*)id->driver_data;
 	int err;
 
Quoted 3 lines

drivers/iio/adc/ti-adc081c.c static int adc081c_read_raw(struct iio_dev *iio, · line 92

Quoted 3 lines
drivers/iio/adc/ti-adc081c.c +0 -3
-	if (id->driver_data != 8 && id->driver_data != 10 && id->driver_data != 12)
-		return -EINVAL;
-

Interesting that you have dropped this check entirely (rather than making it work with the new structures). Good, but drop it from the previous patch.

Quoted 52 lines
Quoted 13 lines
drivers/iio/adc/ti-adc081c.c +1 -1
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -EOPNOTSUPP;
 
@@ -99,7 +158,7 @@ static int adc081c_probe(struct i2c_client *client,
 
 	adc = iio_priv(iio);
 	adc->i2c = client;
-	adc->bits = id->driver_data;
+	adc->bits = model->bits;
 
 	adc->ref = devm_regulator_get(&client->dev, "vref");
 	if (IS_ERR(adc->ref))

drivers/iio/adc/ti-adc081c.c static int adc081c_probe(struct i2c_client *client, · line 177

Quoted 7 lines
drivers/iio/adc/ti-adc081c.c +2 -2
@@ -114,18 +173,26 @@ static int adc081c_probe(struct i2c_client *client,
 	iio->modes = INDIO_DIRECT_MODE;
 	iio->info = &adc081c_info;
 
-	iio->channels = &adc081c_channel;
-	iio->num_channels = 1;
+	iio->channels = model->channels;
+	iio->num_channels = 2;

the number of channels could go into the adcxx1c_info struct

Quoted 24 lines
drivers/iio/adc/ti-adc081c.c +10 -2
+
+	err = iio_triggered_buffer_setup(iio, NULL, adc081c_trigger_handler, NULL);
+	if (err < 0) {
+		dev_err(&client->dev, "iio triggered buffer setup failed\n");
+		goto err_regulator_disable;
+	}
 
 	err = iio_device_register(iio);
 	if (err < 0)
-		goto regulator_disable;
+		goto err_buffer_cleanup;
 
 	i2c_set_clientdata(client, iio);
 
 	return 0;
 
-regulator_disable:
+err_buffer_cleanup:
+	iio_triggered_buffer_cleanup(iio);
+err_regulator_disable:
 	regulator_disable(adc->ref);
 
 	return err;

drivers/iio/adc/ti-adc081c.c static int adc081c_remove(struct i2c_client *client) · line 210

Quoted 1 line
drivers/iio/adc/ti-adc081c.c +0 -0
@@ -143,9 +210,9 @@ static int adc081c_remove(struct i2c_client *client)
 }

iio_triggered_buffer_cleanup() in _remove()?

Quoted 7 lines

drivers/iio/adc/ti-adc081c.c static int adc081c_remove(struct i2c_client *client) · line 213

Quoted 5 lines
drivers/iio/adc/ti-adc081c.c +1 -3
 static const struct i2c_device_id adc081c_id[] = {
-	{ "adc081c",  8 },
-	{ "adc101c", 10 },
-	{ "adc121c", 12 },
+	{ "adc081c", (long)&adc081c_model },

often an enum is used instead of a pointer

Tends to end up slightly cleaner.

Quoted 8 lines
Quoted 7 lines
drivers/iio/adc/ti-adc081c.c +2 -0
+	{ "adc101c", (long)&adc101c_model },
+	{ "adc121c", (long)&adc121c_model },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adc081c_id);
 
#10 lore raw
In reply to Crestez Dan Leonard
Quoted 1 line · Crestez Dan Leonard

This adds support for adc101c* and adc121c* using the ti-adc081c driver.

Thanks for posting this. I am eager to try it out on the BeagleBone Green for this SeeedStudio Grove I2C ADC module which uses ADC121C021:
http://www.seeedstudio.com/wiki/Grove_-_I2C_ADC

The only kernel driver I could find previously was this ADC121C021 I2C Battery Monitor Driver in an Android repo:
https://android.googlesource.com/kernel/bcm/+/android-bcm-tetra-3.10-lollipop-wear-release/drivers/power/adc121c021_driver.c

cheers,
drew

#11 lore raw
Quoted 7 lines · Drew Fustini
Quoted 1 line · Crestez Dan Leonard

This adds support for adc101c* and adc121c* using the ti-adc081c driver.

Thanks for posting this. I am eager to try it out on the BeagleBone Green for this SeeedStudio Grove I2C ADC module which uses ADC121C021:
http://www.seeedstudio.com/wiki/Grove_-_I2C_ADC

It would be nice if somebody else confirmed that this driver used correctly, especially on another arch.

Please note that in order for the driver to correctly report actual voltage a "voltage regulator" needs to be configured somehow. These chips report voltage relative to I2C supply voltage (which can vary) and correctly calculating the input voltage requires fetching the current voltage from a regulator. If no regulator is available then reading in_voltage_raw will work but in_voltage_scale will fail.

The module you linked ensures that the chip input voltage is 3V no matter the i2c supply voltage. This is specific to that PCB not the actual ti adc chip.

The easiest way to handle this would be to create a fixed-regulator reporting a constant 3V in your device tree and pointing the adc's vref-supply to it.

Regards,
Leonard