[PATCH v6 0/3] staging: iio: ad2s1210: Switch to the gpio descriptor interface.

[PATCH v6 0/3] staging: iio: ad2s1210: Switch to the gpio descriptor interface.

[Nishad Kamdar]

Use the gpiod interface instead of the deprecated old non-descriptor

Changes in v6:
 - Split device tree table addition and device tree support 
   addition in two patches.
 - Replace platform data with device tree support.
 - Rename boolean property.
Changes in v5:
 - Add device tree support.
 - Add device tree table for matching vendor ID.
 - Add Support for retrieving platform data from device tree.
Changes in v4:
 - Add spaces after { and before } in gpios[]
   initialization.
 - Check the correct pointer for error.
 - Align the dev_err msg to existing format in the code.
Changes in v3:
 - Use a pointer to pointer for gpio_desc in
   struct ad2s1210_gpio as it will be used to
   modify a pointer.
 - Use dot notation to initialize the structure.
 - Use a pointer variable to avoid writing gpios[i].
Changes in v2:
 - Use the spi_device struct embedded in st instead
   of passing it as an argument to ad2s1210_setup_gpios().
 - Use an array of structs to reduce redundant code in
   in ad2s1210_setup_gpios().
 - Remove ad2s1210_free_gpios() as devm API is being used.
-

Nishad Kamdar (3):
  staging: iio: ad2s1210: Switch to the gpio descriptor interface
  staging: iio: ad2s1210: Add device tree table.
  staging: iio: ad2s1210: Add device tree support.

 drivers/staging/iio/resolver/Kconfig    |   1 +
 drivers/staging/iio/resolver/ad2s1210.c | 114 +++++++++++++-----------
 drivers/staging/iio/resolver/ad2s1210.h |  20 -----
 3 files changed, 65 insertions(+), 70 deletions(-)

-- 
2.17.1

[PATCH v6 1/3] staging: iio: ad2s1210: Switch to the gpio descriptor interface

[Nishad Kamdar]

Use the gpiod interface instead of the deprecated old non-descriptor
interface.

Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1210.c | 92 ++++++++++++++-----------
 drivers/staging/iio/resolver/ad2s1210.h |  3 -
 2 files changed, 50 insertions(+), 45 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index ac13b99bd9cb..93c3c70ce62e 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -15,7 +15,7 @@
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 
 #include <linux/iio/iio.h>
@@ -69,10 +69,21 @@ enum ad2s1210_mode {
 
 static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
 
+struct ad2s1210_gpio {
+	struct gpio_desc **ptr;
+	const char *name;
+	unsigned long flags;
+};
+
 struct ad2s1210_state {
 	const struct ad2s1210_platform_data *pdata;
 	struct mutex lock;
 	struct spi_device *sdev;
+	struct gpio_desc *sample;
+	struct gpio_desc *a0;
+	struct gpio_desc *a1;
+	struct gpio_desc *res0;
+	struct gpio_desc *res1;
 	unsigned int fclkin;
 	unsigned int fexcit;
 	bool hysteresis;
@@ -91,8 +102,8 @@ static const int ad2s1210_mode_vals[4][2] = {
 static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
 				     struct ad2s1210_state *st)
 {
-	gpio_set_value(st->pdata->a[0], ad2s1210_mode_vals[mode][0]);
-	gpio_set_value(st->pdata->a[1], ad2s1210_mode_vals[mode][1]);
+	gpiod_set_value(st->a0, ad2s1210_mode_vals[mode][0]);
+	gpiod_set_value(st->a1, ad2s1210_mode_vals[mode][1]);
 	st->mode = mode;
 }
 
@@ -152,8 +163,8 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)
 
 static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)
 {
-	int resolution = (gpio_get_value(st->pdata->res[0]) << 1) |
-			  gpio_get_value(st->pdata->res[1]);
+	int resolution = (gpiod_get_value(st->res0) << 1) |
+			  gpiod_get_value(st->res1);
 
 	return ad2s1210_resolution_value[resolution];
 }
