staging: iio: ad2s1200: Driver clean up

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

Signed-off-by David Veenstra <[email protected]>

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

drivers/staging/iio/resolver/ad2s1200.c +8 -5

index 00efba598671..eceb86e952de 100644
@@ -118,19 +118,22 @@ static int ad2s1200_probe(struct spi_device *spi)
 	unsigned short *pins = spi->dev.platform_data;
 	struct ad2s1200_state *st;
 	struct iio_dev *indio_dev;
+	struct device *dev;
 	int pn, ret = 0;
 
+	dev = &spi->dev;
+
 	for (pn = 0; pn < AD2S1200_PN; pn++) {
-		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
+		ret = devm_gpio_request_one(dev, pins[pn], GPIOF_DIR_OUT,
 					    DRV_NAME);
 		if (ret) {
-			dev_err(&spi->dev, "request gpio pin %d failed\n",
+			dev_err(dev, "request gpio pin %d failed\n",
 				pins[pn]);
 			return ret;
 		}
 	}
 
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 

@@ -141,14 +144,14 @@ static int ad2s1200_probe(struct spi_device *spi)
 	st->sample = pins[0];
 	st->rdvel = pins[1];
 
-	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.parent = dev;
 	indio_dev->info = &ad2s1200_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = ad2s1200_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ad2s1200_channels);
 	indio_dev->name = spi_get_device_id(spi)->name;
 
-	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret)
 		return ret;
 

Quoted 3 lines · On Sun, 18 Mar 2018 14:35:46 +0100 David Veenstra…

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

Quoted 1 line

Signed-off-by David Veenstra <[email protected]>

I'm a little unconvinced by this one. It adds code to save a really very trivial bit of repetition. If it had been via another pointer or this would have made a big difference in number of lines by bringing a lot of entries below 80 chars that weren't previous, then there would be a good argument.

Here, not so much.

Jonathan

Quoted 51 lines

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

drivers/staging/iio/resolver/ad2s1200.c +8 -5

index 00efba598671..eceb86e952de 100644
@@ -118,19 +118,22 @@ static int ad2s1200_probe(struct spi_device *spi)
 	unsigned short *pins = spi->dev.platform_data;
 	struct ad2s1200_state *st;
 	struct iio_dev *indio_dev;
+	struct device *dev;
 	int pn, ret = 0;
 
+	dev = &spi->dev;
+
 	for (pn = 0; pn < AD2S1200_PN; pn++) {
-		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
+		ret = devm_gpio_request_one(dev, pins[pn], GPIOF_DIR_OUT,
 					    DRV_NAME);
 		if (ret) {
-			dev_err(&spi->dev, "request gpio pin %d failed\n",
+			dev_err(dev, "request gpio pin %d failed\n",
 				pins[pn]);
 			return ret;
 		}
 	}
 
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 

@@ -141,14 +144,14 @@ static int ad2s1200_probe(struct spi_device *spi)
 	st->sample = pins[0];
 	st->rdvel = pins[1];
 
-	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.parent = dev;
 	indio_dev->info = &ad2s1200_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = ad2s1200_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ad2s1200_channels);
 	indio_dev->name = spi_get_device_id(spi)->name;
 
-	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret)
 		return ret;
 

Quoted 13 lines · Jonathan Cameron

Quoted 4 lines · On Sun, 18 Mar 2018 14:35:46 +0100 David Veenstra…

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

Signed-off-by David Veenstra <[email protected]>

I'm a little unconvinced by this one. It adds code to save a really very trivial bit of repetition. If it had been via another pointer or this would have made a big difference in number of lines by bringing a lot of entries below 80 chars that weren't previous, then there would be a good argument.

Quoted 1 line

Here, not so much.

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

Best regards,
David Veenstra

Quoted 48 lines

Jonathan

Quoted 46 lines

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

drivers/staging/iio/resolver/ad2s1200.c +7 -4

