[PATCH 1/7] staging:iio:adis16080: Perform sign extension

[PATCH 1/7] staging:iio:adis16080: Perform sign extension

[Lars-Peter Clausen]

The adis16080 reports sample values in twos complement. So we need to perform a
sign extension on the result, otherwise negative values will be reported
incorrectly as large positive values.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

---
This one should go into 3.8 if possible, the rest of the series is for 3.9
---
 drivers/staging/iio/gyro/adis16080_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
index 3525a68..149ff99 100644
--- a/drivers/staging/iio/gyro/adis16080_core.c
+++ b/drivers/staging/iio/gyro/adis16080_core.c
@@ -69,7 +69,7 @@ static int adis16080_spi_read(struct iio_dev *indio_dev,
 	ret = spi_read(st->us, st->buf, 2);
 
 	if (ret == 0)
-		*val = ((st->buf[0] & 0xF) << 8) | st->buf[1];
+		*val = sign_extend32(12, ((st->buf[0] & 0xF) << 8) | st->buf[1]);
 	mutex_unlock(&st->buf_lock);
 
 	return ret;
-- 
1.8.0

[PATCH 2/7] staging:iio:adis16080: Add device id table entry for the adis16100

[Lars-Peter Clausen]

The adis16100 is very similar to the adis16080. The driver description already
states that the driver supports the adis16100 as-well. But so far the there is
no device id table for the adis16100 and the drivers does not bind to a device
named adis16100.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/gyro/Kconfig          | 4 ++--
 drivers/staging/iio/gyro/adis16080_core.c | 9 ++++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/gyro/Kconfig b/drivers/staging/iio/gyro/Kconfig
index ea295b2..2f9467e 100644
--- a/drivers/staging/iio/gyro/Kconfig
+++ b/drivers/staging/iio/gyro/Kconfig
@@ -14,8 +14,8 @@ config ADIS16080
 	tristate "Analog Devices ADIS16080/100 Yaw Rate Gyroscope with SPI driver"
 	depends on SPI
 	help
-	  Say yes here to build support for Analog Devices adis16080/100 Yaw Rate
-	  Gyroscope with SPI.
+	  Say yes here to build support for Analog Devices ADIS16080, ADIS16100 Yaw
+	  Rate Gyroscope with SPI.
 
 config ADIS16130
 	tristate "Analog Devices ADIS16130 High Precision Angular Rate Sensor driver"
diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
index 149ff99..0268f2a 100644
--- a/drivers/staging/iio/gyro/adis16080_core.c
+++ b/drivers/staging/iio/gyro/adis16080_core.c
@@ -185,6 +185,13 @@ static int adis16080_remove(struct spi_device *spi)
 	return 0;
 }
 