@@ -164,10 +175,10 @@ static const int ad2s1210_res_pins[4][2] = {
 
 static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
 {
-	gpio_set_value(st->pdata->res[0],
-		       ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
-	gpio_set_value(st->pdata->res[1],
-		       ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
+	gpiod_set_value(st->res0,
+			ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
+	gpiod_set_value(st->res1,
+			ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
 }
 
 static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
@@ -401,15 +412,15 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
 	int ret;
 
 	mutex_lock(&st->lock);
-	gpio_set_value(st->pdata->sample, 0);
+	gpiod_set_value(st->sample, 0);
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
-	gpio_set_value(st->pdata->sample, 1);
+	gpiod_set_value(st->sample, 1);
 	ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
 	if (ret < 0)
 		goto error_ret;
-	gpio_set_value(st->pdata->sample, 0);
-	gpio_set_value(st->pdata->sample, 1);
+	gpiod_set_value(st->sample, 0);
+	gpiod_set_value(st->sample, 1);
 error_ret:
 	mutex_unlock(&st->lock);
 
@@ -466,7 +477,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 	s16 vel;
 
 	mutex_lock(&st->lock);
-	gpio_set_value(st->pdata->sample, 0);
+	gpiod_set_value(st->sample, 0);
 	/* delay (6 * tck + 20) nano seconds */
 	udelay(1);
 
@@ -512,7 +523,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 	}
 
 error_ret:
-	gpio_set_value(st->pdata->sample, 1);
+	gpiod_set_value(st->sample, 1);
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
 	mutex_unlock(&st->lock);
@@ -630,30 +641,32 @@ static const struct iio_info ad2s1210_info = {
 
 static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
 {
-	unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
-	struct gpio ad2s1210_gpios[] = {
-		{ st->pdata->sample, GPIOF_DIR_IN, "sample" },
-		{ st->pdata->a[0], flags, "a0" },
-		{ st->pdata->a[1], flags, "a1" },
-		{ st->pdata->res[0], flags, "res0" },
-		{ st->pdata->res[0], flags, "res1" },
+	int ret, i;
+	struct spi_device *spi = st->sdev;
+	struct ad2s1210_gpio *pin;
+	unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
+
+	struct ad2s1210_gpio gpios[] = {
+		{ .ptr = &st->sample, .name = "sample", .flags = GPIOD_IN },
+		{ .ptr = &st->a0, .name = "a0", .flags = flags },
+		{ .ptr = &st->a1, .name = "a1", .flags = flags },
+		{ .ptr = &st->res0, .name = "res0", .flags = flags },
+		{ .ptr = &st->res1, .name = "res1", .flags = flags },
 	};
 
-	return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios));
-}
-
-static void ad2s1210_free_gpios(struct ad2s1210_state *st)
-{
-	unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
-	struct gpio ad2s1210_gpios[] = {
-		{ st->pdata->sample, GPIOF_DIR_IN, "sample" },
-		{ st->pdata->a[0], flags, "a0" },
-		{ st->pdata->a[1], flags, "a1" },
-		{ st->pdata->res[0], flags, "res0" },
-		{ st->pdata->res[0], flags, "res1" },
-	};
+	for (i = 0; i < ARRAY_SIZE(gpios); i++) {
+		pin = &gpios[i];
+		*pin->ptr = devm_gpiod_get(&spi->dev, pin->name, pin->flags);
+		if (IS_ERR(*pin->ptr)) {
+			ret = PTR_ERR(*pin->ptr);
+			dev_err(&spi->dev,
+				"ad2s1210: failed to request %s GPIO: %d\n",
+				pin->name, ret);
+			return ret;
+		}
+	}
 
-	gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios));
+	return 0;
 }
 
 static int ad2s1210_probe(struct spi_device *spi)
@@ -692,7 +705,7 @@ static int ad2s1210_probe(struct spi_device *spi)
 
 	ret = iio_device_register(indio_dev);
 	if (ret)
-		goto error_free_gpios;
+		return ret;
 
 	st->fclkin = spi->max_speed_hz;
 	spi->mode = SPI_MODE_3;
@@ -700,10 +713,6 @@ static int ad2s1210_probe(struct spi_device *spi)
 	ad2s1210_initial(st);
 
 	return 0;
-
-error_free_gpios:
-	ad2s1210_free_gpios(st);
-	return ret;
 }
 
 static int ad2s1210_remove(struct spi_device *spi)
@@ -711,7 +720,6 @@ static int ad2s1210_remove(struct spi_device *spi)
 	struct iio_dev *indio_dev = spi_get_drvdata(spi);
 
 	iio_device_unregister(indio_dev);
-	ad2s1210_free_gpios(iio_priv(indio_dev));
 
 	return 0;
 }
diff --git a/drivers/staging/iio/resolver/ad2s1210.h b/drivers/staging/iio/resolver/ad2s1210.h
index e9b2147701fc..63d479b20a6c 100644
--- a/drivers/staging/iio/resolver/ad2s1210.h
+++ b/drivers/staging/iio/resolver/ad2s1210.h
@@ -12,9 +12,6 @@
 #define _AD2S1210_H
 
 struct ad2s1210_platform_data {
-	unsigned int		sample;
-	unsigned int		a[2];
-	unsigned int		res[2];
 	bool			gpioin;
 };
 #endif /* _AD2S1210_H */
-- 
2.17.1

Re: [PATCH v6 1/3] staging: iio: ad2s1210: Switch to the gpio descriptor interface

[Jonathan Cameron]

On Sun, 28 Oct 2018 13:21:25 +0530
Nishad Kamdar <nishadkamdar@gmail.com> wrote:

> Use the gpiod interface instead of the deprecated old non-descriptor
> interface.
> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
Hi Nishad,

Sorry it took me most of the week to get to this.  It's a trade off when you
want to make fast progress, but I would suggest always leaving a few days
between patches to see if there are other review comments coming in.

I don't particularly mind myself (as I'm very good at ignoring older versions ;)
but I do feel some time got wasted here on your part.

Anyhow, what you have here is correct but the drive to get things as a nice
array has lead to a weird mixture of being all array based and not being at all.
Some suggestions in line that will hopefully tidy this up for you.

I'm basically suggesting adding a layer of indirection where you don't currently
have one (on the actual reads and writes) so that you get more consistency
with the setup code rather than adding a lot of fiddly indirection just in
the setup code.

Note, what I've given is very much meant to be an illustration.  It's probably
full of bugs and silly naming etc, so don't take it as being right!

Jonathan

