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

[PATCH v7 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 v7:
 - Adds a level of indirection to read and write
   the gpio_desc to make the code simpler.
 - Drop gpioin flag which decides how the GPIOs
   are controlled as the GPIOs must be outputs
   for the host as per the datasheet.
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: Drop the gpioin flag.
  staging: iio: ad2s1210: Add device tree table.

 drivers/staging/iio/resolver/ad2s1210.c | 132 +++++++++++-------------
 drivers/staging/iio/resolver/ad2s1210.h |  20 ----
 2 files changed, 62 insertions(+), 90 deletions(-)

-- 
2.17.1

[PATCH v7 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 | 106 ++++++++++++++----------
 drivers/staging/iio/resolver/ad2s1210.h |   3 -
 2 files changed, 62 insertions(+), 47 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index ac13b99bd9cb..82ac9847f6f4 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>
@@ -67,12 +67,42 @@ enum ad2s1210_mode {
 	MOD_RESERVED,
 };
 
+enum ad2s1210_gpios {
+	AD2S1210_SAMPLE,
+	AD2S1210_A0,
+	AD2S1210_A1,
+	AD2S1210_RES0,
+	AD2S1210_RES1,
+};
+
+struct ad2s1210_gpio {
+	const char *name;
+	unsigned long flags;
+};
+
+static const struct ad2s1210_gpio gpios_in[] = {
+	[AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_IN },
+	[AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_IN },
+	[AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_IN },
+	[AD2S1210_RES0] = { .name = "adi,res0", .flags = GPIOD_IN },
+	[AD2S1210_RES1] = { .name = "adi,res1", .flags = GPIOD_IN },
+};
+
+static const struct ad2s1210_gpio gpios_out[] = {
+	[AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_OUT_LOW },
+	[AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_OUT_LOW },
+	[AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_OUT_LOW },
+	[AD2S1210_RES0] = { .name = "adi,res0", .flags = GPIOD_OUT_LOW },
+	[AD2S1210_RES1] = { .name = "adi,res1", .flags = GPIOD_OUT_LOW },
+};
+
 static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
 
 struct ad2s1210_state {
 	const struct ad2s1210_platform_data *pdata;
 	struct mutex lock;
 	struct spi_device *sdev;
+	struct gpio_desc *gpios[5];
 	unsigned int fclkin;
 	unsigned int fexcit;
 	bool hysteresis;
@@ -91,8 +121,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->gpios[AD2S1210_A0], ad2s1210_mode_vals[mode][0]);
+	gpiod_set_value(st->gpios[AD2S1210_A1], ad2s1210_mode_vals[mode][1]);
 	st->mode = mode;
 }
 
@@ -152,8 +182,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->gpios[AD2S1210_RES0]) << 1) |
+			  gpiod_get_value(st->gpios[AD2S1210_RES1]);
 
 	return ad2s1210_resolution_value[resolution];
 }
@@ -164,10 +194,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->gpios[AD2S1210_RES0],
+			ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
+	gpiod_set_value(st->gpios[AD2S1210_RES1],
+			ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
 }
 
 static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
@@ -401,15 +431,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->gpios[AD2S1210_SAMPLE], 0);
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
-	gpio_set_value(st->pdata->sample, 1);
+	gpiod_set_value(st->gpios[AD2S1210_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->gpios[AD2S1210_SAMPLE], 0);
+	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
 error_ret:
 	mutex_unlock(&st->lock);
 
@@ -466,7 +496,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->gpios[AD2S1210_SAMPLE], 0);
 	/* delay (6 * tck + 20) nano seconds */
 	udelay(1);
 
@@ -512,7 +542,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 	}
 
 error_ret:
-	gpio_set_value(st->pdata->sample, 1);
+	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
 	mutex_unlock(&st->lock);
@@ -630,30 +660,23 @@ 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" },
-	};
-
-	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" },
-	};
+	struct ad2s1210_gpio *pin = st->pdata->gpioin ? &gpios_in[0] : &gpios_out[0];
+	struct spi_device *spi = st->sdev;
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(gpios_in); 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[i].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 +715,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 +723,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 +730,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 v7 1/3] staging: iio: ad2s1210: Switch to the gpio descriptor interface

[Jonathan Cameron]

On Wed, 31 Oct 2018 21:28:52 +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>
It would have been less 'noisy' to do these in the reverse order (drop
the flag and support first, then do the gpiod conversion), but I suppose
it doesn't really matter.

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

Thanks,

Jonathan

> ---
>  drivers/staging/iio/resolver/ad2s1210.c | 106 ++++++++++++++----------
>  drivers/staging/iio/resolver/ad2s1210.h |   3 -
>  2 files changed, 62 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index ac13b99bd9cb..82ac9847f6f4 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>
> @@ -67,12 +67,42 @@ enum ad2s1210_mode {
>  	MOD_RESERVED,
>  };
>  
> +enum ad2s1210_gpios {
> +	AD2S1210_SAMPLE,
> +	AD2S1210_A0,
> +	AD2S1210_A1,
> +	AD2S1210_RES0,
> +	AD2S1210_RES1,
> +};
> +
> +struct ad2s1210_gpio {
> +	const char *name;
> +	unsigned long flags;
> +};
> +
> +static const struct ad2s1210_gpio gpios_in[] = {
> +	[AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_IN },
> +	[AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_IN },
> +	[AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_IN },
> +	[AD2S1210_RES0] = { .name = "adi,res0", .flags = GPIOD_IN },
> +	[AD2S1210_RES1] = { .name = "adi,res1", .flags = GPIOD_IN },
> +};
> +
> +static const struct ad2s1210_gpio gpios_out[] = {
> +	[AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_OUT_LOW },
> +	[AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_OUT_LOW },
> +	[AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_OUT_LOW },
> +	[AD2S1210_RES0] = { .name = "adi,res0", .flags = GPIOD_OUT_LOW },
> +	[AD2S1210_RES1] = { .name = "adi,res1", .flags = GPIOD_OUT_LOW },
> +};
> +
>  static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
>  
>  struct ad2s1210_state {
>  	const struct ad2s1210_platform_data *pdata;
>  	struct mutex lock;
>  	struct spi_device *sdev;
> +	struct gpio_desc *gpios[5];
>  	unsigned int fclkin;
>  	unsigned int fexcit;
>  	bool hysteresis;
> @@ -91,8 +121,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->gpios[AD2S1210_A0], ad2s1210_mode_vals[mode][0]);
> +	gpiod_set_value(st->gpios[AD2S1210_A1], ad2s1210_mode_vals[mode][1]);
>  	st->mode = mode;
>  }
>  
> @@ -152,8 +182,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->gpios[AD2S1210_RES0]) << 1) |
> +			  gpiod_get_value(st->gpios[AD2S1210_RES1]);
>  
>  	return ad2s1210_resolution_value[resolution];
>  }
> @@ -164,10 +194,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->gpios[AD2S1210_RES0],
> +			ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> +	gpiod_set_value(st->gpios[AD2S1210_RES1],
> +			ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
>  }
>  
>  static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
> @@ -401,15 +431,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->gpios[AD2S1210_SAMPLE], 0);
>  	/* delay (2 * tck + 20) nano seconds */
>  	udelay(1);
> -	gpio_set_value(st->pdata->sample, 1);
> +	gpiod_set_value(st->gpios[AD2S1210_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->gpios[AD2S1210_SAMPLE], 0);
> +	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
>  error_ret:
>  	mutex_unlock(&st->lock);
>  
> @@ -466,7 +496,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->gpios[AD2S1210_SAMPLE], 0);
>  	/* delay (6 * tck + 20) nano seconds */
>  	udelay(1);
>  
> @@ -512,7 +542,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
>  	}
>  
>  error_ret:
> -	gpio_set_value(st->pdata->sample, 1);
> +	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
>  	/* delay (2 * tck + 20) nano seconds */
>  	udelay(1);
>  	mutex_unlock(&st->lock);
> @@ -630,30 +660,23 @@ 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" },
> -	};
> -
> -	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" },
> -	};
> +	struct ad2s1210_gpio *pin = st->pdata->gpioin ? &gpios_in[0] : &gpios_out[0];
> +	struct spi_device *spi = st->sdev;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(gpios_in); 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[i].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 +715,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 +723,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 +730,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 v7 1/3] staging: iio: ad2s1210: Switch to the gpio descriptor interface

[Jonathan Cameron]

