linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Add support for adc101c* and adc121c*
@ 2016-04-04 18:21 Crestez Dan Leonard
  2016-04-04 18:21 ` [PATCH v2 1/2] ti-adc081c: " Crestez Dan Leonard
  2016-04-04 18:21 ` [PATCH v2 2/2] ti-adc081c: Initial triggered buffer support Crestez Dan Leonard
  0 siblings, 2 replies; 6+ messages in thread
From: Crestez Dan Leonard @ 2016-04-04 18:21 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio
  Cc: linux-kernel, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Daniel Baluta, Crestez Dan Leonard

Changes since last version:
 * Use struct adcxx1_model in the first patch as well.
 * Use VLA buf in adc081c_trigger_handler.
 * Rerrange code so the diffs look nicer.
 * Fixed other smaller review comments.

I did not add an enum for models because it's not clear how adding another
layer of indirection would help. Maybe if we ever want to scan the table of
models? But that's not useful here.

The devices actually have names like ADC121C021/ADC121C027 to identify chips
with different pinouts but identical I2C interfaces. Those differences don't
currently matter to the driver.

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 | 103 ++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 94 insertions(+), 15 deletions(-)

-- 
2.5.5

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

* [PATCH v2 1/2] ti-adc081c: Add support for adc101c* and adc121c*
  2016-04-04 18:21 [PATCH v2 0/2] Add support for adc101c* and adc121c* Crestez Dan Leonard
@ 2016-04-04 18:21 ` Crestez Dan Leonard
  2016-04-10 14:25   ` Jonathan Cameron
  2016-04-04 18:21 ` [PATCH v2 2/2] ti-adc081c: Initial triggered buffer support Crestez Dan Leonard
  1 sibling, 1 reply; 6+ messages in thread
From: Crestez Dan Leonard @ 2016-04-04 18:21 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio
  Cc: linux-kernel, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Daniel Baluta, 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

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/adc/Kconfig      |  6 +++---
 drivers/iio/adc/ti-adc081c.c | 42 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 9ddcd5d..a2d0db5 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -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.
diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
index ecbc121..3b3c656 100644
--- a/drivers/iio/adc/ti-adc081c.c
+++ b/drivers/iio/adc/ti-adc081c.c
@@ -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;
 
@@ -60,6 +75,19 @@ static const struct iio_chan_spec adc081c_channel = {
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 };
 
+struct adcxx1c_model {
+	int bits;
+};
+
+#define DEFINE_ADCxx1C_MODEL(_name, _bits)				\
+	static const struct adcxx1c_model _name ## _model = {		\
+		.bits = (_bits),					\
+	}
+
+DEFINE_ADCxx1C_MODEL(adc081c,  8);
+DEFINE_ADCxx1C_MODEL(adc101c, 10);
+DEFINE_ADCxx1C_MODEL(adc121c, 12);
+
 static const struct iio_info adc081c_info = {
 	.read_raw = adc081c_read_raw,
 	.driver_module = THIS_MODULE,
@@ -70,6 +98,7 @@ static int adc081c_probe(struct i2c_client *client,
 {
 	struct iio_dev *iio;
 	struct adc081c *adc;
+	struct adcxx1c_model *model = (struct adcxx1c_model*)id->driver_data;
 	int err;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
@@ -81,6 +110,7 @@ static int adc081c_probe(struct i2c_client *client,
 
 	adc = iio_priv(iio);
 	adc->i2c = client;
+	adc->bits = model->bits;
 
 	adc->ref = devm_regulator_get(&client->dev, "vref");
 	if (IS_ERR(adc->ref))
@@ -124,7 +154,9 @@ static int adc081c_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id adc081c_id[] = {
-	{ "adc081c", 0 },
+	{ "adc081c", (long)&adc081c_model },
+	{ "adc101c", (long)&adc101c_model },
+	{ "adc121c", (long)&adc121c_model },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adc081c_id);
@@ -132,6 +164,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 +183,5 @@ static struct i2c_driver adc081c_driver = {
 module_i2c_driver(adc081c_driver);
 
 MODULE_AUTHOR("Thierry Reding <thierry.reding@avionic-design.de>");
-MODULE_DESCRIPTION("Texas Instruments ADC081C021/027 driver");
+MODULE_DESCRIPTION("Texas Instruments ADC081C/ADC101C/ADC121C driver");
 MODULE_LICENSE("GPL v2");
-- 
2.5.5

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

* [PATCH v2 2/2] ti-adc081c: Initial triggered buffer support
  2016-04-04 18:21 [PATCH v2 0/2] Add support for adc101c* and adc121c* Crestez Dan Leonard
  2016-04-04 18:21 ` [PATCH v2 1/2] ti-adc081c: " Crestez Dan Leonard
@ 2016-04-04 18:21 ` Crestez Dan Leonard
  2016-04-04 18:39   ` kbuild test robot
  2016-04-10 14:29   ` Jonathan Cameron
  1 sibling, 2 replies; 6+ messages in thread
From: Crestez Dan Leonard @ 2016-04-04 18:21 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio
  Cc: linux-kernel, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Daniel Baluta, 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
offer some sort of interrupt on data ready. Interrupts can only trigger when
values get out of a specific range.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/adc/ti-adc081c.c | 63 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
index 3b3c656..d024df2 100644
--- a/drivers/iio/adc/ti-adc081c.c
+++ b/drivers/iio/adc/ti-adc081c.c
@@ -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,18 +72,31 @@ 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),
-};
+#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,				\
+	},							\
+}
 
 struct adcxx1c_model {
+	const struct iio_chan_spec* channels;
 	int bits;
 };
 
 #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 = {		\
+		.channels = _name ## _channels,				\
 		.bits = (_bits),					\
 	}
 
@@ -88,11 +104,31 @@ DEFINE_ADCxx1C_MODEL(adc081c,  8);
 DEFINE_ADCxx1C_MODEL(adc101c, 10);
 DEFINE_ADCxx1C_MODEL(adc121c, 12);
 
+#define ADC081C_NUM_CHANNELS 2
+
 static const struct iio_info adc081c_info = {
 	.read_raw = adc081c_read_raw,
 	.driver_module = THIS_MODULE,
 };
 
+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);
+	u16 buf[indio_dev->scan_bytes / 2];
+	int ret;
+
+	ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
+	if (ret < 0)
+		goto out;
+	buf[0] = ret;
+	iio_push_to_buffers_with_timestamp(indio_dev, buf, iio_get_time_ns());
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
 static int adc081c_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -125,18 +161,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 = ADC081C_NUM_CHANNELS;
+
+	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;
@@ -148,6 +192,7 @@ static int adc081c_remove(struct i2c_client *client)
 	struct adc081c *adc = iio_priv(iio);
 
 	iio_device_unregister(iio);
+	iio_triggered_buffer_cleanup(iio);
 	regulator_disable(adc->ref);
 
 	return 0;
-- 
2.5.5

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

* Re: [PATCH v2 2/2] ti-adc081c: Initial triggered buffer support
  2016-04-04 18:21 ` [PATCH v2 2/2] ti-adc081c: Initial triggered buffer support Crestez Dan Leonard
@ 2016-04-04 18:39   ` kbuild test robot
  2016-04-10 14:29   ` Jonathan Cameron
  1 sibling, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2016-04-04 18:39 UTC (permalink / raw)
  To: Crestez Dan Leonard
  Cc: kbuild-all, Jonathan Cameron, linux-iio, linux-kernel,
	Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Daniel Baluta, Crestez Dan Leonard

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

Hi Crestez,

[auto build test WARNING on iio/togreg]
[also build test WARNING on v4.6-rc2 next-20160404]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Crestez-Dan-Leonard/Add-support-for-adc101c-and-adc121c/20160405-022459
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: s390-allmodconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

All warnings (new ones prefixed by >>):

   drivers/iio/adc/ti-adc081c.c: In function 'adc081c_trigger_handler':
>> drivers/iio/adc/ti-adc081c.c:130:1: warning: 'adc081c_trigger_handler' uses dynamic stack allocation
    }
    ^

