diff mbox series

[v3,13/27] staging: iio: resolver: ad2s1210: rework gpios

Message ID 20230929-ad2s1210-mainline-v3-13-fa4364281745@baylibre.com (mailing list archive)
State Accepted
Headers show
Series iio: resolver: move ad2s1210 out of staging | expand

Commit Message

David Lechner Sept. 29, 2023, 5:23 p.m. UTC
From: David Lechner <david@lechnology.com>

From: David Lechner <dlechner@baylibre.com>

- Remove "adi," prefix from gpio names.
- Sample gpio is now expected to be active low.
- Convert A0 and A1 gpios to "mode-gpios" gpio array.
- Convert RES0 and RES1 gpios to "resolution-gpios" gpio array.
- Remove extraneous lookup tables.
- Remove unused mode field from state struct.
- Swap argument order of ad2s1210_set_mode() while we are touching this.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

v3 changes:
* Fixed multiline comment style.

 drivers/staging/iio/resolver/ad2s1210.c | 164 +++++++++++++++++---------------
 1 file changed, 85 insertions(+), 79 deletions(-)

Comments

Jonathan Cameron Sept. 30, 2023, 2:55 p.m. UTC | #1
On Fri, 29 Sep 2023 12:23:18 -0500
David Lechner <dlechner@baylibre.com> wrote:

> From: David Lechner <david@lechnology.com>
> 
> From: David Lechner <dlechner@baylibre.com>
> 
> - Remove "adi," prefix from gpio names.
> - Sample gpio is now expected to be active low.
> - Convert A0 and A1 gpios to "mode-gpios" gpio array.
> - Convert RES0 and RES1 gpios to "resolution-gpios" gpio array.
> - Remove extraneous lookup tables.
> - Remove unused mode field from state struct.
> - Swap argument order of ad2s1210_set_mode() while we are touching this.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Applied,

Thanks,
Jonathan

