[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4D878D8B.6070902@cam.ac.uk>
Date: Mon, 21 Mar 2011 17:40:27 +0000
From: Jonathan Cameron <jic23@....ac.uk>
To: cliff.cai@...log.com
CC: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
drivers@...log.com, device-drivers-devel@...ckfin.uclinux.org,
Cliff Cai <cliff@...log.com>
Subject: Re: [PATCH RESEND v2]IIO driver for Analog Devices Digital Output
Gyroscope ADXRS450
On 03/19/11 09:27, cliff.cai@...log.com wrote:
> From: Cliff Cai <cliff.cai@...log.com>
>
> 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@...log.com>
> ---
> 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.
> +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@...log.com>");
> +MODULE_DESCRIPTION("Analog Devices ADXRS450 Gyroscope SPI driver");
> +MODULE_LICENSE("GPL v2");
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists