[PATCH v3 0/9] staging: iio: ad2s1200: Driver clean up

[PATCH v3 0/9] staging: iio: ad2s1200: Driver clean up

[David Veenstra]

For v1 see [1], for v2 see [2].

A summary of this patch:
1. Clean up of minor code style issues.
2. Replace legacy GPIO ABI with modern ABI and remove usage of.
   platform data.
4. Add scaling factor for angular position and angular velocity to
   match the sysfs IIO ABI.
5. Add documentation for device tree binding, and angle channel
6. Move driver to main line.

Changes in v3:
- Improved documentation about used mutex lock.
- Improved readability of read_raw.
- Remove usage of platform data.
- Add dt binding documentation to
  Documentation/devicetree/bindings/iio/ instead of staging folder.
- Change return type of scale for angle and angular velocity channel.
- Remove axis modifier from documentation of angle channel.
- Add missing references to ad2s1205.

Best regards,
David Veenstra

[1] https://marc.info/?l=linux-iio&m=152137920426820&w=2
[2] https://marc.info/?l=linux-iio&m=152425250915148&w=2

David Veenstra (9):
  staging: iio: ad2s1200: Add kernel docs to driver state
  staging: iio: ad2s1200: Improve readability with be16_to_cpup
  staging: iio: ad2s1200: Replace legacy gpio API with modern API
  staging: iio: ad2s1200: Replace platform data with dt bindings
  staging: iio: ad2s1200: Add documentation for device tree binding
  staging: iio: ad2s1200: Add scaling factor for angular velocity
    channel
  staging: iio: Documentation: Add missing sysfs docs for angle channel
  staging: iio: ad2s1200: Add scaling factor for angle channel
  staging: iio: ad2s1200: Move driver out of staging

 Documentation/ABI/testing/sysfs-bus-iio            |   9 +
 .../devicetree/bindings/iio/resolver/ad2s1200.txt  |  16 ++
 drivers/iio/Kconfig                                |   1 +
 drivers/iio/Makefile                               |   1 +
 drivers/iio/resolver/Kconfig                       |  17 ++
 drivers/iio/resolver/Makefile                      |   5 +
 drivers/iio/resolver/ad2s1200.c                    | 201 +++++++++++++++++++++
 drivers/staging/iio/resolver/Kconfig               |  12 --
 drivers/staging/iio/resolver/Makefile              |   1 -
 drivers/staging/iio/resolver/ad2s1200.c            | 171 ------------------
 10 files changed, 250 insertions(+), 184 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
 create mode 100644 drivers/iio/resolver/Kconfig
 create mode 100644 drivers/iio/resolver/Makefile
 create mode 100644 drivers/iio/resolver/ad2s1200.c
 delete mode 100644 drivers/staging/iio/resolver/ad2s1200.c

-- 
2.16.2

[PATCH v3 0/9] staging: iio: ad2s1200: Driver clean up

[David Veenstra]

For v1 see [1], for v2 see [2].

A summary of this patch:
1. Clean up of minor code style issues.
2. Replace legacy GPIO ABI with modern ABI and remove usage of.
   platform data.
4. Add scaling factor for angular position and angular velocity to
   match the sysfs IIO ABI.
5. Add documentation for device tree binding, and angle channel
6. Move driver to main line.

Changes in v3:
- Improved documentation about used mutex lock.
- Improved readability of read_raw.
- Remove usage of platform data.
- Add dt binding documentation to
  Documentation/devicetree/bindings/iio/ instead of staging folder.
- Change return type of scale for angle and angular velocity channel.
- Remove axis modifier from documentation of angle channel.
- Add missing references to ad2s1205.

Best regards,
David Veenstra

[1] https://marc.info/?l=linux-iio&m=152137920426820&w=2
[2] https://marc.info/?l=linux-iio&m=152425250915148&w=2

David Veenstra (9):
  staging: iio: ad2s1200: Add kernel docs to driver state
  staging: iio: ad2s1200: Improve readability with be16_to_cpup
  staging: iio: ad2s1200: Replace legacy gpio API with modern API
  staging: iio: ad2s1200: Replace platform data with dt bindings
  staging: iio: ad2s1200: Add documentation for device tree binding
  staging: iio: ad2s1200: Add scaling factor for angular velocity
    channel
  staging: iio: Documentation: Add missing sysfs docs for angle channel
  staging: iio: ad2s1200: Add scaling factor for angle channel
  staging: iio: ad2s1200: Move driver out of staging

 Documentation/ABI/testing/sysfs-bus-iio            |   9 +
 .../devicetree/bindings/iio/resolver/ad2s1200.txt  |  16 ++
 drivers/iio/Kconfig                                |   1 +
 drivers/iio/Makefile                               |   1 +
 drivers/iio/resolver/Kconfig                       |  17 ++
 drivers/iio/resolver/Makefile                      |   5 +
 drivers/iio/resolver/ad2s1200.c                    | 201 +++++++++++++++++++++
 drivers/staging/iio/resolver/Kconfig               |  12 --
 drivers/staging/iio/resolver/Makefile              |   1 -
 drivers/staging/iio/resolver/ad2s1200.c            | 171 ------------------
 10 files changed, 250 insertions(+), 184 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
 create mode 100644 drivers/iio/resolver/Kconfig
 create mode 100644 drivers/iio/resolver/Makefile
 create mode 100644 drivers/iio/resolver/ad2s1200.c
 delete mode 100644 drivers/staging/iio/resolver/ad2s1200.c

-- 
2.16.2

[PATCH v3 1/9] 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>
---
Changes in v3:
 - Added more explanation to mutex lock.

 drivers/staging/iio/resolver/ad2s1200.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 357fe3c382b3..17d65cb437fd 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -33,6 +33,15 @@
 /* clock period in nano second */
 #define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
 
+/**
+ * struct ad2s1200_state - driver instance specific data.
+ * @lock:	protects both the GPIO pins and the rx buffer, to prevent
+ *		concurrent spi transactions.
+ * @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 v3 1/9] 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>
---
Changes in v3:
 - Added more explanation to mutex lock.

 drivers/staging/iio/resolver/ad2s1200.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 357fe3c382b3..17d65cb437fd 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -33,6 +33,15 @@
 /* clock period in nano second */
 #define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
 
+/**
+ * struct ad2s1200_state - driver instance specific data.
+ * @lock:	protects both the GPIO pins and the rx buffer, to prevent
+ *		concurrent spi transactions.
+ * @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 v3 2/9] 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>
---
Changes in v3:
 - change type of rx to __be16.
 - remove unneeded variable vel.
 - remove unneeded type cast to s16 (patch line 79).

 drivers/staging/iio/resolver/ad2s1200.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 17d65cb437fd..998f458dc1bd 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -47,7 +47,7 @@ struct ad2s1200_state {
 	struct spi_device *sdev;
 	int sample;
 	int rdvel;
-	u8 rx[2] ____cacheline_aligned;
+	__be16 rx ____cacheline_aligned;
 };
 
 static int ad2s1200_read_raw(struct iio_dev *indio_dev,
@@ -58,7 +58,6 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 {
 	struct ad2s1200_state *st = iio_priv(indio_dev);
 	int ret = 0;
-	s16 vel;
 
 	mutex_lock(&st->lock);
 	gpio_set_value(st->sample, 0);
@@ -68,7 +67,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	gpio_set_value(st->sample, 1);
 	gpio_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
 
-	ret = spi_read(st->sdev, st->rx, 2);
+	ret = spi_read(st->sdev, &st->rx, 2);
 	if (ret < 0) {
 		mutex_unlock(&st->lock);
 		return ret;
@@ -76,12 +75,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 	switch (chan->type) {
 	case IIO_ANGL:
-		*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
+		*val = be16_to_cpup(&st->rx) >> 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(be16_to_cpup(&st->rx) >> 4, 11);
 		break;
 	default:
 		mutex_unlock(&st->lock);
-- 
2.16.2

[PATCH v3 2/9] 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>
---
Changes in v3:
 - change type of rx to __be16.
 - remove unneeded variable vel.
 - remove unneeded type cast to s16 (patch line 79).

 drivers/staging/iio/resolver/ad2s1200.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 17d65cb437fd..998f458dc1bd 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -47,7 +47,7 @@ struct ad2s1200_state {
 	struct spi_device *sdev;
 	int sample;
 	int rdvel;
-	u8 rx[2] ____cacheline_aligned;
+	__be16 rx ____cacheline_aligned;
 };
 
 static int ad2s1200_read_raw(struct iio_dev *indio_dev,
@@ -58,7 +58,6 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 {
 	struct ad2s1200_state *st = iio_priv(indio_dev);
 	int ret = 0;
-	s16 vel;
 
 	mutex_lock(&st->lock);
 	gpio_set_value(st->sample, 0);
@@ -68,7 +67,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	gpio_set_value(st->sample, 1);
 	gpio_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
 
-	ret = spi_read(st->sdev, st->rx, 2);
+	ret = spi_read(st->sdev, &st->rx, 2);
 	if (ret < 0) {
 		mutex_unlock(&st->lock);
 		return ret;
@@ -76,12 +75,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 	switch (chan->type) {
 	case IIO_ANGL:
-		*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
+		*val = be16_to_cpup(&st->rx) >> 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(be16_to_cpup(&st->rx) >> 4, 11);
 		break;
 	default:
 		mutex_unlock(&st->lock);
-- 
2.16.2

[PATCH v3 3/9] staging: iio: ad2s1200: Replace legacy gpio API with modern API

[David Veenstra]

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

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - This patch no longer deals with dt bindings. See the next
   patch.

 drivers/staging/iio/resolver/ad2s1200.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 998f458dc1bd..e7bfe5aa4c45 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/spi/spi.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;
 	__be16 rx ____cacheline_aligned;
 };
 
@@ -60,12 +61,12 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	int ret = 0;
 
 	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) {
@@ -135,8 +136,8 @@ 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];
+	st->sample = gpio_to_desc(pins[0]);
+	st->rdvel = gpio_to_desc(pins[1]);
 
 	indio_dev->dev.parent = &spi->dev;
 	indio_dev->info = &ad2s1200_info;
-- 
2.16.2

[PATCH v3 3/9] staging: iio: ad2s1200: Replace legacy gpio API with modern API

[David Veenstra]

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

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - This patch no longer deals with dt bindings. See the next
   patch.

 drivers/staging/iio/resolver/ad2s1200.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 998f458dc1bd..e7bfe5aa4c45 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/spi/spi.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;
 	__be16 rx ____cacheline_aligned;
 };
 
@@ -60,12 +61,12 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	int ret = 0;
 
 	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) {
@@ -135,8 +136,8 @@ 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];
+	st->sample = gpio_to_desc(pins[0]);
+	st->rdvel = gpio_to_desc(pins[1]);
 
 	indio_dev->dev.parent = &spi->dev;
 	indio_dev->info = &ad2s1200_info;
-- 
2.16.2

[PATCH v3 4/9] staging: iio: ad2s1200: Replace platform data with dt bindings

[David Veenstra]

Remove usage of platform data, and replace it with device tree
facilities.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - Introduced in this version.

 drivers/staging/iio/resolver/ad2s1200.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index e7bfe5aa4c45..3c7163610ff6 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -26,9 +26,6 @@
 
 #define DRV_NAME "ad2s1200"
 
-/* input pin sample and rdvel is controlled by driver */
-#define AD2S1200_PN	2
-
 /* input clock on serial interface */
 #define AD2S1200_HZ	8192000
 /* clock period in nano second */
@@ -113,20 +110,9 @@ 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;
-
-	for (pn = 0; pn < AD2S1200_PN; pn++) {
-		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
-					    DRV_NAME);
-		if (ret) {
-			dev_err(&spi->dev, "request gpio pin %d failed\n",
-				pins[pn]);
-			return ret;
-		}
-	}
+	int ret = 0;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)
@@ -136,8 +122,20 @@ static int ad2s1200_probe(struct spi_device *spi)
 	st = iio_priv(indio_dev);
 	mutex_init(&st->lock);
 	st->sdev = spi;
-	st->sample = gpio_to_desc(pins[0]);
-	st->rdvel = gpio_to_desc(pins[1]);
+
+	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
+	if (IS_ERR(st->sample)) {
+		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
+			PTR_ERR(st->sample));
+		return PTR_ERR(st->sample);
+	}
+
+	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
+	if (IS_ERR(st->rdvel)) {
+		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
+			PTR_ERR(st->rdvel));
+		return PTR_ERR(st->rdvel);
+	}
 
 	indio_dev->dev.parent = &spi->dev;
 	indio_dev->info = &ad2s1200_info;
-- 
2.16.2

[PATCH v3 4/9] staging: iio: ad2s1200: Replace platform data with dt bindings

[David Veenstra]

Remove usage of platform data, and replace it with device tree
facilities.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - Introduced in this version.

 drivers/staging/iio/resolver/ad2s1200.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index e7bfe5aa4c45..3c7163610ff6 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -26,9 +26,6 @@
 
 #define DRV_NAME "ad2s1200"
 
-/* input pin sample and rdvel is controlled by driver */
-#define AD2S1200_PN	2
-
 /* input clock on serial interface */
 #define AD2S1200_HZ	8192000
 /* clock period in nano second */
@@ -113,20 +110,9 @@ 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;
-
-	for (pn = 0; pn < AD2S1200_PN; pn++) {
-		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
-					    DRV_NAME);
-		if (ret) {
-			dev_err(&spi->dev, "request gpio pin %d failed\n",
-				pins[pn]);
-			return ret;
-		}
-	}
+	int ret = 0;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)
@@ -136,8 +122,20 @@ static int ad2s1200_probe(struct spi_device *spi)
 	st = iio_priv(indio_dev);
 	mutex_init(&st->lock);
 	st->sdev = spi;
-	st->sample = gpio_to_desc(pins[0]);
-	st->rdvel = gpio_to_desc(pins[1]);
+
+	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
+	if (IS_ERR(st->sample)) {
+		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
+			PTR_ERR(st->sample));
+		return PTR_ERR(st->sample);
+	}
+
+	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
+	if (IS_ERR(st->rdvel)) {
+		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
+			PTR_ERR(st->rdvel));
+		return PTR_ERR(st->rdvel);
+	}
 
 	indio_dev->dev.parent = &spi->dev;
 	indio_dev->info = &ad2s1200_info;
-- 
2.16.2

[PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding

[David Veenstra]

Add documentation for the added device tree bindings.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - Documentation is added to Documentation/devicetree/bindings/iio/resolver
   instead of staging directory.
 - Add mention to ad2s1205 device.

 .../devicetree/bindings/iio/resolver/ad2s1200.txt        | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt

diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
new file mode 100644
index 000000000000..46b51db38368
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
@@ -0,0 +1,16 @@
+Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter
+
+Required properties:
+ - compatible : should be "adi,ad2s1200" or "adi,ad2s1205"
+ - reg : the SPI chip select number of the device
+ - sample-gpios : The GPIO pin connected to the SAMPLE line of the AD2S1200
+ - rdvel-gpios : The GPIO pin connected to the RDVEL line of the AD2S1200
+
+Example:
+
+	resolver {
+		compatible = "adi,ad2s1200";
+		reg = <4>;
+		sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
+		rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
+	};
-- 
2.16.2

[PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding

[David Veenstra]

Add documentation for the added device tree bindings.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - Documentation is added to Documentation/devicetree/bindings/iio/resolver
   instead of staging directory.
 - Add mention to ad2s1205 device.

 .../devicetree/bindings/iio/resolver/ad2s1200.txt        | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt

diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
new file mode 100644
index 000000000000..46b51db38368
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
@@ -0,0 +1,16 @@
+Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter
+
+Required properties:
+ - compatible : should be "adi,ad2s1200" or "adi,ad2s1205"
+ - reg : the SPI chip select number of the device
+ - sample-gpios : The GPIO pin connected to the SAMPLE line of the AD2S1200
+ - rdvel-gpios : The GPIO pin connected to the RDVEL line of the AD2S1200
+
+Example:
+
+	resolver {
+		compatible = "adi,ad2s1200";
+		reg = <4>;
+		sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
+		rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
+	};
-- 
2.16.2

[PATCH v3 6/9] staging: iio: ad2s1200: Add scaling factor for angular velocity channel

[David Veenstra]

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

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - A decimal approximation of the scale is used, instead
   of a fractional approximation.
 - Remove unneeded explanation of iio_convert_raw_to_processed.

 drivers/staging/iio/resolver/ad2s1200.c | 71 +++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 3c7163610ff6..3e1696e52c38 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -57,37 +57,55 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	struct ad2s1200_state *st = iio_priv(indio_dev);
 	int ret = 0;
 
-	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) {
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ANGL_VEL:
+			/* 2 * Pi ~= 6.283185 */
+			*val = 6;
+			*val2 = 283185;
+			return IIO_VAL_INT_PLUS_MICRO;
+		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;
+		}
+
+		switch (chan->type) {
+		case IIO_ANGL:
+			*val = be16_to_cpup(&st->rx) >> 4;
+			break;
+		case IIO_ANGL_VEL:
+			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
+			break;
+		default:
+			mutex_unlock(&st->lock);
+			return -EINVAL;
+		}
+
+		/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
+		udelay(1);
 		mutex_unlock(&st->lock);
-		return ret;
-	}
 
-	switch (chan->type) {
-	case IIO_ANGL:
-		*val = be16_to_cpup(&st->rx) >> 4;
-		break;
-	case IIO_ANGL_VEL:
-		*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
-		break;
+		return IIO_VAL_INT;
 	default:
-		mutex_unlock(&st->lock);
-		return -EINVAL;
+		break;
 	}
 
-	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
-	udelay(1);
-	mutex_unlock(&st->lock);
-
-	return IIO_VAL_INT;
+	return -EINVAL;
 }
 
 static const struct iio_chan_spec ad2s1200_channels[] = {
@@ -101,6 +119,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 v3 6/9] staging: iio: ad2s1200: Add scaling factor for angular velocity channel

[David Veenstra]

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

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - A decimal approximation of the scale is used, instead
   of a fractional approximation.
 - Remove unneeded explanation of iio_convert_raw_to_processed.

 drivers/staging/iio/resolver/ad2s1200.c | 71 +++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 3c7163610ff6..3e1696e52c38 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -57,37 +57,55 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	struct ad2s1200_state *st = iio_priv(indio_dev);
 	int ret = 0;
 
-	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) {
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ANGL_VEL:
+			/* 2 * Pi ~= 6.283185 */
+			*val = 6;
+			*val2 = 283185;
+			return IIO_VAL_INT_PLUS_MICRO;
+		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;
+		}
+
+		switch (chan->type) {
+		case IIO_ANGL:
+			*val = be16_to_cpup(&st->rx) >> 4;
+			break;
+		case IIO_ANGL_VEL:
+			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
+			break;
+		default:
+			mutex_unlock(&st->lock);
+			return -EINVAL;
+		}
+
+		/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
+		udelay(1);
 		mutex_unlock(&st->lock);
-		return ret;
-	}
 
-	switch (chan->type) {
-	case IIO_ANGL:
-		*val = be16_to_cpup(&st->rx) >> 4;
-		break;
-	case IIO_ANGL_VEL:
-		*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
-		break;
+		return IIO_VAL_INT;
 	default:
-		mutex_unlock(&st->lock);
-		return -EINVAL;
+		break;
 	}
 
-	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
-	udelay(1);
-	mutex_unlock(&st->lock);
-
-	return IIO_VAL_INT;
+	return -EINVAL;
 }
 
 static const struct iio_chan_spec ad2s1200_channels[] = {
@@ -101,6 +119,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 v3 7/9] staging: iio: Documentation: Add missing sysfs docs for angle channel

[David Veenstra]

The iio resolver drivers in staging use angle channels. This patch
add missing documentation for this type of channel.

As was discussed in [1], radians is chosen as the unit, to match the
unit of angular velocity.

[1] https://marc.info/?l=linux-driver-devel&m=152190078308330&w=2

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - Remove axis modifier on in_angle.

 Documentation/ABI/testing/sysfs-bus-iio | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 6a5f34b4d5b9..240287e2343a 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -190,6 +190,13 @@ Description:
 		but should match other such assignments on device).
 		Units after application of scale and offset are m/s^2.
 
+What:		/sys/bus/iio/devices/iio:deviceX/in_angl_raw
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Angle of rotation. Units after application of scale and offset
+		are radians.
+
 What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_x_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_y_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_z_raw
@@ -297,6 +304,7 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_pressure_offset
 What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_offset
 What:		/sys/bus/iio/devices/iio:deviceX/in_magn_offset
 What:		/sys/bus/iio/devices/iio:deviceX/in_rot_offset
+What:		/sys/bus/iio/devices/iio:deviceX/in_angl_offset
 KernelVersion:	2.6.35
 Contact:	linux-iio@vger.kernel.org
 Description:
@@ -350,6 +358,7 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_velocity_sqrt(x^2+y^2+z^2)_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_illuminance_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_countY_scale
+What:		/sys/bus/iio/devices/iio:deviceX/in_angl_scale
 KernelVersion:	2.6.35
 Contact:	linux-iio@vger.kernel.org
 Description:
-- 
2.16.2

[PATCH v3 7/9] staging: iio: Documentation: Add missing sysfs docs for angle channel

[David Veenstra]

The iio resolver drivers in staging use angle channels. This patch
add missing documentation for this type of channel.

As was discussed in [1], radians is chosen as the unit, to match the
unit of angular velocity.

[1] https://marc.info/?l=linux-driver-devel&m=152190078308330&w=2

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - Remove axis modifier on in_angle.

 Documentation/ABI/testing/sysfs-bus-iio | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 6a5f34b4d5b9..240287e2343a 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -190,6 +190,13 @@ Description:
 		but should match other such assignments on device).
 		Units after application of scale and offset are m/s^2.
 
+What:		/sys/bus/iio/devices/iio:deviceX/in_angl_raw
+KernelVersion:	4.17
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Angle of rotation. Units after application of scale and offset
+		are radians.
+
 What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_x_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_y_raw
 What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_z_raw
@@ -297,6 +304,7 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_pressure_offset
 What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_offset
 What:		/sys/bus/iio/devices/iio:deviceX/in_magn_offset
 What:		/sys/bus/iio/devices/iio:deviceX/in_rot_offset
+What:		/sys/bus/iio/devices/iio:deviceX/in_angl_offset
 KernelVersion:	2.6.35
 Contact:	linux-iio@vger.kernel.org
 Description:
@@ -350,6 +358,7 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_velocity_sqrt(x^2+y^2+z^2)_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_illuminance_scale
 What:		/sys/bus/iio/devices/iio:deviceX/in_countY_scale
+What:		/sys/bus/iio/devices/iio:deviceX/in_angl_scale
 KernelVersion:	2.6.35
 Contact:	linux-iio@vger.kernel.org
 Description:
-- 
2.16.2

[PATCH v3 8/9] staging: iio: ad2s1200: Add scaling factor for angle channel

[David Veenstra]

A scaling factor of approximately 2 * Pi / (2^12 -1) is added,
to scale the 12-bits angular position to radians.

A return type of IIO_VAL_INT_PLUS_NANO is used, so that the scale of
both the angle channel and angular velocity channel has 7 significant
digits.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - A decimal approximation of the scale is used, instead
   of a fractional approximation.
 - Remove unneeded explanation of iio_convert_raw_to_processed.

 drivers/staging/iio/resolver/ad2s1200.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 3e1696e52c38..d2b62308b31d 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -60,6 +60,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
+		case IIO_ANGL:
+			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
+			*val = 0;
+			*val2 = 1534355;
+			return IIO_VAL_INT_PLUS_NANO;
 		case IIO_ANGL_VEL:
 			/* 2 * Pi ~= 6.283185 */
 			*val = 6;
@@ -114,6 +119,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),
 	}, {
 		.type = IIO_ANGL_VEL,
 		.indexed = 1,
-- 
2.16.2

[PATCH v3 8/9] staging: iio: ad2s1200: Add scaling factor for angle channel

[David Veenstra]

A scaling factor of approximately 2 * Pi / (2^12 -1) is added,
to scale the 12-bits angular position to radians.

A return type of IIO_VAL_INT_PLUS_NANO is used, so that the scale of
both the angle channel and angular velocity channel has 7 significant
digits.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - A decimal approximation of the scale is used, instead
   of a fractional approximation.
 - Remove unneeded explanation of iio_convert_raw_to_processed.

 drivers/staging/iio/resolver/ad2s1200.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
index 3e1696e52c38..d2b62308b31d 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -60,6 +60,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
+		case IIO_ANGL:
+			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
+			*val = 0;
+			*val2 = 1534355;
+			return IIO_VAL_INT_PLUS_NANO;
 		case IIO_ANGL_VEL:
 			/* 2 * Pi ~= 6.283185 */
 			*val = 6;
@@ -114,6 +119,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),
 	}, {
 		.type = IIO_ANGL_VEL,
 		.indexed = 1,
-- 
2.16.2

[PATCH v3 9/9] staging: iio: ad2s1200: Move driver out of staging

[David Veenstra]

Move the iio driver for the ad2s1200 and ad2s1205 resolver-to-digital
converter out of staging, into mainline iio subsystems.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - Add mention of ad2s1205 in commit message.

 drivers/iio/Kconfig                     |   1 +
 drivers/iio/Makefile                    |   1 +
 drivers/iio/resolver/Kconfig            |  17 +++
 drivers/iio/resolver/Makefile           |   5 +
 drivers/iio/resolver/ad2s1200.c         | 201 ++++++++++++++++++++++++++++++++
 drivers/staging/iio/resolver/Kconfig    |  12 --
 drivers/staging/iio/resolver/Makefile   |   1 -
 drivers/staging/iio/resolver/ad2s1200.c | 201 --------------------------------
 8 files changed, 225 insertions(+), 214 deletions(-)
 create mode 100644 drivers/iio/resolver/Kconfig
 create mode 100644 drivers/iio/resolver/Makefile
 create mode 100644 drivers/iio/resolver/ad2s1200.c
 delete mode 100644 drivers/staging/iio/resolver/ad2s1200.c

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/iio/resolver/ad2s1200.c b/drivers/iio/resolver/ad2s1200.c
new file mode 100644
index 000000000000..d2b62308b31d
--- /dev/null
+++ b/drivers/iio/resolver/ad2s1200.c
@@ -0,0 +1,201 @@
+/*
+ * ad2s1200.c simple support for the ADI Resolver to Digital Converters:
+ * AD2S1200/1205
+ *
+ * Copyright (c) 2010-2010 Analog Devices Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/bitops.h>
+#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/spi/spi.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define DRV_NAME "ad2s1200"
+
+/* input clock on serial interface */
+#define AD2S1200_HZ	8192000
+/* clock period in nano second */
+#define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
+
+/**
+ * struct ad2s1200_state - driver instance specific data.
+ * @lock:	protects both the GPIO pins and the rx buffer, to prevent
+ *		concurrent spi transactions.
+ * @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;
+	struct gpio_desc *sample;
+	struct gpio_desc *rdvel;
+	__be16 rx ____cacheline_aligned;
+};
+
+static int ad2s1200_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val,
+			     int *val2,
+			     long m)
+{
+	struct ad2s1200_state *st = iio_priv(indio_dev);
+	int ret = 0;
+
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ANGL:
+			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
+			*val = 0;
+			*val2 = 1534355;
+			return IIO_VAL_INT_PLUS_NANO;
+		case IIO_ANGL_VEL:
+			/* 2 * Pi ~= 6.283185 */
+			*val = 6;
+			*val2 = 283185;
+			return IIO_VAL_INT_PLUS_MICRO;
+		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;
+		}
+
+		switch (chan->type) {
+		case IIO_ANGL:
+			*val = be16_to_cpup(&st->rx) >> 4;
+			break;
+		case IIO_ANGL_VEL:
+			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
+			break;
+		default:
+			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;
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_chan_spec ad2s1200_channels[] = {
+	{
+		.type = IIO_ANGL,
+		.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,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+	}
+};
+
+static const struct iio_info ad2s1200_info = {
+	.read_raw = ad2s1200_read_raw,
+};
+
+static int ad2s1200_probe(struct spi_device *spi)
+{
+	struct ad2s1200_state *st;
+	struct iio_dev *indio_dev;
+	int ret = 0;
+
+	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);
+	st->sdev = spi;
+
+	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
+	if (IS_ERR(st->sample)) {
+		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
+			PTR_ERR(st->sample));
+		return PTR_ERR(st->sample);
+	}
+
+	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
+	if (IS_ERR(st->rdvel)) {
+		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
+			PTR_ERR(st->rdvel));
+		return PTR_ERR(st->rdvel);
+	}
+
+	indio_dev->dev.parent = &spi->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);
+	if (ret)
+		return ret;
+
+	spi->max_speed_hz = AD2S1200_HZ;
+	spi->mode = SPI_MODE_3;
+	spi_setup(spi);
+
+	return 0;
+}
+
+static const struct spi_device_id ad2s1200_id[] = {
+	{ "ad2s1200" },
+	{ "ad2s1205" },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ad2s1200_id);
+
+static struct spi_driver ad2s1200_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe = ad2s1200_probe,
+	.id_table = ad2s1200_id,
+};
+module_spi_driver(ad2s1200_driver);
+
+MODULE_AUTHOR("Graff Yang <graff.yang@gmail.com>");
+MODULE_DESCRIPTION("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver");
+MODULE_LICENSE("GPL v2");
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
diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
deleted file mode 100644
index d2b62308b31d..000000000000
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ /dev/null
@@ -1,201 +0,0 @@
-/*
- * ad2s1200.c simple support for the ADI Resolver to Digital Converters:
- * AD2S1200/1205
- *
- * Copyright (c) 2010-2010 Analog Devices Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- */
-
-#include <linux/bitops.h>
-#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/spi/spi.h>
-#include <linux/sysfs.h>
-#include <linux/types.h>
-
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-
-#define DRV_NAME "ad2s1200"
-
-/* input clock on serial interface */
-#define AD2S1200_HZ	8192000
-/* clock period in nano second */
-#define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
-
-/**
- * struct ad2s1200_state - driver instance specific data.
- * @lock:	protects both the GPIO pins and the rx buffer, to prevent
- *		concurrent spi transactions.
- * @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;
-	struct gpio_desc *sample;
-	struct gpio_desc *rdvel;
-	__be16 rx ____cacheline_aligned;
-};
-
-static int ad2s1200_read_raw(struct iio_dev *indio_dev,
-			     struct iio_chan_spec const *chan,
-			     int *val,
-			     int *val2,
-			     long m)
-{
-	struct ad2s1200_state *st = iio_priv(indio_dev);
-	int ret = 0;
-
-	switch (m) {
-	case IIO_CHAN_INFO_SCALE:
-		switch (chan->type) {
-		case IIO_ANGL:
-			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
-			*val = 0;
-			*val2 = 1534355;
-			return IIO_VAL_INT_PLUS_NANO;
-		case IIO_ANGL_VEL:
-			/* 2 * Pi ~= 6.283185 */
-			*val = 6;
-			*val2 = 283185;
-			return IIO_VAL_INT_PLUS_MICRO;
-		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;
-		}
-
-		switch (chan->type) {
-		case IIO_ANGL:
-			*val = be16_to_cpup(&st->rx) >> 4;
-			break;
-		case IIO_ANGL_VEL:
-			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
-			break;
-		default:
-			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;
-	default:
-		break;
-	}
-
-	return -EINVAL;
-}
-
-static const struct iio_chan_spec ad2s1200_channels[] = {
-	{
-		.type = IIO_ANGL,
-		.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,
-		.channel = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
-	}
-};
-
-static const struct iio_info ad2s1200_info = {
-	.read_raw = ad2s1200_read_raw,
-};
-
-static int ad2s1200_probe(struct spi_device *spi)
-{
-	struct ad2s1200_state *st;
-	struct iio_dev *indio_dev;
-	int ret = 0;
-
-	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);
-	st->sdev = spi;
-
-	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
-	if (IS_ERR(st->sample)) {
-		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
-			PTR_ERR(st->sample));
-		return PTR_ERR(st->sample);
-	}
-
-	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
-	if (IS_ERR(st->rdvel)) {
-		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
-			PTR_ERR(st->rdvel));
-		return PTR_ERR(st->rdvel);
-	}
-
-	indio_dev->dev.parent = &spi->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);
-	if (ret)
-		return ret;
-
-	spi->max_speed_hz = AD2S1200_HZ;
-	spi->mode = SPI_MODE_3;
-	spi_setup(spi);
-
-	return 0;
-}
-
-static const struct spi_device_id ad2s1200_id[] = {
-	{ "ad2s1200" },
-	{ "ad2s1205" },
-	{}
-};
-MODULE_DEVICE_TABLE(spi, ad2s1200_id);
-
-static struct spi_driver ad2s1200_driver = {
-	.driver = {
-		.name = DRV_NAME,
-	},
-	.probe = ad2s1200_probe,
-	.id_table = ad2s1200_id,
-};
-module_spi_driver(ad2s1200_driver);
-
-MODULE_AUTHOR("Graff Yang <graff.yang@gmail.com>");
-MODULE_DESCRIPTION("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver");
-MODULE_LICENSE("GPL v2");
-- 
2.16.2

