RE: [PATCH RESEND v2]IIO driver for Analog Devices Digital OutputGyroscope ADXRS450

[Cai, Cliff]

>-----Original Message-----
>From: Jonathan Cameron [mailto:jic23@xxxxxxxxx]
>Sent: 2011年3月22日 1:40
>To: Cai, Cliff
>Cc: linux-iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>Drivers; device-drivers-devel@xxxxxxxxxxxxxxxxxxxx; Cliff Cai
>Subject: Re: [PATCH RESEND v2]IIO driver for Analog Devices
>Digital Output Gyroscope ADXRS450
>
>On 03/19/11 09:27, cliff.cai@xxxxxxxxxx wrote:
>> From: Cliff Cai <cliff.cai@xxxxxxxxxx>
>>
>> Change v2:v1:
>>
>> Make modification according to Michael Hennerich's comments, correct
>> the spi transfer way,use existing sysfs interfaces.
>Hi Cliff,
>
>As you are proposing a couple of new interfaces we need to
>have documentation for them. They are quad and dynamic_null_correction.
>We need to first establish whether they are of general utility
>and hence should be in the main abi doc. The quadrature one
>isn't something I've seen before. Is it common in gyros?
>
>Dynamic null correction looks like it should be
>gyro_z_calibbias to me but I could be wrong.  The doc says "
>The user can make small adjustments to the rateout of the
>device by asserting these bits. This 10-bit register allows
>the user to adjust the static rateout of the device by up to ±6.4°/sec.
>"
>
>which makes me think it is an internally applied offset on the
>output signal and hence calibbias in our abi.
>
>Other than that, various minor nitpicks inline.
>
>Jonathan
>
>
>>
>> Signed-off-by: Cliff Cai<cliff@xxxxxxxxxx>
>> ---
>>  drivers/staging/iio/gyro/Kconfig         |   10 +
>>  drivers/staging/iio/gyro/Makefile        |    3 +
>>  drivers/staging/iio/gyro/adxrs450.h      |   59 ++++
>>  drivers/staging/iio/gyro/adxrs450_core.c |  471
>> ++++++++++++++++++++++++++++++
>>  4 files changed, 543 insertions(+), 0 deletions(-)  create mode
>> 100644 drivers/staging/iio/gyro/adxrs450.h
>>  create mode 100644 drivers/staging/iio/gyro/adxrs450_core.c
>>
>> diff --git a/drivers/staging/iio/gyro/Kconfig
>> b/drivers/staging/iio/gyro/Kconfig
>> index 236f15f..3432967 100644
>> --- a/drivers/staging/iio/gyro/Kconfig
>> +++ b/drivers/staging/iio/gyro/Kconfig
>> @@ -45,3 +45,13 @@ config ADIS16251
>>
>>        This driver can also be built as a module.  If so, the module
>>        will be called adis16251.
>> +
>> +config ADXRS450
>> +    tristate "Analog Devices ADXRS450 Digital Output
>Gyroscope SPI driver"
>> +    depends on SPI
>> +    help
>> +      Say yes here to build support for Analog Devices
>ADXRS450 programmable
>> +      digital output gyroscope.
>> +
>> +      This driver can also be built as a module.  If so, the module
>> +      will be called adxrs450.
>> diff --git a/drivers/staging/iio/gyro/Makefile
>> b/drivers/staging/iio/gyro/Makefile
>> index 2764c15..2212240 100644
>> --- a/drivers/staging/iio/gyro/Makefile
>> +++ b/drivers/staging/iio/gyro/Makefile
>> @@ -17,3 +17,6 @@ obj-$(CONFIG_ADIS16260) += adis16260.o
>>
>>  adis16251-y             := adis16251_core.o
>>  obj-$(CONFIG_ADIS16251) += adis16251.o
>> +
>> +adxrs450-y             := adxrs450_core.o
>> +obj-$(CONFIG_ADXRS450) += adxrs450.o
>> diff --git a/drivers/staging/iio/gyro/adxrs450.h
>> b/drivers/staging/iio/gyro/adxrs450.h
>> new file mode 100644
>> index 0000000..4633ef9
>> --- /dev/null
>> +++ b/drivers/staging/iio/gyro/adxrs450.h
>> @@ -0,0 +1,59 @@
>> +#ifndef SPI_ADXRS450_H_
>> +#define SPI_ADXRS450_H_
>> +
>> +#define ADXRS450_STARTUP_DELAY      50 /* ms */
>> +
>> +/* The MSB for the spi commands */
>> +#define ADXRS450_SENSOR_DATA    0x20
>> +#define ADXRS450_WRITE_DATA 0x40
>> +#define ADXRS450_READ_DATA  0x80
>> +
>> +#define ADXRS450_RATE1      0x00    /* Rate Registers */
>> +#define ADXRS450_TEMP1      0x02    /* Temperature Registers */
>> +#define ADXRS450_LOCST1     0x04    /* Low CST Memory Registers */
>> +#define ADXRS450_HICST1     0x06    /* High CST Memory Registers */
>> +#define ADXRS450_QUAD1      0x08    /* Quad Memory Registers */
>> +#define ADXRS450_FAULT1     0x0A    /* Fault Registers */
>> +#define ADXRS450_PID1       0x0C    /* Part ID Register 1 */
>> +#define ADXRS450_PID0       0x0D    /* Part ID Register 0 */
>> +#define ADXRS450_SNH        0x0E    /* Serial Number
>Registers, 4 bytes */
>> +#define ADXRS450_SNL        0x10
>> +#define ADXRS450_DNC1       0x12    /* Dynamic Null
>Correction Registers */
>> +/* Check bits */
>> +#define ADXRS450_P  0x01
>> +#define ADXRS450_CHK        0x02
>> +#define ADXRS450_CST        0x04
>> +#define ADXRS450_PWR        0x08
>> +#define ADXRS450_POR        0x10
>> +#define ADXRS450_NVM        0x20
>> +#define ADXRS450_Q  0x40
>> +#define ADXRS450_PLL        0x80
>> +#define ADXRS450_UV 0x100
>> +#define ADXRS450_OV 0x200
>> +#define ADXRS450_AMP        0x400
>> +#define ADXRS450_FAIL       0x800
>> +
>> +#define ADXRS450_WRERR_MASK (0x7 << 29)
>> +
>> +#define ADXRS450_MAX_RX 8
>> +#define ADXRS450_MAX_TX 8
>> +
>> +#define ADXRS450_GET_ST(a)  ((a >> 26) & 0x3)
>> +
>> +/**
>> + * struct adxrs450_state - device instance specific data
>> + * @us:                     actual spi_device
>> + * @indio_dev:              industrial I/O device structure
>> + * @tx:                     transmit buffer
>> + * @rx:                     recieve buffer
>> + * @buf_lock:               mutex to protect tx and rx
>> + **/
>> +struct adxrs450_state {
>> +    struct spi_device               *us;
>> +    struct iio_dev                  *indio_dev;
>> +    u8                              *tx;
>> +    u8                              *rx;
>> +    struct mutex                    buf_lock;
>> +};
>> +
>> +#endif /* SPI_ADXRS450_H_ */
>> diff --git a/drivers/staging/iio/gyro/adxrs450_core.c
>> b/drivers/staging/iio/gyro/adxrs450_core.c
>> new file mode 100644
>> index 0000000..f4f9d49
>> --- /dev/null
>> +++ b/drivers/staging/iio/gyro/adxrs450_core.c
>> @@ -0,0 +1,471 @@
>> +/*
>> + * ADXRS450 Digital Output Gyroscope 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/slab.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/list.h>
>> +
>> +#include "../iio.h"
>> +#include "../sysfs.h"
>> +#include "gyro.h"
>> +#include "../adc/adc.h"
>> +
>> +#include "adxrs450.h"
>> +
>This is only used in one place, I'd hard code it there.
>> +#define DRIVER_NAME         "ADXRS450"
>> +
>> +/**
>> + * adxrs450_spi_read_reg_16() - read 2 bytes from a register pair
>> + * @dev: device associated with child of actual device (iio_dev or
>> +iio_trig)
>> + * @reg_address: the address of the lower of the two
>registers,which
>> +should be an even address,
>> + * Second register's address is reg_address + 1.
>> + * @val: somewhere to pass back the value read  **/ static int
>> +adxrs450_spi_read_reg_16(struct device *dev,
>> +            u8 reg_address,
>> +            u16 *val)
>> +{
>> +    struct spi_message msg;
>> +    struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +    struct adxrs450_state *st = iio_dev_get_devdata(indio_dev);
>> +    int ret;
>  The array only has one element. Please make it not be an array.
>> +    struct spi_transfer xfers[] = {
>> +            {
>> +                    .tx_buf = st->tx,
>> +                    .rx_buf = st->rx,
>> +                    .bits_per_word = 8,
>> +                    .len = 4,
>> +            },
>> +    };
>> +    /* Needs to send the command twice to get the wanted value */
>> +    mutex_lock(&st->buf_lock);
>> +    st->tx[0] = ADXRS450_READ_DATA | reg_address >> 7;
>> +    st->tx[1] = reg_address << 1;
>> +    st->tx[2] = 0;
>> +    st->tx[3] = 0;
>> +    spi_message_init(&msg);
>> +    spi_message_add_tail(&xfers[0], &msg);
>> +    ret = spi_sync(st->us, &msg);
>> +    if (ret) {
>> +            dev_err(&st->us->dev, "problem while reading 16
>bit register 0x%02x\n",
>> +                            reg_address);
>> +            goto error_ret;
>> +    }
>> +
>> +    spi_message_init(&msg);
>> +    spi_message_add_tail(&xfers[0], &msg);
>> +    ret = spi_sync(st->us, &msg);
>> +    if (ret) {
>> +            dev_err(&st->us->dev, "problem while reading 16
>bit register 0x%02x\n",
>> +                            reg_address);
>> +            goto error_ret;
>> +    }
>> +
>> +    *val = (st->rx[1] & 0x1f) << 11 | st->rx[2] << 3 | (st->rx[3] &
>> +0xe0) >> 5;
>> +
>> +error_ret:
>> +    mutex_unlock(&st->buf_lock);
>> +    return ret;
>> +}
>> +
>> +/**
>> + * adxrs450_spi_write_reg_16() - write 2 bytes data to a register
>> +pair
>> + * @dev: device associated with child of actual device (iio_dev or
>> +iio_trig)
>If it's an iio_trig, casting it to an iio_dev will give you
>somewhat interresting results!
>> + * @reg_address: the address of the lower of the two
>registers,which
>> +should be an even address,
>> + * Second register's address is reg_address + 1.
>> + * @val: value to be written.
>> + **/
>> +static int adxrs450_spi_write_reg_16(struct device *dev,
>> +            u8 reg_address,
>> +            u16 *val)
>> +{
>> +    struct spi_message msg;
>> +    struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +    struct adxrs450_state *st = iio_dev_get_devdata(indio_dev);
>> +    int ret;
>Again, shouldn't be an array.
>> +    struct spi_transfer xfers[] = {
>> +            {
>> +                    .tx_buf = st->tx,
>> +                    .rx_buf = st->rx,
>> +                    .bits_per_word = 8,
>> +                    .len = 4,
>> +            },
>> +    };
>> +
>> +    mutex_lock(&st->buf_lock);
>> +    st->tx[0] = ADXRS450_WRITE_DATA | reg_address >> 7;
>> +    st->tx[1] = reg_address << 1 | *val >> 15;
>> +    st->tx[2] = *val >> 7;
>> +    st->tx[3] = *val << 1;
>> +    spi_message_init(&msg);
>> +    spi_message_add_tail(&xfers[0], &msg);
>> +    ret = spi_sync(st->us, &msg);
>> +    if (ret) {
>> +            dev_err(&st->us->dev, "problem while writing 16
>bit register 0x%02x\n",
>> +                            reg_address);
>> +            goto error_ret;
>only going to next line so don't need the goto and as a result
>no need for the brackets either.
>> +    }
>> +
>> +error_ret:
>> +    mutex_unlock(&st->buf_lock);
>> +    return ret;
>> +}
>> +
>> +/**
>> + * adxrs450_spi_sensor_data() - read 2 bytes sensor data
>> + * @dev: device associated with child of actual device (iio_dev or
>> +iio_trig)
>> + * @val: somewhere to pass back the value read  **/ static int
>> +adxrs450_spi_sensor_data(struct device *dev, u16 *val) {
>> +    struct spi_message msg;
>> +    struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +    struct adxrs450_state *st = iio_dev_get_devdata(indio_dev);
>> +    int ret;
>Same array comment.
>> +    struct spi_transfer xfers[] = {
>> +            {
>> +                    .tx_buf = st->tx,
>> +                    .rx_buf = st->rx,
>> +                    .bits_per_word = 8,
>> +                    .len = 4,
>> +            }
>> +    };
>> +
>> +    mutex_lock(&st->buf_lock);
>> +    st->tx[0] = ADXRS450_SENSOR_DATA;
>> +    st->tx[1] = 0;
>> +    st->tx[2] = 0;
>> +    st->tx[3] = 0;
>> +
>> +    spi_message_init(&msg);
>> +    spi_message_add_tail(&xfers[0], &msg);
>> +    ret = spi_sync(st->us, &msg);
>> +    if (ret) {
>> +            dev_err(&st->us->dev, "Problem while reading
>sensor data\n");
>> +            goto error_ret;
>> +    }
>> +
>> +    spi_message_init(&msg);
>> +    spi_message_add_tail(&xfers[0], &msg);
>> +    ret = spi_sync(st->us, &msg);
>> +    if (ret) {
>> +            dev_err(&st->us->dev, "Problem while reading
>sensor data\n");
>> +            goto error_ret;
>> +    }
>> +
>> +    *val = (st->rx[0] & 0x03) << 14 | st->rx[1] << 6 | (st->rx[2] &
>> +0xfc) >> 2;
>> +error_ret:
>> +    mutex_unlock(&st->buf_lock);
>> +    return ret;
>> +}
>> +
>
>This looks to only be called from initial setup. Might as well
>just make it take an adxrs450_state directly and save the
>careful indirection that is currently going on to ensure it
>gets the same dev as the other write functions.
>> +/**
>> + * adxrs450_spi_initial() - use for initializing procedure.
>> + * @dev: device associated with child of actual device (iio_dev or
>> +iio_trig)
>> + * @val: somewhere to pass back the value read  **/ static int
>> +adxrs450_spi_initial(struct device *dev,
>> +            u32 *val, char chk)
>> +{
>> +    struct spi_message msg;
>> +    struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +    struct adxrs450_state *st = iio_dev_get_devdata(indio_dev);
>> +    int ret;
>Another unneeded array.
>> +    struct spi_transfer xfers[] = {
>> +            {
>> +                    .tx_buf = st->tx,
>> +                    .rx_buf = st->rx,
>> +                    .bits_per_word = 8,
>> +                    .len = 4,
>> +            },
>> +    };
>> +
>> +    mutex_lock(&st->buf_lock);
>> +    st->tx[0] = ADXRS450_SENSOR_DATA;
>> +    st->tx[1] = 0;
>> +    st->tx[2] = 0;
>> +    st->tx[3] = 0;
>> +    if (chk)
>> +            st->tx[3] |= (ADXRS450_CHK | ADXRS450_P);
>> +    spi_message_init(&msg);
>> +    spi_message_add_tail(&xfers[0], &msg);
>> +    ret = spi_sync(st->us, &msg);
>> +    if (ret) {
>> +            dev_err(&st->us->dev, "Problem while reading
>initializing data\n");
>> +            goto error_ret;
>> +    }
>> +
>Looks like an endinaness conversion to me. be32tocpu.
>
>> +    *val = st->rx[0] << 24 | st->rx[1] << 16 | st->rx[2] << 8 |
>> +st->rx[3];
>> +
>> +error_ret:
>> +    mutex_unlock(&st->buf_lock);
>> +    return ret;
>> +}
>> +
>> +static ssize_t adxrs450_read_temp(struct device *dev,
>> +            struct device_attribute *attr,
>> +            char *buf)
>> +{
>> +    int ret, len = 0;
>No need to initialize len.
>> +    u16 t;
>> +    ret = adxrs450_spi_read_reg_16(dev,
>> +                    ADXRS450_TEMP1,
>> +                    &t);
>> +    if (ret)
>> +            return ret;
>> +    len = sprintf(buf, "%d\n", t);
>> +    return len;
>> +}
>> +
>> +static ssize_t adxrs450_read_quad(struct device *dev,
>> +            struct device_attribute *attr,
>> +            char *buf)
>> +{
>> +    int ret, len = 0;
>Same for this len
>> +    u16 t;
>> +    ret = adxrs450_spi_read_reg_16(dev,
>> +                    ADXRS450_QUAD1,
>> +                    &t);
>> +    if (ret)
>> +            return ret;
>> +    len = sprintf(buf, "%d\n", t);
>> +    return len;
>> +}
>> +
>> +static ssize_t adxrs450_write_dnc(struct device *dev,
>> +            struct device_attribute *attr,
>> +            const char *buf,
>> +            size_t len)
>> +{
>> +    int ret;
>> +    u16 val;
>> +
>> +    if (len == 0 || len > 2)
>> +            return -EINVAL;
>> +    memcpy(&val, buf, len);
>> +    ret = adxrs450_spi_write_reg_16(dev,
>> +                    ADXRS450_DNC1,
>> +                    &val);
>Err, so what is meant to be written to this?  Looks like
>you'll be dumping a random single character in to the register...
>Documentation would clear this up.
>
>> +    return ret ? ret : len;
>> +}
>> +
>> +static ssize_t adxrs450_read_sensor_data(struct device *dev,
>> +            struct device_attribute *attr,
>> +            char *buf)
>> +{
>> +    int ret, len = 0;
>another unneeded init of len
>> +    u16 t;
>> +
>> +    ret = adxrs450_spi_sensor_data(dev, &t);
>> +    if (ret)
>> +            return ret;
>> +
>> +    len = sprintf(buf, "%d\n", t);
>> +    return len;
>> +}
>> +
>> +/* Recommended Startup Sequence by spec */ static int
>> +adxrs450_initial_setup(struct adxrs450_state *st) {
>> +    u32 t;
>> +    u16 data;
>> +    int ret;
>> +    struct device *dev = &st->indio_dev->dev;
>> +
>> +    msleep(ADXRS450_STARTUP_DELAY*2);
>> +    ret = adxrs450_spi_initial(dev, &t, 1);
>> +    if (ret)
>> +            return ret;
>> +    if (t != 0x01) {
>> +            dev_err(&st->us->dev, "The initial response is
>not correct!\n");
>> +            return -ENODEV;
>> +
>> +    }
>> +
>> +    msleep(ADXRS450_STARTUP_DELAY);
>> +    ret = adxrs450_spi_initial(dev, &t, 0);
>> +    if (ret)
>> +            return ret;
>> +
>> +    msleep(ADXRS450_STARTUP_DELAY);
>> +    ret = adxrs450_spi_initial(dev, &t, 0);
>> +    if (ret)
>> +            return ret;
>> +    if (((t & 0xff) | 0x01) != 0xff || ADXRS450_GET_ST(t) != 2) {
>> +            dev_err(&st->us->dev, "The second response is
>not correct!\n");
>> +            return -EIO;
>> +
>> +    }
>> +    ret = adxrs450_spi_initial(dev, &t, 0);
>> +    if (ret)
>> +            return ret;
>> +    if (((t & 0xff) | 0x01) != 0xff || ADXRS450_GET_ST(t) != 2) {
>> +            dev_err(&st->us->dev, "The third response is
>not correct!\n");
>> +            return -EIO;
>> +
>> +    }
>> +    ret = adxrs450_spi_read_reg_16(dev, ADXRS450_FAULT1, &data);
>> +    if (ret)
>> +            return ret;
>> +    if (data & 0x0fff) {
>> +            dev_err(&st->us->dev, "The device is not in
>normal status!\n");
>> +            return -EINVAL;
>> +    }
>> +    ret = adxrs450_spi_read_reg_16(dev, ADXRS450_PID1, &data);
>> +    if (ret)
>> +            return ret;
>> +    dev_info(&st->us->dev, "The Part ID is 0x%x\n", data);
>> +
>> +    ret = adxrs450_spi_read_reg_16(dev, ADXRS450_SNL, &data);
>> +    if (ret)
>> +            return ret;
>> +    t = data;
>> +    ret = adxrs450_spi_read_reg_16(dev, ADXRS450_SNH, &data);
>> +    if (ret)
>> +            return ret;
>> +    t |= data << 16;
>> +    dev_info(&st->us->dev, "The Serial Number is 0x%x\n", t);
>> +
>> +    dev_info(&st->us->dev, "%s at CS%d\n", DRIVER_NAME,
>> +                    st->us->chip_select);
>Not really useful info to the average reader of the log.
>Please clean this one out.
>> +
>> +    return 0;
>> +}
>> +
>I note there are two versions of this chip (from datasheet).
>We should probably support changing this axis according to
>which one we have.  Z is out of chip plane. The other two are
>arbitrary if we only have 1 axis on the chip.

Currently,I don't know the way to tell what kind package the device has.
So,just keep the Z-axis one.


Cliff

>> +static IIO_DEV_ATTR_GYRO_Z(adxrs450_read_sensor_data, 0); static
>> +IIO_DEV_ATTR_TEMP_RAW(adxrs450_read_temp);
>> +static IIO_DEVICE_ATTR(quad, S_IRUGO,
>> +            adxrs450_read_quad, NULL, 0);
>> +static IIO_DEVICE_ATTR(dynamic_null_correction, S_IWUGO,
>IWUSR please. People get nervous for any greater permissions than that.
>> +            NULL, adxrs450_write_dnc, 0);
>> +static IIO_CONST_ATTR(name, "adxrs450");
>> +
>> +static struct attribute *adxrs450_attributes[] = {
>bonus blank line here. Please remove.
>> +
>> +    &iio_dev_attr_gyro_z_raw.dev_attr.attr,
>> +    &iio_dev_attr_temp_raw.dev_attr.attr,
>> +    &iio_dev_attr_quad.dev_attr.attr,
>> +    &iio_dev_attr_dynamic_null_correction.dev_attr.attr,
>> +    &iio_const_attr_name.dev_attr.attr,
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group adxrs450_attribute_group = {
>> +    .attrs = adxrs450_attributes,
>> +};
>> +
>> +static int __devinit adxrs450_probe(struct spi_device *spi) {
>> +    int ret, regdone = 0;
>> +    struct adxrs450_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)*ADXRS450_MAX_RX, GFP_KERNEL);
>> +    if (st->rx == NULL) {
>> +            ret = -ENOMEM;
>> +            goto error_free_st;
>> +    }
>> +    st->tx = kzalloc(sizeof(*st->tx)*ADXRS450_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->attrs = &adxrs450_attribute_group;
>> +    st->indio_dev->dev_data = (void *)(st);
>> +    st->indio_dev->driver_module = THIS_MODULE;
>> +    st->indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +    ret = iio_device_register(st->indio_dev);
>> +    if (ret)
>> +            goto error_free_dev;
>> +    regdone = 1;
>> +
>> +    /* Get the device into a sane initial state */
>> +    ret = adxrs450_initial_setup(st);
>> +    if (ret)
>> +            goto error_initial;
>> +    return 0;
>> +
>> +error_initial:
>> +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;
>> +}
>> +
>> +/* fixme, confirm ordering in this function */ static int
>> +adxrs450_remove(struct spi_device *spi) {
>> +    struct adxrs450_state *st = spi_get_drvdata(spi);
>
>Might as well just go with
>
>iio_device_unregister(st->indio_dev);
>> +    struct iio_dev *indio_dev = st->indio_dev;
>> +
>> +    iio_device_unregister(indio_dev);
>> +    kfree(st->tx);
>> +    kfree(st->rx);
>> +    kfree(st);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct spi_driver adxrs450_driver = {
>> +    .driver = {
>> +            .name = "adxrs450",
>> +            .owner = THIS_MODULE,
>> +    },
>> +    .probe = adxrs450_probe,
>> +    .remove = __devexit_p(adxrs450_remove), };
>> +
>> +static __init int adxrs450_init(void) {
>> +    return spi_register_driver(&adxrs450_driver);
>> +}
>> +module_init(adxrs450_init);
>> +
>> +static __exit void adxrs450_exit(void) {
>> +    spi_unregister_driver(&adxrs450_driver);
>> +}
>> +module_exit(adxrs450_exit);
>> +
>> +MODULE_AUTHOR("Cliff Cai <cliff.cai@xxxxxxxxxx>");
>> +MODULE_DESCRIPTION("Analog Devices ADXRS450 Gyroscope SPI driver");
>> +MODULE_LICENSE("GPL v2");
>
>
N?叉??y??b??千v??藓{.n???{?赙zXФ?塄}?财??j:+v???赙zZ+€?zf＂?????i????ア??璀??撷f?^j谦y??@A?囤?0鹅h??i