> ---
>  drivers/staging/iio/resolver/ad2s1210.c | 92 ++++++++++++++-----------
>  drivers/staging/iio/resolver/ad2s1210.h |  3 -
>  2 files changed, 50 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index ac13b99bd9cb..93c3c70ce62e 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -15,7 +15,7 @@
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
>  #include <linux/delay.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  
>  #include <linux/iio/iio.h>
> @@ -69,10 +69,21 @@ enum ad2s1210_mode {
>  
>  static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
>  
> +struct ad2s1210_gpio {
> +	struct gpio_desc **ptr;
> +	const char *name;
> +	unsigned long flags;
> +};
> +
>  struct ad2s1210_state {
>  	const struct ad2s1210_platform_data *pdata;
>  	struct mutex lock;
>  	struct spi_device *sdev;

With the setup below this becomes
	struct gpio_desc *gpios[5];

> +	struct gpio_desc *sample;
> +	struct gpio_desc *a0;
> +	struct gpio_desc *a1;
> +	struct gpio_desc *res0;
> +	struct gpio_desc *res1;
>  	unsigned int fclkin;
>  	unsigned int fexcit;
>  	bool hysteresis;
> @@ -91,8 +102,8 @@ static const int ad2s1210_mode_vals[4][2] = {
>  static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
>  				     struct ad2s1210_state *st)
>  {
> -	gpio_set_value(st->pdata->a[0], ad2s1210_mode_vals[mode][0]);
> -	gpio_set_value(st->pdata->a[1], ad2s1210_mode_vals[mode][1]);
> +	gpiod_set_value(st->a0, ad2s1210_mode_vals[mode][0]);
> +	gpiod_set_value(st->a1, ad2s1210_mode_vals[mode][1]);
>  	st->mode = mode;
>  }
>  
> @@ -152,8 +163,8 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)
>  
>  static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)
>  {
> -	int resolution = (gpio_get_value(st->pdata->res[0]) << 1) |
> -			  gpio_get_value(st->pdata->res[1]);
> +	int resolution = (gpiod_get_value(st->res0) << 1) |
> +			  gpiod_get_value(st->res1);
>  
>  	return ad2s1210_resolution_value[resolution];
>  }
> @@ -164,10 +175,10 @@ static const int ad2s1210_res_pins[4][2] = {
>  
>  static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
>  {
> -	gpio_set_value(st->pdata->res[0],
> -		       ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> -	gpio_set_value(st->pdata->res[1],
> -		       ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
> +	gpiod_set_value(st->res0,
> +			ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> +	gpiod_set_value(st->res1,
> +			ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
>  }
>  
>  static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
> @@ -401,15 +412,15 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
>  	int ret;
>  
>  	mutex_lock(&st->lock);
> -	gpio_set_value(st->pdata->sample, 0);
> +	gpiod_set_value(st->sample, 0);
>  	/* delay (2 * tck + 20) nano seconds */
>  	udelay(1);
> -	gpio_set_value(st->pdata->sample, 1);
> +	gpiod_set_value(st->sample, 1);
>  	ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
>  	if (ret < 0)
>  		goto error_ret;
> -	gpio_set_value(st->pdata->sample, 0);
> -	gpio_set_value(st->pdata->sample, 1);
> +	gpiod_set_value(st->sample, 0);
> +	gpiod_set_value(st->sample, 1);
>  error_ret:
>  	mutex_unlock(&st->lock);
>  
> @@ -466,7 +477,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
>  	s16 vel;
>  
>  	mutex_lock(&st->lock);
> -	gpio_set_value(st->pdata->sample, 0);
> +	gpiod_set_value(st->sample, 0);
>  	/* delay (6 * tck + 20) nano seconds */
>  	udelay(1);
>  
> @@ -512,7 +523,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
>  	}
>  
>  error_ret:
> -	gpio_set_value(st->pdata->sample, 1);
> +	gpiod_set_value(st->sample, 1);

With the below this becomes
	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);

>  	/* delay (2 * tck + 20) nano seconds */
>  	udelay(1);
>  	mutex_unlock(&st->lock);
> @@ -630,30 +641,32 @@ static const struct iio_info ad2s1210_info = {
>  
>  static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
>  {
> -	unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
> -	struct gpio ad2s1210_gpios[] = {
> -		{ st->pdata->sample, GPIOF_DIR_IN, "sample" },
> -		{ st->pdata->a[0], flags, "a0" },
> -		{ st->pdata->a[1], flags, "a1" },
> -		{ st->pdata->res[0], flags, "res0" },
> -		{ st->pdata->res[0], flags, "res1" },
> +	int ret, i;
> +	struct spi_device *spi = st->sdev;
> +	struct ad2s1210_gpio *pin;
> +	unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;

So talk me through what is going on here?  All of a and res are controlled
by this one parameter?

> +
> +	struct ad2s1210_gpio gpios[] = {
> +		{ .ptr = &st->sample, .name = "sample", .flags = GPIOD_IN },
> +		{ .ptr = &st->a0, .name = "a0", .flags = flags },
> +		{ .ptr = &st->a1, .name = "a1", .flags = flags },
> +		{ .ptr = &st->res0, .name = "res0", .flags = flags },
> +		{ .ptr = &st->res1, .name = "res1", .flags = flags },

To my eye, there are two sets of gpios in here.  They were originally handled
like that st->pdata->a[0] etc, and it would be sensible to continue doing so.

Actually I don't really like the indirection at all.  Why not have
a simple enum indexing the gpios and then have a single array in st for them all?

enum ad2s1210_gpios {
	AD2S1210_SAMPLE,
	AD2S1210_A0,
	AD2S1210_A1,
	AD2S1210_RES0,
	AD2S1210_RES1,
};

Then have two constant versions of the above structure (shrunk a bit) that you
pick from depending on the flag.

static const struct ad2s1210_gpio gpios_gpioin[] = {
	[AD2S1210_SAMPLE] = { .name = "sample", .flags = GPIOD_IN, },
	[AD2S1210_A0] = { .name = "a0", .flags = GPIOD_IN, },
	[AD2S1210_A1] = { .name = "a1", .flags = GPIOD_IN, },
	[AD2S1210_RES0] = { .name = "res0", .flags = GPIOD_IN, },
	[AD2S1210_RES1] = { .name = "res1", .flags = GPIOD_IN, },
};

static const struct ad2s1210_gpio gpios_gpioout[] = {
	[AD2S1210_SAMPLE] = { .name = "sample", .flags = GPIOD_IN, },
	[AD2S1210_A0] = { .name = "a0", .flags = GPIOD_OUT, },
	[AD2S1210_A1] = { .name = "a1", .flags = GPIOD_OUT, },
	[AD2S1210_RES0] = { .name = "res0", .flags = GPIOD_OUT, },
	[AD2S1210_RES1] = { .name = "res1", .flags = GPIOD_OUT, },
};


static int ad2s1210_setup_gpios(struct ad2s1210_state *st, bool gpioin)
{
//note my naming is awful so do this better than I have ;)
	struct ad2s1201_gpio *pin = gpioin ? &gpios_gpioin[0] : &gpios_gpiout[0]; 
	int i;


	for (i = 0; i < ARRAY_SIZE(gpios_gpioin); i++) {
		st->gpios[i] = devm_gpiod_get(&spi->dev, pin[i]->name, pin[i]->flags);
		if (IS_ERR(st->gpios[i])) {
			ret = PTR_ERR(st->gpios[i]);
			dev_err(&spi->dev,
				"ad2s1210: failed to request %s GPIO: %d\n",
				pin->name, ret);
			return ret;
		}
	}

	return 0;
}

>  	};
>  
> -	return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios));
> -}
> -
> -static void ad2s1210_free_gpios(struct ad2s1210_state *st)
> -{
> -	unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
> -	struct gpio ad2s1210_gpios[] = {
> -		{ st->pdata->sample, GPIOF_DIR_IN, "sample" },
> -		{ st->pdata->a[0], flags, "a0" },
> -		{ st->pdata->a[1], flags, "a1" },
> -		{ st->pdata->res[0], flags, "res0" },
> -		{ st->pdata->res[0], flags, "res1" },
> -	};
> +	for (i = 0; i < ARRAY_SIZE(gpios); i++) {
> +		pin = &gpios[i];
> +		*pin->ptr = devm_gpiod_get(&spi->dev, pin->name, pin->flags);
> +		if (IS_ERR(*pin->ptr)) {
> +			ret = PTR_ERR(*pin->ptr);
> +			dev_err(&spi->dev,
> +				"ad2s1210: failed to request %s GPIO: %d\n",
> +				pin->name, ret);
> +			return ret;
> +		}
> +	}
>  
> -	gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios));
> +	return 0;
>  }
>  
>  static int ad2s1210_probe(struct spi_device *spi)
> @@ -692,7 +705,7 @@ static int ad2s1210_probe(struct spi_device *spi)
>  
>  	ret = iio_device_register(indio_dev);
>  	if (ret)
> -		goto error_free_gpios;
> +		return ret;
>  
>  	st->fclkin = spi->max_speed_hz;
>  	spi->mode = SPI_MODE_3;
> @@ -700,10 +713,6 @@ static int ad2s1210_probe(struct spi_device *spi)
>  	ad2s1210_initial(st);
>  
>  	return 0;
> -
> -error_free_gpios:
> -	ad2s1210_free_gpios(st);
> -	return ret;
>  }
>  
>  static int ad2s1210_remove(struct spi_device *spi)
> @@ -711,7 +720,6 @@ static int ad2s1210_remove(struct spi_device *spi)
>  	struct iio_dev *indio_dev = spi_get_drvdata(spi);
>  
>  	iio_device_unregister(indio_dev);
> -	ad2s1210_free_gpios(iio_priv(indio_dev));
>  
>  	return 0;
>  }
> diff --git a/drivers/staging/iio/resolver/ad2s1210.h b/drivers/staging/iio/resolver/ad2s1210.h
> index e9b2147701fc..63d479b20a6c 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.h
> +++ b/drivers/staging/iio/resolver/ad2s1210.h
> @@ -12,9 +12,6 @@
>  #define _AD2S1210_H
>  
>  struct ad2s1210_platform_data {
> -	unsigned int		sample;
> -	unsigned int		a[2];
> -	unsigned int		res[2];
>  	bool			gpioin;
>  };
>  #endif /* _AD2S1210_H */