On Sat, 3 Nov 2018 12:45:09 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 31 Oct 2018 21:28:52 +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>  
> It would have been less 'noisy' to do these in the reverse order (drop
> the flag and support first, then do the gpiod conversion), but I suppose
> it doesn't really matter.
> 
> Applied to the togreg branch of iio.git and pushed out as testing
> for the autobuilders to play with it.
actually, couple of minor changes.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/staging/iio/resolver/ad2s1210.c | 106 ++++++++++++++----------
> >  drivers/staging/iio/resolver/ad2s1210.h |   3 -
> >  2 files changed, 62 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> > index ac13b99bd9cb..82ac9847f6f4 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>
> > @@ -67,12 +67,42 @@ enum ad2s1210_mode {
> >  	MOD_RESERVED,
> >  };
> >  
> > +enum ad2s1210_gpios {
> > +	AD2S1210_SAMPLE,
> > +	AD2S1210_A0,
> > +	AD2S1210_A1,
> > +	AD2S1210_RES0,
> > +	AD2S1210_RES1,
> > +};
> > +
> > +struct ad2s1210_gpio {
> > +	const char *name;
> > +	unsigned long flags;
> > +};
> > +
> > +static const struct ad2s1210_gpio gpios_in[] = {
> > +	[AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_IN },
> > +	[AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_IN },
> > +	[AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_IN },
> > +	[AD2S1210_RES0] = { .name = "adi,res0", .flags = GPIOD_IN },
> > +	[AD2S1210_RES1] = { .name = "adi,res1", .flags = GPIOD_IN },
> > +};
> > +
> > +static const struct ad2s1210_gpio gpios_out[] = {
> > +	[AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_OUT_LOW },
> > +	[AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_OUT_LOW },
> > +	[AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_OUT_LOW },
> > +	[AD2S1210_RES0] = { .name = "adi,res0", .flags = GPIOD_OUT_LOW },
> > +	[AD2S1210_RES1] = { .name = "adi,res1", .flags = GPIOD_OUT_LOW },
> > +};
> > +
> >  static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
> >  
> >  struct ad2s1210_state {
> >  	const struct ad2s1210_platform_data *pdata;
> >  	struct mutex lock;
> >  	struct spi_device *sdev;
> > +	struct gpio_desc *gpios[5];
> >  	unsigned int fclkin;
> >  	unsigned int fexcit;
> >  	bool hysteresis;
> > @@ -91,8 +121,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->gpios[AD2S1210_A0], ad2s1210_mode_vals[mode][0]);
> > +	gpiod_set_value(st->gpios[AD2S1210_A1], ad2s1210_mode_vals[mode][1]);
> >  	st->mode = mode;
> >  }
> >  
> > @@ -152,8 +182,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->gpios[AD2S1210_RES0]) << 1) |
> > +			  gpiod_get_value(st->gpios[AD2S1210_RES1]);
> >  
> >  	return ad2s1210_resolution_value[resolution];
> >  }
> > @@ -164,10 +194,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->gpios[AD2S1210_RES0],
> > +			ad2s1210_res_pins[(st->resolution - 10) / 2][0]);
> > +	gpiod_set_value(st->gpios[AD2S1210_RES1],
> > +			ad2s1210_res_pins[(st->resolution - 10) / 2][1]);
> >  }
> >  
> >  static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
> > @@ -401,15 +431,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->gpios[AD2S1210_SAMPLE], 0);
> >  	/* delay (2 * tck + 20) nano seconds */
> >  	udelay(1);
> > -	gpio_set_value(st->pdata->sample, 1);
> > +	gpiod_set_value(st->gpios[AD2S1210_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->gpios[AD2S1210_SAMPLE], 0);
> > +	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
> >  error_ret:
> >  	mutex_unlock(&st->lock);
> >  
> > @@ -466,7 +496,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->gpios[AD2S1210_SAMPLE], 0);
> >  	/* delay (6 * tck + 20) nano seconds */
> >  	udelay(1);
> >  
> > @@ -512,7 +542,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> >  	}
> >  
> >  error_ret:
> > -	gpio_set_value(st->pdata->sample, 1);
> > +	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
> >  	/* delay (2 * tck + 20) nano seconds */
> >  	udelay(1);
> >  	mutex_unlock(&st->lock);
> > @@ -630,30 +660,23 @@ 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" },
> > -	};
> > -
> > -	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" },
> > -	};
> > +	struct ad2s1210_gpio *pin = st->pdata->gpioin ? &gpios_in[0] : &gpios_out[0];
const struct ad2s1210_gpio
and the line is too long.

Fixed both.
> > +	struct spi_device *spi = st->sdev;
> > +	int i, ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(gpios_in); 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[i].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 +715,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 +723,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 +730,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 */  
> 

[PATCH v7 2/3] staging: iio: ad2s1210: Drop the gpioin flag.

[Nishad Kamdar]