[PATCH v3 9/9] staging: iio: ad2s1200: Move driver out of staging

[David Veenstra]

Move the iio driver for the ad2s1200 and ad2s1205 resolver-to-digital
converter out of staging, into mainline iio subsystems.

Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
---
Changes in v3:
 - Add mention of ad2s1205 in commit message.

 drivers/iio/Kconfig                     |   1 +
 drivers/iio/Makefile                    |   1 +
 drivers/iio/resolver/Kconfig            |  17 +++
 drivers/iio/resolver/Makefile           |   5 +
 drivers/iio/resolver/ad2s1200.c         | 201 ++++++++++++++++++++++++++++++++
 drivers/staging/iio/resolver/Kconfig    |  12 --
 drivers/staging/iio/resolver/Makefile   |   1 -
 drivers/staging/iio/resolver/ad2s1200.c | 201 --------------------------------
 8 files changed, 225 insertions(+), 214 deletions(-)
 create mode 100644 drivers/iio/resolver/Kconfig
 create mode 100644 drivers/iio/resolver/Makefile
 create mode 100644 drivers/iio/resolver/ad2s1200.c
 delete mode 100644 drivers/staging/iio/resolver/ad2s1200.c

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/iio/resolver/ad2s1200.c b/drivers/iio/resolver/ad2s1200.c
new file mode 100644
index 000000000000..d2b62308b31d
--- /dev/null
+++ b/drivers/iio/resolver/ad2s1200.c
@@ -0,0 +1,201 @@
+/*
+ * ad2s1200.c simple support for the ADI Resolver to Digital Converters:
+ * AD2S1200/1205
+ *
+ * Copyright (c) 2010-2010 Analog Devices Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/bitops.h>
+#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/spi/spi.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define DRV_NAME "ad2s1200"
+
+/* input clock on serial interface */
+#define AD2S1200_HZ	8192000
+/* clock period in nano second */
+#define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
+
+/**
+ * struct ad2s1200_state - driver instance specific data.
+ * @lock:	protects both the GPIO pins and the rx buffer, to prevent
+ *		concurrent spi transactions.
+ * @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;
+	struct gpio_desc *sample;
+	struct gpio_desc *rdvel;
+	__be16 rx ____cacheline_aligned;
+};
+
+static int ad2s1200_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val,
+			     int *val2,
+			     long m)
+{
+	struct ad2s1200_state *st = iio_priv(indio_dev);
+	int ret = 0;
+
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ANGL:
+			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
+			*val = 0;
+			*val2 = 1534355;
+			return IIO_VAL_INT_PLUS_NANO;
+		case IIO_ANGL_VEL:
+			/* 2 * Pi ~= 6.283185 */
+			*val = 6;
+			*val2 = 283185;
+			return IIO_VAL_INT_PLUS_MICRO;
+		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;
+		}
+
+		switch (chan->type) {
+		case IIO_ANGL:
+			*val = be16_to_cpup(&st->rx) >> 4;
+			break;
+		case IIO_ANGL_VEL:
+			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
+			break;
+		default:
+			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;
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_chan_spec ad2s1200_channels[] = {
+	{
+		.type = IIO_ANGL,
+		.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,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+	}
+};
+
+static const struct iio_info ad2s1200_info = {
+	.read_raw = ad2s1200_read_raw,
+};
+
+static int ad2s1200_probe(struct spi_device *spi)
+{
+	struct ad2s1200_state *st;
+	struct iio_dev *indio_dev;
+	int ret = 0;
+
+	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);
+	st->sdev = spi;
+
+	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
+	if (IS_ERR(st->sample)) {
+		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
+			PTR_ERR(st->sample));
+		return PTR_ERR(st->sample);
+	}
+
+	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
+	if (IS_ERR(st->rdvel)) {
+		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
+			PTR_ERR(st->rdvel));
+		return PTR_ERR(st->rdvel);
+	}
+
+	indio_dev->dev.parent = &spi->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);
+	if (ret)
+		return ret;
+
+	spi->max_speed_hz = AD2S1200_HZ;
+	spi->mode = SPI_MODE_3;
+	spi_setup(spi);
+
+	return 0;
+}
+
+static const struct spi_device_id ad2s1200_id[] = {
+	{ "ad2s1200" },
+	{ "ad2s1205" },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ad2s1200_id);
+
+static struct spi_driver ad2s1200_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe = ad2s1200_probe,
+	.id_table = ad2s1200_id,
+};
+module_spi_driver(ad2s1200_driver);
+
+MODULE_AUTHOR("Graff Yang <graff.yang@gmail.com>");
+MODULE_DESCRIPTION("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver");
+MODULE_LICENSE("GPL v2");
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
diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
deleted file mode 100644
index d2b62308b31d..000000000000
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ /dev/null
@@ -1,201 +0,0 @@
-/*
- * ad2s1200.c simple support for the ADI Resolver to Digital Converters:
- * AD2S1200/1205
- *
- * Copyright (c) 2010-2010 Analog Devices Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- */
-
-#include <linux/bitops.h>
-#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/spi/spi.h>
-#include <linux/sysfs.h>
-#include <linux/types.h>
-
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-
-#define DRV_NAME "ad2s1200"
-
-/* input clock on serial interface */
-#define AD2S1200_HZ	8192000
-/* clock period in nano second */
-#define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
-
-/**
- * struct ad2s1200_state - driver instance specific data.
- * @lock:	protects both the GPIO pins and the rx buffer, to prevent
- *		concurrent spi transactions.
- * @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;
-	struct gpio_desc *sample;
-	struct gpio_desc *rdvel;
-	__be16 rx ____cacheline_aligned;
-};
-
-static int ad2s1200_read_raw(struct iio_dev *indio_dev,
-			     struct iio_chan_spec const *chan,
-			     int *val,
-			     int *val2,
-			     long m)
-{
-	struct ad2s1200_state *st = iio_priv(indio_dev);
-	int ret = 0;
-
-	switch (m) {
-	case IIO_CHAN_INFO_SCALE:
-		switch (chan->type) {
-		case IIO_ANGL:
-			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
-			*val = 0;
-			*val2 = 1534355;
-			return IIO_VAL_INT_PLUS_NANO;
-		case IIO_ANGL_VEL:
-			/* 2 * Pi ~= 6.283185 */
-			*val = 6;
-			*val2 = 283185;
-			return IIO_VAL_INT_PLUS_MICRO;
-		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;
-		}
-
-		switch (chan->type) {
-		case IIO_ANGL:
-			*val = be16_to_cpup(&st->rx) >> 4;
-			break;
-		case IIO_ANGL_VEL:
-			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
-			break;
-		default:
-			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;
-	default:
-		break;
-	}
-
-	return -EINVAL;
-}
-
-static const struct iio_chan_spec ad2s1200_channels[] = {
-	{
-		.type = IIO_ANGL,
-		.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,
-		.channel = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
-	}
-};
-
-static const struct iio_info ad2s1200_info = {
-	.read_raw = ad2s1200_read_raw,
-};
-
-static int ad2s1200_probe(struct spi_device *spi)
-{
-	struct ad2s1200_state *st;
-	struct iio_dev *indio_dev;
-	int ret = 0;
-
-	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);
-	st->sdev = spi;
-
-	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
-	if (IS_ERR(st->sample)) {
-		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
-			PTR_ERR(st->sample));
-		return PTR_ERR(st->sample);
-	}
-
-	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
-	if (IS_ERR(st->rdvel)) {
-		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
-			PTR_ERR(st->rdvel));
-		return PTR_ERR(st->rdvel);
-	}
-
-	indio_dev->dev.parent = &spi->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);
-	if (ret)
-		return ret;
-
-	spi->max_speed_hz = AD2S1200_HZ;
-	spi->mode = SPI_MODE_3;
-	spi_setup(spi);
-
-	return 0;
-}
-
-static const struct spi_device_id ad2s1200_id[] = {
-	{ "ad2s1200" },
-	{ "ad2s1205" },
-	{}
-};
-MODULE_DEVICE_TABLE(spi, ad2s1200_id);
-
-static struct spi_driver ad2s1200_driver = {
-	.driver = {
-		.name = DRV_NAME,
-	},
-	.probe = ad2s1200_probe,
-	.id_table = ad2s1200_id,
-};
-module_spi_driver(ad2s1200_driver);
-
-MODULE_AUTHOR("Graff Yang <graff.yang@gmail.com>");
-MODULE_DESCRIPTION("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver");
-MODULE_LICENSE("GPL v2");
-- 
2.16.2

Re: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding

[Rob Herring]

On Mon, Apr 23, 2018 at 12:03:47AM +0200, David Veenstra wrote:
> Add documentation for the added device tree bindings.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> ---
> Changes in v3:
>  - Documentation is added to Documentation/devicetree/bindings/iio/resolver
>    instead of staging directory.
>  - Add mention to ad2s1205 device.
> 
>  .../devicetree/bindings/iio/resolver/ad2s1200.txt        | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> new file mode 100644
> index 000000000000..46b51db38368
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> @@ -0,0 +1,16 @@
> +Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter
> +
> +Required properties:
> + - compatible : should be "adi,ad2s1200" or "adi,ad2s1205"
> + - reg : the SPI chip select number of the device
> + - sample-gpios : The GPIO pin connected to the SAMPLE line of the AD2S1200
> + - rdvel-gpios : The GPIO pin connected to the RDVEL line of the AD2S1200

Are these gpios something that would be common to resolvers (whatever 
that is) or specific to this chip. For the latter, you need vendor 
prefixes.

> +
> +Example:
> +
> +	resolver {

Missing unit address.

> +		compatible = "adi,ad2s1200";
> +		reg = <4>;
> +		sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
> +		rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
> +	};
> -- 
> 2.16.2
> 

Re: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding

[Rob Herring]

On Mon, Apr 23, 2018 at 12:03:47AM +0200, David Veenstra wrote:
> Add documentation for the added device tree bindings.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> ---
> Changes in v3:
>  - Documentation is added to Documentation/devicetree/bindings/iio/resolver
>    instead of staging directory.
>  - Add mention to ad2s1205 device.
> 
>  .../devicetree/bindings/iio/resolver/ad2s1200.txt        | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> new file mode 100644
> index 000000000000..46b51db38368
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> @@ -0,0 +1,16 @@
> +Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter
> +
> +Required properties:
> + - compatible : should be "adi,ad2s1200" or "adi,ad2s1205"
> + - reg : the SPI chip select number of the device
> + - sample-gpios : The GPIO pin connected to the SAMPLE line of the AD2S1200
> + - rdvel-gpios : The GPIO pin connected to the RDVEL line of the AD2S1200

Are these gpios something that would be common to resolvers (whatever 
that is) or specific to this chip. For the latter, you need vendor 
prefixes.

> +
> +Example:
> +
> +	resolver {

Missing unit address.

> +		compatible = "adi,ad2s1200";
> +		reg = <4>;
> +		sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
> +		rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
> +	};
> -- 
> 2.16.2
> 

Re: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding

[Rob Herring]

On Mon, Apr 23, 2018 at 12:03:47AM +0200, David Veenstra wrote:
> Add documentation for the added device tree bindings.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> ---
> Changes in v3:
>  - Documentation is added to Documentation/devicetree/bindings/iio/resolver
>    instead of staging directory.

Also, update the subject. This is not part of staging. "dt-bindings: iio: .." 
is the preferred subject prefix.

>  - Add mention to ad2s1205 device.
> 
>  .../devicetree/bindings/iio/resolver/ad2s1200.txt        | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> new file mode 100644
> index 000000000000..46b51db38368
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> @@ -0,0 +1,16 @@
> +Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter
> +
> +Required properties:
> + - compatible : should be "adi,ad2s1200" or "adi,ad2s1205"
> + - reg : the SPI chip select number of the device
> + - sample-gpios : The GPIO pin connected to the SAMPLE line of the AD2S1200
> + - rdvel-gpios : The GPIO pin connected to the RDVEL line of the AD2S1200
> +
> +Example:
> +
> +	resolver {
> +		compatible = "adi,ad2s1200";
> +		reg = <4>;
> +		sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
> +		rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
> +	};
> -- 
> 2.16.2
> 

Re: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding

[Rob Herring]

On Mon, Apr 23, 2018 at 12:03:47AM +0200, David Veenstra wrote:
> Add documentation for the added device tree bindings.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> ---
> Changes in v3:
>  - Documentation is added to Documentation/devicetree/bindings/iio/resolver
>    instead of staging directory.

Also, update the subject. This is not part of staging. "dt-bindings: iio: .." 
is the preferred subject prefix.

>  - Add mention to ad2s1205 device.
> 
>  .../devicetree/bindings/iio/resolver/ad2s1200.txt        | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> new file mode 100644
> index 000000000000..46b51db38368
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> @@ -0,0 +1,16 @@
> +Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter
> +
> +Required properties:
> + - compatible : should be "adi,ad2s1200" or "adi,ad2s1205"
> + - reg : the SPI chip select number of the device
> + - sample-gpios : The GPIO pin connected to the SAMPLE line of the AD2S1200
> + - rdvel-gpios : The GPIO pin connected to the RDVEL line of the AD2S1200
> +
> +Example:
> +
> +	resolver {
> +		compatible = "adi,ad2s1200";
> +		reg = <4>;
> +		sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
> +		rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
> +	};
> -- 
> 2.16.2
> 

Re: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding

[Jonathan Cameron]

On Fri, 27 Apr 2018 09:48:20 -0500
Rob Herring <robh@kernel.org> wrote:

