staging: iio: ad2s1210: Switch to the gpio descriptor interface.

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

Signed-off-by Nishad Kamdar <[email protected]>

---
 drivers/staging/iio/resolver/ad2s1210.c | 92 ++++++++++++++-----------
 drivers/staging/iio/resolver/ad2s1210.h |  3 -
 2 files changed, 50 insertions(+), 45 deletions(-)

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

index ac13b99bd9cb..93c3c70ce62e 100644
@@ -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 };
 

drivers/staging/iio/resolver/ad2s1210.c +18 -13

+	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);
 

drivers/staging/iio/resolver/ad2s1210.c +0 -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;

drivers/staging/iio/resolver/ad2s1210.c +13 -20

 	};
 
-	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;
 }

drivers/staging/iio/resolver/ad2s1210.h +0 -3

index e9b2147701fc..63d479b20a6c 100644
@@ -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 */

Quoted 3 lines · On Sun, 28 Oct 2018 13:21:25 +0530 Nishad Kamdar <[email protected]> wrote:

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

Quoted 1 line

Signed-off-by Nishad Kamdar <[email protected]>

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

Quoted 22 lines

---
 drivers/staging/iio/resolver/ad2s1210.c | 92 ++++++++++++++-----------
 drivers/staging/iio/resolver/ad2s1210.h |  3 -
 2 files changed, 50 insertions(+), 45 deletions(-)

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

index ac13b99bd9cb..93c3c70ce62e 100644
@@ -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 };
 

drivers/staging/iio/resolver/ad2s1210.c enum ad2s1210_mode { · line 81

Quoted 10 lines

drivers/staging/iio/resolver/ad2s1210.c +6 -0

+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];

Quoted 75 lines

drivers/staging/iio/resolver/ad2s1210.c +18 -13

+	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);
 

drivers/staging/iio/resolver/ad2s1210.c static int ad2s1210_read_raw(struct iio_dev *indio_dev, · line 526

Quoted 5 lines

drivers/staging/iio/resolver/ad2s1210.c +1 -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);

Quoted 5 lines

drivers/staging/iio/resolver/ad2s1210.c +0 -0

 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
 	mutex_unlock(&st->lock);

@@ -630,30 +641,32 @@ static const struct iio_info ad2s1210_info = {
 

drivers/staging/iio/resolver/ad2s1210.c static const struct iio_info ad2s1210_info = { · line 647

Quoted 13 lines

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

 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?

drivers/staging/iio/resolver/ad2s1210.c static const struct iio_info ad2s1210_info = { · line 654

Quoted 7 lines

drivers/staging/iio/resolver/ad2s1210.c +7 -0

+
+	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;
}

Quoted 67 lines

drivers/staging/iio/resolver/ad2s1210.c +13 -20

 	};
 
-	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;
 }

drivers/staging/iio/resolver/ad2s1210.h +0 -0

index e9b2147701fc..63d479b20a6c 100644
@@ -12,9 +12,6 @@
 #define _AD2S1210_H
 

drivers/staging/iio/resolver/ad2s1210.h line 17

Quoted 7 lines

drivers/staging/iio/resolver/ad2s1210.h +0 -3

 struct ad2s1210_platform_data {
-	unsigned int		sample;
-	unsigned int		a[2];
-	unsigned int		res[2];
 	bool			gpioin;
 };
 #endif /* _AD2S1210_H */

Replace platform data with device tree support.

Signed-off-by Nishad Kamdar <[email protected]>

---
 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(-)

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

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

drivers/staging/iio/resolver/ad2s1210.c +7 -5

index 0234869e9d74..5ecdb5785132 100644
@@ -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;

drivers/staging/iio/resolver/ad2s1210.c +0 -0

 	ret = ad2s1210_setup_gpios(st);
 	if (ret < 0)
 		return ret;

drivers/staging/iio/resolver/ad2s1210.h +0 -17

index 63d479b20a6c..e69de29bb2d1 100644
@@ -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 */

Quoted 3 lines · On Sun, 28 Oct 2018 13:23:23 +0530 Nishad Kamdar <[email protected]> wrote:

Replace platform data with device tree support.

Signed-off-by Nishad Kamdar <[email protected]>

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

Quoted 88 lines

---
 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(-)

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

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

drivers/staging/iio/resolver/ad2s1210.c +7 -5

index 0234869e9d74..5ecdb5785132 100644
@@ -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;

drivers/staging/iio/resolver/ad2s1210.c static int ad2s1210_probe(struct spi_device *spi) · line 684

Quoted 10 lines

drivers/staging/iio/resolver/ad2s1210.c +1 -4

 
-	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.

Quoted 17 lines

drivers/staging/iio/resolver/ad2s1210.c +0 -0

 	ret = ad2s1210_setup_gpios(st);
 	if (ret < 0)
 		return ret;

drivers/staging/iio/resolver/ad2s1210.h +0 -9

index 63d479b20a6c..e69de29bb2d1 100644
@@ -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.

drivers/staging/iio/resolver/ad2s1210.h line 17

Quoted 8 lines

drivers/staging/iio/resolver/ad2s1210.h +0 -8

- */
-#ifndef _AD2S1210_H
-#define _AD2S1210_H
-
-struct ad2s1210_platform_data {
-	bool			gpioin;
-};
-#endif /* _AD2S1210_H */