Re: [PATCH v6 1/3] staging: iio: ad2s1210: Switch to the gpio descriptor interface

[Nishad Kamdar]

On Sun, Oct 28, 2018 at 02:36:07PM +0000, Jonathan Cameron wrote:
> On Sun, 28 Oct 2018 13:21:25 +0530
> Nishad Kamdar <nishadkamdar@gmail.com> wrote:
> 
> > Use the gpiod interface instead of the deprecated old non-descriptor
> > interface.
> > 
> > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> Hi Nishad,
> 
> Sorry it took me most of the week to get to this.  It's a trade off when you
> want to make fast progress, but I would suggest always leaving a few days
> between patches to see if there are other review comments coming in.
>

Ok, I'll keep that in mind.
> I don't particularly mind myself (as I'm very good at ignoring older versions ;)
> but I do feel some time got wasted here on your part.
> 
> Anyhow, what you have here is correct but the drive to get things as a nice
> array has lead to a weird mixture of being all array based and not being at all.
> Some suggestions in line that will hopefully tidy this up for you.
> 
> I'm basically suggesting adding a layer of indirection where you don't currently
> have one (on the actual reads and writes) so that you get more consistency
> with the setup code rather than adding a lot of fiddly indirection just in
> the setup code.
> 
> Note, what I've given is very much meant to be an illustration.  It's probably
> full of bugs and silly naming etc, so don't take it as being right!
> 
> Jonathan
> 
> > ---
> >  drivers/staging/iio/resolver/ad2s1210.c | 92 ++++++++++++++-----------
> >  drivers/staging/iio/resolver/ad2s1210.h |  3 -
> >  2 files changed, 50 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> > index ac13b99bd9cb..93c3c70ce62e 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -15,7 +15,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/sysfs.h>
> >  #include <linux/delay.h>
> > -#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/module.h>
> >  
> >  #include <linux/iio/iio.h>
> > @@ -69,10 +69,21 @@ enum ad2s1210_mode {
> >  
> >  static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
> >  
> > +struct ad2s1210_gpio {
> > +	struct gpio_desc **ptr;
> > +	const char *name;
> > +	unsigned long flags;
> > +};
> > +
> >  struct ad2s1210_state {
> >  	const struct ad2s1210_platform_data *pdata;
> >  	struct mutex lock;
> >  	struct spi_device *sdev;
> 
> With the setup below this becomes
> 	struct gpio_desc *gpios[5];
> 
> > +	struct gpio_desc *sample;
> > +	struct gpio_desc *a0;
> > +	struct gpio_desc *a1;
> > +	struct gpio_desc *res0;
> > +	struct gpio_desc *res1;
> >  	unsigned int fclkin;
> >  	unsigned int fexcit;
> >  	bool hysteresis;
> > @@ -91,8 +102,8 @@ static const int ad2s1210_mode_vals[4][2] = {
> >  static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
> >  				     struct ad2s1210_state *st)
> >  {
> > -	gpio_set_value(st->pdata->a[0], ad2s1210_mode_vals[mode][0]);
> > -	gpio_set_value(st->pdata->a[1], ad2s1210_mode_vals[mode][1]);
> > +	gpiod_set_value(st->a0, ad2s1210_mode_vals[mode][0]);
> > +	gpiod_set_value(st->a1, ad2s1210_mode_vals[mode][1]);
> >  	st->mode = mode;
> >  }
> >  
> > @@ -152,8 +163,8 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)
> >  
> >  static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)
> >  {
> > -	int resolution = (gpio_get_value(st->pdata->res[0]) << 1) |
> > -			  gpio_get_value(st->pdata->res[1]);
> > +	int resolution = (gpiod_get_value(st->res0) << 1) |
> > +			  gpiod_get_value(st->res1);
> >  
> >  	return ad2s1210_resolution_value[resolution];
> >  }
> > @@ -164,10 +175,10 @@ static const int ad2s1210_res_pins[4][2] = {
> >  
> >  static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
> >  {
> > -	gpio_set_value(st->pdata->res[0],
> > -		       ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> > -	gpio_set_value(st->pdata->res[1],
> > -		       ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
> > +	gpiod_set_value(st->res0,
> > +			ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> > +	gpiod_set_value(st->res1,
> > +			ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
> >  }
> >  
> >  static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
> > @@ -401,15 +412,15 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
> >  	int ret;
> >  
> >  	mutex_lock(&st->lock);
> > -	gpio_set_value(st->pdata->sample, 0);
> > +	gpiod_set_value(st->sample, 0);
> >  	/* delay (2 * tck + 20) nano seconds */
> >  	udelay(1);
> > -	gpio_set_value(st->pdata->sample, 1);
> > +	gpiod_set_value(st->sample, 1);
> >  	ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
> >  	if (ret < 0)
> >  		goto error_ret;
> > -	gpio_set_value(st->pdata->sample, 0);
> > -	gpio_set_value(st->pdata->sample, 1);
> > +	gpiod_set_value(st->sample, 0);
> > +	gpiod_set_value(st->sample, 1);
> >  error_ret:
> >  	mutex_unlock(&st->lock);
> >  
> > @@ -466,7 +477,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> >  	s16 vel;
> >  
> >  	mutex_lock(&st->lock);
> > -	gpio_set_value(st->pdata->sample, 0);
> > +	gpiod_set_value(st->sample, 0);
> >  	/* delay (6 * tck + 20) nano seconds */
> >  	udelay(1);
> >  
> > @@ -512,7 +523,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> >  	}
> >  
> >  error_ret:
> > -	gpio_set_value(st->pdata->sample, 1);
> > +	gpiod_set_value(st->sample, 1);
> 
> With the below this becomes
> 	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
> 
> >  	/* delay (2 * tck + 20) nano seconds */
> >  	udelay(1);
> >  	mutex_unlock(&st->lock);
> > @@ -630,30 +641,32 @@ static const struct iio_info ad2s1210_info = {
> >  
> >  static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
> >  {
> > -	unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
> > -	struct gpio ad2s1210_gpios[] = {
> > -		{ st->pdata->sample, GPIOF_DIR_IN, "sample" },
> > -		{ st->pdata->a[0], flags, "a0" },
> > -		{ st->pdata->a[1], flags, "a1" },
> > -		{ st->pdata->res[0], flags, "res0" },
> > -		{ st->pdata->res[0], flags, "res1" },
> > +	int ret, i;
> > +	struct spi_device *spi = st->sdev;
> > +	struct ad2s1210_gpio *pin;
> > +	unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
> 
> So talk me through what is going on here?  All of a and res are controlled
> by this one parameter?
>

Yes. This was how the original code was. I kept it as it is.
> > +
> > +	struct ad2s1210_gpio gpios[] = {
> > +		{ .ptr = &st->sample, .name = "sample", .flags = GPIOD_IN },
> > +		{ .ptr = &st->a0, .name = "a0", .flags = flags },
> > +		{ .ptr = &st->a1, .name = "a1", .flags = flags },
> > +		{ .ptr = &st->res0, .name = "res0", .flags = flags },
> > +		{ .ptr = &st->res1, .name = "res1", .flags = flags },
> 
> To my eye, there are two sets of gpios in here.  They were originally handled
> like that st->pdata->a[0] etc, and it would be sensible to continue doing so.
> 
> Actually I don't really like the indirection at all.  Why not have
> a simple enum indexing the gpios and then have a single array in st for them all?
> 
> enum ad2s1210_gpios {
> 	AD2S1210_SAMPLE,
> 	AD2S1210_A0,
> 	AD2S1210_A1,
> 	AD2S1210_RES0,
> 	AD2S1210_RES1,
> };
> 
> Then have two constant versions of the above structure (shrunk a bit) that you
> pick from depending on the flag.
> 
> static const struct ad2s1210_gpio gpios_gpioin[] = {
> 	[AD2S1210_SAMPLE] = { .name = "sample", .flags = GPIOD_IN, },
> 	[AD2S1210_A0] = { .name = "a0", .flags = GPIOD_IN, },
> 	[AD2S1210_A1] = { .name = "a1", .flags = GPIOD_IN, },
> 	[AD2S1210_RES0] = { .name = "res0", .flags = GPIOD_IN, },
> 	[AD2S1210_RES1] = { .name = "res1", .flags = GPIOD_IN, },
> };
> 
> static const struct ad2s1210_gpio gpios_gpioout[] = {
> 	[AD2S1210_SAMPLE] = { .name = "sample", .flags = GPIOD_IN, },
> 	[AD2S1210_A0] = { .name = "a0", .flags = GPIOD_OUT, },
> 	[AD2S1210_A1] = { .name = "a1", .flags = GPIOD_OUT, },
> 	[AD2S1210_RES0] = { .name = "res0", .flags = GPIOD_OUT, },
> 	[AD2S1210_RES1] = { .name = "res1", .flags = GPIOD_OUT, },
> };
> 
> 
> static int ad2s1210_setup_gpios(struct ad2s1210_state *st, bool gpioin)
> {
> //note my naming is awful so do this better than I have ;)
> 	struct ad2s1201_gpio *pin = gpioin ? &gpios_gpioin[0] : &gpios_gpiout[0]; 
> 	int i;
> 
> 
> 	for (i = 0; i < ARRAY_SIZE(gpios_gpioin); i++) {
> 		st->gpios[i] = devm_gpiod_get(&spi->dev, pin[i]->name, pin[i]->flags);
> 		if (IS_ERR(st->gpios[i])) {
> 			ret = PTR_ERR(st->gpios[i]);
> 			dev_err(&spi->dev,
> 				"ad2s1210: failed to request %s GPIO: %d\n",
> 				pin->name, ret);
> 			return ret;
> 		}
> 	}
> 
> 	return 0;
> }
> 
> >  	};
> >  
> > -	return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios));
> > -}
> > -
> > -static void ad2s1210_free_gpios(struct ad2s1210_state *st)
> > -{
> > -	unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT;
> > -	struct gpio ad2s1210_gpios[] = {
> > -		{ st->pdata->sample, GPIOF_DIR_IN, "sample" },
> > -		{ st->pdata->a[0], flags, "a0" },
> > -		{ st->pdata->a[1], flags, "a1" },
> > -		{ st->pdata->res[0], flags, "res0" },
> > -		{ st->pdata->res[0], flags, "res1" },
> > -	};
> > +	for (i = 0; i < ARRAY_SIZE(gpios); i++) {
> > +		pin = &gpios[i];
> > +		*pin->ptr = devm_gpiod_get(&spi->dev, pin->name, pin->flags);
> > +		if (IS_ERR(*pin->ptr)) {
> > +			ret = PTR_ERR(*pin->ptr);
> > +			dev_err(&spi->dev,
> > +				"ad2s1210: failed to request %s GPIO: %d\n",
> > +				pin->name, ret);
> > +			return ret;
> > +		}
> > +	}
> >  
> > -	gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios));
> > +	return 0;
> >  }
> >  
> >  static int ad2s1210_probe(struct spi_device *spi)
> > @@ -692,7 +705,7 @@ static int ad2s1210_probe(struct spi_device *spi)
> >  
> >  	ret = iio_device_register(indio_dev);
> >  	if (ret)
> > -		goto error_free_gpios;
> > +		return ret;
> >  
> >  	st->fclkin = spi->max_speed_hz;
> >  	spi->mode = SPI_MODE_3;
> > @@ -700,10 +713,6 @@ static int ad2s1210_probe(struct spi_device *spi)
> >  	ad2s1210_initial(st);
> >  
> >  	return 0;
> > -
> > -error_free_gpios:
> > -	ad2s1210_free_gpios(st);
> > -	return ret;
> >  }
> >  
> >  static int ad2s1210_remove(struct spi_device *spi)
> > @@ -711,7 +720,6 @@ static int ad2s1210_remove(struct spi_device *spi)
> >  	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> >  
> >  	iio_device_unregister(indio_dev);
> > -	ad2s1210_free_gpios(iio_priv(indio_dev));
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.h b/drivers/staging/iio/resolver/ad2s1210.h
> > index e9b2147701fc..63d479b20a6c 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.h
> > +++ b/drivers/staging/iio/resolver/ad2s1210.h
> > @@ -12,9 +12,6 @@
> >  #define _AD2S1210_H
> >  
> >  struct ad2s1210_platform_data {
> > -	unsigned int		sample;
> > -	unsigned int		a[2];
> > -	unsigned int		res[2];
> >  	bool			gpioin;
> >  };
> >  #endif /* _AD2S1210_H */
> 

ok, I'll make the changes.

Thanks for thre review.

regards,
Nishad

[PATCH v6 2/3] staging: iio: ad2s1210: Add device tree table.

[Nishad Kamdar]

Add device tree table for matching vendor ID.

Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1210.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 93c3c70ce62e..0234869e9d74 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -724,6 +724,12 @@ static int ad2s1210_remove(struct spi_device *spi)
 	return 0;
 }
 