> On Mon, Apr 23, 2018 at 12:03:47AM +0200, David Veenstra wrote:
> > Add documentation for the added device tree bindings.
> > 
> > Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> > ---
> > Changes in v3:
> >  - Documentation is added to Documentation/devicetree/bindings/iio/resolver
> >    instead of staging directory.
> >  - Add mention to ad2s1205 device.
> > 
> >  .../devicetree/bindings/iio/resolver/ad2s1200.txt        | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> > new file mode 100644
> > index 000000000000..46b51db38368
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> > @@ -0,0 +1,16 @@
> > +Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter
> > +
> > +Required properties:
> > + - compatible : should be "adi,ad2s1200" or "adi,ad2s1205"
> > + - reg : the SPI chip select number of the device
> > + - sample-gpios : The GPIO pin connected to the SAMPLE line of the AD2S1200
> > + - rdvel-gpios : The GPIO pin connected to the RDVEL line of the AD2S1200  
> 
> Are these gpios something that would be common to resolvers (whatever 
> that is) or specific to this chip. For the latter, you need vendor 
> prefixes.

They are pretty chip specific I think, so prefixes needed.

I'd not really registered that gpios should be vendor prefixed for some reason
(kind of obvious now you mention it).  We have only a few from a quick grep
of the bindings for IIO devices (there are loads elsewhere that aren't and
are clearly very much part specific).  Ah well, will try and do better moving
forward on this!

Jonathan


> 
> > +
> > +Example:
> > +
> > +	resolver {  
> 
> Missing unit address.
> 
> > +		compatible = "adi,ad2s1200";
> > +		reg = <4>;
> > +		sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
> > +		rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
> > +	};
> > -- 
> > 2.16.2
> >   

Re: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding

[Jonathan Cameron]

On Fri, 27 Apr 2018 09:48:20 -0500
Rob Herring <robh@kernel.org> wrote:

> On Mon, Apr 23, 2018 at 12:03:47AM +0200, David Veenstra wrote:
> > Add documentation for the added device tree bindings.
> > 
> > Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> > ---
> > Changes in v3:
> >  - Documentation is added to Documentation/devicetree/bindings/iio/resolver
> >    instead of staging directory.
> >  - Add mention to ad2s1205 device.
> > 
> >  .../devicetree/bindings/iio/resolver/ad2s1200.txt        | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> > new file mode 100644
> > index 000000000000..46b51db38368
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
> > @@ -0,0 +1,16 @@
> > +Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter
> > +
> > +Required properties:
> > + - compatible : should be "adi,ad2s1200" or "adi,ad2s1205"
> > + - reg : the SPI chip select number of the device
> > + - sample-gpios : The GPIO pin connected to the SAMPLE line of the AD2S1200
> > + - rdvel-gpios : The GPIO pin connected to the RDVEL line of the AD2S1200  
> 
> Are these gpios something that would be common to resolvers (whatever 
> that is) or specific to this chip. For the latter, you need vendor 
> prefixes.

They are pretty chip specific I think, so prefixes needed.

I'd not really registered that gpios should be vendor prefixed for some reason
(kind of obvious now you mention it).  We have only a few from a quick grep
of the bindings for IIO devices (there are loads elsewhere that aren't and
are clearly very much part specific).  Ah well, will try and do better moving
forward on this!

Jonathan


> 
> > +
> > +Example:
> > +
> > +	resolver {  
> 
> Missing unit address.
> 
> > +		compatible = "adi,ad2s1200";
> > +		reg = <4>;
> > +		sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
> > +		rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
> > +	};
> > -- 
> > 2.16.2
> >   

Re: [PATCH v3 1/9] staging: iio: ad2s1200: Add kernel docs to driver state

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:02:51 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> Add missing kernel docs to the ad2s1200 driver state.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Hi David,

Comment inline.

> ---
> Changes in v3:
>  - Added more explanation to mutex lock.
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 357fe3c382b3..17d65cb437fd 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -33,6 +33,15 @@
>  /* clock period in nano second */
>  #define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
>  
> +/**
> + * struct ad2s1200_state - driver instance specific data.
> + * @lock:	protects both the GPIO pins and the rx buffer, to prevent
> + *		concurrent spi transactions.
It isn't actually preventing concurrent spi transactions - that
is done by the spi core itself.  What it is preventing is
concurrent 'actions' with the device - these involve
a mixture of gpio changes and spi transactions.

That would take some considerable explaining and frankly the code
does the job just fine once people know to look.

I'd just leave it as "protects both the GPIO pins and the rx buffer."
Or feel free to see if you can come up with a brief description of
exactly what we need to be 'atomic'.

Thanks,

Jonathan

> + * @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;

Re: [PATCH v3 1/9] staging: iio: ad2s1200: Add kernel docs to driver state

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:02:51 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> Add missing kernel docs to the ad2s1200 driver state.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Hi David,

Comment inline.

> ---
> Changes in v3:
>  - Added more explanation to mutex lock.
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 357fe3c382b3..17d65cb437fd 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -33,6 +33,15 @@
>  /* clock period in nano second */
>  #define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
>  
> +/**
> + * struct ad2s1200_state - driver instance specific data.
> + * @lock:	protects both the GPIO pins and the rx buffer, to prevent
> + *		concurrent spi transactions.
It isn't actually preventing concurrent spi transactions - that
is done by the spi core itself.  What it is preventing is
concurrent 'actions' with the device - these involve
a mixture of gpio changes and spi transactions.

That would take some considerable explaining and frankly the code
does the job just fine once people know to look.

I'd just leave it as "protects both the GPIO pins and the rx buffer."
Or feel free to see if you can come up with a brief description of
exactly what we need to be 'atomic'.

Thanks,

Jonathan

> + * @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;

Re: [PATCH v3 2/9] staging: iio: ad2s1200: Improve readability with be16_to_cpup

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:03:03 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> 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>
Looks good to me.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> ---
> Changes in v3:
>  - change type of rx to __be16.
>  - remove unneeded variable vel.
>  - remove unneeded type cast to s16 (patch line 79).
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 17d65cb437fd..998f458dc1bd 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -47,7 +47,7 @@ struct ad2s1200_state {
>  	struct spi_device *sdev;
>  	int sample;
>  	int rdvel;
> -	u8 rx[2] ____cacheline_aligned;
> +	__be16 rx ____cacheline_aligned;
>  };
>  
>  static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> @@ -58,7 +58,6 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct ad2s1200_state *st = iio_priv(indio_dev);
>  	int ret = 0;
> -	s16 vel;
>  
>  	mutex_lock(&st->lock);
>  	gpio_set_value(st->sample, 0);
> @@ -68,7 +67,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  	gpio_set_value(st->sample, 1);
>  	gpio_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
>  
> -	ret = spi_read(st->sdev, st->rx, 2);
> +	ret = spi_read(st->sdev, &st->rx, 2);
>  	if (ret < 0) {
>  		mutex_unlock(&st->lock);
>  		return ret;
> @@ -76,12 +75,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (chan->type) {
>  	case IIO_ANGL:
> -		*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
> +		*val = be16_to_cpup(&st->rx) >> 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(be16_to_cpup(&st->rx) >> 4, 11);
>  		break;
>  	default:
>  		mutex_unlock(&st->lock);

Re: [PATCH v3 2/9] staging: iio: ad2s1200: Improve readability with be16_to_cpup

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:03:03 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> 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>
Looks good to me.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> ---
> Changes in v3:
>  - change type of rx to __be16.
>  - remove unneeded variable vel.
>  - remove unneeded type cast to s16 (patch line 79).
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 17d65cb437fd..998f458dc1bd 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -47,7 +47,7 @@ struct ad2s1200_state {
>  	struct spi_device *sdev;
>  	int sample;
>  	int rdvel;
> -	u8 rx[2] ____cacheline_aligned;
> +	__be16 rx ____cacheline_aligned;
>  };
>  
>  static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> @@ -58,7 +58,6 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct ad2s1200_state *st = iio_priv(indio_dev);
>  	int ret = 0;
> -	s16 vel;
>  
>  	mutex_lock(&st->lock);
>  	gpio_set_value(st->sample, 0);
> @@ -68,7 +67,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  	gpio_set_value(st->sample, 1);
>  	gpio_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
>  
> -	ret = spi_read(st->sdev, st->rx, 2);
> +	ret = spi_read(st->sdev, &st->rx, 2);
>  	if (ret < 0) {
>  		mutex_unlock(&st->lock);
>  		return ret;
> @@ -76,12 +75,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (chan->type) {
>  	case IIO_ANGL:
> -		*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
> +		*val = be16_to_cpup(&st->rx) >> 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(be16_to_cpup(&st->rx) >> 4, 11);
>  		break;
>  	default:
>  		mutex_unlock(&st->lock);

Re: [PATCH v3 3/9] staging: iio: ad2s1200: Replace legacy gpio API with modern API

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:03:16 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> The legacy, integer based gpio API is replaced with the modern
> descriptor based API.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
This patch is fine, but I'm going to leave it for the next version
as it makes more sense to pick up with the next patch and
that one will change slightly I think due to the requested
change of the gpio names from Rob.

Thanks,

Jonathan

> ---
> Changes in v3:
>  - This patch no longer deals with dt bindings. See the next
>    patch.
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 998f458dc1bd..e7bfe5aa4c45 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/spi/spi.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;
>  	__be16 rx ____cacheline_aligned;
>  };
>  
> @@ -60,12 +61,12 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  	int ret = 0;
>  
>  	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) {
> @@ -135,8 +136,8 @@ 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];
> +	st->sample = gpio_to_desc(pins[0]);
> +	st->rdvel = gpio_to_desc(pins[1]);
>  
>  	indio_dev->dev.parent = &spi->dev;
>  	indio_dev->info = &ad2s1200_info;

Re: [PATCH v3 3/9] staging: iio: ad2s1200: Replace legacy gpio API with modern API

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:03:16 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> The legacy, integer based gpio API is replaced with the modern
> descriptor based API.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
This patch is fine, but I'm going to leave it for the next version
as it makes more sense to pick up with the next patch and
that one will change slightly I think due to the requested
change of the gpio names from Rob.

Thanks,

Jonathan

> ---
> Changes in v3:
>  - This patch no longer deals with dt bindings. See the next
>    patch.
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 998f458dc1bd..e7bfe5aa4c45 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/spi/spi.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;
>  	__be16 rx ____cacheline_aligned;
>  };
>  
> @@ -60,12 +61,12 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  	int ret = 0;
>  
>  	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) {
> @@ -135,8 +136,8 @@ 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];
> +	st->sample = gpio_to_desc(pins[0]);
> +	st->rdvel = gpio_to_desc(pins[1]);
>  
>  	indio_dev->dev.parent = &spi->dev;
>  	indio_dev->info = &ad2s1200_info;

Re: [PATCH v3 4/9] staging: iio: ad2s1200: Replace platform data with dt bindings

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:03:32 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> Remove usage of platform data, and replace it with device tree
> facilities.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
This is fine up to the need to add the prefix to the gpio names.

Thanks,

Jonathan

> ---
> Changes in v3:
>  - Introduced in this version.
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index e7bfe5aa4c45..3c7163610ff6 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -26,9 +26,6 @@
>  
>  #define DRV_NAME "ad2s1200"
>  
> -/* input pin sample and rdvel is controlled by driver */
> -#define AD2S1200_PN	2
> -
>  /* input clock on serial interface */
>  #define AD2S1200_HZ	8192000
>  /* clock period in nano second */
> @@ -113,20 +110,9 @@ 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;
> -
> -	for (pn = 0; pn < AD2S1200_PN; pn++) {
> -		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
> -					    DRV_NAME);
> -		if (ret) {
> -			dev_err(&spi->dev, "request gpio pin %d failed\n",
> -				pins[pn]);
> -			return ret;
> -		}
> -	}
> +	int ret = 0;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>  	if (!indio_dev)
> @@ -136,8 +122,20 @@ static int ad2s1200_probe(struct spi_device *spi)
>  	st = iio_priv(indio_dev);
>  	mutex_init(&st->lock);
>  	st->sdev = spi;
> -	st->sample = gpio_to_desc(pins[0]);
> -	st->rdvel = gpio_to_desc(pins[1]);
> +
> +	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->sample)) {
> +		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
> +			PTR_ERR(st->sample));
> +		return PTR_ERR(st->sample);
> +	}
> +
> +	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->rdvel)) {
> +		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
> +			PTR_ERR(st->rdvel));
> +		return PTR_ERR(st->rdvel);
> +	}
>  
>  	indio_dev->dev.parent = &spi->dev;
>  	indio_dev->info = &ad2s1200_info;

Re: [PATCH v3 4/9] staging: iio: ad2s1200: Replace platform data with dt bindings

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:03:32 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> Remove usage of platform data, and replace it with device tree
> facilities.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
This is fine up to the need to add the prefix to the gpio names.

Thanks,

Jonathan

> ---
> Changes in v3:
>  - Introduced in this version.
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index e7bfe5aa4c45..3c7163610ff6 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -26,9 +26,6 @@
>  
>  #define DRV_NAME "ad2s1200"
>  
> -/* input pin sample and rdvel is controlled by driver */
> -#define AD2S1200_PN	2
> -
>  /* input clock on serial interface */
>  #define AD2S1200_HZ	8192000
>  /* clock period in nano second */
> @@ -113,20 +110,9 @@ 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;
> -
> -	for (pn = 0; pn < AD2S1200_PN; pn++) {
> -		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
> -					    DRV_NAME);
> -		if (ret) {
> -			dev_err(&spi->dev, "request gpio pin %d failed\n",
> -				pins[pn]);
> -			return ret;
> -		}
> -	}
> +	int ret = 0;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>  	if (!indio_dev)
> @@ -136,8 +122,20 @@ static int ad2s1200_probe(struct spi_device *spi)
>  	st = iio_priv(indio_dev);
>  	mutex_init(&st->lock);
>  	st->sdev = spi;
> -	st->sample = gpio_to_desc(pins[0]);
> -	st->rdvel = gpio_to_desc(pins[1]);
> +
> +	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->sample)) {
> +		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
> +			PTR_ERR(st->sample));
> +		return PTR_ERR(st->sample);
> +	}
> +
> +	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->rdvel)) {
> +		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
> +			PTR_ERR(st->rdvel));
> +		return PTR_ERR(st->rdvel);
> +	}
>  
>  	indio_dev->dev.parent = &spi->dev;
>  	indio_dev->info = &ad2s1200_info;

Re: [PATCH v3 6/9] staging: iio: ad2s1200: Add scaling factor for angular velocity channel

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:03:59 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> The sysfs iio ABI states radians per second is expected as the unit for
> angular velocity, but the 12-bit angular velocity register has rps
Really small point, but rps is a bit ambiguous given we are
talking about converting to radian's per second ;)

revs is the 'common' name for what it currently is I think.

Otherwise this looks good.

Jonathan