vim +/adc081c_trigger_handler +130 drivers/iio/adc/ti-adc081c.c

   114	static irqreturn_t adc081c_trigger_handler(int irq, void *p)
   115	{
   116		struct iio_poll_func *pf = p;
   117		struct iio_dev *indio_dev = pf->indio_dev;
   118		struct adc081c *data = iio_priv(indio_dev);
   119		u16 buf[indio_dev->scan_bytes / 2];
   120		int ret;
   121	
   122		ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
   123		if (ret < 0)
   124			goto out;
   125		buf[0] = ret;
   126		iio_push_to_buffers_with_timestamp(indio_dev, buf, iio_get_time_ns());
   127	out:
   128		iio_trigger_notify_done(indio_dev->trig);
   129		return IRQ_HANDLED;
 > 130	}
   131	
   132	static int adc081c_probe(struct i2c_client *client,
   133				 const struct i2c_device_id *id)
   134	{
   135		struct iio_dev *iio;
   136		struct adc081c *adc;
   137		struct adcxx1c_model *model = (struct adcxx1c_model*)id->driver_data;
   138		int err;

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 40033 bytes --]

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

* Re: [PATCH v2 1/2] ti-adc081c: Add support for adc101c* and adc121c*
  2016-04-04 18:21 ` [PATCH v2 1/2] ti-adc081c: " Crestez Dan Leonard
@ 2016-04-10 14:25   ` Jonathan Cameron
  0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2016-04-10 14:25 UTC (permalink / raw)
  To: Crestez Dan Leonard, linux-iio
  Cc: linux-kernel, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Daniel Baluta

On 04/04/16 19:21, Crestez Dan Leonard wrote:
> 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
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
A trivial little request inline to move over to an array of structures
describing the different parts and use an enum indexing that instead of pointers in the
i2c_device_id tables.

