Add support for adc101c* and adc121c*
v2 cover Add support for adc101c* and adc121c*
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(-)
Signature · 1 line
2.5.5
PATCH v2 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 | 42 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 41 insertions(+), 7 deletions(-)
drivers/iio/adc/Kconfig +3 -3
@@ -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 +38 -4
103 lines not on this page · raw message
Signature · 1 line
2.5.5
Jonathan Cameron
maintainerRe: [PATCH v2 1/2] ti-adc081c: Add support for adc101c* and adc121c*
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.pdfSigned-off-by Crestez Dan Leonard <[email protected]>
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.
Quoted 84 lines
--- drivers/iio/adc/Kconfig | 6 +++--- drivers/iio/adc/ti-adc081c.c | 42 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 41 insertions(+), 7 deletions(-)drivers/iio/adc/Kconfig +3
-3@@ -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 +19
-2@@ -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;
drivers/iio/adc/ti-adc081c.c static const struct iio_chan_spec adc081c_channel = { · line 89
Quoted 10 lines
drivers/iio/adc/ti-adc081c.c +10
-0+}; + +#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.
Quoted 21 lines
drivers/iio/adc/ti-adc081c.c +3
-0+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))
drivers/iio/adc/ti-adc081c.c static int adc081c_remove(struct i2c_client *client) · line 159
Quoted 7 lines
drivers/iio/adc/ti-adc081c.c +3
-1@@ -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.
Quoted 20 lines
drivers/iio/adc/ti-adc081c.c +3
-1{ } };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 <[email protected]>" ); -MODULE_DESCRIPTION ("Texas Instruments ADC081C021/027 driver" ); +MODULE_DESCRIPTION ("Texas Instruments ADC081C/ADC101C/ADC121C driver" );MODULE_LICENSE ("GPL v2" );
PATCH v2 2/2 ti-adc081c: Initial triggered buffer support
In reply to Crestez Dan LeonardUsing 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.
--- drivers/iio/adc/ti-adc081c.c | 63 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 9 deletions(-)
drivers/iio/adc/ti-adc081c.c +54 -9
@@ -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 ;
Signature · 1 line
2.5.5
kbuild test robot
build botRe: [PATCH v2 2/2] ti-adc081c: Initial triggered buffer support
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…
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
Jonathan Cameron
maintainerRe: [PATCH v2 2/2] ti-adc081c: Initial triggered buffer support
In reply to Crestez Dan LeonardQuoted 7 lines · 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 <[email protected]>
basically fine, but just use a fixed size allocation for your buffer in the trigger handler.
Quoted 62 lines
--- drivers/iio/adc/ti-adc081c.c | 63 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 9 deletions(-)drivers/iio/adc/ti-adc081c.c +23
-5@@ -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 = {
drivers/iio/adc/ti-adc081c.c DEFINE_ADCxx1C_MODEL(adc081c, 8); · line 119
Quoted 10 lines
drivers/iio/adc/ti-adc081c.c +6
-0.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!)
Quoted 16 lines
drivers/iio/adc/ti-adc081c.c +12
-0+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) {
drivers/iio/adc/ti-adc081c.c static int adc081c_probe(struct i2c_client *client, · line 165
Quoted 7 lines
drivers/iio/adc/ti-adc081c.c +2
-2@@ -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 :)
Quoted 32 lines
drivers/iio/adc/ti-adc081c.c +11
-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;@@ -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 ;