On 02/03/11 14:51, michael.hennerich@xxxxxxxxxx wrote:
> From: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
>
> This patch adds support for the:
> AD7606/AD7606-6/AD7606-4 8/6/4-Channel Data Acquisition
> system (DAS) with 16-Bit, Bipolar, Simultaneous Sampling ADC.
Hi Michael,
Another nice clean driver, various points inline with the code.
With the parallel interface, I'll admit I've not seen one of these
before so my questions may be somewhat uninformed ;) On that note
I'd also like to have some more eyes on that element if possible.
(looks fine to me)
The big one is whether the assumption that all such interfaces
are going to be memory mappable is reasonable or whether there
should be some abstraction in below the driver? I'd like to see
some more documentation on the requirements of that interface,
even if just a quick explanation for those not familiar with this
sort of interface within the kernel.
>
> Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
> ---
> drivers/staging/iio/Documentation/sysfs-bus-iio | 25 +
> drivers/staging/iio/adc/Kconfig | 27 ++
> drivers/staging/iio/adc/Makefile | 6 +
> drivers/staging/iio/adc/ad7606.h | 119 +++++
> drivers/staging/iio/adc/ad7606_core.c | 560 +++++++++++++++++++++++
> drivers/staging/iio/adc/ad7606_par.c | 191 ++++++++
> drivers/staging/iio/adc/ad7606_ring.c | 261 +++++++++++
> drivers/staging/iio/adc/ad7606_spi.c | 133 ++++++
> 8 files changed, 1322 insertions(+), 0 deletions(-)
> create mode 100644 drivers/staging/iio/adc/ad7606.h
> create mode 100644 drivers/staging/iio/adc/ad7606_core.c
> create mode 100644 drivers/staging/iio/adc/ad7606_par.c
> create mode 100644 drivers/staging/iio/adc/ad7606_ring.c
> create mode 100644 drivers/staging/iio/adc/ad7606_spi.c
>
> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio b/drivers/staging/iio/Documentation/sysfs-bus-iio
> index 8e5d8d1..b415019 100644
> --- a/drivers/staging/iio/Documentation/sysfs-bus-iio
> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio
> @@ -53,6 +53,31 @@ Description:
> When the internal sampling clock can only take a small
> discrete set of values, this file lists those available.
>
> +What: /sys/bus/iio/devices/deviceX/range
> +KernelVersion: 2.6.38
> +Contact: linux-iio@xxxxxxxxxxxxxxx
> +Description:
> + Hardware dependent ADC Full Scale Range.
Could you give an illustrative example of a usecase where this is necessary?
I'm generally a bit dubious about the obviously considerable overlap between
this and the _scale and _type attributes. I guess having them yoked together
as you have here works out fine. We might want to set a preference though.
Perhaps always have _scale as the controlable option and range as the dependant
value?
> +
> +What: /sys/bus/iio/devices/deviceX/range_available
> +KernelVersion: 2.6.38
> +Contact: linux-iio@xxxxxxxxxxxxxxx
> +Description:
> + Hardware dependent supported vales for ADC Full Scale Range.
> +
> +What: /sys/bus/iio/devices/deviceX/oversampling
> +KernelVersion: 2.6.38
> +Contact: linux-iio@xxxxxxxxxxxxxxx
> +Description:
> + Hardware dependent ADC oversamplimg.
Typo.
> Controls the sampling ratio
> + of the digital filter if available.
What are the units? The obvious choice is a multiplier, but could be frequency.
Either way, needs to be specified here.
> +
> +What: /sys/bus/iio/devices/deviceX/oversampling_available
> +KernelVersion: 2.6.38
> +Contact: linux-iio@xxxxxxxxxxxxxxx
> +Description:
> + Hardware dependent supported vales
values (and should probably be Hardware dependent values supported by the
oversampling filter.)
> by the oversampling filter.
> +
> What: /sys/bus/iio/devices/deviceX/inY_raw
> What: /sys/bus/iio/devices/deviceX/inY_supply_raw
> KernelVersion: 2.6.35
> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
> index 86869cd..70767ff 100644
> --- a/drivers/staging/iio/adc/Kconfig
> +++ b/drivers/staging/iio/adc/Kconfig
> @@ -62,6 +62,33 @@ config AD7314
> Say yes here to build support for Analog Devices AD7314
> temperature sensors.
>
> +config AD7606
> + tristate "Analog Devices AD7606 ADC driver"
> + select IIO_RING_BUFFER
> + select IIO_TRIGGER
> + select IIO_SW_RING
Looks like a generic gpio dependency is need as well?
> + help
> + Say yes here to build support for Analog Devices:
> + ad7606, ad7606-6, ad7606-4 analog to digital convertors (ADC).
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ad7606.
> +
> +config AD7606_IFACE_PARALLEL
> + bool "parallel interface support"
> + depends on AD7606
> + help
> + Say yes here to include parallel interface support on the AD7606
> + ADC driver.
> +
> +config AD7606_IFACE_SPI
> + bool "spi interface support"
> + depends on AD7606
> + depends on SPI
> + help
> + Say yes here to include parallel interface support on the AD7606
> + ADC driver.
> +
> config AD799X
> tristate "Analog Devices AD799x ADC driver"
> depends on I2C
> diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
> index 6f231a2..cb73346 100644
> --- a/drivers/staging/iio/adc/Makefile
> +++ b/drivers/staging/iio/adc/Makefile
> @@ -7,6 +7,12 @@ max1363-y += max1363_ring.o
>
> obj-$(CONFIG_MAX1363) += max1363.o
>
> +ad7606-y := ad7606_core.o
> +ad7606-$(CONFIG_IIO_RING_BUFFER) += ad7606_ring.o
> +ad7606-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
> +ad7606-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
> +obj-$(CONFIG_AD7606) += ad7606.o
> +
> ad799x-y := ad799x_core.o
> ad799x-$(CONFIG_AD799X_RING_BUFFER) += ad799x_ring.o
> obj-$(CONFIG_AD799X) += ad799x.o
> diff --git a/drivers/staging/iio/adc/ad7606.h b/drivers/staging/iio/adc/ad7606.h
> new file mode 100644
> index 0000000..99f21c5
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7606.h
> @@ -0,0 +1,119 @@
> +/*
> + * AD7606 ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#ifndef IIO_ADC_AD7606_H_
> +#define IIO_ADC_AD7606_H_
> +
Only one use. I'd put it inline.
> +#define DRVNAME "iio-ad7606"
> +
> +/*
> + * TODO: struct ad7606_platform_data needs to go into include/linux/iio
> + */
> +
> +/**
> + * struct ad7606_platform_data - platform/board specifc information
> + * @default_os: default oversampling value {0, 2, 4, 8, 16, 32, 64}
> + * @default_range: default range +/-{5, 10} Volt
> + * @gpio_convst: number of gpio connected to the CONVST pin
> + * @gpio_reset: gpio connected to the RESET pin, if not used set to -1
> + * @gpio_range: gpio connected to the RANGE pin, if not used set to -1
> + * @gpio_os0: gpio connected to the OS0 pin, if not used set to -1
> + * @gpio_os1: gpio connected to the OS1 pin, if not used set to -1
> + * @gpio_os2: gpio connected to the OS2 pin, if not used set to -1
> + * @gpio_frstdata: gpio connected to the FRSTDAT pin, if not used set to -1
> + * @gpio_stby: gpio connected to the STBY pin, if not used set to -1
> + */
> +
> +struct ad7606_platform_data {
> + unsigned default_os;
> + unsigned default_range;
> + unsigned gpio_convst;
> + unsigned gpio_reset;
> + unsigned gpio_range;
> + unsigned gpio_os0;
> + unsigned gpio_os1;
> + unsigned gpio_os2;
> + unsigned gpio_frstdata;
> + unsigned gpio_stby;
> +};
> +
> +/**
> + * struct ad7606_chip_info - chip specifc information
> + * @name: indentification string for chip
> + * @bits: accuracy of the adc in bits
> + * @bits: output coding [s]igned or [u]nsigned
> + * @int_vref_mv: the internal reference voltage
> + * @num_channels: number of physical inputs on chip
> + */
> +
> +struct ad7606_chip_info {
> + char name[10];
> + u8 bits;
> + char sign;
> + u16 int_vref_mv;
> + unsigned num_channels;
> +};
> +
> +/**
> + * struct ad7606_state - driver instance specific data
> + */
> +
> +struct ad7606_state {
> + struct iio_dev *indio_dev;
> + struct device *dev;
> + const struct ad7606_chip_info *chip_info;
> + struct ad7606_platform_data *pdata;
> + struct regulator *reg;
> + struct work_struct poll_work;
> + wait_queue_head_t wq_data_avail;
> + atomic_t protect_ring;
> + size_t d_size;
> + const struct ad7606_bus_ops *bops;
> + int irq;
> + unsigned id;
> + unsigned range;
> + unsigned oversampling;
> + bool done;
> + bool have_frstdata;
> + bool have_os;
> + bool have_stby;
> + bool have_reset;
> + bool have_range;
> + void __iomem *base_address;
> +
> + /*
> + * DMA (thus cache coherency maintenance) requires the
> + * transfer buffers to live in their own cache lines.
> + */
> +
> + unsigned short data[8] ____cacheline_aligned;
> +};
> +
> +struct ad7606_bus_ops {
> + /* more methods added in future? */
> + int (*read_block)(struct device *, int, void *);
> +};
> +
> +void ad7606_suspend(struct ad7606_state *st);
> +void ad7606_resume(struct ad7606_state *st);
> +struct ad7606_state *ad7606_probe(struct device *dev, int irq,
> + void __iomem *base_address, unsigned id,
> + const struct ad7606_bus_ops *bops);
> +int ad7606_remove(struct ad7606_state *st);
> +int ad7606_reset(struct ad7606_state *st);
> +
> +enum ad7606_supported_device_ids {
> + ID_AD7606_8,
> + ID_AD7606_6,
> + ID_AD7606_4
> +};
> +
> +int ad7606_scan_from_ring(struct ad7606_state *st, unsigned ch);
> +int ad7606_register_ring_funcs_and_init(struct iio_dev *indio_dev);
> +void ad7606_ring_cleanup(struct iio_dev *indio_dev);
> +#endif /* IIO_ADC_AD7606_H_ */
> diff --git a/drivers/staging/iio/adc/ad7606_core.c b/drivers/staging/iio/adc/ad7606_core.c
> new file mode 100644
> index 0000000..21e5af8
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7606_core.c
> @@ -0,0 +1,560 @@
> +/*
> + * AD7606 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/list.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/delay.h>
> +
> +#include "../iio.h"
> +#include "../sysfs.h"
> +#include "../ring_generic.h"
> +#include "adc.h"
> +
> +#include "ad7606.h"
> +
> +int ad7606_reset(struct ad7606_state *st)
> +{
> + if (st->have_reset) {
> + gpio_set_value(st->pdata->gpio_reset, 1);
> + ndelay(100); /* t_reset >= 100ns */
could it be a sleep?
> + gpio_set_value(st->pdata->gpio_reset, 0);
> + return 0;
> + }
> +
> + return -ENODEV;
> +}
> +
> +static int ad7606_scan_direct(struct ad7606_state *st, unsigned ch)
> +{
> + int ret;
> +
> + gpio_set_value(st->pdata->gpio_convst, 1);
> + st->done = false;
> +
> + ret = wait_event_interruptible(st->wq_data_avail, st->done);
> + if (ret)
> + goto error_ret;
> +
> + if (st->have_frstdata) {
> + st->bops->read_block(st->dev, 1, st->data);
> + if (!gpio_get_value(st->pdata->gpio_frstdata)) {
> + /* This should never happen */
As below.... does it and if so why?
> + ad7606_reset(st);
> + ret = -EIO;
> + goto error_ret;
> + }
> + st->bops->read_block(st->dev,
> + st->chip_info->num_channels - 1, &st->data[1]);
> + } else {
> + st->bops->read_block(st->dev,
> + st->chip_info->num_channels, st->data);
> + }
> +
> + ret = st->data[ch];
> +
> +error_ret:
> + gpio_set_value(st->pdata->gpio_convst, 0);
> +
> + return ret;
> +}
> +
> +static ssize_t ad7606_scan(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7606_state *st = dev_info->dev_data;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> + int ret;
> +
> + mutex_lock(&dev_info->mlock);
> + if (iio_ring_enabled(dev_info))
> + ret = ad7606_scan_from_ring(st, this_attr->address);
> + else
> + ret = ad7606_scan_direct(st, this_attr->address);
> + mutex_unlock(&dev_info->mlock);
> +
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", (short) ret);
> +}
> +
> +static IIO_DEV_ATTR_IN_RAW(0, ad7606_scan, 0);
> +static IIO_DEV_ATTR_IN_RAW(1, ad7606_scan, 1);
> +static IIO_DEV_ATTR_IN_RAW(2, ad7606_scan, 2);
> +static IIO_DEV_ATTR_IN_RAW(3, ad7606_scan, 3);
> +static IIO_DEV_ATTR_IN_RAW(4, ad7606_scan, 4);
> +static IIO_DEV_ATTR_IN_RAW(5, ad7606_scan, 5);
> +static IIO_DEV_ATTR_IN_RAW(6, ad7606_scan, 6);
> +static IIO_DEV_ATTR_IN_RAW(7, ad7606_scan, 7);
> +
> +static ssize_t ad7606_show_scale(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + /* Driver currently only support internal vref */
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7606_state *st = iio_dev_get_devdata(dev_info);
> + unsigned int scale_uv = (st->range * 1000000) >> st->chip_info->bits;
> +
> + return sprintf(buf, "%d.%03d\n", scale_uv / 1000, scale_uv % 1000);
> +}
> +static IIO_DEVICE_ATTR(in_scale, S_IRUGO, ad7606_show_scale, NULL, 0);
> +
> +static ssize_t ad7606_show_name(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7606_state *st = iio_dev_get_devdata(dev_info);
> +
> + return sprintf(buf, "%s\n", st->chip_info->name);
> +}
> +
> +static IIO_DEVICE_ATTR(name, S_IRUGO, ad7606_show_name, NULL, 0);
> +
> +static ssize_t ad7606_show_range(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7606_state *st = iio_dev_get_devdata(dev_info);
> +
> + return sprintf(buf, "%u\n", st->range);
> +}
> +
> +static ssize_t ad7606_store_range(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7606_state *st = iio_dev_get_devdata(dev_info);
> + unsigned long lval;
> +
These are voltage values, so should be in millivolts? (as
we lift the nearest interface from hwmon)
> + if (strict_strtoul(buf, 10, &lval))
> + return -EINVAL;
> + if (!(lval == 5 || lval == 10)) {
> + dev_err(dev, "range is not supported\n");
> + return -EINVAL;
> + }
> + mutex_lock(&dev_info->mlock);
> + gpio_set_value(st->pdata->gpio_range, lval == 10);
> + st->range = lval;
> + mutex_unlock(&dev_info->mlock);
> +
> + return count;
> +}
> +
> +static IIO_DEVICE_ATTR(range, S_IRUGO | S_IWUSR, \
> + ad7606_show_range, ad7606_store_range, 0);
> +static IIO_CONST_ATTR(range_available, "5 10");
> +
> +static ssize_t ad7606_show_oversampling(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7606_state *st = iio_dev_get_devdata(dev_info);
> +
> + return sprintf(buf, "%u\n", st->oversampling);
> +}
> +
Name is a little missleading. It is actually finding the index and name
should reflect that.
> +static int ad7606_check_oversampling(unsigned val)
> +{
> + unsigned char supported[] = {0, 2, 4, 8, 16, 32, 64};
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(supported); i++)
> + if (val == supported[i])
> + return i;
> +
> + return -EINVAL;
> +}
> +
> +static ssize_t ad7606_store_oversampling(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7606_state *st = iio_dev_get_devdata(dev_info);
> + unsigned long lval;
> + int ret;
> +
> + if (strict_strtoul(buf, 10, &lval))
> + return -EINVAL;
> +
> + ret = ad7606_check_oversampling(lval);
> + if (ret < 0) {
> + dev_err(dev, "oversampling %lu is not supported\n", lval);
> + return ret;
> + }
> +
> + mutex_lock(&dev_info->mlock);
> + gpio_set_value(st->pdata->gpio_os0, (ret >> 0) & 1);
> + gpio_set_value(st->pdata->gpio_os1, (ret >> 1) & 1);
> + gpio_set_value(st->pdata->gpio_os1, (ret >> 2) & 1);
> + st->oversampling = lval;
> + mutex_unlock(&dev_info->mlock);
> +
> + return count;
> +}
> +
> +static IIO_DEVICE_ATTR(oversampling, S_IRUGO | S_IWUSR, \
Need the '\' ?
> + ad7606_show_oversampling, ad7606_store_oversampling, 0);
> +static IIO_CONST_ATTR(oversampling_available, "0 2 4 8 16 32 64");
> +
> +static struct attribute *ad7606_attributes[] = {
> + &iio_dev_attr_in0_raw.dev_attr.attr,
> + &iio_dev_attr_in1_raw.dev_attr.attr,
> + &iio_dev_attr_in2_raw.dev_attr.attr,
> + &iio_dev_attr_in3_raw.dev_attr.attr,
> + &iio_dev_attr_in4_raw.dev_attr.attr,
> + &iio_dev_attr_in5_raw.dev_attr.attr,
> + &iio_dev_attr_in6_raw.dev_attr.attr,
> + &iio_dev_attr_in7_raw.dev_attr.attr,
> + &iio_dev_attr_in_scale.dev_attr.attr,
> + &iio_dev_attr_name.dev_attr.attr,
> + &iio_dev_attr_range.dev_attr.attr,
> + &iio_const_attr_range_available.dev_attr.attr,
> + &iio_dev_attr_oversampling.dev_attr.attr,
> + &iio_const_attr_oversampling_available.dev_attr.attr,
> + NULL,
> +};
> +
> +static mode_t ad7606_attr_is_visible(struct kobject *kobj,
> + struct attribute *attr, int n)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct iio_dev *dev_info = dev_get_drvdata(dev);
> + struct ad7606_state *st = iio_dev_get_devdata(dev_info);
> +
> + mode_t mode = attr->mode;
> +
As with the other is_visible function, I'd rather see this use the
chip_info->num_channels element instead of the ids. Makes it very
obvious what is happening.
Then becomes
if (st->chip_info->num_channels < 6 &
(attr == &iio_dev_attr_in7_raw.dev_attr.attr ||
attr == &iio_dev_attr_in6_raw.dev_attr.attr)
mode = 0;
if (st->chip_info->num_channels < 4 &
(attr == &iio_dev_attr_in5_raw.dev_attr.attr ||
attr == &iio_dev_attr_in4_raw.dev_attr.attr)
mode = 0;
(other the same - though I'd reorder so first thing one
sees is the cause, then the attributes. More readable in
my opinion).
Probably not worth optimizing the order in this function.
> + if ((attr == &iio_dev_attr_in7_raw.dev_attr.attr) &&
> + ((st->id == ID_AD7606_6) || (st->id == ID_AD7606_4)))
> + mode = 0;
> + else if ((attr == &iio_dev_attr_in6_raw.dev_attr.attr) &&
> + ((st->id == ID_AD7606_6) || (st->id == ID_AD7606_4)))
> + mode = 0;
> + else if ((attr == &iio_dev_attr_in5_raw.dev_attr.attr) &&
> + (st->id == ID_AD7606_4))
> + mode = 0;
> + else if ((attr == &iio_dev_attr_in4_raw.dev_attr.attr) &&
> + (st->id == ID_AD7606_4))
> + mode = 0;
> + else if ((attr == &iio_dev_attr_oversampling.dev_attr.attr ||
> + attr == &iio_const_attr_oversampling_available.dev_attr.attr) &&
> + !st->have_os)
> + mode = 0;
> + else if ((attr == &iio_dev_attr_range.dev_attr.attr ||
> + attr == &iio_const_attr_range_available.dev_attr.attr) &&
> + !st->have_range)
> + mode = 0;
> +
> + return mode;
> +}
> +
> +static const struct attribute_group ad7606_attribute_group = {
> + .attrs = ad7606_attributes,
> + .is_visible = ad7606_attr_is_visible,
> +};
> +
> +static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
> + /*
> + * More devices added in future
Good. Otherwise I'd moan about the fact that these are nearly
identical and you could hard code most of them into the driver.
> + */
> + [ID_AD7606_8] = {
> + .name = "ad7606",
> + .bits = 16,
> + .sign = IIO_SCAN_EL_TYPE_SIGNED,
> + .int_vref_mv = 2500,
> + .num_channels = 8,
> + },
> + [ID_AD7606_6] = {
> + .name = "ad7606-6",
> + .bits = 16,
> + .sign = IIO_SCAN_EL_TYPE_SIGNED,
> + .int_vref_mv = 2500,
> + .num_channels = 6,
> + },
> + [ID_AD7606_4] = {
> + .name = "ad7606-4",
> + .bits = 16,
> + .sign = IIO_SCAN_EL_TYPE_SIGNED,
> + .int_vref_mv = 2500,
> + .num_channels = 4,
> + },
> +};
> +
> +static int ad7606_request_gpio_out(struct ad7606_state *st,
> + unsigned gpio, unsigned val)
> +{
> + int ret = -EINVAL;
> +
> + if (gpio_is_valid(gpio)) {
> + ret = gpio_request(gpio, st->chip_info->name);
> + if (!ret)
> + gpio_direction_output(gpio, val);
> + }
> + return ret;
> +}
> +
> +static int ad7606_request_gpios(struct ad7606_state *st)
> +{
> + int ret;
> +
> + ret = ad7606_request_gpio_out(st, st->pdata->gpio_convst, 0);
> + if (ret) {
> + dev_err(st->dev, "failed to request GPIO CONVST\n");
> + return ret;
> + }
> +
> + ret = ad7606_request_gpio_out(st, st->pdata->gpio_os0,
> + (st->oversampling >> 0) & 1);
> + if (ret < 0) {
> + st->have_os = false;
> + } else {
> + ret = ad7606_request_gpio_out(st, st->pdata->gpio_os1,
> + (st->oversampling >> 1) & 1);
st->oversampling & 2 ?
> + if (ret < 0) {
> + gpio_free(st->pdata->gpio_os0);
> + st->have_os = false;
> + } else {
> + ret = ad7606_request_gpio_out(st, st->pdata->gpio_os2,
> + (st->oversampling >> 2) & 1);
st->oversampling & 4 or st->oversampling & (1 << 2) if you want to make it explicity.
Compiler will neatly clean that up into a constant whereas it may not this way around.
> + if (ret < 0) {
> + gpio_free(st->pdata->gpio_os0);
> + gpio_free(st->pdata->gpio_os1);
> + st->have_os = false;
> + } else {
> + st->have_os = true;
> + }
> + }
> + }
> +
> + ret = ad7606_request_gpio_out(st, st->pdata->gpio_reset, 0);
> + if (!ret)
> + st->have_reset = true;
> +
> + ret = ad7606_request_gpio_out(st, st->pdata->gpio_range,
> + st->range == 10);
> + if (!ret)
> + st->have_range = true;
> +
> + ret = ad7606_request_gpio_out(st, st->pdata->gpio_stby, 1);
> + if (!ret)
> + st->have_stby = true;
> +
> + if (gpio_is_valid(st->pdata->gpio_frstdata)) {
> + ret = gpio_request(st->pdata->gpio_frstdata,
> + st->chip_info->name);
Looks like you register a number of gpio's with the same name. Would be
nice to be a little more specific on what they are...
> + if (!ret) {
> + gpio_direction_input(st->pdata->gpio_frstdata);
> + st->have_frstdata = true;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void ad7606_free_gpios(struct ad7606_state *st)
> +{
> + if (st->have_range)
> + gpio_free(st->pdata->gpio_range);
> +
> + if (st->have_stby)
> + gpio_free(st->pdata->gpio_stby);
> +
> + if (st->have_os) {
> + gpio_free(st->pdata->gpio_os0);
> + gpio_free(st->pdata->gpio_os1);
> + gpio_free(st->pdata->gpio_os2);
> + }
> +
> + if (st->have_reset)
> + gpio_free(st->pdata->gpio_reset);
> +
> + if (st->have_frstdata)
> + gpio_free(st->pdata->gpio_frstdata);
> +
> + gpio_free(st->pdata->gpio_convst);
> +}
> +
> +/**
> + * Interrupt handler
> + */
> +static irqreturn_t ad7606_interrupt(int irq, void *dev_id)
> +{
> + struct ad7606_state *st = dev_id;
> +
> + if (iio_ring_enabled(st->indio_dev)) {
> + if (!work_pending(&st->poll_work))
> + schedule_work(&st->poll_work);
> + } else {
> + st->done = true;
> + wake_up_interruptible(&st->wq_data_avail);
> + }
> +
> + return IRQ_HANDLED;
> +};
> +
> +struct ad7606_state *ad7606_probe(struct device *dev, int irq,
> + void __iomem *base_address,
> + unsigned id,
> + const struct ad7606_bus_ops *bops)
> +{
> + struct ad7606_platform_data *pdata = dev->platform_data;
> + struct ad7606_state *st;
> + int ret;
> +
> + st = kzalloc(sizeof(*st), GFP_KERNEL);
> + if (st == NULL) {
> + ret = -ENOMEM;
> + goto error_ret;
> + }
> +
> + st->dev = dev;
> + st->id = id;
> + st->irq = irq;
> + st->bops = bops;
> + st->base_address = base_address;
> + st->range = pdata->default_range == 10 ? 10 : 5;
> +
> + ret = ad7606_check_oversampling(pdata->default_os);
> + if (ret < 0) {
> + dev_warn(dev, "oversampling %d is not supported\n",
> + pdata->default_os);
> + st->oversampling = 0;
> + } else {
> + st->oversampling = pdata->default_os;
> + }
> +
> + st->reg = regulator_get(dev, "vcc");
> + if (!IS_ERR(st->reg)) {
> + ret = regulator_enable(st->reg);
> + if (ret)
> + goto error_put_reg;
> + }
> +
> + st->pdata = pdata;
> + st->chip_info = &ad7606_chip_info_tbl[id];
> +
> + atomic_set(&st->protect_ring, 0);
> +
> + st->indio_dev = iio_allocate_device();
> + if (st->indio_dev == NULL) {
> + ret = -ENOMEM;
> + goto error_disable_reg;
> + }
> +
> + st->indio_dev->dev.parent = dev;
> + st->indio_dev->attrs = &ad7606_attribute_group;
> + st->indio_dev->dev_data = (void *)(st);
> + st->indio_dev->driver_module = THIS_MODULE;
> + st->indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + init_waitqueue_head(&st->wq_data_avail);
> +
> + ret = ad7606_request_gpios(st);
> + if (ret)
> + goto error_free_device;
> +
> + ret = ad7606_reset(st);
> + if (ret)
> + dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n");
> +
> + ret = request_irq(st->irq, ad7606_interrupt,
> + IRQF_TRIGGER_FALLING, st->chip_info->name, st);
> + if (ret)
> + goto error_free_gpios;
> +
> + ret = ad7606_register_ring_funcs_and_init(st->indio_dev);
> + if (ret)
> + goto error_free_irq;
> +
> + ret = iio_device_register(st->indio_dev);
> + if (ret)
> + goto error_free_irq;
> +
> + ret = iio_ring_buffer_register(st->indio_dev->ring, 0);
> + if (ret)
> + goto error_cleanup_ring;
> +
> + return st;
> +
> +error_cleanup_ring:
> + ad7606_ring_cleanup(st->indio_dev);
> + iio_device_unregister(st->indio_dev);
> +
> +error_free_irq:
> + free_irq(st->irq, st);
> +
> +error_free_gpios:
> + ad7606_free_gpios(st);
> +
> +error_free_device:
> + iio_free_device(st->indio_dev);
> +
> +error_disable_reg:
> + if (!IS_ERR(st->reg))
> + regulator_disable(st->reg);
> +error_put_reg:
> + if (!IS_ERR(st->reg))
> + regulator_put(st->reg);
> + kfree(st);
> +error_ret:
> + return ERR_PTR(ret);
> +}
> +
> +int ad7606_remove(struct ad7606_state *st)
> +{
> + struct iio_dev *indio_dev = st->indio_dev;
> + iio_ring_buffer_unregister(indio_dev->ring);
> + ad7606_ring_cleanup(indio_dev);
> + iio_device_unregister(indio_dev);
> + free_irq(st->irq, st);
> + if (!IS_ERR(st->reg)) {
> + regulator_disable(st->reg);
> + regulator_put(st->reg);
> + }
> +
> + ad7606_free_gpios(st);
> +
> + kfree(st);
> + return 0;
> +}
> +
> +void ad7606_suspend(struct ad7606_state *st)
> +{
> + if (st->have_stby) {
> + if (st->have_range)
> + gpio_set_value(st->pdata->gpio_range, 1);
> + gpio_set_value(st->pdata->gpio_stby, 0);
> +
Bonus white line (I'm getting fussy).
> + }
> +}
> +
> +void ad7606_resume(struct ad7606_state *st)
> +{
> + if (st->have_stby) {
> + if (st->have_range)
> + gpio_set_value(st->pdata->gpio_range, st->range == 10);
> +
> + gpio_set_value(st->pdata->gpio_stby, 1);
> + ad7606_reset(st);
> + }
> +}
> +
> +MODULE_AUTHOR("Michael Hennerich <hennerich@xxxxxxxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/staging/iio/adc/ad7606_par.c b/drivers/staging/iio/adc/ad7606_par.c
> new file mode 100644
> index 0000000..d787c06
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7606_par.c
> @@ -0,0 +1,191 @@
> +/*
> + * AD7606 Parallel Interface ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +
> +#include "ad7606.h"
> +
> +static int ad7606_par16_read_block(struct device *dev,
> + int count, void *buf)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct ad7606_state *st = platform_get_drvdata(pdev);
> +
> + insw((unsigned long) st->base_address, buf, count);
> +
> + return 0;
> +}
> +
> +static const struct ad7606_bus_ops ad7606_par16_bops = {
> + .read_block = ad7606_par16_read_block,
> +};
> +
> +static int ad7606_par8_read_block(struct device *dev,
> + int count, void *buf)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct ad7606_state *st = platform_get_drvdata(pdev);
> +
> + insb((unsigned long) st->base_address, buf, count * 2);
> +
> + return 0;
> +}
> +
> +static const struct ad7606_bus_ops ad7606_par8_bops = {
> + .read_block = ad7606_par8_read_block,
> +};
> +
> +static int __devinit ad7606_par_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + struct ad7606_state *st;
> + void __iomem *addr;
> + resource_size_t remap_size;
> + int ret, irq;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "no irq\n");
> + return -ENODEV;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> + remap_size = resource_size(res);
> +
Nitpick. I think this the only time you use this particular syntax
for comments. Please stick to /* ... */ on one line.
> + /*
> + * Request the regions.
> + */
> + if (!request_mem_region(res->start, remap_size, DRVNAME)) {
> + ret = -EBUSY;
> + goto out1;
> + }
> + addr = ioremap(res->start, remap_size);
> + if (!addr) {
> + ret = -ENOMEM;
> + goto out1;
> + }
> +
> + st = ad7606_probe(&pdev->dev, irq, addr,
> + platform_get_device_id(pdev)->driver_data,
> + remap_size > 1 ? &ad7606_par16_bops :
> + &ad7606_par8_bops);
> +
> + if (IS_ERR(st)) {
> + ret = PTR_ERR(st);
> + goto out2;
> + }
> +
> + platform_set_drvdata(pdev, st);
> +
> + return 0;
> +
> +out2:
> + iounmap(addr);
> +out1:
> + release_mem_region(res->start, remap_size);
> +
> + return ret;
> +}
> +
> +static int __devexit ad7606_par_remove(struct platform_device *pdev)
> +{
> + struct ad7606_state *st = platform_get_drvdata(pdev);
> + struct resource *res;
> +
> + ad7606_remove(st);
> +
> + iounmap(st->base_address);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + release_mem_region(res->start, resource_size(res));
> +
> + platform_set_drvdata(pdev, NULL);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int ad7606_par_suspend(struct device *dev)
> +{
> + struct ad7606_state *st = dev_get_drvdata(dev);
> +
> + ad7606_suspend(st);
> +
> + return 0;
> +}
> +
> +static int ad7606_par_resume(struct device *dev)
> +{
> + struct ad7606_state *st = dev_get_drvdata(dev);
> +
> + ad7606_resume(st);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ad7606_pm_ops = {
> + .suspend = ad7606_par_suspend,
> + .resume = ad7606_par_resume,
> +};
> +#define AD7606_PAR_PM_OPS (&ad7606_pm_ops)
> +
> +#else
> +#define AD7606_PAR_PM_OPS NULL
> +#endif /* CONFIG_PM */
> +
Nitpick, Bonus empty line.
> +
> +static struct platform_device_id ad7606_driver_ids[] = {
> + {
> + .name = "ad7606-8",
> + .driver_data = ID_AD7606_8,
> + }, {
> + .name = "ad7606-6",
> + .driver_data = ID_AD7606_6,
> + }, {
> + .name = "ad7606-4",
> + .driver_data = ID_AD7606_4,
> + },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(platform, ad7606_driver_ids);
> +
> +static struct platform_driver ad7606_driver = {
> + .probe = ad7606_par_probe,
> + .remove = __devexit_p(ad7606_par_remove),
> + .id_table = ad7606_driver_ids,
> + .driver = {
> + .name = "ad7606",
> + .owner = THIS_MODULE,
> + .pm = AD7606_PAR_PM_OPS,
> + },
> +};
> +
> +static int __init ad7606_init(void)
> +{
> + return platform_driver_register(&ad7606_driver);
> +}
> +
> +static void __exit ad7606_cleanup(void)
> +{
> + platform_driver_unregister(&ad7606_driver);
> +}
> +
> +module_init(ad7606_init);
> +module_exit(ad7606_cleanup);
> +
> +MODULE_AUTHOR("Michael Hennerich <hennerich@xxxxxxxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:ad7606-par");
> diff --git a/drivers/staging/iio/adc/ad7606_ring.c b/drivers/staging/iio/adc/ad7606_ring.c
> new file mode 100644
> index 0000000..ab06503
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7606_ring.c
> @@ -0,0 +1,261 @@
> +/*
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + *
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/gpio.h>
> +#include <linux/workqueue.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
Do we need this?
> +#include <linux/list.h>
> +
> +#include "../iio.h"
> +#include "../ring_generic.h"
> +#include "../ring_sw.h"
> +#include "../trigger.h"
> +#include "../sysfs.h"
> +
> +#include "ad7606.h"
> +
> +static IIO_SCAN_EL_C(in0, 0, 0, NULL);
> +static IIO_SCAN_EL_C(in1, 1, 0, NULL);
> +static IIO_SCAN_EL_C(in2, 2, 0, NULL);
> +static IIO_SCAN_EL_C(in3, 3, 0, NULL);
> +static IIO_SCAN_EL_C(in4, 4, 0, NULL);
> +static IIO_SCAN_EL_C(in5, 5, 0, NULL);
> +static IIO_SCAN_EL_C(in6, 6, 0, NULL);
> +static IIO_SCAN_EL_C(in7, 7, 0, NULL);
> +
> +static ssize_t ad7606_show_type(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_ring_buffer *ring = dev_get_drvdata(dev);
> + struct iio_dev *indio_dev = ring->indio_dev;
> + struct ad7606_state *st = indio_dev->dev_data;
> +
> + return sprintf(buf, "%c%d/%d\n", st->chip_info->sign,
> + st->chip_info->bits, st->chip_info->bits);
> +}
> +static IIO_DEVICE_ATTR(in_type, S_IRUGO, ad7606_show_type, NULL, 0);
> +
> +static struct attribute *ad7606_scan_el_attrs[] = {
> + &iio_scan_el_in0.dev_attr.attr,
> + &iio_const_attr_in0_index.dev_attr.attr,
> + &iio_scan_el_in1.dev_attr.attr,
> + &iio_const_attr_in1_index.dev_attr.attr,
> + &iio_scan_el_in2.dev_attr.attr,
> + &iio_const_attr_in2_index.dev_attr.attr,
> + &iio_scan_el_in3.dev_attr.attr,
> + &iio_const_attr_in3_index.dev_attr.attr,
> + &iio_scan_el_in4.dev_attr.attr,
> + &iio_const_attr_in4_index.dev_attr.attr,
> + &iio_scan_el_in5.dev_attr.attr,
> + &iio_const_attr_in5_index.dev_attr.attr,
> + &iio_scan_el_in6.dev_attr.attr,
> + &iio_const_attr_in6_index.dev_attr.attr,
> + &iio_scan_el_in7.dev_attr.attr,
> + &iio_const_attr_in7_index.dev_attr.attr,
> + &iio_dev_attr_in_type.dev_attr.attr,
> + NULL,
> +};
> +
> +static mode_t ad7606_scan_el_attr_is_visible(struct kobject *kobj,
> + struct attribute *attr, int n)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct iio_ring_buffer *ring = dev_get_drvdata(dev);
> + struct iio_dev *indio_dev = ring->indio_dev;
> + struct ad7606_state *st = indio_dev->dev_data;
> +
> + mode_t mode = attr->mode;
> +
Ouch. I think this works out as the same, perhaps simpler to read?
if ((st->id == ID_AD7606_6) || (st->id == ID_AD7606_4) &&
attr == &iio_scan_el_in7.dev_attr.attr ||
attr == &iio_const_attr_in7_index.dev_attr.attr ||
attr == &iio_scan_el_in6.dev_attr.attr ||
attr == &iio_const_attr_in6_index.dev_attr.attr)
mode = 0;
if (st->id == ID_AD7606_4 &&
(attr == &iio_scan_el_in4.dev_attr.attr ||
attr == &iio_const_attr_in4_index.dev_attr.attr ||
attr == &iio_scan_el_in5.dev_attr.attr ||
attr == &iio_const_attr_in5_index.dev_attr.attr))
mode = 0;
Or could use st->id != ID_AD7606_8 for first condition.
Actually I'd personally the tests to be on the chip_info->num_channels
as then is more obvious what is going on.
> + if ((attr == &iio_scan_el_in7.dev_attr.attr ||
> + attr == &iio_const_attr_in7_index.dev_attr.attr) &&
> + ((st->id == ID_AD7606_6) || (st->id == ID_AD7606_4)))
> + mode = 0;
> + else if ((attr == &iio_scan_el_in6.dev_attr.attr ||
> + attr == &iio_const_attr_in6_index.dev_attr.attr) &&
> + ((st->id == ID_AD7606_6) || (st->id == ID_AD7606_4)))
> + mode = 0;
> + else if ((attr == &iio_scan_el_in5.dev_attr.attr ||
> + attr == &iio_const_attr_in5_index.dev_attr.attr) &&
> + (st->id == ID_AD7606_4))
> + mode = 0;
> + else if ((attr == &iio_scan_el_in4.dev_attr.attr ||
> + attr == &iio_const_attr_in4_index.dev_attr.attr) &&
> + (st->id == ID_AD7606_4))
> + mode = 0;
> +
> + return mode;
> +}
> +
> +static struct attribute_group ad7606_scan_el_group = {
> + .name = "scan_elements",
> + .attrs = ad7606_scan_el_attrs,
> + .is_visible = ad7606_scan_el_attr_is_visible,
> +};
> +
> +int ad7606_scan_from_ring(struct ad7606_state *st, unsigned ch)
> +{
> + struct iio_ring_buffer *ring = st->indio_dev->ring;
> + int ret;
> + u16 *ring_data;
> +
> + ring_data = kmalloc(ring->access.get_bytes_per_datum(ring), GFP_KERNEL);
> + if (ring_data == NULL) {
> + ret = -ENOMEM;
> + goto error_ret;
> + }
> + ret = ring->access.read_last(ring, (u8 *) ring_data);
> + if (ret)
> + goto error_free_ring_data;
> +
> + ret = ring_data[ch];
> +
> +error_free_ring_data:
> + kfree(ring_data);
> +error_ret:
> + return ret;
> +}
> +
> +/**
> + * ad7606_ring_preenable() setup the parameters of the ring before enabling
> + *
> + * The complex nature of the setting of the nuber of bytes per datum is due
> + * to this driver currently ensuring that the timestamp is stored at an 8
> + * byte boundary.
> + **/
> +static int ad7606_ring_preenable(struct iio_dev *indio_dev)
> +{
> + struct ad7606_state *st = indio_dev->dev_data;
> + struct iio_ring_buffer *ring = indio_dev->ring;
> + size_t d_size;
> +
> + d_size = st->chip_info->num_channels *
> + st->chip_info->bits / 8 + sizeof(s64);
> +
> + if (d_size % sizeof(s64))
> + d_size += sizeof(s64) - (d_size % sizeof(s64));
> +
> + if (ring->access.set_bytes_per_datum)
> + ring->access.set_bytes_per_datum(ring, d_size);
> +
> + st->d_size = d_size;
> +
> + return 0;
> +}
> +
> +/**
> + * ad7606_poll_func_th() th of trigger launched polling to ring buffer
> + *
> + **/
> +static void ad7606_poll_func_th(struct iio_dev *indio_dev, s64 time)
> +{
> + struct ad7606_state *st = indio_dev->dev_data;
> + gpio_set_value(st->pdata->gpio_convst, 1);
> +
> + return;
> +}
> +/**
> + * ad7606_poll_bh_to_ring() bh of trigger launched polling to ring buffer
> + * @work_s: the work struct through which this was scheduled
> + *
> + * Currently there is no option in this driver to disable the saving of
> + * timestamps within the ring.
> + * I think the one copy of this at a time was to avoid problems if the
> + * trigger was set far too high and the reads then locked up the computer.
> + **/
> +static void ad7606_poll_bh_to_ring(struct work_struct *work_s)
> +{
> + struct ad7606_state *st = container_of(work_s, struct ad7606_state,
> + poll_work);
> + struct iio_dev *indio_dev = st->indio_dev;
> + struct iio_sw_ring_buffer *sw_ring = iio_to_sw_ring(indio_dev->ring);
> + struct iio_ring_buffer *ring = indio_dev->ring;
> + s64 time_ns;
> + __u8 *buf;
> +
> + /* Ensure only one copy of this function running at a time */
> + if (atomic_inc_return(&st->protect_ring) > 1)
> + return;
> +
> + buf = kzalloc(st->d_size, GFP_KERNEL);
> + if (buf == NULL)
> + return;
> +
> + if (st->have_frstdata) {
> + st->bops->read_block(st->dev, 1, buf);
> + if (!gpio_get_value(st->pdata->gpio_frstdata)) {
> + /* This should never happen */
With that comment... The question is 'Does it?' If not why is this
check here? Some explanation needed!
> + ad7606_reset(st);
> + goto done;
> + }
> + st->bops->read_block(st->dev,
> + st->chip_info->num_channels - 1, buf + 2);
> + } else {
> + st->bops->read_block(st->dev,
> + st->chip_info->num_channels, buf);
> + }
> +
> + time_ns = iio_get_time_ns();
> + memcpy(buf + st->d_size - sizeof(s64), &time_ns, sizeof(time_ns));
> +
> + ring->access.store_to(&sw_ring->buf, buf, time_ns);
> +done:
> + gpio_set_value(st->pdata->gpio_convst, 0);
> + kfree(buf);
> + atomic_dec(&st->protect_ring);
> +}
> +
> +int ad7606_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> +{
> + struct ad7606_state *st = indio_dev->dev_data;
> + int ret;
> +
> + indio_dev->ring = iio_sw_rb_allocate(indio_dev);
> + if (!indio_dev->ring) {
> + ret = -ENOMEM;
> + goto error_ret;
> + }
> +
> + /* Effectively select the ring buffer implementation */
> + iio_ring_sw_register_funcs(&indio_dev->ring->access);
> + ret = iio_alloc_pollfunc(indio_dev, NULL, &ad7606_poll_func_th);
> + if (ret)
> + goto error_deallocate_sw_rb;
> +
> + /* Ring buffer functions - here trigger setup related */
> +
> + indio_dev->ring->preenable = &ad7606_ring_preenable;
> + indio_dev->ring->postenable = &iio_triggered_ring_postenable;
> + indio_dev->ring->predisable = &iio_triggered_ring_predisable;
> + indio_dev->ring->scan_el_attrs = &ad7606_scan_el_group;
> +
> + INIT_WORK(&st->poll_work, &ad7606_poll_bh_to_ring);
> +
> + /* Flag that polled ring buffering is possible */
> + indio_dev->modes |= INDIO_RING_TRIGGERED;
> + return 0;
> +error_deallocate_sw_rb:
> + iio_sw_rb_free(indio_dev->ring);
> +error_ret:
> + return ret;
> +}
> +
> +void ad7606_ring_cleanup(struct iio_dev *indio_dev)
> +{
> + /* ensure that the trigger has been detached */
This does look suspiciously like a work around for a deficiency in
the core. Is it?
> + if (indio_dev->trig) {
> + iio_put_trigger(indio_dev->trig);
> + iio_trigger_dettach_poll_func(indio_dev->trig,
> + indio_dev->pollfunc);
> + }
> + kfree(indio_dev->pollfunc);
> + iio_sw_rb_free(indio_dev->ring);
> +}
> diff --git a/drivers/staging/iio/adc/ad7606_spi.c b/drivers/staging/iio/adc/ad7606_spi.c
> new file mode 100644
> index 0000000..fb69bba
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7606_spi.c
> @@ -0,0 +1,133 @@
> +/*
> + * AD7606 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/err.h>
> +#include "ad7606.h"
> +
> +#define MAX_SPI_FREQ_HZ 23500000 /* VDRIVE above 4.75 V */
> +
> +static int ad7606_spi_read_block(struct device *dev,
> + int count, void *buf)
> +{
> + struct spi_device *spi = to_spi_device(dev);
> + int i, ret;
> + unsigned short *data = buf;
> +
> + ret = spi_read(spi, (u8 *)buf, count * 2);
> + if (ret < 0) {
> + dev_err(&spi->dev, "SPI read error\n");
> + return ret;
> + }
> +
> + for (i = 0; i < count; i++)
> + data[i] = be16_to_cpu(data[i]);
> +
> + return 0;
> +}
> +
> +static const struct ad7606_bus_ops ad7606_spi_bops = {
> + .read_block = ad7606_spi_read_block,
> +};
> +
> +static int __devinit ad7606_spi_probe(struct spi_device *spi)
> +{
> + struct ad7606_state *st;
> +
I'd as a general rule be cynical and say its the job of the board
file writer to get this right, so could loose this test. Guess
there is no harm in being sure though.
> + /* don't exceed max specified SPI CLK frequency */
> + if (spi->max_speed_hz > MAX_SPI_FREQ_HZ) {
> + dev_err(&spi->dev, "SPI CLK %d Hz too fast\n",
> + spi->max_speed_hz);
> + return -EINVAL;
> + }
> +
> + st = ad7606_probe(&spi->dev, spi->irq, NULL,
> + spi_get_device_id(spi)->driver_data,
> + &ad7606_spi_bops);
> +
> + if (IS_ERR(st))
> + return PTR_ERR(st);
> +
> + spi_set_drvdata(spi, st);
> +
> + return 0;
> +}
> +
> +static int __devexit ad7606_spi_remove(struct spi_device *spi)
> +{
> + struct ad7606_state *st = dev_get_drvdata(&spi->dev);
> +
> + return ad7606_remove(st);
> +}
> +
> +#ifdef CONFIG_PM
These are identical for the two buses. Admittedly they are trivial,
but can we easily share them between them (i.e. move them into
the core driver code?)
> +static int ad7606_spi_suspend(struct device *dev)
> +{
> + struct ad7606_state *st = dev_get_drvdata(dev);
> +
> + ad7606_suspend(st);
> +
> + return 0;
> +}
> +
> +static int ad7606_spi_resume(struct device *dev)
> +{
> + struct ad7606_state *st = dev_get_drvdata(dev);
> +
> + ad7606_resume(st);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ad7606_pm_ops = {
> + .suspend = ad7606_spi_suspend,
> + .resume = ad7606_spi_resume,
> +};
> +#define AD7606_SPI_PM_OPS (&ad7606_pm_ops)
> +
> +#else
> +#define AD7606_SPI_PM_OPS NULL
> +#endif
> +
> +static const struct spi_device_id ad7606_id[] = {
> + {"ad7606-8", ID_AD7606_8},
> + {"ad7606-6", ID_AD7606_6},
> + {"ad7606-4", ID_AD7606_4},
> + {}
> +};
> +
> +static struct spi_driver ad7606_driver = {
> + .driver = {
> + .name = "ad7606",
> + .bus = &spi_bus_type,
> + .owner = THIS_MODULE,
> + .pm = AD7606_SPI_PM_OPS,
> + },
> + .probe = ad7606_spi_probe,
> + .remove = __devexit_p(ad7606_spi_remove),
> + .id_table = ad7606_id,
> +};
> +
> +static int __init ad7606_spi_init(void)
> +{
> + return spi_register_driver(&ad7606_driver);
> +}
> +module_init(ad7606_spi_init);
> +
> +static void __exit ad7606_spi_exit(void)
> +{
> + spi_unregister_driver(&ad7606_driver);
> +}
> +module_exit(ad7606_spi_exit);
> +
> +MODULE_AUTHOR("Michael Hennerich <hennerich@xxxxxxxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:ad7606-spi");
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
