[PATCH 00/11] staging: iio: ad2s1200: Driver clean up

[PATCH 00/11] staging: iio: ad2s1200: Driver clean up

[David Veenstra]

Please forget the previous 3 emails, I forgot the message-id and
reply-to business.

This patch series is meant to move the ad2s1200 driver to mainline. In short
the following is done:

1. Clean up of minor code style issues
2. Fix minor bugs
3. Replace legacy GPIO ABI with modern ABI
4. Change the channel definitions of angular position and angular
   velocity to match the sysfs IIO ABI.

The channel for angular position had to be changed to another type
as there is no definition for IIO_ANGL in sysfs IIO ABI. There are other
types that model some kind of rotation, the candidates are:
- in_rot_from_north_*
- in_rot_quaternion
- in_incli

The in_rot_from_north_*_raw family is a rotation relevative to the
geographical north, or the magnetic north. These semantics are not a
good fit for the angular position of a resolver. The
in_rot_quaternion_raw als does not seem like a good choice, as it is
overkill for a rotation over a single and fixed axis. This is also
likely to be a cubersome format for users of the ABI interface. Finally,
there is in_incli which seems to be meant for any kind of inclination,
given in degrees. In my opinion in_incli is the best choice, as
it has a conveniant format and has semantics similar to that of angular
position.

David Veenstra (11):
  staging: iio: ad2s1200: Sort includes alphabetically
  staging: iio: ad2s1200: Reverse Christmas tree order
  staging: iio: ad2s1200: Add blank lines
  staging: iio: ad2s1200: Add kernel docs to driver state
  staging: iio: ad2s1200: Introduce variable for repeated value
  staging: iio: ad2s1200: Improve readability with be16_to_cpup
  staging: iio: ad2s1200: Ensure udelay(1) in all necessary code paths
  staging: iio: ad2s1200: Replace legacy gpio ABI with modern ABI
  staging: iio: ad2s1200: Add scaling factor for IIO_ANGL_VEL channel
  staging: iio: ad2s1200: Replace angle channel with inclination channel
  Move resolver ad2c1200 driver out of staging to mainline iio

 drivers/iio/Kconfig                           |   1 +
 drivers/iio/Makefile                          |   1 +
 drivers/iio/resolver/Kconfig                  |  17 +++
 drivers/iio/resolver/Makefile                 |   5 +
 drivers/{staging => }/iio/resolver/ad2s1200.c | 177 ++++++++++++++++++--------
 drivers/staging/iio/resolver/Kconfig          |  12 --
 drivers/staging/iio/resolver/Makefile         |   1 -
 7 files changed, 151 insertions(+), 63 deletions(-)
 create mode 100644 drivers/iio/resolver/Kconfig
 create mode 100644 drivers/iio/resolver/Makefile
 rename drivers/{staging => }/iio/resolver/ad2s1200.c (50%)

-- 
2.16.2

[PATCH 01/11] staging: iio: ad2s1200: Sort includes alphabetically

[David Veenstra]

This patches sorts all the includes in alphabetic order.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1200.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index aa62c64e9bc4..7a5b2d1927d6 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -9,16 +9,16 @@
  * published by the Free Software Foundation.
  *
  */
-#include <linux/types.h>
-#include <linux/mutex.h>
-#include <linux/device.h>
-#include <linux/spi/spi.h>
-#include <linux/slab.h>
-#include <linux/sysfs.h>
+#include <linux/bitops.h>
 #include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/gpio.h>
 #include <linux/module.h>
-#include <linux/bitops.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-- 
2.16.2

[PATCH 02/11] staging: iio: ad2s1200: Reverse Christmas tree order

[David Veenstra]

Reorders the variable declarations to prefer a reverse Christmas tree
order to improve readability.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1200.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 7a5b2d1927d6..94d0a66532fd 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -47,9 +47,9 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 			     int *val2,
 			     long m)
 {
+	struct ad2s1200_state *st = iio_priv(indio_dev);
 	int ret = 0;
 	s16 vel;
-	struct ad2s1200_state *st = iio_priv(indio_dev);
 
 	mutex_lock(&st->lock);
 	gpio_set_value(st->sample, 0);
@@ -102,10 +102,10 @@ static const struct iio_info ad2s1200_info = {
 
 static int ad2s1200_probe(struct spi_device *spi)
 {
+	unsigned short *pins = spi->dev.platform_data;
 	struct ad2s1200_state *st;
 	struct iio_dev *indio_dev;
 	int pn, ret = 0;
-	unsigned short *pins = spi->dev.platform_data;
 
 	for (pn = 0; pn < AD2S1200_PN; pn++) {
 		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
-- 
2.16.2

[PATCH 03/11] staging: iio: ad2s1200: Add blank lines

[David Veenstra]

Add blank lines to improve readability.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1200.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 94d0a66532fd..20df16b7852b 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -9,6 +9,7 @@
  * published by the Free Software Foundation.
  *
  */
+
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -53,10 +54,12 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 	mutex_lock(&st->lock);
 	gpio_set_value(st->sample, 0);
+
 	/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
 	udelay(1);
 	gpio_set_value(st->sample, 1);
 	gpio_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
+
 	ret = spi_read(st->sdev, st->rx, 2);
 	if (ret < 0) {
 		mutex_unlock(&st->lock);
@@ -76,9 +79,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&st->lock);
 		return -EINVAL;
 	}
+
 	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
 	udelay(1);
 	mutex_unlock(&st->lock);
+
 	return IIO_VAL_INT;
 }
 
@@ -116,9 +121,11 @@ static int ad2s1200_probe(struct spi_device *spi)
 			return ret;
 		}
 	}
+
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
+
 	spi_set_drvdata(spi, indio_dev);
 	st = iio_priv(indio_dev);
 	mutex_init(&st->lock);
-- 
2.16.2

[PATCH 04/11] staging: iio: ad2s1200: Add kernel docs to driver state

[David Veenstra]

Add missing kernel docs to the ad2s1200 driver state.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1200.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 20df16b7852b..00efba598671 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -34,6 +34,14 @@
 /* clock period in nano second */
 #define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
 
+/**
+ * struct ad2s1200_state - driver instance specific data
+ * @lock:	protect driver state
+ * @sdev:	spi device
+ * @sample:	GPIO pin SAMPLE
+ * @rdvel:	GPIO pin RDVEL
+ * @rx:		buffer for spi transfers
+ */
 struct ad2s1200_state {
 	struct mutex lock;
 	struct spi_device *sdev;
-- 
2.16.2

[PATCH 05/11] staging: iio: ad2s1200: Introduce variable for repeated value

[David Veenstra]

Add variable to hold &spi->dev in ad2s1200_probe. This value is repeatedly
used in ad2s1200_probe.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1200.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 00efba598671..eceb86e952de 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -118,19 +118,22 @@ static int ad2s1200_probe(struct spi_device *spi)
 	unsigned short *pins = spi->dev.platform_data;
 	struct ad2s1200_state *st;
 	struct iio_dev *indio_dev;
+	struct device *dev;
 	int pn, ret = 0;
 
+	dev = &spi->dev;
+
 	for (pn = 0; pn < AD2S1200_PN; pn++) {
-		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
+		ret = devm_gpio_request_one(dev, pins[pn], GPIOF_DIR_OUT,
 					    DRV_NAME);
 		if (ret) {
-			dev_err(&spi->dev, "request gpio pin %d failed\n",
+			dev_err(dev, "request gpio pin %d failed\n",
 				pins[pn]);
 			return ret;
 		}
 	}
 
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 
@@ -141,14 +144,14 @@ static int ad2s1200_probe(struct spi_device *spi)
 	st->sample = pins[0];
 	st->rdvel = pins[1];
 
-	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.parent = dev;
 	indio_dev->info = &ad2s1200_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = ad2s1200_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ad2s1200_channels);
 	indio_dev->name = spi_get_device_id(spi)->name;
 
-	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret)
 		return ret;
 