> ---
> 
> v3 changes:
> * Fixed multiline comment style.
> 
>  drivers/staging/iio/resolver/ad2s1210.c | 164 +++++++++++++++++---------------
>  1 file changed, 85 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 2b9377447f6a..0ec3598b600a 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -58,39 +58,21 @@
>  #define AD2S1210_DEF_EXCIT	10000
>  
>  enum ad2s1210_mode {
> -	MOD_POS = 0,
> -	MOD_VEL,
> -	MOD_CONFIG,
> -	MOD_RESERVED,
> +	MOD_POS = 0b00,
> +	MOD_VEL = 0b01,
> +	MOD_RESERVED = 0b10,
> +	MOD_CONFIG = 0b11,
>  };
>  
> -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[] = {
> -	[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 {
>  	struct mutex lock;
>  	struct spi_device *sdev;
> -	struct gpio_desc *gpios[5];
> +	/** GPIO pin connected to SAMPLE line. */
> +	struct gpio_desc *sample_gpio;
> +	/** GPIO pins connected to A0 and A1 lines. */
> +	struct gpio_descs *mode_gpios;
> +	/** GPIO pins connected to RES0 and RES1 lines. */
> +	struct gpio_descs *resolution_gpios;
>  	/** Used to access config registers. */
>  	struct regmap *regmap;
>  	/** The external oscillator frequency in Hz. */
> @@ -98,7 +80,6 @@ struct ad2s1210_state {
>  	unsigned int fexcit;
>  	bool hysteresis;
>  	u8 resolution;
> -	enum ad2s1210_mode mode;
>  	/** For reading raw sample value via SPI. */
>  	__be16 sample __aligned(IIO_DMA_MINALIGN);
>  	/** SPI transmit buffer. */
> @@ -107,18 +88,15 @@ struct ad2s1210_state {
>  	u8 tx[2];
>  };
>  
> -static const int ad2s1210_mode_vals[4][2] = {
> -	[MOD_POS] = { 0, 0 },
> -	[MOD_VEL] = { 0, 1 },
> -	[MOD_CONFIG] = { 1, 1 },
> -};
> -
> -static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
> -				     struct ad2s1210_state *st)
> +static int ad2s1210_set_mode(struct ad2s1210_state *st, enum ad2s1210_mode mode)
>  {
> -	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;
> +	struct gpio_descs *gpios = st->mode_gpios;
> +	DECLARE_BITMAP(bitmap, 2);
> +
> +	bitmap[0] = mode;
> +
> +	return gpiod_set_array_value(gpios->ndescs, gpios->desc, gpios->info,
> +				     bitmap);
>  }
>  
>  /*
> @@ -143,6 +121,7 @@ static int ad2s1210_regmap_reg_write(void *context, unsigned int reg,
>  			.tx_buf = &st->tx[1],
>  		},
>  	};
> +	int ret;
>  
>  	/* values can only be 7 bits, the MSB indicates an address */
>  	if (val & ~0x7F)
> @@ -151,7 +130,9 @@ static int ad2s1210_regmap_reg_write(void *context, unsigned int reg,
>  	st->tx[0] = reg;
>  	st->tx[1] = val;
>  
> -	ad2s1210_set_mode(MOD_CONFIG, st);
> +	ret = ad2s1210_set_mode(st, MOD_CONFIG);
> +	if (ret < 0)
> +		return ret;
>  
>  	return spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
>  }
> @@ -180,7 +161,10 @@ static int ad2s1210_regmap_reg_read(void *context, unsigned int reg,
>  	};
>  	int ret;
>  
> -	ad2s1210_set_mode(MOD_CONFIG, st);
> +	ret = ad2s1210_set_mode(st, MOD_CONFIG);
> +	if (ret < 0)
> +		return ret;
> +
>  	st->tx[0] = reg;
>  	/*
>  	 * Must be valid register address here otherwise this could write data.
> @@ -219,16 +203,16 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)
>  	return regmap_write(st->regmap, AD2S1210_REG_EXCIT_FREQ, fcw);
>  }
>  
> -static const int ad2s1210_res_pins[4][2] = {
> -	{ 0, 0 }, {0, 1}, {1, 0}, {1, 1}
> -};
> -
> -static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
> +static int ad2s1210_set_resolution_gpios(struct ad2s1210_state *st,
> +					 u8 resolution)
>  {
> -	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]);
> +	struct gpio_descs *gpios = st->resolution_gpios;
> +	DECLARE_BITMAP(bitmap, 2);
> +
> +	bitmap[0] = (resolution - 10) >> 1;
> +
> +	return gpiod_set_array_value(gpios->ndescs, gpios->desc, gpios->info,
> +				     bitmap);
>  }
>  
>  static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
> @@ -305,10 +289,13 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
>  	if (ret < 0)
>  		goto error_ret;
>  
> -	st->resolution =
> -		ad2s1210_resolution_value[data & AD2S1210_SET_RES];
> -	ad2s1210_set_resolution_pin(st);
> +	ret = ad2s1210_set_resolution_gpios(st, udata);
> +	if (ret < 0)
> +		goto error_ret;
> +
> +	st->resolution = udata;
>  	ret = len;
> +
>  error_ret:
>  	mutex_unlock(&st->lock);
>  	return ret;
> @@ -339,15 +326,19 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
>  	int ret;
>  
>  	mutex_lock(&st->lock);
> -	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 0);
> +
> +	gpiod_set_value(st->sample_gpio, 1);
>  	/* delay (2 * tck + 20) nano seconds */
>  	udelay(1);
> -	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
> +	gpiod_set_value(st->sample_gpio, 0);
> +
>  	ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &value);
>  	if (ret < 0)
>  		goto error_ret;
> -	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 0);
> -	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
> +
> +	gpiod_set_value(st->sample_gpio, 1);
> +	gpiod_set_value(st->sample_gpio, 0);
> +
>  error_ret:
>  	mutex_unlock(&st->lock);
>  
> @@ -393,19 +384,19 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st,
>  				      struct iio_chan_spec const *chan,
>  				      int *val)
>  {
> -	int ret = 0;
> +	int ret;
>  
>  	mutex_lock(&st->lock);
> -	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 0);
> +	gpiod_set_value(st->sample_gpio, 1);
>  	/* delay (6 * tck + 20) nano seconds */
>  	udelay(1);
>  
>  	switch (chan->type) {
>  	case IIO_ANGL:
> -		ad2s1210_set_mode(MOD_POS, st);
> +		ret = ad2s1210_set_mode(st, MOD_POS);
>  		break;
>  	case IIO_ANGL_VEL:
> -		ad2s1210_set_mode(MOD_VEL, st);
> +		ret = ad2s1210_set_mode(st, MOD_VEL);
>  		break;
>  	default:
>  		ret = -EINVAL;
> @@ -432,7 +423,7 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st,
>  	}
>  
>  error_ret:
> -	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
> +	gpiod_set_value(st->sample_gpio, 0);
>  	/* delay (2 * tck + 20) nano seconds */
>  	udelay(1);
>  	mutex_unlock(&st->lock);
> @@ -546,7 +537,9 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
>  	int ret;
>  
>  	mutex_lock(&st->lock);
> -	ad2s1210_set_resolution_pin(st);
> +	ret = ad2s1210_set_resolution_gpios(st, st->resolution);
> +	if (ret < 0)
> +		return ret;
>  
>  	/* Use default config register value plus resolution from devicetree. */
>  	data = FIELD_PREP(AD2S1210_PHASE_LOCK_RANGE_44, 1);
> @@ -612,20 +605,34 @@ static int ad2s1210_setup_clocks(struct ad2s1210_state *st)
>  
>  static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
>  {
> -	struct spi_device *spi = st->sdev;
> -	int i, ret;
> -
> -	for (i = 0; i < ARRAY_SIZE(gpios); i++) {
> -		st->gpios[i] = devm_gpiod_get(&spi->dev, gpios[i].name,
> -					      gpios[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",
> -				gpios[i].name, ret);
> -			return ret;
> -		}
> -	}
> +	struct device *dev = &st->sdev->dev;
> +
> +	/* should not be sampling on startup */
> +	st->sample_gpio = devm_gpiod_get(dev, "sample", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->sample_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->sample_gpio),
> +				     "failed to request sample GPIO\n");
> +
> +	/* both pins high means that we start in config mode */
> +	st->mode_gpios = devm_gpiod_get_array(dev, "mode", GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->mode_gpios))
> +		return dev_err_probe(dev, PTR_ERR(st->mode_gpios),
> +				     "failed to request mode GPIOs\n");
> +
> +	if (st->mode_gpios->ndescs != 2)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "requires exactly 2 mode-gpios\n");
> +
> +	/* both pins high means that we start with 16-bit resolution */
> +	st->resolution_gpios = devm_gpiod_get_array(dev, "resolution",
> +						    GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->resolution_gpios))
> +		return dev_err_probe(dev, PTR_ERR(st->resolution_gpios),
> +				     "failed to request resolution GPIOs\n");
> +
> +	if (st->resolution_gpios->ndescs != 2)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "requires exactly 2 resolution-gpios\n");
>  
>  	return 0;
>  }
> @@ -690,7 +697,6 @@ static int ad2s1210_probe(struct spi_device *spi)
>  	mutex_init(&st->lock);
>  	st->sdev = spi;
>  	st->hysteresis = true;
> -	st->mode = MOD_CONFIG;
>  	st->resolution = 12;
>  	st->fexcit = AD2S1210_DEF_EXCIT;
>  
>
Jonathan Cameron Oct. 5, 2023, 1:38 p.m. UTC | #2
On Sat, 30 Sep 2023 15:55:36 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 29 Sep 2023 12:23:18 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > From: David Lechner <david@lechnology.com>
> > 
> > From: David Lechner <dlechner@baylibre.com>
> > 
> > - Remove "adi," prefix from gpio names.
> > - Sample gpio is now expected to be active low.
> > - Convert A0 and A1 gpios to "mode-gpios" gpio array.
> > - Convert RES0 and RES1 gpios to "resolution-gpios" gpio array.
> > - Remove extraneous lookup tables.
> > - Remove unused mode field from state struct.
> > - Swap argument order of ad2s1210_set_mode() while we are touching this.
> > 
> > Signed-off-by: David Lechner <dlechner@baylibre.com>  
> Applied,
0-day ran smatch on this and it picked up that a log isn't released in
an error path.  I've fixed that up with a goto error_ret and will push out a
fresh testing branch for 0-day to take another look at.

