staging: iio: ad2s1200: Driver clean up
Subsystems (5)
v3 cover staging: iio: ad2s1200: Driver clean up
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
Signature · 1 line
2.16.2
PATCH v3 1/9 staging: iio: ad2s1200: Add kernel docs to driver state
Add missing kernel docs to the ad2s1200 driver state.
---
Changes in v3:
- Added more explanation to mutex lock.
drivers/staging/iio/resolver/ad2s1200.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/staging/iio/resolver/ad2s1200.c +9 -0
@@ -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;
Signature · 1 line
2.16.2
Jonathan Cameron
maintainerRe: [PATCH v3 1/9] staging: iio: ad2s1200: Add kernel docs to driver state
Quoted 3 lines · On Mon, 23 Apr 2018 00:02:51 +0200 David Veenstra…
Add missing kernel docs to the ad2s1200 driver state.
Signed-off-by David Veenstra <[email protected]>
Hi David,
Comment inline.
Quoted 12 lines
---
Changes in v3:
- Added more explanation to mutex lock.drivers/staging/iio/resolver/ad2s1200.c | 9 +++++++++ 1 file changed, 9 insertions(+)drivers/staging/iio/resolver/ad2s1200.c +0
-0
drivers/staging/iio/resolver/ad2s1200.c line 39
Quoted 7 lines
drivers/staging/iio/resolver/ad2s1200.c +4
-0@@ -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
Quoted 8 lines
drivers/staging/iio/resolver/ad2s1200.c +5
-0+* @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
Quoted 34 lines · Jonathan Cameron
Quoted 3 lines · On Mon, 23 Apr 2018 00:02:51 +0200 David Veenstra…
Add missing kernel docs to the ad2s1200 driver state.
Signed-off-by David Veenstra <[email protected]>Hi David,
Comment inline.
Quoted 12 lines
---
Changes in v3:
- Added more explanation to mutex lock.drivers/staging/iio/resolver/ad2s1200.c | 9 +++++++++ 1 file changed, 9 insertions(+)drivers/staging/iio/resolver/ad2s1200.c +0
-0drivers/staging/iio/resolver/ad2s1200.c line 39
Quoted 7 lines
drivers/staging/iio/resolver/ad2s1200.c +4
-0@@ -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.
Quoted 6 lines
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
Quoted 13 lines
Thanks,
Jonathan
Quoted 8 lines
drivers/staging/iio/resolver/ad2s1200.c +5
-0+* @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;
PATCH v3 2/9 staging: iio: ad2s1200: Improve readability with be16_to_cpup
In reply to David VeenstraThe 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.
---
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(-)
drivers/staging/iio/resolver/ad2s1200.c +4 -7
@@ -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);
Signature · 1 line
2.16.2
Jonathan Cameron
maintainerRe: [PATCH v3 2/9] staging: iio: ad2s1200: Improve readability with be16_to_cpup
Quoted 9 lines · On Mon, 23 Apr 2018 00:03:03 +0200 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 <[email protected]>
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
Quoted 54 lines
---
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(-)drivers/staging/iio/resolver/ad2s1200.c +4
-7@@ -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);
PATCH v3 3/9 staging: iio: ad2s1200: Replace legacy gpio API with modern API
In reply to David VeenstraThe legacy, integer based gpio API is replaced with the modern descriptor based API.
---
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(-)
drivers/staging/iio/resolver/ad2s1200.c +8 -7
@@ -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;
Signature · 1 line
2.16.2
Jonathan Cameron
maintainerRe: [PATCH v3 3/9] staging: iio: ad2s1200: Replace legacy gpio API with modern API
Quoted 4 lines · On Mon, 23 Apr 2018 00:03:16 +0200 David Veenstra…
The legacy, integer based gpio API is replaced with the modern descriptor based API.
Signed-off-by David Veenstra <[email protected]>
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
Quoted 58 lines
---
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(-)drivers/staging/iio/resolver/ad2s1200.c +8
-7@@ -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;
PATCH v3 4/9 staging: iio: ad2s1200: Replace platform data with dt bindings
In reply to David VeenstraRemove usage of platform data, and replace it with device tree facilities.
---
Changes in v3:
- Introduced in this version.
drivers/staging/iio/resolver/ad2s1200.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)
drivers/staging/iio/resolver/ad2s1200.c +15 -17
@@ -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;
Signature · 1 line
2.16.2
Jonathan Cameron
maintainerRe: [PATCH v3 4/9] staging: iio: ad2s1200: Replace platform data with dt bindings
Quoted 4 lines · On Mon, 23 Apr 2018 00:03:32 +0200 David Veenstra…
Remove usage of platform data, and replace it with device tree facilities.
Signed-off-by David Veenstra <[email protected]>
This is fine up to the need to add the prefix to the gpio names.
Thanks,
Jonathan
Quoted 66 lines
---
Changes in v3:
- Introduced in this version.drivers/staging/iio/resolver/ad2s1200.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)drivers/staging/iio/resolver/ad2s1200.c +15
-17@@ -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;
PATCH v3 5/9 staging: iio: ad2s1200: Add documentation for device tree binding
In reply to David VeenstraAdd documentation for the added device tree bindings.
---
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
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +16 -0
@@ -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>; + };
Signature · 1 line
2.16.2
Rob Herring
maintainerRe: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding
Quoted 19 lines · David Veenstra
Add documentation for the added device tree bindings.
Signed-off-by David Veenstra <[email protected]>---
Changes in v3:
- Documentation is added toDocumentation/devicetree/bindings/iio/resolverinstead 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.txtDocumentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +0
-0
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt line 7
Quoted 7 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +7
-0@@ -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.
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt line 11
Quoted 4 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +4
-0+ +Example: + + resolver {
Missing unit address.
Quoted 8 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +5
-0+ compatible = "adi,ad2s1200"; + reg = <4>; + sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>; + rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>; + };Signature · 1 line
2.16.2
Jonathan Cameron
maintainerRe: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding
Quoted 23 lines · On Fri, 27 Apr 2018 09:48:20 -0500 Rob Herring <[email protected]> wrote:
Quoted 22 lines · David Veenstra
Add documentation for the added device tree bindings.
Signed-off-by David Veenstra <[email protected]>---
Changes in v3:
- Documentation is added toDocumentation/devicetree/bindings/iio/resolverinstead 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.txtDocumentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +3
-0@@ -0,0 +1,16 @@ +Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter + +Required properties:
Quoted 8 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt line 7
Quoted 4 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +4
-0+ - 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 AD2S1200Are 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
Quoted 16 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt line 11
Quoted 5 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +4
-0+ +Example: + + resolver {Missing unit address.
Quoted 8 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +5
-0+ compatible = "adi,ad2s1200"; + reg = <4>; + sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>; + rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>; + };Signature · 1 line
2.16.2
Re: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding
Quoted 35 lines · Jonathan Cameron
Quoted 23 lines · On Fri, 27 Apr 2018 09:48:20 -0500 Rob Herring <[email protected]> wrote:
Quoted 22 lines · David Veenstra
Add documentation for the added device tree bindings.
Signed-off-by David Veenstra <[email protected]>---
Changes in v3:
- Documentation is added toDocumentation/devicetree/bindings/iio/resolverinstead 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.txtDocumentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +3
-0@@ -0,0 +1,16 @@ +Analog Devices AD2S1200 and AD2S1205 Resolver-to-Digital Converter + +Required properties:Quoted 8 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt line 7
Quoted 4 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +4
-0+ - 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 AD2S1200Are these gpios something that would be common to resolvers (whatever that is) or specific to this chip. For the latter, you need vendor prefixes.
Quoted 7 lines
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
Quoted 20 lines
Jonathan
Quoted 16 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt line 11
Quoted 5 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +4
-0+ +Example: + + resolver {Missing unit address.
Quoted 8 lines
Documentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +5
-0+ compatible = "adi,ad2s1200"; + reg = <4>; + sample-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>; + rdvel-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>; + };Signature · 1 line
2.16.2
Rob Herring
maintainerRe: [PATCH v3 5/9] staging: iio: ad2s1200: Add documentation for device tree binding
In reply to David VeenstraQuoted 7 lines · David Veenstra
Add documentation for the added device tree bindings.
Signed-off-by David Veenstra <[email protected]>---
Changes in v3:
- Documentation is added toDocumentation/devicetree/bindings/iio/resolverinstead of staging directory.
Also, update the subject. This is not part of staging. "dt-bindings: iio: .." is the preferred subject prefix.
Quoted 31 lines
- 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.txtDocumentation/devicetree/bindings/iio/resolver/ad2s1200.txt new file +16
-0@@ -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>; + };Signature · 1 line
2.16.2
PATCH v3 6/9 staging: iio: ad2s1200: Add scaling factor for angular velocity channel
In reply to David VeenstraThe 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.
---
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(-)
drivers/staging/iio/resolver/ad2s1200.c +45 -26
@@ -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), } };
Signature · 1 line
2.16.2
Jonathan Cameron
maintainerRe: [PATCH v3 6/9] staging: iio: ad2s1200: Add scaling factor for angular velocity channel
Quoted 2 lines · On Mon, 23 Apr 2018 00:03:59 +0200 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
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
Quoted 107 lines
as its unit. So a scaling factor of approximately 2 * Pi is added to the angular velocity channel.
Signed-off-by David Veenstra <[email protected]>---
Changes in v3:
- A decimal approximation of the scale is used, instead of a fractional approximation.
- Remove unneeded explanation ofiio_convert_raw_to_processed.drivers/staging/iio/resolver/ad2s1200.c | 71 +++++++++++++++++++++------------ 1 file changed, 45 insertions(+), 26 deletions(-)drivers/staging/iio/resolver/ad2s1200.c +45
-26@@ -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
Quoted 11 lines · On Sat, 28 Apr 2018 18:23:44 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 2 lines · On Mon, 23 Apr 2018 00:03:59 +0200 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
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
Quoted 102 lines
Jonathan
Quoted 98 lines
as its unit. So a scaling factor of approximately 2 * Pi is added to the angular velocity channel.
Signed-off-by David Veenstra <[email protected]>---
Changes in v3:
- A decimal approximation of the scale is used, instead of a fractional approximation.
- Remove unneeded explanation ofiio_convert_raw_to_processed.drivers/staging/iio/resolver/ad2s1200.c | 71 +++++++++++++++++++++------------ 1 file changed, 45 insertions(+), 26 deletions(-)drivers/staging/iio/resolver/ad2s1200.c +44
-26@@ -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 ; }
Quoted 14 lines
drivers/staging/iio/resolver/ad2s1200.c static const struct iio_chan_spec ad2s1200_channels[] = { · line 125
Quoted 9 lines
drivers/staging/iio/resolver/ad2s1200.c +1
-0static 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 6/9] staging: iio: ad2s1200: Add scaling factor for angular velocity channel
Quoted 38 lines · Jonathan Cameron
Quoted 11 lines · On Sat, 28 Apr 2018 18:23:44 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 2 lines · On Mon, 23 Apr 2018 00:03:59 +0200 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
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
_SCALEis 360/(2^12)But I'm not seeing any hint whatsoever on the scaling for the rotational velocity...
Quoted 6 lines
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
Quoted 118 lines
Jonathan
Quoted 116 lines
Jonathan
Quoted 100 lines
as its unit. So a scaling factor of approximately 2 * Pi is added to the angular velocity channel.
Signed-off-by David Veenstra <[email protected]>---
Changes in v3:
- A decimal approximation of the scale is used, instead of a fractional approximation.
- Remove unneeded explanation ofiio_convert_raw_to_processed.drivers/staging/iio/resolver/ad2s1200.c | 71 +++++++++++++++++++++------------ 1 file changed, 45 insertions(+), 26 deletions(-)drivers/staging/iio/resolver/ad2s1200.c +44
-26@@ -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[] = {drivers/staging/iio/resolver/ad2s1200.c static const struct iio_chan_spec ad2s1200_channels[] = { · line 125
Quoted 7 lines
drivers/staging/iio/resolver/ad2s1200.c +1
-0@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
PATCH v3 7/9 staging: iio: Documentation: Add missing sysfs docs for angle channel
In reply to David VeenstraThe 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
---
Changes in v3:
- Remove axis modifier on in_angle.
Documentation/ABI/testing/sysfs-bus-iio | 9 +++++++++ 1 file changed, 9 insertions(+)
Documentation/ABI/testing/sysfs-bus-iio +9 -0
@@ -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: [email protected] +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: [email protected] 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: [email protected] Description:
Signature · 1 line
2.16.2
Re: [PATCH v3 7/9] staging: iio: Documentation: Add missing sysfs docs for angle channel
Quoted 9 lines · On Mon, 23 Apr 2018 00:04:10 +0200 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 <[email protected]>
Great, no point in waiting on the other patches for the ABI docs as we are fine with this.
Applied,
Thanks,
Jonathan
Quoted 41 lines
---
Changes in v3:
- Remove axis modifier onin_angle.Documentation/ABI/testing/sysfs-bus-iio | 9 +++++++++ 1 file changed, 9 insertions(+)Documentation/ABI/testing/sysfs-bus-iio +9
-0@@ -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: [email protected] +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: [email protected] 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: [email protected] Description:
PATCH v3 8/9 staging: iio: ad2s1200: Add scaling factor for angle channel
In reply to David VeenstraA 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.
---
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(+)
drivers/staging/iio/resolver/ad2s1200.c +6 -0
@@ -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,
Signature · 1 line
2.16.2
Jonathan Cameron
maintainerRe: [PATCH v3 8/9] staging: iio: ad2s1200: Add scaling factor for angle channel
Quoted 8 lines · On Mon, 23 Apr 2018 00:04:24 +0200 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_NANOis used, so that the scale of both the angle channel and angular velocity channel has 7 significant digits.Signed-off-by David Veenstra <[email protected]>
Good, will pick up with the precursors in v4.
Thanks,
Jonathan
Quoted 33 lines
---
Changes in v3:
- A decimal approximation of the scale is used, instead of a fractional approximation.
- Remove unneeded explanation ofiio_convert_raw_to_processed.drivers/staging/iio/resolver/ad2s1200.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/staging/iio/resolver/ad2s1200.c +6
-0@@ -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,
PATCH v3 9/9 staging: iio: ad2s1200: Move driver out of staging
In reply to David VeenstraMove the iio driver for the ad2s1200 and ad2s1205 resolver-to-digital converter out of staging, into mainline iio subsystems.
---
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
drivers/iio/Kconfig +1 -0
@@ -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
@@ -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
@@ -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
@@ -0,0 +1,5 @@ +# +# Makefile for Resolver/Synchro drivers +# + +obj-$(CONFIG_AD2S1200) += ad2s1200.o
drivers/iio/resolver/ad2s1200.c new file +201 -0
201 lines not on this page · raw message
drivers/staging/iio/resolver/Kconfig +0 -12
@@ -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
@@ -3,5 +3,4 @@ # obj-$(CONFIG_AD2S90) += ad2s90.o -obj-$(CONFIG_AD2S1200) += ad2s1200.o obj-$(CONFIG_AD2S1210) += ad2s1210.o
drivers/staging/iio/resolver/ad2s1200.c deleted +0 -201
201 lines not on this page · raw message
Signature · 1 line
2.16.2
Jonathan Cameron
maintainerRe: [PATCH v3 9/9] staging: iio: ad2s1200: Move driver out of staging
Quoted 4 lines · On Mon, 23 Apr 2018 00:04:42 +0200 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 <[email protected]>
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
Quoted 81 lines
---
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.cdrivers/iio/Kconfig +1
-0@@ -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 # IIOdrivers/iio/Makefile +1
-0@@ -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@@ -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. +endmenudrivers/iio/resolver/Makefile new file +5
-0@@ -0,0 +1,5 @@ +# +# Makefile for Resolver/Synchro drivers +# + +obj-$(CONFIG_AD2S1200) += ad2s1200.odrivers/iio/resolver/ad2s1200.c new file +0
-0
drivers/iio/resolver/ad2s1200.c line 5
Quoted 5 lines
drivers/iio/resolver/ad2s1200.c new file +5
-0@@ -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.
drivers/iio/resolver/ad2s1200.c line 10
Quoted 5 lines
drivers/iio/resolver/ad2s1200.c new file +5
-0+* +* 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.
Quoted 38 lines
drivers/iio/resolver/ad2s1200.c new file +38
-0+*/ + +#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 ;
drivers/iio/resolver/ad2s1200.c line 58
Quoted 10 lines
drivers/iio/resolver/ad2s1200.c new file +10
-0+}; + +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.
Quoted 72 lines
drivers/iio/resolver/ad2s1200.c new file +72
-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 ), + } +};
drivers/iio/resolver/ad2s1200.c line 140
Quoted 10 lines
drivers/iio/resolver/ad2s1200.c new file +10
-0+ +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.
Quoted 28 lines
drivers/iio/resolver/ad2s1200.c new file +28
-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;
drivers/iio/resolver/ad2s1200.c line 178
Quoted 10 lines
drivers/iio/resolver/ad2s1200.c new file +10
-0+ 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.
drivers/iio/resolver/ad2s1200.c line 184
Quoted 6 lines
drivers/iio/resolver/ad2s1200.c new file +6
-0+ +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.
Quoted 17 lines
drivers/iio/resolver/ad2s1200.c new file +17
-0+ { "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 <[email protected]>" ); +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
Quoted 6 lines · Jonathan Cameron
Quoted 3 lines · On Mon, 23 Apr 2018 00:04:42 +0200 David Veenstra…
Move the iio driver for the ad2s1200 and ad2s1205 resolver-to-digital converter out of staging, into mainline iio subsystems.
Quoted 9 lines
Quoted 1 line
Signed-off-by David Veenstra <[email protected]>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
Quoted 320 lines
thanks,
Jonathan
Quoted 81 lines
---
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.cdrivers/iio/Kconfig +1
-0@@ -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 # IIOdrivers/iio/Makefile +1
-0@@ -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@@ -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. +endmenudrivers/iio/resolver/Makefile new file +5
-0@@ -0,0 +1,5 @@ +# +# Makefile for Resolver/Synchro drivers +# + +obj-$(CONFIG_AD2S1200) += ad2s1200.odrivers/iio/resolver/ad2s1200.c new file +0
-0drivers/iio/resolver/ad2s1200.c line 5
Quoted 5 lines
drivers/iio/resolver/ad2s1200.c new file +5
-0@@ -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.
drivers/iio/resolver/ad2s1200.c line 10
Quoted 5 lines
drivers/iio/resolver/ad2s1200.c new file +5
-0+* +* 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.Quoted 38 lines
drivers/iio/resolver/ad2s1200.c new file +38
-0+*/ + +#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 ;drivers/iio/resolver/ad2s1200.c line 58
Quoted 10 lines
drivers/iio/resolver/ad2s1200.c new file +10
-0+}; + +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.
Quoted 72 lines
drivers/iio/resolver/ad2s1200.c new file +72
-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 ), + } +};drivers/iio/resolver/ad2s1200.c line 140
Quoted 10 lines
drivers/iio/resolver/ad2s1200.c new file +10
-0+ +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.
Quoted 28 lines
drivers/iio/resolver/ad2s1200.c new file +28
-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;drivers/iio/resolver/ad2s1200.c line 178
Quoted 10 lines
drivers/iio/resolver/ad2s1200.c new file +10
-0+ 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_registercall.Reorder this to put them before that then just do
returndevm_iio_device_register
to finish up probe.drivers/iio/resolver/ad2s1200.c line 184
Quoted 6 lines
drivers/iio/resolver/ad2s1200.c new file +6
-0+ +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_idtable.No reason this needs to happen before taking it out of staging.
Quoted 17 lines
drivers/iio/resolver/ad2s1200.c new file +17
-0+ { "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 <[email protected]>" ); +MODULE_DESCRIPTION ("Analog Devices AD2S1200/1205 Resolver to Digital SPI driver" ); +MODULE_LICENSE ("GPL v2" );...