+static const struct of_device_id ad2s1210_of_match[] = {
+	{ .compatible = "adi,ad2s1210", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad2s1210_of_match);
+
 static const struct spi_device_id ad2s1210_id[] = {
 	{ "ad2s1210" },
 	{}
@@ -733,6 +739,7 @@ MODULE_DEVICE_TABLE(spi, ad2s1210_id);
 static struct spi_driver ad2s1210_driver = {
 	.driver = {
 		.name = DRV_NAME,
+		.of_match_table = of_match_ptr(ad2s1210_of_match),
 	},
 	.probe = ad2s1210_probe,
 	.remove = ad2s1210_remove,
-- 
2.17.1

[PATCH v6 3/3] staging: iio: ad2s1210: Add device tree support.

[Nishad Kamdar]

Replace platform data with device tree support.

Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
---
 drivers/staging/iio/resolver/Kconfig    |  1 +
 drivers/staging/iio/resolver/ad2s1210.c | 17 ++++++++---------
 drivers/staging/iio/resolver/ad2s1210.h | 17 -----------------
 3 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/iio/resolver/Kconfig b/drivers/staging/iio/resolver/Kconfig
index 6a469ee6101f..cc1202ff813d 100644
--- a/drivers/staging/iio/resolver/Kconfig
+++ b/drivers/staging/iio/resolver/Kconfig
@@ -15,6 +15,7 @@ config AD2S90
 
 config AD2S1210
 	tristate "Analog Devices ad2s1210 driver"
+	depends on OF
 	depends on SPI
 	depends on GPIOLIB || COMPILE_TEST
 	help
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 0234869e9d74..5ecdb5785132 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -17,6 +17,7 @@
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
+#include <linux/of.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -76,7 +77,6 @@ struct ad2s1210_gpio {
 };
 
 struct ad2s1210_state {
-	const struct ad2s1210_platform_data *pdata;
 	struct mutex lock;
 	struct spi_device *sdev;
 	struct gpio_desc *sample;
@@ -84,6 +84,7 @@ struct ad2s1210_state {
 	struct gpio_desc *a1;
 	struct gpio_desc *res0;
 	struct gpio_desc *res1;
+	bool gpioin;
 	unsigned int fclkin;
 	unsigned int fexcit;
 	bool hysteresis;
@@ -314,7 +315,7 @@ static ssize_t ad2s1210_store_control(struct device *dev,
 	}
 	st->resolution
 		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
-	if (st->pdata->gpioin) {
+	if (st->gpioin) {
 		data = ad2s1210_read_resolution_pin(st);
 		if (data != st->resolution)
 			dev_warn(dev, "ad2s1210: resolution settings not match\n");
@@ -376,7 +377,7 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
 	}
 	st->resolution
 		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
-	if (st->pdata->gpioin) {
+	if (st->gpioin) {
 		data = ad2s1210_read_resolution_pin(st);
 		if (data != st->resolution)
 			dev_warn(dev, "ad2s1210: resolution settings not match\n");
@@ -603,7 +604,7 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
 	int ret;
 
 	mutex_lock(&st->lock);
-	if (st->pdata->gpioin)
+	if (st->gpioin)
 		st->resolution = ad2s1210_read_resolution_pin(st);
 	else
 		ad2s1210_set_resolution_pin(st);
@@ -644,7 +645,7 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
 	int ret, i;
 	struct spi_device *spi = st->sdev;
 	struct ad2s1210_gpio *pin;
-	unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
+	unsigned long flags = st->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
 
 	struct ad2s1210_gpio gpios[] = {
 		{ .ptr = &st->sample, .name = "sample", .flags = GPIOD_IN },
@@ -673,16 +674,14 @@ static int ad2s1210_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;
 	struct ad2s1210_state *st;
+	struct device_node *np = spi->dev.of_node;
 	int ret;
 
-	if (!spi->dev.platform_data)
-		return -EINVAL;
-
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 	st = iio_priv(indio_dev);
-	st->pdata = spi->dev.platform_data;
+	st->gpioin = of_property_read_bool(np, "gpioin");
 	ret = ad2s1210_setup_gpios(st);
 	if (ret < 0)
 		return ret;
diff --git a/drivers/staging/iio/resolver/ad2s1210.h b/drivers/staging/iio/resolver/ad2s1210.h
index 63d479b20a6c..e69de29bb2d1 100644
--- a/drivers/staging/iio/resolver/ad2s1210.h
+++ b/drivers/staging/iio/resolver/ad2s1210.h
@@ -1,17 +0,0 @@
-/*
- * ad2s1210.h plaform data for the ADI Resolver to Digital Converters:
- * AD2S1210
- *
- * Copyright (c) 2010-2010 Analog Devices Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-#ifndef _AD2S1210_H
-#define _AD2S1210_H
-
-struct ad2s1210_platform_data {
-	bool			gpioin;
-};
-#endif /* _AD2S1210_H */
-- 
2.17.1

Re: [PATCH v6 3/3] staging: iio: ad2s1210: Add device tree support.

[Jonathan Cameron]

On Sun, 28 Oct 2018 13:23:23 +0530
Nishad Kamdar <nishadkamdar@gmail.com> wrote:

> Replace platform data with device tree support.
> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
The whole gpio in or out thing makes less and less sense to
me and seems to contradict the datasheet.

If I'm not missing something I would just get rid of the option.
If I am missing something (not hard in the datasheet which, whilst
fairly clear, is a rather long ;)

Jonathan

> ---
>  drivers/staging/iio/resolver/Kconfig    |  1 +
>  drivers/staging/iio/resolver/ad2s1210.c | 17 ++++++++---------
>  drivers/staging/iio/resolver/ad2s1210.h | 17 -----------------
>  3 files changed, 9 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/Kconfig b/drivers/staging/iio/resolver/Kconfig
> index 6a469ee6101f..cc1202ff813d 100644
> --- a/drivers/staging/iio/resolver/Kconfig
> +++ b/drivers/staging/iio/resolver/Kconfig
> @@ -15,6 +15,7 @@ config AD2S90
>  
>  config AD2S1210
>  	tristate "Analog Devices ad2s1210 driver"
> +	depends on OF
>  	depends on SPI
>  	depends on GPIOLIB || COMPILE_TEST
>  	help
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 0234869e9d74..5ecdb5785132 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -17,6 +17,7 @@
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -76,7 +77,6 @@ struct ad2s1210_gpio {
>  };
>  
>  struct ad2s1210_state {
> -	const struct ad2s1210_platform_data *pdata;
>  	struct mutex lock;
>  	struct spi_device *sdev;
>  	struct gpio_desc *sample;
> @@ -84,6 +84,7 @@ struct ad2s1210_state {
>  	struct gpio_desc *a1;
>  	struct gpio_desc *res0;
>  	struct gpio_desc *res1;
> +	bool gpioin;
>  	unsigned int fclkin;
>  	unsigned int fexcit;
>  	bool hysteresis;
> @@ -314,7 +315,7 @@ static ssize_t ad2s1210_store_control(struct device *dev,
>  	}
>  	st->resolution
>  		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> -	if (st->pdata->gpioin) {
> +	if (st->gpioin) {
>  		data = ad2s1210_read_resolution_pin(st);
>  		if (data != st->resolution)
>  			dev_warn(dev, "ad2s1210: resolution settings not match\n");
> @@ -376,7 +377,7 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
>  	}
>  	st->resolution
>  		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> -	if (st->pdata->gpioin) {
> +	if (st->gpioin) {
>  		data = ad2s1210_read_resolution_pin(st);
>  		if (data != st->resolution)
>  			dev_warn(dev, "ad2s1210: resolution settings not match\n");
> @@ -603,7 +604,7 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
>  	int ret;
>  
>  	mutex_lock(&st->lock);
> -	if (st->pdata->gpioin)
> +	if (st->gpioin)
>  		st->resolution = ad2s1210_read_resolution_pin(st);
>  	else
>  		ad2s1210_set_resolution_pin(st);
> @@ -644,7 +645,7 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
>  	int ret, i;
>  	struct spi_device *spi = st->sdev;
>  	struct ad2s1210_gpio *pin;
> -	unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
> +	unsigned long flags = st->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
>  
>  	struct ad2s1210_gpio gpios[] = {
>  		{ .ptr = &st->sample, .name = "sample", .flags = GPIOD_IN },
> @@ -673,16 +674,14 @@ static int ad2s1210_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
>  	struct ad2s1210_state *st;
> +	struct device_node *np = spi->dev.of_node;
>  	int ret;
>  
> -	if (!spi->dev.platform_data)
> -		return -EINVAL;
> -
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  	st = iio_priv(indio_dev);
> -	st->pdata = spi->dev.platform_data;
> +	st->gpioin = of_property_read_bool(np, "gpioin");

Hmm. This bothered me in the earlier patch.  How are we meant to configure
these pins... (and this time I actually loaded the datasheet)

A0 and A1 always seem to be control signals written to the device so
I don't really understand why we would ever want them to be 'inputs'
on our host.

RES0 and RES1 are also control signals. Clearly marked as logic inputs.

The only thing I can think here is there is an evaluation board out there
were we are not in control of these, some other controller is.
That is a pretty weird board if so, hence I would only support the version
where we use GPIO outputs from the host.

This will further simplify patch 1 and get rid fo the need for this
non standard bit of devicetree binding.


>  	ret = ad2s1210_setup_gpios(st);
>  	if (ret < 0)
>  		return ret;
> diff --git a/drivers/staging/iio/resolver/ad2s1210.h b/drivers/staging/iio/resolver/ad2s1210.h
> index 63d479b20a6c..e69de29bb2d1 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.h
> +++ b/drivers/staging/iio/resolver/ad2s1210.h
> @@ -1,17 +0,0 @@
> -/*
> - * ad2s1210.h plaform data for the ADI Resolver to Digital Converters:
> - * AD2S1210
> - *
> - * Copyright (c) 2010-2010 Analog Devices Inc.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */
> -#ifndef _AD2S1210_H
> -#define _AD2S1210_H
> -
> -struct ad2s1210_platform_data {
> -	bool			gpioin;
> -};
> -#endif /* _AD2S1210_H */

Re: [PATCH v6 3/3] staging: iio: ad2s1210: Add device tree support.

[Nishad Kamdar]

On Sun, Oct 28, 2018 at 02:51:08PM +0000, Jonathan Cameron wrote:
> On Sun, 28 Oct 2018 13:23:23 +0530
> Nishad Kamdar <nishadkamdar@gmail.com> wrote:
> 
> > Replace platform data with device tree support.
> > 
> > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> The whole gpio in or out thing makes less and less sense to
> me and seems to contradict the datasheet.
> 
> If I'm not missing something I would just get rid of the option.
> If I am missing something (not hard in the datasheet which, whilst
> fairly clear, is a rather long ;)
> 
> Jonathan
> 

Ok, I'll drop it.
> > ---
> >  drivers/staging/iio/resolver/Kconfig    |  1 +
> >  drivers/staging/iio/resolver/ad2s1210.c | 17 ++++++++---------
> >  drivers/staging/iio/resolver/ad2s1210.h | 17 -----------------
> >  3 files changed, 9 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/resolver/Kconfig b/drivers/staging/iio/resolver/Kconfig
> > index 6a469ee6101f..cc1202ff813d 100644
> > --- a/drivers/staging/iio/resolver/Kconfig
> > +++ b/drivers/staging/iio/resolver/Kconfig
> > @@ -15,6 +15,7 @@ config AD2S90
> >  
> >  config AD2S1210
> >  	tristate "Analog Devices ad2s1210 driver"
> > +	depends on OF
> >  	depends on SPI
> >  	depends on GPIOLIB || COMPILE_TEST
> >  	help
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> > index 0234869e9d74..5ecdb5785132 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/module.h>
> > +#include <linux/of.h>
> >  
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > @@ -76,7 +77,6 @@ struct ad2s1210_gpio {
> >  };
> >  
> >  struct ad2s1210_state {
> > -	const struct ad2s1210_platform_data *pdata;
> >  	struct mutex lock;
> >  	struct spi_device *sdev;
> >  	struct gpio_desc *sample;
> > @@ -84,6 +84,7 @@ struct ad2s1210_state {
> >  	struct gpio_desc *a1;
> >  	struct gpio_desc *res0;
> >  	struct gpio_desc *res1;
> > +	bool gpioin;
> >  	unsigned int fclkin;
> >  	unsigned int fexcit;
> >  	bool hysteresis;
> > @@ -314,7 +315,7 @@ static ssize_t ad2s1210_store_control(struct device *dev,
> >  	}
> >  	st->resolution
> >  		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> > -	if (st->pdata->gpioin) {
> > +	if (st->gpioin) {
> >  		data = ad2s1210_read_resolution_pin(st);
> >  		if (data != st->resolution)
> >  			dev_warn(dev, "ad2s1210: resolution settings not match\n");
> > @@ -376,7 +377,7 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
> >  	}
> >  	st->resolution
> >  		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> > -	if (st->pdata->gpioin) {
> > +	if (st->gpioin) {
> >  		data = ad2s1210_read_resolution_pin(st);
> >  		if (data != st->resolution)
> >  			dev_warn(dev, "ad2s1210: resolution settings not match\n");
> > @@ -603,7 +604,7 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
> >  	int ret;
> >  
> >  	mutex_lock(&st->lock);
> > -	if (st->pdata->gpioin)
> > +	if (st->gpioin)
> >  		st->resolution = ad2s1210_read_resolution_pin(st);
> >  	else
> >  		ad2s1210_set_resolution_pin(st);
> > @@ -644,7 +645,7 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
> >  	int ret, i;
> >  	struct spi_device *spi = st->sdev;
> >  	struct ad2s1210_gpio *pin;
> > -	unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
> > +	unsigned long flags = st->gpioin ? GPIOD_IN : GPIOD_OUT_LOW;
> >  
> >  	struct ad2s1210_gpio gpios[] = {
> >  		{ .ptr = &st->sample, .name = "sample", .flags = GPIOD_IN },
> > @@ -673,16 +674,14 @@ static int ad2s1210_probe(struct spi_device *spi)
> >  {
> >  	struct iio_dev *indio_dev;
> >  	struct ad2s1210_state *st;
> > +	struct device_node *np = spi->dev.of_node;
> >  	int ret;
> >  
> > -	if (!spi->dev.platform_data)
> > -		return -EINVAL;
> > -
> >  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> >  	st = iio_priv(indio_dev);
> > -	st->pdata = spi->dev.platform_data;
> > +	st->gpioin = of_property_read_bool(np, "gpioin");
> 
> Hmm. This bothered me in the earlier patch.  How are we meant to configure
> these pins... (and this time I actually loaded the datasheet)
> 
> A0 and A1 always seem to be control signals written to the device so
> I don't really understand why we would ever want them to be 'inputs'
> on our host.
> 
> RES0 and RES1 are also control signals. Clearly marked as logic inputs.
> 
> The only thing I can think here is there is an evaluation board out there
> were we are not in control of these, some other controller is.
> That is a pretty weird board if so, hence I would only support the version
> where we use GPIO outputs from the host.
> 
> This will further simplify patch 1 and get rid fo the need for this
> non standard bit of devicetree binding.
> 
> 
> >  	ret = ad2s1210_setup_gpios(st);
> >  	if (ret < 0)
> >  		return ret;
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.h b/drivers/staging/iio/resolver/ad2s1210.h
> > index 63d479b20a6c..e69de29bb2d1 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.h
> > +++ b/drivers/staging/iio/resolver/ad2s1210.h
> > @@ -1,17 +0,0 @@
> > -/*
> > - * ad2s1210.h plaform data for the ADI Resolver to Digital Converters:
> > - * AD2S1210
> > - *
> > - * Copyright (c) 2010-2010 Analog Devices Inc.
> > - *
> > - * This program is free software; you can redistribute it and/or modify
> > - * it under the terms of the GNU General Public License version 2 as
> > - * published by the Free Software Foundation.
> > - */
> > -#ifndef _AD2S1210_H
> > -#define _AD2S1210_H
> > -
> > -struct ad2s1210_platform_data {
> > -	bool			gpioin;
> > -};
> > -#endif /* _AD2S1210_H */
> 

Ok, I'll make the changes.

Thanks for the review.

regards,
Nishad