Otherwise looks good.
> ---
>  drivers/iio/adc/Kconfig      |  6 +++---
>  drivers/iio/adc/ti-adc081c.c | 42 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 9ddcd5d..a2d0db5 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -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.
> diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
> index ecbc121..3b3c656 100644
> --- a/drivers/iio/adc/ti-adc081c.c
> +++ b/drivers/iio/adc/ti-adc081c.c
> @@ -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;
>  
> @@ -60,6 +75,19 @@ static const struct iio_chan_spec adc081c_channel = {
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>  };
>  
> +struct adcxx1c_model {
> +	int bits;
> +};
> +
> +#define DEFINE_ADCxx1C_MODEL(_name, _bits)				\
> +	static const struct adcxx1c_model _name ## _model = {		\
> +		.bits = (_bits),					\
> +	}
> +
> +DEFINE_ADCxx1C_MODEL(adc081c,  8);
> +DEFINE_ADCxx1C_MODEL(adc101c, 10);
> +DEFINE_ADCxx1C_MODEL(adc121c, 12);
As suggested below, I'd prefer these were elements of an array of such structures
as it tends to lead to cleaner / more obvious code in my view.
> +
>  static const struct iio_info adc081c_info = {
>  	.read_raw = adc081c_read_raw,
>  	.driver_module = THIS_MODULE,
> @@ -70,6 +98,7 @@ static int adc081c_probe(struct i2c_client *client,
>  {
>  	struct iio_dev *iio;
>  	struct adc081c *adc;
> +	struct adcxx1c_model *model = (struct adcxx1c_model*)id->driver_data;
>  	int err;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> @@ -81,6 +110,7 @@ static int adc081c_probe(struct i2c_client *client,
>  
>  	adc = iio_priv(iio);
>  	adc->i2c = client;
> +	adc->bits = model->bits;
>  
>  	adc->ref = devm_regulator_get(&client->dev, "vref");
>  	if (IS_ERR(adc->ref))
> @@ -124,7 +154,9 @@ static int adc081c_remove(struct i2c_client *client)
>  }
>  
>  static const struct i2c_device_id adc081c_id[] = {
> -	{ "adc081c", 0 },
> +	{ "adc081c", (long)&adc081c_model },
> +	{ "adc101c", (long)&adc101c_model },
> +	{ "adc121c", (long)&adc121c_model },
It's often cleaner to use an enum instead of direct pointers.  Avoids some nasty
casting like you have needed to do here.  The enum can then be used to reference
into an array of model description structures.
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, adc081c_id);
> @@ -132,6 +164,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 +183,5 @@ static struct i2c_driver adc081c_driver = {
>  module_i2c_driver(adc081c_driver);
>  
>  MODULE_AUTHOR("Thierry Reding <thierry.reding@avionic-design.de>");
> -MODULE_DESCRIPTION("Texas Instruments ADC081C021/027 driver");
> +MODULE_DESCRIPTION("Texas Instruments ADC081C/ADC101C/ADC121C driver");
>  MODULE_LICENSE("GPL v2");
> 

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

* Re: [PATCH v2 2/2] ti-adc081c: Initial triggered buffer support
  2016-04-04 18:21 ` [PATCH v2 2/2] ti-adc081c: Initial triggered buffer support Crestez Dan Leonard
  2016-04-04 18:39   ` kbuild test robot
@ 2016-04-10 14:29   ` Jonathan Cameron
  1 sibling, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2016-04-10 14:29 UTC (permalink / raw)
  To: Crestez Dan Leonard, linux-iio
  Cc: linux-kernel, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Daniel Baluta

On 04/04/16 19:21, Crestez Dan Leonard wrote:
> Using this requires software triggers like CONFIG_IIO_HRTIMER_TRIGGER.
> 
> The device can be configured to do internal periodic sampling but does not
> offer some sort of interrupt on data ready. Interrupts can only trigger when
> values get out of a specific range.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
basically fine, but just use a fixed size allocation for your buffer in the
trigger handler.
> ---
>  drivers/iio/adc/ti-adc081c.c | 63 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 54 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
> index 3b3c656..d024df2 100644
> --- a/drivers/iio/adc/ti-adc081c.c
> +++ b/drivers/iio/adc/ti-adc081c.c
> @@ -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,18 +72,31 @@ 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),
> -};
> +#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,				\
> +	},							\
> +}
>  
>  struct adcxx1c_model {
> +	const struct iio_chan_spec* channels;
>  	int bits;
>  };
>  
>  #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 = {		\
> +		.channels = _name ## _channels,				\
>  		.bits = (_bits),					\
>  	}
>  
> @@ -88,11 +104,31 @@ DEFINE_ADCxx1C_MODEL(adc081c,  8);
>  DEFINE_ADCxx1C_MODEL(adc101c, 10);
>  DEFINE_ADCxx1C_MODEL(adc121c, 12);
>  
> +#define ADC081C_NUM_CHANNELS 2
> +
>  static const struct iio_info adc081c_info = {
>  	.read_raw = adc081c_read_raw,
>  	.driver_module = THIS_MODULE,
>  };
>  
> +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);
> +	u16 buf[indio_dev->scan_bytes / 2];
Just use a fixed size that is beig enough for whatever you may
get.  Here you need room for a word + an aligned 8 byte timestamp
so 16 bytes, or 8 u16s.  It's not variable so don't let the compiler think
it is.

Also you shouldn't be accessing indio_dev->scan_bytes directly in a driver.
A long time back we spent a while pulling that idiom out of drivers as it
was causing a lot more trouble than the convenience was worth (can't
actually recall why now though!)
> +	int ret;
> +
> +	ret = i2c_smbus_read_word_swapped(data->i2c, REG_CONV_RES);
> +	if (ret < 0)
> +		goto out;
> +	buf[0] = ret;
> +	iio_push_to_buffers_with_timestamp(indio_dev, buf, iio_get_time_ns());
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
>  static int adc081c_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -125,18 +161,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 = ADC081C_NUM_CHANNELS;
I'm not sure the define adds anything here!  Kind of obvious what 2 means anyway -
rather than it being a magic number :)
> +
> +	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;
> @@ -148,6 +192,7 @@ static int adc081c_remove(struct i2c_client *client)
>  	struct adc081c *adc = iio_priv(iio);
>  
>  	iio_device_unregister(iio);
> +	iio_triggered_buffer_cleanup(iio);
>  	regulator_disable(adc->ref);
>  
>  	return 0;
> 

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

end of thread, other threads:[~2016-04-10 14:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-04 18:21 [PATCH v2 0/2] Add support for adc101c* and adc121c* Crestez Dan Leonard
2016-04-04 18:21 ` [PATCH v2 1/2] ti-adc081c: " Crestez Dan Leonard
2016-04-10 14:25   ` Jonathan Cameron
2016-04-04 18:21 ` [PATCH v2 2/2] ti-adc081c: Initial triggered buffer support Crestez Dan Leonard
2016-04-04 18:39   ` kbuild test robot
2016-04-10 14:29   ` Jonathan Cameron

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