+static const struct spi_device_id adis16080_ids[] = {
+	{ "adis16080", 0 },
+	{ "adis16100", 0 },
+	{},
+};
+MODULE_DEVICE_TABLE(spi, adis16080_ids);
+
 static struct spi_driver adis16080_driver = {
 	.driver = {
 		.name = "adis16080",
@@ -192,10 +199,10 @@ static struct spi_driver adis16080_driver = {
 	},
 	.probe = adis16080_probe,
 	.remove = adis16080_remove,
+	.id_table = adis16080_ids,
 };
 module_spi_driver(adis16080_driver);
 
 MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
 MODULE_DESCRIPTION("Analog Devices ADIS16080/100 Yaw Rate Gyroscope Driver");
 MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("spi:adis16080");
-- 
1.8.0

[PATCH 3/7] staging:iio:adis16080: Cleanup SPI transfer

[Lars-Peter Clausen]

During sampling the driver currently does a spi_read followed by a spi_write.
This is not a problem per se, since CS needs to be deasserted between the two
transfers. So even if another device claims the bus between the two transfers we
should still get a result. But the code is actually spread out over multiple
functions. E.g. the spi_read happens in one function the spi_write in another,
this makes the code harder to follow. This patch re-factors the code to just use
a single spi transaction to do both the read and the write transfer.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/gyro/adis16080_core.c | 61 +++++++++++++------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
index 0268f2a..6016e4c 100644
--- a/drivers/staging/iio/gyro/adis16080_core.c
+++ b/drivers/staging/iio/gyro/adis16080_core.c
@@ -42,32 +42,32 @@ struct adis16080_state {
 	u8 buf[2] ____cacheline_aligned;
 };
 
-static int adis16080_spi_write(struct iio_dev *indio_dev,
-		u16 val)
+static int adis16080_read_sample(struct iio_dev *indio_dev,
+		u16 addr, int *val)
 {
-	int ret;
 	struct adis16080_state *st = iio_priv(indio_dev);
-
-	mutex_lock(&st->buf_lock);
-	st->buf[0] = val >> 8;
-	st->buf[1] = val;
-
-	ret = spi_write(st->us, st->buf, 2);
-	mutex_unlock(&st->buf_lock);
-
-	return ret;
-}
-
-static int adis16080_spi_read(struct iio_dev *indio_dev,
-			      u16 *val)
-{
+	struct spi_message m;
 	int ret;
-	struct adis16080_state *st = iio_priv(indio_dev);
+	struct spi_transfer	t[] = {
+		{
+			.tx_buf		= &st->buf,
+			.len		= 2,
+			.cs_change	= 1,
+		}, {
+			.rx_buf		= &st->buf,
+			.len		= 2,
+		},
+	};
 
 	mutex_lock(&st->buf_lock);
+	st->buf[0] = addr >> 8;
+	st->buf[1] = addr;
 
-	ret = spi_read(st->us, st->buf, 2);
+	spi_message_init(&m);
+	spi_message_add_tail(&t[0], &m);
+	spi_message_add_tail(&t[1], &m);
 
+	ret = spi_sync(st->us, &m);
 	if (ret == 0)
 		*val = sign_extend32(12, ((st->buf[0] & 0xF) << 8) | st->buf[1]);
 	mutex_unlock(&st->buf_lock);
@@ -81,28 +81,17 @@ static int adis16080_read_raw(struct iio_dev *indio_dev,
 			     int *val2,
 			     long mask)
 {
-	int ret = -EINVAL;
-	u16 ut = 0;
-	/* Take the iio_dev status lock */
+	int ret;
 
-	mutex_lock(&indio_dev->mlock);
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ret = adis16080_spi_write(indio_dev,
-					  chan->address |
-					  ADIS16080_DIN_WRITE);
-		if (ret < 0)
-			break;
-		ret = adis16080_spi_read(indio_dev, &ut);
-		if (ret < 0)
-			break;
-		*val = ut;
-		ret = IIO_VAL_INT;
-		break;
+		mutex_lock(&indio_dev->mlock);
+		ret = adis16080_read_sample(indio_dev, chan->address, val);
+		mutex_unlock(&indio_dev->mlock);
+		return ret ? ret : IIO_VAL_INT;
 	}
-	mutex_unlock(&indio_dev->mlock);
 
-	return ret;
+	return -EINVAL;
 }
 
 static const struct iio_chan_spec adis16080_channels[] = {
-- 
1.8.0

[PATCH 4/7] staging:iio:adis16080: be16 cleanups

[Lars-Peter Clausen]

The sample buffer contains big endian 16bit words. So use the be16 datatype for
the buffer and use the proper helper functions for endianness conversion instead
of openconding it.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/gyro/adis16080_core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
index 6016e4c..6ff9ffc 100644
--- a/drivers/staging/iio/gyro/adis16080_core.c
+++ b/drivers/staging/iio/gyro/adis16080_core.c
@@ -39,7 +39,7 @@ struct adis16080_state {
 	struct spi_device		*us;
 	struct mutex			buf_lock;
 
-	u8 buf[2] ____cacheline_aligned;
+	__be16 buf ____cacheline_aligned;
 };
 
 static int adis16080_read_sample(struct iio_dev *indio_dev,
@@ -60,8 +60,7 @@ static int adis16080_read_sample(struct iio_dev *indio_dev,
 	};
 
 	mutex_lock(&st->buf_lock);
-	st->buf[0] = addr >> 8;
-	st->buf[1] = addr;
+	st->buf = cpu_to_be16(addr | ADIS16080_DIN_WRITE);
 
 	spi_message_init(&m);
 	spi_message_add_tail(&t[0], &m);
@@ -69,7 +68,7 @@ static int adis16080_read_sample(struct iio_dev *indio_dev,
 
 	ret = spi_sync(st->us, &m);
 	if (ret == 0)
-		*val = sign_extend32(12, ((st->buf[0] & 0xF) << 8) | st->buf[1]);
+		*val = sign_extend32(12, be16_to_cpu(st->buf));
 	mutex_unlock(&st->buf_lock);
 
 	return ret;
-- 
1.8.0

[PATCH 5/7] staging:iio:adis16080: Remove unnecessary lock

[Lars-Peter Clausen]

All sections in which the transfer buffer is accessed are already protected by
the IIO device's mlock. So we do not need the extra mutex protecting the buffer.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/gyro/adis16080_core.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
index 6ff9ffc..55ffe39 100644
--- a/drivers/staging/iio/gyro/adis16080_core.c
+++ b/drivers/staging/iio/gyro/adis16080_core.c
@@ -33,11 +33,9 @@
  * struct adis16080_state - device instance specific data
  * @us:			actual spi_device to write data
  * @buf:		transmit or receive buffer
- * @buf_lock:		mutex to protect tx and rx
  **/
 struct adis16080_state {
 	struct spi_device		*us;
-	struct mutex			buf_lock;
 
 	__be16 buf ____cacheline_aligned;
 };
@@ -59,7 +57,6 @@ static int adis16080_read_sample(struct iio_dev *indio_dev,
 		},
 	};
 
-	mutex_lock(&st->buf_lock);
 	st->buf = cpu_to_be16(addr | ADIS16080_DIN_WRITE);
 
 	spi_message_init(&m);
@@ -69,7 +66,6 @@ static int adis16080_read_sample(struct iio_dev *indio_dev,
 	ret = spi_sync(st->us, &m);
 	if (ret == 0)
 		*val = sign_extend32(12, be16_to_cpu(st->buf));
-	mutex_unlock(&st->buf_lock);
 
 	return ret;
 }
@@ -144,7 +140,6 @@ static int adis16080_probe(struct spi_device *spi)
 
 	/* Allocate the comms buffers */
 	st->us = spi;
-	mutex_init(&st->buf_lock);
 
 	indio_dev->name = spi->dev.driver->name;
 	indio_dev->channels = adis16080_channels;
-- 
1.8.0

[PATCH 6/7] staging:iio:adis16080: Add scale and offset attributes

[Lars-Peter Clausen]

Report scale and offset for the velocity, voltage and temperature channels.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/gyro/adis16080_core.c | 81 ++++++++++++++++++++++++++++---
 1 file changed, 75 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
index 55ffe39..1dde3c7 100644
--- a/drivers/staging/iio/gyro/adis16080_core.c
+++ b/drivers/staging/iio/gyro/adis16080_core.c
@@ -29,13 +29,20 @@
 
 #define ADIS16080_DIN_WRITE  (1 << 15)
 
+struct adis16080_chip_info {
+	int scale_val;
+	int scale_val2;
+};
+
 /**
  * struct adis16080_state - device instance specific data
  * @us:			actual spi_device to write data
+ * @info:		chip specific parameters
  * @buf:		transmit or receive buffer
  **/
 struct adis16080_state {
 	struct spi_device		*us;
+	const struct adis16080_chip_info *info;
 
 	__be16 buf ____cacheline_aligned;
 };
@@ -76,6 +83,7 @@ static int adis16080_read_raw(struct iio_dev *indio_dev,
 			     int *val2,
 			     long mask)
 {
+	struct adis16080_state *st = iio_priv(indio_dev);
 	int ret;
 
 	switch (mask) {
@@ -84,6 +92,40 @@ static int adis16080_read_raw(struct iio_dev *indio_dev,
 		ret = adis16080_read_sample(indio_dev, chan->address, val);
 		mutex_unlock(&indio_dev->mlock);
 		return ret ? ret : IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ANGL_VEL:
+			*val = st->info->scale_val;
+			*val2 = st->info->scale_val2;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_VOLTAGE:
+			/* VREF = 5V, 12 bits */
+			*val = 5000;
+			*val2 = 12;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		case IIO_TEMP:
+			/* 85 C = 585, 25 C = 0 */
+			*val = 85000 - 25000;
+			*val2 = 585;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_OFFSET:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			/* 2.5 V = 0 */
+			*val = 2048;
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			/* 85 C = 585, 25 C = 0 */
+			*val = DIV_ROUND_CLOSEST(25 * 585, 85 - 25);
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		break;
 	}
 
 	return -EINVAL;
@@ -94,25 +136,32 @@ static const struct iio_chan_spec adis16080_channels[] = {
 		.type = IIO_ANGL_VEL,
 		.modified = 1,
 		.channel2 = IIO_MOD_Z,
-		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT,
+		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
+			IIO_CHAN_INFO_SCALE_SEPARATE_BIT,
 		.address = ADIS16080_DIN_GYRO,
 	}, {
 		.type = IIO_VOLTAGE,
 		.indexed = 1,
 		.channel = 0,
-		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT,
+		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
+			IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+			IIO_CHAN_INFO_OFFSET_SEPARATE_BIT,
 		.address = ADIS16080_DIN_AIN1,
 	}, {
 		.type = IIO_VOLTAGE,
 		.indexed = 1,
 		.channel = 1,
-		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT,
+		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
+			IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+			IIO_CHAN_INFO_OFFSET_SEPARATE_BIT,
 		.address = ADIS16080_DIN_AIN2,
 	}, {
 		.type = IIO_TEMP,
 		.indexed = 1,
 		.channel = 0,
-		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT,
+		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
+			IIO_CHAN_INFO_SCALE_SEPARATE_BIT |
+			IIO_CHAN_INFO_OFFSET_SEPARATE_BIT,
 		.address = ADIS16080_DIN_TEMP,
 	}
 };
@@ -122,8 +171,27 @@ static const struct iio_info adis16080_info = {
 	.driver_module = THIS_MODULE,
 };
 
+enum {
+	ID_ADIS16080,
+	ID_ADIS16100,
+};
+
+static const struct adis16080_chip_info adis16080_chip_info[] = {
+	[ID_ADIS16080] = {
+		/* 80 degree = 819, 819 rad = 46925 degree */
+		.scale_val = 80,
+		.scale_val2 = 46925,
+	},
+	[ID_ADIS16100] = {
+		/* 300 degree = 1230, 1230 rad = 70474 degree */
+		.scale_val = 300,
+		.scale_val2 = 70474,
+	},
+};
+
 static int adis16080_probe(struct spi_device *spi)
 {
+	const struct spi_device_id *id = spi_get_device_id(spi);
 	int ret;
 	struct adis16080_state *st;
 	struct iio_dev *indio_dev;
@@ -140,6 +208,7 @@ static int adis16080_probe(struct spi_device *spi)
 
 	/* Allocate the comms buffers */
 	st->us = spi;
+	st->info = &adis16080_chip_info[id->driver_data];
 
 	indio_dev->name = spi->dev.driver->name;
 	indio_dev->channels = adis16080_channels;
@@ -169,8 +238,8 @@ static int adis16080_remove(struct spi_device *spi)
 }
 
 static const struct spi_device_id adis16080_ids[] = {
-	{ "adis16080", 0 },
-	{ "adis16100", 0 },
+	{ "adis16080", ID_ADIS16080 },
+	{ "adis16100", ID_ADIS16100 },
 	{},
 };
 MODULE_DEVICE_TABLE(spi, adis16080_ids);
-- 
1.8.0

[PATCH 7/7] staging:iio:adis16080: Move out of staging

[Lars-Peter Clausen]

The driver is rather simple and in a good shape. It follows the IIO ABI and the
standard codechecker tools do not report any issues, so move it out of staging.

While moving it also remove one outdated 'fixme' comment.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/iio/gyro/Kconfig                                           | 7 +++++++
 drivers/iio/gyro/Makefile                                          | 1 +
 .../{staging/iio/gyro/adis16080_core.c => iio/gyro/adis16080.c}    | 1 -
 drivers/staging/iio/gyro/Kconfig                                   | 7 -------
 drivers/staging/iio/gyro/Makefile                                  | 3 ---
 5 files changed, 8 insertions(+), 11 deletions(-)
 rename drivers/{staging/iio/gyro/adis16080_core.c => iio/gyro/adis16080.c} (99%)

diff --git a/drivers/iio/gyro/Kconfig b/drivers/iio/gyro/Kconfig
index 48ed148..e8d3a56 100644
--- a/drivers/iio/gyro/Kconfig
+++ b/drivers/iio/gyro/Kconfig
@@ -3,6 +3,13 @@
 #
 menu "Digital gyroscope sensors"
 
+config ADIS16080
+	tristate "Analog Devices ADIS16080/100 Yaw Rate Gyroscope with SPI driver"
+	depends on SPI
+	help
+	  Say yes here to build support for Analog Devices ADIS16080, ADIS16100 Yaw
+	  Rate Gyroscope with SPI.
+
 config ADIS16136
 	tristate "Analog devices ADIS16136 and similar gyroscopes driver"
 	depends on SPI_MASTER
diff --git a/drivers/iio/gyro/Makefile b/drivers/iio/gyro/Makefile
index 702a058..9b090ee 100644
--- a/drivers/iio/gyro/Makefile
+++ b/drivers/iio/gyro/Makefile
@@ -2,5 +2,6 @@
 # Makefile for industrial I/O gyroscope sensor drivers
 #
 
+obj-$(CONFIG_ADIS16080) += adis16080.o
 obj-$(CONFIG_ADIS16136) += adis16136.o
 obj-$(CONFIG_HID_SENSOR_GYRO_3D) += hid-sensor-gyro-3d.o
diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/iio/gyro/adis16080.c
similarity index 99%
rename from drivers/staging/iio/gyro/adis16080_core.c
rename to drivers/iio/gyro/adis16080.c
index 1dde3c7..ce23a23 100644
--- a/drivers/staging/iio/gyro/adis16080_core.c
+++ b/drivers/iio/gyro/adis16080.c
@@ -228,7 +228,6 @@ error_ret:
 	return ret;
 }
 
-/* fixme, confirm ordering in this function */
 static int adis16080_remove(struct spi_device *spi)
 {
 	iio_device_unregister(spi_get_drvdata(spi));
diff --git a/drivers/staging/iio/gyro/Kconfig b/drivers/staging/iio/gyro/Kconfig
index 2f9467e..b307c31 100644
--- a/drivers/staging/iio/gyro/Kconfig
+++ b/drivers/staging/iio/gyro/Kconfig
@@ -10,13 +10,6 @@ config ADIS16060
 	  Say yes here to build support for Analog Devices adis16060 wide bandwidth
 	  yaw rate gyroscope with SPI.
 
-config ADIS16080
-	tristate "Analog Devices ADIS16080/100 Yaw Rate Gyroscope with SPI driver"
-	depends on SPI
-	help
-	  Say yes here to build support for Analog Devices ADIS16080, ADIS16100 Yaw
-	  Rate Gyroscope with SPI.
-
 config ADIS16130
 	tristate "Analog Devices ADIS16130 High Precision Angular Rate Sensor driver"
 	depends on SPI
diff --git a/drivers/staging/iio/gyro/Makefile b/drivers/staging/iio/gyro/Makefile
index 1303569..8d6f60e 100644
--- a/drivers/staging/iio/gyro/Makefile
+++ b/drivers/staging/iio/gyro/Makefile
@@ -5,9 +5,6 @@
 adis16060-y             := adis16060_core.o
 obj-$(CONFIG_ADIS16060) += adis16060.o
 
-adis16080-y             := adis16080_core.o
-obj-$(CONFIG_ADIS16080) += adis16080.o
-
 adis16130-y             := adis16130_core.o
 obj-$(CONFIG_ADIS16130) += adis16130.o
 
-- 
1.8.0

Re: [PATCH 1/7] staging:iio:adis16080: Perform sign extension

[Jonathan Cameron]

On 01/09/2013 02:01 PM, Lars-Peter Clausen wrote:
> The adis16080 reports sample values in twos complement. So we need to perform a
> sign extension on the result, otherwise negative values will be reported
> incorrectly as large positive values.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Added to fixes-togreg branch of iio.git
> 
> ---
> This one should go into 3.8 if possible, the rest of the series is for 3.9
> ---
>  drivers/staging/iio/gyro/adis16080_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
> index 3525a68..149ff99 100644
> --- a/drivers/staging/iio/gyro/adis16080_core.c
> +++ b/drivers/staging/iio/gyro/adis16080_core.c
> @@ -69,7 +69,7 @@ static int adis16080_spi_read(struct iio_dev *indio_dev,
>  	ret = spi_read(st->us, st->buf, 2);
>  
>  	if (ret == 0)
> -		*val = ((st->buf[0] & 0xF) << 8) | st->buf[1];
> +		*val = sign_extend32(12, ((st->buf[0] & 0xF) << 8) | st->buf[1]);
>  	mutex_unlock(&st->buf_lock);
>  
>  	return ret;
> 

Re: [PATCH 1/7] staging:iio:adis16080: Perform sign extension

[Lars-Peter Clausen]

On 01/09/2013 09:09 PM, Jonathan Cameron wrote:
> On 01/09/2013 02:01 PM, Lars-Peter Clausen wrote:
>> The adis16080 reports sample values in twos complement. So we need to perform a
>> sign extension on the result, otherwise negative values will be reported
>> incorrectly as large positive values.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Added to fixes-togreg branch of iio.git
>>
>> ---
>> This one should go into 3.8 if possible, the rest of the series is for 3.9
>> ---
>>  drivers/staging/iio/gyro/adis16080_core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
>> index 3525a68..149ff99 100644
>> --- a/drivers/staging/iio/gyro/adis16080_core.c
>> +++ b/drivers/staging/iio/gyro/adis16080_core.c
>> @@ -69,7 +69,7 @@ static int adis16080_spi_read(struct iio_dev *indio_dev,
>>  	ret = spi_read(st->us, st->buf, 2);
>>  
>>  	if (ret == 0)
>> -		*val = ((st->buf[0] & 0xF) << 8) | st->buf[1];
>> +		*val = sign_extend32(12, ((st->buf[0] & 0xF) << 8) | st->buf[1]);
>>  	mutex_unlock(&st->buf_lock);
>>  

Sorry, now that I look at it from a distance: This is completely wrong...
It needs to be,

sign_extend32((st->buf[0] & 0xF) << 8) | st->buf[1], 11);

Sorry again,
- Lars

Re: [PATCH 1/7] staging:iio:adis16080: Perform sign extension

[Jonathan Cameron]

On 01/09/2013 08:34 PM, Lars-Peter Clausen wrote:
> On 01/09/2013 09:09 PM, Jonathan Cameron wrote:
>> On 01/09/2013 02:01 PM, Lars-Peter Clausen wrote:
>>> The adis16080 reports sample values in twos complement. So we need to perform a
>>> sign extension on the result, otherwise negative values will be reported
>>> incorrectly as large positive values.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> Added to fixes-togreg branch of iio.git
>>>
>>> ---
>>> This one should go into 3.8 if possible, the rest of the series is for 3.9
>>> ---
>>>  drivers/staging/iio/gyro/adis16080_core.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
>>> index 3525a68..149ff99 100644
>>> --- a/drivers/staging/iio/gyro/adis16080_core.c
>>> +++ b/drivers/staging/iio/gyro/adis16080_core.c
>>> @@ -69,7 +69,7 @@ static int adis16080_spi_read(struct iio_dev *indio_dev,
>>>  	ret = spi_read(st->us, st->buf, 2);
>>>  
>>>  	if (ret == 0)
>>> -		*val = ((st->buf[0] & 0xF) << 8) | st->buf[1];
>>> +		*val = sign_extend32(12, ((st->buf[0] & 0xF) << 8) | st->buf[1]);
>>>  	mutex_unlock(&st->buf_lock);
>>>  
> 
> Sorry, now that I look at it from a distance: This is completely wrong...
> It needs to be,
> 
> sign_extend32((st->buf[0] & 0xF) << 8) | st->buf[1], 11);
> 
> Sorry again,
> - Lars
> 
Fixed up.

Hadn't pushed out yet anyway so only messing in my local tree.

Re: [PATCH 2/7] staging:iio:adis16080: Add device id table entry for the adis16100

[Jonathan Cameron]

On 01/09/2013 02:01 PM, Lars-Peter Clausen wrote:
> The adis16100 is very similar to the adis16080. The driver description already
> states that the driver supports the adis16100 as-well. But so far the there is
> no device id table for the adis16100 and the drivers does not bind to a device
> named adis16100.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
applied to togreg branch of iio.git
> ---
>  drivers/staging/iio/gyro/Kconfig          | 4 ++--
>  drivers/staging/iio/gyro/adis16080_core.c | 9 ++++++++-
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/gyro/Kconfig b/drivers/staging/iio/gyro/Kconfig
> index ea295b2..2f9467e 100644
> --- a/drivers/staging/iio/gyro/Kconfig
> +++ b/drivers/staging/iio/gyro/Kconfig
> @@ -14,8 +14,8 @@ config ADIS16080
>  	tristate "Analog Devices ADIS16080/100 Yaw Rate Gyroscope with SPI driver"
>  	depends on SPI
>  	help
> -	  Say yes here to build support for Analog Devices adis16080/100 Yaw Rate
> -	  Gyroscope with SPI.
> +	  Say yes here to build support for Analog Devices ADIS16080, ADIS16100 Yaw
> +	  Rate Gyroscope with SPI.
>  
>  config ADIS16130
>  	tristate "Analog Devices ADIS16130 High Precision Angular Rate Sensor driver"
> diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
> index 149ff99..0268f2a 100644
> --- a/drivers/staging/iio/gyro/adis16080_core.c
> +++ b/drivers/staging/iio/gyro/adis16080_core.c
> @@ -185,6 +185,13 @@ static int adis16080_remove(struct spi_device *spi)
>  	return 0;
>  }
>  
> +static const struct spi_device_id adis16080_ids[] = {
> +	{ "adis16080", 0 },
> +	{ "adis16100", 0 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(spi, adis16080_ids);
> +
>  static struct spi_driver adis16080_driver = {
>  	.driver = {
>  		.name = "adis16080",
> @@ -192,10 +199,10 @@ static struct spi_driver adis16080_driver = {
>  	},
>  	.probe = adis16080_probe,
>  	.remove = adis16080_remove,
> +	.id_table = adis16080_ids,
>  };
>  module_spi_driver(adis16080_driver);
>  
>  MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
>  MODULE_DESCRIPTION("Analog Devices ADIS16080/100 Yaw Rate Gyroscope Driver");
>  MODULE_LICENSE("GPL v2");
> -MODULE_ALIAS("spi:adis16080");
> 

Re: [PATCH 3/7] staging:iio:adis16080: Cleanup SPI transfer

[Jonathan Cameron]

On 01/09/2013 02:01 PM, Lars-Peter Clausen wrote:
> During sampling the driver currently does a spi_read followed by a spi_write.
> This is not a problem per se, since CS needs to be deasserted between the two
> transfers. So even if another device claims the bus between the two transfers we
> should still get a result. But the code is actually spread out over multiple
> functions. E.g. the spi_read happens in one function the spi_write in another,
> this makes the code harder to follow. This patch re-factors the code to just use
> a single spi transaction to do both the read and the write transfer.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Definitely an improvement.

Added to togreg branch of iio.git  There was a bit of fuzz
as patch 1 has changed and for that matter is going through
a different route so wasn't there.  Nothing significant
though..

> ---
>  drivers/staging/iio/gyro/adis16080_core.c | 61 +++++++++++++------------------
>  1 file changed, 25 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
> index 0268f2a..6016e4c 100644
> --- a/drivers/staging/iio/gyro/adis16080_core.c
> +++ b/drivers/staging/iio/gyro/adis16080_core.c
> @@ -42,32 +42,32 @@ struct adis16080_state {
>  	u8 buf[2] ____cacheline_aligned;
>  };
>  
> -static int adis16080_spi_write(struct iio_dev *indio_dev,
> -		u16 val)
> +static int adis16080_read_sample(struct iio_dev *indio_dev,
> +		u16 addr, int *val)
>  {
> -	int ret;
>  	struct adis16080_state *st = iio_priv(indio_dev);
> -
> -	mutex_lock(&st->buf_lock);
> -	st->buf[0] = val >> 8;
> -	st->buf[1] = val;
> -
> -	ret = spi_write(st->us, st->buf, 2);
> -	mutex_unlock(&st->buf_lock);
> -
> -	return ret;
> -}
> -
> -static int adis16080_spi_read(struct iio_dev *indio_dev,
> -			      u16 *val)
> -{
> +	struct spi_message m;
>  	int ret;
> -	struct adis16080_state *st = iio_priv(indio_dev);
> +	struct spi_transfer	t[] = {
> +		{
> +			.tx_buf		= &st->buf,
> +			.len		= 2,
> +			.cs_change	= 1,
> +		}, {
> +			.rx_buf		= &st->buf,
> +			.len		= 2,
> +		},
> +	};
>  
>  	mutex_lock(&st->buf_lock);
> +	st->buf[0] = addr >> 8;
> +	st->buf[1] = addr;
>  
> -	ret = spi_read(st->us, st->buf, 2);
> +	spi_message_init(&m);
> +	spi_message_add_tail(&t[0], &m);
> +	spi_message_add_tail(&t[1], &m);
>  
> +	ret = spi_sync(st->us, &m);
>  	if (ret == 0)
>  		*val = sign_extend32(12, ((st->buf[0] & 0xF) << 8) | st->buf[1]);
>  	mutex_unlock(&st->buf_lock);
> @@ -81,28 +81,17 @@ static int adis16080_read_raw(struct iio_dev *indio_dev,
>  			     int *val2,
>  			     long mask)
>  {
> -	int ret = -EINVAL;
> -	u16 ut = 0;
> -	/* Take the iio_dev status lock */
> +	int ret;
>  
> -	mutex_lock(&indio_dev->mlock);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		ret = adis16080_spi_write(indio_dev,
> -					  chan->address |
> -					  ADIS16080_DIN_WRITE);
> -		if (ret < 0)
> -			break;
> -		ret = adis16080_spi_read(indio_dev, &ut);
> -		if (ret < 0)
> -			break;
> -		*val = ut;
> -		ret = IIO_VAL_INT;
> -		break;
> +		mutex_lock(&indio_dev->mlock);
> +		ret = adis16080_read_sample(indio_dev, chan->address, val);
> +		mutex_unlock(&indio_dev->mlock);
> +		return ret ? ret : IIO_VAL_INT;
>  	}
> -	mutex_unlock(&indio_dev->mlock);
>  
> -	return ret;
> +	return -EINVAL;
>  }
>  
>  static const struct iio_chan_spec adis16080_channels[] = {
> 

Re: [PATCH 4/7] staging:iio:adis16080: be16 cleanups

[Jonathan Cameron]

On 01/09/2013 02:01 PM, Lars-Peter Clausen wrote:
> The sample buffer contains big endian 16bit words. So use the be16 datatype for
> the buffer and use the proper helper functions for endianness conversion instead
> of openconding it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Hmm.. Should have read on.  Having seen this I'm going to back
out the series and wait for the fix to propogate through to Greg's
staging-next tree.  Annoying but we are fairly early in the
cycle so if I get that out to him sometime in the next few days
there should be masses of time to add this series later.

Thanks

Jonathan
> ---
>  drivers/staging/iio/gyro/adis16080_core.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
> index 6016e4c..6ff9ffc 100644
> --- a/drivers/staging/iio/gyro/adis16080_core.c
> +++ b/drivers/staging/iio/gyro/adis16080_core.c
> @@ -39,7 +39,7 @@ struct adis16080_state {
>  	struct spi_device		*us;
>  	struct mutex			buf_lock;
>  
> -	u8 buf[2] ____cacheline_aligned;
> +	__be16 buf ____cacheline_aligned;
>  };
>  
>  static int adis16080_read_sample(struct iio_dev *indio_dev,
> @@ -60,8 +60,7 @@ static int adis16080_read_sample(struct iio_dev *indio_dev,
>  	};
>  
>  	mutex_lock(&st->buf_lock);
> -	st->buf[0] = addr >> 8;
> -	st->buf[1] = addr;
> +	st->buf = cpu_to_be16(addr | ADIS16080_DIN_WRITE);
>  
>  	spi_message_init(&m);
>  	spi_message_add_tail(&t[0], &m);
> @@ -69,7 +68,7 @@ static int adis16080_read_sample(struct iio_dev *indio_dev,
>  
>  	ret = spi_sync(st->us, &m);
>  	if (ret == 0)
> -		*val = sign_extend32(12, ((st->buf[0] & 0xF) << 8) | st->buf[1]);
> +		*val = sign_extend32(12, be16_to_cpu(st->buf));
>  	mutex_unlock(&st->buf_lock);
>  
>  	return ret;
> 

Re: [PATCH 1/7] staging:iio:adis16080: Perform sign extension

[Jonathan Cameron]

On 01/09/2013 09:16 PM, Jonathan Cameron wrote:
> On 01/09/2013 08:34 PM, Lars-Peter Clausen wrote:
>> On 01/09/2013 09:09 PM, Jonathan Cameron wrote:
>>> On 01/09/2013 02:01 PM, Lars-Peter Clausen wrote:
>>>> The adis16080 reports sample values in twos complement. So we need to perform a
>>>> sign extension on the result, otherwise negative values will be reported
>>>> incorrectly as large positive values.
>>>>
>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>> Added to fixes-togreg branch of iio.git
All now applied was a little fiddly as somehow the small change
here propogated to the context of about half the remaining patches.

Result all looks fine though...

>>>>
>>>> ---
>>>> This one should go into 3.8 if possible, the rest of the series is for 3.9
>>>> ---
>>>>  drivers/staging/iio/gyro/adis16080_core.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/staging/iio/gyro/adis16080_core.c b/drivers/staging/iio/gyro/adis16080_core.c
>>>> index 3525a68..149ff99 100644
>>>> --- a/drivers/staging/iio/gyro/adis16080_core.c
>>>> +++ b/drivers/staging/iio/gyro/adis16080_core.c
>>>> @@ -69,7 +69,7 @@ static int adis16080_spi_read(struct iio_dev *indio_dev,
>>>>  	ret = spi_read(st->us, st->buf, 2);
>>>>  
>>>>  	if (ret == 0)
>>>> -		*val = ((st->buf[0] & 0xF) << 8) | st->buf[1];
>>>> +		*val = sign_extend32(12, ((st->buf[0] & 0xF) << 8) | st->buf[1]);
>>>>  	mutex_unlock(&st->buf_lock);
>>>>  
>>
>> Sorry, now that I look at it from a distance: This is completely wrong...
>> It needs to be,
>>
>> sign_extend32((st->buf[0] & 0xF) << 8) | st->buf[1], 11);
>>
>> Sorry again,
>> - Lars
>>
> Fixed up.
> 
> Hadn't pushed out yet anyway so only messing in my local tree.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>