...

> > @@ -546,7 +537,9 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
> >  	int ret;
> >  
> >  	mutex_lock(&st->lock);
> > -	ad2s1210_set_resolution_pin(st);
> > +	ret = ad2s1210_set_resolution_gpios(st, st->resolution);
> > +	if (ret < 0)
Exiting with lock held.  There is an error_ret label that releases the lock
so use that.

> > +		return ret;
> >  
> >  	/* Use default config register value plus resolution from devicetree. */
> >  	data = FIELD_PREP(AD2S1210_PHASE_LOCK_RANGE_44, 1);
diff mbox series

Patch

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 2b9377447f6a..0ec3598b600a 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -58,39 +58,21 @@ 
 #define AD2S1210_DEF_EXCIT	10000
 
 enum ad2s1210_mode {
-	MOD_POS = 0,
-	MOD_VEL,
-	MOD_CONFIG,
-	MOD_RESERVED,
+	MOD_POS = 0b00,
+	MOD_VEL = 0b01,
+	MOD_RESERVED = 0b10,
+	MOD_CONFIG = 0b11,
 };
 
-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[] = {
-	[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 {
 	struct mutex lock;
 	struct spi_device *sdev;
-	struct gpio_desc *gpios[5];
+	/** GPIO pin connected to SAMPLE line. */
+	struct gpio_desc *sample_gpio;
+	/** GPIO pins connected to A0 and A1 lines. */
+	struct gpio_descs *mode_gpios;
+	/** GPIO pins connected to RES0 and RES1 lines. */
+	struct gpio_descs *resolution_gpios;
 	/** Used to access config registers. */
 	struct regmap *regmap;
 	/** The external oscillator frequency in Hz. */
@@ -98,7 +80,6 @@  struct ad2s1210_state {
 	unsigned int fexcit;
 	bool hysteresis;
 	u8 resolution;
-	enum ad2s1210_mode mode;
 	/** For reading raw sample value via SPI. */
 	__be16 sample __aligned(IIO_DMA_MINALIGN);
 	/** SPI transmit buffer. */
@@ -107,18 +88,15 @@  struct ad2s1210_state {
 	u8 tx[2];
 };
 
-static const int ad2s1210_mode_vals[4][2] = {
-	[MOD_POS] = { 0, 0 },
-	[MOD_VEL] = { 0, 1 },
-	[MOD_CONFIG] = { 1, 1 },
-};
-
-static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
-				     struct ad2s1210_state *st)
+static int ad2s1210_set_mode(struct ad2s1210_state *st, enum ad2s1210_mode mode)
 {
-	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;
+	struct gpio_descs *gpios = st->mode_gpios;
+	DECLARE_BITMAP(bitmap, 2);
+
+	bitmap[0] = mode;
+
+	return gpiod_set_array_value(gpios->ndescs, gpios->desc, gpios->info,
+				     bitmap);
 }
 
 /*
@@ -143,6 +121,7 @@  static int ad2s1210_regmap_reg_write(void *context, unsigned int reg,
 			.tx_buf = &st->tx[1],
 		},
 	};
+	int ret;
 
 	/* values can only be 7 bits, the MSB indicates an address */
 	if (val & ~0x7F)
@@ -151,7 +130,9 @@  static int ad2s1210_regmap_reg_write(void *context, unsigned int reg,
 	st->tx[0] = reg;
 	st->tx[1] = val;
 
-	ad2s1210_set_mode(MOD_CONFIG, st);
+	ret = ad2s1210_set_mode(st, MOD_CONFIG);
+	if (ret < 0)
+		return ret;
 
 	return spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
 }
@@ -180,7 +161,10 @@  static int ad2s1210_regmap_reg_read(void *context, unsigned int reg,
 	};
 	int ret;
 
-	ad2s1210_set_mode(MOD_CONFIG, st);
+	ret = ad2s1210_set_mode(st, MOD_CONFIG);
+	if (ret < 0)
+		return ret;
+
 	st->tx[0] = reg;
 	/*
 	 * Must be valid register address here otherwise this could write data.
@@ -219,16 +203,16 @@  int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)
 	return regmap_write(st->regmap, AD2S1210_REG_EXCIT_FREQ, fcw);
 }
 
-static const int ad2s1210_res_pins[4][2] = {
-	{ 0, 0 }, {0, 1}, {1, 0}, {1, 1}
-};
-
-static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
+static int ad2s1210_set_resolution_gpios(struct ad2s1210_state *st,
+					 u8 resolution)
 {
-	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]);
+	struct gpio_descs *gpios = st->resolution_gpios;
+	DECLARE_BITMAP(bitmap, 2);
+
+	bitmap[0] = (resolution - 10) >> 1;
+
+	return gpiod_set_array_value(gpios->ndescs, gpios->desc, gpios->info,
+				     bitmap);
 }
 
 static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
@@ -305,10 +289,13 @@  static ssize_t ad2s1210_store_resolution(struct device *dev,
 	if (ret < 0)
 		goto error_ret;
 
-	st->resolution =
-		ad2s1210_resolution_value[data & AD2S1210_SET_RES];
-	ad2s1210_set_resolution_pin(st);
+	ret = ad2s1210_set_resolution_gpios(st, udata);
+	if (ret < 0)
+		goto error_ret;
+
+	st->resolution = udata;
 	ret = len;
+
 error_ret:
 	mutex_unlock(&st->lock);
 	return ret;
@@ -339,15 +326,19 @@  static ssize_t ad2s1210_clear_fault(struct device *dev,
 	int ret;
 
 	mutex_lock(&st->lock);
-	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 0);
+
+	gpiod_set_value(st->sample_gpio, 1);
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
-	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
+	gpiod_set_value(st->sample_gpio, 0);
+
 	ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &value);
 	if (ret < 0)
 		goto error_ret;
-	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 0);
-	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
+
+	gpiod_set_value(st->sample_gpio, 1);
+	gpiod_set_value(st->sample_gpio, 0);
+
 error_ret:
 	mutex_unlock(&st->lock);
 
@@ -393,19 +384,19 @@  static int ad2s1210_single_conversion(struct ad2s1210_state *st,
 				      struct iio_chan_spec const *chan,
 				      int *val)
 {
-	int ret = 0;
+	int ret;
 
 	mutex_lock(&st->lock);
-	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 0);
+	gpiod_set_value(st->sample_gpio, 1);
 	/* delay (6 * tck + 20) nano seconds */
 	udelay(1);
 
 	switch (chan->type) {
 	case IIO_ANGL:
-		ad2s1210_set_mode(MOD_POS, st);
+		ret = ad2s1210_set_mode(st, MOD_POS);
 		break;
 	case IIO_ANGL_VEL:
-		ad2s1210_set_mode(MOD_VEL, st);
+		ret = ad2s1210_set_mode(st, MOD_VEL);
 		break;
 	default:
 		ret = -EINVAL;
@@ -432,7 +423,7 @@  static int ad2s1210_single_conversion(struct ad2s1210_state *st,
 	}
 
 error_ret:
-	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
+	gpiod_set_value(st->sample_gpio, 0);
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
 	mutex_unlock(&st->lock);
@@ -546,7 +537,9 @@  static int ad2s1210_initial(struct ad2s1210_state *st)
 	int ret;
 
 	mutex_lock(&st->lock);
-	ad2s1210_set_resolution_pin(st);
+	ret = ad2s1210_set_resolution_gpios(st, st->resolution);
+	if (ret < 0)
+		return ret;
 
 	/* Use default config register value plus resolution from devicetree. */
 	data = FIELD_PREP(AD2S1210_PHASE_LOCK_RANGE_44, 1);
@@ -612,20 +605,34 @@  static int ad2s1210_setup_clocks(struct ad2s1210_state *st)
 
 static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
 {
-	struct spi_device *spi = st->sdev;
-	int i, ret;
-
-	for (i = 0; i < ARRAY_SIZE(gpios); i++) {
-		st->gpios[i] = devm_gpiod_get(&spi->dev, gpios[i].name,
-					      gpios[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",
-				gpios[i].name, ret);
-			return ret;
-		}
-	}
+	struct device *dev = &st->sdev->dev;
+
+	/* should not be sampling on startup */
+	st->sample_gpio = devm_gpiod_get(dev, "sample", GPIOD_OUT_LOW);
+	if (IS_ERR(st->sample_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->sample_gpio),
+				     "failed to request sample GPIO\n");
+
+	/* both pins high means that we start in config mode */
+	st->mode_gpios = devm_gpiod_get_array(dev, "mode", GPIOD_OUT_HIGH);
+	if (IS_ERR(st->mode_gpios))
+		return dev_err_probe(dev, PTR_ERR(st->mode_gpios),
+				     "failed to request mode GPIOs\n");
+
+	if (st->mode_gpios->ndescs != 2)
+		return dev_err_probe(dev, -EINVAL,
+				     "requires exactly 2 mode-gpios\n");
+
+	/* both pins high means that we start with 16-bit resolution */
+	st->resolution_gpios = devm_gpiod_get_array(dev, "resolution",
+						    GPIOD_OUT_HIGH);
+	if (IS_ERR(st->resolution_gpios))
+		return dev_err_probe(dev, PTR_ERR(st->resolution_gpios),
+				     "failed to request resolution GPIOs\n");
+
+	if (st->resolution_gpios->ndescs != 2)
+		return dev_err_probe(dev, -EINVAL,
+				     "requires exactly 2 resolution-gpios\n");
 
 	return 0;
 }
@@ -690,7 +697,6 @@  static int ad2s1210_probe(struct spi_device *spi)
 	mutex_init(&st->lock);
 	st->sdev = spi;
 	st->hysteresis = true;
-	st->mode = MOD_CONFIG;
 	st->resolution = 12;
 	st->fexcit = AD2S1210_DEF_EXCIT;