index 00efba598671..eceb86e952de 100644
@@ -118,19 +118,22 @@ static int ad2s1200_probe(struct spi_device *spi)
 	unsigned short *pins = spi->dev.platform_data;
 	struct ad2s1200_state *st;
 	struct iio_dev *indio_dev;
+	struct device *dev;
 	int pn, ret = 0;
 
+	dev = &spi->dev;
+
 	for (pn = 0; pn < AD2S1200_PN; pn++) {
-		ret = devm_gpio_request_one(&spi->dev, pins[pn], GPIOF_DIR_OUT,
+		ret = devm_gpio_request_one(dev, pins[pn], GPIOF_DIR_OUT,
 					    DRV_NAME);
 		if (ret) {
-			dev_err(&spi->dev, "request gpio pin %d failed\n",
+			dev_err(dev, "request gpio pin %d failed\n",
 				pins[pn]);
 			return ret;
 		}
 	}
 
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 

@@ -141,14 +144,14 @@ static int ad2s1200_probe(struct spi_device *spi)
 	st->sample = pins[0];
 	st->rdvel = pins[1];
 
-	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.parent = dev;
 	indio_dev->info = &ad2s1200_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = ad2s1200_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ad2s1200_channels);
 	indio_dev->name = spi_get_device_id(spi)->name;
 

Quoted 5 lines

drivers/staging/iio/resolver/ad2s1200.c static int ad2s1200_probe(struct spi_device *spi) · line 157

Quoted 5 lines

drivers/staging/iio/resolver/ad2s1200.c +1 -1

-	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret)
 		return ret;
 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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

Signed-off-by David Veenstra <[email protected]>

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

drivers/staging/iio/resolver/ad2s1200.c +5 -3

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

Quoted 5 lines · On Sun, 18 Mar 2018 14:36:15 +0100 David Veenstra…

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

Signed-off-by David Veenstra <[email protected]>

Given this one can't actually happen as the chan->type is always one of the two options, I can't see the point in making sure we sleep.

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

So I am unconvinced on this patch.

Jonathan

Quoted 34 lines

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

drivers/staging/iio/resolver/ad2s1200.c +5 -3

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

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

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

Signed-off-by David Veenstra <[email protected]>

---
 drivers/staging/iio/resolver/ad2s1200.c | 51 ++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 14 deletions(-)

drivers/staging/iio/resolver/ad2s1200.c +37 -14

index 6ce9ca13094a..7ee1d9f76dfb 100644
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>

@@ -45,8 +46,8 @@
 struct ad2s1200_state {
 	struct mutex lock;
 	struct spi_device *sdev;
-	int sample;
-	int rdvel;
+	struct gpio_desc *sample;
+	struct gpio_desc *rdvel;
 	u8 rx[2] ____cacheline_aligned;
 };
 

@@ -61,12 +62,12 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	u16 vel;
 
 	mutex_lock(&st->lock);
-	gpio_set_value(st->sample, 0);
+	gpiod_set_value(st->sample, 0);
 
 	/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
 	udelay(1);
-	gpio_set_value(st->sample, 1);
-	gpio_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
+	gpiod_set_value(st->sample, 1);
+	gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
 
 	ret = spi_read(st->sdev, st->rx, 2);
 	if (ret < 0) {

@@ -124,13 +125,18 @@ static int ad2s1200_probe(struct spi_device *spi)
 
 	dev = &spi->dev;
 
-	for (pn = 0; pn < AD2S1200_PN; pn++) {
-		ret = devm_gpio_request_one(dev, pins[pn], GPIOF_DIR_OUT,
-					    DRV_NAME);
-		if (ret) {
-			dev_err(dev, "request gpio pin %d failed\n",
-				pins[pn]);
-			return ret;
+	if (pins) {
+		for (pn = 0; pn < AD2S1200_PN; pn++) {
+			ret = devm_gpio_request_one(dev, pins[pn],
+						    GPIOF_DIR_OUT,
+						    DRV_NAME);
+			if (ret) {
+				dev_err(dev,
+					"Failed to claim gpio %d\n: err=%d",
+					pins[pn],
+					ret);
+				return ret;
+			}
 		}
 	}
 

@@ -142,8 +148,25 @@ static int ad2s1200_probe(struct spi_device *spi)
 	st = iio_priv(indio_dev);
 	mutex_init(&st->lock);
 	st->sdev = spi;
-	st->sample = pins[0];
-	st->rdvel = pins[1];
+
+	if (pins) {
+		st->sample = gpio_to_desc(pins[0]);
+		st->rdvel = gpio_to_desc(pins[1]);
+	} else {
+		st->sample = devm_gpiod_get(dev, "sample", GPIOD_OUT_LOW);
+		if (IS_ERR(st->sample)) {
+			dev_err(dev, "Failed to claim SAMPLE gpio: err=%ld\n",
+				PTR_ERR(st->sample));
+			return PTR_ERR(st->sample);
+		}
+
+		st->rdvel = devm_gpiod_get(dev, "rdvel", GPIOD_OUT_LOW);
+		if (IS_ERR(st->rdvel)) {
+			dev_err(dev, "Failed to claim RDVEL gpio: err=%ld\n",
+				PTR_ERR(st->rdvel));
+			return PTR_ERR(st->rdvel);
+		}
+	}
 
 	indio_dev->dev.parent = dev;
 	indio_dev->info = &ad2s1200_info;

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

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

Signed-off-by David Veenstra <[email protected]>

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

drivers/staging/iio/resolver/ad2s1200.c +8 -3

index 4b97a106012c..937676bcc0d4 100644
@@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
+		case IIO_INCLI:
+			*val = 360;
+			*val2 = 0xFFF;
+			return IIO_VAL_FRACTIONAL;
 		case IIO_ANGL_VEL:
 			/*
 			 * 2 * Pi is almost equal to

@@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
 		udelay(1);
 		gpiod_set_value(st->sample, 1);
-		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
+		gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI));
 
 		ret = spi_read(st->sdev, st->rx, 2);
 		if (ret < 0) {

@@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 		vel = be16_to_cpup((__be16 *)st->rx);
 		switch (chan->type) {
-		case IIO_ANGL:
+		case IIO_INCLI:
 			*val = vel >> 4;
 			ret = IIO_VAL_INT;
 			break;

@@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 static const struct iio_chan_spec ad2s1200_channels[] = {
 	{
-		.type = IIO_ANGL,
+		.type = IIO_INCLI,
 		.indexed = 1,
 		.channel = 0,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 	}, {
 		.type = IIO_ANGL_VEL,
 		.indexed = 1,

Quoted 8 lines · On Sun, 18 Mar 2018 14:37:04 +0100 David Veenstra…

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

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

Signed-off-by David Veenstra <[email protected]>

No, please do not blugeon a device into the existing documented ABI.

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

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

Jonathan

Quoted 50 lines

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

drivers/staging/iio/resolver/ad2s1200.c +8 -3

index 4b97a106012c..937676bcc0d4 100644
@@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
+		case IIO_INCLI:
+			*val = 360;
+			*val2 = 0xFFF;
+			return IIO_VAL_FRACTIONAL;
 		case IIO_ANGL_VEL:
 			/*
 			 * 2 * Pi is almost equal to

@@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
 		udelay(1);
 		gpiod_set_value(st->sample, 1);
-		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
+		gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI));
 
 		ret = spi_read(st->sdev, st->rx, 2);
 		if (ret < 0) {

@@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 		vel = be16_to_cpup((__be16 *)st->rx);
 		switch (chan->type) {
-		case IIO_ANGL:
+		case IIO_INCLI:
 			*val = vel >> 4;
 			ret = IIO_VAL_INT;
 			break;

@@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 static const struct iio_chan_spec ad2s1200_channels[] = {
 	{
-		.type = IIO_ANGL,
+		.type = IIO_INCLI,
 		.indexed = 1,
 		.channel = 0,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 	}, {
 		.type = IIO_ANGL_VEL,
 		.indexed = 1,

Quoted 17 lines · Jonathan Cameron

Quoted 8 lines · On Sun, 18 Mar 2018 14:37:04 +0100 David Veenstra…

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

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

Signed-off-by David Veenstra <[email protected]>

No, please do not blugeon a device into the existing documented ABI.

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

Quoted 2 lines

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

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

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

Best regards,
David Veenstra

Quoted 42 lines

Jonathan

Quoted 39 lines

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

drivers/staging/iio/resolver/ad2s1200.c +6 -2

index 4b97a106012c..937676bcc0d4 100644
@@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
+		case IIO_INCLI:
+			*val = 360;
+			*val2 = 0xFFF;
+			return IIO_VAL_FRACTIONAL;
 		case IIO_ANGL_VEL:
 			/*
 			 * 2 * Pi is almost equal to

@@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
 		udelay(1);
 		gpiod_set_value(st->sample, 1);
-		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
+		gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI));
 
 		ret = spi_read(st->sdev, st->rx, 2);
 		if (ret < 0) {

@@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 		vel = be16_to_cpup((__be16 *)st->rx);
 		switch (chan->type) {
-		case IIO_ANGL:
+		case IIO_INCLI:
 			*val = vel >> 4;
 			ret = IIO_VAL_INT;
 			break;

@@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 

Quoted 11 lines

drivers/staging/iio/resolver/ad2s1200.c static int ad2s1200_read_raw(struct iio_dev *indio_dev, · line 142

Quoted 11 lines

drivers/staging/iio/resolver/ad2s1200.c +2 -1

 static const struct iio_chan_spec ad2s1200_channels[] = {
 	{
-		.type = IIO_ANGL,
+		.type = IIO_INCLI,
 		.indexed = 1,
 		.channel = 0,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 	}, {
 		.type = IIO_ANGL_VEL,
 		.indexed = 1,  

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 19 lines · On Sat, 24 Mar 2018 13:36:44 +0100 David Julian Veenstra…

Quoted 17 lines · Jonathan Cameron

Quoted 8 lines · On Sun, 18 Mar 2018 14:37:04 +0100 David Veenstra…

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

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

Signed-off-by David Veenstra <[email protected]>

No, please do not blugeon a device into the existing documented ABI.

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

Quoted 6 lines

Quoted 2 lines

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

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

Ah, no I was making the point we need to match with the 'units' not use the channel type.

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

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

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

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

Jonathan

Quoted 66 lines

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

Best regards,
David Veenstra

Quoted 42 lines

Jonathan

Quoted 39 lines

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

drivers/staging/iio/resolver/ad2s1200.c +6 -2

index 4b97a106012c..937676bcc0d4 100644
@@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
+		case IIO_INCLI:
+			*val = 360;
+			*val2 = 0xFFF;
+			return IIO_VAL_FRACTIONAL;
 		case IIO_ANGL_VEL:
 			/*
 			 * 2 * Pi is almost equal to

@@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
 		udelay(1);
 		gpiod_set_value(st->sample, 1);
-		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
+		gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI));
 
 		ret = spi_read(st->sdev, st->rx, 2);
 		if (ret < 0) {

@@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 		vel = be16_to_cpup((__be16 *)st->rx);
 		switch (chan->type) {
-		case IIO_ANGL:
+		case IIO_INCLI:
 			*val = vel >> 4;
 			ret = IIO_VAL_INT;
 			break;

@@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 

Quoted 11 lines

drivers/staging/iio/resolver/ad2s1200.c static int ad2s1200_read_raw(struct iio_dev *indio_dev, · line 142

Quoted 11 lines

drivers/staging/iio/resolver/ad2s1200.c +2 -1

 static const struct iio_chan_spec ad2s1200_channels[] = {
 	{
-		.type = IIO_ANGL,
+		.type = IIO_INCLI,
 		.indexed = 1,
 		.channel = 0,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 	}, {
 		.type = IIO_ANGL_VEL,
 		.indexed = 1,  

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 34 lines · Jonathan Cameron

Quoted 19 lines · On Sat, 24 Mar 2018 13:36:44 +0100 David Julian Veenstra…

Quoted 17 lines · Jonathan Cameron

Quoted 8 lines · On Sun, 18 Mar 2018 14:37:04 +0100 David Veenstra…

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

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

Signed-off-by David Veenstra <[email protected]>

No, please do not blugeon a device into the existing documented ABI.

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

Quoted 6 lines

Quoted 2 lines

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

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

Ah, no I was making the point we need to match with the 'units' not use the channel type.

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

Quoted 7 lines

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

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

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

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

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

Best regards,
David Veenstra

Quoted 72 lines

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

Jonathan

Quoted 66 lines

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

Best regards,
David Veenstra

Quoted 42 lines

Jonathan

Quoted 39 lines

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

drivers/staging/iio/resolver/ad2s1200.c +6 -2

index 4b97a106012c..937676bcc0d4 100644
@@ -64,6 +64,10 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
+		case IIO_INCLI:
+			*val = 360;
+			*val2 = 0xFFF;
+			return IIO_VAL_FRACTIONAL;
 		case IIO_ANGL_VEL:
 			/*
 			 * 2 * Pi is almost equal to

@@ -91,7 +95,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 		/* delay (6 * AD2S1200_TSCLK + 20) nano seconds */
 		udelay(1);
 		gpiod_set_value(st->sample, 1);
-		gpiod_set_value(st->rdvel, !!(chan->type == IIO_ANGL));
+		gpiod_set_value(st->rdvel, !!(chan->type == IIO_INCLI));
 
 		ret = spi_read(st->sdev, st->rx, 2);
 		if (ret < 0) {

@@ -101,7 +105,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 
 		vel = be16_to_cpup((__be16 *)st->rx);
 		switch (chan->type) {
-		case IIO_ANGL:
+		case IIO_INCLI:
 			*val = vel >> 4;
 			ret = IIO_VAL_INT;
 			break;

@@ -128,10 +132,11 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
 

Quoted 11 lines

drivers/staging/iio/resolver/ad2s1200.c static int ad2s1200_read_raw(struct iio_dev *indio_dev, · line 142

Quoted 11 lines

drivers/staging/iio/resolver/ad2s1200.c +2 -1

 static const struct iio_chan_spec ad2s1200_channels[] = {
 	{
-		.type = IIO_ANGL,
+		.type = IIO_INCLI,
 		.indexed = 1,
 		.channel = 0,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 	}, {
 		.type = IIO_ANGL_VEL,
 		.indexed = 1,    

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signed-off-by David Veenstra <[email protected]>

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

drivers/iio/Kconfig +1 -0

index b3c8c6ef0dff..4bec3ccbf4a1 100644
@@ -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

drivers/iio/Makefile +1 -0

index b16b2e9ddc40..1865361b8714 100644
@@ -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/

drivers/iio/resolver/Kconfig new file +17 -0

new file mode 100644
index 000000000000..2ced9f22aa70
@@ -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

drivers/iio/resolver/Makefile new file +5 -0

new file mode 100644
index 000000000000..4e1dccae07e7
@@ -0,0 +1,5 @@
+#
+# Makefile for Resolver/Synchro drivers
+#
+
+obj-$(CONFIG_AD2S1200) += ad2s1200.o

drivers/iio/resolver/ad2s1200.c renamed from drivers/staging/iio/resolver/ad2s1200.c +0 -0

similarity index 100%
rename from drivers/staging/iio/resolver/ad2s1200.c
rename to drivers/iio/resolver/ad2s1200.c

drivers/staging/iio/resolver/Kconfig +0 -12

index 1c7e2860d6b7..6a469ee6101f 100644
@@ -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

drivers/staging/iio/resolver/Makefile +0 -1

index 14375e444ebf..8d901dc7500b 100644
@@ -3,5 +3,4 @@
 #
 
 obj-$(CONFIG_AD2S90) += ad2s90.o
-obj-$(CONFIG_AD2S1200) += ad2s1200.o
 obj-$(CONFIG_AD2S1210) += ad2s1210.o