> as its unit. So a scaling factor of approximately 2 * Pi is
> added to the angular velocity channel.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> ---
> Changes in v3:
>  - A decimal approximation of the scale is used, instead
>    of a fractional approximation.
>  - Remove unneeded explanation of iio_convert_raw_to_processed.
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 71 +++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 3c7163610ff6..3e1696e52c38 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -57,37 +57,55 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  	struct ad2s1200_state *st = iio_priv(indio_dev);
>  	int ret = 0;
>  
> -	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) {
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_ANGL_VEL:
> +			/* 2 * Pi ~= 6.283185 */
> +			*val = 6;
> +			*val2 = 283185;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		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;
> +		}
> +
> +		switch (chan->type) {
> +		case IIO_ANGL:
> +			*val = be16_to_cpup(&st->rx) >> 4;
> +			break;
> +		case IIO_ANGL_VEL:
> +			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> +			break;
> +		default:
> +			mutex_unlock(&st->lock);
> +			return -EINVAL;
> +		}
> +
> +		/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
> +		udelay(1);
>  		mutex_unlock(&st->lock);
> -		return ret;
> -	}
>  
> -	switch (chan->type) {
> -	case IIO_ANGL:
> -		*val = be16_to_cpup(&st->rx) >> 4;
> -		break;
> -	case IIO_ANGL_VEL:
> -		*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> -		break;
> +		return IIO_VAL_INT;
>  	default:
> -		mutex_unlock(&st->lock);
> -		return -EINVAL;
> +		break;
>  	}
>  
> -	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
> -	udelay(1);
> -	mutex_unlock(&st->lock);
> -
> -	return IIO_VAL_INT;
> +	return -EINVAL;
>  }
>  
>  static const struct iio_chan_spec ad2s1200_channels[] = {
> @@ -101,6 +119,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),
>  	}
>  };
>  

Re: [PATCH v3 6/9] staging: iio: ad2s1200: Add scaling factor for angular velocity channel

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:03:59 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> The sysfs iio ABI states radians per second is expected as the unit for
> angular velocity, but the 12-bit angular velocity register has rps
Really small point, but rps is a bit ambiguous given we are
talking about converting to radian's per second ;)

revs is the 'common' name for what it currently is I think.

Otherwise this looks good.

Jonathan


> as its unit. So a scaling factor of approximately 2 * Pi is
> added to the angular velocity channel.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> ---
> Changes in v3:
>  - A decimal approximation of the scale is used, instead
>    of a fractional approximation.
>  - Remove unneeded explanation of iio_convert_raw_to_processed.
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 71 +++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 3c7163610ff6..3e1696e52c38 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -57,37 +57,55 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  	struct ad2s1200_state *st = iio_priv(indio_dev);
>  	int ret = 0;
>  
> -	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) {
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_ANGL_VEL:
> +			/* 2 * Pi ~= 6.283185 */
> +			*val = 6;
> +			*val2 = 283185;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		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;
> +		}
> +
> +		switch (chan->type) {
> +		case IIO_ANGL:
> +			*val = be16_to_cpup(&st->rx) >> 4;
> +			break;
> +		case IIO_ANGL_VEL:
> +			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> +			break;
> +		default:
> +			mutex_unlock(&st->lock);
> +			return -EINVAL;
> +		}
> +
> +		/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
> +		udelay(1);
>  		mutex_unlock(&st->lock);
> -		return ret;
> -	}
>  
> -	switch (chan->type) {
> -	case IIO_ANGL:
> -		*val = be16_to_cpup(&st->rx) >> 4;
> -		break;
> -	case IIO_ANGL_VEL:
> -		*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> -		break;
> +		return IIO_VAL_INT;
>  	default:
> -		mutex_unlock(&st->lock);
> -		return -EINVAL;
> +		break;
>  	}
>  
> -	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
> -	udelay(1);
> -	mutex_unlock(&st->lock);
> -
> -	return IIO_VAL_INT;
> +	return -EINVAL;
>  }
>  
>  static const struct iio_chan_spec ad2s1200_channels[] = {
> @@ -101,6 +119,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),
>  	}
>  };
>  

Re: [PATCH v3 6/9] staging: iio: ad2s1200: Add scaling factor for angular velocity channel

[Jonathan Cameron]

On Sat, 28 Apr 2018 18:23:44 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 23 Apr 2018 00:03:59 +0200
> David Veenstra <davidjulianveenstra@gmail.com> wrote:
> 
> > The sysfs iio ABI states radians per second is expected as the unit for
> > angular velocity, but the 12-bit angular velocity register has rps  
> Really small point, but rps is a bit ambiguous given we are
> talking about converting to radian's per second ;)
> 
> revs is the 'common' name for what it currently is I think.
> 
> Otherwise this looks good.
Actually, I can't immediately tell from the datasheet what the scaling is...

"Bit 15 through bit 4 correspond to the angular
information. The angular position data format is unsigned
binary, with all zeros corresponding to 0 degrees and all ones
corresponding to 360 degrees –l LSB. The angular velocity data
format instead is twos complement binary, with the MSB
representing the rotation direction"

Earlier was also had:

"Data Format
The digital angle signal represents the absolute position of the
resolver shaft as a 12-bit unsigned binary word. The digital
velocity signal is a 12-bit twos complement word, which
represents the velocity of the resolver shaft rotating in either a
clockwise or a counterclockwise direction."

So for position the 12 bits correspond to 0 to 360 - 360/(2^12)
hence _SCALE is 360/(2^12)

But I'm not seeing any hint whatsoever on the scaling for the
rotational velocity...

Am I missing it somewhere?  you might be right in how you have
read it given it will track up to 1000rps and we are 2 directional
so -2048 to 2047 so not a huge amount of wasted space.

Failing any other evidence I'll go with what you have until we
can test this, but the datasheet could be clearer!

Jonathan
> 
> Jonathan
> 
> 
> > as its unit. So a scaling factor of approximately 2 * Pi is
> > added to the angular velocity channel.
> > 
> > Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> > ---
> > Changes in v3:
> >  - A decimal approximation of the scale is used, instead
> >    of a fractional approximation.
> >  - Remove unneeded explanation of iio_convert_raw_to_processed.
> > 
> >  drivers/staging/iio/resolver/ad2s1200.c | 71 +++++++++++++++++++++------------
> >  1 file changed, 45 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> > index 3c7163610ff6..3e1696e52c38 100644
> > --- a/drivers/staging/iio/resolver/ad2s1200.c
> > +++ b/drivers/staging/iio/resolver/ad2s1200.c
> > @@ -57,37 +57,55 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> >  	struct ad2s1200_state *st = iio_priv(indio_dev);
> >  	int ret = 0;
> >  
> > -	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) {
> > +	switch (m) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		switch (chan->type) {
> > +		case IIO_ANGL_VEL:
> > +			/* 2 * Pi ~= 6.283185 */
> > +			*val = 6;
> > +			*val2 = 283185;
> > +			return IIO_VAL_INT_PLUS_MICRO;
> > +		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;
> > +		}
> > +
> > +		switch (chan->type) {
> > +		case IIO_ANGL:
> > +			*val = be16_to_cpup(&st->rx) >> 4;
> > +			break;
> > +		case IIO_ANGL_VEL:
> > +			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> > +			break;
> > +		default:
> > +			mutex_unlock(&st->lock);
> > +			return -EINVAL;
> > +		}
> > +
> > +		/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
> > +		udelay(1);
> >  		mutex_unlock(&st->lock);
> > -		return ret;
> > -	}
> >  
> > -	switch (chan->type) {
> > -	case IIO_ANGL:
> > -		*val = be16_to_cpup(&st->rx) >> 4;
> > -		break;
> > -	case IIO_ANGL_VEL:
> > -		*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> > -		break;
> > +		return IIO_VAL_INT;
> >  	default:
> > -		mutex_unlock(&st->lock);
> > -		return -EINVAL;
> > +		break;
> >  	}
> >  
> > -	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
> > -	udelay(1);
> > -	mutex_unlock(&st->lock);
> > -
> > -	return IIO_VAL_INT;
> > +	return -EINVAL;
> >  }
> >  
> >  static const struct iio_chan_spec ad2s1200_channels[] = {
> > @@ -101,6 +119,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),
> >  	}
> >  };
> >    
> 
> --
> 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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 6/9] staging: iio: ad2s1200: Add scaling factor for angular velocity channel

[Jonathan Cameron]

On Sat, 28 Apr 2018 18:23:44 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 23 Apr 2018 00:03:59 +0200
> David Veenstra <davidjulianveenstra@gmail.com> wrote:
> 
> > The sysfs iio ABI states radians per second is expected as the unit for
> > angular velocity, but the 12-bit angular velocity register has rps  
> Really small point, but rps is a bit ambiguous given we are
> talking about converting to radian's per second ;)
> 
> revs is the 'common' name for what it currently is I think.
> 
> Otherwise this looks good.
Actually, I can't immediately tell from the datasheet what the scaling is...

"Bit 15 through bit 4 correspond to the angular
information. The angular position data format is unsigned
binary, with all zeros corresponding to 0 degrees and all ones
corresponding to 360 degrees –l LSB. The angular velocity data
format instead is twos complement binary, with the MSB
representing the rotation direction"

Earlier was also had:

"Data Format
The digital angle signal represents the absolute position of the
resolver shaft as a 12-bit unsigned binary word. The digital
velocity signal is a 12-bit twos complement word, which
represents the velocity of the resolver shaft rotating in either a
clockwise or a counterclockwise direction."

So for position the 12 bits correspond to 0 to 360 - 360/(2^12)
hence _SCALE is 360/(2^12)

But I'm not seeing any hint whatsoever on the scaling for the
rotational velocity...

Am I missing it somewhere?  you might be right in how you have
read it given it will track up to 1000rps and we are 2 directional
so -2048 to 2047 so not a huge amount of wasted space.

Failing any other evidence I'll go with what you have until we
can test this, but the datasheet could be clearer!

Jonathan
> 
> Jonathan
> 
> 
> > as its unit. So a scaling factor of approximately 2 * Pi is
> > added to the angular velocity channel.
> > 
> > Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> > ---
> > Changes in v3:
> >  - A decimal approximation of the scale is used, instead
> >    of a fractional approximation.
> >  - Remove unneeded explanation of iio_convert_raw_to_processed.
> > 
> >  drivers/staging/iio/resolver/ad2s1200.c | 71 +++++++++++++++++++++------------
> >  1 file changed, 45 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> > index 3c7163610ff6..3e1696e52c38 100644
> > --- a/drivers/staging/iio/resolver/ad2s1200.c
> > +++ b/drivers/staging/iio/resolver/ad2s1200.c
> > @@ -57,37 +57,55 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> >  	struct ad2s1200_state *st = iio_priv(indio_dev);
> >  	int ret = 0;
> >  
> > -	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) {
> > +	switch (m) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		switch (chan->type) {
> > +		case IIO_ANGL_VEL:
> > +			/* 2 * Pi ~= 6.283185 */
> > +			*val = 6;
> > +			*val2 = 283185;
> > +			return IIO_VAL_INT_PLUS_MICRO;
> > +		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;
> > +		}
> > +
> > +		switch (chan->type) {
> > +		case IIO_ANGL:
> > +			*val = be16_to_cpup(&st->rx) >> 4;
> > +			break;
> > +		case IIO_ANGL_VEL:
> > +			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> > +			break;
> > +		default:
> > +			mutex_unlock(&st->lock);
> > +			return -EINVAL;
> > +		}
> > +
> > +		/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
> > +		udelay(1);
> >  		mutex_unlock(&st->lock);
> > -		return ret;
> > -	}
> >  
> > -	switch (chan->type) {
> > -	case IIO_ANGL:
> > -		*val = be16_to_cpup(&st->rx) >> 4;
> > -		break;
> > -	case IIO_ANGL_VEL:
> > -		*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> > -		break;
> > +		return IIO_VAL_INT;
> >  	default:
> > -		mutex_unlock(&st->lock);
> > -		return -EINVAL;
> > +		break;
> >  	}
> >  
> > -	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
> > -	udelay(1);
> > -	mutex_unlock(&st->lock);
> > -
> > -	return IIO_VAL_INT;
> > +	return -EINVAL;
> >  }
> >  
> >  static const struct iio_chan_spec ad2s1200_channels[] = {
> > @@ -101,6 +119,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),
> >  	}
> >  };
> >    
> 
> --
> 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 v3 7/9] staging: iio: Documentation: Add missing sysfs docs for angle channel

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:04:10 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> The iio resolver drivers in staging use angle channels. This patch
> add missing documentation for this type of channel.
> 
> As was discussed in [1], radians is chosen as the unit, to match the
> unit of angular velocity.
> 
> [1] https://marc.info/?l=linux-driver-devel&m=152190078308330&w=2
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Great, no point in waiting on the other patches for the ABI docs
as we are fine with this.

Applied,

Thanks,

Jonathan

> ---
> Changes in v3:
>  - Remove axis modifier on in_angle.
> 
>  Documentation/ABI/testing/sysfs-bus-iio | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 6a5f34b4d5b9..240287e2343a 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -190,6 +190,13 @@ Description:
>  		but should match other such assignments on device).
>  		Units after application of scale and offset are m/s^2.
>  
> +What:		/sys/bus/iio/devices/iio:deviceX/in_angl_raw
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Angle of rotation. Units after application of scale and offset
> +		are radians.
> +
>  What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_x_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_y_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_z_raw
> @@ -297,6 +304,7 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_pressure_offset
>  What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_offset
>  What:		/sys/bus/iio/devices/iio:deviceX/in_magn_offset
>  What:		/sys/bus/iio/devices/iio:deviceX/in_rot_offset
> +What:		/sys/bus/iio/devices/iio:deviceX/in_angl_offset
>  KernelVersion:	2.6.35
>  Contact:	linux-iio@vger.kernel.org
>  Description:
> @@ -350,6 +358,7 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_scale
>  What:		/sys/bus/iio/devices/iio:deviceX/in_velocity_sqrt(x^2+y^2+z^2)_scale
>  What:		/sys/bus/iio/devices/iio:deviceX/in_illuminance_scale
>  What:		/sys/bus/iio/devices/iio:deviceX/in_countY_scale
> +What:		/sys/bus/iio/devices/iio:deviceX/in_angl_scale
>  KernelVersion:	2.6.35
>  Contact:	linux-iio@vger.kernel.org
>  Description:

Re: [PATCH v3 7/9] staging: iio: Documentation: Add missing sysfs docs for angle channel

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:04:10 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> The iio resolver drivers in staging use angle channels. This patch
> add missing documentation for this type of channel.
> 
> As was discussed in [1], radians is chosen as the unit, to match the
> unit of angular velocity.
> 
> [1] https://marc.info/?l=linux-driver-devel&m=152190078308330&w=2
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Great, no point in waiting on the other patches for the ABI docs
as we are fine with this.

Applied,

Thanks,

Jonathan

> ---
> Changes in v3:
>  - Remove axis modifier on in_angle.
> 
>  Documentation/ABI/testing/sysfs-bus-iio | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 6a5f34b4d5b9..240287e2343a 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -190,6 +190,13 @@ Description:
>  		but should match other such assignments on device).
>  		Units after application of scale and offset are m/s^2.
>  
> +What:		/sys/bus/iio/devices/iio:deviceX/in_angl_raw
> +KernelVersion:	4.17
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Angle of rotation. Units after application of scale and offset
> +		are radians.
> +
>  What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_x_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_y_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/in_anglvel_z_raw
> @@ -297,6 +304,7 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_pressure_offset
>  What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_offset
>  What:		/sys/bus/iio/devices/iio:deviceX/in_magn_offset
>  What:		/sys/bus/iio/devices/iio:deviceX/in_rot_offset
> +What:		/sys/bus/iio/devices/iio:deviceX/in_angl_offset
>  KernelVersion:	2.6.35
>  Contact:	linux-iio@vger.kernel.org
>  Description:
> @@ -350,6 +358,7 @@ What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_scale
>  What:		/sys/bus/iio/devices/iio:deviceX/in_velocity_sqrt(x^2+y^2+z^2)_scale
>  What:		/sys/bus/iio/devices/iio:deviceX/in_illuminance_scale
>  What:		/sys/bus/iio/devices/iio:deviceX/in_countY_scale
> +What:		/sys/bus/iio/devices/iio:deviceX/in_angl_scale
>  KernelVersion:	2.6.35
>  Contact:	linux-iio@vger.kernel.org
>  Description:

