From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ppsw-41.csi.cam.ac.uk ([131.111.8.141]:36063 "EHLO ppsw-41.csi.cam.ac.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753896Ab1CURjQ (ORCPT ); Mon, 21 Mar 2011 13:39:16 -0400 Message-ID: <4D878D8B.6070902@cam.ac.uk> Date: Mon, 21 Mar 2011 17:40:27 +0000 From: Jonathan Cameron MIME-Version: 1.0 To: cliff.cai@analog.com CC: linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, drivers@analog.com, device-drivers-devel@blackfin.uclinux.org, Cliff Cai Subject: Re: [PATCH RESEND v2]IIO driver for Analog Devices Digital Output Gyroscope ADXRS450 References: <1300526846-27183-1-git-send-email-cliff.cai@analog.com> In-Reply-To: <1300526846-27183-1-git-send-email-cliff.cai@analog.com> Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org On 03/19/11 09:27, cliff.cai@analog.com wrote: > From: Cliff Cai >=20 > Change v2:v1: >=20 > 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 documen= tation for them. They are quad and dynamic_null_correction. We need to first establish whether they are of general utility and henc= e 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 ass= erting these bits. This 10-bit register allows the user to adjust the static rateout of the device by up to =B16.4=B0/sec. " which makes me think it is an internally applied offset on the output s= ignal and hence calibbias in our abi. Other than that, various minor nitpicks inline. Jonathan >=20 > Signed-off-by: Cliff Cai > --- > 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 >=20 > diff --git a/drivers/staging/iio/gyro/Kconfig b/drivers/staging/iio/g= yro/Kconfig > index 236f15f..3432967 100644 > --- a/drivers/staging/iio/gyro/Kconfig > +++ b/drivers/staging/iio/gyro/Kconfig > @@ -45,3 +45,13 @@ config ADIS16251 > =20 > 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 driv= er" > + depends on SPI > + help > + Say yes here to build support for Analog Devices ADXRS450 program= mable > + 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) +=3D adis16260.o > =20 > adis16251-y :=3D adis16251_core.o > obj-$(CONFIG_ADIS16251) +=3D adis16251.o > + > +adxrs450-y :=3D adxrs450_core.o > +obj-$(CONFIG_ADXRS450) +=3D adxrs450.o > diff --git a/drivers/staging/iio/gyro/adxrs450.h b/drivers/staging/ii= o/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/stagi= ng/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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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 i= io_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 =3D dev_get_drvdata(dev); > + struct adxrs450_state *st =3D 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[] =3D { > + { > + .tx_buf =3D st->tx, > + .rx_buf =3D st->rx, > + .bits_per_word =3D 8, > + .len =3D 4, > + }, > + }; > + /* Needs to send the command twice to get the wanted value */ > + mutex_lock(&st->buf_lock); > + st->tx[0] =3D ADXRS450_READ_DATA | reg_address >> 7; > + st->tx[1] =3D reg_address << 1; > + st->tx[2] =3D 0; > + st->tx[3] =3D 0; > + spi_message_init(&msg); > + spi_message_add_tail(&xfers[0], &msg); > + ret =3D spi_sync(st->us, &msg); > + if (ret) { > + dev_err(&st->us->dev, "problem while reading 16 bit register 0x%02= x\n", > + reg_address); > + goto error_ret; > + } > + > + spi_message_init(&msg); > + spi_message_add_tail(&xfers[0], &msg); > + ret =3D spi_sync(st->us, &msg); > + if (ret) { > + dev_err(&st->us->dev, "problem while reading 16 bit register 0x%02= x\n", > + reg_address); > + goto error_ret; > + } > + > + *val =3D (st->rx[1] & 0x1f) << 11 | st->rx[2] << 3 | (st->rx[3] & 0= xe0) >> 5; > + > +error_ret: > + mutex_unlock(&st->buf_lock); > + return ret; > +} > + > +/** > + * adxrs450_spi_write_reg_16() - write 2 bytes data to a register pa= ir > + * @dev: device associated with child of actual device (iio_dev or i= io_trig) If it's an iio_trig, casting it to an iio_dev will give you somewhat in= terresting 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 =3D dev_get_drvdata(dev); > + struct adxrs450_state *st =3D iio_dev_get_devdata(indio_dev); > + int ret; Again, shouldn't be an array. > + struct spi_transfer xfers[] =3D { > + { > + .tx_buf =3D st->tx, > + .rx_buf =3D st->rx, > + .bits_per_word =3D 8, > + .len =3D 4, > + }, > + }; > + > + mutex_lock(&st->buf_lock); > + st->tx[0] =3D ADXRS450_WRITE_DATA | reg_address >> 7; > + st->tx[1] =3D reg_address << 1 | *val >> 15; > + st->tx[2] =3D *val >> 7; > + st->tx[3] =3D *val << 1; > + spi_message_init(&msg); > + spi_message_add_tail(&xfers[0], &msg); > + ret =3D spi_sync(st->us, &msg); > + if (ret) { > + dev_err(&st->us->dev, "problem while writing 16 bit register 0x%02= x\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 i= io_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 =3D dev_get_drvdata(dev); > + struct adxrs450_state *st =3D iio_dev_get_devdata(indio_dev); > + int ret; Same array comment. > + struct spi_transfer xfers[] =3D { > + { > + .tx_buf =3D st->tx, > + .rx_buf =3D st->rx, > + .bits_per_word =3D 8, > + .len =3D 4, > + } > + }; > + > + mutex_lock(&st->buf_lock); > + st->tx[0] =3D ADXRS450_SENSOR_DATA; > + st->tx[1] =3D 0; > + st->tx[2] =3D 0; > + st->tx[3] =3D 0; > + > + spi_message_init(&msg); > + spi_message_add_tail(&xfers[0], &msg); > + ret =3D 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 =3D spi_sync(st->us, &msg); > + if (ret) { > + dev_err(&st->us->dev, "Problem while reading sensor data\n"); > + goto error_ret; > + } > + > + *val =3D (st->rx[0] & 0x03) << 14 | st->rx[1] << 6 | (st->rx[2] & 0= xfc) >> 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 indirectio= n 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 i= io_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 =3D dev_get_drvdata(dev); > + struct adxrs450_state *st =3D iio_dev_get_devdata(indio_dev); > + int ret; Another unneeded array. > + struct spi_transfer xfers[] =3D { > + { > + .tx_buf =3D st->tx, > + .rx_buf =3D st->rx, > + .bits_per_word =3D 8, > + .len =3D 4, > + }, > + }; > + > + mutex_lock(&st->buf_lock); > + st->tx[0] =3D ADXRS450_SENSOR_DATA; > + st->tx[1] =3D 0; > + st->tx[2] =3D 0; > + st->tx[3] =3D 0; > + if (chk) > + st->tx[3] |=3D (ADXRS450_CHK | ADXRS450_P); > + spi_message_init(&msg); > + spi_message_add_tail(&xfers[0], &msg); > + ret =3D 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 =3D st->rx[0] << 24 | st->rx[1] << 16 | st->rx[2] << 8 | st->r= x[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 =3D 0; No need to initialize len. > + u16 t; > + ret =3D adxrs450_spi_read_reg_16(dev, > + ADXRS450_TEMP1, > + &t); > + if (ret) > + return ret; > + len =3D 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 =3D 0; Same for this len > + u16 t; > + ret =3D adxrs450_spi_read_reg_16(dev, > + ADXRS450_QUAD1, > + &t); > + if (ret) > + return ret; > + len =3D 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 =3D=3D 0 || len > 2) > + return -EINVAL; > + memcpy(&val, buf, len); > + ret =3D 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 =3D 0; another unneeded init of len > + u16 t; > + > + ret =3D adxrs450_spi_sensor_data(dev, &t); > + if (ret) > + return ret; > + > + len =3D 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 =3D &st->indio_dev->dev; > + > + msleep(ADXRS450_STARTUP_DELAY*2); > + ret =3D adxrs450_spi_initial(dev, &t, 1); > + if (ret) > + return ret; > + if (t !=3D 0x01) { > + dev_err(&st->us->dev, "The initial response is not correct!\n"); > + return -ENODEV; > + > + } > + > + msleep(ADXRS450_STARTUP_DELAY); > + ret =3D adxrs450_spi_initial(dev, &t, 0); > + if (ret) > + return ret; > + > + msleep(ADXRS450_STARTUP_DELAY); > + ret =3D adxrs450_spi_initial(dev, &t, 0); > + if (ret) > + return ret; > + if (((t & 0xff) | 0x01) !=3D 0xff || ADXRS450_GET_ST(t) !=3D 2) { > + dev_err(&st->us->dev, "The second response is not correct!\n"); > + return -EIO; > + > + } > + ret =3D adxrs450_spi_initial(dev, &t, 0); > + if (ret) > + return ret; > + if (((t & 0xff) | 0x01) !=3D 0xff || ADXRS450_GET_ST(t) !=3D 2) { > + dev_err(&st->us->dev, "The third response is not correct!\n"); > + return -EIO; > + > + } > + ret =3D 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 =3D 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 =3D adxrs450_spi_read_reg_16(dev, ADXRS450_SNL, &data); > + if (ret) > + return ret; > + t =3D data; > + ret =3D adxrs450_spi_read_reg_16(dev, ADXRS450_SNH, &data); > + if (ret) > + return ret; > + t |=3D 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[] =3D { 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 =3D { > + .attrs =3D adxrs450_attributes, > +}; > + > +static int __devinit adxrs450_probe(struct spi_device *spi) > +{ > + int ret, regdone =3D 0; > + struct adxrs450_state *st =3D kzalloc(sizeof *st, GFP_KERNEL); > + if (!st) { > + ret =3D -ENOMEM; > + goto error_ret; > + } > + /* This is only used for removal purposes */ > + spi_set_drvdata(spi, st); > + > + /* Allocate the comms buffers */ > + st->rx =3D kzalloc(sizeof(*st->rx)*ADXRS450_MAX_RX, GFP_KERNEL); > + if (st->rx =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto error_free_st; > + } > + st->tx =3D kzalloc(sizeof(*st->tx)*ADXRS450_MAX_TX, GFP_KERNEL); > + if (st->tx =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto error_free_rx; > + } > + st->us =3D spi; > + mutex_init(&st->buf_lock); > + /* setup the industrialio driver allocated elements */ > + st->indio_dev =3D iio_allocate_device(); > + if (st->indio_dev =3D=3D NULL) { > + ret =3D -ENOMEM; > + goto error_free_tx; > + } > + > + st->indio_dev->dev.parent =3D &spi->dev; > + st->indio_dev->attrs =3D &adxrs450_attribute_group; > + st->indio_dev->dev_data =3D (void *)(st); > + st->indio_dev->driver_module =3D THIS_MODULE; > + st->indio_dev->modes =3D INDIO_DIRECT_MODE; > + > + ret =3D iio_device_register(st->indio_dev); > + if (ret) > + goto error_free_dev; > + regdone =3D 1; > + > + /* Get the device into a sane initial state */ > + ret =3D 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 =3D spi_get_drvdata(spi); Might as well just go with iio_device_unregister(st->indio_dev); > + struct iio_dev *indio_dev =3D st->indio_dev; > + > + iio_device_unregister(indio_dev); > + kfree(st->tx); > + kfree(st->rx); > + kfree(st); > + > + return 0; > +} > + > +static struct spi_driver adxrs450_driver =3D { > + .driver =3D { > + .name =3D "adxrs450", > + .owner =3D THIS_MODULE, > + }, > + .probe =3D adxrs450_probe, > + .remove =3D __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 "); > +MODULE_DESCRIPTION("Analog Devices ADXRS450 Gyroscope SPI driver"); > +MODULE_LICENSE("GPL v2");