Drop gpioin flag which decides how the GPIOs
are controlled as the GPIOs must be outputs
for the host as per the datasheet.

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

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 82ac9847f6f4..d3e7d5aad2c8 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -80,15 +80,7 @@ struct ad2s1210_gpio {
 	unsigned long flags;
 };
 
-static const struct ad2s1210_gpio gpios_in[] = {
-	[AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_IN },
-	[AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_IN },
-	[AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_IN },
-	[AD2S1210_RES0] = { .name = "adi,res0", .flags = GPIOD_IN },
-	[AD2S1210_RES1] = { .name = "adi,res1", .flags = GPIOD_IN },
-};
-
-static const struct ad2s1210_gpio gpios_out[] = {
+static const struct ad2s1210_gpio gpios[] = {
 	[AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_OUT_LOW },
 	[AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_OUT_LOW },
 	[AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_OUT_LOW },
@@ -99,7 +91,6 @@ static const struct ad2s1210_gpio gpios_out[] = {
 static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
 
 struct ad2s1210_state {
-	const struct ad2s1210_platform_data *pdata;
 	struct mutex lock;
 	struct spi_device *sdev;
 	struct gpio_desc *gpios[5];
@@ -180,14 +171,6 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)
 	return ad2s1210_config_write(st, fcw);
 }
 
-static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)
-{
-	int resolution = (gpiod_get_value(st->gpios[AD2S1210_RES0]) << 1) |
-			  gpiod_get_value(st->gpios[AD2S1210_RES1]);
-
-	return ad2s1210_resolution_value[resolution];
-}
-
 static const int ad2s1210_res_pins[4][2] = {
 	{ 0, 0 }, {0, 1}, {1, 0}, {1, 1}
 };
@@ -333,13 +316,7 @@ static ssize_t ad2s1210_store_control(struct device *dev,
 	}
 	st->resolution
 		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
-	if (st->pdata->gpioin) {
-		data = ad2s1210_read_resolution_pin(st);
-		if (data != st->resolution)
-			dev_warn(dev, "ad2s1210: resolution settings not match\n");
-	} else {
-		ad2s1210_set_resolution_pin(st);
-	}
+	ad2s1210_set_resolution_pin(st);
 	ret = len;
 	st->hysteresis = !!(data & AD2S1210_ENABLE_HYSTERESIS);
 
@@ -395,13 +372,7 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
 	}
 	st->resolution
 		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
-	if (st->pdata->gpioin) {
-		data = ad2s1210_read_resolution_pin(st);
-		if (data != st->resolution)
-			dev_warn(dev, "ad2s1210: resolution settings not match\n");
-	} else {
-		ad2s1210_set_resolution_pin(st);
-	}
+	ad2s1210_set_resolution_pin(st);
 	ret = len;
 error_ret:
 	mutex_unlock(&st->lock);
@@ -622,10 +593,7 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
 	int ret;
 
 	mutex_lock(&st->lock);
-	if (st->pdata->gpioin)
-		st->resolution = ad2s1210_read_resolution_pin(st);
-	else
-		ad2s1210_set_resolution_pin(st);
+	ad2s1210_set_resolution_pin(st);
 
 	ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
 	if (ret < 0)