Re: [PATCH v3 8/9] staging: iio: ad2s1200: Add scaling factor for angle channel

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:04:24 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> A scaling factor of approximately 2 * Pi / (2^12 -1) is added,
> to scale the 12-bits angular position to radians.
> 
> A return type of IIO_VAL_INT_PLUS_NANO is used, so that the scale of
> both the angle channel and angular velocity channel has 7 significant
> digits.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Good, will pick up with the precursors in v4.

Thanks,

Jonathan

> ---
> Changes in v3:
>  - A decimal approximation of the scale is used, instead
>    of a fractional approximation.
>  - Remove unneeded explanation of iio_convert_raw_to_processed.
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 3e1696e52c38..d2b62308b31d 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -60,6 +60,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  	switch (m) {
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
> +		case IIO_ANGL:
> +			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
> +			*val = 0;
> +			*val2 = 1534355;
> +			return IIO_VAL_INT_PLUS_NANO;
>  		case IIO_ANGL_VEL:
>  			/* 2 * Pi ~= 6.283185 */
>  			*val = 6;
> @@ -114,6 +119,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),
>  	}, {
>  		.type = IIO_ANGL_VEL,
>  		.indexed = 1,

Re: [PATCH v3 8/9] staging: iio: ad2s1200: Add scaling factor for angle channel

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:04:24 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> A scaling factor of approximately 2 * Pi / (2^12 -1) is added,
> to scale the 12-bits angular position to radians.
> 
> A return type of IIO_VAL_INT_PLUS_NANO is used, so that the scale of
> both the angle channel and angular velocity channel has 7 significant
> digits.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
Good, will pick up with the precursors in v4.

Thanks,

Jonathan

> ---
> Changes in v3:
>  - A decimal approximation of the scale is used, instead
>    of a fractional approximation.
>  - Remove unneeded explanation of iio_convert_raw_to_processed.
> 
>  drivers/staging/iio/resolver/ad2s1200.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
> index 3e1696e52c38..d2b62308b31d 100644
> --- a/drivers/staging/iio/resolver/ad2s1200.c
> +++ b/drivers/staging/iio/resolver/ad2s1200.c
> @@ -60,6 +60,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>  	switch (m) {
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
> +		case IIO_ANGL:
> +			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
> +			*val = 0;
> +			*val2 = 1534355;
> +			return IIO_VAL_INT_PLUS_NANO;
>  		case IIO_ANGL_VEL:
>  			/* 2 * Pi ~= 6.283185 */
>  			*val = 6;
> @@ -114,6 +119,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),
>  	}, {
>  		.type = IIO_ANGL_VEL,
>  		.indexed = 1,

Re: [PATCH v3 9/9] staging: iio: ad2s1200: Move driver out of staging

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:04:42 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> Move the iio driver for the ad2s1200 and ad2s1205 resolver-to-digital
> converter out of staging, into mainline iio subsystems.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
A few more minor bits and bobs + suggestions.

Some little things that I missed in earlier patches.  If I haven't
taken the relevant patch, please roll the fixes in for V4.  If I have
just do a little follow up patch.

Very nearly there!

thanks,

Jonathan

> ---
> Changes in v3:
>  - Add mention of ad2s1205 in commit message.
> 
>  drivers/iio/Kconfig                     |   1 +
>  drivers/iio/Makefile                    |   1 +
>  drivers/iio/resolver/Kconfig            |  17 +++
>  drivers/iio/resolver/Makefile           |   5 +
>  drivers/iio/resolver/ad2s1200.c         | 201 ++++++++++++++++++++++++++++++++
>  drivers/staging/iio/resolver/Kconfig    |  12 --
>  drivers/staging/iio/resolver/Makefile   |   1 -
>  drivers/staging/iio/resolver/ad2s1200.c | 201 --------------------------------
>  8 files changed, 225 insertions(+), 214 deletions(-)
>  create mode 100644 drivers/iio/resolver/Kconfig
>  create mode 100644 drivers/iio/resolver/Makefile
>  create mode 100644 drivers/iio/resolver/ad2s1200.c
>  delete mode 100644 drivers/staging/iio/resolver/ad2s1200.c
> 
> 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/iio/resolver/ad2s1200.c b/drivers/iio/resolver/ad2s1200.c
> new file mode 100644
> index 000000000000..d2b62308b31d
> --- /dev/null
> +++ b/drivers/iio/resolver/ad2s1200.c
> @@ -0,0 +1,201 @@
> +/*
> + * ad2s1200.c simple support for the ADI Resolver to Digital Converters:
> + * AD2S1200/1205
> + *
> + * Copyright (c) 2010-2010 Analog Devices Inc.

I think you have done enough changes in here that if you want to add your
own copyright I think it would be justified.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *

One of my pet hates if you want to clean it up :)
No point in having a blank line above here.

> + */
> +
> +#include <linux/bitops.h>
> +#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/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define DRV_NAME "ad2s1200"
> +
> +/* input clock on serial interface */
> +#define AD2S1200_HZ	8192000
> +/* clock period in nano second */
> +#define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
> +
> +/**
> + * struct ad2s1200_state - driver instance specific data.
> + * @lock:	protects both the GPIO pins and the rx buffer, to prevent
> + *		concurrent spi transactions.
> + * @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;
> +	struct gpio_desc *sample;
> +	struct gpio_desc *rdvel;
> +	__be16 rx ____cacheline_aligned;
> +};
> +
> +static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val,
> +			     int *val2,
> +			     long m)
> +{
> +	struct ad2s1200_state *st = iio_priv(indio_dev);
> +	int ret = 0;
We only return ret in paths that set it. So doesn't need
initializing any more.  This might want to be pushed down
to the earlier patch changing this code.
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_ANGL:
> +			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
> +			*val = 0;
> +			*val2 = 1534355;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		case IIO_ANGL_VEL:
> +			/* 2 * Pi ~= 6.283185 */
> +			*val = 6;
> +			*val2 = 283185;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		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;
> +		}
> +
> +		switch (chan->type) {
> +		case IIO_ANGL:
> +			*val = be16_to_cpup(&st->rx) >> 4;
> +			break;
> +		case IIO_ANGL_VEL:
> +			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> +			break;
> +		default:
> +			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;
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_chan_spec ad2s1200_channels[] = {
> +	{
> +		.type = IIO_ANGL,
> +		.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,
> +		.channel = 0,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +	}
> +};
> +
> +static const struct iio_info ad2s1200_info = {
> +	.read_raw = ad2s1200_read_raw,
> +};
> +
> +static int ad2s1200_probe(struct spi_device *spi)
> +{
> +	struct ad2s1200_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret = 0;
I guess this is left from somewhere and should have been in
an earlier patch, but there is no need to set to 0 and
it is set in all paths.

Mind you I've suggested you get rid of the only user
anyway so that won't matter as this will go away anyway.

> +
> +	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);
> +	st->sdev = spi;
> +
> +	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->sample)) {
> +		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
> +			PTR_ERR(st->sample));
> +		return PTR_ERR(st->sample);
> +	}
> +
> +	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->rdvel)) {
> +		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
> +			PTR_ERR(st->rdvel));
> +		return PTR_ERR(st->rdvel);
> +	}
> +
> +	indio_dev->dev.parent = &spi->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);
> +	if (ret)
> +		return ret;
> +
> +	spi->max_speed_hz = AD2S1200_HZ;
> +	spi->mode = SPI_MODE_3;
> +	spi_setup(spi);
This ordering is racy.  We could in theory have been
talking to the device after it's interfaces were exposed
in the devm_iio_device_register call.

Reorder this to put them before that then just do
return devm_iio_device_register
to finish up probe.

> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id ad2s1200_id[] = {
> +	{ "ad2s1200" },
Worth putting in an explicit device tree table for matching
with the vendor ID.  The intent long term is to remove
the nasty code that leads to fall back matching with the
spi_device_id table.

No reason this needs to happen before taking it out of staging.

> +	{ "ad2s1205" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, ad2s1200_id);
> +
> +static struct spi_driver ad2s1200_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +	.probe = ad2s1200_probe,
> +	.id_table = ad2s1200_id,
> +};
> +module_spi_driver(ad2s1200_driver);
> +
> +MODULE_AUTHOR("Graff Yang <graff.yang@gmail.com>");
> +MODULE_DESCRIPTION("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver");
> +MODULE_LICENSE("GPL v2");
...

Re: [PATCH v3 9/9] staging: iio: ad2s1200: Move driver out of staging

[Jonathan Cameron]

On Mon, 23 Apr 2018 00:04:42 +0200
David Veenstra <davidjulianveenstra@gmail.com> wrote:

> Move the iio driver for the ad2s1200 and ad2s1205 resolver-to-digital
> converter out of staging, into mainline iio subsystems.
> 
> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
A few more minor bits and bobs + suggestions.

Some little things that I missed in earlier patches.  If I haven't
taken the relevant patch, please roll the fixes in for V4.  If I have
just do a little follow up patch.

Very nearly there!

thanks,

Jonathan

> ---
> Changes in v3:
>  - Add mention of ad2s1205 in commit message.
> 
>  drivers/iio/Kconfig                     |   1 +
>  drivers/iio/Makefile                    |   1 +
>  drivers/iio/resolver/Kconfig            |  17 +++
>  drivers/iio/resolver/Makefile           |   5 +
>  drivers/iio/resolver/ad2s1200.c         | 201 ++++++++++++++++++++++++++++++++
>  drivers/staging/iio/resolver/Kconfig    |  12 --
>  drivers/staging/iio/resolver/Makefile   |   1 -
>  drivers/staging/iio/resolver/ad2s1200.c | 201 --------------------------------
>  8 files changed, 225 insertions(+), 214 deletions(-)
>  create mode 100644 drivers/iio/resolver/Kconfig
>  create mode 100644 drivers/iio/resolver/Makefile
>  create mode 100644 drivers/iio/resolver/ad2s1200.c
>  delete mode 100644 drivers/staging/iio/resolver/ad2s1200.c
> 
> 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/iio/resolver/ad2s1200.c b/drivers/iio/resolver/ad2s1200.c
> new file mode 100644
> index 000000000000..d2b62308b31d
> --- /dev/null
> +++ b/drivers/iio/resolver/ad2s1200.c
> @@ -0,0 +1,201 @@
> +/*
> + * ad2s1200.c simple support for the ADI Resolver to Digital Converters:
> + * AD2S1200/1205
> + *
> + * Copyright (c) 2010-2010 Analog Devices Inc.

I think you have done enough changes in here that if you want to add your
own copyright I think it would be justified.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *

One of my pet hates if you want to clean it up :)
No point in having a blank line above here.

> + */
> +
> +#include <linux/bitops.h>
> +#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/spi/spi.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define DRV_NAME "ad2s1200"
> +
> +/* input clock on serial interface */
> +#define AD2S1200_HZ	8192000
> +/* clock period in nano second */
> +#define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
> +
> +/**
> + * struct ad2s1200_state - driver instance specific data.
> + * @lock:	protects both the GPIO pins and the rx buffer, to prevent
> + *		concurrent spi transactions.
> + * @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;
> +	struct gpio_desc *sample;
> +	struct gpio_desc *rdvel;
> +	__be16 rx ____cacheline_aligned;
> +};
> +
> +static int ad2s1200_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val,
> +			     int *val2,
> +			     long m)
> +{
> +	struct ad2s1200_state *st = iio_priv(indio_dev);
> +	int ret = 0;
We only return ret in paths that set it. So doesn't need
initializing any more.  This might want to be pushed down
to the earlier patch changing this code.
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_ANGL:
> +			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
> +			*val = 0;
> +			*val2 = 1534355;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		case IIO_ANGL_VEL:
> +			/* 2 * Pi ~= 6.283185 */
> +			*val = 6;
> +			*val2 = 283185;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		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;
> +		}
> +
> +		switch (chan->type) {
> +		case IIO_ANGL:
> +			*val = be16_to_cpup(&st->rx) >> 4;
> +			break;
> +		case IIO_ANGL_VEL:
> +			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
> +			break;
> +		default:
> +			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;
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_chan_spec ad2s1200_channels[] = {
> +	{
> +		.type = IIO_ANGL,
> +		.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,
> +		.channel = 0,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +	}
> +};
> +
> +static const struct iio_info ad2s1200_info = {
> +	.read_raw = ad2s1200_read_raw,
> +};
> +
> +static int ad2s1200_probe(struct spi_device *spi)
> +{
> +	struct ad2s1200_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret = 0;
I guess this is left from somewhere and should have been in
an earlier patch, but there is no need to set to 0 and
it is set in all paths.

Mind you I've suggested you get rid of the only user
anyway so that won't matter as this will go away anyway.

> +
> +	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);
> +	st->sdev = spi;
> +
> +	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->sample)) {
> +		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
> +			PTR_ERR(st->sample));
> +		return PTR_ERR(st->sample);
> +	}
> +
> +	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->rdvel)) {
> +		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
> +			PTR_ERR(st->rdvel));
> +		return PTR_ERR(st->rdvel);
> +	}
> +
> +	indio_dev->dev.parent = &spi->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);
> +	if (ret)
> +		return ret;
> +
> +	spi->max_speed_hz = AD2S1200_HZ;
> +	spi->mode = SPI_MODE_3;
> +	spi_setup(spi);
This ordering is racy.  We could in theory have been
talking to the device after it's interfaces were exposed
in the devm_iio_device_register call.

Reorder this to put them before that then just do
return devm_iio_device_register
to finish up probe.

> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id ad2s1200_id[] = {
> +	{ "ad2s1200" },
Worth putting in an explicit device tree table for matching
with the vendor ID.  The intent long term is to remove
the nasty code that leads to fall back matching with the
spi_device_id table.

No reason this needs to happen before taking it out of staging.

> +	{ "ad2s1205" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, ad2s1200_id);
> +
> +static struct spi_driver ad2s1200_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +	.probe = ad2s1200_probe,
> +	.id_table = ad2s1200_id,
> +};
> +module_spi_driver(ad2s1200_driver);
> +
> +MODULE_AUTHOR("Graff Yang <graff.yang@gmail.com>");
> +MODULE_DESCRIPTION("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver");
> +MODULE_LICENSE("GPL v2");
...

Re: [PATCH v3 1/9] staging: iio: ad2s1200: Add kernel docs to driver state

[David Julian Veenstra]

On 28, April 2018 17:11, Jonathan Cameron wrote:

> On Mon, 23 Apr 2018 00:02:51 +0200
> David Veenstra <davidjulianveenstra@gmail.com> wrote:
>
>> Add missing kernel docs to the ad2s1200 driver state.
>> 
>> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> Hi David,
>
> Comment inline.
>
>> ---
>> Changes in v3:
>>  - Added more explanation to mutex lock.
>> 
>>  drivers/staging/iio/resolver/ad2s1200.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
>> index 357fe3c382b3..17d65cb437fd 100644
>> --- a/drivers/staging/iio/resolver/ad2s1200.c
>> +++ b/drivers/staging/iio/resolver/ad2s1200.c
>> @@ -33,6 +33,15 @@
>>  /* clock period in nano second */
>>  #define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
>>  
>> +/**
>> + * struct ad2s1200_state - driver instance specific data.
>> + * @lock:	protects both the GPIO pins and the rx buffer, to prevent
>> + *		concurrent spi transactions.
> It isn't actually preventing concurrent spi transactions - that
> is done by the spi core itself.  What it is preventing is
> concurrent 'actions' with the device - these involve
> a mixture of gpio changes and spi transactions.
>
> That would take some considerable explaining and frankly the code
> does the job just fine once people know to look.
>
> I'd just leave it as "protects both the GPIO pins and the rx buffer."
> Or feel free to see if you can come up with a brief description of
> exactly what we need to be 'atomic'.

Agreed that the code is clear. I'll revert the description.

Best regards,
David Veenstra

>
> Thanks,
>
> Jonathan
>
>> + * @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;

Re: [PATCH v3 1/9] staging: iio: ad2s1200: Add kernel docs to driver state

[David Julian Veenstra]

On 28, April 2018 17:11, Jonathan Cameron wrote:

> On Mon, 23 Apr 2018 00:02:51 +0200
> David Veenstra <davidjulianveenstra@gmail.com> wrote:
>
>> Add missing kernel docs to the ad2s1200 driver state.
>> 
>> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> Hi David,
>
> Comment inline.
>
>> ---
>> Changes in v3:
>>  - Added more explanation to mutex lock.
>> 
>>  drivers/staging/iio/resolver/ad2s1200.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
>> index 357fe3c382b3..17d65cb437fd 100644
>> --- a/drivers/staging/iio/resolver/ad2s1200.c
>> +++ b/drivers/staging/iio/resolver/ad2s1200.c
>> @@ -33,6 +33,15 @@
>>  /* clock period in nano second */
>>  #define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
>>  
>> +/**
>> + * struct ad2s1200_state - driver instance specific data.
>> + * @lock:	protects both the GPIO pins and the rx buffer, to prevent
>> + *		concurrent spi transactions.
> It isn't actually preventing concurrent spi transactions - that
> is done by the spi core itself.  What it is preventing is
> concurrent 'actions' with the device - these involve
> a mixture of gpio changes and spi transactions.
>
> That would take some considerable explaining and frankly the code
> does the job just fine once people know to look.
>
> I'd just leave it as "protects both the GPIO pins and the rx buffer."
> Or feel free to see if you can come up with a brief description of
> exactly what we need to be 'atomic'.

Agreed that the code is clear. I'll revert the description.

Best regards,
David Veenstra

>
> Thanks,
>
> Jonathan
>
>> + * @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;

Re: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding

[David Julian Veenstra]

On 28, April 2018 15:27, Jonathan Cameron wrote:

> On Fri, 27 Apr 2018 09:48:20 -0500
> Rob Herring <robh@kernel.org> wrote:
>
>> On Mon, Apr 23, 2018 at 12:03:47AM +0200, David Veenstra wrote:
>> > Add documentation for the added device tree bindings.
>> > 
>> > Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
>> > ---
>> > Changes in v3:
>> >  - Documentation is added to Documentation/devicetree/bindings/iio/resolver
>> >    instead of staging directory.
>> >  - Add mention to ad2s1205 device.
>> > 
>> >  .../devicetree/bindings/iio/resolver/ad2s1200.txt        | 16 ++++++++++++++++
>> >  1 file changed, 16 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
>> > 
>> > diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
>> > new file mode 100644
>> > index 000000000000..46b51db38368
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
>> > @@ -0,0 +1,16 @@
>> > +Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter
>> > +
>> > +Required properties:
>> > + - compatible : should be "adi,ad2s1200" or "adi,ad2s1205"
>> > + - reg : the SPI chip select number of the device
>> > + - sample-gpios : The GPIO pin connected to the SAMPLE line of the AD2S1200
>> > + - rdvel-gpios : The GPIO pin connected to the RDVEL line of the AD2S1200  
>> 
>> Are these gpios something that would be common to resolvers (whatever 
>> that is) or specific to this chip. For the latter, you need vendor 
>> prefixes.
>
> They are pretty chip specific I think, so prefixes needed.
>
> I'd not really registered that gpios should be vendor prefixed for some reason
> (kind of obvious now you mention it).  We have only a few from a quick grep
> of the bindings for IIO devices (there are loads elsewhere that aren't and
> are clearly very much part specific).  Ah well, will try and do better moving
> forward on this!

The vendor prefixes are new to me. I'll add it in the next version.

Best regards,
David Veenstra

>
> Jonathan
>
>
>> 
>> > +
>> > +Example:
>> > +
>> > +	resolver {  
>> 
>> Missing unit address.
>> 
>> > +		compatible = "adi,ad2s1200";
>> > +		reg = <4>;
>> > +		sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
>> > +		rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
>> > +	};
>> > -- 
>> > 2.16.2
>> >   

Re: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding

[David Julian Veenstra]

On 28, April 2018 15:27, Jonathan Cameron wrote:

> On Fri, 27 Apr 2018 09:48:20 -0500
> Rob Herring <robh@kernel.org> wrote:
>
>> On Mon, Apr 23, 2018 at 12:03:47AM +0200, David Veenstra wrote:
>> > Add documentation for the added device tree bindings.
>> > 
>> > Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
>> > ---
>> > Changes in v3:
>> >  - Documentation is added to Documentation/devicetree/bindings/iio/resolver
>> >    instead of staging directory.
>> >  - Add mention to ad2s1205 device.
>> > 
>> >  .../devicetree/bindings/iio/resolver/ad2s1200.txt        | 16 ++++++++++++++++
>> >  1 file changed, 16 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
>> > 
>> > diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
>> > new file mode 100644
>> > index 000000000000..46b51db38368
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt
>> > @@ -0,0 +1,16 @@
>> > +Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter
>> > +
>> > +Required properties:
>> > + - compatible : should be "adi,ad2s1200" or "adi,ad2s1205"
>> > + - reg : the SPI chip select number of the device
>> > + - sample-gpios : The GPIO pin connected to the SAMPLE line of the AD2S1200
>> > + - rdvel-gpios : The GPIO pin connected to the RDVEL line of the AD2S1200  
>> 
>> Are these gpios something that would be common to resolvers (whatever 
>> that is) or specific to this chip. For the latter, you need vendor 
>> prefixes.
>
> They are pretty chip specific I think, so prefixes needed.
>
> I'd not really registered that gpios should be vendor prefixed for some reason
> (kind of obvious now you mention it).  We have only a few from a quick grep
> of the bindings for IIO devices (there are loads elsewhere that aren't and
> are clearly very much part specific).  Ah well, will try and do better moving
> forward on this!

The vendor prefixes are new to me. I'll add it in the next version.

Best regards,
David Veenstra

>
> Jonathan
>
>
>> 
>> > +
>> > +Example:
>> > +
>> > +	resolver {  
>> 
>> Missing unit address.
>> 
>> > +		compatible = "adi,ad2s1200";
>> > +		reg = <4>;
>> > +		sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>;
>> > +		rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>;
>> > +	};
>> > -- 
>> > 2.16.2
>> >   

Re: [PATCH v3 6/9] staging: iio: ad2s1200: Add scaling factor for angular velocity channel

[David Julian Veenstra]

On 28, April 2018 17:35, Jonathan Cameron wrote:

> On Sat, 28 Apr 2018 18:23:44 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
>
>> On Mon, 23 Apr 2018 00:03:59 +0200
>> David Veenstra <davidjulianveenstra@gmail.com> wrote:
>> 
>> > The sysfs iio ABI states radians per second is expected as the unit for
>> > angular velocity, but the 12-bit angular velocity register has rps  
>> Really small point, but rps is a bit ambiguous given we are
>> talking about converting to radian's per second ;)
>> 
>> revs is the 'common' name for what it currently is I think.
>> 
>> Otherwise this looks good.
> Actually, I can't immediately tell from the datasheet what the scaling is...
>
> "Bit 15 through bit 4 correspond to the angular
> information. The angular position data format is unsigned
> binary, with all zeros corresponding to 0 degrees and all ones
> corresponding to 360 degrees –l LSB. The angular velocity data
> format instead is twos complement binary, with the MSB
> representing the rotation direction"
>
> Earlier was also had:
>
> "Data Format
> The digital angle signal represents the absolute position of the
> resolver shaft as a 12-bit unsigned binary word. The digital
> velocity signal is a 12-bit twos complement word, which
> represents the velocity of the resolver shaft rotating in either a
> clockwise or a counterclockwise direction."
>
> So for position the 12 bits correspond to 0 to 360 - 360/(2^12)
> hence _SCALE is 360/(2^12)
>
> But I'm not seeing any hint whatsoever on the scaling for the
> rotational velocity...
>
> Am I missing it somewhere?  you might be right in how you have
> read it given it will track up to 1000rps and we are 2 directional
> so -2048 to 2047 so not a huge amount of wasted space.
>
> Failing any other evidence I'll go with what you have until we
> can test this, but the datasheet could be clearer!

When I first read the datasheet it wasn't clear to me either. So I
emailed Michael Hennerich. He confirmed to me that the scaling factor
for degrees is indeed 360 / (2^12- 1). And he told me that the unit
for angular velocity is revolutions per second, and that I should
use a scaling factor of 2PI to convert it to rad/s.

Best regards,
David Veenstra

>
> Jonathan
>> 
>> Jonathan
>> 
>> 
>> > as its unit. So a scaling factor of approximately 2 * Pi is
>> > added to the angular velocity channel.
>> > 
>> > Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
>> > ---
>> > Changes in v3:
>> >  - A decimal approximation of the scale is used, instead
>> >    of a fractional approximation.
>> >  - Remove unneeded explanation of iio_convert_raw_to_processed.
>> > 
>> >  drivers/staging/iio/resolver/ad2s1200.c | 71 +++++++++++++++++++++------------
>> >  1 file changed, 45 insertions(+), 26 deletions(-)
>> > 
>> > diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
>> > index 3c7163610ff6..3e1696e52c38 100644
>> > --- a/drivers/staging/iio/resolver/ad2s1200.c
>> > +++ b/drivers/staging/iio/resolver/ad2s1200.c
>> > @@ -57,37 +57,55 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>> >  	struct ad2s1200_state *st = iio_priv(indio_dev);
>> >  	int ret = 0;
>> >  
>> > -	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) {
>> > +	switch (m) {
>> > +	case IIO_CHAN_INFO_SCALE:
>> > +		switch (chan->type) {
>> > +		case IIO_ANGL_VEL:
>> > +			/* 2 * Pi ~= 6.283185 */
>> > +			*val = 6;
>> > +			*val2 = 283185;
>> > +			return IIO_VAL_INT_PLUS_MICRO;
>> > +		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;
>> > +		}
>> > +
>> > +		switch (chan->type) {
>> > +		case IIO_ANGL:
>> > +			*val = be16_to_cpup(&st->rx) >> 4;
>> > +			break;
>> > +		case IIO_ANGL_VEL:
>> > +			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
>> > +			break;
>> > +		default:
>> > +			mutex_unlock(&st->lock);
>> > +			return -EINVAL;
>> > +		}
>> > +
>> > +		/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
>> > +		udelay(1);
>> >  		mutex_unlock(&st->lock);
>> > -		return ret;
>> > -	}
>> >  
>> > -	switch (chan->type) {
>> > -	case IIO_ANGL:
>> > -		*val = be16_to_cpup(&st->rx) >> 4;
>> > -		break;
>> > -	case IIO_ANGL_VEL:
>> > -		*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
>> > -		break;
>> > +		return IIO_VAL_INT;
>> >  	default:
>> > -		mutex_unlock(&st->lock);
>> > -		return -EINVAL;
>> > +		break;
>> >  	}
>> >  
>> > -	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
>> > -	udelay(1);
>> > -	mutex_unlock(&st->lock);
>> > -
>> > -	return IIO_VAL_INT;
>> > +	return -EINVAL;
>> >  }
>> >  
>> >  static const struct iio_chan_spec ad2s1200_channels[] = {
>> > @@ -101,6 +119,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),
>> >  	}
>> >  };
>> >    
>> 
>> --
>> 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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 6/9] staging: iio: ad2s1200: Add scaling factor for angular velocity channel

[David Julian Veenstra]

On 28, April 2018 17:35, Jonathan Cameron wrote:

> On Sat, 28 Apr 2018 18:23:44 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
>
>> On Mon, 23 Apr 2018 00:03:59 +0200
>> David Veenstra <davidjulianveenstra@gmail.com> wrote:
>> 
>> > The sysfs iio ABI states radians per second is expected as the unit for
>> > angular velocity, but the 12-bit angular velocity register has rps  
>> Really small point, but rps is a bit ambiguous given we are
>> talking about converting to radian's per second ;)
>> 
>> revs is the 'common' name for what it currently is I think.
>> 
>> Otherwise this looks good.
> Actually, I can't immediately tell from the datasheet what the scaling is...
>
> "Bit 15 through bit 4 correspond to the angular
> information. The angular position data format is unsigned
> binary, with all zeros corresponding to 0 degrees and all ones
> corresponding to 360 degrees –l LSB. The angular velocity data
> format instead is twos complement binary, with the MSB
> representing the rotation direction"
>
> Earlier was also had:
>
> "Data Format
> The digital angle signal represents the absolute position of the
> resolver shaft as a 12-bit unsigned binary word. The digital
> velocity signal is a 12-bit twos complement word, which
> represents the velocity of the resolver shaft rotating in either a
> clockwise or a counterclockwise direction."
>
> So for position the 12 bits correspond to 0 to 360 - 360/(2^12)
> hence _SCALE is 360/(2^12)
>
> But I'm not seeing any hint whatsoever on the scaling for the
> rotational velocity...
>
> Am I missing it somewhere?  you might be right in how you have
> read it given it will track up to 1000rps and we are 2 directional
> so -2048 to 2047 so not a huge amount of wasted space.
>
> Failing any other evidence I'll go with what you have until we
> can test this, but the datasheet could be clearer!

When I first read the datasheet it wasn't clear to me either. So I
emailed Michael Hennerich. He confirmed to me that the scaling factor
for degrees is indeed 360 / (2^12- 1). And he told me that the unit
for angular velocity is revolutions per second, and that I should
use a scaling factor of 2PI to convert it to rad/s.

Best regards,
David Veenstra

>
> Jonathan
>> 
>> Jonathan
>> 
>> 
>> > as its unit. So a scaling factor of approximately 2 * Pi is
>> > added to the angular velocity channel.
>> > 
>> > Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
>> > ---
>> > Changes in v3:
>> >  - A decimal approximation of the scale is used, instead
>> >    of a fractional approximation.
>> >  - Remove unneeded explanation of iio_convert_raw_to_processed.
>> > 
>> >  drivers/staging/iio/resolver/ad2s1200.c | 71 +++++++++++++++++++++------------
>> >  1 file changed, 45 insertions(+), 26 deletions(-)
>> > 
>> > diff --git a/drivers/staging/iio/resolver/ad2s1200.c b/drivers/staging/iio/resolver/ad2s1200.c
>> > index 3c7163610ff6..3e1696e52c38 100644
>> > --- a/drivers/staging/iio/resolver/ad2s1200.c
>> > +++ b/drivers/staging/iio/resolver/ad2s1200.c
>> > @@ -57,37 +57,55 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>> >  	struct ad2s1200_state *st = iio_priv(indio_dev);
>> >  	int ret = 0;
>> >  
>> > -	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) {
>> > +	switch (m) {
>> > +	case IIO_CHAN_INFO_SCALE:
>> > +		switch (chan->type) {
>> > +		case IIO_ANGL_VEL:
>> > +			/* 2 * Pi ~= 6.283185 */
>> > +			*val = 6;
>> > +			*val2 = 283185;
>> > +			return IIO_VAL_INT_PLUS_MICRO;
>> > +		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;
>> > +		}
>> > +
>> > +		switch (chan->type) {
>> > +		case IIO_ANGL:
>> > +			*val = be16_to_cpup(&st->rx) >> 4;
>> > +			break;
>> > +		case IIO_ANGL_VEL:
>> > +			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
>> > +			break;
>> > +		default:
>> > +			mutex_unlock(&st->lock);
>> > +			return -EINVAL;
>> > +		}
>> > +
>> > +		/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
>> > +		udelay(1);
>> >  		mutex_unlock(&st->lock);
>> > -		return ret;
>> > -	}
>> >  
>> > -	switch (chan->type) {
>> > -	case IIO_ANGL:
>> > -		*val = be16_to_cpup(&st->rx) >> 4;
>> > -		break;
>> > -	case IIO_ANGL_VEL:
>> > -		*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
>> > -		break;
>> > +		return IIO_VAL_INT;
>> >  	default:
>> > -		mutex_unlock(&st->lock);
>> > -		return -EINVAL;
>> > +		break;
>> >  	}
>> >  
>> > -	/* delay (2 * AD2S1200_TSCLK + 20) ns for sample pulse */
>> > -	udelay(1);
>> > -	mutex_unlock(&st->lock);
>> > -
>> > -	return IIO_VAL_INT;
>> > +	return -EINVAL;
>> >  }
>> >  
>> >  static const struct iio_chan_spec ad2s1200_channels[] = {
>> > @@ -101,6 +119,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),
>> >  	}
>> >  };
>> >    
>> 
>> --
>> 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 v3 9/9] staging: iio: ad2s1200: Move driver out of staging

[David Julian Veenstra]

On 28, April 2018 17:46, Jonathan Cameron wrote:

> On Mon, 23 Apr 2018 00:04:42 +0200
> David Veenstra <davidjulianveenstra@gmail.com> wrote:
>
>> Move the iio driver for the ad2s1200 and ad2s1205 resolver-to-digital
>> converter out of staging, into mainline iio subsystems.
>> 
>> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> A few more minor bits and bobs + suggestions.
>
> Some little things that I missed in earlier patches.  If I haven't
> taken the relevant patch, please roll the fixes in for V4.  If I have
> just do a little follow up patch.
>
> Very nearly there!
>

Thanks, I'll add all the recommendations in V4.

Best regards,
David Veenstra

> thanks,
>
> Jonathan
>
>> ---
>> Changes in v3:
>>  - Add mention of ad2s1205 in commit message.
>> 
>>  drivers/iio/Kconfig                     |   1 +
>>  drivers/iio/Makefile                    |   1 +
>>  drivers/iio/resolver/Kconfig            |  17 +++
>>  drivers/iio/resolver/Makefile           |   5 +
>>  drivers/iio/resolver/ad2s1200.c         | 201 ++++++++++++++++++++++++++++++++
>>  drivers/staging/iio/resolver/Kconfig    |  12 --
>>  drivers/staging/iio/resolver/Makefile   |   1 -
>>  drivers/staging/iio/resolver/ad2s1200.c | 201 --------------------------------
>>  8 files changed, 225 insertions(+), 214 deletions(-)
>>  create mode 100644 drivers/iio/resolver/Kconfig
>>  create mode 100644 drivers/iio/resolver/Makefile
>>  create mode 100644 drivers/iio/resolver/ad2s1200.c
>>  delete mode 100644 drivers/staging/iio/resolver/ad2s1200.c
>> 
>> 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/iio/resolver/ad2s1200.c b/drivers/iio/resolver/ad2s1200.c
>> new file mode 100644
>> index 000000000000..d2b62308b31d
>> --- /dev/null
>> +++ b/drivers/iio/resolver/ad2s1200.c
>> @@ -0,0 +1,201 @@
>> +/*
>> + * ad2s1200.c simple support for the ADI Resolver to Digital Converters:
>> + * AD2S1200/1205
>> + *
>> + * Copyright (c) 2010-2010 Analog Devices Inc.
>
> I think you have done enough changes in here that if you want to add your
> own copyright I think it would be justified.
>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>
> One of my pet hates if you want to clean it up :)
> No point in having a blank line above here.
>
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#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/spi/spi.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/types.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define DRV_NAME "ad2s1200"
>> +
>> +/* input clock on serial interface */
>> +#define AD2S1200_HZ	8192000
>> +/* clock period in nano second */
>> +#define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
>> +
>> +/**
>> + * struct ad2s1200_state - driver instance specific data.
>> + * @lock:	protects both the GPIO pins and the rx buffer, to prevent
>> + *		concurrent spi transactions.
>> + * @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;
>> +	struct gpio_desc *sample;
>> +	struct gpio_desc *rdvel;
>> +	__be16 rx ____cacheline_aligned;
>> +};
>> +
>> +static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int *val,
>> +			     int *val2,
>> +			     long m)
>> +{
>> +	struct ad2s1200_state *st = iio_priv(indio_dev);
>> +	int ret = 0;
> We only return ret in paths that set it. So doesn't need
> initializing any more.  This might want to be pushed down
> to the earlier patch changing this code.
>> +
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->type) {
>> +		case IIO_ANGL:
>> +			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
>> +			*val = 0;
>> +			*val2 = 1534355;
>> +			return IIO_VAL_INT_PLUS_NANO;
>> +		case IIO_ANGL_VEL:
>> +			/* 2 * Pi ~= 6.283185 */
>> +			*val = 6;
>> +			*val2 = 283185;
>> +			return IIO_VAL_INT_PLUS_MICRO;
>> +		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;
>> +		}
>> +
>> +		switch (chan->type) {
>> +		case IIO_ANGL:
>> +			*val = be16_to_cpup(&st->rx) >> 4;
>> +			break;
>> +		case IIO_ANGL_VEL:
>> +			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
>> +			break;
>> +		default:
>> +			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;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_chan_spec ad2s1200_channels[] = {
>> +	{
>> +		.type = IIO_ANGL,
>> +		.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,
>> +		.channel = 0,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>> +	}
>> +};
>> +
>> +static const struct iio_info ad2s1200_info = {
>> +	.read_raw = ad2s1200_read_raw,
>> +};
>> +
>> +static int ad2s1200_probe(struct spi_device *spi)
>> +{
>> +	struct ad2s1200_state *st;
>> +	struct iio_dev *indio_dev;
>> +	int ret = 0;
> I guess this is left from somewhere and should have been in
> an earlier patch, but there is no need to set to 0 and
> it is set in all paths.
>
> Mind you I've suggested you get rid of the only user
> anyway so that won't matter as this will go away anyway.
>
>> +
>> +	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);
>> +	st->sdev = spi;
>> +
>> +	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
>> +	if (IS_ERR(st->sample)) {
>> +		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
>> +			PTR_ERR(st->sample));
>> +		return PTR_ERR(st->sample);
>> +	}
>> +
>> +	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
>> +	if (IS_ERR(st->rdvel)) {
>> +		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
>> +			PTR_ERR(st->rdvel));
>> +		return PTR_ERR(st->rdvel);
>> +	}
>> +
>> +	indio_dev->dev.parent = &spi->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);
>> +	if (ret)
>> +		return ret;
>> +
>> +	spi->max_speed_hz = AD2S1200_HZ;
>> +	spi->mode = SPI_MODE_3;
>> +	spi_setup(spi);
> This ordering is racy.  We could in theory have been
> talking to the device after it's interfaces were exposed
> in the devm_iio_device_register call.
>
> Reorder this to put them before that then just do
> return devm_iio_device_register
> to finish up probe.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct spi_device_id ad2s1200_id[] = {
>> +	{ "ad2s1200" },
> Worth putting in an explicit device tree table for matching
> with the vendor ID.  The intent long term is to remove
> the nasty code that leads to fall back matching with the
> spi_device_id table.
>
> No reason this needs to happen before taking it out of staging.
>
>> +	{ "ad2s1205" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(spi, ad2s1200_id);
>> +
>> +static struct spi_driver ad2s1200_driver = {
>> +	.driver = {
>> +		.name = DRV_NAME,
>> +	},
>> +	.probe = ad2s1200_probe,
>> +	.id_table = ad2s1200_id,
>> +};
>> +module_spi_driver(ad2s1200_driver);
>> +
>> +MODULE_AUTHOR("Graff Yang <graff.yang@gmail.com>");
>> +MODULE_DESCRIPTION("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver");
>> +MODULE_LICENSE("GPL v2");
> ...

Re: [PATCH v3 9/9] staging: iio: ad2s1200: Move driver out of staging

[David Julian Veenstra]

On 28, April 2018 17:46, Jonathan Cameron wrote:

> On Mon, 23 Apr 2018 00:04:42 +0200
> David Veenstra <davidjulianveenstra@gmail.com> wrote:
>
>> Move the iio driver for the ad2s1200 and ad2s1205 resolver-to-digital
>> converter out of staging, into mainline iio subsystems.
>> 
>> Signed-off-by: David Veenstra <davidjulianveenstra@gmail.com>
> A few more minor bits and bobs + suggestions.
>
> Some little things that I missed in earlier patches.  If I haven't
> taken the relevant patch, please roll the fixes in for V4.  If I have
> just do a little follow up patch.
>
> Very nearly there!
>

Thanks, I'll add all the recommendations in V4.

Best regards,
David Veenstra

> thanks,
>
> Jonathan
>
>> ---
>> Changes in v3:
>>  - Add mention of ad2s1205 in commit message.
>> 
>>  drivers/iio/Kconfig                     |   1 +
>>  drivers/iio/Makefile                    |   1 +
>>  drivers/iio/resolver/Kconfig            |  17 +++
>>  drivers/iio/resolver/Makefile           |   5 +
>>  drivers/iio/resolver/ad2s1200.c         | 201 ++++++++++++++++++++++++++++++++
>>  drivers/staging/iio/resolver/Kconfig    |  12 --
>>  drivers/staging/iio/resolver/Makefile   |   1 -
>>  drivers/staging/iio/resolver/ad2s1200.c | 201 --------------------------------
>>  8 files changed, 225 insertions(+), 214 deletions(-)
>>  create mode 100644 drivers/iio/resolver/Kconfig
>>  create mode 100644 drivers/iio/resolver/Makefile
>>  create mode 100644 drivers/iio/resolver/ad2s1200.c
>>  delete mode 100644 drivers/staging/iio/resolver/ad2s1200.c
>> 
>> 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/iio/resolver/ad2s1200.c b/drivers/iio/resolver/ad2s1200.c
>> new file mode 100644
>> index 000000000000..d2b62308b31d
>> --- /dev/null
>> +++ b/drivers/iio/resolver/ad2s1200.c
>> @@ -0,0 +1,201 @@
>> +/*
>> + * ad2s1200.c simple support for the ADI Resolver to Digital Converters:
>> + * AD2S1200/1205
>> + *
>> + * Copyright (c) 2010-2010 Analog Devices Inc.
>
> I think you have done enough changes in here that if you want to add your
> own copyright I think it would be justified.
>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>
> One of my pet hates if you want to clean it up :)
> No point in having a blank line above here.
>
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#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/spi/spi.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/types.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define DRV_NAME "ad2s1200"
>> +
>> +/* input clock on serial interface */
>> +#define AD2S1200_HZ	8192000
>> +/* clock period in nano second */
>> +#define AD2S1200_TSCLK	(1000000000 / AD2S1200_HZ)
>> +
>> +/**
>> + * struct ad2s1200_state - driver instance specific data.
>> + * @lock:	protects both the GPIO pins and the rx buffer, to prevent
>> + *		concurrent spi transactions.
>> + * @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;
>> +	struct gpio_desc *sample;
>> +	struct gpio_desc *rdvel;
>> +	__be16 rx ____cacheline_aligned;
>> +};
>> +
>> +static int ad2s1200_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int *val,
>> +			     int *val2,
>> +			     long m)
>> +{
>> +	struct ad2s1200_state *st = iio_priv(indio_dev);
>> +	int ret = 0;
> We only return ret in paths that set it. So doesn't need
> initializing any more.  This might want to be pushed down
> to the earlier patch changing this code.
>> +
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->type) {
>> +		case IIO_ANGL:
>> +			/* 2 * Pi / (2^12 - 1) ~= 0.001534355 */
>> +			*val = 0;
>> +			*val2 = 1534355;
>> +			return IIO_VAL_INT_PLUS_NANO;
>> +		case IIO_ANGL_VEL:
>> +			/* 2 * Pi ~= 6.283185 */
>> +			*val = 6;
>> +			*val2 = 283185;
>> +			return IIO_VAL_INT_PLUS_MICRO;
>> +		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;
>> +		}
>> +
>> +		switch (chan->type) {
>> +		case IIO_ANGL:
>> +			*val = be16_to_cpup(&st->rx) >> 4;
>> +			break;
>> +		case IIO_ANGL_VEL:
>> +			*val = sign_extend32(be16_to_cpup(&st->rx) >> 4, 11);
>> +			break;
>> +		default:
>> +			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;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_chan_spec ad2s1200_channels[] = {
>> +	{
>> +		.type = IIO_ANGL,
>> +		.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,
>> +		.channel = 0,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>> +	}
>> +};
>> +
>> +static const struct iio_info ad2s1200_info = {
>> +	.read_raw = ad2s1200_read_raw,
>> +};
>> +
>> +static int ad2s1200_probe(struct spi_device *spi)
>> +{
>> +	struct ad2s1200_state *st;
>> +	struct iio_dev *indio_dev;
>> +	int ret = 0;
> I guess this is left from somewhere and should have been in
> an earlier patch, but there is no need to set to 0 and
> it is set in all paths.
>
> Mind you I've suggested you get rid of the only user
> anyway so that won't matter as this will go away anyway.
>
>> +
>> +	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);
>> +	st->sdev = spi;
>> +
>> +	st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_OUT_LOW);
>> +	if (IS_ERR(st->sample)) {
>> +		dev_err(&spi->dev, "Failed to claim SAMPLE gpio: err=%ld\n",
>> +			PTR_ERR(st->sample));
>> +		return PTR_ERR(st->sample);
>> +	}
>> +
>> +	st->rdvel = devm_gpiod_get(&spi->dev, "rdvel", GPIOD_OUT_LOW);
>> +	if (IS_ERR(st->rdvel)) {
>> +		dev_err(&spi->dev, "Failed to claim RDVEL gpio: err=%ld\n",
>> +			PTR_ERR(st->rdvel));
>> +		return PTR_ERR(st->rdvel);
>> +	}
>> +
>> +	indio_dev->dev.parent = &spi->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);
>> +	if (ret)
>> +		return ret;
>> +
>> +	spi->max_speed_hz = AD2S1200_HZ;
>> +	spi->mode = SPI_MODE_3;
>> +	spi_setup(spi);
> This ordering is racy.  We could in theory have been
> talking to the device after it's interfaces were exposed
> in the devm_iio_device_register call.
>
> Reorder this to put them before that then just do
> return devm_iio_device_register
> to finish up probe.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct spi_device_id ad2s1200_id[] = {
>> +	{ "ad2s1200" },
> Worth putting in an explicit device tree table for matching
> with the vendor ID.  The intent long term is to remove
> the nasty code that leads to fall back matching with the
> spi_device_id table.
>
> No reason this needs to happen before taking it out of staging.
>
>> +	{ "ad2s1205" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(spi, ad2s1200_id);
>> +
>> +static struct spi_driver ad2s1200_driver = {
>> +	.driver = {
>> +		.name = DRV_NAME,
>> +	},
>> +	.probe = ad2s1200_probe,
>> +	.id_table = ad2s1200_id,
>> +};
>> +module_spi_driver(ad2s1200_driver);
>> +
>> +MODULE_AUTHOR("Graff Yang <graff.yang@gmail.com>");
>> +MODULE_DESCRIPTION("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver");
>> +MODULE_LICENSE("GPL v2");
> ...