From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ppsw-50.csi.cam.ac.uk ([131.111.8.150]:38562 "EHLO ppsw-50.csi.cam.ac.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756967Ab2FUORC (ORCPT ); Thu, 21 Jun 2012 10:17:02 -0400 Message-ID: <4FE32CDA.7040001@cam.ac.uk> Date: Thu, 21 Jun 2012 15:16:58 +0100 From: Jonathan Cameron MIME-Version: 1.0 To: Lars-Peter Clausen CC: linux-iio@vger.kernel.org Subject: Re: [PATCH] staging:iio:adc: Add AD7265/AD7266 support References: <1340039656-19403-1-git-send-email-lars@metafoo.de> In-Reply-To: <1340039656-19403-1-git-send-email-lars@metafoo.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org On 6/18/2012 6:14 PM, Lars-Peter Clausen wrote: > This patch adds support for the Analog Devices AD7265 and AD7266 > Analog-to-Digital converters. Mostly fine. I'm unconvinced the complexity around the channel array creation is necessary. Might save a little on data but loses in readability! Jonathan > > Signed-off-by: Lars-Peter Clausen > --- > drivers/iio/adc/Kconfig | 10 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ad7266.c | 470 ++++++++++++++++++++++++++++++++++ > include/linux/platform_data/ad7266.h | 54 ++++ > 4 files changed, 535 insertions(+) > create mode 100644 drivers/iio/adc/ad7266.c > create mode 100644 include/linux/platform_data/ad7266.h > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 9a0df81..1af72d9 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -3,6 +3,16 @@ > # > menu "Analog to digital converters" > > +config AD7266 > + tristate "Analog Devices AD7265/AD7266 ADC driver" > + depends on SPI_MASTER > + select IIO_BUFFER > + select IIO_TRIGGER > + select IIO_TRIGGERED_BUFFER > + help > + Say yes here to build support for Analog Devices AD7265 and AD7266 > + ADCs. > + > config AT91_ADC > tristate "Atmel AT91 ADC" > depends on ARCH_AT91 > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 175c8d4..52eec25 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -2,4 +2,5 @@ > # Makefile for IIO ADC drivers > # > > +obj-$(CONFIG_AD7266) += ad7266.o > obj-$(CONFIG_AT91_ADC) += at91_adc.o > diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c > new file mode 100644 > index 0000000..4a9983e > --- /dev/null > +++ b/drivers/iio/adc/ad7266.c > @@ -0,0 +1,470 @@ > +/* > + * AD7266/65 SPI ADC driver > + * > + * Copyright 2012 Analog Devices Inc. > + * > + * Licensed under the GPL-2. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#include > +#include > +#include > + > +#include > + > +struct ad7266_state { > + struct spi_device *spi; > + struct regulator *reg; > + unsigned long vref_uv; > + > + struct spi_transfer single_xfer[3]; > + struct spi_message single_msg; > + > + enum ad7266_range range; > + enum ad7266_mode mode; > + bool fixed_addr; > + struct gpio gpios[3]; > + > + /* > + * DMA (thus cache coherency maintenance) requires the > + * transfer buffers to live in their own cache lines. > + */ Maybe a comment about the use of ALIGN here as well? > + uint8_t data[ALIGN(4, sizeof(s64)) + sizeof(s64)] ____cacheline_aligned; > +}; > + > +static int ad7266_wakeup(struct ad7266_state *st) > +{ > + /* Any read with >= 2 bytes will wake the device */ > + return spi_read(st->spi, st->data, 2); > +} > + > +static int ad7266_powerdown(struct ad7266_state *st) > +{ > + /* Any read with < 2 bytes will wake the device */ > + return spi_read(st->spi, st->data, 1); > +} > + > +static int ad7266_preenable(struct iio_dev *indio_dev) > +{ > + struct ad7266_state *st = iio_priv(indio_dev); > + int ret; > + > + ret = ad7266_wakeup(st); > + if (ret) > + return ret; > + > + ret = iio_sw_buffer_preenable(indio_dev); > + if (ret) > + ad7266_powerdown(st); > + > + return ret; > +} > + > +static int ad7266_postdisable(struct iio_dev *indio_dev) > +{ > + struct ad7266_state *st = iio_priv(indio_dev); > + return ad7266_powerdown(st); > +} > + > +static const struct iio_buffer_setup_ops iio_triggered_buffer_setup_ops = { > + .preenable = &ad7266_preenable, > + .postenable = &iio_triggered_buffer_postenable, > + .predisable = &iio_triggered_buffer_predisable, > + .postdisable = &ad7266_postdisable, > +}; > + > +static irqreturn_t ad7266_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct iio_buffer *buffer = indio_dev->buffer; > + struct ad7266_state *st = iio_priv(indio_dev); > + int ret; > + > + ret = spi_read(st->spi, st->data, 4); > + if (ret == 0) { > + if (indio_dev->scan_timestamp) > + ((s64 *)st->data)[1] = pf->timestamp; > + iio_push_to_buffer(buffer, (u8 *)st->data, pf->timestamp); > + } > + > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > +static void ad7266_select_input(struct ad7266_state *st, unsigned int nr) > +{ > + unsigned int i; > + > + if (st->fixed_addr) > + return; > + > + switch (st->mode) { > + case AD7266_MODE_SINGLE_ENDED: > + nr >>= 1; > + break; > + case AD7266_MODE_PSEUDO_DIFF: > + nr |= 1; > + break; > + case AD7266_MODE_DIFF: > + nr &= ~1; > + break; > + } > + > + for (i = 0; i < 3; ++i) > + gpio_set_value(st->gpios[i].gpio, (bool)(nr & BIT(i))); > +} > + > +int ad7266_update_scan_mode(struct iio_dev *indio_dev, > + const unsigned long *scan_mask) > +{ > + struct ad7266_state *st = iio_priv(indio_dev); > + unsigned int nr = find_first_bit(scan_mask, indio_dev->masklength); > + > + ad7266_select_input(st, nr); > + > + return 0; > +} > + > +static int ad7266_read_single(struct ad7266_state *st, int *val, > + unsigned int address) > +{ > + int ret; > + > + ad7266_select_input(st, address); > + > + ret = spi_sync(st->spi, &st->single_msg); > + *val = be16_to_cpu(st->data[address % 2]); > + > + return ret; > +} > + > +static int ad7266_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, int *val2, long m) > +{ > + struct ad7266_state *st = iio_priv(indio_dev); > + unsigned long scale_uv; > + int ret; > + > + switch (m) { > + case IIO_CHAN_INFO_RAW: > + if (iio_buffer_enabled(indio_dev)) > + return -EBUSY; > + > + ret = ad7266_read_single(st, val, chan->address); > + if (ret) > + return ret; > + > + *val = (*val >> 2) & 0xfff; > + if (chan->scan_type.sign == 's') > + *val = sign_extend32(*val, 11); > + > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + scale_uv = (st->vref_uv * 100); > + if (st->range == AD7266_RANGE_2VREF) > + scale_uv *= 2; > + > + scale_uv >>= chan->scan_type.realbits; > + *val = scale_uv / 100000; > + *val2 = (scale_uv % 100000) * 10; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_CHAN_INFO_OFFSET: > + if (st->range == AD7266_RANGE_2VREF && > + st->mode == AD7266_MODE_SINGLE_ENDED) > + *val = -2048; > + else > + *val = 0; > + return IIO_VAL_INT; > + } > + return -EINVAL; > +} > + > +#define AD7266_CHAN(_chan, _sign) { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = (_chan), \ > + .address = (_chan), \ > + .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT \ > + | IIO_CHAN_INFO_SCALE_SHARED_BIT \ > + | IIO_CHAN_INFO_OFFSET_SHARED_BIT, \ > + .scan_index = (_chan), \ > + .scan_type = IIO_ST((_sign), 12, 16, 2), \ > +} > + > +#define AD7266_DECLARE_SINGLE_ENDED_CHANNELS(_name, _sign) \ > +const struct iio_chan_spec ad7266_channels_##_name[] = { \ > + AD7266_CHAN(0, (_sign)), \ > + AD7266_CHAN(1, (_sign)), \ > + AD7266_CHAN(2, (_sign)), \ > + AD7266_CHAN(3, (_sign)), \ > + AD7266_CHAN(4, (_sign)), \ > + AD7266_CHAN(5, (_sign)), \ > + AD7266_CHAN(6, (_sign)), \ > + AD7266_CHAN(7, (_sign)), \ > + AD7266_CHAN(8, (_sign)), \ > + AD7266_CHAN(9, (_sign)), \ > + AD7266_CHAN(10, (_sign)), \ > + AD7266_CHAN(11, (_sign)), \ > +}; > + > +static AD7266_DECLARE_SINGLE_ENDED_CHANNELS(u, 'u'); > +static AD7266_DECLARE_SINGLE_ENDED_CHANNELS(s, 's'); > + > +#define AD7266_CHAN_DIFF(_chan) { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = (_chan) * 2, \ > + .channel2 = (_chan) * 2 + 1, \ > + .address = (_chan), \ > + .info_mask = IIO_CHAN_INFO_SCALE_SHARED_BIT, \ > + .scan_index = (_chan), \ > + .scan_type = IIO_ST('s', 12, 16, 2), \ > + .differential = 1, \ > +} > + > +static const struct iio_chan_spec ad7266_channels_diff[] = { > + AD7266_CHAN_DIFF(0), > + AD7266_CHAN_DIFF(1), > + AD7266_CHAN_DIFF(2), > + AD7266_CHAN_DIFF(3), > + AD7266_CHAN_DIFF(4), > + AD7266_CHAN_DIFF(5), > +}; > + > +static const struct iio_info ad7266_info = { > + .read_raw = &ad7266_read_raw, > + .driver_module = THIS_MODULE, > +}; > + > +static unsigned long ad7266_available_scan_masks[] = { > + 0x003, > + 0x00c, > + 0x030, > + 0x0c0, > + 0x300, > + 0xc00, > + 0x000, > +}; > + > +static unsigned long ad7266_available_scan_masks_diff[] = { > + 0x003, > + 0x00c, > + 0x030, > + 0x000, > +}; > + > +static unsigned long ad7266_available_scan_masks_fixed[] = { > + 0x003, > + 0x000, > +}; > + > +static const char * const ad7266_gpio_labels[] = { > + "AD0", "AD1", "AD2", > +}; > + > +static int __devinit ad7266_alloc_channels(struct iio_dev *indio_dev) > +{ > + struct ad7266_state *st = iio_priv(indio_dev); > + const struct iio_chan_spec *channel_template; > + struct iio_chan_spec *channels; > + unsigned long *scan_masks; > + unsigned int num_channels; > + I'm unclear on why all this complexity is necessary. We aren't dealing with huge numbers of channels here. Why can't we just have a couple of const static arrays and pick between them? This is just nasty to read for a fairly small gain. > + if (st->mode == AD7266_MODE_SINGLE_ENDED) { > + if (st->range == AD7266_RANGE_VREF) { > + channel_template = ad7266_channels_u; > + num_channels = ARRAY_SIZE(ad7266_channels_u); > + } else { > + channel_template = ad7266_channels_s; > + num_channels = ARRAY_SIZE(ad7266_channels_s); > + } > + scan_masks = ad7266_available_scan_masks; > + } else { > + channel_template = ad7266_channels_diff; > + num_channels = ARRAY_SIZE(ad7266_channels_diff); > + scan_masks = ad7266_available_scan_masks_diff; > + } > + > + if (!st->fixed_addr) { > + indio_dev->available_scan_masks = scan_masks; > + indio_dev->masklength = indio_dev->num_channels; > + } else { check patch is moaning at me about this next line being two long. > + indio_dev->available_scan_masks = ad7266_available_scan_masks_fixed; > + indio_dev->masklength = 2; > + num_channels = 2; > + } > + > + channels = kcalloc(num_channels + 1, sizeof(*channels), > + GFP_KERNEL); > + if (!channels) > + return -ENOMEM; > + > + memcpy(channels, channel_template, num_channels * sizeof(*channels)); > + channels[num_channels] = > + (struct iio_chan_spec)IIO_CHAN_SOFT_TIMESTAMP(num_channels); > + > + indio_dev->num_channels = num_channels + 1; > + indio_dev->channels = channels; > + > + return 0; > +} > + > +static int __devinit ad7266_probe(struct spi_device *spi) > +{ > + struct ad7266_platform_data *pdata = spi->dev.platform_data; > + struct iio_dev *indio_dev; > + struct ad7266_state *st; > + unsigned int i; > + int ret; > + > + indio_dev = iio_device_alloc(sizeof(*st)); > + if (indio_dev == NULL) > + return -ENOMEM; > + > + st = iio_priv(indio_dev); > + > + st->reg = regulator_get(&spi->dev, "vref"); > + if (!IS_ERR_OR_NULL(st->reg)) { > + ret = regulator_enable(st->reg); > + if (ret) > + goto error_put_reg; > + > + st->vref_uv = regulator_get_voltage(st->reg); > + } else { > + /* Use internal reference */ > + st->vref_uv = 2500000; > + } > + > + if (pdata) { > + st->fixed_addr = pdata->fixed_addr; > + st->mode = pdata->mode; > + st->range = pdata->range; > + > + if (!st->fixed_addr) { > + for (i = 0; i < ARRAY_SIZE(st->gpios); ++i) { > + st->gpios[i].gpio = pdata->addr_gpios[i]; > + st->gpios[i].flags = GPIOF_OUT_INIT_LOW; > + st->gpios[i].label = ad7266_gpio_labels[i]; > + } > + ret = gpio_request_array(st->gpios, > + ARRAY_SIZE(st->gpios)); > + if (ret) > + goto error_disable_reg; > + } > + } else { > + st->fixed_addr = true; > + st->range = AD7266_RANGE_VREF; > + st->mode = AD7266_MODE_DIFF; > + } > + > + spi_set_drvdata(spi, indio_dev); > + st->spi = spi; > + > + indio_dev->dev.parent = &spi->dev; > + indio_dev->name = spi_get_device_id(spi)->name; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &ad7266_info; > + > + ret = ad7266_alloc_channels(indio_dev); > + if (ret) > + goto error_free_gpios; > + > + /* wakeup */ > + st->single_xfer[0].rx_buf = &st->data; > + st->single_xfer[0].len = 2; > + st->single_xfer[0].cs_change = 1; > + /* conversion */ > + st->single_xfer[1].rx_buf = &st->data; > + st->single_xfer[1].len = 4; > + st->single_xfer[1].cs_change = 1; > + /* powerdown */ > + st->single_xfer[2].tx_buf = &st->data; > + st->single_xfer[2].len = 1; > + > + spi_message_init(&st->single_msg); > + spi_message_add_tail(&st->single_xfer[0], &st->single_msg); > + spi_message_add_tail(&st->single_xfer[1], &st->single_msg); > + spi_message_add_tail(&st->single_xfer[2], &st->single_msg); > + > + ret = iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time, > + &ad7266_trigger_handler, &iio_triggered_buffer_setup_ops); > + if (ret) > + goto error_free_channels; > + > + ret = iio_device_register(indio_dev); > + if (ret) > + goto error_buffer_cleanup; > + > + return 0; > + > +error_buffer_cleanup: > + iio_triggered_buffer_cleanup(indio_dev); > +error_free_channels: > + kfree(indio_dev->channels); > +error_free_gpios: > + gpio_free_array(st->gpios, ARRAY_SIZE(st->gpios)); > +error_disable_reg: > + if (!IS_ERR_OR_NULL(st->reg)) > + regulator_disable(st->reg); > +error_put_reg: > + if (!IS_ERR_OR_NULL(st->reg)) > + regulator_put(st->reg); > + > + iio_device_free(indio_dev); > + > + return ret; > +} > + > +static int __devexit ad7266_remove(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct ad7266_state *st = iio_priv(indio_dev); > + > + iio_device_unregister(indio_dev); > + iio_triggered_buffer_cleanup(indio_dev); > + kfree(indio_dev->channels); > + gpio_free_array(st->gpios, ARRAY_SIZE(st->gpios)); > + if (!IS_ERR_OR_NULL(st->reg)) { > + regulator_disable(st->reg); > + regulator_put(st->reg); > + } > + iio_device_free(indio_dev); > + > + return 0; > +} > + > +static const struct spi_device_id ad7266_id[] = { > + {"ad7265", 0}, > + {"ad7266", 0}, > + { } > +}; > +MODULE_DEVICE_TABLE(spi, ad7266_id); > + > +static struct spi_driver ad7266_driver = { > + .driver = { > + .name = "ad7266", > + .owner = THIS_MODULE, > + }, > + .probe = ad7266_probe, > + .remove = __devexit_p(ad7266_remove), > + .id_table = ad7266_id, > +}; > +module_spi_driver(ad7266_driver); > + > +MODULE_AUTHOR("Lars-Peter Clausen "); > +MODULE_DESCRIPTION("Analog Devices AD7266/65 ADC"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/platform_data/ad7266.h b/include/linux/platform_data/ad7266.h > new file mode 100644 > index 0000000..eabfdcb > --- /dev/null > +++ b/include/linux/platform_data/ad7266.h > @@ -0,0 +1,54 @@ > +/* > + * AD7266/65 SPI ADC driver > + * > + * Copyright 2012 Analog Devices Inc. > + * > + * Licensed under the GPL-2. > + */ > + > +#ifndef __IIO_ADC_AD7266_H__ > +#define __IIO_ADC_AD7266_H__ > + > +/** > + * enum ad7266_range - AD7266 reference voltage range > + * @AD7266_RANGE_VREF: Device is configured for input range 0V - VREF > + * (RANGE pin set to low) > + * @AD7266_RANGE_2VREF: Device is configured for input range 0V - 2VREF > + * (RANGE pin set to high) > + */ > +enum ad7266_range { > + AD7266_RANGE_VREF, > + AD7266_RANGE_2VREF, > +}; > + > +/** > + * enum ad7266_mode - AD7266 sample mode > + * @AD7266_MODE_DIFF: Device is configured for full differential mode > + * (SGL/DIFF pin set to low, AD0 pin set to low) > + * @AD7266_MODE_PSEUDO_DIFF: Device is configured for pseudo differential mode > + * (SGL/DIFF pin set to low, AD0 pin set to high) > + * @AD7266_MODE_SINGLE_ENDED: Device is configured for single-ended mode > + * (SGL/DIFF pin set to high) > + */ > +enum ad7266_mode { > + AD7266_MODE_DIFF, > + AD7266_MODE_PSEUDO_DIFF, > + AD7266_MODE_SINGLE_ENDED, > +}; > + > +/** > + * struct ad7266_platform_data - Platform data for the AD7266 driver > + * @range: Reference voltage range the device is configured for > + * @mode: Sample mode the device is configured for > + * @fixed_addr: Whether the address pins are hard-wired > + * @addr_gpios: GPIOs used for controlling the address pins, only used if > + * fixed_addr is set to false. > + */ > +struct ad7266_platform_data { > + enum ad7266_range range; > + enum ad7266_mode mode; > + bool fixed_addr; > + unsigned int addr_gpios[3]; > +}; > + > +#endif >