diff mbox series

[v2,09/19] staging: iio: resolver: ad2s1210: use regmap for config registers

Message ID 20230921144400.62380-10-dlechner@baylibre.com
State New
Headers
Series iio: resolver: move ad2s1210 out of staging |

Commit Message

David Lechner Sept. 21, 2023, 2:43 p.m. UTC 
  This makes use of the regmap API to read and write the configuration
registers. This simplifies code quite a bit and makes it safer
(previously, it was easy to write a bad value to the config registers
which causes the chip to lock up and need to be reset).

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/staging/iio/resolver/ad2s1210.c | 251 ++++++++++++++----------
 1 file changed, 149 insertions(+), 102 deletions(-)

Comments

Jonathan Cameron Sept. 24, 2023, 5:59 p.m. UTC | #1 
On Thu, 21 Sep 2023 09:43:50 -0500
David Lechner <dlechner@baylibre.com> wrote:

> This makes use of the regmap API to read and write the configuration
> registers. This simplifies code quite a bit and makes it safer
> (previously, it was easy to write a bad value to the config registers
> which causes the chip to lock up and need to be reset).
> 
I'd like a bit more description in here -mostly because I have no idea
what the original code was doing.
What were the MSB writes that followed main config writes for and why
can we get rid of them>

Also, using regmap for only some accesses is a bit unusual. Add some
text here to justify that decision.

Jonathan

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


