Thanks a lot for your review and good comments. I have updated
adis16209 and adis16240(which has been tested too) according to your
comment. I am 100% sure we will pull new iio features to our tree. But
i am busy on testing adis16220/260/350, so if possible, i still need
your help to merge them into 2.6.35 just like adis16300/400 for the
moment.
On Thu, May 6, 2010 at 4:13 AM, Jonathan Cameron <jic23@xxxxxxxxx> wrote:
> On 05/05/10 16:23, Jonathan Cameron wrote:
>> From: Barry Song <Barry.Song@xxxxxxxxxx>
> Hi All,
>
> As stated with the original post, here is some coments from me on this
> driver as it currently stands.
>
> One or two are related to changes to the core that are clearly needed
> (like the ability to have read only scan_element enable attributes for
> devices that are always on). I'll get those done when I have a few
> mins.
>
> Barry, I'm happy to do all the other changes mentioned if you can have
> a quick look at check I'm not actually going to break anything. Or if
> you would prefer, we can leave these until after the iio changes hit
> the analog tree.
>
> Jonathan
>>
>> This driver was lifted from the analog devices blackfin tree by
>> Jonathan Cameron. There are currently some over 80 character check patch
>> issues left to be fixed that I know of.
>>
>> I'd like to move to a more standard review type process for iio drivers hence
>> I've posted this one to linux-iio prior to sending it to Greg KH.
>> So if anyone has a chance to take a look (and Barry can you sign off if
>> you are happy with the changes I've made).
>>
>> drivers/staging/iio/accel/Kconfig | 10 +
>> drivers/staging/iio/accel/Makefile | 4 +
>> drivers/staging/iio/accel/adis16209.h | 171 +++++++
>> drivers/staging/iio/accel/adis16209_core.c | 654 +++++++++++++++++++++++++
>> drivers/staging/iio/accel/adis16209_ring.c | 219 +++++++++
>> drivers/staging/iio/accel/adis16209_trigger.c | 124 +++++
>> 6 files changed, 1182 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/staging/iio/accel/Kconfig b/drivers/staging/iio/accel/Kconfig
>> index 3d3c333..8d7a780 100644
>> --- a/drivers/staging/iio/accel/Kconfig
>> +++ b/drivers/staging/iio/accel/Kconfig
>> @@ -3,6 +3,16 @@
>> #
>> comment "Accelerometers"
>>
>> +config ADIS16209
>> + tristate "Analog Devices ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer"
>> + depends on SPI
>> + select IIO_TRIGGER
>> + select IIO_SW_RING
> Oops, this one is my fault. The makefile makes it clear that this driver can be built
> without ring support. This select should be dependent on CONFIG_IIO_RING_BUFFER.
>> + help
>> + Say yes here to build support for Analog Devices adis16209 dual-axis digital inclinometer
>> + and accelerometer.
>> +
>> +
>> config KXSD9
>> tristate "Kionix KXSD9 Accelerometer Driver"
>> depends on SPI
>> diff --git a/drivers/staging/iio/accel/Makefile b/drivers/staging/iio/accel/Makefile
>> index d5335f9..f8f2124 100644
>> --- a/drivers/staging/iio/accel/Makefile
>> +++ b/drivers/staging/iio/accel/Makefile
>> @@ -1,6 +1,10 @@
>> #
>> # Makefile for industrial I/O accelerometer drivers
>> #
>> +adis16209-y := adis16209_core.o
>> +adis16209-$(CONFIG_IIO_RING_BUFFER) += adis16209_ring.o adis16209_trigger.o
>> +obj-$(CONFIG_ADIS16209) += adis16209.o
>> +
>> obj-$(CONFIG_KXSD9) += kxsd9.o
>>
>> lis3l02dq-y := lis3l02dq_core.o
>> diff --git a/drivers/staging/iio/accel/adis16209.h b/drivers/staging/iio/accel/adis16209.h
>> new file mode 100644
>> index 0000000..1e59782
>> --- /dev/null
>> +++ b/drivers/staging/iio/accel/adis16209.h
>> @@ -0,0 +1,171 @@
>> +#ifndef SPI_ADIS16209_H_
>> +#define SPI_ADIS16209_H_
>> +
>> +#define ADIS16209_STARTUP_DELAY 220 /* ms */
>> +
>> +#define ADIS16209_READ_REG(a) a
>> +#define ADIS16209_WRITE_REG(a) ((a) | 0x80)
>> +
> There are quite a few overlong lines in here. Some of the comments are rather
> on the obvious side, so some reorganization and removal of unecessary comments
> should get this sorted out.
>> +#define ADIS16209_FLASH_CNT 0x00 /* Flash memory write count */
>> +#define ADIS16209_SUPPLY_OUT 0x02 /* Output, power supply */
>> +#define ADIS16209_XACCL_OUT 0x04 /* Output, x-axis accelerometer */
>> +#define ADIS16209_YACCL_OUT 0x06 /* Output, y-axis accelerometer */
>> +#define ADIS16209_AUX_ADC 0x08 /* Output, auxiliary ADC input */
>> +#define ADIS16209_TEMP_OUT 0x0A /* Output, temperature */
>> +#define ADIS16209_XINCL_OUT 0x0C /* Output, x-axis inclination */
>> +#define ADIS16209_YINCL_OUT 0x0E /* Output, y-axis inclination */
>> +#define ADIS16209_ROT_OUT 0x10 /* Output, +/-180 vertical rotational position */
>> +#define ADIS16209_XACCL_NULL 0x12 /* Calibration, x-axis acceleration offset null */
>> +#define ADIS16209_YACCL_NULL 0x14 /* Calibration, y-axis acceleration offset null */
>> +#define ADIS16209_XINCL_NULL 0x16 /* Calibration, x-axis inclination offset null */
>> +#define ADIS16209_YINCL_NULL 0x18 /* Calibration, y-axis inclination offset null */
>> +#define ADIS16209_ROT_NULL 0x1A /* Calibration, vertical rotation offset null */
>> +#define ADIS16209_ALM_MAG1 0x20 /* Alarm 1 amplitude threshold */
>> +#define ADIS16209_ALM_MAG2 0x22 /* Alarm 2 amplitude threshold */
>> +#define ADIS16209_ALM_SMPL1 0x24 /* Alarm 1, sample period */
>> +#define ADIS16209_ALM_SMPL2 0x26 /* Alarm 2, sample period */
>> +#define ADIS16209_ALM_CTRL 0x28 /* Alarm control */
>> +#define ADIS16209_AUX_DAC 0x30 /* Auxiliary DAC data */
>> +#define ADIS16209_GPIO_CTRL 0x32 /* General-purpose digital input/output control */
>> +#define ADIS16209_MSC_CTRL 0x34 /* Miscellaneous control */
>> +#define ADIS16209_SMPL_PRD 0x36 /* Internal sample period (rate) control */
>> +#define ADIS16209_AVG_CNT 0x38 /* Operation, filter configuration */
>> +#define ADIS16209_SLP_CNT 0x3A /* Operation, sleep mode control */
>> +#define ADIS16209_DIAG_STAT 0x3C /* Diagnostics, system status register */
>> +#define ADIS16209_GLOB_CMD 0x3E /* Operation, system command register */
>> +
>> +#define ADIS16209_OUTPUTS 8
>> +
>> +/* MSC_CTRL */
>> +#define ADIS16209_MSC_CTRL_PWRUP_SELF_TEST (1 << 10) /* Self-test at power-on: 1 = disabled, 0 = enabled */
>> +#define ADIS16209_MSC_CTRL_SELF_TEST_EN (1 << 8) /* Self-test enable */
>> +#define ADIS16209_MSC_CTRL_DATA_RDY_EN (1 << 2) /* Data-ready enable: 1 = enabled, 0 = disabled */
>> +#define ADIS16209_MSC_CTRL_ACTIVE_HIGH (1 << 1) /* Data-ready polarity: 1 = active high, 0 = active low */
>> +#define ADIS16209_MSC_CTRL_DATA_RDY_DIO2 (1 << 0) /* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
>> +
>> +/* DIAG_STAT */
>> +#define ADIS16209_DIAG_STAT_ALARM2 (1<<9) /* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
>> +#define ADIS16209_DIAG_STAT_ALARM1 (1<<8) /* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
>> +#define ADIS16209_DIAG_STAT_SELFTEST_FAIL (1<<5) /* Self-test diagnostic error flag: 1 = error condition,
>> + 0 = normal operation */
>> +#define ADIS16209_DIAG_STAT_SPI_FAIL (1<<3) /* SPI communications failure */
>> +#define ADIS16209_DIAG_STAT_FLASH_UPT (1<<2) /* Flash update failure */
>> +#define ADIS16209_DIAG_STAT_POWER_HIGH (1<<1) /* Power supply above 3.625 V */
>> +#define ADIS16209_DIAG_STAT_POWER_LOW (1<<0) /* Power supply below 3.15 V */
>> +
>> +/* GLOB_CMD */
>> +#define ADIS16209_GLOB_CMD_SW_RESET (1<<7)
>> +#define ADIS16209_GLOB_CMD_CLEAR_STAT (1<<4)
>> +#define ADIS16209_GLOB_CMD_FACTORY_CAL (1<<1)
>> +
>> +#define ADIS16209_MAX_TX 24
>> +#define ADIS16209_MAX_RX 24
>> +
>> +#define ADIS16209_SPI_BURST (u32)(1000 * 1000)
>> +#define ADIS16209_SPI_FAST (u32)(2000 * 1000)
>> +
>> +/**
>> + * struct adis16209_state - device instance specific data
>> + * @us: actual spi_device
>> + * @work_trigger_to_ring: bh for triggered event handling
>> + * @work_cont_thresh: CLEAN
>> + * @inter: used to check if new interrupt has been triggered
>> + * @last_timestamp: passing timestamp from th to bh of interrupt handler
>> + * @indio_dev: industrial I/O device structure
>> + * @trig: data ready trigger registered with iio
>> + * @tx: transmit buffer
>> + * @rx: recieve buffer
>> + * @buf_lock: mutex to protect tx and rx
>> + **/
>> +struct adis16209_state {
>> + struct spi_device *us;
>> + struct work_struct work_trigger_to_ring;
>> + struct iio_work_cont work_cont_thresh;
>> + s64 last_timestamp;
>> + struct iio_dev *indio_dev;
>> + struct iio_trigger *trig;
>> + u8 *tx;
>> + u8 *rx;
>> + struct mutex buf_lock;
>> +};
>> +
>> +int adis16209_spi_write_reg_8(struct device *dev,
>> + u8 reg_address,
>> + u8 val);
>> +
>> +int adis16209_spi_read_burst(struct device *dev, u8 *rx);
>> +
>> +int adis16209_spi_read_sequence(struct device *dev,
>> + u8 *tx, u8 *rx, int num);
>> +
>> +int adis16209_set_irq(struct device *dev, bool enable);
>> +
>> +int adis16209_reset(struct device *dev);
>> +
>> +int adis16209_stop_device(struct device *dev);
>> +
>> +int adis16209_check_status(struct device *dev);
>> +
>> +#ifdef CONFIG_IIO_RING_BUFFER
>> +enum adis16209_scan {
>> + ADIS16209_SCAN_SUPPLY,
>> + ADIS16209_SCAN_ACC_X,
>> + ADIS16209_SCAN_ACC_Y,
>> + ADIS16209_SCAN_AUX_ADC,
>> + ADIS16209_SCAN_TEMP,
>> + ADIS16209_SCAN_INCLI_X,
>> + ADIS16209_SCAN_INCLI_Y,
>> + ADIS16209_SCAN_ROT,
>> +};
>> +
>> +void adis16209_remove_trigger(struct iio_dev *indio_dev);
>> +int adis16209_probe_trigger(struct iio_dev *indio_dev);
>> +
>> +ssize_t adis16209_read_data_from_ring(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf);
>> +
>> +int adis16209_configure_ring(struct iio_dev *indio_dev);
>> +void adis16209_unconfigure_ring(struct iio_dev *indio_dev);
>> +
>> +int adis16209_initialize_ring(struct iio_ring_buffer *ring);
>> +void adis16209_uninitialize_ring(struct iio_ring_buffer *ring);
>> +#else /* CONFIG_IIO_RING_BUFFER */
>> +
>> +static inline void adis16209_remove_trigger(struct iio_dev *indio_dev)
>> +{
>> +}
>> +
>> +static inline int adis16209_probe_trigger(struct iio_dev *indio_dev)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline ssize_t
>> +adis16209_read_data_from_ring(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + return 0;
>> +}
>> +
>> +static int adis16209_configure_ring(struct iio_dev *indio_dev)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline void adis16209_unconfigure_ring(struct iio_dev *indio_dev)
>> +{
>> +}
>> +
>> +static inline int adis16209_initialize_ring(struct iio_ring_buffer *ring)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline void adis16209_uninitialize_ring(struct iio_ring_buffer *ring)
>> +{
>> +}
>> +
>> +#endif /* CONFIG_IIO_RING_BUFFER */
>> +#endif /* SPI_ADIS16209_H_ */
>> diff --git a/drivers/staging/iio/accel/adis16209_core.c b/drivers/staging/iio/accel/adis16209_core.c
>> new file mode 100644
>> index 0000000..fb0e66d
>> --- /dev/null
>> +++ b/drivers/staging/iio/accel/adis16209_core.c
>> @@ -0,0 +1,654 @@
>> +/*
>> + * ADIS16209 Programmable Digital Vibration Sensor driver
>> + *
>> + * Copyright 2010 Analog Devices Inc.
>> + *
>> + * Licensed under the GPL-2 or later.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/gpio.h>
>> +#include <linux/delay.h>
>> +#include <linux/mutex.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/sysfs.h>
>> +#include <linux/list.h>
>> +
>> +#include "../iio.h"
>> +#include "../sysfs.h"
>> +#include "accel.h"
>> +#include "../gyro/gyro.h"
>> +#include "../adc/adc.h"
>> +
>> +#include "adis16209.h"
>> +
>> +#define DRIVER_NAME "adis16209"
>> +
>> +/**
>> + * adis16209_spi_write_reg_8() - write single byte to a register
>> + * @dev: device associated with child of actual device (iio_dev or iio_trig)
>> + * @reg_address: the address of the register to be written
>> + * @val: the value to write
>> + **/
>> +int adis16209_spi_write_reg_8(struct device *dev,
>> + u8 reg_address,
>> + u8 val)
>> +{
>> + int ret;
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + struct adis16209_state *st = iio_dev_get_devdata(indio_dev);
>> +
>> + mutex_lock(&st->buf_lock);
>> + st->tx[0] = ADIS16209_WRITE_REG(reg_address);
>> + st->tx[1] = val;
>> +
>> + ret = spi_write(st->us, st->tx, 2);
>> + mutex_unlock(&st->buf_lock);
>> +
>> + return ret;
>> +}
adis16209_spi_write_reg_8 has been "static".
>> +
>> +/**
>> + * adis16209_spi_write_reg_16() - write 2 bytes to a pair of registers
>> + * @dev: device associated with child of actual device (iio_dev or iio_trig)
>> + * @reg_address: the address of the lower of the two registers. Second register
>> + * is assumed to have address one greater.
>> + * @val: value to be written
>> + **/
>> +static int adis16209_spi_write_reg_16(struct device *dev,
>> + u8 lower_reg_address,
>> + u16 value)
>> +{
>> + int ret;
>> + struct spi_message msg;
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + struct adis16209_state *st = iio_dev_get_devdata(indio_dev);
>> + struct spi_transfer xfers[] = {
>> + {
>> + .tx_buf = st->tx,
>> + .bits_per_word = 8,
>> + .len = 2,
>> + .cs_change = 1,
>> + }, {
>> + .tx_buf = st->tx + 2,
>> + .bits_per_word = 8,
>> + .len = 2,
>> + .cs_change = 1,
>> + },
>> + };
>> +
>> + mutex_lock(&st->buf_lock);
>> + st->tx[0] = ADIS16209_WRITE_REG(lower_reg_address);
>> + st->tx[1] = value & 0xFF;
>> + st->tx[2] = ADIS16209_WRITE_REG(lower_reg_address + 1);
>> + st->tx[3] = (value >> 8) & 0xFF;
>> +
>> + spi_message_init(&msg);
>> + spi_message_add_tail(&xfers[0], &msg);
>> + spi_message_add_tail(&xfers[1], &msg);
>> + ret = spi_sync(st->us, &msg);
>> + mutex_unlock(&st->buf_lock);
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * adis16209_spi_read_reg_16() - read 2 bytes from a 16-bit register
>> + * @dev: device associated with child of actual device (iio_dev or iio_trig)
>> + * @reg_address: the address of the lower of the two registers. Second register
>> + * is assumed to have address one greater.
>> + * @val: somewhere to pass back the value read
>> + **/
>> +static int adis16209_spi_read_reg_16(struct device *dev,
>> + u8 lower_reg_address,
>> + u16 *val)
>> +{
>> + struct spi_message msg;
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + struct adis16209_state *st = iio_dev_get_devdata(indio_dev);
>> + int ret;
>> + struct spi_transfer xfers[] = {
>> + {
>> + .tx_buf = st->tx,
>> + .bits_per_word = 8,
>> + .len = 2,
>> + .cs_change = 1,
>> + .delay_usecs = 20,
>> + }, {
>> + .rx_buf = st->rx,
>> + .bits_per_word = 8,
>> + .len = 2,
>> + .cs_change = 1,
>> + .delay_usecs = 20,
>> + },
>> + };
>> +
>> + mutex_lock(&st->buf_lock);
>> + st->tx[0] = ADIS16209_READ_REG(lower_reg_address);
>> + st->tx[1] = 0;
>> +
>> + spi_message_init(&msg);
>> + spi_message_add_tail(&xfers[0], &msg);
>> + spi_message_add_tail(&xfers[1], &msg);
>> + ret = spi_sync(st->us, &msg);
>> + if (ret) {
>> + dev_err(&st->us->dev, "problem when reading 16 bit register 0x%02X",
>> + lower_reg_address);
>> + goto error_ret;
>> + }
>> + *val = (st->rx[0] << 8) | st->rx[1];
>> +
>> +error_ret:
>> + mutex_unlock(&st->buf_lock);
>> + return ret;
>> +}
>> +
>
> I do wonder if this function shouldn't really be in the adis16209_ring.c
> as it is only used there and hence is just bloat if the buffering isn't enabled.
> The disadvantage is it means one single read function is in a different place
> from all the others.
it has been movied to adis16209_ring.c and given a new name
adis16209_read_ring_data
>> +/**
>> + * adis16209_spi_read_burst() - read all data registers
>> + * @dev: device associated with child of actual device (iio_dev or iio_trig)
>> + * @rx: somewhere to pass back the value read
>> + **/
>> +int adis16209_spi_read_burst(struct device *dev, u8 *rx)
>> +{
>> + struct spi_message msg;
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + struct adis16209_state *st = iio_dev_get_devdata(indio_dev);
>> + struct spi_transfer xfers[ADIS16209_OUTPUTS + 1];
>> + int ret;
>> + int i;
>> +
>> + mutex_lock(&st->buf_lock);
>> +
>> + spi_message_init(&msg);
>> +
>> + memset(xfers, 0, sizeof(xfers));
>> + for (i = 0; i <= ADIS16209_OUTPUTS; i++) {
>> + xfers[i].bits_per_word = 8;
>> + xfers[i].cs_change = 1;
>> + xfers[i].len = 2;
>> + xfers[i].delay_usecs = 20;
>> + xfers[i].tx_buf = st->tx + 2 * i;
>> + st->tx[2 * i] = ADIS16209_READ_REG(ADIS16209_SUPPLY_OUT + 2 * i);
>> + st->tx[2 * i + 1] = 0;
>> + if (i >= 1)
>> + xfers[i].rx_buf = rx + 2 * (i - 1);
>> + spi_message_add_tail(&xfers[i], &msg);
>> + }
>> +
>> + ret = spi_sync(st->us, &msg);
>> + if (ret)
>> + dev_err(&st->us->dev, "problem when burst reading");
>> +
>> + mutex_unlock(&st->buf_lock);
>> +
>> + return ret;
>> +}
>> +
> I only see one size of signed read. Hence why does this nice general function
> exist? Admittedly the compiler probably squashes it, but it does add unecessary
> code. Not to mention it results in the mlock being held longer than strictly
> necessary.
it has been merged into adis16209_read_14bit_signed()
>
>> +static ssize_t adis16209_spi_read_signed(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf,
>> + unsigned bits)
>> +{
>> + int ret;
>> + s16 val = 0;
>> + unsigned shift = 16 - bits;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + ret = adis16209_spi_read_reg_16(dev, this_attr->address, (u16 *)&val);
>> + if (ret)
>> + return ret;
>> +
>> + val = ((s16)(val << shift) >> shift);
>> + return sprintf(buf, "%d\n", val);
>> +}
>> +
>> +static ssize_t adis16209_read_12bit_unsigned(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + int ret;
>> + u16 val = 0;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + ret = adis16209_spi_read_reg_16(dev, this_attr->address, &val);
>> + if (ret)
>> + return ret;
>> +
>> + return sprintf(buf, "%u\n", val & 0x0FFF);
>> +}
>> +
>> +static ssize_t adis16209_read_14bit_unsigned(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + int ret;
>> + u16 val = 0;
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + ret = adis16209_spi_read_reg_16(dev, this_attr->address, &val);
>> + if (ret)
>> + return ret;
>> +
>> + return sprintf(buf, "%u\n", val & 0x3FFF);
>> +}
>> +
>> +static ssize_t adis16209_read_temp(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + ssize_t ret;
>> + s16 val;
>> +
>> + /* Take the iio_dev status lock */
>> + mutex_lock(&indio_dev->mlock);
>> +
>> + ret = adis16209_spi_read_reg_16(dev, ADIS16209_TEMP_OUT, (u16 *)&val);
>> + if (ret)
>> + goto error_ret;
>> +
> I'm not sure on this. The datasheet doesn't list the temp as a 2's comp
> number? If it is please add a comment here to explain that!
datasheet shows "Output at 25°C : 1278 Scale Factor: −0.47°C/LSB. "
Then suppose output is 0, temp will be 625°C, if it is 2047(2^11),
temp will -937°C. I believe the value is only effective in a small
area since the chips only work from −40°C to +85°C. So looking it as
signed /unsigned is not important at all. But i think it will be
better to leave it alone here without (<< 4) >> 4, but use &0xFFF.
>> + val = ((s16)(val << 4) >> 4);
>> + ret = sprintf(buf, "%d\n", val);
>> +
>> +error_ret:
>> + mutex_unlock(&indio_dev->mlock);
>> + return ret;
>> +}
>> +
> As stated above, this might as well be rolled together with
> read_signed as this is the only user.
>
>> +static ssize_t adis16209_read_14bit_signed(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + ssize_t ret;
>> +
>> + /* Take the iio_dev status lock */
>> + mutex_lock(&indio_dev->mlock);
>> + ret = adis16209_spi_read_signed(dev, attr, buf, 14);
>> + mutex_unlock(&indio_dev->mlock);
>> +
>> + return ret;
>> +}
>> +
>> +static ssize_t adis16209_write_16bit(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf,
>> + size_t len)
>> +{
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> + int ret;
>> + long val;
>> +
>> + ret = strict_strtol(buf, 10, &val);
>> + if (ret)
>> + goto error_ret;
>> + ret = adis16209_spi_write_reg_16(dev, this_attr->address, val);
>> +
>> +error_ret:
>> + return ret ? ret : len;
>> +}
>> +
>> +static ssize_t adis16209_write_reset(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> + if (len < 1)
>> + return -1;
>> + switch (buf[0]) {
>> + case '1':
>> + case 'y':
>> + case 'Y':
>> + return adis16209_reset(dev);
>> + }
> Proper error please. -EINVAL seems the most sensible to me.
>
>> + return -1;
>> +}
>> +
> Whilst there is currently only one non probe user of this (and I don't think
> that multiple concurrent calls to that can occur), I'm a little dubious that
> no lock is taken to ensure msc isn't changed by someone else between the
> read and the write. Perhaps a comment to ensure that this is revisted if
> event interupts are added?
>> +int adis16209_set_irq(struct device *dev, bool enable)
>> +{
>> + int ret;
>> + u16 msc;
>> + ret = adis16209_spi_read_reg_16(dev, ADIS16209_MSC_CTRL, &msc);
>> + if (ret)
>> + goto error_ret;
>> +
>> + msc |= ADIS16209_MSC_CTRL_ACTIVE_HIGH;
>> + msc &= ~ADIS16209_MSC_CTRL_DATA_RDY_DIO2;
>> + if (enable)
>> + msc |= ADIS16209_MSC_CTRL_DATA_RDY_EN;
>> + else
>> + msc &= ~ADIS16209_MSC_CTRL_DATA_RDY_EN;
>> +
>> + ret = adis16209_spi_write_reg_16(dev, ADIS16209_MSC_CTRL, msc);
>> + if (ret)
>> + goto error_ret;
>> +
>> +error_ret:
>> + return ret;
>> +}
>> +
>> +int adis16209_reset(struct device *dev)
>> +{
>> + int ret;
>> + ret = adis16209_spi_write_reg_8(dev,
>> + ADIS16209_GLOB_CMD,
>> + ADIS16209_GLOB_CMD_SW_RESET);
>> + if (ret)
>> + dev_err(dev, "problem resetting device");
>> +
>> + return ret;
>> +}
>> +
>> +static int adis16209_self_test(struct device *dev)
>> +{
>> + int ret;
>> + ret = adis16209_spi_write_reg_16(dev,
>> + ADIS16209_MSC_CTRL,
>> + ADIS16209_MSC_CTRL_SELF_TEST_EN);
>> + if (ret) {
>> + dev_err(dev, "problem starting self test");
>> + goto err_ret;
>> + }
>> +
> check status can return an error code under some circumstances.
> It isn't currently handled.
>> + adis16209_check_status(dev);
>> +
>> +err_ret:
>> + return ret;
>> +}
>> +
>> +int adis16209_check_status(struct device *dev)
>> +{
>> + u16 status;
>> + int ret;
>> +
>> + ret = adis16209_spi_read_reg_16(dev, ADIS16209_DIAG_STAT, &status);
>> + if (ret < 0) {
>> + dev_err(dev, "Reading status failed\n");
>> + goto error_ret;
>> + }
>> + ret = status & 0x1F;
>> +
>> + if (status & ADIS16209_DIAG_STAT_SELFTEST_FAIL)
>> + dev_err(dev, "Self test failure\n");
>> + if (status & ADIS16209_DIAG_STAT_SPI_FAIL)
>> + dev_err(dev, "SPI failure\n");
>> + if (status & ADIS16209_DIAG_STAT_FLASH_UPT)
>> + dev_err(dev, "Flash update failed\n");
>> + if (status & ADIS16209_DIAG_STAT_POWER_HIGH)
>> + dev_err(dev, "Power supply above 3.625V\n");
>> + if (status & ADIS16209_DIAG_STAT_POWER_LOW)
>> + dev_err(dev, "Power supply below 3.15V\n");
>> +
>> +error_ret:
>> + return ret;
>> +}
>> +
>> +static int adis16209_initial_setup(struct adis16209_state *st)
>> +{
>> + int ret;
>> + struct device *dev = &st->indio_dev->dev;
>> +
>> + /* Disable IRQ */
>> + ret = adis16209_set_irq(dev, false);
>> + if (ret) {
>> + dev_err(dev, "disable irq failed");
>> + goto err_ret;
>> + }
>> +
>> + /* Do self test */
>> + ret = adis16209_self_test(dev);
>> + if (ret) {
>> + dev_err(dev, "self test failure");
>> + goto err_ret;
>> + }
> The result of the logic here is to clear other errors
> (by reading status). It won't even fail if the self
> test fails, just print an error. Now if the next read
> occurs fast enough (before next sample cycle) any other
> errors will not have been re asserted. (particularly
> important if we have power supply problems!).
Yes. but not sure what we should do after finding an error. It
probably means a hardware error.
I'd like to handle this as enhancement with more information from
hardware team later.
>> +
>> + /* Read status register to check the result */
>> + ret = adis16209_check_status(dev);
>> + if (ret) {
>> + adis16209_reset(dev);
>> + dev_err(dev, "device not playing ball -> reset");
>> + msleep(ADIS16209_STARTUP_DELAY);
>> + ret = adis16209_check_status(dev);
>> + if (ret) {
>> + dev_err(dev, "giving up");
>> + goto err_ret;
>> + }
>> + }
>> +
>> + printk(KERN_INFO DRIVER_NAME ": at CS%d (irq %d)\n",
>> + st->us->chip_select, st->us->irq);
>> +
>> +err_ret:
>> + return ret;
>> +}
>> +
>> +static IIO_DEV_ATTR_IN_NAMED_RAW(supply, adis16209_read_14bit_unsigned,
>> + ADIS16209_SUPPLY_OUT);
>> +static IIO_CONST_ATTR(in_supply_scale, "0.30518 mV");
>> +static IIO_DEV_ATTR_IN_RAW(0, adis16209_read_12bit_unsigned,
>> + ADIS16209_AUX_ADC);
> In the abi doc these should be scalling to volts (and no unit is needed).
>> +static IIO_CONST_ATTR(in0_scale, "0.6105 mV");
>> +
>> +static IIO_DEV_ATTR_ACCEL_X(adis16209_read_14bit_signed,
>> + ADIS16209_XACCL_OUT);
>> +static IIO_DEV_ATTR_ACCEL_Y(adis16209_read_14bit_signed,
>> + ADIS16209_YACCL_OUT);
>> +static IIO_DEV_ATTR_ACCEL_X_OFFSET(S_IWUSR | S_IRUGO,
>> + adis16209_read_14bit_signed,
>> + adis16209_write_16bit,
>> + ADIS16209_XACCL_NULL);
>> +static IIO_DEV_ATTR_ACCEL_Y_OFFSET(S_IWUSR | S_IRUGO,
>> + adis16209_read_14bit_signed,
>> + adis16209_write_16bit,
>> + ADIS16209_YACCL_NULL);
>> +static IIO_CONST_ATTR(accel_scale, "0.24414 mg");
>> +
>
> I've only just noticed (as I lifted it without a close enough read)
> that gyro.h include inclinometer readings. Given they are acceleration
> based we can either move them to accel.h or given they might be established
> in some other way, create an incli.h file to contain them.
>
> These (along with the rest of gyro.h) need updating to the _raw naming.
> Whether that happens before or after this patch depends on what other
> people want to say about this!
>
>> +static IIO_DEV_ATTR_INCLI_X(adis16209_read_14bit_signed,
>> + ADIS16209_XINCL_OUT);
>> +static IIO_DEV_ATTR_INCLI_Y(adis16209_read_14bit_signed,
>> + ADIS16209_YINCL_OUT);
>> +static IIO_DEV_ATTR_INCLI_X_OFFSET(S_IWUSR | S_IRUGO,
>> + adis16209_read_14bit_signed,
>> + adis16209_write_16bit,
>> + ADIS16209_XACCL_NULL);
>> +static IIO_DEV_ATTR_INCLI_Y_OFFSET(S_IWUSR | S_IRUGO,
>> + adis16209_read_14bit_signed,
>> + adis16209_write_16bit,
>> + ADIS16209_YACCL_NULL);
> Again, no units plase.
>> +static IIO_CONST_ATTR(incli_scale, "0.025 D");
>> +
> Hmm.. This one is a bit device specific to my mind. What do
> people think? In a sense it is just a signed version of inclination.
> Perhaps this is something for documentation rather than in the naming
> itself. I'll try and bash out an explanation (the diagrams on the data
> sheet are pretty good for the curious) and add that to the abi spec.
>
>> +static IIO_DEV_ATTR_ROT(adis16209_read_14bit_signed,
>> + ADIS16209_ROT_OUT);
>> +
> I admit to being somewhat confused by the temp part of the
> data sheet. As written here, I would expect the example
> reading of 0x04FE as given on the datasheet (as 25 degrees C)
> to be 25 - 0.47* 0x4FE Kelvin which I make -898.66 degrees C
>
> As 4FE even if assumed to be 2's complement 12 bit is a positive
> number. Can someone with the magic abilities to understand that bit
> of the data sheet explain how it is meant to work!
>
> Whilst we are here, the intent is to have all readings in standard
> units (and hence not supply the units in sysfs - just like hwmon
> does it). However we can clean these out and standardise everything
> in a later patch set (along with documenting the choices).
ok
>> +static IIO_DEV_ATTR_TEMP(adis16209_read_temp);
>> +static IIO_CONST_ATTR(temp_offset, "25 K");
>> +static IIO_CONST_ATTR(temp_scale, "-0.47 K");
>> +
>> +static IIO_DEVICE_ATTR(reset, S_IWUSR, NULL, adis16209_write_reset, 0);
>> +
>> +static IIO_CONST_ATTR(name, "adis16209");
>> +
> If we need to have this defined as empty I've got a bug in the core code.
> I have a couple of drivers without event lines so I think this is definitely
> unecessary. I guess there might be an interaction with the trigger code I
> haven't handled right. Please poke me if there
kernel will panic when register interrupt in iio after deleting this.
There should be some unnecessary couple in core.
>> +static struct attribute *adis16209_event_attributes[] = {
>> + NULL
>> +};
>> +
>> +static struct attribute_group adis16209_event_attribute_group = {
>> + .attrs = adis16209_event_attributes,
>> +};
>> +
>> +static struct attribute *adis16209_attributes[] = {
>> + &iio_dev_attr_in_supply_raw.dev_attr.attr,
>> + &iio_const_attr_in_supply_scale.dev_attr.attr,
>> + &iio_dev_attr_temp.dev_attr.attr,
>> + &iio_const_attr_temp_offset.dev_attr.attr,
>> + &iio_const_attr_temp_scale.dev_attr.attr,
>> + &iio_dev_attr_reset.dev_attr.attr,
>> + &iio_const_attr_name.dev_attr.attr,
>> + &iio_dev_attr_in0_raw.dev_attr.attr,
>> + &iio_const_attr_in0_scale.dev_attr.attr,
>> + &iio_dev_attr_accel_x_raw.dev_attr.attr,
>> + &iio_dev_attr_accel_y_raw.dev_attr.attr,
>> + &iio_dev_attr_accel_x_offset.dev_attr.attr,
>> + &iio_dev_attr_accel_y_offset.dev_attr.attr,
>> + &iio_const_attr_accel_scale.dev_attr.attr,
>> + &iio_dev_attr_incli_x.dev_attr.attr,
>> + &iio_dev_attr_incli_y.dev_attr.attr,
>> + &iio_dev_attr_incli_x_offset.dev_attr.attr,
>> + &iio_dev_attr_incli_y_offset.dev_attr.attr,
>> + &iio_const_attr_incli_scale.dev_attr.attr,
>> + &iio_dev_attr_rot.dev_attr.attr,
>> + NULL
>> +};
>> +
>> +static const struct attribute_group adis16209_attribute_group = {
>> + .attrs = adis16209_attributes,
>> +};
>> +
>> +static int __devinit adis16209_probe(struct spi_device *spi)
>> +{
>> + int ret, regdone = 0;
>> + struct adis16209_state *st = kzalloc(sizeof *st, GFP_KERNEL);
>> + if (!st) {
>> + ret = -ENOMEM;
>> + goto error_ret;
>> + }
>> + /* this is only used for removal purposes */
>> + spi_set_drvdata(spi, st);
>> +
>> + /* Allocate the comms buffers */
>> + st->rx = kzalloc(sizeof(*st->rx)*ADIS16209_MAX_RX, GFP_KERNEL);
>> + if (st->rx == NULL) {
>> + ret = -ENOMEM;
>> + goto error_free_st;
>> + }
>> + st->tx = kzalloc(sizeof(*st->tx)*ADIS16209_MAX_TX, GFP_KERNEL);
>> + if (st->tx == NULL) {
>> + ret = -ENOMEM;
>> + goto error_free_rx;
>> + }
>> + st->us = spi;
>> + mutex_init(&st->buf_lock);
>> + /* setup the industrialio driver allocated elements */
>> + st->indio_dev = iio_allocate_device();
>> + if (st->indio_dev == NULL) {
>> + ret = -ENOMEM;
>> + goto error_free_tx;
>> + }
>> +
>> + st->indio_dev->dev.parent = &spi->dev;
>> + st->indio_dev->num_interrupt_lines = 1;
>> + st->indio_dev->event_attrs = &adis16209_event_attribute_group;
> As per the above comment, I think the two lines above can go away for
> now.
>
>> + st->indio_dev->attrs = &adis16209_attribute_group;
>> + st->indio_dev->dev_data = (void *)(st);
>> + st->indio_dev->driver_module = THIS_MODULE;
>> + st->indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + ret = adis16209_configure_ring(st->indio_dev);
>> + if (ret)
>> + goto error_free_dev;
>> +
>> + ret = iio_device_register(st->indio_dev);
>> + if (ret)
>> + goto error_unreg_ring_funcs;
>> + regdone = 1;
>> +
>> + ret = adis16209_initialize_ring(st->indio_dev->ring);
>> + if (ret) {
>> + printk(KERN_ERR "failed to initialize the ring\n");
>> + goto error_unreg_ring_funcs;
>> + }
>> +
> I'm guessing this might actually have come out of some of my code, but
> then I'm still learning :) Do we actually need the gpio_is_valid call?
> At the end of the day, if the interrupt is specified do we care about
> the gpio bit? If we do, can we have a comment as its not obvious to me!
gpio_is_valid call is not necessary here. request_irq will fail if the
external irq number is invalid.
>
>> + if (spi->irq && gpio_is_valid(irq_to_gpio(spi->irq)) > 0) {
>> + ret = iio_register_interrupt_line(spi->irq,
>> + st->indio_dev,
>> + 0,
>> + IRQF_TRIGGER_RISING,
>> + "adis16209");
>> + if (ret)
>> + goto error_uninitialize_ring;
>> +
>> + ret = adis16209_probe_trigger(st->indio_dev);
>> + if (ret)
>> + goto error_unregister_line;
>> + }
>> +
>> + /* Get the device into a sane initial state */
>> + ret = adis16209_initial_setup(st);
>> + if (ret)
>> + goto error_remove_trigger;
>> + return 0;
>> +
>> +error_remove_trigger:
>> + if (st->indio_dev->modes & INDIO_RING_TRIGGERED)
>> + adis16209_remove_trigger(st->indio_dev);
>> +error_unregister_line:
>> + if (st->indio_dev->modes & INDIO_RING_TRIGGERED)
>> + iio_unregister_interrupt_line(st->indio_dev, 0);
>> +error_uninitialize_ring:
>> + adis16209_uninitialize_ring(st->indio_dev->ring);
>> +error_unreg_ring_funcs:
>> + adis16209_unconfigure_ring(st->indio_dev);
>> +error_free_dev:
>> + if (regdone)
>> + iio_device_unregister(st->indio_dev);
>> + else
>> + iio_free_device(st->indio_dev);
>> +error_free_tx:
>> + kfree(st->tx);
>> +error_free_rx:
>> + kfree(st->rx);
>> +error_free_st:
>> + kfree(st);
>> +error_ret:
>> + return ret;
>> +}
>> +
> Has this been confirmed? I'm guessing not, so perhaps
> I pinched this one from your tree to early!
>> +/* fixme, confirm ordering in this function */
>> +static int adis16209_remove(struct spi_device *spi)
>> +{
>> + struct adis16209_state *st = spi_get_drvdata(spi);
>> + struct iio_dev *indio_dev = st->indio_dev;
>> +
>> + flush_scheduled_work();
>> +
> I think this should be in the if statement below. (to match the probe).
Here i think if statement is useless in probe, there has been an empty
adis16209_remove_trigger() function for non-ring.
iio_unregister_interrupt_line should always be called if spi->irq is
valid:
error_remove_trigger:
- if (st->indio_dev->modes & INDIO_RING_TRIGGERED)
adis16209_remove_trigger(st->indio_dev);
error_unregister_line:
- if (st->indio_dev->modes & INDIO_RING_TRIGGERED)
+ if (spi->irq)
iio_unregister_interrupt_line(st->indio_dev, 0);
>> + adis16209_remove_trigger(indio_dev);
>> + if (spi->irq && gpio_is_valid(irq_to_gpio(spi->irq)) > 0)
>> + iio_unregister_interrupt_line(indio_dev, 0);
>> +
>> + adis16209_uninitialize_ring(indio_dev->ring);
>
> I think the next two are in the wrong order. Probably doesn't matter, but
> it'll read more coherently the other way round (as they reverse what
> goes on in probe).
It should be
iio_device_unregister(indio_dev);
adis16209_unconfigure_ring(indio_dev);
for the moment.
>> + adis16209_unconfigure_ring(indio_dev);
>> + iio_device_unregister(indio_dev);
>> + kfree(st->tx);
>> + kfree(st->rx);
>> + kfree(st);
>> +
>> + return 0;
>> +}
>> +
>> +static struct spi_driver adis16209_driver = {
>> + .driver = {
>> + .name = "adis16209",
>> + .owner = THIS_MODULE,
>> + },
>> + .probe = adis16209_probe,
>> + .remove = __devexit_p(adis16209_remove),
>> +};
>> +
>> +static __init int adis16209_init(void)
>> +{
>> + return spi_register_driver(&adis16209_driver);
>> +}
>> +module_init(adis16209_init);
>> +
>> +static __exit void adis16209_exit(void)
>> +{
>> + spi_unregister_driver(&adis16209_driver);
>> +}
>> +module_exit(adis16209_exit);
>> +
>> +MODULE_AUTHOR("Barry Song <21cnbao@xxxxxxxxx>");
>> +MODULE_DESCRIPTION("Analog Devices ADIS16209 Programmable Digital Vibration Sensor driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/staging/iio/accel/adis16209_ring.c b/drivers/staging/iio/accel/adis16209_ring.c
>> new file mode 100644
>> index 0000000..9e2073f
>> --- /dev/null
>> +++ b/drivers/staging/iio/accel/adis16209_ring.c
>> @@ -0,0 +1,219 @@
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/gpio.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/mutex.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/list.h>
>> +
>> +#include "../iio.h"
>> +#include "../sysfs.h"
>> +#include "../ring_sw.h"
>> +#include "accel.h"
>> +#include "../trigger.h"
>> +#include "adis16209.h"
>> +
>> +/**
>> + * combine_8_to_16() utility function to munge to u8s into u16
>> + **/
>> +static inline u16 combine_8_to_16(u8 lower, u8 upper)
>> +{
>> + u16 _lower = lower;
>> + u16 _upper = upper;
>> + return _lower | (_upper << 8);
>> +}
>> +
> Hmm. before the recent abi change, there was no point in having read only
> scan elements. Now we need that ability. I'll add the functionality to the
> core and update drivers appropriately. As things currently stand I think
> this will change the scan_count, without actually changing what is incomming
> as we never disable channels.
>> +static IIO_SCAN_EL_C(supply, ADIS16209_SCAN_SUPPLY, IIO_UNSIGNED(14),
>> + ADIS16209_SUPPLY_OUT, NULL);
>> +static IIO_SCAN_EL_C(accel_x, ADIS16209_SCAN_ACC_X, IIO_SIGNED(14),
>> + ADIS16209_XACCL_OUT, NULL);
>> +static IIO_SCAN_EL_C(accel_y, ADIS16209_SCAN_ACC_Y, IIO_SIGNED(14),
>> + ADIS16209_YACCL_OUT, NULL);
>> +static IIO_SCAN_EL_C(aux_adc, ADIS16209_SCAN_AUX_ADC, IIO_UNSIGNED(12),
>> + ADIS16209_AUX_ADC, NULL);
>> +static IIO_SCAN_EL_C(temp, ADIS16209_SCAN_TEMP, IIO_SIGNED(12),
>> + ADIS16209_TEMP_OUT, NULL);
>> +static IIO_SCAN_EL_C(incli_x, ADIS16209_SCAN_INCLI_X, IIO_SIGNED(14),
>> + ADIS16209_XINCL_OUT, NULL);
>> +static IIO_SCAN_EL_C(incli_y, ADIS16209_SCAN_INCLI_Y, IIO_SIGNED(14),
>> + ADIS16209_YINCL_OUT, NULL);
>> +static IIO_SCAN_EL_C(rot, ADIS16209_SCAN_ROT, IIO_SIGNED(14),
>> + ADIS16209_ROT_OUT, NULL);
>> +
>> +static IIO_SCAN_EL_TIMESTAMP(8);
>> +
>> +static struct attribute *adis16209_scan_el_attrs[] = {
>> + &iio_scan_el_supply.dev_attr.attr,
>> + &iio_scan_el_accel_x.dev_attr.attr,
>> + &iio_scan_el_accel_y.dev_attr.attr,
>> + &iio_scan_el_aux_adc.dev_attr.attr,
>> + &iio_scan_el_temp.dev_attr.attr,
>> + &iio_scan_el_incli_x.dev_attr.attr,
>> + &iio_scan_el_incli_y.dev_attr.attr,
>> + &iio_scan_el_rot.dev_attr.attr,
>> + &iio_scan_el_timestamp.dev_attr.attr,
>> + NULL,
>> +};
>> +
>> +static struct attribute_group adis16209_scan_el_group = {
>> + .attrs = adis16209_scan_el_attrs,
>> + .name = "scan_elements",
>> +};
>> +
>> +/**
>> + * adis16209_poll_func_th() top half interrupt handler called by trigger
>> + * @private_data: iio_dev
>> + **/
>> +static void adis16209_poll_func_th(struct iio_dev *indio_dev)
>> +{
>> + struct adis16209_state *st = iio_dev_get_devdata(indio_dev);
>> + st->last_timestamp = indio_dev->trig->timestamp;
>> + schedule_work(&st->work_trigger_to_ring);
>> +}
>> +
>> +/* Whilst this makes a lot of calls to iio_sw_ring functions - it is to device
>> + * specific to be rolled into the core.
>> + */
>> +static void adis16209_trigger_bh_to_ring(struct work_struct *work_s)
>> +{
>> + struct adis16209_state *st
>> + = container_of(work_s, struct adis16209_state,
>> + work_trigger_to_ring);
>> +
>> + int i = 0;
>> + s16 *data;
>> + size_t datasize = st->indio_dev
>> + ->ring->access.get_bpd(st->indio_dev->ring);
>> +
>> + data = kmalloc(datasize , GFP_KERNEL);
>> + if (data == NULL) {
>> + dev_err(&st->us->dev, "memory alloc failed in ring bh");
>> + return;
>> + }
>> +
> As things currently stand (I think all scan elements are read only) this could
> be greatly simplified. (afterall scan count etc are fixed). However this is
> good initial work for moving to scan element selection should anyone want to
> in the future (this certainly is not a requirement until someone actually cares!).
>> + if (st->indio_dev->scan_count)
>> + if (adis16209_spi_read_burst(&st->indio_dev->dev, st->rx) >= 0)
>> + for (; i < st->indio_dev->scan_count; i++) {
>> + data[i] = combine_8_to_16(st->rx[i*2+1],
>> + st->rx[i*2]);
>> + }
>> +
>> + /* Guaranteed to be aligned with 8 byte boundary */
>> + if (st->indio_dev->scan_timestamp)
>> + *((s64 *)(data + ((i + 3)/4)*4)) = st->last_timestamp;
>> +
>> + st->indio_dev->ring->access.store_to(st->indio_dev->ring,
>> + (u8 *)data,
>> + st->last_timestamp);
>> +
>> + iio_trigger_notify_done(st->indio_dev->trig);
>> + kfree(data);
>> +
>> + return;
>> +}
> Hehe. That looks suspiciously like something I would have writen a long time
> back. Oh. It's still in the lis3l02dq as well. For reference I decided that no,
> aligned data is much simpler. If anyone prefers unalighted they can figure out
> how to do it.
>
>> +/* in these circumstances is it better to go with unaligned packing and
>> + * deal with the cost?*/
>> +static int adis16209_data_rdy_ring_preenable(struct iio_dev *indio_dev)
>> +{
>> + size_t size;
>> + dev_dbg(&indio_dev->dev, "%s\n", __func__);
>> + /* Check if there are any scan elements enabled, if not fail*/
>> + if (!(indio_dev->scan_count || indio_dev->scan_timestamp))
>> + return -EINVAL;
>> +
>> + if (indio_dev->ring->access.set_bpd) {
>> + if (indio_dev->scan_timestamp)
>> + if (indio_dev->scan_count) /* Timestamp and data */
>> + size = 3*sizeof(s64);
> That is nasty. Please change to something based on actual sizes. It would
> actually be preferable to hard code them to this random looking 3 *.
It is replaced by:
size = (((indio_dev->scan_count * sizeof(s16)) + sizeof(s64) - 1) &
~(sizeof(s64) - 1))
+ sizeof(s64);
>
>> + else /* Timestamp only */
>> + size = sizeof(s64);
>> + else /* Data only */
>> + size = indio_dev->scan_count*sizeof(s16);
>> + indio_dev->ring->access.set_bpd(indio_dev->ring, size);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int adis16209_data_rdy_ring_postenable(struct iio_dev *indio_dev)
>> +{
>> + return indio_dev->trig
>> + ? iio_trigger_attach_poll_func(indio_dev->trig,
>> + indio_dev->pollfunc)
>> + : 0;
>> +}
>> +
>> +static int adis16209_data_rdy_ring_predisable(struct iio_dev *indio_dev)
>> +{
>> + return indio_dev->trig
>> + ? iio_trigger_dettach_poll_func(indio_dev->trig,
>> + indio_dev->pollfunc)
>> + : 0;
>> +}
>> +
>> +void adis16209_unconfigure_ring(struct iio_dev *indio_dev)
>> +{
>> + kfree(indio_dev->pollfunc);
>> + iio_sw_rb_free(indio_dev->ring);
>> +}
>> +
>> +int adis16209_configure_ring(struct iio_dev *indio_dev)
>> +{
>> + int ret = 0;
>> + struct adis16209_state *st = indio_dev->dev_data;
>> + struct iio_ring_buffer *ring;
>> + INIT_WORK(&st->work_trigger_to_ring, adis16209_trigger_bh_to_ring);
>> + /* Set default scan mode */
>> +
> Don't know who did this first, but I rather like it. I was just writing
> directly to the relevant mask. This is much clearer.
>> + iio_scan_mask_set(indio_dev, iio_scan_el_supply.number);
>> + iio_scan_mask_set(indio_dev, iio_scan_el_rot.number);
>> + iio_scan_mask_set(indio_dev, iio_scan_el_accel_x.number);
>> + iio_scan_mask_set(indio_dev, iio_scan_el_accel_y.number);
>> + iio_scan_mask_set(indio_dev, iio_scan_el_temp.number);
>> + iio_scan_mask_set(indio_dev, iio_scan_el_aux_adc.number);
>> + iio_scan_mask_set(indio_dev, iio_scan_el_incli_x.number);
>> + iio_scan_mask_set(indio_dev, iio_scan_el_incli_y.number);
>> + indio_dev->scan_timestamp = true;
>> +
>> + indio_dev->scan_el_attrs = &adis16209_scan_el_group;
>> +
>> + ring = iio_sw_rb_allocate(indio_dev);
>> + if (!ring) {
>> + ret = -ENOMEM;
>> + return ret;
>> + }
>> + indio_dev->ring = ring;
>> + /* Effectively select the ring buffer implementation */
>> + iio_ring_sw_register_funcs(&ring->access);
>> + ring->preenable = &adis16209_data_rdy_ring_preenable;
>> + ring->postenable = &adis16209_data_rdy_ring_postenable;
>> + ring->predisable = &adis16209_data_rdy_ring_predisable;
>> + ring->owner = THIS_MODULE;
>> +
>> + indio_dev->pollfunc = kzalloc(sizeof(*indio_dev->pollfunc), GFP_KERNEL);
>> + if (indio_dev->pollfunc == NULL) {
>> + ret = -ENOMEM;
>> + goto error_iio_sw_rb_free;;
>> + }
>> + indio_dev->pollfunc->poll_func_main = &adis16209_poll_func_th;
>> + indio_dev->pollfunc->private_data = indio_dev;
>> + indio_dev->modes |= INDIO_RING_TRIGGERED;
>> + return 0;
>> +
>> +error_iio_sw_rb_free:
>> + iio_sw_rb_free(indio_dev->ring);
>> + return ret;
>> +}
>> +
>> +int adis16209_initialize_ring(struct iio_ring_buffer *ring)
>> +{
>> + return iio_ring_buffer_register(ring, 0);
>> +}
>> +
>> +void adis16209_uninitialize_ring(struct iio_ring_buffer *ring)
>> +{
>> + iio_ring_buffer_unregister(ring);
>> +}
>> diff --git a/drivers/staging/iio/accel/adis16209_trigger.c b/drivers/staging/iio/accel/adis16209_trigger.c
>> new file mode 100644
>> index 0000000..4a0507c
>> --- /dev/null
>> +++ b/drivers/staging/iio/accel/adis16209_trigger.c
>> @@ -0,0 +1,124 @@
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/mutex.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/list.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include "../iio.h"
>> +#include "../sysfs.h"
>> +#include "../trigger.h"
>> +#include "adis16209.h"
>> +
>> +/**
>> + * adis16209_data_rdy_trig_poll() the event handler for the data rdy trig
>> + **/
>> +static int adis16209_data_rdy_trig_poll(struct iio_dev *dev_info,
>> + int index,
>> + s64 timestamp,
>> + int no_test)
>> +{
>> + struct adis16209_state *st = iio_dev_get_devdata(dev_info);
>> + struct iio_trigger *trig = st->trig;
>> +
>> + trig->timestamp = timestamp;
>> + iio_trigger_poll(trig);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +IIO_EVENT_SH(data_rdy_trig, &adis16209_data_rdy_trig_poll);
>> +
>> +static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);
>> +
>> +static struct attribute *adis16209_trigger_attrs[] = {
>> + &dev_attr_name.attr,
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group adis16209_trigger_attr_group = {
>> + .attrs = adis16209_trigger_attrs,
>> +};
>> +
>> +/**
>> + * adis16209_data_rdy_trigger_set_state() set datardy interrupt state
>> + **/
>> +static int adis16209_data_rdy_trigger_set_state(struct iio_trigger *trig,
>> + bool state)
>> +{
>> + struct adis16209_state *st = trig->private_data;
>> + struct iio_dev *indio_dev = st->indio_dev;
>> + int ret = 0;
>> +
>> + dev_dbg(&indio_dev->dev, "%s (%d)\n", __func__, state);
>> + ret = adis16209_set_irq(&st->indio_dev->dev, state);
>> + if (state == false) {
>> + iio_remove_event_from_list(&iio_event_data_rdy_trig,
>> + &indio_dev->interrupts[0]
>> + ->ev_list);
>> + flush_scheduled_work();
>> + } else {
>> + iio_add_event_to_list(&iio_event_data_rdy_trig,
>> + &indio_dev->interrupts[0]->ev_list);
>> + }
>> + return ret;
>> +}
>> +
>> +/**
>> + * adis16209_trig_try_reen() try renabling irq for data rdy trigger
>> + * @trig: the datardy trigger
>> + **/
>> +static int adis16209_trig_try_reen(struct iio_trigger *trig)
>> +{
>> + struct adis16209_state *st = trig->private_data;
>> + enable_irq(st->us->irq);
>> + return 0;
>> +}
>> +
>> +int adis16209_probe_trigger(struct iio_dev *indio_dev)
>> +{
>> + int ret;
>> + struct adis16209_state *st = indio_dev->dev_data;
>> +
>> + st->trig = iio_allocate_trigger();
>> + st->trig->name = kmalloc(IIO_TRIGGER_NAME_LENGTH, GFP_KERNEL);
>> + if (!st->trig->name) {
>> + ret = -ENOMEM;
>> + goto error_free_trig;
>> + }
>> + snprintf((char *)st->trig->name,
>> + IIO_TRIGGER_NAME_LENGTH,
>> + "adis16209-dev%d", indio_dev->id);
>> + st->trig->dev.parent = &st->us->dev;
>> + st->trig->owner = THIS_MODULE;
>> + st->trig->private_data = st;
>> + st->trig->set_trigger_state = &adis16209_data_rdy_trigger_set_state;
>> + st->trig->try_reenable = &adis16209_trig_try_reen;
>> + st->trig->control_attrs = &adis16209_trigger_attr_group;
>> + ret = iio_trigger_register(st->trig);
>> +
>> + /* select default trigger */
>> + indio_dev->trig = st->trig;
>> + if (ret)
>> + goto error_free_trig_name;
>> +
>> + return 0;
>> +
>> +error_free_trig_name:
>> + kfree(st->trig->name);
>> +error_free_trig:
>> + iio_free_trigger(st->trig);
>> +
>> + return ret;
>> +}
>> +
>> +void adis16209_remove_trigger(struct iio_dev *indio_dev)
>> +{
>> + struct adis16209_state *state = indio_dev->dev_data;
>> +
>> + iio_trigger_unregister(state->trig);
>> + kfree(state->trig->name);
>> + iio_free_trigger(state->trig);
>> +}
>
> --
> 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
>
--
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
