* [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 a/d
@ 2011-07-18 7:46 Paul Thomas
2011-07-18 11:01 ` [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 Jonathan Cameron
2011-07-18 21:45 ` Jonathan Cameron
0 siblings, 2 replies; 12+ messages in thread
From: Paul Thomas @ 2011-07-18 7:46 UTC (permalink / raw)
To: lm-sensors
This uses the iio sysfs interface, and inculdes gain and differential settings
Signed-off-by: Paul Thomas <pthomas8589@gmail.com>
---
drivers/staging/iio/adc/Kconfig | 7 +
drivers/staging/iio/adc/Makefile | 1 +
drivers/staging/iio/adc/ad7194.c | 462 ++++++++++++++++++++++++++++++++++++++
3 files changed, 470 insertions(+), 0 deletions(-)
create mode 100644 drivers/staging/iio/adc/ad7194.c
diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
index 8c751c4..871605b 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -17,6 +17,13 @@ config AD7152
Say yes here to build support for Analog Devices capacitive sensors.
(ad7152, ad7153) Provides direct access via sysfs.
+config AD7194
+ tristate "Analog Devices AD7194 ADC driver"
+ depends on SPI
+ help
+ Say yes here to build support for Analog Devices ad7194.
+ Provides direct access via sysfs.
+
config AD7291
tristate "Analog Devices AD7291 temperature sensor driver"
depends on I2C
diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
index 1d9b3f5..4da3c40 100644
--- a/drivers/staging/iio/adc/Makefile
+++ b/drivers/staging/iio/adc/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_AD7298) += ad7298.o
obj-$(CONFIG_AD7150) += ad7150.o
obj-$(CONFIG_AD7152) += ad7152.o
+obj-$(CONFIG_AD7194) += ad7194.o
obj-$(CONFIG_AD7291) += ad7291.o
obj-$(CONFIG_AD7314) += ad7314.o
obj-$(CONFIG_AD7745) += ad7745.o
diff --git a/drivers/staging/iio/adc/ad7194.c b/drivers/staging/iio/adc/ad7194.c
new file mode 100644
index 0000000..a867662
--- /dev/null
+++ b/drivers/staging/iio/adc/ad7194.c
@@ -0,0 +1,462 @@
+/*
+ * ad7194 - driver for Analog Devices AD7194 A/D converter
+ *
+ * Copyright (c) 2011 Paul Thomas <pthomas8589@gmail.com>
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or
+ * later as publishhed by the Free Software Foundation.
+ *
+ * You need to have something like this in struct spi_board_info
+ * {
+ * .modalias = "ad7194",
+ * .max_speed_hz = 2*1000*1000,
+ * .chip_select = 0,
+ * .bus_num = 1,
+ * },
+ *
+ * Three sysfs files are used chX_value, chX_diff and chX_gain
+ * chX_value is read only and returns that channels value in volts.
+ * chX_gain is read/write, the chX_value is scaled by the gain so it retains
+ * it's proper units.
+ * chX_diff is read/write, if chX_diff is 0 then pseude mod is used,
+ * if chX_diff is 1 then differential mode is used.
+ * In this mode ch1_value selects ch1-ch2, and ch2_value selects ch2-ch1
+ */
+
+#define DEBUG 1
+
+/*From Table 15 in the datasheet*/
+/*Register addresses*/
+#define REG_STATUS 0
+#define REG_MODE 1
+#define REG_CONF 2
+#define REG_DATA 3
+#define REG_ID 4
+#define REG_GPOCON 5
+#define REG_OFFSET 6
+#define REG_FS 7
+
+/*From Table 15 in the datasheet*/
+#define COMM_ADDR_bp 3
+#define COMM_READ_bm (1 << 6)
+
+#define CONF_CHOP_bm (1 << 23)
+#define CONF_PSEUDO_bm (1 << 18)
+#define CONF_BUF_bm (1 << 4)
+#define CONF_CHAN_NEG_bp 8
+#define CONF_CHAN_POS_bp 12
+
+
+#define MODE_MD_SINGLE_gc (0x01 << 21)
+#define MODE_MD_ZS_CAL_gc (0x04 << 21)
+#define MODE_MD_FS_CAL_gc (0x05 << 21)
+#define MODE_CLK_INTTRI_gc (0x02 << 18)
+/*Table 8 in the datasheet provides options for the Filter Word*/
+#define MODE_FILTER_WORD 1
+#define SETTLE_MS 2
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/spi/spi.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+
+#include "../iio.h"
+#include "../sysfs.h"
+
+#define DEVICE_NAME "ad7194"
+#define NUM_CHANNELS 16
+
+const uint8_t gains[NUM_CHANNELS] = {1, 0, 0, 8, 16, 32, 64, 128};
+
+struct ad7194_data {
+ struct spi_device *spi_dev;
+ struct iio_dev *indio_dev;
+ uint8_t gain[NUM_CHANNELS];
+ uint8_t diff[NUM_CHANNELS];
+ struct mutex lock;
+};
+
+static ssize_t show_gain(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *dev_info = dev_get_drvdata(dev);
+ struct ad7194_data *pdata = dev_info->dev_data;
+ struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+
+ return sprintf(buf, "%d\n", gains[pdata->gain[this_attr->address]]);
+}
+
+static ssize_t set_gain(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
+{
+ uint8_t gain_real, i;
+ struct iio_dev *dev_info = dev_get_drvdata(dev);
+ struct ad7194_data *pdata = dev_info->dev_data;
+ struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+
+ gain_real = simple_strtol(buf, NULL, 10);
+ if (gain_real = 0)
+ return -EPERM;
+ for (i = 0; i < NUM_CHANNELS; i++) {
+ if (gains[i] = gain_real) {
+ pdata->gain[this_attr->address] = i;
+ return count;
+ }
+ }
+ return -EPERM;
+}
+
+static ssize_t show_diff(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *dev_info = dev_get_drvdata(dev);
+ struct ad7194_data *pdata = dev_info->dev_data;
+ struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+
+ return sprintf(buf, "%d\n", pdata->diff[this_attr->address]);
+}
+
+static ssize_t set_diff(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
+{
+ uint8_t diff;
+ struct iio_dev *dev_info = dev_get_drvdata(dev);
+ struct ad7194_data *pdata = dev_info->dev_data;
+ struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+
+ diff = simple_strtol(buf, NULL, 10);
+ if (diff)
+ pdata->diff[this_attr->address] = 1;
+ else
+ pdata->diff[this_attr->address] = 0;
+ return count;
+}
+
+static inline int ad7194_read_reg8(struct spi_device *spi,
+ int reg, uint8_t *val)
+{
+ int ret;
+ uint8_t tx[1], rx[1];
+
+ tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
+
+ ret = spi_write_then_read(spi, tx, 1, rx, 1);
+ *val = rx[0];
+ return ret;
+}
+
+static inline int ad7194_read_reg24(struct spi_device *spi,
+ int reg, uint32_t *val)
+{
+ int ret;
+ uint8_t tx[1], rx[3];
+
+ tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
+
+ ret = spi_write_then_read(spi, tx, 1, rx, 3);
+ *val = (rx[0] << 16) + (rx[1] << 8) + rx[2];
+ return ret;
+}
+
+static inline int ad7194_write_reg24(struct spi_device *spi,
+ int reg, uint32_t *val)
+{
+ uint8_t tx[4];
+
+ tx[0] = (reg << COMM_ADDR_bp);
+ tx[1] = (*val >> 16) & 0xff;
+ tx[2] = (*val >> 8) & 0xff;
+ tx[3] = (*val >> 0) & 0xff;
+
+ return spi_write(spi, tx, 4);
+}
+
+static ssize_t show_voltage(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ uint32_t conf, mode, val, sign, pos_chan, neg_chan = 0;
+ uint8_t status, gain, diff;
+ int32_t whole, fract;
+ int ret;
+
+ struct iio_dev *dev_info = dev_get_drvdata(dev);
+ struct ad7194_data *pdata = dev_info->dev_data;
+ struct spi_device *spi = pdata->spi_dev;
+ struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+
+ pos_chan = this_attr->address;
+ gain = pdata->gain[this_attr->address];
+ diff = pdata->diff[this_attr->address];
+
+ if (diff = 1) {
+ /*if in0_diff is true, reading in0_input still returns
+ * in0, but it is in0-in1, if you read in1_input
+ * then you get in1-in0 */
+ if ((pos_chan % 2) = 0)
+ neg_chan = pos_chan+1;
+ else
+ neg_chan = pos_chan-1;
+
+ conf = CONF_CHOP_bm | CONF_BUF_bm |
+ (pos_chan << CONF_CHAN_POS_bp) |
+ (neg_chan << CONF_CHAN_NEG_bp) | gain;
+ } else {
+ conf = CONF_CHOP_bm | CONF_PSEUDO_bm | CONF_BUF_bm |
+ (pos_chan << CONF_CHAN_POS_bp) | gain;
+ }
+
+ ret = mutex_lock_interruptible(&pdata->lock);
+ if (ret != 0)
+ return ret;
+
+ ret = ad7194_write_reg24(spi, REG_CONF, &conf);
+ if (ret != 0)
+ goto out;
+
+ mode = MODE_MD_SINGLE_gc | MODE_CLK_INTTRI_gc | MODE_FILTER_WORD;
+ ret = ad7194_write_reg24(spi, REG_MODE, &mode);
+ if (ret != 0)
+ goto out;
+
+ msleep_interruptible(SETTLE_MS);
+
+ ret = ad7194_read_reg8(spi, REG_STATUS, &status);
+ if (ret != 0)
+ goto out;
+ status = (status >> 6) & 0x01;
+
+ ret = ad7194_read_reg24(spi, REG_DATA, &val);
+ if (ret != 0)
+ goto out;
+
+ sign = (val & 0x800000) >> 23;
+ if (sign)
+ fract = (val & 0x7fffff);
+ else
+ fract = 0x7fffff - (val & 0x7fffff);
+ fract = ((int64_t)fract*4095000) >> 23;
+ fract = fract / gains[gain];
+ whole = fract / 1000000;
+ fract = fract % 1000000;
+
+ if (status = 0) {
+ mutex_unlock(&pdata->lock);
+ if (sign)
+ return sprintf(buf, "%d.%.6d\n", whole, fract);
+ else
+ return sprintf(buf, "-%d.%.6d\n", whole, fract);
+ } else {
+ ret = -EAGAIN;
+ goto out;
+ }
+
+out:
+ mutex_unlock(&pdata->lock);
+ return ret;
+}
+
+static IIO_DEVICE_ATTR(ch1_value, S_IRUGO, show_voltage, NULL, 0);
+static IIO_DEVICE_ATTR(ch2_value, S_IRUGO, show_voltage, NULL, 1);
+static IIO_DEVICE_ATTR(ch3_value, S_IRUGO, show_voltage, NULL, 2);
+static IIO_DEVICE_ATTR(ch4_value, S_IRUGO, show_voltage, NULL, 3);
+static IIO_DEVICE_ATTR(ch5_value, S_IRUGO, show_voltage, NULL, 4);
+static IIO_DEVICE_ATTR(ch6_value, S_IRUGO, show_voltage, NULL, 5);
+static IIO_DEVICE_ATTR(ch7_value, S_IRUGO, show_voltage, NULL, 6);
+static IIO_DEVICE_ATTR(ch8_value, S_IRUGO, show_voltage, NULL, 7);
+static IIO_DEVICE_ATTR(ch9_value, S_IRUGO, show_voltage, NULL, 8);
+static IIO_DEVICE_ATTR(ch10_value, S_IRUGO, show_voltage, NULL, 9);
+static IIO_DEVICE_ATTR(ch11_value, S_IRUGO, show_voltage, NULL, 10);
+static IIO_DEVICE_ATTR(ch12_value, S_IRUGO, show_voltage, NULL, 11);
+static IIO_DEVICE_ATTR(ch13_value, S_IRUGO, show_voltage, NULL, 12);
+static IIO_DEVICE_ATTR(ch14_value, S_IRUGO, show_voltage, NULL, 13);
+static IIO_DEVICE_ATTR(ch15_value, S_IRUGO, show_voltage, NULL, 14);
+static IIO_DEVICE_ATTR(ch16_value, S_IRUGO, show_voltage, NULL, 15);
+
+static IIO_DEVICE_ATTR(ch1_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 0);
+static IIO_DEVICE_ATTR(ch2_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 1);
+static IIO_DEVICE_ATTR(ch3_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 2);
+static IIO_DEVICE_ATTR(ch4_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 3);
+static IIO_DEVICE_ATTR(ch5_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 4);
+static IIO_DEVICE_ATTR(ch6_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 5);
+static IIO_DEVICE_ATTR(ch7_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 6);
+static IIO_DEVICE_ATTR(ch8_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 7);
+static IIO_DEVICE_ATTR(ch9_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 8);
+static IIO_DEVICE_ATTR(ch10_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 9);
+static IIO_DEVICE_ATTR(ch11_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 10);
+static IIO_DEVICE_ATTR(ch12_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 11);
+static IIO_DEVICE_ATTR(ch13_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 12);
+static IIO_DEVICE_ATTR(ch14_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 13);
+static IIO_DEVICE_ATTR(ch15_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 14);
+static IIO_DEVICE_ATTR(ch16_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 15);
+
+static IIO_DEVICE_ATTR(ch1_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 0);
+static IIO_DEVICE_ATTR(ch2_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 1);
+static IIO_DEVICE_ATTR(ch3_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 2);
+static IIO_DEVICE_ATTR(ch4_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 3);
+static IIO_DEVICE_ATTR(ch5_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 4);
+static IIO_DEVICE_ATTR(ch6_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 5);
+static IIO_DEVICE_ATTR(ch7_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 6);
+static IIO_DEVICE_ATTR(ch8_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 7);
+static IIO_DEVICE_ATTR(ch9_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 8);
+static IIO_DEVICE_ATTR(ch10_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 9);
+static IIO_DEVICE_ATTR(ch11_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 10);
+static IIO_DEVICE_ATTR(ch12_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 11);
+static IIO_DEVICE_ATTR(ch13_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 12);
+static IIO_DEVICE_ATTR(ch14_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 13);
+static IIO_DEVICE_ATTR(ch15_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 14);
+static IIO_DEVICE_ATTR(ch16_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 15);
+
+static struct attribute *ad7194_attributes[] = {
+ &iio_dev_attr_ch1_value.dev_attr.attr,
+ &iio_dev_attr_ch2_value.dev_attr.attr,
+ &iio_dev_attr_ch3_value.dev_attr.attr,
+ &iio_dev_attr_ch4_value.dev_attr.attr,
+ &iio_dev_attr_ch5_value.dev_attr.attr,
+ &iio_dev_attr_ch6_value.dev_attr.attr,
+ &iio_dev_attr_ch7_value.dev_attr.attr,
+ &iio_dev_attr_ch8_value.dev_attr.attr,
+ &iio_dev_attr_ch9_value.dev_attr.attr,
+ &iio_dev_attr_ch10_value.dev_attr.attr,
+ &iio_dev_attr_ch11_value.dev_attr.attr,
+ &iio_dev_attr_ch12_value.dev_attr.attr,
+ &iio_dev_attr_ch13_value.dev_attr.attr,
+ &iio_dev_attr_ch14_value.dev_attr.attr,
+ &iio_dev_attr_ch15_value.dev_attr.attr,
+ &iio_dev_attr_ch16_value.dev_attr.attr,
+ &iio_dev_attr_ch1_gain.dev_attr.attr,
+ &iio_dev_attr_ch2_gain.dev_attr.attr,
+ &iio_dev_attr_ch3_gain.dev_attr.attr,
+ &iio_dev_attr_ch4_gain.dev_attr.attr,
+ &iio_dev_attr_ch5_gain.dev_attr.attr,
+ &iio_dev_attr_ch6_gain.dev_attr.attr,
+ &iio_dev_attr_ch7_gain.dev_attr.attr,
+ &iio_dev_attr_ch8_gain.dev_attr.attr,
+ &iio_dev_attr_ch9_gain.dev_attr.attr,
+ &iio_dev_attr_ch10_gain.dev_attr.attr,
+ &iio_dev_attr_ch11_gain.dev_attr.attr,
+ &iio_dev_attr_ch12_gain.dev_attr.attr,
+ &iio_dev_attr_ch13_gain.dev_attr.attr,
+ &iio_dev_attr_ch14_gain.dev_attr.attr,
+ &iio_dev_attr_ch15_gain.dev_attr.attr,
+ &iio_dev_attr_ch16_gain.dev_attr.attr,
+ &iio_dev_attr_ch1_diff.dev_attr.attr,
+ &iio_dev_attr_ch2_diff.dev_attr.attr,
+ &iio_dev_attr_ch3_diff.dev_attr.attr,
+ &iio_dev_attr_ch4_diff.dev_attr.attr,
+ &iio_dev_attr_ch5_diff.dev_attr.attr,
+ &iio_dev_attr_ch6_diff.dev_attr.attr,
+ &iio_dev_attr_ch7_diff.dev_attr.attr,
+ &iio_dev_attr_ch8_diff.dev_attr.attr,
+ &iio_dev_attr_ch9_diff.dev_attr.attr,
+ &iio_dev_attr_ch10_diff.dev_attr.attr,
+ &iio_dev_attr_ch11_diff.dev_attr.attr,
+ &iio_dev_attr_ch12_diff.dev_attr.attr,
+ &iio_dev_attr_ch13_diff.dev_attr.attr,
+ &iio_dev_attr_ch14_diff.dev_attr.attr,
+ &iio_dev_attr_ch15_diff.dev_attr.attr,
+ &iio_dev_attr_ch16_diff.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group ad7194_attribute_group = {
+ .attrs = ad7194_attributes,
+};
+
+static const struct iio_info ad7194_info = {
+ .attrs = &ad7194_attribute_group,
+ .driver_module = THIS_MODULE,
+};
+
+static int __devinit ad7194_probe(struct spi_device *spi)
+{
+ int ret, err;
+ struct ad7194_data *pdata;
+
+ /* Configure the SPI bus */
+ spi->mode = (SPI_MODE_0);
+ spi->bits_per_word = 8;
+ spi_setup(spi);
+
+ pdata = kzalloc(sizeof(struct ad7194_data), GFP_KERNEL);
+ if (!pdata) {
+ err = -ENOMEM;
+ goto exit;
+ }
+
+ dev_set_drvdata(&spi->dev, pdata);
+
+ pdata->spi_dev = spi;
+
+ pdata->indio_dev = iio_allocate_device(0);
+ if (pdata->indio_dev = NULL) {
+ ret = -ENOMEM;
+ goto error_free;
+ }
+
+ mutex_init(&pdata->lock);
+ pdata->indio_dev->name = "ad7194";
+ pdata->indio_dev->dev.parent = &spi->dev;
+ pdata->indio_dev->info = &ad7194_info;
+ pdata->indio_dev->dev_data = (void *)pdata;
+
+ ret = iio_device_register(pdata->indio_dev);
+ if (ret)
+ goto error_free_dev;
+
+ return 0;
+
+error_free_dev:
+ iio_free_device(pdata->indio_dev);
+error_free:
+ kfree(pdata);
+exit:
+ return err;
+}
+
+static int __devexit ad7194_remove(struct spi_device *spi)
+{
+ struct ad7194_data *pdata = dev_get_drvdata(&spi->dev);
+
+ dev_set_drvdata(&spi->dev, NULL);
+ iio_device_unregister(pdata->indio_dev);
+ iio_free_device(pdata->indio_dev);
+ kfree(pdata);
+ return 0;
+}
+
+static struct spi_driver ad7194_driver = {
+ .driver = {
+ .name = DEVICE_NAME,
+ .bus = &spi_bus_type,
+ .owner = THIS_MODULE,
+ },
+
+ .probe = ad7194_probe,
+ .remove = __devexit_p(ad7194_remove),
+};
+
+static int __init ad7194_init(void)
+{
+ return spi_register_driver(&ad7194_driver);
+}
+
+static void __exit ad7194_exit(void)
+{
+ spi_unregister_driver(&ad7194_driver);
+}
+
+module_init(ad7194_init);
+module_exit(ad7194_exit);
+
+MODULE_AUTHOR("Paul Thomas <pthomas8589@gmail.com>");
+MODULE_DESCRIPTION("TI AD7194 A/D driver");
+MODULE_LICENSE("GPL");
--
1.6.2.5
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: add support for Analog Devices ad7194 a/d converter
2011-07-18 7:46 [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 a/d Paul Thomas
@ 2011-07-18 11:01 ` Jonathan Cameron
2011-07-18 21:45 ` Jonathan Cameron
1 sibling, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2011-07-18 11:01 UTC (permalink / raw)
To: Paul Thomas
Cc: lm-sensors, device-drivers-devel@blackfin.uclinux.org,
linux-iio@vger.kernel.org
cc'ing linux-iio and AD's driver list.
Any particular reason for posting to lm-sensors? Now it's there we'll
keep them in the list though as someone might be interested.
On 07/18/11 08:46, Paul Thomas wrote:
> This uses the iio sysfs interface, and inculdes gain and differential settings
>
> Signed-off-by: Paul Thomas <pthomas8589@gmail.com>
> ---
> drivers/staging/iio/adc/Kconfig | 7 +
> drivers/staging/iio/adc/Makefile | 1 +
> drivers/staging/iio/adc/ad7194.c | 462 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 470 insertions(+), 0 deletions(-)
> create mode 100644 drivers/staging/iio/adc/ad7194.c
>
> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
> index 8c751c4..871605b 100644
> --- a/drivers/staging/iio/adc/Kconfig
> +++ b/drivers/staging/iio/adc/Kconfig
> @@ -17,6 +17,13 @@ config AD7152
> Say yes here to build support for Analog Devices capacitive sensors.
> (ad7152, ad7153) Provides direct access via sysfs.
>
> +config AD7194
> + tristate "Analog Devices AD7194 ADC driver"
> + depends on SPI
> + help
> + Say yes here to build support for Analog Devices ad7194.
> + Provides direct access via sysfs.
> +
> config AD7291
> tristate "Analog Devices AD7291 temperature sensor driver"
> depends on I2C
> diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
> index 1d9b3f5..4da3c40 100644
> --- a/drivers/staging/iio/adc/Makefile
> +++ b/drivers/staging/iio/adc/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_AD7298) += ad7298.o
>
> obj-$(CONFIG_AD7150) += ad7150.o
> obj-$(CONFIG_AD7152) += ad7152.o
> +obj-$(CONFIG_AD7194) += ad7194.o
> obj-$(CONFIG_AD7291) += ad7291.o
> obj-$(CONFIG_AD7314) += ad7314.o
> obj-$(CONFIG_AD7745) += ad7745.o
> diff --git a/drivers/staging/iio/adc/ad7194.c b/drivers/staging/iio/adc/ad7194.c
> new file mode 100644
> index 0000000..a867662
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7194.c
> @@ -0,0 +1,462 @@
> +/*
> + * ad7194 - driver for Analog Devices AD7194 A/D converter
> + *
> + * Copyright (c) 2011 Paul Thomas <pthomas8589@gmail.com>
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or
> + * later as publishhed by the Free Software Foundation.
> + *
> + * You need to have something like this in struct spi_board_info
> + * {
> + * .modalias = "ad7194",
> + * .max_speed_hz = 2*1000*1000,
> + * .chip_select = 0,
> + * .bus_num = 1,
> + * },
> + *
> + * Three sysfs files are used chX_value, chX_diff and chX_gain
> + * chX_value is read only and returns that channels value in volts.
> + * chX_gain is read/write, the chX_value is scaled by the gain so it retains
> + * it's proper units.
> + * chX_diff is read/write, if chX_diff is 0 then pseude mod is used,
> + * if chX_diff is 1 then differential mode is used.
> + * In this mode ch1_value selects ch1-ch2, and ch2_value selects ch2-ch1
> + */
> +
> +#define DEBUG 1
> +
> +/*From Table 15 in the datasheet*/
> +/*Register addresses*/
> +#define REG_STATUS 0
> +#define REG_MODE 1
> +#define REG_CONF 2
> +#define REG_DATA 3
> +#define REG_ID 4
> +#define REG_GPOCON 5
> +#define REG_OFFSET 6
> +#define REG_FS 7
> +
> +/*From Table 15 in the datasheet*/
> +#define COMM_ADDR_bp 3
> +#define COMM_READ_bm (1 << 6)
> +
> +#define CONF_CHOP_bm (1 << 23)
> +#define CONF_PSEUDO_bm (1 << 18)
> +#define CONF_BUF_bm (1 << 4)
> +#define CONF_CHAN_NEG_bp 8
> +#define CONF_CHAN_POS_bp 12
> +
> +
> +#define MODE_MD_SINGLE_gc (0x01 << 21)
> +#define MODE_MD_ZS_CAL_gc (0x04 << 21)
> +#define MODE_MD_FS_CAL_gc (0x05 << 21)
> +#define MODE_CLK_INTTRI_gc (0x02 << 18)
> +/*Table 8 in the datasheet provides options for the Filter Word*/
> +#define MODE_FILTER_WORD 1
> +#define SETTLE_MS 2
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/spi/spi.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +
> +#include "../iio.h"
> +#include "../sysfs.h"
> +
> +#define DEVICE_NAME "ad7194"
> +#define NUM_CHANNELS 16
> +
> +const uint8_t gains[NUM_CHANNELS] = {1, 0, 0, 8, 16, 32, 64, 128};
> +
> +struct ad7194_data {
> + struct spi_device *spi_dev;
> + struct iio_dev *indio_dev;
> + uint8_t gain[NUM_CHANNELS];
> + uint8_t diff[NUM_CHANNELS];
> + struct mutex lock;
> +};
> +
> +static ssize_t show_gain(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7194_data *pdata = dev_info->dev_data;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + return sprintf(buf, "%d\n", gains[pdata->gain[this_attr->address]]);
> +}
> +
> +static ssize_t set_gain(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + uint8_t gain_real, i;
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7194_data *pdata = dev_info->dev_data;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + gain_real = simple_strtol(buf, NULL, 10);
> + if (gain_real == 0)
> + return -EPERM;
> + for (i = 0; i < NUM_CHANNELS; i++) {
> + if (gains[i] == gain_real) {
> + pdata->gain[this_attr->address] = i;
> + return count;
> + }
> + }
> + return -EPERM;
> +}
> +
> +static ssize_t show_diff(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7194_data *pdata = dev_info->dev_data;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + return sprintf(buf, "%d\n", pdata->diff[this_attr->address]);
> +}
> +
> +static ssize_t set_diff(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + uint8_t diff;
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7194_data *pdata = dev_info->dev_data;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + diff = simple_strtol(buf, NULL, 10);
> + if (diff)
> + pdata->diff[this_attr->address] = 1;
> + else
> + pdata->diff[this_attr->address] = 0;
> + return count;
> +}
> +
> +static inline int ad7194_read_reg8(struct spi_device *spi,
> + int reg, uint8_t *val)
> +{
> + int ret;
> + uint8_t tx[1], rx[1];
> +
> + tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
> +
> + ret = spi_write_then_read(spi, tx, 1, rx, 1);
> + *val = rx[0];
> + return ret;
> +}
> +
> +static inline int ad7194_read_reg24(struct spi_device *spi,
> + int reg, uint32_t *val)
> +{
> + int ret;
> + uint8_t tx[1], rx[3];
> +
> + tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
> +
> + ret = spi_write_then_read(spi, tx, 1, rx, 3);
> + *val = (rx[0] << 16) + (rx[1] << 8) + rx[2];
> + return ret;
> +}
> +
> +static inline int ad7194_write_reg24(struct spi_device *spi,
> + int reg, uint32_t *val)
> +{
> + uint8_t tx[4];
> +
> + tx[0] = (reg << COMM_ADDR_bp);
> + tx[1] = (*val >> 16) & 0xff;
> + tx[2] = (*val >> 8) & 0xff;
> + tx[3] = (*val >> 0) & 0xff;
> +
> + return spi_write(spi, tx, 4);
> +}
> +
> +static ssize_t show_voltage(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + uint32_t conf, mode, val, sign, pos_chan, neg_chan = 0;
> + uint8_t status, gain, diff;
> + int32_t whole, fract;
> + int ret;
> +
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7194_data *pdata = dev_info->dev_data;
> + struct spi_device *spi = pdata->spi_dev;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + pos_chan = this_attr->address;
> + gain = pdata->gain[this_attr->address];
> + diff = pdata->diff[this_attr->address];
> +
> + if (diff == 1) {
> + /*if in0_diff is true, reading in0_input still returns
> + * in0, but it is in0-in1, if you read in1_input
> + * then you get in1-in0 */
> + if ((pos_chan % 2) == 0)
> + neg_chan = pos_chan+1;
> + else
> + neg_chan = pos_chan-1;
> +
> + conf = CONF_CHOP_bm | CONF_BUF_bm |
> + (pos_chan << CONF_CHAN_POS_bp) |
> + (neg_chan << CONF_CHAN_NEG_bp) | gain;
> + } else {
> + conf = CONF_CHOP_bm | CONF_PSEUDO_bm | CONF_BUF_bm |
> + (pos_chan << CONF_CHAN_POS_bp) | gain;
> + }
> +
> + ret = mutex_lock_interruptible(&pdata->lock);
> + if (ret != 0)
> + return ret;
> +
> + ret = ad7194_write_reg24(spi, REG_CONF, &conf);
> + if (ret != 0)
> + goto out;
> +
> + mode = MODE_MD_SINGLE_gc | MODE_CLK_INTTRI_gc | MODE_FILTER_WORD;
> + ret = ad7194_write_reg24(spi, REG_MODE, &mode);
> + if (ret != 0)
> + goto out;
> +
> + msleep_interruptible(SETTLE_MS);
> +
> + ret = ad7194_read_reg8(spi, REG_STATUS, &status);
> + if (ret != 0)
> + goto out;
> + status = (status >> 6) & 0x01;
> +
> + ret = ad7194_read_reg24(spi, REG_DATA, &val);
> + if (ret != 0)
> + goto out;
> +
> + sign = (val & 0x800000) >> 23;
> + if (sign)
> + fract = (val & 0x7fffff);
> + else
> + fract = 0x7fffff - (val & 0x7fffff);
> + fract = ((int64_t)fract*4095000) >> 23;
> + fract = fract / gains[gain];
> + whole = fract / 1000000;
> + fract = fract % 1000000;
> +
> + if (status == 0) {
> + mutex_unlock(&pdata->lock);
> + if (sign)
> + return sprintf(buf, "%d.%.6d\n", whole, fract);
> + else
> + return sprintf(buf, "-%d.%.6d\n", whole, fract);
> + } else {
> + ret = -EAGAIN;
> + goto out;
> + }
> +
> +out:
> + mutex_unlock(&pdata->lock);
> + return ret;
> +}
> +
> +static IIO_DEVICE_ATTR(ch1_value, S_IRUGO, show_voltage, NULL, 0);
> +static IIO_DEVICE_ATTR(ch2_value, S_IRUGO, show_voltage, NULL, 1);
> +static IIO_DEVICE_ATTR(ch3_value, S_IRUGO, show_voltage, NULL, 2);
> +static IIO_DEVICE_ATTR(ch4_value, S_IRUGO, show_voltage, NULL, 3);
> +static IIO_DEVICE_ATTR(ch5_value, S_IRUGO, show_voltage, NULL, 4);
> +static IIO_DEVICE_ATTR(ch6_value, S_IRUGO, show_voltage, NULL, 5);
> +static IIO_DEVICE_ATTR(ch7_value, S_IRUGO, show_voltage, NULL, 6);
> +static IIO_DEVICE_ATTR(ch8_value, S_IRUGO, show_voltage, NULL, 7);
> +static IIO_DEVICE_ATTR(ch9_value, S_IRUGO, show_voltage, NULL, 8);
> +static IIO_DEVICE_ATTR(ch10_value, S_IRUGO, show_voltage, NULL, 9);
> +static IIO_DEVICE_ATTR(ch11_value, S_IRUGO, show_voltage, NULL, 10);
> +static IIO_DEVICE_ATTR(ch12_value, S_IRUGO, show_voltage, NULL, 11);
> +static IIO_DEVICE_ATTR(ch13_value, S_IRUGO, show_voltage, NULL, 12);
> +static IIO_DEVICE_ATTR(ch14_value, S_IRUGO, show_voltage, NULL, 13);
> +static IIO_DEVICE_ATTR(ch15_value, S_IRUGO, show_voltage, NULL, 14);
> +static IIO_DEVICE_ATTR(ch16_value, S_IRUGO, show_voltage, NULL, 15);
> +
> +static IIO_DEVICE_ATTR(ch1_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 0);
> +static IIO_DEVICE_ATTR(ch2_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 1);
> +static IIO_DEVICE_ATTR(ch3_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 2);
> +static IIO_DEVICE_ATTR(ch4_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 3);
> +static IIO_DEVICE_ATTR(ch5_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 4);
> +static IIO_DEVICE_ATTR(ch6_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 5);
> +static IIO_DEVICE_ATTR(ch7_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 6);
> +static IIO_DEVICE_ATTR(ch8_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 7);
> +static IIO_DEVICE_ATTR(ch9_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 8);
> +static IIO_DEVICE_ATTR(ch10_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 9);
> +static IIO_DEVICE_ATTR(ch11_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 10);
> +static IIO_DEVICE_ATTR(ch12_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 11);
> +static IIO_DEVICE_ATTR(ch13_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 12);
> +static IIO_DEVICE_ATTR(ch14_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 13);
> +static IIO_DEVICE_ATTR(ch15_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 14);
> +static IIO_DEVICE_ATTR(ch16_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 15);
> +
> +static IIO_DEVICE_ATTR(ch1_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 0);
> +static IIO_DEVICE_ATTR(ch2_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 1);
> +static IIO_DEVICE_ATTR(ch3_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 2);
> +static IIO_DEVICE_ATTR(ch4_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 3);
> +static IIO_DEVICE_ATTR(ch5_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 4);
> +static IIO_DEVICE_ATTR(ch6_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 5);
> +static IIO_DEVICE_ATTR(ch7_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 6);
> +static IIO_DEVICE_ATTR(ch8_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 7);
> +static IIO_DEVICE_ATTR(ch9_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 8);
> +static IIO_DEVICE_ATTR(ch10_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 9);
> +static IIO_DEVICE_ATTR(ch11_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 10);
> +static IIO_DEVICE_ATTR(ch12_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 11);
> +static IIO_DEVICE_ATTR(ch13_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 12);
> +static IIO_DEVICE_ATTR(ch14_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 13);
> +static IIO_DEVICE_ATTR(ch15_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 14);
> +static IIO_DEVICE_ATTR(ch16_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 15);
> +
> +static struct attribute *ad7194_attributes[] = {
> + &iio_dev_attr_ch1_value.dev_attr.attr,
> + &iio_dev_attr_ch2_value.dev_attr.attr,
> + &iio_dev_attr_ch3_value.dev_attr.attr,
> + &iio_dev_attr_ch4_value.dev_attr.attr,
> + &iio_dev_attr_ch5_value.dev_attr.attr,
> + &iio_dev_attr_ch6_value.dev_attr.attr,
> + &iio_dev_attr_ch7_value.dev_attr.attr,
> + &iio_dev_attr_ch8_value.dev_attr.attr,
> + &iio_dev_attr_ch9_value.dev_attr.attr,
> + &iio_dev_attr_ch10_value.dev_attr.attr,
> + &iio_dev_attr_ch11_value.dev_attr.attr,
> + &iio_dev_attr_ch12_value.dev_attr.attr,
> + &iio_dev_attr_ch13_value.dev_attr.attr,
> + &iio_dev_attr_ch14_value.dev_attr.attr,
> + &iio_dev_attr_ch15_value.dev_attr.attr,
> + &iio_dev_attr_ch16_value.dev_attr.attr,
> + &iio_dev_attr_ch1_gain.dev_attr.attr,
> + &iio_dev_attr_ch2_gain.dev_attr.attr,
> + &iio_dev_attr_ch3_gain.dev_attr.attr,
> + &iio_dev_attr_ch4_gain.dev_attr.attr,
> + &iio_dev_attr_ch5_gain.dev_attr.attr,
> + &iio_dev_attr_ch6_gain.dev_attr.attr,
> + &iio_dev_attr_ch7_gain.dev_attr.attr,
> + &iio_dev_attr_ch8_gain.dev_attr.attr,
> + &iio_dev_attr_ch9_gain.dev_attr.attr,
> + &iio_dev_attr_ch10_gain.dev_attr.attr,
> + &iio_dev_attr_ch11_gain.dev_attr.attr,
> + &iio_dev_attr_ch12_gain.dev_attr.attr,
> + &iio_dev_attr_ch13_gain.dev_attr.attr,
> + &iio_dev_attr_ch14_gain.dev_attr.attr,
> + &iio_dev_attr_ch15_gain.dev_attr.attr,
> + &iio_dev_attr_ch16_gain.dev_attr.attr,
> + &iio_dev_attr_ch1_diff.dev_attr.attr,
> + &iio_dev_attr_ch2_diff.dev_attr.attr,
> + &iio_dev_attr_ch3_diff.dev_attr.attr,
> + &iio_dev_attr_ch4_diff.dev_attr.attr,
> + &iio_dev_attr_ch5_diff.dev_attr.attr,
> + &iio_dev_attr_ch6_diff.dev_attr.attr,
> + &iio_dev_attr_ch7_diff.dev_attr.attr,
> + &iio_dev_attr_ch8_diff.dev_attr.attr,
> + &iio_dev_attr_ch9_diff.dev_attr.attr,
> + &iio_dev_attr_ch10_diff.dev_attr.attr,
> + &iio_dev_attr_ch11_diff.dev_attr.attr,
> + &iio_dev_attr_ch12_diff.dev_attr.attr,
> + &iio_dev_attr_ch13_diff.dev_attr.attr,
> + &iio_dev_attr_ch14_diff.dev_attr.attr,
> + &iio_dev_attr_ch15_diff.dev_attr.attr,
> + &iio_dev_attr_ch16_diff.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group ad7194_attribute_group = {
> + .attrs = ad7194_attributes,
> +};
> +
> +static const struct iio_info ad7194_info = {
> + .attrs = &ad7194_attribute_group,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int __devinit ad7194_probe(struct spi_device *spi)
> +{
> + int ret, err;
> + struct ad7194_data *pdata;
> +
> + /* Configure the SPI bus */
> + spi->mode = (SPI_MODE_0);
> + spi->bits_per_word = 8;
> + spi_setup(spi);
> +
> + pdata = kzalloc(sizeof(struct ad7194_data), GFP_KERNEL);
> + if (!pdata) {
> + err = -ENOMEM;
> + goto exit;
> + }
> +
> + dev_set_drvdata(&spi->dev, pdata);
> +
> + pdata->spi_dev = spi;
> +
> + pdata->indio_dev = iio_allocate_device(0);
> + if (pdata->indio_dev == NULL) {
> + ret = -ENOMEM;
> + goto error_free;
> + }
> +
> + mutex_init(&pdata->lock);
> + pdata->indio_dev->name = "ad7194";
> + pdata->indio_dev->dev.parent = &spi->dev;
> + pdata->indio_dev->info = &ad7194_info;
> + pdata->indio_dev->dev_data = (void *)pdata;
> +
> + ret = iio_device_register(pdata->indio_dev);
> + if (ret)
> + goto error_free_dev;
> +
> + return 0;
> +
> +error_free_dev:
> + iio_free_device(pdata->indio_dev);
> +error_free:
> + kfree(pdata);
> +exit:
> + return err;
> +}
> +
> +static int __devexit ad7194_remove(struct spi_device *spi)
> +{
> + struct ad7194_data *pdata = dev_get_drvdata(&spi->dev);
> +
> + dev_set_drvdata(&spi->dev, NULL);
> + iio_device_unregister(pdata->indio_dev);
> + iio_free_device(pdata->indio_dev);
> + kfree(pdata);
> + return 0;
> +}
> +
> +static struct spi_driver ad7194_driver = {
> + .driver = {
> + .name = DEVICE_NAME,
> + .bus = &spi_bus_type,
> + .owner = THIS_MODULE,
> + },
> +
> + .probe = ad7194_probe,
> + .remove = __devexit_p(ad7194_remove),
> +};
> +
> +static int __init ad7194_init(void)
> +{
> + return spi_register_driver(&ad7194_driver);
> +}
> +
> +static void __exit ad7194_exit(void)
> +{
> + spi_unregister_driver(&ad7194_driver);
> +}
> +
> +module_init(ad7194_init);
> +module_exit(ad7194_exit);
> +
> +MODULE_AUTHOR("Paul Thomas <pthomas8589@gmail.com>");
> +MODULE_DESCRIPTION("TI AD7194 A/D driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194
@ 2011-07-18 11:01 ` Jonathan Cameron
0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2011-07-18 11:01 UTC (permalink / raw)
To: Paul Thomas
Cc: lm-sensors, device-drivers-devel@blackfin.uclinux.org,
linux-iio@vger.kernel.org
cc'ing linux-iio and AD's driver list.
Any particular reason for posting to lm-sensors? Now it's there we'll
keep them in the list though as someone might be interested.
On 07/18/11 08:46, Paul Thomas wrote:
> This uses the iio sysfs interface, and inculdes gain and differential settings
>
> Signed-off-by: Paul Thomas <pthomas8589@gmail.com>
> ---
> drivers/staging/iio/adc/Kconfig | 7 +
> drivers/staging/iio/adc/Makefile | 1 +
> drivers/staging/iio/adc/ad7194.c | 462 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 470 insertions(+), 0 deletions(-)
> create mode 100644 drivers/staging/iio/adc/ad7194.c
>
> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
> index 8c751c4..871605b 100644
> --- a/drivers/staging/iio/adc/Kconfig
> +++ b/drivers/staging/iio/adc/Kconfig
> @@ -17,6 +17,13 @@ config AD7152
> Say yes here to build support for Analog Devices capacitive sensors.
> (ad7152, ad7153) Provides direct access via sysfs.
>
> +config AD7194
> + tristate "Analog Devices AD7194 ADC driver"
> + depends on SPI
> + help
> + Say yes here to build support for Analog Devices ad7194.
> + Provides direct access via sysfs.
> +
> config AD7291
> tristate "Analog Devices AD7291 temperature sensor driver"
> depends on I2C
> diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
> index 1d9b3f5..4da3c40 100644
> --- a/drivers/staging/iio/adc/Makefile
> +++ b/drivers/staging/iio/adc/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_AD7298) += ad7298.o
>
> obj-$(CONFIG_AD7150) += ad7150.o
> obj-$(CONFIG_AD7152) += ad7152.o
> +obj-$(CONFIG_AD7194) += ad7194.o
> obj-$(CONFIG_AD7291) += ad7291.o
> obj-$(CONFIG_AD7314) += ad7314.o
> obj-$(CONFIG_AD7745) += ad7745.o
> diff --git a/drivers/staging/iio/adc/ad7194.c b/drivers/staging/iio/adc/ad7194.c
> new file mode 100644
> index 0000000..a867662
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7194.c
> @@ -0,0 +1,462 @@
> +/*
> + * ad7194 - driver for Analog Devices AD7194 A/D converter
> + *
> + * Copyright (c) 2011 Paul Thomas <pthomas8589@gmail.com>
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or
> + * later as publishhed by the Free Software Foundation.
> + *
> + * You need to have something like this in struct spi_board_info
> + * {
> + * .modalias = "ad7194",
> + * .max_speed_hz = 2*1000*1000,
> + * .chip_select = 0,
> + * .bus_num = 1,
> + * },
> + *
> + * Three sysfs files are used chX_value, chX_diff and chX_gain
> + * chX_value is read only and returns that channels value in volts.
> + * chX_gain is read/write, the chX_value is scaled by the gain so it retains
> + * it's proper units.
> + * chX_diff is read/write, if chX_diff is 0 then pseude mod is used,
> + * if chX_diff is 1 then differential mode is used.
> + * In this mode ch1_value selects ch1-ch2, and ch2_value selects ch2-ch1
> + */
> +
> +#define DEBUG 1
> +
> +/*From Table 15 in the datasheet*/
> +/*Register addresses*/
> +#define REG_STATUS 0
> +#define REG_MODE 1
> +#define REG_CONF 2
> +#define REG_DATA 3
> +#define REG_ID 4
> +#define REG_GPOCON 5
> +#define REG_OFFSET 6
> +#define REG_FS 7
> +
> +/*From Table 15 in the datasheet*/
> +#define COMM_ADDR_bp 3
> +#define COMM_READ_bm (1 << 6)
> +
> +#define CONF_CHOP_bm (1 << 23)
> +#define CONF_PSEUDO_bm (1 << 18)
> +#define CONF_BUF_bm (1 << 4)
> +#define CONF_CHAN_NEG_bp 8
> +#define CONF_CHAN_POS_bp 12
> +
> +
> +#define MODE_MD_SINGLE_gc (0x01 << 21)
> +#define MODE_MD_ZS_CAL_gc (0x04 << 21)
> +#define MODE_MD_FS_CAL_gc (0x05 << 21)
> +#define MODE_CLK_INTTRI_gc (0x02 << 18)
> +/*Table 8 in the datasheet provides options for the Filter Word*/
> +#define MODE_FILTER_WORD 1
> +#define SETTLE_MS 2
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/spi/spi.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +
> +#include "../iio.h"
> +#include "../sysfs.h"
> +
> +#define DEVICE_NAME "ad7194"
> +#define NUM_CHANNELS 16
> +
> +const uint8_t gains[NUM_CHANNELS] = {1, 0, 0, 8, 16, 32, 64, 128};
> +
> +struct ad7194_data {
> + struct spi_device *spi_dev;
> + struct iio_dev *indio_dev;
> + uint8_t gain[NUM_CHANNELS];
> + uint8_t diff[NUM_CHANNELS];
> + struct mutex lock;
> +};
> +
> +static ssize_t show_gain(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7194_data *pdata = dev_info->dev_data;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + return sprintf(buf, "%d\n", gains[pdata->gain[this_attr->address]]);
> +}
> +
> +static ssize_t set_gain(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + uint8_t gain_real, i;
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7194_data *pdata = dev_info->dev_data;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + gain_real = simple_strtol(buf, NULL, 10);
> + if (gain_real = 0)
> + return -EPERM;
> + for (i = 0; i < NUM_CHANNELS; i++) {
> + if (gains[i] = gain_real) {
> + pdata->gain[this_attr->address] = i;
> + return count;
> + }
> + }
> + return -EPERM;
> +}
> +
> +static ssize_t show_diff(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7194_data *pdata = dev_info->dev_data;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + return sprintf(buf, "%d\n", pdata->diff[this_attr->address]);
> +}
> +
> +static ssize_t set_diff(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + uint8_t diff;
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7194_data *pdata = dev_info->dev_data;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + diff = simple_strtol(buf, NULL, 10);
> + if (diff)
> + pdata->diff[this_attr->address] = 1;
> + else
> + pdata->diff[this_attr->address] = 0;
> + return count;
> +}
> +
> +static inline int ad7194_read_reg8(struct spi_device *spi,
> + int reg, uint8_t *val)
> +{
> + int ret;
> + uint8_t tx[1], rx[1];
> +
> + tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
> +
> + ret = spi_write_then_read(spi, tx, 1, rx, 1);
> + *val = rx[0];
> + return ret;
> +}
> +
> +static inline int ad7194_read_reg24(struct spi_device *spi,
> + int reg, uint32_t *val)
> +{
> + int ret;
> + uint8_t tx[1], rx[3];
> +
> + tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
> +
> + ret = spi_write_then_read(spi, tx, 1, rx, 3);
> + *val = (rx[0] << 16) + (rx[1] << 8) + rx[2];
> + return ret;
> +}
> +
> +static inline int ad7194_write_reg24(struct spi_device *spi,
> + int reg, uint32_t *val)
> +{
> + uint8_t tx[4];
> +
> + tx[0] = (reg << COMM_ADDR_bp);
> + tx[1] = (*val >> 16) & 0xff;
> + tx[2] = (*val >> 8) & 0xff;
> + tx[3] = (*val >> 0) & 0xff;
> +
> + return spi_write(spi, tx, 4);
> +}
> +
> +static ssize_t show_voltage(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + uint32_t conf, mode, val, sign, pos_chan, neg_chan = 0;
> + uint8_t status, gain, diff;
> + int32_t whole, fract;
> + int ret;
> +
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7194_data *pdata = dev_info->dev_data;
> + struct spi_device *spi = pdata->spi_dev;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + pos_chan = this_attr->address;
> + gain = pdata->gain[this_attr->address];
> + diff = pdata->diff[this_attr->address];
> +
> + if (diff = 1) {
> + /*if in0_diff is true, reading in0_input still returns
> + * in0, but it is in0-in1, if you read in1_input
> + * then you get in1-in0 */
> + if ((pos_chan % 2) = 0)
> + neg_chan = pos_chan+1;
> + else
> + neg_chan = pos_chan-1;
> +
> + conf = CONF_CHOP_bm | CONF_BUF_bm |
> + (pos_chan << CONF_CHAN_POS_bp) |
> + (neg_chan << CONF_CHAN_NEG_bp) | gain;
> + } else {
> + conf = CONF_CHOP_bm | CONF_PSEUDO_bm | CONF_BUF_bm |
> + (pos_chan << CONF_CHAN_POS_bp) | gain;
> + }
> +
> + ret = mutex_lock_interruptible(&pdata->lock);
> + if (ret != 0)
> + return ret;
> +
> + ret = ad7194_write_reg24(spi, REG_CONF, &conf);
> + if (ret != 0)
> + goto out;
> +
> + mode = MODE_MD_SINGLE_gc | MODE_CLK_INTTRI_gc | MODE_FILTER_WORD;
> + ret = ad7194_write_reg24(spi, REG_MODE, &mode);
> + if (ret != 0)
> + goto out;
> +
> + msleep_interruptible(SETTLE_MS);
> +
> + ret = ad7194_read_reg8(spi, REG_STATUS, &status);
> + if (ret != 0)
> + goto out;
> + status = (status >> 6) & 0x01;
> +
> + ret = ad7194_read_reg24(spi, REG_DATA, &val);
> + if (ret != 0)
> + goto out;
> +
> + sign = (val & 0x800000) >> 23;
> + if (sign)
> + fract = (val & 0x7fffff);
> + else
> + fract = 0x7fffff - (val & 0x7fffff);
> + fract = ((int64_t)fract*4095000) >> 23;
> + fract = fract / gains[gain];
> + whole = fract / 1000000;
> + fract = fract % 1000000;
> +
> + if (status = 0) {
> + mutex_unlock(&pdata->lock);
> + if (sign)
> + return sprintf(buf, "%d.%.6d\n", whole, fract);
> + else
> + return sprintf(buf, "-%d.%.6d\n", whole, fract);
> + } else {
> + ret = -EAGAIN;
> + goto out;
> + }
> +
> +out:
> + mutex_unlock(&pdata->lock);
> + return ret;
> +}
> +
> +static IIO_DEVICE_ATTR(ch1_value, S_IRUGO, show_voltage, NULL, 0);
> +static IIO_DEVICE_ATTR(ch2_value, S_IRUGO, show_voltage, NULL, 1);
> +static IIO_DEVICE_ATTR(ch3_value, S_IRUGO, show_voltage, NULL, 2);
> +static IIO_DEVICE_ATTR(ch4_value, S_IRUGO, show_voltage, NULL, 3);
> +static IIO_DEVICE_ATTR(ch5_value, S_IRUGO, show_voltage, NULL, 4);
> +static IIO_DEVICE_ATTR(ch6_value, S_IRUGO, show_voltage, NULL, 5);
> +static IIO_DEVICE_ATTR(ch7_value, S_IRUGO, show_voltage, NULL, 6);
> +static IIO_DEVICE_ATTR(ch8_value, S_IRUGO, show_voltage, NULL, 7);
> +static IIO_DEVICE_ATTR(ch9_value, S_IRUGO, show_voltage, NULL, 8);
> +static IIO_DEVICE_ATTR(ch10_value, S_IRUGO, show_voltage, NULL, 9);
> +static IIO_DEVICE_ATTR(ch11_value, S_IRUGO, show_voltage, NULL, 10);
> +static IIO_DEVICE_ATTR(ch12_value, S_IRUGO, show_voltage, NULL, 11);
> +static IIO_DEVICE_ATTR(ch13_value, S_IRUGO, show_voltage, NULL, 12);
> +static IIO_DEVICE_ATTR(ch14_value, S_IRUGO, show_voltage, NULL, 13);
> +static IIO_DEVICE_ATTR(ch15_value, S_IRUGO, show_voltage, NULL, 14);
> +static IIO_DEVICE_ATTR(ch16_value, S_IRUGO, show_voltage, NULL, 15);
> +
> +static IIO_DEVICE_ATTR(ch1_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 0);
> +static IIO_DEVICE_ATTR(ch2_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 1);
> +static IIO_DEVICE_ATTR(ch3_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 2);
> +static IIO_DEVICE_ATTR(ch4_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 3);
> +static IIO_DEVICE_ATTR(ch5_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 4);
> +static IIO_DEVICE_ATTR(ch6_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 5);
> +static IIO_DEVICE_ATTR(ch7_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 6);
> +static IIO_DEVICE_ATTR(ch8_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 7);
> +static IIO_DEVICE_ATTR(ch9_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 8);
> +static IIO_DEVICE_ATTR(ch10_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 9);
> +static IIO_DEVICE_ATTR(ch11_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 10);
> +static IIO_DEVICE_ATTR(ch12_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 11);
> +static IIO_DEVICE_ATTR(ch13_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 12);
> +static IIO_DEVICE_ATTR(ch14_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 13);
> +static IIO_DEVICE_ATTR(ch15_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 14);
> +static IIO_DEVICE_ATTR(ch16_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 15);
> +
> +static IIO_DEVICE_ATTR(ch1_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 0);
> +static IIO_DEVICE_ATTR(ch2_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 1);
> +static IIO_DEVICE_ATTR(ch3_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 2);
> +static IIO_DEVICE_ATTR(ch4_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 3);
> +static IIO_DEVICE_ATTR(ch5_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 4);
> +static IIO_DEVICE_ATTR(ch6_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 5);
> +static IIO_DEVICE_ATTR(ch7_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 6);
> +static IIO_DEVICE_ATTR(ch8_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 7);
> +static IIO_DEVICE_ATTR(ch9_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 8);
> +static IIO_DEVICE_ATTR(ch10_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 9);
> +static IIO_DEVICE_ATTR(ch11_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 10);
> +static IIO_DEVICE_ATTR(ch12_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 11);
> +static IIO_DEVICE_ATTR(ch13_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 12);
> +static IIO_DEVICE_ATTR(ch14_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 13);
> +static IIO_DEVICE_ATTR(ch15_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 14);
> +static IIO_DEVICE_ATTR(ch16_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 15);
> +
> +static struct attribute *ad7194_attributes[] = {
> + &iio_dev_attr_ch1_value.dev_attr.attr,
> + &iio_dev_attr_ch2_value.dev_attr.attr,
> + &iio_dev_attr_ch3_value.dev_attr.attr,
> + &iio_dev_attr_ch4_value.dev_attr.attr,
> + &iio_dev_attr_ch5_value.dev_attr.attr,
> + &iio_dev_attr_ch6_value.dev_attr.attr,
> + &iio_dev_attr_ch7_value.dev_attr.attr,
> + &iio_dev_attr_ch8_value.dev_attr.attr,
> + &iio_dev_attr_ch9_value.dev_attr.attr,
> + &iio_dev_attr_ch10_value.dev_attr.attr,
> + &iio_dev_attr_ch11_value.dev_attr.attr,
> + &iio_dev_attr_ch12_value.dev_attr.attr,
> + &iio_dev_attr_ch13_value.dev_attr.attr,
> + &iio_dev_attr_ch14_value.dev_attr.attr,
> + &iio_dev_attr_ch15_value.dev_attr.attr,
> + &iio_dev_attr_ch16_value.dev_attr.attr,
> + &iio_dev_attr_ch1_gain.dev_attr.attr,
> + &iio_dev_attr_ch2_gain.dev_attr.attr,
> + &iio_dev_attr_ch3_gain.dev_attr.attr,
> + &iio_dev_attr_ch4_gain.dev_attr.attr,
> + &iio_dev_attr_ch5_gain.dev_attr.attr,
> + &iio_dev_attr_ch6_gain.dev_attr.attr,
> + &iio_dev_attr_ch7_gain.dev_attr.attr,
> + &iio_dev_attr_ch8_gain.dev_attr.attr,
> + &iio_dev_attr_ch9_gain.dev_attr.attr,
> + &iio_dev_attr_ch10_gain.dev_attr.attr,
> + &iio_dev_attr_ch11_gain.dev_attr.attr,
> + &iio_dev_attr_ch12_gain.dev_attr.attr,
> + &iio_dev_attr_ch13_gain.dev_attr.attr,
> + &iio_dev_attr_ch14_gain.dev_attr.attr,
> + &iio_dev_attr_ch15_gain.dev_attr.attr,
> + &iio_dev_attr_ch16_gain.dev_attr.attr,
> + &iio_dev_attr_ch1_diff.dev_attr.attr,
> + &iio_dev_attr_ch2_diff.dev_attr.attr,
> + &iio_dev_attr_ch3_diff.dev_attr.attr,
> + &iio_dev_attr_ch4_diff.dev_attr.attr,
> + &iio_dev_attr_ch5_diff.dev_attr.attr,
> + &iio_dev_attr_ch6_diff.dev_attr.attr,
> + &iio_dev_attr_ch7_diff.dev_attr.attr,
> + &iio_dev_attr_ch8_diff.dev_attr.attr,
> + &iio_dev_attr_ch9_diff.dev_attr.attr,
> + &iio_dev_attr_ch10_diff.dev_attr.attr,
> + &iio_dev_attr_ch11_diff.dev_attr.attr,
> + &iio_dev_attr_ch12_diff.dev_attr.attr,
> + &iio_dev_attr_ch13_diff.dev_attr.attr,
> + &iio_dev_attr_ch14_diff.dev_attr.attr,
> + &iio_dev_attr_ch15_diff.dev_attr.attr,
> + &iio_dev_attr_ch16_diff.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group ad7194_attribute_group = {
> + .attrs = ad7194_attributes,
> +};
> +
> +static const struct iio_info ad7194_info = {
> + .attrs = &ad7194_attribute_group,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int __devinit ad7194_probe(struct spi_device *spi)
> +{
> + int ret, err;
> + struct ad7194_data *pdata;
> +
> + /* Configure the SPI bus */
> + spi->mode = (SPI_MODE_0);
> + spi->bits_per_word = 8;
> + spi_setup(spi);
> +
> + pdata = kzalloc(sizeof(struct ad7194_data), GFP_KERNEL);
> + if (!pdata) {
> + err = -ENOMEM;
> + goto exit;
> + }
> +
> + dev_set_drvdata(&spi->dev, pdata);
> +
> + pdata->spi_dev = spi;
> +
> + pdata->indio_dev = iio_allocate_device(0);
> + if (pdata->indio_dev = NULL) {
> + ret = -ENOMEM;
> + goto error_free;
> + }
> +
> + mutex_init(&pdata->lock);
> + pdata->indio_dev->name = "ad7194";
> + pdata->indio_dev->dev.parent = &spi->dev;
> + pdata->indio_dev->info = &ad7194_info;
> + pdata->indio_dev->dev_data = (void *)pdata;
> +
> + ret = iio_device_register(pdata->indio_dev);
> + if (ret)
> + goto error_free_dev;
> +
> + return 0;
> +
> +error_free_dev:
> + iio_free_device(pdata->indio_dev);
> +error_free:
> + kfree(pdata);
> +exit:
> + return err;
> +}
> +
> +static int __devexit ad7194_remove(struct spi_device *spi)
> +{
> + struct ad7194_data *pdata = dev_get_drvdata(&spi->dev);
> +
> + dev_set_drvdata(&spi->dev, NULL);
> + iio_device_unregister(pdata->indio_dev);
> + iio_free_device(pdata->indio_dev);
> + kfree(pdata);
> + return 0;
> +}
> +
> +static struct spi_driver ad7194_driver = {
> + .driver = {
> + .name = DEVICE_NAME,
> + .bus = &spi_bus_type,
> + .owner = THIS_MODULE,
> + },
> +
> + .probe = ad7194_probe,
> + .remove = __devexit_p(ad7194_remove),
> +};
> +
> +static int __init ad7194_init(void)
> +{
> + return spi_register_driver(&ad7194_driver);
> +}
> +
> +static void __exit ad7194_exit(void)
> +{
> + spi_unregister_driver(&ad7194_driver);
> +}
> +
> +module_init(ad7194_init);
> +module_exit(ad7194_exit);
> +
> +MODULE_AUTHOR("Paul Thomas <pthomas8589@gmail.com>");
> +MODULE_DESCRIPTION("TI AD7194 A/D driver");
> +MODULE_LICENSE("GPL");
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: add support for Analog Devices ad7194 a/d converter
2011-07-18 11:01 ` [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 Jonathan Cameron
@ 2011-07-18 12:24 ` Jonathan Cameron
-1 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2011-07-18 12:24 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Paul Thomas, lm-sensors,
device-drivers-devel@blackfin.uclinux.org,
linux-iio@vger.kernel.org
On 07/18/11 12:01, Jonathan Cameron wrote:
> cc'ing linux-iio and AD's driver list.
>
> Any particular reason for posting to lm-sensors? Now it's there we'll
> keep them in the list though as someone might be interested.
>
> On 07/18/11 08:46, Paul Thomas wrote:
>> This uses the iio sysfs interface, and inculdes gain and differential settings
Hi Paul,
This driver is lagging somewhat in interface terms. Having said that, it applies
and compiles fine which will make catching up to current point much easier.
If you are short on time I'm happy to do the conversion (as it is a nice simple
driver), but obviously I'll need to test it to find out what I messed up this
time.
Big issues:
1) iio_dev->dev_data is going away, so please use the iio_priv stuff.
2) interface is not terribly close the abi spec.
3) use chan_spec based registration. Actually that'll clean up the abi
issues as well and give you much shorter code.
4) differential channels are treated as separate channels (with appropriate
numbering). This is easy here as there are no nasty constraints on channel
combinations (it only reads one at a time anyway!).
Various nitpicks inline. Though the above seems like a lot, you have done
all the fiddly stuff about actually talking the the chips. Cleaning up
interfaces is relatively straight forward! Lots of fun stuff to add to this
chip at a later date, but in the spirit of your driver, lets keep it simple
for now!
As long as you are happy to do a couple of rounds of testing, we could merge
this as is and do the abi work as a series of small steps on top of it?
Thanks,
Jonathan
>>
>> Signed-off-by: Paul Thomas <pthomas8589@gmail.com>
>> ---
>> drivers/staging/iio/adc/Kconfig | 7 +
>> drivers/staging/iio/adc/Makefile | 1 +
>> drivers/staging/iio/adc/ad7194.c | 462 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 470 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/staging/iio/adc/ad7194.c
>>
>> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
>> index 8c751c4..871605b 100644
>> --- a/drivers/staging/iio/adc/Kconfig
>> +++ b/drivers/staging/iio/adc/Kconfig
>> @@ -17,6 +17,13 @@ config AD7152
>> Say yes here to build support for Analog Devices capacitive sensors.
>> (ad7152, ad7153) Provides direct access via sysfs.
>>
>> +config AD7194
>> + tristate "Analog Devices AD7194 ADC driver"
>> + depends on SPI
>> + help
>> + Say yes here to build support for Analog Devices ad7194.
>> + Provides direct access via sysfs.
>> +
>> config AD7291
>> tristate "Analog Devices AD7291 temperature sensor driver"
>> depends on I2C
>> diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
>> index 1d9b3f5..4da3c40 100644
>> --- a/drivers/staging/iio/adc/Makefile
>> +++ b/drivers/staging/iio/adc/Makefile
>> @@ -31,6 +31,7 @@ obj-$(CONFIG_AD7298) += ad7298.o
>>
>> obj-$(CONFIG_AD7150) += ad7150.o
>> obj-$(CONFIG_AD7152) += ad7152.o
>> +obj-$(CONFIG_AD7194) += ad7194.o
>> obj-$(CONFIG_AD7291) += ad7291.o
>> obj-$(CONFIG_AD7314) += ad7314.o
>> obj-$(CONFIG_AD7745) += ad7745.o
>> diff --git a/drivers/staging/iio/adc/ad7194.c b/drivers/staging/iio/adc/ad7194.c
>> new file mode 100644
>> index 0000000..a867662
>> --- /dev/null
>> +++ b/drivers/staging/iio/adc/ad7194.c
>> @@ -0,0 +1,462 @@
>> +/*
>> + * ad7194 - driver for Analog Devices AD7194 A/D converter
>> + *
>> + * Copyright (c) 2011 Paul Thomas <pthomas8589@gmail.com>
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 or
>> + * later as publishhed by the Free Software Foundation.
>> + *
>> + * You need to have something like this in struct spi_board_info
>> + * {
>> + * .modalias = "ad7194",
>> + * .max_speed_hz = 2*1000*1000,
>> + * .chip_select = 0,
>> + * .bus_num = 1,
>> + * },
>> + *
>> + * Three sysfs files are used chX_value, chX_diff and chX_gain
>> + * chX_value is read only and returns that channels value in volts.
>> + * chX_gain is read/write, the chX_value is scaled by the gain so it retains
>> + * it's proper units.
>> + * chX_diff is read/write, if chX_diff is 0 then pseude mod is used,
>> + * if chX_diff is 1 then differential mode is used.
>> + * In this mode ch1_value selects ch1-ch2, and ch2_value selects ch2-ch1
>> + */
>> +
>> +#define DEBUG 1
>> +
>> +/*From Table 15 in the datasheet*/
>> +/*Register addresses*/
>> +#define REG_STATUS 0
>> +#define REG_MODE 1
>> +#define REG_CONF 2
>> +#define REG_DATA 3
>> +#define REG_ID 4
>> +#define REG_GPOCON 5
>> +#define REG_OFFSET 6
>> +#define REG_FS 7
>> +
>> +/*From Table 15 in the datasheet*/
>> +#define COMM_ADDR_bp 3
>> +#define COMM_READ_bm (1 << 6)
>> +
>> +#define CONF_CHOP_bm (1 << 23)
>> +#define CONF_PSEUDO_bm (1 << 18)
>> +#define CONF_BUF_bm (1 << 4)
>> +#define CONF_CHAN_NEG_bp 8
>> +#define CONF_CHAN_POS_bp 12
>> +
>> +
>> +#define MODE_MD_SINGLE_gc (0x01 << 21)
>> +#define MODE_MD_ZS_CAL_gc (0x04 << 21)
>> +#define MODE_MD_FS_CAL_gc (0x05 << 21)
>> +#define MODE_CLK_INTTRI_gc (0x02 << 18)
>> +/*Table 8 in the datasheet provides options for the Filter Word*/
>> +#define MODE_FILTER_WORD 1
>> +#define SETTLE_MS 2
>> +
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/delay.h>
>> +
>> +#include "../iio.h"
>> +#include "../sysfs.h"
>> +
>> +#define DEVICE_NAME "ad7194"
>> +#define NUM_CHANNELS 16
>> +
This wins the odd award. The available gains seem unlikely to be tied to the
number of channels.
>> +const uint8_t gains[NUM_CHANNELS] = {1, 0, 0, 8, 16, 32, 64, 128};
>> +
>> +struct ad7194_data {
>> + struct spi_device *spi_dev;
>> + struct iio_dev *indio_dev;
>> + uint8_t gain[NUM_CHANNELS];
>> + uint8_t diff[NUM_CHANNELS];
>> + struct mutex lock;
>> +};
>> +
>> +static ssize_t show_gain(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>> + struct ad7194_data *pdata = dev_info->dev_data;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + return sprintf(buf, "%d\n", gains[pdata->gain[this_attr->address]]);
>> +}
>> +
>> +static ssize_t set_gain(struct device *dev,
>> + struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> + uint8_t gain_real, i;
>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>> + struct ad7194_data *pdata = dev_info->dev_data;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + gain_real = simple_strtol(buf, NULL, 10);
>> + if (gain_real == 0)
>> + return -EPERM;
>> + for (i = 0; i < NUM_CHANNELS; i++) {
>> + if (gains[i] == gain_real) {
>> + pdata->gain[this_attr->address] = i;
>> + return count;
>> + }
>> + }
>> + return -EPERM;
>> +}
>> +
>> +static ssize_t show_diff(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>> + struct ad7194_data *pdata = dev_info->dev_data;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + return sprintf(buf, "%d\n", pdata->diff[this_attr->address]);
>> +}
>> +
>> +static ssize_t set_diff(struct device *dev,
>> + struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> + uint8_t diff;
>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>> + struct ad7194_data *pdata = dev_info->dev_data;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + diff = simple_strtol(buf, NULL, 10);
strtobool and set value to the resulting bool.
>> + if (diff)
>> + pdata->diff[this_attr->address] = 1;
>> + else
>> + pdata->diff[this_attr->address] = 0;
>> + return count;
>> +}
>> +
>> +static inline int ad7194_read_reg8(struct spi_device *spi,
>> + int reg, uint8_t *val)
>> +{
>> + int ret;
>> + uint8_t tx[1], rx[1];
>> +
>> + tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
>> +
>> + ret = spi_write_then_read(spi, tx, 1, rx, 1);
>> + *val = rx[0];
>> + return ret;
>> +}
>> +
>> +static inline int ad7194_read_reg24(struct spi_device *spi,
>> + int reg, uint32_t *val)
>> +{
>> + int ret;
>> + uint8_t tx[1], rx[3];
>> +
>> + tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
Don't allocate single element arrays. Just makes things
harder to read.
>> +
>> + ret = spi_write_then_read(spi, tx, 1, rx, 3);
>> + *val = (rx[0] << 16) + (rx[1] << 8) + rx[2];
>> + return ret;
>> +}
>> +
>> +static inline int ad7194_write_reg24(struct spi_device *spi,
>> + int reg, uint32_t *val)
>> +{
>> + uint8_t tx[4];
>> +
>> + tx[0] = (reg << COMM_ADDR_bp);
>> + tx[1] = (*val >> 16) & 0xff;
>> + tx[2] = (*val >> 8) & 0xff;
>> + tx[3] = (*val >> 0) & 0xff;
>> +
>> + return spi_write(spi, tx, 4);
IIRC spi_write requires dma safe buffers (cache line aligned
buffers on the heap).
>> +}
>> +
>> +static ssize_t show_voltage(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + uint32_t conf, mode, val, sign, pos_chan, neg_chan = 0;
>> + uint8_t status, gain, diff;
>> + int32_t whole, fract;
>> + int ret;
>> +
>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>> + struct ad7194_data *pdata = dev_info->dev_data;
Pdata is a bad name choice. THat implies platform data (by conventional
use). Here it is instance specific data I think? state is a typical
variable name, or chip.
>> + struct spi_device *spi = pdata->spi_dev;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + pos_chan = this_attr->address;
>> + gain = pdata->gain[this_attr->address];
>> + diff = pdata->diff[this_attr->address];
Just general point. Don't bother with local variables for stuff that
is only used once. Just makes reading harder.
>> +
>> + if (diff == 1) {
>> + /*if in0_diff is true, reading in0_input still returns
>> + * in0, but it is in0-in1, if you read in1_input
>> + * then you get in1-in0 */
In a nutshell that explains why we don't use the interface you've gone
with but have an explicit one for differential channels. (see the max1363
driver for examples).
>> + if ((pos_chan % 2) == 0)
>> + neg_chan = pos_chan+1;
>> + else
>> + neg_chan = pos_chan-1;
>> +
>> + conf = CONF_CHOP_bm | CONF_BUF_bm |
>> + (pos_chan << CONF_CHAN_POS_bp) |
>> + (neg_chan << CONF_CHAN_NEG_bp) | gain;
>> + } else {
>> + conf = CONF_CHOP_bm | CONF_PSEUDO_bm | CONF_BUF_bm |
>> + (pos_chan << CONF_CHAN_POS_bp) | gain;
>> + }
>> +
>> + ret = mutex_lock_interruptible(&pdata->lock);
>> + if (ret != 0)
>> + return ret;
>> +
>> + ret = ad7194_write_reg24(spi, REG_CONF, &conf);
>> + if (ret != 0)
>> + goto out;
>> +
>> + mode = MODE_MD_SINGLE_gc | MODE_CLK_INTTRI_gc | MODE_FILTER_WORD;
>> + ret = ad7194_write_reg24(spi, REG_MODE, &mode);
>> + if (ret != 0)
>> + goto out;
>> +
>> + msleep_interruptible(SETTLE_MS);
>> +
>> + ret = ad7194_read_reg8(spi, REG_STATUS, &status);
>> + if (ret != 0)
>> + goto out;
>> + status = (status >> 6) & 0x01;
>> +
>> + ret = ad7194_read_reg24(spi, REG_DATA, &val);
>> + if (ret != 0)
>> + goto out;
>> +
>> + sign = (val & 0x800000) >> 23;
>> + if (sign)
>> + fract = (val & 0x7fffff);
>> + else
>> + fract = 0x7fffff - (val & 0x7fffff);
>> + fract = ((int64_t)fract*4095000) >> 23;
>> + fract = fract / gains[gain];
>> + whole = fract / 1000000;
>> + fract = fract % 1000000;
As a general rule, we'd push this into userspace, but the interface
allows for either so we can keep this as is if you really want to.
I'd like to see a little comment explaining what the calcuation is
though!
>> +
>> + if (status == 0) {
>> + mutex_unlock(&pdata->lock);
>> + if (sign)
>> + return sprintf(buf, "%d.%.6d\n", whole, fract);
>> + else
>> + return sprintf(buf, "-%d.%.6d\n", whole, fract);
>> + } else {
>> + ret = -EAGAIN;
>> + goto out;
>> + }
>> +
>> +out:
>> + mutex_unlock(&pdata->lock);
>> + return ret;
>> +}
>> +
These don't even vaguely conform to the abi. See
drivers/staging/iio/Documentation/sysfs-bus-iio. Mostly this will
get cleaned up in converting the driver over to chan_spec based
registration. Hehe, this reminds me why we introduced the chan spec
stuff in the first place!
>> +static IIO_DEVICE_ATTR(ch1_value, S_IRUGO, show_voltage, NULL, 0);
>> +static IIO_DEVICE_ATTR(ch2_value, S_IRUGO, show_voltage, NULL, 1);
>> +static IIO_DEVICE_ATTR(ch3_value, S_IRUGO, show_voltage, NULL, 2);
>> +static IIO_DEVICE_ATTR(ch4_value, S_IRUGO, show_voltage, NULL, 3);
>> +static IIO_DEVICE_ATTR(ch5_value, S_IRUGO, show_voltage, NULL, 4);
>> +static IIO_DEVICE_ATTR(ch6_value, S_IRUGO, show_voltage, NULL, 5);
>> +static IIO_DEVICE_ATTR(ch7_value, S_IRUGO, show_voltage, NULL, 6);
>> +static IIO_DEVICE_ATTR(ch8_value, S_IRUGO, show_voltage, NULL, 7);
>> +static IIO_DEVICE_ATTR(ch9_value, S_IRUGO, show_voltage, NULL, 8);
>> +static IIO_DEVICE_ATTR(ch10_value, S_IRUGO, show_voltage, NULL, 9);
>> +static IIO_DEVICE_ATTR(ch11_value, S_IRUGO, show_voltage, NULL, 10);
>> +static IIO_DEVICE_ATTR(ch12_value, S_IRUGO, show_voltage, NULL, 11);
>> +static IIO_DEVICE_ATTR(ch13_value, S_IRUGO, show_voltage, NULL, 12);
>> +static IIO_DEVICE_ATTR(ch14_value, S_IRUGO, show_voltage, NULL, 13);
>> +static IIO_DEVICE_ATTR(ch15_value, S_IRUGO, show_voltage, NULL, 14);
>> +static IIO_DEVICE_ATTR(ch16_value, S_IRUGO, show_voltage, NULL, 15);
>> +
>> +static IIO_DEVICE_ATTR(ch1_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 0);
>> +static IIO_DEVICE_ATTR(ch2_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 1);
>> +static IIO_DEVICE_ATTR(ch3_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 2);
>> +static IIO_DEVICE_ATTR(ch4_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 3);
>> +static IIO_DEVICE_ATTR(ch5_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 4);
>> +static IIO_DEVICE_ATTR(ch6_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 5);
>> +static IIO_DEVICE_ATTR(ch7_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 6);
>> +static IIO_DEVICE_ATTR(ch8_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 7);
>> +static IIO_DEVICE_ATTR(ch9_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 8);
>> +static IIO_DEVICE_ATTR(ch10_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 9);
>> +static IIO_DEVICE_ATTR(ch11_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 10);
>> +static IIO_DEVICE_ATTR(ch12_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 11);
>> +static IIO_DEVICE_ATTR(ch13_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 12);
>> +static IIO_DEVICE_ATTR(ch14_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 13);
>> +static IIO_DEVICE_ATTR(ch15_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 14);
>> +static IIO_DEVICE_ATTR(ch16_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 15);
>> +
Umm.. What are these? I think based on quick look at the datasheet that you
are using them to switch the _value attributes from unipolar to differential?
Please register them as separate channels (see max1363 for lots and lots of
examples). It can be a bit fiddly to maintain the internal state but it does
give us a coherent general interface.
>> +static IIO_DEVICE_ATTR(ch1_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 0);
>> +static IIO_DEVICE_ATTR(ch2_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 1);
>> +static IIO_DEVICE_ATTR(ch3_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 2);
>> +static IIO_DEVICE_ATTR(ch4_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 3);
>> +static IIO_DEVICE_ATTR(ch5_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 4);
>> +static IIO_DEVICE_ATTR(ch6_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 5);
>> +static IIO_DEVICE_ATTR(ch7_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 6);
>> +static IIO_DEVICE_ATTR(ch8_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 7);
>> +static IIO_DEVICE_ATTR(ch9_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 8);
>> +static IIO_DEVICE_ATTR(ch10_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 9);
>> +static IIO_DEVICE_ATTR(ch11_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 10);
>> +static IIO_DEVICE_ATTR(ch12_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 11);
>> +static IIO_DEVICE_ATTR(ch13_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 12);
>> +static IIO_DEVICE_ATTR(ch14_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 13);
>> +static IIO_DEVICE_ATTR(ch15_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 14);
>> +static IIO_DEVICE_ATTR(ch16_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 15);
>> +
>> +static struct attribute *ad7194_attributes[] = {
>> + &iio_dev_attr_ch1_value.dev_attr.attr,
>> + &iio_dev_attr_ch2_value.dev_attr.attr,
>> + &iio_dev_attr_ch3_value.dev_attr.attr,
>> + &iio_dev_attr_ch4_value.dev_attr.attr,
>> + &iio_dev_attr_ch5_value.dev_attr.attr,
>> + &iio_dev_attr_ch6_value.dev_attr.attr,
>> + &iio_dev_attr_ch7_value.dev_attr.attr,
>> + &iio_dev_attr_ch8_value.dev_attr.attr,
>> + &iio_dev_attr_ch9_value.dev_attr.attr,
>> + &iio_dev_attr_ch10_value.dev_attr.attr,
>> + &iio_dev_attr_ch11_value.dev_attr.attr,
>> + &iio_dev_attr_ch12_value.dev_attr.attr,
>> + &iio_dev_attr_ch13_value.dev_attr.attr,
>> + &iio_dev_attr_ch14_value.dev_attr.attr,
>> + &iio_dev_attr_ch15_value.dev_attr.attr,
>> + &iio_dev_attr_ch16_value.dev_attr.attr,
>> + &iio_dev_attr_ch1_gain.dev_attr.attr,
>> + &iio_dev_attr_ch2_gain.dev_attr.attr,
>> + &iio_dev_attr_ch3_gain.dev_attr.attr,
>> + &iio_dev_attr_ch4_gain.dev_attr.attr,
>> + &iio_dev_attr_ch5_gain.dev_attr.attr,
>> + &iio_dev_attr_ch6_gain.dev_attr.attr,
>> + &iio_dev_attr_ch7_gain.dev_attr.attr,
>> + &iio_dev_attr_ch8_gain.dev_attr.attr,
>> + &iio_dev_attr_ch9_gain.dev_attr.attr,
>> + &iio_dev_attr_ch10_gain.dev_attr.attr,
>> + &iio_dev_attr_ch11_gain.dev_attr.attr,
>> + &iio_dev_attr_ch12_gain.dev_attr.attr,
>> + &iio_dev_attr_ch13_gain.dev_attr.attr,
>> + &iio_dev_attr_ch14_gain.dev_attr.attr,
>> + &iio_dev_attr_ch15_gain.dev_attr.attr,
>> + &iio_dev_attr_ch16_gain.dev_attr.attr,
>> + &iio_dev_attr_ch1_diff.dev_attr.attr,
>> + &iio_dev_attr_ch2_diff.dev_attr.attr,
>> + &iio_dev_attr_ch3_diff.dev_attr.attr,
>> + &iio_dev_attr_ch4_diff.dev_attr.attr,
>> + &iio_dev_attr_ch5_diff.dev_attr.attr,
>> + &iio_dev_attr_ch6_diff.dev_attr.attr,
>> + &iio_dev_attr_ch7_diff.dev_attr.attr,
>> + &iio_dev_attr_ch8_diff.dev_attr.attr,
>> + &iio_dev_attr_ch9_diff.dev_attr.attr,
>> + &iio_dev_attr_ch10_diff.dev_attr.attr,
>> + &iio_dev_attr_ch11_diff.dev_attr.attr,
>> + &iio_dev_attr_ch12_diff.dev_attr.attr,
>> + &iio_dev_attr_ch13_diff.dev_attr.attr,
>> + &iio_dev_attr_ch14_diff.dev_attr.attr,
>> + &iio_dev_attr_ch15_diff.dev_attr.attr,
>> + &iio_dev_attr_ch16_diff.dev_attr.attr,
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group ad7194_attribute_group = {
>> + .attrs = ad7194_attributes,
>> +};
>> +
>> +static const struct iio_info ad7194_info = {
>> + .attrs = &ad7194_attribute_group,
>> + .driver_module = THIS_MODULE,
>> +};
>> +
>> +static int __devinit ad7194_probe(struct spi_device *spi)
>> +{
>> + int ret, err;
>> + struct ad7194_data *pdata;
>> +
>> + /* Configure the SPI bus */
>> + spi->mode = (SPI_MODE_0);
>> + spi->bits_per_word = 8;
>> + spi_setup(spi);
>> +
>> + pdata = kzalloc(sizeof(struct ad7194_data), GFP_KERNEL);
>> + if (!pdata) {
>> + err = -ENOMEM;
>> + goto exit;
>> + }
>> +
>> + dev_set_drvdata(&spi->dev, pdata);
>> +
>> + pdata->spi_dev = spi;
>> +
>> + pdata->indio_dev = iio_allocate_device(0);
>> + if (pdata->indio_dev == NULL) {
>> + ret = -ENOMEM;
>> + goto error_free;
>> + }
>> +
>> + mutex_init(&pdata->lock);
>> + pdata->indio_dev->name = "ad7194";
>> + pdata->indio_dev->dev.parent = &spi->dev;
>> + pdata->indio_dev->info = &ad7194_info;
>> + pdata->indio_dev->dev_data = (void *)pdata;
Devdata is going away I'm afraid (only still there in
1 driver and that's because Jon is having issues getting
the kernel up and running on his board to check I haven't
messed up the conversion!) Please do this with
iio_allocate_device(sizeof(*pdata)), then use iio_priv
to get to the resulting private data.
>> +
>> + ret = iio_device_register(pdata->indio_dev);
>> + if (ret)
>> + goto error_free_dev;
>> +
>> + return 0;
>> +
>> +error_free_dev:
>> + iio_free_device(pdata->indio_dev);
>> +error_free:
>> + kfree(pdata);
>> +exit:
>> + return err;
>> +}
>> +
>> +static int __devexit ad7194_remove(struct spi_device *spi)
>> +{
>> + struct ad7194_data *pdata = dev_get_drvdata(&spi->dev);
>> +
>> + dev_set_drvdata(&spi->dev, NULL);
>> + iio_device_unregister(pdata->indio_dev);
>> + iio_free_device(pdata->indio_dev);
>> + kfree(pdata);
>> + return 0;
>> +}
>> +
>> +static struct spi_driver ad7194_driver = {
>> + .driver = {
I've never understood why people like having a define
for the part name. Much better to just put it here
where everyone expects to find it!
>> + .name = DEVICE_NAME,
>> + .bus = &spi_bus_type,
>> + .owner = THIS_MODULE,
>> + },
>> +
>> + .probe = ad7194_probe,
>> + .remove = __devexit_p(ad7194_remove),
>> +};
>> +
>> +static int __init ad7194_init(void)
>> +{
>> + return spi_register_driver(&ad7194_driver);
>> +}
>> +
>> +static void __exit ad7194_exit(void)
>> +{
>> + spi_unregister_driver(&ad7194_driver);
>> +}
>> +
>> +module_init(ad7194_init);
>> +module_exit(ad7194_exit);
>> +
>> +MODULE_AUTHOR("Paul Thomas <pthomas8589@gmail.com>");
>> +MODULE_DESCRIPTION("TI AD7194 A/D driver");
>> +MODULE_LICENSE("GPL");
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194
@ 2011-07-18 12:24 ` Jonathan Cameron
0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2011-07-18 12:24 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Paul Thomas, lm-sensors,
device-drivers-devel@blackfin.uclinux.org,
linux-iio@vger.kernel.org
On 07/18/11 12:01, Jonathan Cameron wrote:
> cc'ing linux-iio and AD's driver list.
>
> Any particular reason for posting to lm-sensors? Now it's there we'll
> keep them in the list though as someone might be interested.
>
> On 07/18/11 08:46, Paul Thomas wrote:
>> This uses the iio sysfs interface, and inculdes gain and differential settings
Hi Paul,
This driver is lagging somewhat in interface terms. Having said that, it applies
and compiles fine which will make catching up to current point much easier.
If you are short on time I'm happy to do the conversion (as it is a nice simple
driver), but obviously I'll need to test it to find out what I messed up this
time.
Big issues:
1) iio_dev->dev_data is going away, so please use the iio_priv stuff.
2) interface is not terribly close the abi spec.
3) use chan_spec based registration. Actually that'll clean up the abi
issues as well and give you much shorter code.
4) differential channels are treated as separate channels (with appropriate
numbering). This is easy here as there are no nasty constraints on channel
combinations (it only reads one at a time anyway!).
Various nitpicks inline. Though the above seems like a lot, you have done
all the fiddly stuff about actually talking the the chips. Cleaning up
interfaces is relatively straight forward! Lots of fun stuff to add to this
chip at a later date, but in the spirit of your driver, lets keep it simple
for now!
As long as you are happy to do a couple of rounds of testing, we could merge
this as is and do the abi work as a series of small steps on top of it?
Thanks,
Jonathan
>>
>> Signed-off-by: Paul Thomas <pthomas8589@gmail.com>
>> ---
>> drivers/staging/iio/adc/Kconfig | 7 +
>> drivers/staging/iio/adc/Makefile | 1 +
>> drivers/staging/iio/adc/ad7194.c | 462 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 470 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/staging/iio/adc/ad7194.c
>>
>> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
>> index 8c751c4..871605b 100644
>> --- a/drivers/staging/iio/adc/Kconfig
>> +++ b/drivers/staging/iio/adc/Kconfig
>> @@ -17,6 +17,13 @@ config AD7152
>> Say yes here to build support for Analog Devices capacitive sensors.
>> (ad7152, ad7153) Provides direct access via sysfs.
>>
>> +config AD7194
>> + tristate "Analog Devices AD7194 ADC driver"
>> + depends on SPI
>> + help
>> + Say yes here to build support for Analog Devices ad7194.
>> + Provides direct access via sysfs.
>> +
>> config AD7291
>> tristate "Analog Devices AD7291 temperature sensor driver"
>> depends on I2C
>> diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
>> index 1d9b3f5..4da3c40 100644
>> --- a/drivers/staging/iio/adc/Makefile
>> +++ b/drivers/staging/iio/adc/Makefile
>> @@ -31,6 +31,7 @@ obj-$(CONFIG_AD7298) += ad7298.o
>>
>> obj-$(CONFIG_AD7150) += ad7150.o
>> obj-$(CONFIG_AD7152) += ad7152.o
>> +obj-$(CONFIG_AD7194) += ad7194.o
>> obj-$(CONFIG_AD7291) += ad7291.o
>> obj-$(CONFIG_AD7314) += ad7314.o
>> obj-$(CONFIG_AD7745) += ad7745.o
>> diff --git a/drivers/staging/iio/adc/ad7194.c b/drivers/staging/iio/adc/ad7194.c
>> new file mode 100644
>> index 0000000..a867662
>> --- /dev/null
>> +++ b/drivers/staging/iio/adc/ad7194.c
>> @@ -0,0 +1,462 @@
>> +/*
>> + * ad7194 - driver for Analog Devices AD7194 A/D converter
>> + *
>> + * Copyright (c) 2011 Paul Thomas <pthomas8589@gmail.com>
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 or
>> + * later as publishhed by the Free Software Foundation.
>> + *
>> + * You need to have something like this in struct spi_board_info
>> + * {
>> + * .modalias = "ad7194",
>> + * .max_speed_hz = 2*1000*1000,
>> + * .chip_select = 0,
>> + * .bus_num = 1,
>> + * },
>> + *
>> + * Three sysfs files are used chX_value, chX_diff and chX_gain
>> + * chX_value is read only and returns that channels value in volts.
>> + * chX_gain is read/write, the chX_value is scaled by the gain so it retains
>> + * it's proper units.
>> + * chX_diff is read/write, if chX_diff is 0 then pseude mod is used,
>> + * if chX_diff is 1 then differential mode is used.
>> + * In this mode ch1_value selects ch1-ch2, and ch2_value selects ch2-ch1
>> + */
>> +
>> +#define DEBUG 1
>> +
>> +/*From Table 15 in the datasheet*/
>> +/*Register addresses*/
>> +#define REG_STATUS 0
>> +#define REG_MODE 1
>> +#define REG_CONF 2
>> +#define REG_DATA 3
>> +#define REG_ID 4
>> +#define REG_GPOCON 5
>> +#define REG_OFFSET 6
>> +#define REG_FS 7
>> +
>> +/*From Table 15 in the datasheet*/
>> +#define COMM_ADDR_bp 3
>> +#define COMM_READ_bm (1 << 6)
>> +
>> +#define CONF_CHOP_bm (1 << 23)
>> +#define CONF_PSEUDO_bm (1 << 18)
>> +#define CONF_BUF_bm (1 << 4)
>> +#define CONF_CHAN_NEG_bp 8
>> +#define CONF_CHAN_POS_bp 12
>> +
>> +
>> +#define MODE_MD_SINGLE_gc (0x01 << 21)
>> +#define MODE_MD_ZS_CAL_gc (0x04 << 21)
>> +#define MODE_MD_FS_CAL_gc (0x05 << 21)
>> +#define MODE_CLK_INTTRI_gc (0x02 << 18)
>> +/*Table 8 in the datasheet provides options for the Filter Word*/
>> +#define MODE_FILTER_WORD 1
>> +#define SETTLE_MS 2
>> +
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/delay.h>
>> +
>> +#include "../iio.h"
>> +#include "../sysfs.h"
>> +
>> +#define DEVICE_NAME "ad7194"
>> +#define NUM_CHANNELS 16
>> +
This wins the odd award. The available gains seem unlikely to be tied to the
number of channels.
>> +const uint8_t gains[NUM_CHANNELS] = {1, 0, 0, 8, 16, 32, 64, 128};
>> +
>> +struct ad7194_data {
>> + struct spi_device *spi_dev;
>> + struct iio_dev *indio_dev;
>> + uint8_t gain[NUM_CHANNELS];
>> + uint8_t diff[NUM_CHANNELS];
>> + struct mutex lock;
>> +};
>> +
>> +static ssize_t show_gain(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>> + struct ad7194_data *pdata = dev_info->dev_data;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + return sprintf(buf, "%d\n", gains[pdata->gain[this_attr->address]]);
>> +}
>> +
>> +static ssize_t set_gain(struct device *dev,
>> + struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> + uint8_t gain_real, i;
>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>> + struct ad7194_data *pdata = dev_info->dev_data;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + gain_real = simple_strtol(buf, NULL, 10);
>> + if (gain_real = 0)
>> + return -EPERM;
>> + for (i = 0; i < NUM_CHANNELS; i++) {
>> + if (gains[i] = gain_real) {
>> + pdata->gain[this_attr->address] = i;
>> + return count;
>> + }
>> + }
>> + return -EPERM;
>> +}
>> +
>> +static ssize_t show_diff(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>> + struct ad7194_data *pdata = dev_info->dev_data;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + return sprintf(buf, "%d\n", pdata->diff[this_attr->address]);
>> +}
>> +
>> +static ssize_t set_diff(struct device *dev,
>> + struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> + uint8_t diff;
>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>> + struct ad7194_data *pdata = dev_info->dev_data;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + diff = simple_strtol(buf, NULL, 10);
strtobool and set value to the resulting bool.
>> + if (diff)
>> + pdata->diff[this_attr->address] = 1;
>> + else
>> + pdata->diff[this_attr->address] = 0;
>> + return count;
>> +}
>> +
>> +static inline int ad7194_read_reg8(struct spi_device *spi,
>> + int reg, uint8_t *val)
>> +{
>> + int ret;
>> + uint8_t tx[1], rx[1];
>> +
>> + tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
>> +
>> + ret = spi_write_then_read(spi, tx, 1, rx, 1);
>> + *val = rx[0];
>> + return ret;
>> +}
>> +
>> +static inline int ad7194_read_reg24(struct spi_device *spi,
>> + int reg, uint32_t *val)
>> +{
>> + int ret;
>> + uint8_t tx[1], rx[3];
>> +
>> + tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
Don't allocate single element arrays. Just makes things
harder to read.
>> +
>> + ret = spi_write_then_read(spi, tx, 1, rx, 3);
>> + *val = (rx[0] << 16) + (rx[1] << 8) + rx[2];
>> + return ret;
>> +}
>> +
>> +static inline int ad7194_write_reg24(struct spi_device *spi,
>> + int reg, uint32_t *val)
>> +{
>> + uint8_t tx[4];
>> +
>> + tx[0] = (reg << COMM_ADDR_bp);
>> + tx[1] = (*val >> 16) & 0xff;
>> + tx[2] = (*val >> 8) & 0xff;
>> + tx[3] = (*val >> 0) & 0xff;
>> +
>> + return spi_write(spi, tx, 4);
IIRC spi_write requires dma safe buffers (cache line aligned
buffers on the heap).
>> +}
>> +
>> +static ssize_t show_voltage(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + uint32_t conf, mode, val, sign, pos_chan, neg_chan = 0;
>> + uint8_t status, gain, diff;
>> + int32_t whole, fract;
>> + int ret;
>> +
>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>> + struct ad7194_data *pdata = dev_info->dev_data;
Pdata is a bad name choice. THat implies platform data (by conventional
use). Here it is instance specific data I think? state is a typical
variable name, or chip.
>> + struct spi_device *spi = pdata->spi_dev;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + pos_chan = this_attr->address;
>> + gain = pdata->gain[this_attr->address];
>> + diff = pdata->diff[this_attr->address];
Just general point. Don't bother with local variables for stuff that
is only used once. Just makes reading harder.
>> +
>> + if (diff = 1) {
>> + /*if in0_diff is true, reading in0_input still returns
>> + * in0, but it is in0-in1, if you read in1_input
>> + * then you get in1-in0 */
In a nutshell that explains why we don't use the interface you've gone
with but have an explicit one for differential channels. (see the max1363
driver for examples).
>> + if ((pos_chan % 2) = 0)
>> + neg_chan = pos_chan+1;
>> + else
>> + neg_chan = pos_chan-1;
>> +
>> + conf = CONF_CHOP_bm | CONF_BUF_bm |
>> + (pos_chan << CONF_CHAN_POS_bp) |
>> + (neg_chan << CONF_CHAN_NEG_bp) | gain;
>> + } else {
>> + conf = CONF_CHOP_bm | CONF_PSEUDO_bm | CONF_BUF_bm |
>> + (pos_chan << CONF_CHAN_POS_bp) | gain;
>> + }
>> +
>> + ret = mutex_lock_interruptible(&pdata->lock);
>> + if (ret != 0)
>> + return ret;
>> +
>> + ret = ad7194_write_reg24(spi, REG_CONF, &conf);
>> + if (ret != 0)
>> + goto out;
>> +
>> + mode = MODE_MD_SINGLE_gc | MODE_CLK_INTTRI_gc | MODE_FILTER_WORD;
>> + ret = ad7194_write_reg24(spi, REG_MODE, &mode);
>> + if (ret != 0)
>> + goto out;
>> +
>> + msleep_interruptible(SETTLE_MS);
>> +
>> + ret = ad7194_read_reg8(spi, REG_STATUS, &status);
>> + if (ret != 0)
>> + goto out;
>> + status = (status >> 6) & 0x01;
>> +
>> + ret = ad7194_read_reg24(spi, REG_DATA, &val);
>> + if (ret != 0)
>> + goto out;
>> +
>> + sign = (val & 0x800000) >> 23;
>> + if (sign)
>> + fract = (val & 0x7fffff);
>> + else
>> + fract = 0x7fffff - (val & 0x7fffff);
>> + fract = ((int64_t)fract*4095000) >> 23;
>> + fract = fract / gains[gain];
>> + whole = fract / 1000000;
>> + fract = fract % 1000000;
As a general rule, we'd push this into userspace, but the interface
allows for either so we can keep this as is if you really want to.
I'd like to see a little comment explaining what the calcuation is
though!
>> +
>> + if (status = 0) {
>> + mutex_unlock(&pdata->lock);
>> + if (sign)
>> + return sprintf(buf, "%d.%.6d\n", whole, fract);
>> + else
>> + return sprintf(buf, "-%d.%.6d\n", whole, fract);
>> + } else {
>> + ret = -EAGAIN;
>> + goto out;
>> + }
>> +
>> +out:
>> + mutex_unlock(&pdata->lock);
>> + return ret;
>> +}
>> +
These don't even vaguely conform to the abi. See
drivers/staging/iio/Documentation/sysfs-bus-iio. Mostly this will
get cleaned up in converting the driver over to chan_spec based
registration. Hehe, this reminds me why we introduced the chan spec
stuff in the first place!
>> +static IIO_DEVICE_ATTR(ch1_value, S_IRUGO, show_voltage, NULL, 0);
>> +static IIO_DEVICE_ATTR(ch2_value, S_IRUGO, show_voltage, NULL, 1);
>> +static IIO_DEVICE_ATTR(ch3_value, S_IRUGO, show_voltage, NULL, 2);
>> +static IIO_DEVICE_ATTR(ch4_value, S_IRUGO, show_voltage, NULL, 3);
>> +static IIO_DEVICE_ATTR(ch5_value, S_IRUGO, show_voltage, NULL, 4);
>> +static IIO_DEVICE_ATTR(ch6_value, S_IRUGO, show_voltage, NULL, 5);
>> +static IIO_DEVICE_ATTR(ch7_value, S_IRUGO, show_voltage, NULL, 6);
>> +static IIO_DEVICE_ATTR(ch8_value, S_IRUGO, show_voltage, NULL, 7);
>> +static IIO_DEVICE_ATTR(ch9_value, S_IRUGO, show_voltage, NULL, 8);
>> +static IIO_DEVICE_ATTR(ch10_value, S_IRUGO, show_voltage, NULL, 9);
>> +static IIO_DEVICE_ATTR(ch11_value, S_IRUGO, show_voltage, NULL, 10);
>> +static IIO_DEVICE_ATTR(ch12_value, S_IRUGO, show_voltage, NULL, 11);
>> +static IIO_DEVICE_ATTR(ch13_value, S_IRUGO, show_voltage, NULL, 12);
>> +static IIO_DEVICE_ATTR(ch14_value, S_IRUGO, show_voltage, NULL, 13);
>> +static IIO_DEVICE_ATTR(ch15_value, S_IRUGO, show_voltage, NULL, 14);
>> +static IIO_DEVICE_ATTR(ch16_value, S_IRUGO, show_voltage, NULL, 15);
>> +
>> +static IIO_DEVICE_ATTR(ch1_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 0);
>> +static IIO_DEVICE_ATTR(ch2_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 1);
>> +static IIO_DEVICE_ATTR(ch3_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 2);
>> +static IIO_DEVICE_ATTR(ch4_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 3);
>> +static IIO_DEVICE_ATTR(ch5_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 4);
>> +static IIO_DEVICE_ATTR(ch6_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 5);
>> +static IIO_DEVICE_ATTR(ch7_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 6);
>> +static IIO_DEVICE_ATTR(ch8_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 7);
>> +static IIO_DEVICE_ATTR(ch9_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 8);
>> +static IIO_DEVICE_ATTR(ch10_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 9);
>> +static IIO_DEVICE_ATTR(ch11_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 10);
>> +static IIO_DEVICE_ATTR(ch12_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 11);
>> +static IIO_DEVICE_ATTR(ch13_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 12);
>> +static IIO_DEVICE_ATTR(ch14_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 13);
>> +static IIO_DEVICE_ATTR(ch15_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 14);
>> +static IIO_DEVICE_ATTR(ch16_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 15);
>> +
Umm.. What are these? I think based on quick look at the datasheet that you
are using them to switch the _value attributes from unipolar to differential?
Please register them as separate channels (see max1363 for lots and lots of
examples). It can be a bit fiddly to maintain the internal state but it does
give us a coherent general interface.
>> +static IIO_DEVICE_ATTR(ch1_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 0);
>> +static IIO_DEVICE_ATTR(ch2_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 1);
>> +static IIO_DEVICE_ATTR(ch3_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 2);
>> +static IIO_DEVICE_ATTR(ch4_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 3);
>> +static IIO_DEVICE_ATTR(ch5_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 4);
>> +static IIO_DEVICE_ATTR(ch6_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 5);
>> +static IIO_DEVICE_ATTR(ch7_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 6);
>> +static IIO_DEVICE_ATTR(ch8_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 7);
>> +static IIO_DEVICE_ATTR(ch9_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 8);
>> +static IIO_DEVICE_ATTR(ch10_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 9);
>> +static IIO_DEVICE_ATTR(ch11_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 10);
>> +static IIO_DEVICE_ATTR(ch12_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 11);
>> +static IIO_DEVICE_ATTR(ch13_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 12);
>> +static IIO_DEVICE_ATTR(ch14_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 13);
>> +static IIO_DEVICE_ATTR(ch15_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 14);
>> +static IIO_DEVICE_ATTR(ch16_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 15);
>> +
>> +static struct attribute *ad7194_attributes[] = {
>> + &iio_dev_attr_ch1_value.dev_attr.attr,
>> + &iio_dev_attr_ch2_value.dev_attr.attr,
>> + &iio_dev_attr_ch3_value.dev_attr.attr,
>> + &iio_dev_attr_ch4_value.dev_attr.attr,
>> + &iio_dev_attr_ch5_value.dev_attr.attr,
>> + &iio_dev_attr_ch6_value.dev_attr.attr,
>> + &iio_dev_attr_ch7_value.dev_attr.attr,
>> + &iio_dev_attr_ch8_value.dev_attr.attr,
>> + &iio_dev_attr_ch9_value.dev_attr.attr,
>> + &iio_dev_attr_ch10_value.dev_attr.attr,
>> + &iio_dev_attr_ch11_value.dev_attr.attr,
>> + &iio_dev_attr_ch12_value.dev_attr.attr,
>> + &iio_dev_attr_ch13_value.dev_attr.attr,
>> + &iio_dev_attr_ch14_value.dev_attr.attr,
>> + &iio_dev_attr_ch15_value.dev_attr.attr,
>> + &iio_dev_attr_ch16_value.dev_attr.attr,
>> + &iio_dev_attr_ch1_gain.dev_attr.attr,
>> + &iio_dev_attr_ch2_gain.dev_attr.attr,
>> + &iio_dev_attr_ch3_gain.dev_attr.attr,
>> + &iio_dev_attr_ch4_gain.dev_attr.attr,
>> + &iio_dev_attr_ch5_gain.dev_attr.attr,
>> + &iio_dev_attr_ch6_gain.dev_attr.attr,
>> + &iio_dev_attr_ch7_gain.dev_attr.attr,
>> + &iio_dev_attr_ch8_gain.dev_attr.attr,
>> + &iio_dev_attr_ch9_gain.dev_attr.attr,
>> + &iio_dev_attr_ch10_gain.dev_attr.attr,
>> + &iio_dev_attr_ch11_gain.dev_attr.attr,
>> + &iio_dev_attr_ch12_gain.dev_attr.attr,
>> + &iio_dev_attr_ch13_gain.dev_attr.attr,
>> + &iio_dev_attr_ch14_gain.dev_attr.attr,
>> + &iio_dev_attr_ch15_gain.dev_attr.attr,
>> + &iio_dev_attr_ch16_gain.dev_attr.attr,
>> + &iio_dev_attr_ch1_diff.dev_attr.attr,
>> + &iio_dev_attr_ch2_diff.dev_attr.attr,
>> + &iio_dev_attr_ch3_diff.dev_attr.attr,
>> + &iio_dev_attr_ch4_diff.dev_attr.attr,
>> + &iio_dev_attr_ch5_diff.dev_attr.attr,
>> + &iio_dev_attr_ch6_diff.dev_attr.attr,
>> + &iio_dev_attr_ch7_diff.dev_attr.attr,
>> + &iio_dev_attr_ch8_diff.dev_attr.attr,
>> + &iio_dev_attr_ch9_diff.dev_attr.attr,
>> + &iio_dev_attr_ch10_diff.dev_attr.attr,
>> + &iio_dev_attr_ch11_diff.dev_attr.attr,
>> + &iio_dev_attr_ch12_diff.dev_attr.attr,
>> + &iio_dev_attr_ch13_diff.dev_attr.attr,
>> + &iio_dev_attr_ch14_diff.dev_attr.attr,
>> + &iio_dev_attr_ch15_diff.dev_attr.attr,
>> + &iio_dev_attr_ch16_diff.dev_attr.attr,
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group ad7194_attribute_group = {
>> + .attrs = ad7194_attributes,
>> +};
>> +
>> +static const struct iio_info ad7194_info = {
>> + .attrs = &ad7194_attribute_group,
>> + .driver_module = THIS_MODULE,
>> +};
>> +
>> +static int __devinit ad7194_probe(struct spi_device *spi)
>> +{
>> + int ret, err;
>> + struct ad7194_data *pdata;
>> +
>> + /* Configure the SPI bus */
>> + spi->mode = (SPI_MODE_0);
>> + spi->bits_per_word = 8;
>> + spi_setup(spi);
>> +
>> + pdata = kzalloc(sizeof(struct ad7194_data), GFP_KERNEL);
>> + if (!pdata) {
>> + err = -ENOMEM;
>> + goto exit;
>> + }
>> +
>> + dev_set_drvdata(&spi->dev, pdata);
>> +
>> + pdata->spi_dev = spi;
>> +
>> + pdata->indio_dev = iio_allocate_device(0);
>> + if (pdata->indio_dev = NULL) {
>> + ret = -ENOMEM;
>> + goto error_free;
>> + }
>> +
>> + mutex_init(&pdata->lock);
>> + pdata->indio_dev->name = "ad7194";
>> + pdata->indio_dev->dev.parent = &spi->dev;
>> + pdata->indio_dev->info = &ad7194_info;
>> + pdata->indio_dev->dev_data = (void *)pdata;
Devdata is going away I'm afraid (only still there in
1 driver and that's because Jon is having issues getting
the kernel up and running on his board to check I haven't
messed up the conversion!) Please do this with
iio_allocate_device(sizeof(*pdata)), then use iio_priv
to get to the resulting private data.
>> +
>> + ret = iio_device_register(pdata->indio_dev);
>> + if (ret)
>> + goto error_free_dev;
>> +
>> + return 0;
>> +
>> +error_free_dev:
>> + iio_free_device(pdata->indio_dev);
>> +error_free:
>> + kfree(pdata);
>> +exit:
>> + return err;
>> +}
>> +
>> +static int __devexit ad7194_remove(struct spi_device *spi)
>> +{
>> + struct ad7194_data *pdata = dev_get_drvdata(&spi->dev);
>> +
>> + dev_set_drvdata(&spi->dev, NULL);
>> + iio_device_unregister(pdata->indio_dev);
>> + iio_free_device(pdata->indio_dev);
>> + kfree(pdata);
>> + return 0;
>> +}
>> +
>> +static struct spi_driver ad7194_driver = {
>> + .driver = {
I've never understood why people like having a define
for the part name. Much better to just put it here
where everyone expects to find it!
>> + .name = DEVICE_NAME,
>> + .bus = &spi_bus_type,
>> + .owner = THIS_MODULE,
>> + },
>> +
>> + .probe = ad7194_probe,
>> + .remove = __devexit_p(ad7194_remove),
>> +};
>> +
>> +static int __init ad7194_init(void)
>> +{
>> + return spi_register_driver(&ad7194_driver);
>> +}
>> +
>> +static void __exit ad7194_exit(void)
>> +{
>> + spi_unregister_driver(&ad7194_driver);
>> +}
>> +
>> +module_init(ad7194_init);
>> +module_exit(ad7194_exit);
>> +
>> +MODULE_AUTHOR("Paul Thomas <pthomas8589@gmail.com>");
>> +MODULE_DESCRIPTION("TI AD7194 A/D driver");
>> +MODULE_LICENSE("GPL");
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: add support for Analog Devices ad7194 a/d converter
2011-07-18 12:24 ` [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 Jonathan Cameron
@ 2011-07-18 16:03 ` Paul Thomas
-1 siblings, 0 replies; 12+ messages in thread
From: Paul Thomas @ 2011-07-18 16:03 UTC (permalink / raw)
To: Jonathan Cameron
Cc: lm-sensors, device-drivers-devel@blackfin.uclinux.org,
linux-iio@vger.kernel.org
On Mon, Jul 18, 2011 at 5:24 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> On 07/18/11 12:01, Jonathan Cameron wrote:
>> cc'ing linux-iio and AD's driver list.
>>
>> Any particular reason for posting to lm-sensors? Now it's there we'll
>> keep them in the list though as someone might be interested.
>>
>> On 07/18/11 08:46, Paul Thomas wrote:
>>> This uses the iio sysfs interface, and inculdes gain and differential s=
ettings
>
> Hi Paul,
>
> This driver is lagging somewhat in interface terms. Having said that, it =
applies
> and compiles fine which will make catching up to current point much easie=
r.
>
> If you are short on time I'm happy to do the conversion (as it is a nice =
simple
> driver), but obviously I'll need to test it to find out what I messed up =
this
> time.
>
> Big issues:
>
> 1) iio_dev->dev_data is going away, so please use the iio_priv stuff.
> 2) interface is not terribly close the abi spec.
> 3) use chan_spec based registration. Actually that'll clean up the abi
> issues as well and give you much shorter code.
> 4) differential channels are treated as separate channels (with appropria=
te
> numbering). =A0This is easy here as there are no nasty constraints on cha=
nnel
> combinations (it only reads one at a time anyway!).
>
> Various nitpicks inline. =A0Though the above seems like a lot, you have d=
one
> all the fiddly stuff about actually talking the the chips. Cleaning up
> interfaces is relatively straight forward! =A0Lots of fun stuff to add to=
this
> chip at a later date, but in the spirit of your driver, lets keep it simp=
le
> for now!
>
> As long as you are happy to do a couple of rounds of testing, we could me=
rge
> this as is and do the abi work as a series of small steps on top of it?
>
> Thanks,
>
> Jonathan
Hi Jonathan, I'd be happy to do the fixing. Is there an existing
multi-channel driver that might be helpful to reference here?
thanks,
Paul
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194
@ 2011-07-18 16:03 ` Paul Thomas
0 siblings, 0 replies; 12+ messages in thread
From: Paul Thomas @ 2011-07-18 16:03 UTC (permalink / raw)
To: Jonathan Cameron
Cc: lm-sensors, device-drivers-devel@blackfin.uclinux.org,
linux-iio@vger.kernel.org
On Mon, Jul 18, 2011 at 5:24 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> On 07/18/11 12:01, Jonathan Cameron wrote:
>> cc'ing linux-iio and AD's driver list.
>>
>> Any particular reason for posting to lm-sensors? Now it's there we'll
>> keep them in the list though as someone might be interested.
>>
>> On 07/18/11 08:46, Paul Thomas wrote:
>>> This uses the iio sysfs interface, and inculdes gain and differential settings
>
> Hi Paul,
>
> This driver is lagging somewhat in interface terms. Having said that, it applies
> and compiles fine which will make catching up to current point much easier.
>
> If you are short on time I'm happy to do the conversion (as it is a nice simple
> driver), but obviously I'll need to test it to find out what I messed up this
> time.
>
> Big issues:
>
> 1) iio_dev->dev_data is going away, so please use the iio_priv stuff.
> 2) interface is not terribly close the abi spec.
> 3) use chan_spec based registration. Actually that'll clean up the abi
> issues as well and give you much shorter code.
> 4) differential channels are treated as separate channels (with appropriate
> numbering). This is easy here as there are no nasty constraints on channel
> combinations (it only reads one at a time anyway!).
>
> Various nitpicks inline. Though the above seems like a lot, you have done
> all the fiddly stuff about actually talking the the chips. Cleaning up
> interfaces is relatively straight forward! Lots of fun stuff to add to this
> chip at a later date, but in the spirit of your driver, lets keep it simple
> for now!
>
> As long as you are happy to do a couple of rounds of testing, we could merge
> this as is and do the abi work as a series of small steps on top of it?
>
> Thanks,
>
> Jonathan
Hi Jonathan, I'd be happy to do the fixing. Is there an existing
multi-channel driver that might be helpful to reference here?
thanks,
Paul
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: add support for Analog Devices ad7194 a/d converter
2011-07-18 16:03 ` [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 Paul Thomas
@ 2011-07-18 16:11 ` Jonathan Cameron
-1 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2011-07-18 16:11 UTC (permalink / raw)
To: Paul Thomas
Cc: lm-sensors, device-drivers-devel@blackfin.uclinux.org,
linux-iio@vger.kernel.org
On 07/18/11 17:03, Paul Thomas wrote:
> On Mon, Jul 18, 2011 at 5:24 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
>> On 07/18/11 12:01, Jonathan Cameron wrote:
>>> cc'ing linux-iio and AD's driver list.
>>>
>>> Any particular reason for posting to lm-sensors? Now it's there we'll
>>> keep them in the list though as someone might be interested.
>>>
>>> On 07/18/11 08:46, Paul Thomas wrote:
>>>> This uses the iio sysfs interface, and inculdes gain and differential settings
>>
>> Hi Paul,
>>
>> This driver is lagging somewhat in interface terms. Having said that, it applies
>> and compiles fine which will make catching up to current point much easier.
>>
>> If you are short on time I'm happy to do the conversion (as it is a nice simple
>> driver), but obviously I'll need to test it to find out what I messed up this
>> time.
>>
>> Big issues:
>>
>> 1) iio_dev->dev_data is going away, so please use the iio_priv stuff.
>> 2) interface is not terribly close the abi spec.
>> 3) use chan_spec based registration. Actually that'll clean up the abi
>> issues as well and give you much shorter code.
>> 4) differential channels are treated as separate channels (with appropriate
>> numbering). This is easy here as there are no nasty constraints on channel
>> combinations (it only reads one at a time anyway!).
>>
>> Various nitpicks inline. Though the above seems like a lot, you have done
>> all the fiddly stuff about actually talking the the chips. Cleaning up
>> interfaces is relatively straight forward! Lots of fun stuff to add to this
>> chip at a later date, but in the spirit of your driver, lets keep it simple
>> for now!
>>
>> As long as you are happy to do a couple of rounds of testing, we could merge
>> this as is and do the abi work as a series of small steps on top of it?
>>
>> Thanks,
>>
>> Jonathan
>
> Hi Jonathan, I'd be happy to do the fixing. Is there an existing
> multi-channel driver that might be helpful to reference here?
>
Cool.
Lots of suitable drivers to copy. Max1363 is my standard adc and that
driver is reasonably fully featured. I think most of the ADI drivers
are mostly unipolar only, but a quick grep tells me the ad7793 has
a mixture of differential an unipolar.
max1363 is probably the closest to what you have here in that it supports
the same range of combinations of inputs.
Jonathan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194
@ 2011-07-18 16:11 ` Jonathan Cameron
0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2011-07-18 16:11 UTC (permalink / raw)
To: Paul Thomas
Cc: lm-sensors, device-drivers-devel@blackfin.uclinux.org,
linux-iio@vger.kernel.org
On 07/18/11 17:03, Paul Thomas wrote:
> On Mon, Jul 18, 2011 at 5:24 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
>> On 07/18/11 12:01, Jonathan Cameron wrote:
>>> cc'ing linux-iio and AD's driver list.
>>>
>>> Any particular reason for posting to lm-sensors? Now it's there we'll
>>> keep them in the list though as someone might be interested.
>>>
>>> On 07/18/11 08:46, Paul Thomas wrote:
>>>> This uses the iio sysfs interface, and inculdes gain and differential settings
>>
>> Hi Paul,
>>
>> This driver is lagging somewhat in interface terms. Having said that, it applies
>> and compiles fine which will make catching up to current point much easier.
>>
>> If you are short on time I'm happy to do the conversion (as it is a nice simple
>> driver), but obviously I'll need to test it to find out what I messed up this
>> time.
>>
>> Big issues:
>>
>> 1) iio_dev->dev_data is going away, so please use the iio_priv stuff.
>> 2) interface is not terribly close the abi spec.
>> 3) use chan_spec based registration. Actually that'll clean up the abi
>> issues as well and give you much shorter code.
>> 4) differential channels are treated as separate channels (with appropriate
>> numbering). This is easy here as there are no nasty constraints on channel
>> combinations (it only reads one at a time anyway!).
>>
>> Various nitpicks inline. Though the above seems like a lot, you have done
>> all the fiddly stuff about actually talking the the chips. Cleaning up
>> interfaces is relatively straight forward! Lots of fun stuff to add to this
>> chip at a later date, but in the spirit of your driver, lets keep it simple
>> for now!
>>
>> As long as you are happy to do a couple of rounds of testing, we could merge
>> this as is and do the abi work as a series of small steps on top of it?
>>
>> Thanks,
>>
>> Jonathan
>
> Hi Jonathan, I'd be happy to do the fixing. Is there an existing
> multi-channel driver that might be helpful to reference here?
>
Cool.
Lots of suitable drivers to copy. Max1363 is my standard adc and that
driver is reasonably fully featured. I think most of the ADI drivers
are mostly unipolar only, but a quick grep tells me the ad7793 has
a mixture of differential an unipolar.
max1363 is probably the closest to what you have here in that it supports
the same range of combinations of inputs.
Jonathan
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iio: add support for Analog Devices ad7194 a/d converter
2011-07-18 12:24 ` [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 Jonathan Cameron
@ 2011-07-18 17:11 ` Paul Thomas
-1 siblings, 0 replies; 12+ messages in thread
From: Paul Thomas @ 2011-07-18 17:11 UTC (permalink / raw)
To: Jonathan Cameron
Cc: lm-sensors, device-drivers-devel@blackfin.uclinux.org,
linux-iio@vger.kernel.org
Aside from the ABI stuff, I had a couple responses to your comments.
thanks,
Paul
>>>
>>> Signed-off-by: Paul Thomas <pthomas8589@gmail.com>
>>> ---
>>> =A0drivers/staging/iio/adc/Kconfig =A0| =A0 =A07 +
>>> =A0drivers/staging/iio/adc/Makefile | =A0 =A01 +
>>> =A0drivers/staging/iio/adc/ad7194.c | =A0462 ++++++++++++++++++++++++++=
++++++++++++
>>> =A03 files changed, 470 insertions(+), 0 deletions(-)
>>> =A0create mode 100644 drivers/staging/iio/adc/ad7194.c
>>>
>>> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/=
Kconfig
>>> index 8c751c4..871605b 100644
>>> --- a/drivers/staging/iio/adc/Kconfig
>>> +++ b/drivers/staging/iio/adc/Kconfig
>>> @@ -17,6 +17,13 @@ config AD7152
>>> =A0 =A0 =A0 =A0Say yes here to build support for Analog Devices capacit=
ive sensors.
>>> =A0 =A0 =A0 =A0(ad7152, ad7153) Provides direct access via sysfs.
>>>
>>> +config AD7194
>>> + =A0 =A0tristate "Analog Devices AD7194 ADC driver"
>>> + =A0 =A0depends on SPI
>>> + =A0 =A0help
>>> + =A0 =A0 =A0Say yes here to build support for Analog Devices ad7194.
>>> + =A0 =A0 =A0Provides direct access via sysfs.
>>> +
>>> =A0config AD7291
>>> =A0 =A0 =A0tristate "Analog Devices AD7291 temperature sensor driver"
>>> =A0 =A0 =A0depends on I2C
>>> diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc=
/Makefile
>>> index 1d9b3f5..4da3c40 100644
>>> --- a/drivers/staging/iio/adc/Makefile
>>> +++ b/drivers/staging/iio/adc/Makefile
>>> @@ -31,6 +31,7 @@ obj-$(CONFIG_AD7298) +=3D ad7298.o
>>>
>>> =A0obj-$(CONFIG_AD7150) +=3D ad7150.o
>>> =A0obj-$(CONFIG_AD7152) +=3D ad7152.o
>>> +obj-$(CONFIG_AD7194) +=3D ad7194.o
>>> =A0obj-$(CONFIG_AD7291) +=3D ad7291.o
>>> =A0obj-$(CONFIG_AD7314) +=3D ad7314.o
>>> =A0obj-$(CONFIG_AD7745) +=3D ad7745.o
>>> diff --git a/drivers/staging/iio/adc/ad7194.c b/drivers/staging/iio/adc=
/ad7194.c
>>> new file mode 100644
>>> index 0000000..a867662
>>> --- /dev/null
>>> +++ b/drivers/staging/iio/adc/ad7194.c
>>> @@ -0,0 +1,462 @@
>>> +/*
>>> + * =A0ad7194 - driver for Analog Devices AD7194 A/D converter
>>> + *
>>> + * =A0Copyright (c) 2011 Paul Thomas <pthomas8589@gmail.com>
>>> + *
>>> + * =A0This program is distributed in the hope that it will be useful,
>>> + * =A0but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * =A0MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * =A0GNU General Public License for more details.
>>> + *
>>> + * =A0This program is free software; you can redistribute it and/or mo=
dify
>>> + * =A0it under the terms of the GNU General Public License version 2 o=
r
>>> + * =A0later as publishhed by the Free Software Foundation.
>>> + *
>>> + * =A0You need to have something like this in struct spi_board_info
>>> + * =A0{
>>> + * =A0 =A0 =A0 =A0 =A0.modalias =A0 =A0 =A0 =3D "ad7194",
>>> + * =A0 =A0 =A0 =A0 =A0.max_speed_hz =A0 =3D 2*1000*1000,
>>> + * =A0 =A0 =A0 =A0 =A0.chip_select =A0 =A0=3D 0,
>>> + * =A0 =A0 =A0 =A0 =A0.bus_num =A0 =A0 =A0 =A0=3D 1,
>>> + * =A0},
>>> + *
>>> + * Three sysfs files are used chX_value, chX_diff and chX_gain
>>> + * chX_value is read only and returns that channels value in volts.
>>> + * chX_gain is read/write, the chX_value is scaled by the gain so it r=
etains
>>> + * it's proper units.
>>> + * chX_diff is read/write, if chX_diff is 0 then pseude mod is used,
>>> + * if chX_diff is 1 then differential mode is used.
>>> + * In this mode ch1_value selects ch1-ch2, and ch2_value selects ch2-c=
h1
>>> + */
>>> +
>>> +#define DEBUG 1
>>> +
>>> +/*From Table 15 in the datasheet*/
>>> +/*Register addresses*/
>>> +#define REG_STATUS =A00
>>> +#define REG_MODE =A0 =A01
>>> +#define REG_CONF =A0 =A02
>>> +#define REG_DATA =A0 =A03
>>> +#define REG_ID =A0 =A0 =A04
>>> +#define REG_GPOCON =A05
>>> +#define REG_OFFSET =A06
>>> +#define REG_FS =A0 =A0 =A07
>>> +
>>> +/*From Table 15 in the datasheet*/
>>> +#define COMM_ADDR_bp 3
>>> +#define COMM_READ_bm (1 << 6)
>>> +
>>> +#define CONF_CHOP_bm (1 << 23)
>>> +#define CONF_PSEUDO_bm (1 << 18)
>>> +#define CONF_BUF_bm (1 << 4)
>>> +#define CONF_CHAN_NEG_bp 8
>>> +#define CONF_CHAN_POS_bp 12
>>> +
>>> +
>>> +#define MODE_MD_SINGLE_gc (0x01 << 21)
>>> +#define MODE_MD_ZS_CAL_gc (0x04 << 21)
>>> +#define MODE_MD_FS_CAL_gc (0x05 << 21)
>>> +#define MODE_CLK_INTTRI_gc (0x02 << 18)
>>> +/*Table 8 in the datasheet provides options for the Filter Word*/
>>> +#define MODE_FILTER_WORD 1
>>> +#define SETTLE_MS 2
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/err.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/delay.h>
>>> +
>>> +#include "../iio.h"
>>> +#include "../sysfs.h"
>>> +
>>> +#define DEVICE_NAME "ad7194"
>>> +#define NUM_CHANNELS 16
>>> +
> This wins the odd award. =A0The available gains seem unlikely to be tied =
to the
> number of channels.
Right! what was I thinking?
>>> +const uint8_t gains[NUM_CHANNELS] =3D {1, 0, 0, 8, 16, 32, 64, 128};
>>> +
>>> +struct ad7194_data {
>>> + =A0 =A0struct spi_device *spi_dev;
>>> + =A0 =A0struct iio_dev *indio_dev;
>>> + =A0 =A0uint8_t gain[NUM_CHANNELS];
>>> + =A0 =A0uint8_t diff[NUM_CHANNELS];
>>> + =A0 =A0struct mutex lock;
>>> +};
>>> +
>>> +static ssize_t show_gain(struct device *dev,
>>> + =A0 =A0 =A0 =A0 =A0 =A0struct device_attribute *attr, char *buf)
>>> +{
>>> + =A0 =A0struct iio_dev *dev_info =3D dev_get_drvdata(dev);
>>> + =A0 =A0struct ad7194_data *pdata =3D dev_info->dev_data;
>>> + =A0 =A0struct iio_dev_attr *this_attr =3D to_iio_dev_attr(attr);
>>> +
>>> + =A0 =A0return sprintf(buf, "%d\n", gains[pdata->gain[this_attr->addre=
ss]]);
>>> +}
>>> +
>>> +static ssize_t set_gain(struct device *dev,
>>> + =A0 =A0 =A0 =A0 =A0 =A0struct device_attribute *attr, const char *buf=
, size_t count)
>>> +{
>>> + =A0 =A0uint8_t gain_real, i;
>>> + =A0 =A0struct iio_dev *dev_info =3D dev_get_drvdata(dev);
>>> + =A0 =A0struct ad7194_data *pdata =3D dev_info->dev_data;
>>> + =A0 =A0struct iio_dev_attr *this_attr =3D to_iio_dev_attr(attr);
>>> +
>>> + =A0 =A0gain_real =3D simple_strtol(buf, NULL, 10);
>>> + =A0 =A0if (gain_real =3D=3D 0)
>>> + =A0 =A0 =A0 =A0 =A0 =A0return -EPERM;
>>> + =A0 =A0for (i =3D 0; i < NUM_CHANNELS; i++) {
>>> + =A0 =A0 =A0 =A0 =A0 =A0if (gains[i] =3D=3D gain_real) {
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pdata->gain[this_attr->address=
] =3D =A0i;
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return count;
>>> + =A0 =A0 =A0 =A0 =A0 =A0}
>>> + =A0 =A0}
>>> + =A0 =A0return -EPERM;
>>> +}
>>> +
>>> +static ssize_t show_diff(struct device *dev,
>>> + =A0 =A0 =A0 =A0 =A0 =A0struct device_attribute *attr, char *buf)
>>> +{
>>> + =A0 =A0struct iio_dev *dev_info =3D dev_get_drvdata(dev);
>>> + =A0 =A0struct ad7194_data *pdata =3D dev_info->dev_data;
>>> + =A0 =A0struct iio_dev_attr *this_attr =3D to_iio_dev_attr(attr);
>>> +
>>> + =A0 =A0return sprintf(buf, "%d\n", pdata->diff[this_attr->address]);
>>> +}
>>> +
>>> +static ssize_t set_diff(struct device *dev,
>>> + =A0 =A0 =A0 =A0 =A0 =A0struct device_attribute *attr, const char *buf=
, size_t count)
>>> +{
>>> + =A0 =A0uint8_t diff;
>>> + =A0 =A0struct iio_dev *dev_info =3D dev_get_drvdata(dev);
>>> + =A0 =A0struct ad7194_data *pdata =3D dev_info->dev_data;
>>> + =A0 =A0struct iio_dev_attr *this_attr =3D to_iio_dev_attr(attr);
>>> +
>>> + =A0 =A0diff =3D simple_strtol(buf, NULL, 10);
> strtobool and set value to the resulting bool.
>>> + =A0 =A0if (diff)
>>> + =A0 =A0 =A0 =A0 =A0 =A0pdata->diff[this_attr->address] =3D 1;
>>> + =A0 =A0else
>>> + =A0 =A0 =A0 =A0 =A0 =A0pdata->diff[this_attr->address] =3D 0;
>>> + =A0 =A0return count;
>>> +}
>>> +
>>> +static inline int ad7194_read_reg8(struct spi_device *spi,
>>> + =A0 =A0 =A0 =A0 =A0 =A0int reg, uint8_t *val)
>>> +{
>>> + =A0 =A0int ret;
>>> + =A0 =A0uint8_t tx[1], rx[1];
>>> +
>>> + =A0 =A0tx[0] =3D (COMM_READ_bm | (reg << COMM_ADDR_bp));
>>> +
>>> + =A0 =A0ret =3D spi_write_then_read(spi, tx, 1, rx, 1);
>>> + =A0 =A0*val =3D rx[0];
>>> + =A0 =A0return ret;
>>> +}
>>> +
>>> +static inline int ad7194_read_reg24(struct spi_device *spi,
>>> + =A0 =A0 =A0 =A0 =A0 =A0int reg, uint32_t *val)
>>> +{
>>> + =A0 =A0int ret;
>>> + =A0 =A0uint8_t tx[1], rx[3];
>>> +
>>> + =A0 =A0tx[0] =3D (COMM_READ_bm | (reg << COMM_ADDR_bp));
> Don't allocate single element arrays. =A0Just makes things
> harder to read.
I was trying to keep consistency between the _reg8 & _reg24 versions
as well as between the rx & tx buffers. It makes the
spi_write_then_read(spi, tx, 1, rx, 3) read nicely, but if that's a
no-no then I can change it.
>
>>> +
>>> + =A0 =A0ret =3D spi_write_then_read(spi, tx, 1, rx, 3);
>>> + =A0 =A0*val =3D (rx[0] << 16) + (rx[1] << 8) + rx[2];
>>> + =A0 =A0return ret;
>>> +}
>>> +
>>> +static inline int ad7194_write_reg24(struct spi_device *spi,
>>> + =A0 =A0 =A0 =A0 =A0 =A0int reg, uint32_t *val)
>>> +{
>>> + =A0 =A0uint8_t tx[4];
>>> +
>>> + =A0 =A0tx[0] =3D (reg << COMM_ADDR_bp);
>>> + =A0 =A0tx[1] =3D (*val >> 16) & 0xff;
>>> + =A0 =A0tx[2] =3D (*val >> 8) & 0xff;
>>> + =A0 =A0tx[3] =3D (*val >> 0) & 0xff;
>>> +
>>> + =A0 =A0return spi_write(spi, tx, 4);
> IIRC spi_write requires dma safe buffers (cache line aligned
> buffers on the heap).
Could you expand here?
>>> +}
>>> +
>>> +static ssize_t show_voltage(struct device *dev,
>>> + =A0 =A0 =A0 =A0 =A0 =A0struct device_attribute *attr, char *buf)
>>> +{
>>> + =A0 =A0uint32_t conf, mode, val, sign, pos_chan, neg_chan =3D 0;
>>> + =A0 =A0uint8_t status, gain, diff;
>>> + =A0 =A0int32_t whole, fract;
>>> + =A0 =A0int ret;
>>> +
>>> + =A0 =A0struct iio_dev *dev_info =3D dev_get_drvdata(dev);
>>> + =A0 =A0struct ad7194_data *pdata =3D dev_info->dev_data;
> Pdata is a bad name choice. =A0THat implies platform data (by conventiona=
l
> use). =A0Here it is instance specific data I think? state is a typical
> variable name, or chip.
>
>>> + =A0 =A0struct spi_device *spi =3D pdata->spi_dev;
>>> + =A0 =A0struct iio_dev_attr *this_attr =3D to_iio_dev_attr(attr);
>>> +
>>> + =A0 =A0pos_chan =3D this_attr->address;
>>> + =A0 =A0gain =3D pdata->gain[this_attr->address];
>>> + =A0 =A0diff =3D pdata->diff[this_attr->address];
> Just general point. Don't bother with local variables for stuff that
> is only used once. =A0Just makes reading harder.
>>> +
>>> + =A0 =A0if (diff =3D=3D 1) {
>>> + =A0 =A0 =A0 =A0 =A0 =A0/*if in0_diff is true, reading in0_input still=
returns
>>> + =A0 =A0 =A0 =A0 =A0 =A0* in0, but it is in0-in1, if you read in1_inpu=
t
>>> + =A0 =A0 =A0 =A0 =A0 =A0* then you get in1-in0 */
> In a nutshell that explains why we don't use the interface you've gone
> with but have an explicit one for differential channels. (see the max1363
> driver for examples).
>
>>> + =A0 =A0 =A0 =A0 =A0 =A0if ((pos_chan % 2) =3D=3D 0)
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0neg_chan =3D pos_chan+1;
>>> + =A0 =A0 =A0 =A0 =A0 =A0else
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0neg_chan =3D pos_chan-1;
>>> +
>>> + =A0 =A0 =A0 =A0 =A0 =A0conf =3D CONF_CHOP_bm | CONF_BUF_bm |
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(pos_chan << CONF_CHAN_POS_bp)=
|
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(neg_chan << CONF_CHAN_NEG_bp)=
| gain;
>>> + =A0 =A0} else {
>>> + =A0 =A0 =A0 =A0 =A0 =A0conf =3D CONF_CHOP_bm | CONF_PSEUDO_bm | CONF_=
BUF_bm |
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(pos_chan << CONF_CHAN_POS_bp)=
| gain;
>>> + =A0 =A0}
>>> +
>>> + =A0 =A0ret =3D mutex_lock_interruptible(&pdata->lock);
>>> + =A0 =A0if (ret !=3D 0)
>>> + =A0 =A0 =A0 =A0 =A0 =A0return ret;
>>> +
>>> + =A0 =A0ret =3D ad7194_write_reg24(spi, REG_CONF, &conf);
>>> + =A0 =A0if (ret !=3D 0)
>>> + =A0 =A0 =A0 =A0 =A0 =A0goto out;
>>> +
>>> + =A0 =A0mode =3D MODE_MD_SINGLE_gc | MODE_CLK_INTTRI_gc | MODE_FILTER_=
WORD;
>>> + =A0 =A0ret =3D ad7194_write_reg24(spi, REG_MODE, &mode);
>>> + =A0 =A0if (ret !=3D 0)
>>> + =A0 =A0 =A0 =A0 =A0 =A0goto out;
>>> +
>>> + =A0 =A0msleep_interruptible(SETTLE_MS);
>>> +
>>> + =A0 =A0ret =3D ad7194_read_reg8(spi, REG_STATUS, &status);
>>> + =A0 =A0if (ret !=3D 0)
>>> + =A0 =A0 =A0 =A0 =A0 =A0goto out;
>>> + =A0 =A0status =3D (status >> 6) & 0x01;
>>> +
>>> + =A0 =A0ret =3D ad7194_read_reg24(spi, REG_DATA, &val);
>>> + =A0 =A0if (ret !=3D 0)
>>> + =A0 =A0 =A0 =A0 =A0 =A0goto out;
>>> +
>>> + =A0 =A0sign =3D (val & 0x800000) >> 23;
>>> + =A0 =A0if (sign)
>>> + =A0 =A0 =A0 =A0 =A0 =A0fract =3D (val & 0x7fffff);
>>> + =A0 =A0else
>>> + =A0 =A0 =A0 =A0 =A0 =A0fract =3D 0x7fffff - (val & 0x7fffff);
>>> + =A0 =A0fract =3D ((int64_t)fract*4095000) >> 23;
>>> + =A0 =A0fract =3D fract / gains[gain];
>>> + =A0 =A0whole =3D fract / 1000000;
>>> + =A0 =A0fract =3D fract % 1000000;
> As a general rule, we'd push this into userspace, but the interface
> allows for either so we can keep this as is if you really want to.
> I'd like to see a little comment explaining what the calcuation is
> though!
>>> +
>>> + =A0 =A0if (status =3D=3D 0) {
>>> + =A0 =A0 =A0 =A0 =A0 =A0mutex_unlock(&pdata->lock);
>>> + =A0 =A0 =A0 =A0 =A0 =A0if (sign)
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return sprintf(buf, "%d.%.6d\n=
", whole, fract);
>>> + =A0 =A0 =A0 =A0 =A0 =A0else
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return sprintf(buf, "-%d.%.6d\=
n", whole, fract);
>>> + =A0 =A0} else {
>>> + =A0 =A0 =A0 =A0 =A0 =A0ret =3D -EAGAIN;
>>> + =A0 =A0 =A0 =A0 =A0 =A0goto out;
>>> + =A0 =A0}
>>> +
>>> +out:
>>> + =A0 =A0mutex_unlock(&pdata->lock);
>>> + =A0 =A0return ret;
>>> +}
>>> +
>
>
> These don't even vaguely conform to the abi. See
> drivers/staging/iio/Documentation/sysfs-bus-iio. =A0Mostly this will
> get cleaned up in converting the driver over to chan_spec based
> registration. =A0Hehe, this reminds me why we introduced the chan spec
> stuff in the first place!
>>> +static IIO_DEVICE_ATTR(ch1_value, S_IRUGO, show_voltage, NULL, 0);
>>> +static IIO_DEVICE_ATTR(ch2_value, S_IRUGO, show_voltage, NULL, 1);
>>> +static IIO_DEVICE_ATTR(ch3_value, S_IRUGO, show_voltage, NULL, 2);
>>> +static IIO_DEVICE_ATTR(ch4_value, S_IRUGO, show_voltage, NULL, 3);
>>> +static IIO_DEVICE_ATTR(ch5_value, S_IRUGO, show_voltage, NULL, 4);
>>> +static IIO_DEVICE_ATTR(ch6_value, S_IRUGO, show_voltage, NULL, 5);
>>> +static IIO_DEVICE_ATTR(ch7_value, S_IRUGO, show_voltage, NULL, 6);
>>> +static IIO_DEVICE_ATTR(ch8_value, S_IRUGO, show_voltage, NULL, 7);
>>> +static IIO_DEVICE_ATTR(ch9_value, S_IRUGO, show_voltage, NULL, 8);
>>> +static IIO_DEVICE_ATTR(ch10_value, S_IRUGO, show_voltage, NULL, 9);
>>> +static IIO_DEVICE_ATTR(ch11_value, S_IRUGO, show_voltage, NULL, 10);
>>> +static IIO_DEVICE_ATTR(ch12_value, S_IRUGO, show_voltage, NULL, 11);
>>> +static IIO_DEVICE_ATTR(ch13_value, S_IRUGO, show_voltage, NULL, 12);
>>> +static IIO_DEVICE_ATTR(ch14_value, S_IRUGO, show_voltage, NULL, 13);
>>> +static IIO_DEVICE_ATTR(ch15_value, S_IRUGO, show_voltage, NULL, 14);
>>> +static IIO_DEVICE_ATTR(ch16_value, S_IRUGO, show_voltage, NULL, 15);
>>> +
>>> +static IIO_DEVICE_ATTR(ch1_gain, S_IWUSR | S_IRUGO, show_gain, set_gai=
n, 0);
>>> +static IIO_DEVICE_ATTR(ch2_gain, S_IWUSR | S_IRUGO, show_gain, set_gai=
n, 1);
>>> +static IIO_DEVICE_ATTR(ch3_gain, S_IWUSR | S_IRUGO, show_gain, set_gai=
n, 2);
>>> +static IIO_DEVICE_ATTR(ch4_gain, S_IWUSR | S_IRUGO, show_gain, set_gai=
n, 3);
>>> +static IIO_DEVICE_ATTR(ch5_gain, S_IWUSR | S_IRUGO, show_gain, set_gai=
n, 4);
>>> +static IIO_DEVICE_ATTR(ch6_gain, S_IWUSR | S_IRUGO, show_gain, set_gai=
n, 5);
>>> +static IIO_DEVICE_ATTR(ch7_gain, S_IWUSR | S_IRUGO, show_gain, set_gai=
n, 6);
>>> +static IIO_DEVICE_ATTR(ch8_gain, S_IWUSR | S_IRUGO, show_gain, set_gai=
n, 7);
>>> +static IIO_DEVICE_ATTR(ch9_gain, S_IWUSR | S_IRUGO, show_gain, set_gai=
n, 8);
>>> +static IIO_DEVICE_ATTR(ch10_gain, S_IWUSR | S_IRUGO, show_gain, set_ga=
in, 9);
>>> +static IIO_DEVICE_ATTR(ch11_gain, S_IWUSR | S_IRUGO, show_gain, set_ga=
in, 10);
>>> +static IIO_DEVICE_ATTR(ch12_gain, S_IWUSR | S_IRUGO, show_gain, set_ga=
in, 11);
>>> +static IIO_DEVICE_ATTR(ch13_gain, S_IWUSR | S_IRUGO, show_gain, set_ga=
in, 12);
>>> +static IIO_DEVICE_ATTR(ch14_gain, S_IWUSR | S_IRUGO, show_gain, set_ga=
in, 13);
>>> +static IIO_DEVICE_ATTR(ch15_gain, S_IWUSR | S_IRUGO, show_gain, set_ga=
in, 14);
>>> +static IIO_DEVICE_ATTR(ch16_gain, S_IWUSR | S_IRUGO, show_gain, set_ga=
in, 15);
>>> +
>
> Umm.. What are these? =A0I think based on quick look at the datasheet tha=
t you
> are using them to switch the _value attributes from unipolar to different=
ial?
> Please register them as separate channels (see max1363 for lots and lots =
of
> examples). =A0It can be a bit fiddly to maintain the internal state but i=
t does
> give us a coherent general interface.
>>> +static IIO_DEVICE_ATTR(ch1_diff, S_IWUSR | S_IRUGO, show_diff, set_dif=
f, 0);
>>> +static IIO_DEVICE_ATTR(ch2_diff, S_IWUSR | S_IRUGO, show_diff, set_dif=
f, 1);
>>> +static IIO_DEVICE_ATTR(ch3_diff, S_IWUSR | S_IRUGO, show_diff, set_dif=
f, 2);
>>> +static IIO_DEVICE_ATTR(ch4_diff, S_IWUSR | S_IRUGO, show_diff, set_dif=
f, 3);
>>> +static IIO_DEVICE_ATTR(ch5_diff, S_IWUSR | S_IRUGO, show_diff, set_dif=
f, 4);
>>> +static IIO_DEVICE_ATTR(ch6_diff, S_IWUSR | S_IRUGO, show_diff, set_dif=
f, 5);
>>> +static IIO_DEVICE_ATTR(ch7_diff, S_IWUSR | S_IRUGO, show_diff, set_dif=
f, 6);
>>> +static IIO_DEVICE_ATTR(ch8_diff, S_IWUSR | S_IRUGO, show_diff, set_dif=
f, 7);
>>> +static IIO_DEVICE_ATTR(ch9_diff, S_IWUSR | S_IRUGO, show_diff, set_dif=
f, 8);
>>> +static IIO_DEVICE_ATTR(ch10_diff, S_IWUSR | S_IRUGO, show_diff, set_di=
ff, 9);
>>> +static IIO_DEVICE_ATTR(ch11_diff, S_IWUSR | S_IRUGO, show_diff, set_di=
ff, 10);
>>> +static IIO_DEVICE_ATTR(ch12_diff, S_IWUSR | S_IRUGO, show_diff, set_di=
ff, 11);
>>> +static IIO_DEVICE_ATTR(ch13_diff, S_IWUSR | S_IRUGO, show_diff, set_di=
ff, 12);
>>> +static IIO_DEVICE_ATTR(ch14_diff, S_IWUSR | S_IRUGO, show_diff, set_di=
ff, 13);
>>> +static IIO_DEVICE_ATTR(ch15_diff, S_IWUSR | S_IRUGO, show_diff, set_di=
ff, 14);
>>> +static IIO_DEVICE_ATTR(ch16_diff, S_IWUSR | S_IRUGO, show_diff, set_di=
ff, 15);
>>> +
>>> +static struct attribute *ad7194_attributes[] =3D {
>>> + =A0 =A0&iio_dev_attr_ch1_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch2_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch3_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch4_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch5_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch6_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch7_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch8_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch9_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch10_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch11_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch12_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch13_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch14_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch15_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch16_value.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch1_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch2_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch3_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch4_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch5_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch6_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch7_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch8_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch9_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch10_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch11_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch12_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch13_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch14_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch15_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch16_gain.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch1_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch2_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch3_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch4_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch5_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch6_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch7_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch8_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch9_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch10_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch11_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch12_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch13_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch14_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch15_diff.dev_attr.attr,
>>> + =A0 =A0&iio_dev_attr_ch16_diff.dev_attr.attr,
>>> + =A0 =A0NULL,
>>> +};
>>> +
>>> +static const struct attribute_group ad7194_attribute_group =3D {
>>> + =A0 =A0.attrs =3D ad7194_attributes,
>>> +};
>>> +
>>> +static const struct iio_info ad7194_info =3D {
>>> + =A0 =A0.attrs =3D &ad7194_attribute_group,
>>> + =A0 =A0.driver_module =3D THIS_MODULE,
>>> +};
>>> +
>>> +static int __devinit ad7194_probe(struct spi_device *spi)
>>> +{
>>> + =A0 =A0int ret, err;
>>> + =A0 =A0struct ad7194_data *pdata;
>>> +
>>> + =A0 =A0/* Configure the SPI bus */
>>> + =A0 =A0spi->mode =3D (SPI_MODE_0);
>>> + =A0 =A0spi->bits_per_word =3D 8;
>>> + =A0 =A0spi_setup(spi);
>>> +
>>> + =A0 =A0pdata =3D kzalloc(sizeof(struct ad7194_data), GFP_KERNEL);
>>> + =A0 =A0if (!pdata) {
>>> + =A0 =A0 =A0 =A0 =A0 =A0err =3D -ENOMEM;
>>> + =A0 =A0 =A0 =A0 =A0 =A0goto exit;
>>> + =A0 =A0}
>>> +
>>> + =A0 =A0dev_set_drvdata(&spi->dev, pdata);
>>> +
>>> + =A0 =A0pdata->spi_dev =3D spi;
>>> +
>>> + =A0 =A0pdata->indio_dev =3D iio_allocate_device(0);
>>> + =A0 =A0if (pdata->indio_dev =3D=3D NULL) {
>>> + =A0 =A0 =A0 =A0 =A0 =A0ret =3D -ENOMEM;
>>> + =A0 =A0 =A0 =A0 =A0 =A0goto error_free;
>>> + =A0 =A0}
>>> +
>>> + =A0 =A0mutex_init(&pdata->lock);
>>> + =A0 =A0pdata->indio_dev->name =3D "ad7194";
>>> + =A0 =A0pdata->indio_dev->dev.parent =3D &spi->dev;
>>> + =A0 =A0pdata->indio_dev->info =3D &ad7194_info;
>>> + =A0 =A0pdata->indio_dev->dev_data =3D (void *)pdata;
> Devdata is going away I'm afraid (only still there in
> 1 driver and that's because Jon is having issues getting
> the kernel up and running on his board to check I haven't
> messed up the conversion!) =A0Please do this with
> iio_allocate_device(sizeof(*pdata)), then use iio_priv
> to get to the resulting private data.
>
>>> +
>>> + =A0 =A0ret =3D iio_device_register(pdata->indio_dev);
>>> + =A0 =A0if (ret)
>>> + =A0 =A0 =A0 =A0 =A0 =A0goto error_free_dev;
>>> +
>>> + =A0 =A0return 0;
>>> +
>>> +error_free_dev:
>>> + =A0 =A0iio_free_device(pdata->indio_dev);
>>> +error_free:
>>> + =A0 =A0kfree(pdata);
>>> +exit:
>>> + =A0 =A0return err;
>>> +}
>>> +
>>> +static int __devexit ad7194_remove(struct spi_device *spi)
>>> +{
>>> + =A0 =A0struct ad7194_data *pdata =3D dev_get_drvdata(&spi->dev);
>>> +
>>> + =A0 =A0dev_set_drvdata(&spi->dev, NULL);
>>> + =A0 =A0iio_device_unregister(pdata->indio_dev);
>>> + =A0 =A0iio_free_device(pdata->indio_dev);
>>> + =A0 =A0kfree(pdata);
>>> + =A0 =A0return 0;
>>> +}
>>> +
>>> +static struct spi_driver ad7194_driver =3D {
>>> + =A0 =A0.driver =3D {
> I've never understood why people like having a define
> for the part name. =A0Much better to just put it here
> where everyone expects to find it!
>
>>> + =A0 =A0 =A0 =A0 =A0 =A0.name =3D DEVICE_NAME,
>>> + =A0 =A0 =A0 =A0 =A0 =A0.bus =3D &spi_bus_type,
>>> + =A0 =A0 =A0 =A0 =A0 =A0.owner =3D THIS_MODULE,
>>> + =A0 =A0},
>>> +
>>> + =A0 =A0.probe =3D ad7194_probe,
>>> + =A0 =A0.remove =3D __devexit_p(ad7194_remove),
>>> +};
>>> +
>>> +static int __init ad7194_init(void)
>>> +{
>>> + =A0 =A0return spi_register_driver(&ad7194_driver);
>>> +}
>>> +
>>> +static void __exit ad7194_exit(void)
>>> +{
>>> + =A0 =A0spi_unregister_driver(&ad7194_driver);
>>> +}
>>> +
>>> +module_init(ad7194_init);
>>> +module_exit(ad7194_exit);
>>> +
>>> +MODULE_AUTHOR("Paul Thomas <pthomas8589@gmail.com>");
>>> +MODULE_DESCRIPTION("TI AD7194 A/D driver");
>>> +MODULE_LICENSE("GPL");
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194
@ 2011-07-18 17:11 ` Paul Thomas
0 siblings, 0 replies; 12+ messages in thread
From: Paul Thomas @ 2011-07-18 17:11 UTC (permalink / raw)
To: Jonathan Cameron
Cc: lm-sensors, device-drivers-devel@blackfin.uclinux.org,
linux-iio@vger.kernel.org
Aside from the ABI stuff, I had a couple responses to your comments.
thanks,
Paul
>>>
>>> Signed-off-by: Paul Thomas <pthomas8589@gmail.com>
>>> ---
>>> drivers/staging/iio/adc/Kconfig | 7 +
>>> drivers/staging/iio/adc/Makefile | 1 +
>>> drivers/staging/iio/adc/ad7194.c | 462 ++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 470 insertions(+), 0 deletions(-)
>>> create mode 100644 drivers/staging/iio/adc/ad7194.c
>>>
>>> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
>>> index 8c751c4..871605b 100644
>>> --- a/drivers/staging/iio/adc/Kconfig
>>> +++ b/drivers/staging/iio/adc/Kconfig
>>> @@ -17,6 +17,13 @@ config AD7152
>>> Say yes here to build support for Analog Devices capacitive sensors.
>>> (ad7152, ad7153) Provides direct access via sysfs.
>>>
>>> +config AD7194
>>> + tristate "Analog Devices AD7194 ADC driver"
>>> + depends on SPI
>>> + help
>>> + Say yes here to build support for Analog Devices ad7194.
>>> + Provides direct access via sysfs.
>>> +
>>> config AD7291
>>> tristate "Analog Devices AD7291 temperature sensor driver"
>>> depends on I2C
>>> diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
>>> index 1d9b3f5..4da3c40 100644
>>> --- a/drivers/staging/iio/adc/Makefile
>>> +++ b/drivers/staging/iio/adc/Makefile
>>> @@ -31,6 +31,7 @@ obj-$(CONFIG_AD7298) += ad7298.o
>>>
>>> obj-$(CONFIG_AD7150) += ad7150.o
>>> obj-$(CONFIG_AD7152) += ad7152.o
>>> +obj-$(CONFIG_AD7194) += ad7194.o
>>> obj-$(CONFIG_AD7291) += ad7291.o
>>> obj-$(CONFIG_AD7314) += ad7314.o
>>> obj-$(CONFIG_AD7745) += ad7745.o
>>> diff --git a/drivers/staging/iio/adc/ad7194.c b/drivers/staging/iio/adc/ad7194.c
>>> new file mode 100644
>>> index 0000000..a867662
>>> --- /dev/null
>>> +++ b/drivers/staging/iio/adc/ad7194.c
>>> @@ -0,0 +1,462 @@
>>> +/*
>>> + * ad7194 - driver for Analog Devices AD7194 A/D converter
>>> + *
>>> + * Copyright (c) 2011 Paul Thomas <pthomas8589@gmail.com>
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 or
>>> + * later as publishhed by the Free Software Foundation.
>>> + *
>>> + * You need to have something like this in struct spi_board_info
>>> + * {
>>> + * .modalias = "ad7194",
>>> + * .max_speed_hz = 2*1000*1000,
>>> + * .chip_select = 0,
>>> + * .bus_num = 1,
>>> + * },
>>> + *
>>> + * Three sysfs files are used chX_value, chX_diff and chX_gain
>>> + * chX_value is read only and returns that channels value in volts.
>>> + * chX_gain is read/write, the chX_value is scaled by the gain so it retains
>>> + * it's proper units.
>>> + * chX_diff is read/write, if chX_diff is 0 then pseude mod is used,
>>> + * if chX_diff is 1 then differential mode is used.
>>> + * In this mode ch1_value selects ch1-ch2, and ch2_value selects ch2-ch1
>>> + */
>>> +
>>> +#define DEBUG 1
>>> +
>>> +/*From Table 15 in the datasheet*/
>>> +/*Register addresses*/
>>> +#define REG_STATUS 0
>>> +#define REG_MODE 1
>>> +#define REG_CONF 2
>>> +#define REG_DATA 3
>>> +#define REG_ID 4
>>> +#define REG_GPOCON 5
>>> +#define REG_OFFSET 6
>>> +#define REG_FS 7
>>> +
>>> +/*From Table 15 in the datasheet*/
>>> +#define COMM_ADDR_bp 3
>>> +#define COMM_READ_bm (1 << 6)
>>> +
>>> +#define CONF_CHOP_bm (1 << 23)
>>> +#define CONF_PSEUDO_bm (1 << 18)
>>> +#define CONF_BUF_bm (1 << 4)
>>> +#define CONF_CHAN_NEG_bp 8
>>> +#define CONF_CHAN_POS_bp 12
>>> +
>>> +
>>> +#define MODE_MD_SINGLE_gc (0x01 << 21)
>>> +#define MODE_MD_ZS_CAL_gc (0x04 << 21)
>>> +#define MODE_MD_FS_CAL_gc (0x05 << 21)
>>> +#define MODE_CLK_INTTRI_gc (0x02 << 18)
>>> +/*Table 8 in the datasheet provides options for the Filter Word*/
>>> +#define MODE_FILTER_WORD 1
>>> +#define SETTLE_MS 2
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/err.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/delay.h>
>>> +
>>> +#include "../iio.h"
>>> +#include "../sysfs.h"
>>> +
>>> +#define DEVICE_NAME "ad7194"
>>> +#define NUM_CHANNELS 16
>>> +
> This wins the odd award. The available gains seem unlikely to be tied to the
> number of channels.
Right! what was I thinking?
>>> +const uint8_t gains[NUM_CHANNELS] = {1, 0, 0, 8, 16, 32, 64, 128};
>>> +
>>> +struct ad7194_data {
>>> + struct spi_device *spi_dev;
>>> + struct iio_dev *indio_dev;
>>> + uint8_t gain[NUM_CHANNELS];
>>> + uint8_t diff[NUM_CHANNELS];
>>> + struct mutex lock;
>>> +};
>>> +
>>> +static ssize_t show_gain(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>>> + struct ad7194_data *pdata = dev_info->dev_data;
>>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>>> +
>>> + return sprintf(buf, "%d\n", gains[pdata->gain[this_attr->address]]);
>>> +}
>>> +
>>> +static ssize_t set_gain(struct device *dev,
>>> + struct device_attribute *attr, const char *buf, size_t count)
>>> +{
>>> + uint8_t gain_real, i;
>>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>>> + struct ad7194_data *pdata = dev_info->dev_data;
>>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>>> +
>>> + gain_real = simple_strtol(buf, NULL, 10);
>>> + if (gain_real = 0)
>>> + return -EPERM;
>>> + for (i = 0; i < NUM_CHANNELS; i++) {
>>> + if (gains[i] = gain_real) {
>>> + pdata->gain[this_attr->address] = i;
>>> + return count;
>>> + }
>>> + }
>>> + return -EPERM;
>>> +}
>>> +
>>> +static ssize_t show_diff(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>>> + struct ad7194_data *pdata = dev_info->dev_data;
>>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>>> +
>>> + return sprintf(buf, "%d\n", pdata->diff[this_attr->address]);
>>> +}
>>> +
>>> +static ssize_t set_diff(struct device *dev,
>>> + struct device_attribute *attr, const char *buf, size_t count)
>>> +{
>>> + uint8_t diff;
>>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>>> + struct ad7194_data *pdata = dev_info->dev_data;
>>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>>> +
>>> + diff = simple_strtol(buf, NULL, 10);
> strtobool and set value to the resulting bool.
>>> + if (diff)
>>> + pdata->diff[this_attr->address] = 1;
>>> + else
>>> + pdata->diff[this_attr->address] = 0;
>>> + return count;
>>> +}
>>> +
>>> +static inline int ad7194_read_reg8(struct spi_device *spi,
>>> + int reg, uint8_t *val)
>>> +{
>>> + int ret;
>>> + uint8_t tx[1], rx[1];
>>> +
>>> + tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
>>> +
>>> + ret = spi_write_then_read(spi, tx, 1, rx, 1);
>>> + *val = rx[0];
>>> + return ret;
>>> +}
>>> +
>>> +static inline int ad7194_read_reg24(struct spi_device *spi,
>>> + int reg, uint32_t *val)
>>> +{
>>> + int ret;
>>> + uint8_t tx[1], rx[3];
>>> +
>>> + tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
> Don't allocate single element arrays. Just makes things
> harder to read.
I was trying to keep consistency between the _reg8 & _reg24 versions
as well as between the rx & tx buffers. It makes the
spi_write_then_read(spi, tx, 1, rx, 3) read nicely, but if that's a
no-no then I can change it.
>
>>> +
>>> + ret = spi_write_then_read(spi, tx, 1, rx, 3);
>>> + *val = (rx[0] << 16) + (rx[1] << 8) + rx[2];
>>> + return ret;
>>> +}
>>> +
>>> +static inline int ad7194_write_reg24(struct spi_device *spi,
>>> + int reg, uint32_t *val)
>>> +{
>>> + uint8_t tx[4];
>>> +
>>> + tx[0] = (reg << COMM_ADDR_bp);
>>> + tx[1] = (*val >> 16) & 0xff;
>>> + tx[2] = (*val >> 8) & 0xff;
>>> + tx[3] = (*val >> 0) & 0xff;
>>> +
>>> + return spi_write(spi, tx, 4);
> IIRC spi_write requires dma safe buffers (cache line aligned
> buffers on the heap).
Could you expand here?
>>> +}
>>> +
>>> +static ssize_t show_voltage(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + uint32_t conf, mode, val, sign, pos_chan, neg_chan = 0;
>>> + uint8_t status, gain, diff;
>>> + int32_t whole, fract;
>>> + int ret;
>>> +
>>> + struct iio_dev *dev_info = dev_get_drvdata(dev);
>>> + struct ad7194_data *pdata = dev_info->dev_data;
> Pdata is a bad name choice. THat implies platform data (by conventional
> use). Here it is instance specific data I think? state is a typical
> variable name, or chip.
>
>>> + struct spi_device *spi = pdata->spi_dev;
>>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>>> +
>>> + pos_chan = this_attr->address;
>>> + gain = pdata->gain[this_attr->address];
>>> + diff = pdata->diff[this_attr->address];
> Just general point. Don't bother with local variables for stuff that
> is only used once. Just makes reading harder.
>>> +
>>> + if (diff = 1) {
>>> + /*if in0_diff is true, reading in0_input still returns
>>> + * in0, but it is in0-in1, if you read in1_input
>>> + * then you get in1-in0 */
> In a nutshell that explains why we don't use the interface you've gone
> with but have an explicit one for differential channels. (see the max1363
> driver for examples).
>
>>> + if ((pos_chan % 2) = 0)
>>> + neg_chan = pos_chan+1;
>>> + else
>>> + neg_chan = pos_chan-1;
>>> +
>>> + conf = CONF_CHOP_bm | CONF_BUF_bm |
>>> + (pos_chan << CONF_CHAN_POS_bp) |
>>> + (neg_chan << CONF_CHAN_NEG_bp) | gain;
>>> + } else {
>>> + conf = CONF_CHOP_bm | CONF_PSEUDO_bm | CONF_BUF_bm |
>>> + (pos_chan << CONF_CHAN_POS_bp) | gain;
>>> + }
>>> +
>>> + ret = mutex_lock_interruptible(&pdata->lock);
>>> + if (ret != 0)
>>> + return ret;
>>> +
>>> + ret = ad7194_write_reg24(spi, REG_CONF, &conf);
>>> + if (ret != 0)
>>> + goto out;
>>> +
>>> + mode = MODE_MD_SINGLE_gc | MODE_CLK_INTTRI_gc | MODE_FILTER_WORD;
>>> + ret = ad7194_write_reg24(spi, REG_MODE, &mode);
>>> + if (ret != 0)
>>> + goto out;
>>> +
>>> + msleep_interruptible(SETTLE_MS);
>>> +
>>> + ret = ad7194_read_reg8(spi, REG_STATUS, &status);
>>> + if (ret != 0)
>>> + goto out;
>>> + status = (status >> 6) & 0x01;
>>> +
>>> + ret = ad7194_read_reg24(spi, REG_DATA, &val);
>>> + if (ret != 0)
>>> + goto out;
>>> +
>>> + sign = (val & 0x800000) >> 23;
>>> + if (sign)
>>> + fract = (val & 0x7fffff);
>>> + else
>>> + fract = 0x7fffff - (val & 0x7fffff);
>>> + fract = ((int64_t)fract*4095000) >> 23;
>>> + fract = fract / gains[gain];
>>> + whole = fract / 1000000;
>>> + fract = fract % 1000000;
> As a general rule, we'd push this into userspace, but the interface
> allows for either so we can keep this as is if you really want to.
> I'd like to see a little comment explaining what the calcuation is
> though!
>>> +
>>> + if (status = 0) {
>>> + mutex_unlock(&pdata->lock);
>>> + if (sign)
>>> + return sprintf(buf, "%d.%.6d\n", whole, fract);
>>> + else
>>> + return sprintf(buf, "-%d.%.6d\n", whole, fract);
>>> + } else {
>>> + ret = -EAGAIN;
>>> + goto out;
>>> + }
>>> +
>>> +out:
>>> + mutex_unlock(&pdata->lock);
>>> + return ret;
>>> +}
>>> +
>
>
> These don't even vaguely conform to the abi. See
> drivers/staging/iio/Documentation/sysfs-bus-iio. Mostly this will
> get cleaned up in converting the driver over to chan_spec based
> registration. Hehe, this reminds me why we introduced the chan spec
> stuff in the first place!
>>> +static IIO_DEVICE_ATTR(ch1_value, S_IRUGO, show_voltage, NULL, 0);
>>> +static IIO_DEVICE_ATTR(ch2_value, S_IRUGO, show_voltage, NULL, 1);
>>> +static IIO_DEVICE_ATTR(ch3_value, S_IRUGO, show_voltage, NULL, 2);
>>> +static IIO_DEVICE_ATTR(ch4_value, S_IRUGO, show_voltage, NULL, 3);
>>> +static IIO_DEVICE_ATTR(ch5_value, S_IRUGO, show_voltage, NULL, 4);
>>> +static IIO_DEVICE_ATTR(ch6_value, S_IRUGO, show_voltage, NULL, 5);
>>> +static IIO_DEVICE_ATTR(ch7_value, S_IRUGO, show_voltage, NULL, 6);
>>> +static IIO_DEVICE_ATTR(ch8_value, S_IRUGO, show_voltage, NULL, 7);
>>> +static IIO_DEVICE_ATTR(ch9_value, S_IRUGO, show_voltage, NULL, 8);
>>> +static IIO_DEVICE_ATTR(ch10_value, S_IRUGO, show_voltage, NULL, 9);
>>> +static IIO_DEVICE_ATTR(ch11_value, S_IRUGO, show_voltage, NULL, 10);
>>> +static IIO_DEVICE_ATTR(ch12_value, S_IRUGO, show_voltage, NULL, 11);
>>> +static IIO_DEVICE_ATTR(ch13_value, S_IRUGO, show_voltage, NULL, 12);
>>> +static IIO_DEVICE_ATTR(ch14_value, S_IRUGO, show_voltage, NULL, 13);
>>> +static IIO_DEVICE_ATTR(ch15_value, S_IRUGO, show_voltage, NULL, 14);
>>> +static IIO_DEVICE_ATTR(ch16_value, S_IRUGO, show_voltage, NULL, 15);
>>> +
>>> +static IIO_DEVICE_ATTR(ch1_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 0);
>>> +static IIO_DEVICE_ATTR(ch2_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 1);
>>> +static IIO_DEVICE_ATTR(ch3_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 2);
>>> +static IIO_DEVICE_ATTR(ch4_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 3);
>>> +static IIO_DEVICE_ATTR(ch5_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 4);
>>> +static IIO_DEVICE_ATTR(ch6_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 5);
>>> +static IIO_DEVICE_ATTR(ch7_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 6);
>>> +static IIO_DEVICE_ATTR(ch8_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 7);
>>> +static IIO_DEVICE_ATTR(ch9_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 8);
>>> +static IIO_DEVICE_ATTR(ch10_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 9);
>>> +static IIO_DEVICE_ATTR(ch11_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 10);
>>> +static IIO_DEVICE_ATTR(ch12_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 11);
>>> +static IIO_DEVICE_ATTR(ch13_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 12);
>>> +static IIO_DEVICE_ATTR(ch14_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 13);
>>> +static IIO_DEVICE_ATTR(ch15_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 14);
>>> +static IIO_DEVICE_ATTR(ch16_gain, S_IWUSR | S_IRUGO, show_gain, set_gain, 15);
>>> +
>
> Umm.. What are these? I think based on quick look at the datasheet that you
> are using them to switch the _value attributes from unipolar to differential?
> Please register them as separate channels (see max1363 for lots and lots of
> examples). It can be a bit fiddly to maintain the internal state but it does
> give us a coherent general interface.
>>> +static IIO_DEVICE_ATTR(ch1_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 0);
>>> +static IIO_DEVICE_ATTR(ch2_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 1);
>>> +static IIO_DEVICE_ATTR(ch3_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 2);
>>> +static IIO_DEVICE_ATTR(ch4_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 3);
>>> +static IIO_DEVICE_ATTR(ch5_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 4);
>>> +static IIO_DEVICE_ATTR(ch6_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 5);
>>> +static IIO_DEVICE_ATTR(ch7_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 6);
>>> +static IIO_DEVICE_ATTR(ch8_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 7);
>>> +static IIO_DEVICE_ATTR(ch9_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 8);
>>> +static IIO_DEVICE_ATTR(ch10_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 9);
>>> +static IIO_DEVICE_ATTR(ch11_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 10);
>>> +static IIO_DEVICE_ATTR(ch12_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 11);
>>> +static IIO_DEVICE_ATTR(ch13_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 12);
>>> +static IIO_DEVICE_ATTR(ch14_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 13);
>>> +static IIO_DEVICE_ATTR(ch15_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 14);
>>> +static IIO_DEVICE_ATTR(ch16_diff, S_IWUSR | S_IRUGO, show_diff, set_diff, 15);
>>> +
>>> +static struct attribute *ad7194_attributes[] = {
>>> + &iio_dev_attr_ch1_value.dev_attr.attr,
>>> + &iio_dev_attr_ch2_value.dev_attr.attr,
>>> + &iio_dev_attr_ch3_value.dev_attr.attr,
>>> + &iio_dev_attr_ch4_value.dev_attr.attr,
>>> + &iio_dev_attr_ch5_value.dev_attr.attr,
>>> + &iio_dev_attr_ch6_value.dev_attr.attr,
>>> + &iio_dev_attr_ch7_value.dev_attr.attr,
>>> + &iio_dev_attr_ch8_value.dev_attr.attr,
>>> + &iio_dev_attr_ch9_value.dev_attr.attr,
>>> + &iio_dev_attr_ch10_value.dev_attr.attr,
>>> + &iio_dev_attr_ch11_value.dev_attr.attr,
>>> + &iio_dev_attr_ch12_value.dev_attr.attr,
>>> + &iio_dev_attr_ch13_value.dev_attr.attr,
>>> + &iio_dev_attr_ch14_value.dev_attr.attr,
>>> + &iio_dev_attr_ch15_value.dev_attr.attr,
>>> + &iio_dev_attr_ch16_value.dev_attr.attr,
>>> + &iio_dev_attr_ch1_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch2_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch3_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch4_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch5_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch6_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch7_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch8_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch9_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch10_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch11_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch12_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch13_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch14_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch15_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch16_gain.dev_attr.attr,
>>> + &iio_dev_attr_ch1_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch2_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch3_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch4_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch5_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch6_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch7_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch8_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch9_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch10_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch11_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch12_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch13_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch14_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch15_diff.dev_attr.attr,
>>> + &iio_dev_attr_ch16_diff.dev_attr.attr,
>>> + NULL,
>>> +};
>>> +
>>> +static const struct attribute_group ad7194_attribute_group = {
>>> + .attrs = ad7194_attributes,
>>> +};
>>> +
>>> +static const struct iio_info ad7194_info = {
>>> + .attrs = &ad7194_attribute_group,
>>> + .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +static int __devinit ad7194_probe(struct spi_device *spi)
>>> +{
>>> + int ret, err;
>>> + struct ad7194_data *pdata;
>>> +
>>> + /* Configure the SPI bus */
>>> + spi->mode = (SPI_MODE_0);
>>> + spi->bits_per_word = 8;
>>> + spi_setup(spi);
>>> +
>>> + pdata = kzalloc(sizeof(struct ad7194_data), GFP_KERNEL);
>>> + if (!pdata) {
>>> + err = -ENOMEM;
>>> + goto exit;
>>> + }
>>> +
>>> + dev_set_drvdata(&spi->dev, pdata);
>>> +
>>> + pdata->spi_dev = spi;
>>> +
>>> + pdata->indio_dev = iio_allocate_device(0);
>>> + if (pdata->indio_dev = NULL) {
>>> + ret = -ENOMEM;
>>> + goto error_free;
>>> + }
>>> +
>>> + mutex_init(&pdata->lock);
>>> + pdata->indio_dev->name = "ad7194";
>>> + pdata->indio_dev->dev.parent = &spi->dev;
>>> + pdata->indio_dev->info = &ad7194_info;
>>> + pdata->indio_dev->dev_data = (void *)pdata;
> Devdata is going away I'm afraid (only still there in
> 1 driver and that's because Jon is having issues getting
> the kernel up and running on his board to check I haven't
> messed up the conversion!) Please do this with
> iio_allocate_device(sizeof(*pdata)), then use iio_priv
> to get to the resulting private data.
>
>>> +
>>> + ret = iio_device_register(pdata->indio_dev);
>>> + if (ret)
>>> + goto error_free_dev;
>>> +
>>> + return 0;
>>> +
>>> +error_free_dev:
>>> + iio_free_device(pdata->indio_dev);
>>> +error_free:
>>> + kfree(pdata);
>>> +exit:
>>> + return err;
>>> +}
>>> +
>>> +static int __devexit ad7194_remove(struct spi_device *spi)
>>> +{
>>> + struct ad7194_data *pdata = dev_get_drvdata(&spi->dev);
>>> +
>>> + dev_set_drvdata(&spi->dev, NULL);
>>> + iio_device_unregister(pdata->indio_dev);
>>> + iio_free_device(pdata->indio_dev);
>>> + kfree(pdata);
>>> + return 0;
>>> +}
>>> +
>>> +static struct spi_driver ad7194_driver = {
>>> + .driver = {
> I've never understood why people like having a define
> for the part name. Much better to just put it here
> where everyone expects to find it!
>
>>> + .name = DEVICE_NAME,
>>> + .bus = &spi_bus_type,
>>> + .owner = THIS_MODULE,
>>> + },
>>> +
>>> + .probe = ad7194_probe,
>>> + .remove = __devexit_p(ad7194_remove),
>>> +};
>>> +
>>> +static int __init ad7194_init(void)
>>> +{
>>> + return spi_register_driver(&ad7194_driver);
>>> +}
>>> +
>>> +static void __exit ad7194_exit(void)
>>> +{
>>> + spi_unregister_driver(&ad7194_driver);
>>> +}
>>> +
>>> +module_init(ad7194_init);
>>> +module_exit(ad7194_exit);
>>> +
>>> +MODULE_AUTHOR("Paul Thomas <pthomas8589@gmail.com>");
>>> +MODULE_DESCRIPTION("TI AD7194 A/D driver");
>>> +MODULE_LICENSE("GPL");
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194
2011-07-18 7:46 [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 a/d Paul Thomas
2011-07-18 11:01 ` [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 Jonathan Cameron
@ 2011-07-18 21:45 ` Jonathan Cameron
1 sibling, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2011-07-18 21:45 UTC (permalink / raw)
To: lm-sensors
On 07/18/11 18:11, Paul Thomas wrote:
> Aside from the ABI stuff, I had a couple responses to your comments.
>
> thanks,
> Paul
...
>>>> +static inline int ad7194_read_reg24(struct spi_device *spi,
>>>> + int reg, uint32_t *val)
>>>> +{
>>>> + int ret;
>>>> + uint8_t tx[1], rx[3];
>>>> +
>>>> + tx[0] = (COMM_READ_bm | (reg << COMM_ADDR_bp));
>> Don't allocate single element arrays. Just makes things
>> harder to read.
> I was trying to keep consistency between the _reg8 & _reg24 versions
> as well as between the rx & tx buffers. It makes the
> spi_write_then_read(spi, tx, 1, rx, 3) read nicely, but if that's a
> no-no then I can change it.
Not really important so either is fine.
>
>>
>>>> +
>>>> + ret = spi_write_then_read(spi, tx, 1, rx, 3);
>>>> + *val = (rx[0] << 16) + (rx[1] << 8) + rx[2];
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static inline int ad7194_write_reg24(struct spi_device *spi,
>>>> + int reg, uint32_t *val)
>>>> +{
>>>> + uint8_t tx[4];
>>>> +
>>>> + tx[0] = (reg << COMM_ADDR_bp);
>>>> + tx[1] = (*val >> 16) & 0xff;
>>>> + tx[2] = (*val >> 8) & 0xff;
>>>> + tx[3] = (*val >> 0) & 0xff;
>>>> +
>>>> + return spi_write(spi, tx, 4);
>> IIRC spi_write requires dma safe buffers (cache line aligned
>> buffers on the heap).
> Could you expand here?
>
Basically it boils down to meaning that all spi buffers (other than
through write_then_read (which copies the data) have to be allocated
using kmalloc or equilvalent. If you want to embed the in the
chip specific structure, then this can be done by using ____cachline_aligned
(grep for it in iio drivers to see what I mean).
The issue is that if you issue a dma request to an spi controller, it can
mess with the data. This can mean the contents of memory around the dma
buffer in the processor cache can become different from that in memory.
The solution is to ensure nothing else sits in the cacheline (the chunk
of memory on which the cache operates). This is always true for kmalloc'd
memory. The ____cacheline_aligned trick ensures that enough padding is added
to make this rule still be obeyed even when allocating a larger structure.
It's a common trap people fall into the first time they write an spi driver.
Note that spi_write_then read is obviously a slow option hence the comments
in the header (or maybe the c file) explaining why it is a bad idea for anything
other than small infrequent use cases.
Jonathan
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-07-18 21:45 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-18 7:46 [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 a/d Paul Thomas
2011-07-18 11:01 ` [PATCH] iio: add support for Analog Devices ad7194 a/d converter Jonathan Cameron
2011-07-18 11:01 ` [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 Jonathan Cameron
2011-07-18 12:24 ` [PATCH] iio: add support for Analog Devices ad7194 a/d converter Jonathan Cameron
2011-07-18 12:24 ` [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 Jonathan Cameron
2011-07-18 16:03 ` [PATCH] iio: add support for Analog Devices ad7194 a/d converter Paul Thomas
2011-07-18 16:03 ` [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 Paul Thomas
2011-07-18 16:11 ` [PATCH] iio: add support for Analog Devices ad7194 a/d converter Jonathan Cameron
2011-07-18 16:11 ` [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 Jonathan Cameron
2011-07-18 17:11 ` [PATCH] iio: add support for Analog Devices ad7194 a/d converter Paul Thomas
2011-07-18 17:11 ` [lm-sensors] [PATCH] iio: add support for Analog Devices ad7194 Paul Thomas
2011-07-18 21:45 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.