@@ -660,11 +628,11 @@ static const struct iio_info ad2s1210_info = {
 
 static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
 {
-	struct ad2s1210_gpio *pin = st->pdata->gpioin ? &gpios_in[0] : &gpios_out[0];
+	struct ad2s1210_gpio *pin = &gpios[0];
 	struct spi_device *spi = st->sdev;
 	int i, ret;
 
-	for (i = 0; i < ARRAY_SIZE(gpios_in); i++) {
+	for (i = 0; i < ARRAY_SIZE(gpios); i++) {
 		st->gpios[i] = devm_gpiod_get(&spi->dev, pin[i].name,
 					      pin[i].flags);
 		if (IS_ERR(st->gpios[i])) {
@@ -692,7 +660,6 @@ static int ad2s1210_probe(struct spi_device *spi)
 	if (!indio_dev)
 		return -ENOMEM;
 	st = iio_priv(indio_dev);
-	st->pdata = spi->dev.platform_data;
 	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 v7 2/3] staging: iio: ad2s1210: Drop the gpioin flag.

[Jonathan Cameron]

On Wed, 31 Oct 2018 21:29:53 +0530
Nishad Kamdar <nishadkamdar@gmail.com> wrote:

> Drop gpioin flag which decides how the GPIOs
> are controlled as the GPIOs must be outputs
> for the host as per the datasheet.
> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
Whilst this does the right thing, it doesn't take advantage
of opportunities to clean up.  I've made a few changes in applying.
See below. Also added a note that this gets rid of the platform data.
Note you need to do git rm drivers/staging/iio/resolver/ad2s1210.h to
actually get rid of the file. + remove it from the includes. I did
that as well whilst here.

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

Thanks,

Jonathan


> ---
>  drivers/staging/iio/resolver/ad2s1210.c | 45 ++++---------------------
>  drivers/staging/iio/resolver/ad2s1210.h | 17 ----------
>  2 files changed, 6 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 82ac9847f6f4..d3e7d5aad2c8 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -80,15 +80,7 @@ struct ad2s1210_gpio {
>  	unsigned long flags;
>  };
>  
> -static const struct ad2s1210_gpio gpios_in[] = {
> -	[AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_IN },
> -	[AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_IN },
> -	[AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_IN },
> -	[AD2S1210_RES0] = { .name = "adi,res0", .flags = GPIOD_IN },
> -	[AD2S1210_RES1] = { .name = "adi,res1", .flags = GPIOD_IN },
> -};
> -
> -static const struct ad2s1210_gpio gpios_out[] = {
> +static const struct ad2s1210_gpio gpios[] = {
>  	[AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_OUT_LOW },
>  	[AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_OUT_LOW },
>  	[AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_OUT_LOW },
> @@ -99,7 +91,6 @@ static const struct ad2s1210_gpio gpios_out[] = {
>  static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
>  
>  struct ad2s1210_state {
> -	const struct ad2s1210_platform_data *pdata;
>  	struct mutex lock;
>  	struct spi_device *sdev;
>  	struct gpio_desc *gpios[5];
> @@ -180,14 +171,6 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)
>  	return ad2s1210_config_write(st, fcw);
>  }
>  
> -static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)
> -{
> -	int resolution = (gpiod_get_value(st->gpios[AD2S1210_RES0]) << 1) |
> -			  gpiod_get_value(st->gpios[AD2S1210_RES1]);
> -
> -	return ad2s1210_resolution_value[resolution];
> -}
> -
>  static const int ad2s1210_res_pins[4][2] = {
>  	{ 0, 0 }, {0, 1}, {1, 0}, {1, 1}
>  };
> @@ -333,13 +316,7 @@ static ssize_t ad2s1210_store_control(struct device *dev,
>  	}
>  	st->resolution
>  		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> -	if (st->pdata->gpioin) {
> -		data = ad2s1210_read_resolution_pin(st);
> -		if (data != st->resolution)
> -			dev_warn(dev, "ad2s1210: resolution settings not match\n");
> -	} else {
> -		ad2s1210_set_resolution_pin(st);
> -	}
> +	ad2s1210_set_resolution_pin(st);
>  	ret = len;
>  	st->hysteresis = !!(data & AD2S1210_ENABLE_HYSTERESIS);
>  
> @@ -395,13 +372,7 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
>  	}
>  	st->resolution
>  		= ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> -	if (st->pdata->gpioin) {
> -		data = ad2s1210_read_resolution_pin(st);
> -		if (data != st->resolution)
> -			dev_warn(dev, "ad2s1210: resolution settings not match\n");
> -	} else {
> -		ad2s1210_set_resolution_pin(st);
> -	}
> +	ad2s1210_set_resolution_pin(st);
>  	ret = len;
>  error_ret:
>  	mutex_unlock(&st->lock);
> @@ -622,10 +593,7 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
>  	int ret;
>  
>  	mutex_lock(&st->lock);
> -	if (st->pdata->gpioin)
> -		st->resolution = ad2s1210_read_resolution_pin(st);
> -	else
> -		ad2s1210_set_resolution_pin(st);
> +	ad2s1210_set_resolution_pin(st);
>  
>  	ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
>  	if (ret < 0)
> @@ -660,11 +628,11 @@ static const struct iio_info ad2s1210_info = {
>  
>  static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
>  {
> -	struct ad2s1210_gpio *pin = st->pdata->gpioin ? &gpios_in[0] : &gpios_out[0];
> +	struct ad2s1210_gpio *pin = &gpios[0];

This local parameter no longer does anything useful so dropped it.

>  	struct spi_device *spi = st->sdev;
>  	int i, ret;
>  
> -	for (i = 0; i < ARRAY_SIZE(gpios_in); i++) {
> +	for (i = 0; i < ARRAY_SIZE(gpios); i++) {
>  		st->gpios[i] = devm_gpiod_get(&spi->dev, pin[i].name,
>  					      pin[i].flags);
>  		if (IS_ERR(st->gpios[i])) {
> @@ -692,7 +660,6 @@ static int ad2s1210_probe(struct spi_device *spi)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  	st = iio_priv(indio_dev);
> -	st->pdata = spi->dev.platform_data;
>  	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 */

[PATCH v7 3/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 d3e7d5aad2c8..7c50def91a2b 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -701,6 +701,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" },
 	{}
@@ -710,6 +716,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

Re: [PATCH v7 3/3] staging: iio: ad2s1210: Add device tree table.

[Himanshu Jha]

On Wed, Oct 31, 2018 at 09:30:36PM +0530, Nishad Kamdar wrote:
> 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 d3e7d5aad2c8..7c50def91a2b 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -701,6 +701,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);

I believe this needs to be documented at:

Documentation/devicetree/bindings/iio/resolver/ad2s1210.txt

Cc'ed to devictree list + Rob(DT Maintainer).

Just wondering why didn't it came up till now from the IIO reviewers ? v7!!


-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Re: [PATCH v7 3/3] staging: iio: ad2s1210: Add device tree table.

[Jonathan Cameron]

On Thu, 1 Nov 2018 21:05:09 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Wed, Oct 31, 2018 at 09:30:36PM +0530, Nishad Kamdar wrote:
> > 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 d3e7d5aad2c8..7c50def91a2b 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -701,6 +701,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);  
> 
> I believe this needs to be documented at:
> 
> Documentation/devicetree/bindings/iio/resolver/ad2s1210.txt
> 
> Cc'ed to devictree list + Rob(DT Maintainer).
> 
> Just wondering why didn't it came up till now from the IIO reviewers ? v7!!

Because in staging drivers graduations we often hold off doing the
dt-bindings document until we have full visibility of where we are going.

A lot of them have dodgy DT bindings (and that might even be the reason
they are in staging).  What we don't want is to have a doc for a silly
binding in the 'official' list as we'll have to support it for ever.

It needs documenting before moving out staging, but not necessarily now.
Particularly as this device is complex and has a 'lot' of other stuff
that isn't currently supported and quite possibly never will be.
Some of that would have non obvious dt bindings if we did support it.
For example we 'might' route the encoder outputs round to the inputs
of a counter driver and end up with a complex entity representing
the facilities that both fo them provide.

Agreed, the DT binding doc needs to come soon and before the move out
staging, but I am quite happy with it being in the next series.

A line in the description to that effect would have been useful of
course!

Jonathan

> 
> 

Re: [PATCH v7 3/3] staging: iio: ad2s1210: Add device tree table.

[Jonathan Cameron]

On Sat, 3 Nov 2018 12:39:27 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu, 1 Nov 2018 21:05:09 +0530
> Himanshu Jha <himanshujha199640@gmail.com> wrote:
> 
> > On Wed, Oct 31, 2018 at 09:30:36PM +0530, Nishad Kamdar wrote:  
> > > 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 d3e7d5aad2c8..7c50def91a2b 100644
> > > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > > @@ -701,6 +701,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);    
> > 
> > I believe this needs to be documented at:
> > 
> > Documentation/devicetree/bindings/iio/resolver/ad2s1210.txt
> > 
> > Cc'ed to devictree list + Rob(DT Maintainer).
> > 
> > Just wondering why didn't it came up till now from the IIO reviewers ? v7!!  
> 
> Because in staging drivers graduations we often hold off doing the
> dt-bindings document until we have full visibility of where we are going.
> 
> A lot of them have dodgy DT bindings (and that might even be the reason
> they are in staging).  What we don't want is to have a doc for a silly
> binding in the 'official' list as we'll have to support it for ever.
> 
> It needs documenting before moving out staging, but not necessarily now.
> Particularly as this device is complex and has a 'lot' of other stuff
> that isn't currently supported and quite possibly never will be.
> Some of that would have non obvious dt bindings if we did support it.
> For example we 'might' route the encoder outputs round to the inputs
> of a counter driver and end up with a complex entity representing
> the facilities that both fo them provide.
> 
> Agreed, the DT binding doc needs to come soon and before the move out
> staging, but I am quite happy with it being in the next series.
> 
> A line in the description to that effect would have been useful of
> course!
> 
Applied, with a line on the intent to document once driver is cleaned
up added.

Thanks,

Jonathan
> Jonathan
> 
> > 
> >   
>