> -/* write 1 bytes (address or data) to the chip */
> -static int ad2s1210_config_write(struct ad2s1210_state *st, u8 data)
> +/*
> + * Writes the given data to the given register address.
> + *
> + * If the mode is configurable, the device will first be placed in
> + * configuration mode.
> + */
> +static int ad2s1210_regmap_reg_write(void *context, unsigned int reg,
> +				     unsigned int val)
>  {
> -	int ret;
> +	struct ad2s1210_state *st = context;
> +	struct spi_transfer xfers[] = {
> +		{
> +			.len = 1,
> +			.rx_buf = &st->rx[0],
> +			.tx_buf = &st->tx[0],
> +			.cs_change = 1,
> +		}, {
> +			.len = 1,
> +			.rx_buf = &st->rx[1],
> +			.tx_buf = &st->tx[1],
> +		},
> +	};
> +
> +	/* values can only be 7 bits, the MSB indicates an address */
> +	if (val & ~0x7F)
> +		return -EINVAL;
> +
> +	st->tx[0] = reg;
> +	st->tx[1] = val;
>  
>  	ad2s1210_set_mode(MOD_CONFIG, st);
> -	st->tx[0] = data;
> -	ret = spi_write(st->sdev, st->tx, 1);
> -	if (ret < 0)
> -		return ret;
>  
> -	return 0;
> +	return spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
>  }
>  
> -/* read value from one of the registers */
> -static int ad2s1210_config_read(struct ad2s1210_state *st,
> -				unsigned char address)
> +/*
> + * Reads value from one of the registers.
> + *
> + * If the mode is configurable, the device will first be placed in
> + * configuration mode.
> + */
> +static int ad2s1210_regmap_reg_read(void *context, unsigned int reg,
> +				    unsigned int *val)
>  {
> +	struct ad2s1210_state *st = context;
>  	struct spi_transfer xfers[] = {
>  		{
>  			.len = 1,
> @@ -146,22 +176,34 @@ static int ad2s1210_config_read(struct ad2s1210_state *st,
>  			.tx_buf = &st->tx[1],
>  		},
>  	};
> -	int ret = 0;
> +	int ret;
>  
>  	ad2s1210_set_mode(MOD_CONFIG, st);
> -	st->tx[0] = address | AD2S1210_MSB_IS_HIGH;
> +	st->tx[0] = reg;
> +	/* Must be valid register address here otherwise this could write data.
> +	 * It doesn't matter which one.
> +	 */
>  	st->tx[1] = AD2S1210_REG_FAULT;
> -	ret = spi_sync_transfer(st->sdev, xfers, 2);
> +
> +	ret = spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
>  	if (ret < 0)
>  		return ret;
>  
> -	return st->rx[1];
> +	/* If the D7 bit is set on any read/write register, it indicates a

IIO comments are
	/*
	 * If ...

> +	 * parity error. The fault register is read-only and the D7 bit means
> +	 * something else there.
> +	 */
> +	if (reg != AD2S1210_REG_FAULT && st->rx[1] & AD2S1210_ADDRESS_DATA)
> +		return -EBADMSG;
> +
> +	*val = st->rx[1];
> +
> +	return 0;
>  }
>


>  static ssize_t ad2s1210_store_control(struct device *dev,
> @@ -264,25 +297,13 @@ static ssize_t ad2s1210_store_control(struct device *dev,
>  		return -EINVAL;
>  
>  	mutex_lock(&st->lock);
> -	ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
> -	if (ret < 0)
> -		goto error_ret;
> -	data = udata & AD2S1210_MSB_IS_LOW;
> -	ret = ad2s1210_config_write(st, data);
> +	data = udata & ~AD2S1210_ADDRESS_DATA;
> +	ret = regmap_write(st->regmap, AD2S1210_REG_CONTROL, data);
>  	if (ret < 0)
>  		goto error_ret;
>  
> -	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
> -	if (ret < 0)
> -		goto error_ret;
> -	if (ret & AD2S1210_MSB_IS_HIGH) {
> -		ret = -EIO;
> -		dev_err(dev,
> -			"ad2s1210: write control register fail\n");
> -		goto error_ret;
> -	}
>  	st->resolution =
> -		ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> +		ad2s1210_resolution_value[data & AD2S1210_SET_RES];
>  	ad2s1210_set_resolution_pin(st);
>  	ret = len;
>  	st->hysteresis = !!(data & AD2S1210_ENABLE_HYSTERESIS);
> @@ -315,30 +336,17 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
>  		dev_err(dev, "ad2s1210: resolution out of range\n");
>  		return -EINVAL;
>  	}
> +
> +	data = (udata - 10) >> 1;
> +
>  	mutex_lock(&st->lock);
> -	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
> -	if (ret < 0)
> -		goto error_ret;
> -	data = ret;
> -	data &= ~AD2S1210_SET_RESOLUTION;
> -	data |= (udata - 10) >> 1;
> -	ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
> -	if (ret < 0)
> -		goto error_ret;
> -	ret = ad2s1210_config_write(st, data & AD2S1210_MSB_IS_LOW);
> +	ret = regmap_update_bits(st->regmap, AD2S1210_REG_CONTROL,
> +				 AD2S1210_SET_RES, data);
>  	if (ret < 0)
>  		goto error_ret;
> -	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
> -	if (ret < 0)
> -		goto error_ret;
> -	data = ret;
> -	if (data & AD2S1210_MSB_IS_HIGH) {
> -		ret = -EIO;
> -		dev_err(dev, "ad2s1210: setting resolution fail\n");
> -		goto error_ret;
> -	}
> +
>  	st->resolution =
> -		ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
> +		ad2s1210_resolution_value[data & AD2S1210_SET_RES];
>  	ad2s1210_set_resolution_pin(st);
>  	ret = len;
>  error_ret:
> @@ -351,13 +359,14 @@ static ssize_t ad2s1210_show_fault(struct device *dev,
>  				   struct device_attribute *attr, char *buf)
>  {
>  	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> +	unsigned int value;
>  	int ret;
>  
>  	mutex_lock(&st->lock);
> -	ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
> +	ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &value);
>  	mutex_unlock(&st->lock);
>  
> -	return (ret < 0) ? ret : sprintf(buf, "0x%02x\n", ret);
> +	return ret < 0 ? ret : sprintf(buf, "0x%02x\n", value);

You added the brackets in earlier patch. I've dropped them now
from my tree which will make this not quite apply.



>  }
>  

...

>  
>  static ssize_t ad2s1210_store_reg(struct device *dev,
> @@ -409,14 +420,11 @@ static ssize_t ad2s1210_store_reg(struct device *dev,
>  	struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
>  
>  	ret = kstrtou8(buf, 10, &data);
> -	if (ret)
> +	if (ret < 0)
>  		return -EINVAL;
> +
Unrelated. Also unnecessary as it doesn't return postive values.

>  	mutex_lock(&st->lock);
> -	ret = ad2s1210_config_write(st, iattr->address);
> -	if (ret < 0)
> -		goto error_ret;
> -	ret = ad2s1210_config_write(st, data & AD2S1210_MSB_IS_LOW);
> -error_ret:
> +	ret = regmap_write(st->regmap, iattr->address, data);
>  	mutex_unlock(&st->lock);
>  	return ret < 0 ? ret : len;
>  }
> @@ -583,23 +591,12 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
>  	mutex_lock(&st->lock);
>  	ad2s1210_set_resolution_pin(st);
>  
> -	ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
> -	if (ret < 0)
> -		goto error_ret;
> -	data = AD2S1210_DEF_CONTROL & ~(AD2S1210_SET_RESOLUTION);
> +	data = AD2S1210_DEF_CONTROL & ~AD2S1210_SET_RES;

Somewhat unrelated and you may sort it later, but I'd like
to see DEF_CONTROL broken out and FIELD_PREP() used for all the fields.
Seems crazy to use a value, then drop some bits then fill them in with
something else.

>  	data |= (st->resolution - 10) >> 1;
> -	ret = ad2s1210_config_write(st, data);
> -	if (ret < 0)
> -		goto error_ret;
> -	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
> +	ret = regmap_write(st->regmap, AD2S1210_REG_CONTROL, data);
>  	if (ret < 0)
>  		goto error_ret;
>  
> -	if (ret & AD2S1210_MSB_IS_HIGH) {
I guess this was meant to be a sanity check on the chip responding. 

> -		ret = -EIO;
> -		goto error_ret;
> -	}
> -
>  	ret = ad2s1210_update_frequency_control_word(st);
>  	if (ret < 0)
>  		goto error_ret;
> @@ -652,6 +649,52 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
>  	return 0;
>  }
David Lechner Sept. 25, 2023, 4:37 p.m. UTC | #2 
> > -     if (ret & AD2S1210_MSB_IS_HIGH) {
> I guess this was meant to be a sanity check on the chip responding.
>
> > -             ret = -EIO;
> > -             goto error_ret;
> > -     }
> > -

Yes, according to the data sheet, if this bit is set, it means there was a
configuration parity error. This test has been moved to the
ad2s1210_regmap_reg_read() function.
diff mbox series

Patch

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 153ac7704ad7..3c81ee61b897 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -5,6 +5,7 @@ 
  * Copyright (c) 2010-2010 Analog Devices Inc.
  * Copyright (C) 2023 BayLibre, SAS
  */
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -12,6 +13,7 @@ 
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
 #include <linux/sysfs.h>
@@ -22,21 +24,20 @@ 
 
 #define DRV_NAME "ad2s1210"
 
+/* default value of control register on powerup */
 #define AD2S1210_DEF_CONTROL		0x7E
 
-#define AD2S1210_MSB_IS_HIGH		0x80
-#define AD2S1210_MSB_IS_LOW		0x7F
-#define AD2S1210_PHASE_LOCK_RANGE_44	0x20
-#define AD2S1210_ENABLE_HYSTERESIS	0x10
-#define AD2S1210_SET_ENRES1		0x08
-#define AD2S1210_SET_ENRES0		0x04
-#define AD2S1210_SET_RES1		0x02
-#define AD2S1210_SET_RES0		0x01
-
-#define AD2S1210_SET_RESOLUTION		(AD2S1210_SET_RES1 | AD2S1210_SET_RES0)
-
-#define AD2S1210_REG_POSITION		0x80
-#define AD2S1210_REG_VELOCITY		0x82
+/* control register flags */
+#define AD2S1210_ADDRESS_DATA		BIT(7)
+#define AD2S1210_PHASE_LOCK_RANGE_44	BIT(5)
+#define AD2S1210_ENABLE_HYSTERESIS	BIT(4)
+#define AD2S1210_SET_ENRES		GENMASK(3, 2)
+#define AD2S1210_SET_RES		GENMASK(1, 0)
+
+#define AD2S1210_REG_POSITION_MSB	0x80
+#define AD2S1210_REG_POSITION_LSB	0x81
+#define AD2S1210_REG_VELOCITY_MSB	0x82
+#define AD2S1210_REG_VELOCITY_LSB	0x83
 #define AD2S1210_REG_LOS_THRD		0x88
 #define AD2S1210_REG_DOS_OVR_THRD	0x89
 #define AD2S1210_REG_DOS_MIS_THRD	0x8A
@@ -92,6 +93,8 @@  struct ad2s1210_state {
 	struct mutex lock;
 	struct spi_device *sdev;
 	struct gpio_desc *gpios[5];
+	/** Used to access config registers. */
+	struct regmap *regmap;
 	/** The external oscillator frequency in Hz. */
 	unsigned long fclkin;
 	unsigned int fexcit;
@@ -116,24 +119,51 @@  static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
 	st->mode = mode;
 }
 
-/* write 1 bytes (address or data) to the chip */
-static int ad2s1210_config_write(struct ad2s1210_state *st, u8 data)
+/*
+ * Writes the given data to the given register address.
+ *
+ * If the mode is configurable, the device will first be placed in
+ * configuration mode.
+ */
+static int ad2s1210_regmap_reg_write(void *context, unsigned int reg,
+				     unsigned int val)
 {
-	int ret;
+	struct ad2s1210_state *st = context;
+	struct spi_transfer xfers[] = {
+		{
+			.len = 1,
+			.rx_buf = &st->rx[0],
+			.tx_buf = &st->tx[0],
+			.cs_change = 1,
+		}, {
+			.len = 1,
+			.rx_buf = &st->rx[1],
+			.tx_buf = &st->tx[1],
+		},
+	};
+
+	/* values can only be 7 bits, the MSB indicates an address */
+	if (val & ~0x7F)
+		return -EINVAL;
+
+	st->tx[0] = reg;
+	st->tx[1] = val;
 
 	ad2s1210_set_mode(MOD_CONFIG, st);
-	st->tx[0] = data;
-	ret = spi_write(st->sdev, st->tx, 1);
-	if (ret < 0)
-		return ret;
 
-	return 0;
+	return spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
 }
 
-/* read value from one of the registers */
-static int ad2s1210_config_read(struct ad2s1210_state *st,
-				unsigned char address)
+/*
+ * Reads value from one of the registers.
+ *
+ * If the mode is configurable, the device will first be placed in
+ * configuration mode.
+ */
+static int ad2s1210_regmap_reg_read(void *context, unsigned int reg,
+				    unsigned int *val)
 {
+	struct ad2s1210_state *st = context;
 	struct spi_transfer xfers[] = {
 		{
 			.len = 1,
@@ -146,22 +176,34 @@  static int ad2s1210_config_read(struct ad2s1210_state *st,
 			.tx_buf = &st->tx[1],
 		},
 	};
-	int ret = 0;
+	int ret;
 
 	ad2s1210_set_mode(MOD_CONFIG, st);
-	st->tx[0] = address | AD2S1210_MSB_IS_HIGH;
+	st->tx[0] = reg;
+	/* Must be valid register address here otherwise this could write data.
+	 * It doesn't matter which one.
+	 */
 	st->tx[1] = AD2S1210_REG_FAULT;
-	ret = spi_sync_transfer(st->sdev, xfers, 2);
+
+	ret = spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
 	if (ret < 0)
 		return ret;
 
-	return st->rx[1];
+	/* If the D7 bit is set on any read/write register, it indicates a
+	 * parity error. The fault register is read-only and the D7 bit means
+	 * something else there.
+	 */
+	if (reg != AD2S1210_REG_FAULT && st->rx[1] & AD2S1210_ADDRESS_DATA)
+		return -EBADMSG;
+
+	*val = st->rx[1];
+
+	return 0;
 }
 
 static inline
 int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)
 {
-	int ret;
 	unsigned char fcw;
 
 	fcw = (unsigned char)(st->fexcit * (1 << 15) / st->fclkin);
@@ -170,11 +212,7 @@  int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)
 		return -ERANGE;
 	}
 