-- 
2.16.2

[PATCH 06/11] staging: iio: ad2s1200: Improve readability with be16_to_cpup

[David Veenstra]

The manual states that the data is contained in the upper 12 bits
of the 16 bits read by spi. The code that extracts these 12 bits
is correct for both be and le machines, but this is not clear
from a first glance.

To improve readability the relevant expressions are replaced
with equivalent expressions that use be16_to_cpup.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1200.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index eceb86e952de..e0e7c88368ed 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -58,7 +58,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 {
 	struct ad2s1200_state *st = iio_priv(indio_dev);
 	int ret = 0;
-	s16 vel;
+	u16 vel;
 
 	mutex_lock(&st->lock);
 	gpio_set_value(st->sample, 0);
@@ -74,14 +74,13 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 		return ret;
 	}
 
+	vel = be16_to_cpup((__be16 *)st->rx);
 	switch (chan->type) {
 	case IIO_ANGL:
-		*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
+		*val = vel >> 4;
 		break;
 	case IIO_ANGL_VEL:
-		vel = (((s16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
-		vel = sign_extend32(vel, 11);
-		*val = vel;
+		*val = sign_extend32((s16)vel >> 4, 11);
 		break;
 	default:
 		mutex_unlock(&st->lock);
-- 
2.16.2

[PATCH 07/11] staging: iio: ad2s1200: Ensure udelay(1) in all necessary code paths

[David Veenstra]

After a successful spi transaction, a udelay(1) is needed.
This doesn't happen for the default case of the switch statement
in ad2s1200_read_raw. This patch makes sure that it does.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1200.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index e0e7c88368ed..6ce9ca13094a 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -78,20 +78,22 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	switch (chan->type) {
 	case IIO_ANGL:
 		*val = vel >> 4;
+		ret = IIO_VAL_INT;
 		break;
 	case IIO_ANGL_VEL:
 		*val = sign_extend32((s16)vel >> 4, 11);
+		ret = IIO_VAL_INT;
 		break;
 	default:
-		mutex_unlock(&st->lock);
-		return -EINVAL;
+		ret = -EINVAL;
+		break;
 	}
 
 	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
 	udelay(1);
 	mutex_unlock(&st->lock);
 
-	return IIO_VAL_INT;
+	return ret;
 }
 
 static const struct iio_chan_spec ad2s1200_channels[] = {
-- 
2.16.2

[PATCH 08/11] staging: iio: ad2s1200: Replace legacy gpio ABI with modern ABI

[David Veenstra]

The legacy, integer based gpio ABI is replaced with the descriptor
based ABI.

For compatibility, it is first tried to use the platform data to
request the gpio's. Otherwise, it looks for the "sample" and "rdvel"
gpio function.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1200.c | 51 ++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 6ce9ca13094a..7ee1d9f76dfb 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
@@ -45,8 +46,8 @@
 struct ad2s1200_state {
 	struct mutex lock;
 	struct spi_device *sdev;
-	int sample;
-	int rdvel;
+	struct gpio_desc *sample;
+	struct gpio_desc *rdvel;
 	u8 rx[2] ____cacheline_aligned;
 };
 
@@ -61,12 +62,12 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	u16 vel;
 
 	mutex_lock(&st->lock);
-	gpio_set_value(st->sample, 0);
+	gpiod_set_value(st->sample, 0);
 
 	/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
 	udelay(1);
-	gpio_set_value(st->sample, 1);
-	gpio_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
+	gpiod_set_value(st->sample, 1);
+	gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
 
 	ret = spi_read(st->sdev, st->rx, 2);
 	if (ret < 0) {
@@ -124,13 +125,18 @@ static int ad2s1200_probe(struct spi_device *spi)
 
 	dev = &spi->dev;
 
-	for (pn = 0; pn < AD2S1200_PN; pn++) {
-		ret = devm_gpio_request_one(dev, pins[pn], GPIOF_DIR_OUT,
-					    DRV_NAME);
-		if (ret) {
-			dev_err(dev, "request gpio pin %d failed\n",
-				pins[pn]);
-			return ret;
+	if (pins) {
+		for (pn = 0; pn < AD2S1200_PN; pn++) {
+			ret = devm_gpio_request_one(dev, pins[pn],
+						    GPIOF_DIR_OUT,
+						    DRV_NAME);
+			if (ret) {
+				dev_err(dev,
+					"Failed to claim gpio %d\n: err=%d",
+					pins[pn],
+					ret);
+				return ret;
+			}
 		}
 	}
 
@@ -142,8 +148,25 @@ static int ad2s1200_probe(struct spi_device *spi)
 	st = iio_priv(indio_dev);
 	mutex_init(&st->lock);
 	st->sdev = spi;
-	st->sample = pins[0];
-	st->rdvel = pins[1];
+
+	if (pins) {
+		st->sample = gpio_to_desc(pins[0]);
+		st->rdvel = gpio_to_desc(pins[1]);
+	} else {
+		st->sample = devm_gpiod_get(dev, "sample", GPIOD_OUT_LOW);
+		if (IS_ERR(st->sample)) {
+			dev_err(dev, "Failed to claim SAMPLE gpio: err=%ld\n",
+				PTR_ERR(st->sample));
+			return PTR_ERR(st->sample);
+		}
+
+		st->rdvel = devm_gpiod_get(dev, "rdvel", GPIOD_OUT_LOW);
+		if (IS_ERR(st->rdvel)) {
+			dev_err(dev, "Failed to claim RDVEL gpio: err=%ld\n",
+				PTR_ERR(st->rdvel));
+			return PTR_ERR(st->rdvel);
+		}
+	}
 
 	indio_dev->dev.parent = dev;
 	indio_dev->info = &ad2s1200_info;
-- 
2.16.2

[PATCH 09/11] staging: iio: ad2s1200: Add scaling factor for IIO_ANGL_VEL channel

[David Veenstra]

The sysfs iio ABI states that a unit of radians per second is
expected, but the 12-bit angular velocity register has rps as its unit.
So a fractional scaling factor of approximately 2 * Pi is added to the
angular velocity channel.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1200.c | 82 ++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 7ee1d9f76dfb..4b97a106012c 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -61,40 +61,69 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	int ret = 0;
 	u16 vel;
 
-	mutex_lock(&st->lock);
-	gpiod_set_value(st->sample, 0);
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ANGL_VEL:
+			/*
+			 * 2 * Pi is almost equal to
+			 * 2 * 103993 / 33102 = 103993 / (33102 / 2)
+			 * This fraction is based on OEIS series of
+			 * of nominator/denominator of convergents to Pi
+			 * (A002485 and A002486).
+			 *
+			 * iio_convert_raw_to_processed_unlocked uses integer
+			 * division. This will cause at most 5% error
+			 * (for very small values). But for 99.5% of the values
+			 * it will cause less that 1% error.
+			 */
+			*val = 103993;
+			*val2 = 33102 / 2;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&st->lock);
+		gpiod_set_value(st->sample, 0);
+
+		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
+		udelay(1);
+		gpiod_set_value(st->sample, 1);
+		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
+
+		ret = spi_read(st->sdev, st->rx, 2);
+		if (ret < 0) {
+			mutex_unlock(&st->lock);
+			return ret;
+		}
 
-	/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
-	udelay(1);
-	gpiod_set_value(st->sample, 1);
-	gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
+		vel = be16_to_cpup((__be16 *)st->rx);
+		switch (chan->type) {
+		case IIO_ANGL:
+			*val = vel >> 4;
+			ret = IIO_VAL_INT;
+			break;
+		case IIO_ANGL_VEL:
+			*val = sign_extend32((s16)vel >> 4, 11);
+			ret = IIO_VAL_INT;
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
 
-	ret = spi_read(st->sdev, st->rx, 2);
-	if (ret < 0) {
+		/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
+		udelay(1);
 		mutex_unlock(&st->lock);
-		return ret;
-	}
 
-	vel = be16_to_cpup((__be16 *)st->rx);
-	switch (chan->type) {
-	case IIO_ANGL:
-		*val = vel >> 4;
-		ret = IIO_VAL_INT;
-		break;
-	case IIO_ANGL_VEL:
-		*val = sign_extend32((s16)vel >> 4, 11);
-		ret = IIO_VAL_INT;
-		break;
+		return ret;
 	default:
-		ret = -EINVAL;
 		break;
 	}
 
-	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
-	udelay(1);
-	mutex_unlock(&st->lock);
-
-	return ret;
+	return -EINVAL;
 }
 
 static const struct iio_chan_spec ad2s1200_channels[] = {
@@ -108,6 +137,7 @@ static const struct iio_chan_spec ad2s1200_channels[] = {
 		.indexed = 1,
 		.channel = 0,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 	}
 };
 
-- 
2.16.2

[PATCH 10/11] staging: iio: ad2s1200: Replace angle channel with inclination channel

[David Veenstra]

The angle channel is not defined in sysfs iio ABI. So it is replaced
with an inclination channel, because it is defined in the ABI, and has the
semantics of an angle.

In addition, a fractional scaling factor of 360 / (2^12 -1) is added,
to scale the 12-bits angular position to degrees, conform the ABI.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1200.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 4b97a106012c..937676bcc0d4 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
+		case IIO_INCLI:
+			*val = 360;
+			*val2 = 0xFFF;
+			return IIO_VAL_FRACTIONAL;
 		case IIO_ANGL_VEL:
 			/*
 			 * 2 * Pi is almost equal to
@@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
 		udelay(1);
 		gpiod_set_value(st->sample, 1);
-		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
+		gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI));
 
 		ret = spi_read(st->sdev, st->rx, 2);
 		if (ret < 0) {
@@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 		vel = be16_to_cpup((__be16 *)st->rx);
 		switch (chan->type) {
-		case IIO_ANGL:
+		case IIO_INCLI:
 			*val = vel >> 4;
 			ret = IIO_VAL_INT;
 			break;
@@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 static const struct iio_chan_spec ad2s1200_channels[] = {
 	{
-		.type = IIO_ANGL,
+		.type = IIO_INCLI,
 		.indexed = 1,
 		.channel = 0,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 	}, {
 		.type = IIO_ANGL_VEL,
 		.indexed = 1,
-- 
2.16.2

[PATCH 11/11] Move resolver ad2c1200 driver out of staging to mainline iio

[David Veenstra]

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
 drivers/iio/Kconfig                           |  1 +
 drivers/iio/Makefile                          |  1 +
 drivers/iio/resolver/Kconfig                  | 17 +++++++++++++++++
 drivers/iio/resolver/Makefile                 |  5 +++++
 drivers/{staging => }/iio/resolver/ad2s1200.c |  0
 drivers/staging/iio/resolver/Kconfig          | 12 ------------
 drivers/staging/iio/resolver/Makefile         |  1 -
 7 files changed, 24 insertions(+), 13 deletions(-)
 create mode 100644 drivers/iio/resolver/Kconfig
 create mode 100644 drivers/iio/resolver/Makefile
 rename drivers/{staging => }/iio/resolver/ad2s1200.c (100%)

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index b3c8c6ef0dff..4bec3ccbf4a1 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -92,6 +92,7 @@ source "drivers/iio/potentiometer/Kconfig"
 source "drivers/iio/potentiostat/Kconfig"
 source "drivers/iio/pressure/Kconfig"
 source "drivers/iio/proximity/Kconfig"
+source "drivers/iio/resolver/Kconfig"
 source "drivers/iio/temperature/Kconfig"
 
 endif # IIO
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index b16b2e9ddc40..1865361b8714 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -35,5 +35,6 @@ obj-y += potentiometer/
 obj-y += potentiostat/
 obj-y += pressure/
 obj-y += proximity/
+obj-y += resolver/
 obj-y += temperature/
 obj-y += trigger/
diff --git a/drivers/iio/resolver/Kconfig b/drivers/iio/resolver/Kconfig
new file mode 100644
index 000000000000..2ced9f22aa70
--- /dev/null
+++ b/drivers/iio/resolver/Kconfig
@@ -0,0 +1,17 @@
+#
+# Resolver/Synchro drivers
+#
+menu "Resolver to digital converters"
+
+config AD2S1200
+	tristate "Analog Devices ad2s1200/ad2s1205 driver"
+	depends on SPI
+	depends on GPIOLIB || COMPILE_TEST
+	help
+	  Say yes here to build support for Analog Devices spi resolver
+	  to digital converters, ad2s1200 and ad2s1205, provides direct access
+	  via sysfs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad2s1200.
+endmenu
diff --git a/drivers/iio/resolver/Makefile b/drivers/iio/resolver/Makefile
new file mode 100644
index 000000000000..4e1dccae07e7
--- /dev/null
+++ b/drivers/iio/resolver/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for Resolver/Synchro drivers
+#
+
+obj-$(CONFIG_AD2S1200) += ad2s1200.o
diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/iio/resolver/ad2s1200.c
similarity index 100%
rename from drivers/staging/iio/resolver/ad2s1200.c
rename to drivers/iio/resolver/ad2s1200.c
diff --git a/drivers/staging/iio/resolver/Kconfig b/drivers/staging/iio/resolver/Kconfig
index 1c7e2860d6b7..6a469ee6101f 100644
--- a/drivers/staging/iio/resolver/Kconfig
+++ b/drivers/staging/iio/resolver/Kconfig
@@ -13,18 +13,6 @@ config AD2S90
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad2s90.
 
-config AD2S1200
-	tristate "Analog Devices ad2s1200/ad2s1205 driver"
-	depends on SPI
-	depends on GPIOLIB || COMPILE_TEST
-	help
-	  Say yes here to build support for Analog Devices spi resolver
-	  to digital converters, ad2s1200 and ad2s1205, provides direct access
-	  via sysfs.
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called ad2s1200.
-
 config AD2S1210
 	tristate "Analog Devices ad2s1210 driver"
 	depends on SPI
diff --git a/drivers/staging/iio/resolver/Makefile b/drivers/staging/iio/resolver/Makefile
index 14375e444ebf..8d901dc7500b 100644
--- a/drivers/staging/iio/resolver/Makefile
+++ b/drivers/staging/iio/resolver/Makefile
@@ -3,5 +3,4 @@
 #
 
 obj-$(CONFIG_AD2S90) += ad2s90.o
-obj-$(CONFIG_AD2S1200) += ad2s1200.o
 obj-$(CONFIG_AD2S1210) += ad2s1210.o
-- 
2.16.2

Re: [PATCH 05/11] staging: iio: ad2s1200: Introduce variable for repeated value

[Jonathan Cameron]

On Sun, 18 Mar 2018 14:35:46 +0100
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> Add variable to hold &spi->dev in ad2s1200_probe. This value is repeatedly
> used in ad2s1200_probe.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
I'm a little unconvinced by this one. It adds code to save
a really very trivial bit of repetition.  If it had been
via another pointer or this would have made a big difference
in number of lines by bringing a lot of entries below 80 chars
that weren't previous, then there would be a good argument.

Here, not so much.

Jonathan
> ---
>  drivers/staging/iio/resolver/ad2s1200.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 00efba598671..eceb86e952de 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -118,19 +118,22 @@ static int ad2s1200_probe(struct spi_device *spi)
>  	unsigned short *pins = spi->dev.platform_data;
>  	struct ad2s1200_state *st;
>  	struct iio_dev *indio_dev;
> +	struct device *dev;
>  	int pn, ret = 0;
>  
> +	dev = &spi->dev;
> +
>  	for (pn = 0; pn < AD2S1200_PN; pn++) {
> -		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
> +		ret = devm_gpio_request_one(dev, pins[pn], GPIOF_DIR_OUT,
>  					    DRV_NAME);
>  		if (ret) {
> -			dev_err(&spi->dev, "request gpio pin %d failed\n",
> +			dev_err(dev, "request gpio pin %d failed\n",
>  				pins[pn]);
>  			return ret;
>  		}
>  	}
>  
> -	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> @@ -141,14 +144,14 @@ static int ad2s1200_probe(struct spi_device *spi)
>  	st->sample = pins[0];
>  	st->rdvel = pins[1];
>  
> -	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->dev.parent = dev;
>  	indio_dev->info = &ad2s1200_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = ad2s1200_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(ad2s1200_channels);
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  
> -	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret)
>  		return ret;
>  

Re: [PATCH 07/11] staging: iio: ad2s1200: Ensure udelay(1) in all necessary code paths

[Jonathan Cameron]

On Sun, 18 Mar 2018 14:36:15 +0100
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> After a successful spi transaction, a udelay(1) is needed.
> This doesn't happen for the default case of the switch statement
> in ad2s1200_read_raw. This patch makes sure that it does.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Given this one can't actually happen as the chan->type
is always one of the two options, I can't see the point
in making sure we sleep.

The default is really just there to catch bugs and to suppress
the build warning we would otherwise get.

So I am unconvinced on this patch.

Jonathan
> ---
>  drivers/staging/iio/resolver/ad2s1200.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index e0e7c88368ed..6ce9ca13094a 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -78,20 +78,22 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  	switch (chan->type) {
>  	case IIO_ANGL:
>  		*val = vel >> 4;
> +		ret = IIO_VAL_INT;
>  		break;
>  	case IIO_ANGL_VEL:
>  		*val = sign_extend32((s16)vel >> 4, 11);
> +		ret = IIO_VAL_INT;
>  		break;
>  	default:
> -		mutex_unlock(&st->lock);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		break;
>  	}
>  
>  	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
>  	udelay(1);
>  	mutex_unlock(&st->lock);
>  
> -	return IIO_VAL_INT;
> +	return ret;
>  }
>  
>  static const struct iio_chan_spec ad2s1200_channels[] = {

Re: [PATCH 08/11] staging: iio: ad2s1200: Replace legacy gpio ABI with modern ABI

[Jonathan Cameron]

On Sun, 18 Mar 2018 14:36:33 +0100
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> The legacy, integer based gpio ABI is replaced with the descriptor
> based ABI.
> 
> For compatibility, it is first tried to use the platform data to
> request the gpio's. Otherwise, it looks for the "sample" and "rdvel"
> gpio function.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>

I'd like Michael's opinion on this.  Personally I think we are safe to just
drop the old platform data code and have just use the new method. It's not
exactly hard to convert any board files over!

Code looks fine to me other than that.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/resolver/ad2s1200.c | 51 ++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 6ce9ca13094a..7ee1d9f76dfb 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -14,6 +14,7 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> @@ -45,8 +46,8 @@
>  struct ad2s1200_state {
>  	struct mutex lock;
>  	struct spi_device *sdev;
> -	int sample;
> -	int rdvel;
> +	struct gpio_desc *sample;
> +	struct gpio_desc *rdvel;
>  	u8 rx[2] ____cacheline_aligned;
>  };
>  
> @@ -61,12 +62,12 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  	u16 vel;
>  
>  	mutex_lock(&st->lock);
> -	gpio_set_value(st->sample, 0);
> +	gpiod_set_value(st->sample, 0);
>  
>  	/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
>  	udelay(1);
> -	gpio_set_value(st->sample, 1);
> -	gpio_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
> +	gpiod_set_value(st->sample, 1);
> +	gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
>  
>  	ret = spi_read(st->sdev, st->rx, 2);
>  	if (ret < 0) {
> @@ -124,13 +125,18 @@ static int ad2s1200_probe(struct spi_device *spi)
>  
>  	dev = &spi->dev;
>  
> -	for (pn = 0; pn < AD2S1200_PN; pn++) {
> -		ret = devm_gpio_request_one(dev, pins[pn], GPIOF_DIR_OUT,
> -					    DRV_NAME);
> -		if (ret) {
> -			dev_err(dev, "request gpio pin %d failed\n",
> -				pins[pn]);
> -			return ret;
> +	if (pins) {
> +		for (pn = 0; pn < AD2S1200_PN; pn++) {
> +			ret = devm_gpio_request_one(dev, pins[pn],
> +						    GPIOF_DIR_OUT,
> +						    DRV_NAME);
> +			if (ret) {
> +				dev_err(dev,
> +					"Failed to claim gpio %d\n: err=%d",
> +					pins[pn],
> +					ret);
> +				return ret;
> +			}
>  		}
>  	}
>  
> @@ -142,8 +148,25 @@ static int ad2s1200_probe(struct spi_device *spi)
>  	st = iio_priv(indio_dev);
>  	mutex_init(&st->lock);
>  	st->sdev = spi;
> -	st->sample = pins[0];
> -	st->rdvel = pins[1];
> +
> +	if (pins) {
> +		st->sample = gpio_to_desc(pins[0]);
> +		st->rdvel = gpio_to_desc(pins[1]);
> +	} else {
> +		st->sample = devm_gpiod_get(dev, "sample", GPIOD_OUT_LOW);
> +		if (IS_ERR(st->sample)) {
> +			dev_err(dev, "Failed to claim SAMPLE gpio: err=%ld\n",
> +				PTR_ERR(st->sample));
> +			return PTR_ERR(st->sample);
> +		}
> +
> +		st->rdvel = devm_gpiod_get(dev, "rdvel", GPIOD_OUT_LOW);
> +		if (IS_ERR(st->rdvel)) {
> +			dev_err(dev, "Failed to claim RDVEL gpio: err=%ld\n",
> +				PTR_ERR(st->rdvel));
> +			return PTR_ERR(st->rdvel);
> +		}
> +	}
>  
>  	indio_dev->dev.parent = dev;
>  	indio_dev->info = &ad2s1200_info;

Re: [PATCH 10/11] staging: iio: ad2s1200: Replace angle channel with inclination channel

[Jonathan Cameron]

On Sun, 18 Mar 2018 14:37:04 +0100
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> The angle channel is not defined in sysfs iio ABI. So it is replaced
> with an inclination channel, because it is defined in the ABI, and has the
> semantics of an angle.
> 
> In addition, a fractional scaling factor of 360 / (2^12 -1) is added,
> to scale the 12-bits angular position to degrees, conform the ABI.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
No, please do not blugeon a device into the existing documented ABI.

A resolver does not measure something that can be remotely
sensibly mapped to inclination.  Inclination is relative to 'down'.
A resolver doesn't care about down.

Add an angle type to the ABI docs. It simply hasn't been documented
before because there were no drivers outside staging that use it.

Jonathan

> ---
>  drivers/staging/iio/resolver/ad2s1200.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 4b97a106012c..937676bcc0d4 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  	switch (m) {
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
> +		case IIO_INCLI:
> +			*val = 360;
> +			*val2 = 0xFFF;
> +			return IIO_VAL_FRACTIONAL;
>  		case IIO_ANGL_VEL:
>  			/*
>  			 * 2 * Pi is almost equal to
> @@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
>  		udelay(1);
>  		gpiod_set_value(st->sample, 1);
> -		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
> +		gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI));
>  
>  		ret = spi_read(st->sdev, st->rx, 2);
>  		if (ret < 0) {
> @@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  
>  		vel = be16_to_cpup((__be16 *)st->rx);
>  		switch (chan->type) {
> -		case IIO_ANGL:
> +		case IIO_INCLI:
>  			*val = vel >> 4;
>  			ret = IIO_VAL_INT;
>  			break;
> @@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  
>  static const struct iio_chan_spec ad2s1200_channels[] = {
>  	{
> -		.type = IIO_ANGL,
> +		.type = IIO_INCLI,
>  		.indexed = 1,
>  		.channel = 0,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>  	}, {
>  		.type = IIO_ANGL_VEL,
>  		.indexed = 1,

Re: [PATCH 11/11] Move resolver ad2c1200 driver out of staging to mainline iio

[Jonathan Cameron]

On Sun, 18 Mar 2018 14:37:22 +0100
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Please disable git move detection for a patch moving an IIO driver
out of staging.  It provides the opportunity for a full review of the
code as if we were considering a new driver.

Thanks for the set - I'm reasonably happy with all the patches I didn't
comment on btw.

Looking forward to V2.

Thanks,

Jonathan

> ---
>  drivers/iio/Kconfig                           |  1 +
>  drivers/iio/Makefile                          |  1 +
>  drivers/iio/resolver/Kconfig                  | 17 +++++++++++++++++
>  drivers/iio/resolver/Makefile                 |  5 +++++
>  drivers/{staging => }/iio/resolver/ad2s1200.c |  0
>  drivers/staging/iio/resolver/Kconfig          | 12 ------------
>  drivers/staging/iio/resolver/Makefile         |  1 -
>  7 files changed, 24 insertions(+), 13 deletions(-)
>  create mode 100644 drivers/iio/resolver/Kconfig
>  create mode 100644 drivers/iio/resolver/Makefile
>  rename drivers/{staging => }/iio/resolver/ad2s1200.c (100%)
> 
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index b3c8c6ef0dff..4bec3ccbf4a1 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -92,6 +92,7 @@ source "drivers/iio/potentiometer/Kconfig"
>  source "drivers/iio/potentiostat/Kconfig"
>  source "drivers/iio/pressure/Kconfig"
>  source "drivers/iio/proximity/Kconfig"
> +source "drivers/iio/resolver/Kconfig"
>  source "drivers/iio/temperature/Kconfig"
>  
>  endif # IIO
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index b16b2e9ddc40..1865361b8714 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -35,5 +35,6 @@ obj-y += potentiometer/
>  obj-y += potentiostat/
>  obj-y += pressure/
>  obj-y += proximity/
> +obj-y += resolver/
>  obj-y += temperature/
>  obj-y += trigger/
> diff --git a/drivers/iio/resolver/Kconfig b/drivers/iio/resolver/Kconfig
> new file mode 100644
> index 000000000000..2ced9f22aa70
> --- /dev/null
> +++ b/drivers/iio/resolver/Kconfig
> @@ -0,0 +1,17 @@
> +#
> +# Resolver/Synchro drivers
> +#
> +menu "Resolver to digital converters"
> +
> +config AD2S1200
> +	tristate "Analog Devices ad2s1200/ad2s1205 driver"
> +	depends on SPI
> +	depends on GPIOLIB || COMPILE_TEST
> +	help
> +	  Say yes here to build support for Analog Devices spi resolver
> +	  to digital converters, ad2s1200 and ad2s1205, provides direct access
> +	  via sysfs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad2s1200.
> +endmenu
> diff --git a/drivers/iio/resolver/Makefile b/drivers/iio/resolver/Makefile
> new file mode 100644
> index 000000000000..4e1dccae07e7
> --- /dev/null
> +++ b/drivers/iio/resolver/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for Resolver/Synchro drivers
> +#
> +
> +obj-$(CONFIG_AD2S1200) += ad2s1200.o
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/iio/resolver/ad2s1200.c
> similarity index 100%
> rename from drivers/staging/iio/resolver/ad2s1200.c
> rename to drivers/iio/resolver/ad2s1200.c
> diff --git a/drivers/staging/iio/resolver/Kconfig b/drivers/staging/iio/resolver/Kconfig
> index 1c7e2860d6b7..6a469ee6101f 100644
> --- a/drivers/staging/iio/resolver/Kconfig
> +++ b/drivers/staging/iio/resolver/Kconfig
> @@ -13,18 +13,6 @@ config AD2S90
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad2s90.
>  
> -config AD2S1200
> -	tristate "Analog Devices ad2s1200/ad2s1205 driver"
> -	depends on SPI
> -	depends on GPIOLIB || COMPILE_TEST
> -	help
> -	  Say yes here to build support for Analog Devices spi resolver
> -	  to digital converters, ad2s1200 and ad2s1205, provides direct access
> -	  via sysfs.
> -
> -	  To compile this driver as a module, choose M here: the
> -	  module will be called ad2s1200.
> -
>  config AD2S1210
>  	tristate "Analog Devices ad2s1210 driver"
>  	depends on SPI
> diff --git a/drivers/staging/iio/resolver/Makefile b/drivers/staging/iio/resolver/Makefile
> index 14375e444ebf..8d901dc7500b 100644
> --- a/drivers/staging/iio/resolver/Makefile
> +++ b/drivers/staging/iio/resolver/Makefile
> @@ -3,5 +3,4 @@
>  #
>  
>  obj-$(CONFIG_AD2S90) += ad2s90.o
> -obj-$(CONFIG_AD2S1200) += ad2s1200.o
>  obj-$(CONFIG_AD2S1210) += ad2s1210.o

Re: [PATCH 05/11] staging: iio: ad2s1200: Introduce variable for repeated value

[David Julian Veenstra]

On 23, March 2018 14:17, Jonathan Cameron wrote:

> On Sun, 18 Mar 2018 14:35:46 +0100
> David Veenstra <davidjulianveenstra@gmail.com> wrote:
>
>> Add variable to hold &spi->dev in ad2s1200_probe. This value is repeatedly
>> used in ad2s1200_probe.
>> 
>> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> I'm a little unconvinced by this one. It adds code to save
> a really very trivial bit of repetition.  If it had been
> via another pointer or this would have made a big difference
> in number of lines by bringing a lot of entries below 80 chars
> that weren't previous, then there would be a good argument.
>
> Here, not so much.

The 9th patch adds another 4 occurrences. So in total &st->dev appears
9 times in ad2s1200_probe. To me it seemed worth it to
introduce a variable for it. But I agree that it doesn't add much.

Best regards,
David Veenstra

>
> Jonathan
>> ---
>>  drivers/staging/iio/resolver/ad2s1200.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
>> index 00efba598671..eceb86e952de 100644
>> --- a/drivers/staging/iio/resolver/ad2s1200.c
>> +++ b/drivers/staging/iio/resolver/ad2s1200.c
>> @@ -118,19 +118,22 @@ static int ad2s1200_probe(struct spi_device *spi)
>>  	unsigned short *pins = spi->dev.platform_data;
>>  	struct ad2s1200_state *st;
>>  	struct iio_dev *indio_dev;
>> +	struct device *dev;
>>  	int pn, ret = 0;
>>  
>> +	dev = &spi->dev;
>> +
>>  	for (pn = 0; pn < AD2S1200_PN; pn++) {
>> -		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
>> +		ret = devm_gpio_request_one(dev, pins[pn], GPIOF_DIR_OUT,
>>  					    DRV_NAME);
>>  		if (ret) {
>> -			dev_err(&spi->dev, "request gpio pin %d failed\n",
>> +			dev_err(dev, "request gpio pin %d failed\n",
>>  				pins[pn]);
>>  			return ret;
>>  		}
>>  	}
>>  
>> -	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>>  	if (!indio_dev)
>>  		return -ENOMEM;
>>  
>> @@ -141,14 +144,14 @@ static int ad2s1200_probe(struct spi_device *spi)
>>  	st->sample = pins[0];
>>  	st->rdvel = pins[1];
>>  
>> -	indio_dev->dev.parent = &spi->dev;
>> +	indio_dev->dev.parent = dev;
>>  	indio_dev->info = &ad2s1200_info;
>>  	indio_dev->modes = INDIO_DIRECT_MODE;
>>  	indio_dev->channels = ad2s1200_channels;
>>  	indio_dev->num_channels = ARRAY_SIZE(ad2s1200_channels);
>>  	indio_dev->name = spi_get_device_id(spi)->name;
>>  
>> -	ret = devm_iio_device_register(&spi->dev, indio_dev);
>> +	ret = devm_iio_device_register(dev, indio_dev);
>>  	if (ret)
>>  		return ret;
>>  

Re: [PATCH 07/11] staging: iio: ad2s1200: Ensure udelay(1) in all necessary code paths

[David Julian Veenstra]

On 23, March 2018 14:20, Jonathan Cameron wrote:

> On Sun, 18 Mar 2018 14:36:15 +0100
> David Veenstra <davidjulianveenstra@gmail.com> wrote:
>
>> After a successful spi transaction, a udelay(1) is needed.
>> This doesn't happen for the default case of the switch statement
>> in ad2s1200_read_raw. This patch makes sure that it does.
>> 
>> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> Given this one can't actually happen as the chan->type
> is always one of the two options, I can't see the point
> in making sure we sleep.
>
> The default is really just there to catch bugs and to suppress
> the build warning we would otherwise get.
>
> So I am unconvinced on this patch.

I wasn't sure about this patch, but tried to be as critical as possible.
I'll leave it out for V2.

Best regards,
David Veenstra

>
> Jonathan
>> ---
>>  drivers/staging/iio/resolver/ad2s1200.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
>> index e0e7c88368ed..6ce9ca13094a 100644
>> --- a/drivers/staging/iio/resolver/ad2s1200.c
>> +++ b/drivers/staging/iio/resolver/ad2s1200.c
>> @@ -78,20 +78,22 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>>  	switch (chan->type) {
>>  	case IIO_ANGL:
>>  		*val = vel >> 4;
>> +		ret = IIO_VAL_INT;
>>  		break;
>>  	case IIO_ANGL_VEL:
>>  		*val = sign_extend32((s16)vel >> 4, 11);
>> +		ret = IIO_VAL_INT;
>>  		break;
>>  	default:
>> -		mutex_unlock(&st->lock);
>> -		return -EINVAL;
>> +		ret = -EINVAL;
>> +		break;
>>  	}
>>  
>>  	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
>>  	udelay(1);
>>  	mutex_unlock(&st->lock);
>>  
>> -	return IIO_VAL_INT;
>> +	return ret;
>>  }
>>  
>>  static const struct iio_chan_spec ad2s1200_channels[] = {

Re: [PATCH 10/11] staging: iio: ad2s1200: Replace angle channel with inclination channel

[David Julian Veenstra]

On 23, March 2018 14:27, Jonathan Cameron wrote:

> On Sun, 18 Mar 2018 14:37:04 +0100
> David Veenstra <davidjulianveenstra@gmail.com> wrote:
>
>> The angle channel is not defined in sysfs iio ABI. So it is replaced
>> with an inclination channel, because it is defined in the ABI, and has the
>> semantics of an angle.
>> 
>> In addition, a fractional scaling factor of 360 / (2^12 -1) is added,
>> to scale the 12-bits angular position to degrees, conform the ABI.
>> 
>> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> No, please do not blugeon a device into the existing documented ABI.
>
> A resolver does not measure something that can be remotely
> sensibly mapped to inclination.  Inclination is relative to 'down'.
> A resolver doesn't care about down.
>
> Add an angle type to the ABI docs. It simply hasn't been documented
> before because there were no drivers outside staging that use it.

I'm sorry, I misunderstood our previous discussing on this topic
(https://marc.info/?l=linux-iio&m=152087432322446&w=2) as saying:
"there already exists another channel type that is a good enough match".

The in_incli and in_rot_from_* family all use degrees as their unit.
For V2 I'll change it back to an angle channel and use angle as its
unit.

Best regards,
David Veenstra

>
> Jonathan
>
>> ---
>>  drivers/staging/iio/resolver/ad2s1200.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
>> index 4b97a106012c..937676bcc0d4 100644
>> --- a/drivers/staging/iio/resolver/ad2s1200.c
>> +++ b/drivers/staging/iio/resolver/ad2s1200.c
>> @@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>>  	switch (m) {
>>  	case IIO_CHAN_INFO_SCALE:
>>  		switch (chan->type) {
>> +		case IIO_INCLI:
>> +			*val = 360;
>> +			*val2 = 0xFFF;
>> +			return IIO_VAL_FRACTIONAL;
>>  		case IIO_ANGL_VEL:
>>  			/*
>>  			 * 2 * Pi is almost equal to
>> @@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>>  		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
>>  		udelay(1);
>>  		gpiod_set_value(st->sample, 1);
>> -		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
>> +		gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI));
>>  
>>  		ret = spi_read(st->sdev, st->rx, 2);
>>  		if (ret < 0) {
>> @@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>>  
>>  		vel = be16_to_cpup((__be16 *)st->rx);
>>  		switch (chan->type) {
>> -		case IIO_ANGL:
>> +		case IIO_INCLI:
>>  			*val = vel >> 4;
>>  			ret = IIO_VAL_INT;
>>  			break;
>> @@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>>  
>>  static const struct iio_chan_spec ad2s1200_channels[] = {
>>  	{
>> -		.type = IIO_ANGL,
>> +		.type = IIO_INCLI,
>>  		.indexed = 1,
>>  		.channel = 0,
>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>>  	}, {
>>  		.type = IIO_ANGL_VEL,
>>  		.indexed = 1,

Re: [PATCH 05/11] staging: iio: ad2s1200: Introduce variable for repeated value

[Jonathan Cameron]

On Sat, 24 Mar 2018 13:22:22 +0100
David Julian Veenstra <davidjulianveenstra@gmail.com> wrote:

> On 23, March 2018 14:17, Jonathan Cameron wrote:
> 
> > On Sun, 18 Mar 2018 14:35:46 +0100
> > David Veenstra <davidjulianveenstra@gmail.com> wrote:
> >  
> >> Add variable to hold &spi->dev in ad2s1200_probe. This value is repeatedly
> >> used in ad2s1200_probe.
> >> 
> >> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>  
> > I'm a little unconvinced by this one. It adds code to save
> > a really very trivial bit of repetition.  If it had been
> > via another pointer or this would have made a big difference
> > in number of lines by bringing a lot of entries below 80 chars
> > that weren't previous, then there would be a good argument.
> >
> > Here, not so much.  
> 
> The 9th patch adds another 4 occurrences. So in total &st->dev appears
> 9 times in ad2s1200_probe. To me it seemed worth it to
> introduce a variable for it. But I agree that it doesn't add much.
> 
> Best regards,
> David Veenstra
> 
OK, up to you then, I don't care enough to resist the change.

Jonathan


> >
> > Jonathan  
> >> ---
> >>  drivers/staging/iio/resolver/ad2s1200.c | 13 ++++++++-----
> >>  1 file changed, 8 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> >> index 00efba598671..eceb86e952de 100644
> >> --- a/drivers/staging/iio/resolver/ad2s1200.c
> >> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> >> @@ -118,19 +118,22 @@ static int ad2s1200_probe(struct spi_device *spi)
> >>  	unsigned short *pins = spi->dev.platform_data;
> >>  	struct ad2s1200_state *st;
> >>  	struct iio_dev *indio_dev;
> >> +	struct device *dev;
> >>  	int pn, ret = 0;
> >>  
> >> +	dev = &spi->dev;
> >> +
> >>  	for (pn = 0; pn < AD2S1200_PN; pn++) {
> >> -		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
> >> +		ret = devm_gpio_request_one(dev, pins[pn], GPIOF_DIR_OUT,
> >>  					    DRV_NAME);
> >>  		if (ret) {
> >> -			dev_err(&spi->dev, "request gpio pin %d failed\n",
> >> +			dev_err(dev, "request gpio pin %d failed\n",
> >>  				pins[pn]);
> >>  			return ret;
> >>  		}
> >>  	}
> >>  
> >> -	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> >> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> >>  	if (!indio_dev)
> >>  		return -ENOMEM;
> >>  
> >> @@ -141,14 +144,14 @@ static int ad2s1200_probe(struct spi_device *spi)
> >>  	st->sample = pins[0];
> >>  	st->rdvel = pins[1];
> >>  
> >> -	indio_dev->dev.parent = &spi->dev;
> >> +	indio_dev->dev.parent = dev;
> >>  	indio_dev->info = &ad2s1200_info;
> >>  	indio_dev->modes = INDIO_DIRECT_MODE;
> >>  	indio_dev->channels = ad2s1200_channels;
> >>  	indio_dev->num_channels = ARRAY_SIZE(ad2s1200_channels);
> >>  	indio_dev->name = spi_get_device_id(spi)->name;
> >>  
> >> -	ret = devm_iio_device_register(&spi->dev, indio_dev);
> >> +	ret = devm_iio_device_register(dev, indio_dev);
> >>  	if (ret)
> >>  		return ret;
> >>    
> 
> --
> 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

Re: [PATCH 10/11] staging: iio: ad2s1200: Replace angle channel with inclination channel

[Jonathan Cameron]

On Sat, 24 Mar 2018 13:36:44 +0100
David Julian Veenstra <davidjulianveenstra@gmail.com> wrote:

> On 23, March 2018 14:27, Jonathan Cameron wrote:
> 
> > On Sun, 18 Mar 2018 14:37:04 +0100
> > David Veenstra <davidjulianveenstra@gmail.com> wrote:
> >  
> >> The angle channel is not defined in sysfs iio ABI. So it is replaced
> >> with an inclination channel, because it is defined in the ABI, and has the
> >> semantics of an angle.
> >> 
> >> In addition, a fractional scaling factor of 360 / (2^12 -1) is added,
> >> to scale the 12-bits angular position to degrees, conform the ABI.
> >> 
> >> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>  
> > No, please do not blugeon a device into the existing documented ABI.
> >
> > A resolver does not measure something that can be remotely
> > sensibly mapped to inclination.  Inclination is relative to 'down'.
> > A resolver doesn't care about down.
> >
> > Add an angle type to the ABI docs. It simply hasn't been documented
> > before because there were no drivers outside staging that use it.  
> 
> I'm sorry, I misunderstood our previous discussing on this topic
> (https://marc.info/?l=linux-iio&m=152087432322446&w=2) as saying:
> "there already exists another channel type that is a good enough match".
Ah, no I was making the point we need to match with the 'units' not
use the channel type. 

Hmm. We have an an unfortunate inconsistency between use of radians/sec
and degrees for different types.

Radians feels perfectly sensible for a rotary device, but not so much
for an angle of tilt.

I'm not sure how we resolve this cleanly or if we can.
My gut feeling is we need to go with radians for angle measures on
a rotating devices (to match against anglvel) and leave inclination
in degrees.

Sorry for the confusion, I'd missed that the units were inconsistent
which would have made that comment harder to interpret.

Jonathan

> 
> The in_incli and in_rot_from_* family all use degrees as their unit.
> For V2 I'll change it back to an angle channel and use angle as its
> unit.
> 
> Best regards,
> David Veenstra
> 
> >
> > Jonathan
> >  
> >> ---
> >>  drivers/staging/iio/resolver/ad2s1200.c | 11 ++++++++---
> >>  1 file changed, 8 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> >> index 4b97a106012c..937676bcc0d4 100644
> >> --- a/drivers/staging/iio/resolver/ad2s1200.c
> >> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> >> @@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> >>  	switch (m) {
> >>  	case IIO_CHAN_INFO_SCALE:
> >>  		switch (chan->type) {
> >> +		case IIO_INCLI:
> >> +			*val = 360;
> >> +			*val2 = 0xFFF;
> >> +			return IIO_VAL_FRACTIONAL;
> >>  		case IIO_ANGL_VEL:
> >>  			/*
> >>  			 * 2 * Pi is almost equal to
> >> @@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> >>  		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
> >>  		udelay(1);
> >>  		gpiod_set_value(st->sample, 1);
> >> -		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
> >> +		gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI));
> >>  
> >>  		ret = spi_read(st->sdev, st->rx, 2);
> >>  		if (ret < 0) {
> >> @@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> >>  
> >>  		vel = be16_to_cpup((__be16 *)st->rx);
> >>  		switch (chan->type) {
> >> -		case IIO_ANGL:
> >> +		case IIO_INCLI:
> >>  			*val = vel >> 4;
> >>  			ret = IIO_VAL_INT;
> >>  			break;
> >> @@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> >>  
> >>  static const struct iio_chan_spec ad2s1200_channels[] = {
> >>  	{
> >> -		.type = IIO_ANGL,
> >> +		.type = IIO_INCLI,
> >>  		.indexed = 1,
> >>  		.channel = 0,
> >>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >>  	}, {
> >>  		.type = IIO_ANGL_VEL,
> >>  		.indexed = 1,  
> 
> --
> 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

Re: [PATCH 10/11] staging: iio: ad2s1200: Replace angle channel with inclination channel

[David Julian Veenstra]

On 24, March 2018 14:12, Jonathan Cameron wrote:

> On Sat, 24 Mar 2018 13:36:44 +0100
> David Julian Veenstra <davidjulianveenstra@gmail.com> wrote:
>
>> On 23, March 2018 14:27, Jonathan Cameron wrote:
>> 
>> > On Sun, 18 Mar 2018 14:37:04 +0100
>> > David Veenstra <davidjulianveenstra@gmail.com> wrote:
>> >  
>> >> The angle channel is not defined in sysfs iio ABI. So it is replaced
>> >> with an inclination channel, because it is defined in the ABI, and has the
>> >> semantics of an angle.
>> >> 
>> >> In addition, a fractional scaling factor of 360 / (2^12 -1) is added,
>> >> to scale the 12-bits angular position to degrees, conform the ABI.
>> >> 
>> >> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>  
>> > No, please do not blugeon a device into the existing documented ABI.
>> >
>> > A resolver does not measure something that can be remotely
>> > sensibly mapped to inclination.  Inclination is relative to 'down'.
>> > A resolver doesn't care about down.
>> >
>> > Add an angle type to the ABI docs. It simply hasn't been documented
>> > before because there were no drivers outside staging that use it.  
>> 
>> I'm sorry, I misunderstood our previous discussing on this topic
>> (https://marc.info/?l=linux-iio&m=152087432322446&w=2) as saying:
>> "there already exists another channel type that is a good enough match".
> Ah, no I was making the point we need to match with the 'units' not
> use the channel type. 
>
> Hmm. We have an an unfortunate inconsistency between use of radians/sec
> and degrees for different types.
>
> Radians feels perfectly sensible for a rotary device, but not so much
> for an angle of tilt.
>
> I'm not sure how we resolve this cleanly or if we can.
> My gut feeling is we need to go with radians for angle measures on
> a rotating devices (to match against anglvel) and leave inclination
> in degrees.

I agree that radians doesn't make sense for inclination, and that it
makes sense to have consistency between the unit of angle and that of
angular velocity. 

However, if ABI consistency is desired, another option would be to
change the unit of angular velocity to degrees per seconds. Then
everything would be the same. But this sounds like a very painful
change...

For now, I'll use radians for the angle. 

Best regards,
David Veenstra

>
> Sorry for the confusion, I'd missed that the units were inconsistent
> which would have made that comment harder to interpret.
>
> Jonathan
>
>> 
>> The in_incli and in_rot_from_* family all use degrees as their unit.
>> For V2 I'll change it back to an angle channel and use angle as its
>> unit.
>> 
>> Best regards,
>> David Veenstra
>> 
>> >
>> > Jonathan
>> >  
>> >> ---
>> >>  drivers/staging/iio/resolver/ad2s1200.c | 11 ++++++++---
>> >>  1 file changed, 8 insertions(+), 3 deletions(-)
>> >> 
>> >> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
>> >> index 4b97a106012c..937676bcc0d4 100644
>> >> --- a/drivers/staging/iio/resolver/ad2s1200.c
>> >> +++ b/drivers/staging/iio/resolver/ad2s1200.c
>> >> @@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>> >>  	switch (m) {
>> >>  	case IIO_CHAN_INFO_SCALE:
>> >>  		switch (chan->type) {
>> >> +		case IIO_INCLI:
>> >> +			*val = 360;
>> >> +			*val2 = 0xFFF;
>> >> +			return IIO_VAL_FRACTIONAL;
>> >>  		case IIO_ANGL_VEL:
>> >>  			/*
>> >>  			 * 2 * Pi is almost equal to
>> >> @@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>> >>  		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
>> >>  		udelay(1);
>> >>  		gpiod_set_value(st->sample, 1);
>> >> -		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
>> >> +		gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI));
>> >>  
>> >>  		ret = spi_read(st->sdev, st->rx, 2);
>> >>  		if (ret < 0) {
>> >> @@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>> >>  
>> >>  		vel = be16_to_cpup((__be16 *)st->rx);
>> >>  		switch (chan->type) {
>> >> -		case IIO_ANGL:
>> >> +		case IIO_INCLI:
>> >>  			*val = vel >> 4;
>> >>  			ret = IIO_VAL_INT;
>> >>  			break;
>> >> @@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>> >>  
>> >>  static const struct iio_chan_spec ad2s1200_channels[] = {
>> >>  	{
>> >> -		.type = IIO_ANGL,
>> >> +		.type = IIO_INCLI,
>> >>  		.indexed = 1,
>> >>  		.channel = 0,
>> >>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> >> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>> >>  	}, {
>> >>  		.type = IIO_ANGL_VEL,
>> >>  		.indexed = 1,  
>> 
>> --
>> 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

Re: [PATCH 10/11] staging: iio: ad2s1200: Replace angle channel with inclination channel

[Jonathan Cameron]

On Sat, 24 Mar 2018 15:57:19 +0100
David Julian Veenstra <davidjulianveenstra@gmail.com> wrote:

> On 24, March 2018 14:12, Jonathan Cameron wrote:
> 
> > On Sat, 24 Mar 2018 13:36:44 +0100
> > David Julian Veenstra <davidjulianveenstra@gmail.com> wrote:
> >  
> >> On 23, March 2018 14:27, Jonathan Cameron wrote:
> >>   
> >> > On Sun, 18 Mar 2018 14:37:04 +0100
> >> > David Veenstra <davidjulianveenstra@gmail.com> wrote:
> >> >    
> >> >> The angle channel is not defined in sysfs iio ABI. So it is replaced
> >> >> with an inclination channel, because it is defined in the ABI, and has the
> >> >> semantics of an angle.
> >> >> 
> >> >> In addition, a fractional scaling factor of 360 / (2^12 -1) is added,
> >> >> to scale the 12-bits angular position to degrees, conform the ABI.
> >> >> 
> >> >> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>    
> >> > No, please do not blugeon a device into the existing documented ABI.
> >> >
> >> > A resolver does not measure something that can be remotely
> >> > sensibly mapped to inclination.  Inclination is relative to 'down'.
> >> > A resolver doesn't care about down.
> >> >
> >> > Add an angle type to the ABI docs. It simply hasn't been documented
> >> > before because there were no drivers outside staging that use it.    
> >> 
> >> I'm sorry, I misunderstood our previous discussing on this topic
> >> (https://marc.info/?l=linux-iio&m=152087432322446&w=2) as saying:
> >> "there already exists another channel type that is a good enough match".  
> > Ah, no I was making the point we need to match with the 'units' not
> > use the channel type. 
> >
> > Hmm. We have an an unfortunate inconsistency between use of radians/sec
> > and degrees for different types.
> >
> > Radians feels perfectly sensible for a rotary device, but not so much
> > for an angle of tilt.
> >
> > I'm not sure how we resolve this cleanly or if we can.
> > My gut feeling is we need to go with radians for angle measures on
> > a rotating devices (to match against anglvel) and leave inclination
> > in degrees.  
> 
> I agree that radians doesn't make sense for inclination, and that it
> makes sense to have consistency between the unit of angle and that of
> angular velocity. 
> 
> However, if ABI consistency is desired, another option would be to
> change the unit of angular velocity to degrees per seconds. Then
> everything would be the same. But this sounds like a very painful
> change...
It's effectively impossible without ABI breakage and getting shouted
at by users (and potentially Linus forcing a revert).

The only way we could do it would be to introduce a new 'similarly'
named type and deprecate the old one, removing it some years in the
future.

Unfortunately we (where I meant I :() mess up sometimes and have
to live with the result.

Jonathan
> 
> For now, I'll use radians for the angle. 
> 
> Best regards,
> David Veenstra
> 
> >
> > Sorry for the confusion, I'd missed that the units were inconsistent
> > which would have made that comment harder to interpret.
> >
> > Jonathan
> >  
> >> 
> >> The in_incli and in_rot_from_* family all use degrees as their unit.
> >> For V2 I'll change it back to an angle channel and use angle as its
> >> unit.
> >> 
> >> Best regards,
> >> David Veenstra
> >>   
> >> >
> >> > Jonathan
> >> >    
> >> >> ---
> >> >>  drivers/staging/iio/resolver/ad2s1200.c | 11 ++++++++---
> >> >>  1 file changed, 8 insertions(+), 3 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> >> >> index 4b97a106012c..937676bcc0d4 100644
> >> >> --- a/drivers/staging/iio/resolver/ad2s1200.c
> >> >> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> >> >> @@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> >> >>  	switch (m) {
> >> >>  	case IIO_CHAN_INFO_SCALE:
> >> >>  		switch (chan->type) {
> >> >> +		case IIO_INCLI:
> >> >> +			*val = 360;
> >> >> +			*val2 = 0xFFF;
> >> >> +			return IIO_VAL_FRACTIONAL;
> >> >>  		case IIO_ANGL_VEL:
> >> >>  			/*
> >> >>  			 * 2 * Pi is almost equal to
> >> >> @@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> >> >>  		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
> >> >>  		udelay(1);
> >> >>  		gpiod_set_value(st->sample, 1);
> >> >> -		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
> >> >> +		gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI));
> >> >>  
> >> >>  		ret = spi_read(st->sdev, st->rx, 2);
> >> >>  		if (ret < 0) {
> >> >> @@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> >> >>  
> >> >>  		vel = be16_to_cpup((__be16 *)st->rx);
> >> >>  		switch (chan->type) {
> >> >> -		case IIO_ANGL:
> >> >> +		case IIO_INCLI:
> >> >>  			*val = vel >> 4;
> >> >>  			ret = IIO_VAL_INT;
> >> >>  			break;
> >> >> @@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> >> >>  
> >> >>  static const struct iio_chan_spec ad2s1200_channels[] = {
> >> >>  	{
> >> >> -		.type = IIO_ANGL,
> >> >> +		.type = IIO_INCLI,
> >> >>  		.indexed = 1,
> >> >>  		.channel = 0,
> >> >>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> >> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >> >>  	}, {
> >> >>  		.type = IIO_ANGL_VEL,
> >> >>  		.indexed = 1,    
> >> 
> >> --
> >> 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  
> --
> 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