-	ret = ad2s1210_config_write(st, AD2S1210_REG_EXCIT_FREQ);
-	if (ret < 0)
-		return ret;
-
-	return ad2s1210_config_write(st, fcw);
+	return regmap_write(st->regmap, AD2S1210_REG_EXCIT_FREQ, fcw);
 }
 
 static const int ad2s1210_res_pins[4][2] = {
@@ -191,13 +229,7 @@  static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st)
 
 static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
 {
-	int ret;
-
-	ret = ad2s1210_config_write(st, AD2S1210_REG_SOFT_RESET);
-	if (ret < 0)
-		return ret;
-
-	return ad2s1210_config_write(st, 0x0);
+	return regmap_write(st->regmap, AD2S1210_REG_SOFT_RESET, 0);
 }
 
 static ssize_t ad2s1210_show_fexcit(struct device *dev,
@@ -242,12 +274,13 @@  static ssize_t ad2s1210_show_control(struct device *dev,
 				     char *buf)
 {
 	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
+	unsigned int value;
 	int ret;
 
 	mutex_lock(&st->lock);
-	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
+	ret = regmap_read(st->regmap, AD2S1210_REG_CONTROL, &value);
 	mutex_unlock(&st->lock);
-	return ret < 0 ? ret : sprintf(buf, "0x%x\n", ret);
+	return ret < 0 ? ret : sprintf(buf, "0x%x\n", value);
 }
 
 static ssize_t ad2s1210_store_control(struct device *dev,
@@ -264,25 +297,13 @@  static ssize_t ad2s1210_store_control(struct device *dev,
 		return -EINVAL;
 
 	mutex_lock(&st->lock);
-	ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
-	if (ret < 0)
-		goto error_ret;
-	data = udata & AD2S1210_MSB_IS_LOW;
-	ret = ad2s1210_config_write(st, data);
+	data = udata & ~AD2S1210_ADDRESS_DATA;
+	ret = regmap_write(st->regmap, AD2S1210_REG_CONTROL, data);
 	if (ret < 0)
 		goto error_ret;
 
-	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
-	if (ret < 0)
-		goto error_ret;
-	if (ret & AD2S1210_MSB_IS_HIGH) {
-		ret = -EIO;
-		dev_err(dev,
-			"ad2s1210: write control register fail\n");
-		goto error_ret;
-	}
 	st->resolution =
-		ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
+		ad2s1210_resolution_value[data & AD2S1210_SET_RES];
 	ad2s1210_set_resolution_pin(st);
 	ret = len;
 	st->hysteresis = !!(data & AD2S1210_ENABLE_HYSTERESIS);
@@ -315,30 +336,17 @@  static ssize_t ad2s1210_store_resolution(struct device *dev,
 		dev_err(dev, "ad2s1210: resolution out of range\n");
 		return -EINVAL;
 	}
+
+	data = (udata - 10) >> 1;
+
 	mutex_lock(&st->lock);
-	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
-	if (ret < 0)
-		goto error_ret;
-	data = ret;
-	data &= ~AD2S1210_SET_RESOLUTION;
-	data |= (udata - 10) >> 1;
-	ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
-	if (ret < 0)
-		goto error_ret;
-	ret = ad2s1210_config_write(st, data & AD2S1210_MSB_IS_LOW);
+	ret = regmap_update_bits(st->regmap, AD2S1210_REG_CONTROL,
+				 AD2S1210_SET_RES, data);
 	if (ret < 0)
 		goto error_ret;
-	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
-	if (ret < 0)
-		goto error_ret;
-	data = ret;
-	if (data & AD2S1210_MSB_IS_HIGH) {
-		ret = -EIO;
-		dev_err(dev, "ad2s1210: setting resolution fail\n");
-		goto error_ret;
-	}
+
 	st->resolution =
-		ad2s1210_resolution_value[data & AD2S1210_SET_RESOLUTION];
+		ad2s1210_resolution_value[data & AD2S1210_SET_RES];
 	ad2s1210_set_resolution_pin(st);
 	ret = len;
 error_ret:
@@ -351,13 +359,14 @@  static ssize_t ad2s1210_show_fault(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
 	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
+	unsigned int value;
 	int ret;
 
 	mutex_lock(&st->lock);
-	ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
+	ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &value);
 	mutex_unlock(&st->lock);
 
-	return (ret < 0) ? ret : sprintf(buf, "0x%02x\n", ret);
+	return ret < 0 ? ret : sprintf(buf, "0x%02x\n", value);
 }
 
 static ssize_t ad2s1210_clear_fault(struct device *dev,
@@ -366,6 +375,7 @@  static ssize_t ad2s1210_clear_fault(struct device *dev,
 				    size_t len)
 {
 	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
+	unsigned int value;
 	int ret;
 
 	mutex_lock(&st->lock);
@@ -373,7 +383,7 @@  static ssize_t ad2s1210_clear_fault(struct device *dev,
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
 	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1);
-	ret = ad2s1210_config_read(st, AD2S1210_REG_FAULT);
+	ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &value);
 	if (ret < 0)
 		goto error_ret;
 	gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 0);
@@ -390,13 +400,14 @@  static ssize_t ad2s1210_show_reg(struct device *dev,
 {
 	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
 	struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
+	unsigned int value;
 	int ret;
 
 	mutex_lock(&st->lock);
-	ret = ad2s1210_config_read(st, iattr->address);
+	ret = regmap_read(st->regmap, iattr->address, &value);
 	mutex_unlock(&st->lock);
 
-	return ret < 0 ? ret : sprintf(buf, "%d\n", ret);
+	return ret < 0 ? ret : sprintf(buf, "%d\n", value);
 }
 
 static ssize_t ad2s1210_store_reg(struct device *dev,
@@ -409,14 +420,11 @@  static ssize_t ad2s1210_store_reg(struct device *dev,
 	struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
 
 	ret = kstrtou8(buf, 10, &data);
-	if (ret)
+	if (ret < 0)
 		return -EINVAL;
+
 	mutex_lock(&st->lock);
-	ret = ad2s1210_config_write(st, iattr->address);
-	if (ret < 0)
-		goto error_ret;
-	ret = ad2s1210_config_write(st, data & AD2S1210_MSB_IS_LOW);
-error_ret:
+	ret = regmap_write(st->regmap, iattr->address, data);
 	mutex_unlock(&st->lock);
 	return ret < 0 ? ret : len;
 }
@@ -583,23 +591,12 @@  static int ad2s1210_initial(struct ad2s1210_state *st)
 	mutex_lock(&st->lock);
 	ad2s1210_set_resolution_pin(st);
 
-	ret = ad2s1210_config_write(st, AD2S1210_REG_CONTROL);
-	if (ret < 0)
-		goto error_ret;
-	data = AD2S1210_DEF_CONTROL & ~(AD2S1210_SET_RESOLUTION);
+	data = AD2S1210_DEF_CONTROL & ~AD2S1210_SET_RES;
 	data |= (st->resolution - 10) >> 1;
-	ret = ad2s1210_config_write(st, data);
-	if (ret < 0)
-		goto error_ret;
-	ret = ad2s1210_config_read(st, AD2S1210_REG_CONTROL);
+	ret = regmap_write(st->regmap, AD2S1210_REG_CONTROL, data);
 	if (ret < 0)
 		goto error_ret;
 
-	if (ret & AD2S1210_MSB_IS_HIGH) {
-		ret = -EIO;
-		goto error_ret;
-	}
-
 	ret = ad2s1210_update_frequency_control_word(st);
 	if (ret < 0)
 		goto error_ret;
@@ -652,6 +649,52 @@  static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
 	return 0;
 }
 
+static const struct regmap_range ad2s1210_regmap_readable_ranges[] = {
+	regmap_reg_range(AD2S1210_REG_POSITION_MSB, AD2S1210_REG_VELOCITY_LSB),
+	regmap_reg_range(AD2S1210_REG_LOS_THRD, AD2S1210_REG_LOT_LOW_THRD),
+	regmap_reg_range(AD2S1210_REG_EXCIT_FREQ, AD2S1210_REG_CONTROL),
+	regmap_reg_range(AD2S1210_REG_FAULT, AD2S1210_REG_FAULT),
+};
+
+static const struct regmap_access_table ad2s1210_regmap_rd_table = {
+	.yes_ranges = ad2s1210_regmap_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad2s1210_regmap_readable_ranges),
+};
+
+static const struct regmap_range ad2s1210_regmap_writeable_ranges[] = {
+	regmap_reg_range(AD2S1210_REG_LOS_THRD, AD2S1210_REG_LOT_LOW_THRD),
+	regmap_reg_range(AD2S1210_REG_EXCIT_FREQ, AD2S1210_REG_CONTROL),
+	regmap_reg_range(AD2S1210_REG_SOFT_RESET, AD2S1210_REG_SOFT_RESET),
+	regmap_reg_range(AD2S1210_REG_FAULT, AD2S1210_REG_FAULT),
+};
+
+static const struct regmap_access_table ad2s1210_regmap_wr_table = {
+	.yes_ranges = ad2s1210_regmap_writeable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad2s1210_regmap_writeable_ranges),
+};
+
+static int ad2s1210_setup_regmap(struct ad2s1210_state *st)
+{
+	struct device *dev = &st->sdev->dev;
+	const struct regmap_config config = {
+		.reg_bits = 8,
+		.val_bits = 8,
+		.disable_locking = true,
+		.reg_read = ad2s1210_regmap_reg_read,
+		.reg_write = ad2s1210_regmap_reg_write,
+		.rd_table = &ad2s1210_regmap_rd_table,
+		.wr_table = &ad2s1210_regmap_wr_table,
+		.can_sleep = true,
+	};
+
+	st->regmap = devm_regmap_init(dev, NULL, st, &config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap),
+				     "failed to allocate register map\n");
+
+	return 0;
+}
+
 static int ad2s1210_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;
@@ -679,6 +722,10 @@  static int ad2s1210_probe(struct spi_device *spi)
 	if (ret < 0)
 		return ret;
 
+	ret = ad2s1210_setup_regmap(st);
+	if (ret < 0)
+		return ret;
+
 	ret = ad2s1210_initial(st);
 	if (ret < 0)
 		return ret;