[RFC PATCH 0/2 V2] Using regmap with ADIS devices.

[RFC PATCH 0/2 V2] Using regmap with ADIS devices.

[Jonathan Cameron]

Hi Mark,

I've tried your suggestion of doing things at the marshalling level
and would be interested to hear what you think of the result.

Note these patches are not clean yet hence no sign offs.

The tricky bit was triggering the two separate writes per register
and the result is somewhat hacky.

An alternative there would be to add some configuration options to
the spi_write (or allow a couple of varients) so as to break the
long transfer that would generate up and hence bounce the cs line
in the right place.  Gah. SPI can be so uggly sometimes.

If this approach isn't acceptable in principle I'll give up and
conclude that these parts will never play well! (which is fine
by me if true).

Jonathan

Jonathan Cameron (2):
  regmap: Support half writes and padding between register and value.
  staging:iio:imu:adis16400 regmap introduction.

 drivers/base/regmap/internal.h           |    1 +
 drivers/base/regmap/regmap.c             |   42 ++++-
 drivers/staging/iio/imu/Kconfig          |    1 +
 drivers/staging/iio/imu/adis16400.h      |    2 +
 drivers/staging/iio/imu/adis16400_core.c |  301 +++++++++++++++++++-----------
 include/linux/regmap.h                   |    6 +
 6 files changed, 234 insertions(+), 119 deletions(-)

-- 
1.7.3.4

[PATCH 1/2] regmap: Support half writes and padding between register and value.

[Jonathan Cameron]

Note half writes currently assume address numbers are even only.
That's a pain for caching so other suggestions welcome.  I could set
it as a 7 bit address and increase the padding to 9 bits.  That makes
the write bit a little strange though as it will be going into the
padding.

Not signed off by Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/base/regmap/internal.h |    1 +
 drivers/base/regmap/regmap.c   |   42 ++++++++++++++++++++++++++++++++++-----
 include/linux/regmap.h         |    6 +++++
 3 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 7e14d5a..a6fb2f4 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -22,6 +22,7 @@ struct regmap_format {
 	size_t buf_size;
 	size_t reg_bytes;
 	size_t val_bytes;
+	bool half_write;
 	void (*format_write)(struct regmap *map,
 			     unsigned int reg, unsigned int val);
 	void (*format_reg)(void *buf, unsigned int reg);
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index e7adfe7..41f1e7f 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -71,6 +71,16 @@ static void regmap_format_4_12_write(struct regmap *map,
 	*out = cpu_to_be16((reg << 12) | val);
 }
 
+static void regmap_format_8_16_half_write(struct regmap *map,
+					 unsigned int reg, unsigned int val)
+{
+	u8 *out8 = map->work_buf;
+	out8[0] = reg | map->write_flag_mask;; 
+	out8[1] = val;
+	out8[2] = (reg + 1) | map->write_flag_mask;
+	out8[3] = val >> 8;
+}
+
 static void regmap_format_7_9_write(struct regmap *map,
 				    unsigned int reg, unsigned int val)
 {
@@ -136,9 +146,12 @@ struct regmap *regmap_init(struct device *dev,
 	}
 
 	mutex_init(&map->lock);
-	map->format.buf_size = (config->reg_bits + config->val_bits) / 8;
-	map->format.reg_bytes = config->reg_bits / 8;
+	map->format.buf_size = (config->reg_bits +
+				config->reg_pad_bits +
+				config->val_bits) / 8;
+	map->format.reg_bytes = (config->reg_bits + config->reg_pad_bits)/ 8;
 	map->format.val_bytes = config->val_bits / 8;
+	map->format.half_write = config->half_write;
 	map->dev = dev;
 	map->bus = bus;
 	map->max_register = config->max_register;
@@ -176,6 +189,9 @@ struct regmap *regmap_init(struct device *dev,
 		break;
 
 	case 8:
+		if (map->format.half_write)
+			map->format.format_write =
+				regmap_format_8_16_half_write;
 		map->format.format_reg = regmap_format_8;
 		break;
 
@@ -256,13 +272,27 @@ static int _regmap_raw_write(struct regmap *map, unsigned int reg,
 	 * send the work_buf directly, otherwise try to do a gather
 	 * write.
 	 */
-	if (val == map->work_buf + map->format.reg_bytes)
-		ret = map->bus->write(map->dev, map->work_buf,
-				      map->format.reg_bytes + val_len);
-	else if (map->bus->gather_write)
+	if (val == map->work_buf + map->format.reg_bytes) {
+		if (map->format.half_write) {
+			ret = map->bus->write(map->dev, map->work_buf,
+					      (map->format.reg_bytes +
+					       val_len) >> 1);
+			if (ret >= 0)
+				ret = map->bus->write(map->dev,
+						      map->work_buf +
+						      ((map->format.reg_bytes +
+							val_len) >> 1),
+						      (map->format.reg_bytes +
+						       val_len) >> 1);
+		} else {
+			ret = map->bus->write(map->dev, map->work_buf,
+					      map->format.reg_bytes + val_len);
+		}
+	} else if (map->bus->gather_write) {
 		ret = map->bus->gather_write(map->dev, map->work_buf,
 					     map->format.reg_bytes,
 					     val, val_len);
+	}
 
 	/* If that didn't work fall back on linearising by hand. */
 	if (ret == -ENOTSUPP) {
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 18d4afa..564d703 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -37,6 +37,8 @@ struct reg_default {
  * Configuration for the register map of a device.
  *
  * @reg_bits: Number of bits in a register address, mandatory.
+ * @reg_pad_bits: Number of bits of padding between register
+ *                address and start of value.
  * @val_bits: Number of bits in a register value, mandatory.
  *
  * @writeable_reg: Optional callback returning true if the register
@@ -59,9 +61,12 @@ struct reg_default {
  * @write_flag_mask: Mask to be set in the top byte of the register when doing
  *                   a write. If both read_flag_mask and write_flag_mask are
  *                   empty the regmap_bus default masks are used.
+ * @half_write: Flag to indicate that writes are done in two parts, half of
+ *              the register in each.
  */
 struct regmap_config {
 	int reg_bits;
+	int reg_pad_bits;
 	int val_bits;
 
 	bool (*writeable_reg)(struct device *dev, unsigned int reg);
@@ -75,6 +80,7 @@ struct regmap_config {
 
 	u8 read_flag_mask;
 	u8 write_flag_mask;
+	bool half_write;
 };
 
 typedef int (*regmap_hw_write)(struct device *dev, const void *data,
-- 
1.7.3.4

Re: [PATCH 1/2] regmap: Support half writes and padding between register and value.

[Mark Brown]

On Thu, Sep 08, 2011 at 03:09:23PM +0100, Jonathan Cameron wrote:

> Note half writes currently assume address numbers are even only.
> That's a pain for caching so other suggestions welcome.  I could set
> it as a 7 bit address and increase the padding to 9 bits.  That makes
> the write bit a little strange though as it will be going into the
> padding.

This needs more documentation somewhere explaining what a half write is
and/or a better name for half write but doesn't look too invasive so I
think this approach can work.  If we come up with something better later
then we can always change things, there shouldn't be too many users to
update if that happens.

> -	map->format.reg_bytes = config->reg_bits / 8;
> +	map->format.buf_size = (config->reg_bits +
> +				config->reg_pad_bits +
> +				config->val_bits) / 8;
> +	map->format.reg_bytes = (config->reg_bits + config->reg_pad_bits)/ 8;

Please do a patch adding padding separately - that does seem like a
useful thing to have in general, with the option of having it both
before and after the register.

> -	if (val == map->work_buf + map->format.reg_bytes)
> -		ret = map->bus->write(map->dev, map->work_buf,
> -				      map->format.reg_bytes + val_len);
> -	else if (map->bus->gather_write)
> +	if (val == map->work_buf + map->format.reg_bytes) {
> +		if (map->format.half_write) {
> +			ret = map->bus->write(map->dev, map->work_buf,
> +					      (map->format.reg_bytes +
> +					       val_len) >> 1);
> +			if (ret >= 0)
> +				ret = map->bus->write(map->dev,
> +						      map->work_buf +
> +						      ((map->format.reg_bytes +
> +							val_len) >> 1),
> +						      (map->format.reg_bytes +
> +						       val_len) >> 1);
> +		} else {
> +			ret = map->bus->write(map->dev, map->work_buf,
> +					      map->format.reg_bytes + val_len);
> +		}

This code is getting very complicated...  I think it'd be clearer to
have a special case at the head of the function that does the half write
stuff.  It also feels like the half bit needs parameterisation, but I
can't immediately think of how to do that sensibly.

Re: [PATCH 1/2] regmap: Support half writes and padding between register and value.

[Jonathan Cameron]

On 09/08/11 17:27, Mark Brown wrote:
> On Thu, Sep 08, 2011 at 03:09:23PM +0100, Jonathan Cameron wrote:
> 
>> Note half writes currently assume address numbers are even only.
>> That's a pain for caching so other suggestions welcome.  I could set
>> it as a 7 bit address and increase the padding to 9 bits.  That makes
>> the write bit a little strange though as it will be going into the
>> padding.
> 
> This needs more documentation somewhere explaining what a half write is
> and/or a better name for half write but doesn't look too invasive so I
> think this approach can work.  If we come up with something better later
> then we can always change things, there shouldn't be too many users to
> update if that happens.
Agreed. I'll document it thoroughly and explain the restricted use case.
> 
>> -	map->format.reg_bytes = config->reg_bits / 8;
>> +	map->format.buf_size = (config->reg_bits +
>> +				config->reg_pad_bits +
>> +				config->val_bits) / 8;
>> +	map->format.reg_bytes = (config->reg_bits + config->reg_pad_bits)/ 8;
> 
> Please do a patch adding padding separately - that does seem like a
> useful thing to have in general, with the option of having it both
> before and after the register.
Happy to do it separately, but not sure the padding before the register
address is a real use case.  You pad after to give the device time to
respond, before doesn't make much sense to me on a register based device...
I'll change the naming to make it explicit that it is after the register though
so as to leave room for this to be added in future.

> 
>> -	if (val == map->work_buf + map->format.reg_bytes)
>> -		ret = map->bus->write(map->dev, map->work_buf,
>> -				      map->format.reg_bytes + val_len);
>> -	else if (map->bus->gather_write)
>> +	if (val == map->work_buf + map->format.reg_bytes) {
>> +		if (map->format.half_write) {
>> +			ret = map->bus->write(map->dev, map->work_buf,
>> +					      (map->format.reg_bytes +
>> +					       val_len) >> 1);
>> +			if (ret >= 0)
>> +				ret = map->bus->write(map->dev,
>> +						      map->work_buf +
>> +						      ((map->format.reg_bytes +
>> +							val_len) >> 1),
>> +						      (map->format.reg_bytes +
>> +						       val_len) >> 1);
>> +		} else {
>> +			ret = map->bus->write(map->dev, map->work_buf,
>> +					      map->format.reg_bytes + val_len);
>> +		}
> 
> This code is getting very complicated...  I think it'd be clearer to
> have a special case at the head of the function that does the half write
> stuff.  It also feels like the half bit needs parameterisation, but I
> can't immediately think of how to do that sensibly.
Likewise. It's a very specific case. In theory there are two parameters that
I can think of.

write_length (bits per write)
address_mangler (one for each write needed?)

I'll clean it up and repost.
> 

Re: [PATCH 1/2] regmap: Support half writes and padding between register and value.

[Mark Brown]

On Fri, Sep 09, 2011 at 10:44:57AM +0100, Jonathan Cameron wrote:
> On 09/08/11 17:27, Mark Brown wrote:

> > Please do a patch adding padding separately - that does seem like a
> > useful thing to have in general, with the option of having it both
> > before and after the register.

> Happy to do it separately, but not sure the padding before the register
> address is a real use case.  You pad after to give the device time to
> respond, before doesn't make much sense to me on a register based device...
> I'll change the naming to make it explicit that it is after the register though
> so as to leave room for this to be added in future.

Some devices apparently use an entire byte or word with a magic flag in
it to distinguish reads and writes.  These end up looking like 16 bit
registers with only the lower 8 bits actually used in the current code.

Re: [PATCH 1/2] regmap: Support half writes and padding between register and value.

[Jonathan Cameron]

On 09/09/11 17:14, Mark Brown wrote:
> On Fri, Sep 09, 2011 at 10:44:57AM +0100, Jonathan Cameron wrote:
>> On 09/08/11 17:27, Mark Brown wrote:
> 
>>> Please do a patch adding padding separately - that does seem like a
>>> useful thing to have in general, with the option of having it both
>>> before and after the register.
> 
>> Happy to do it separately, but not sure the padding before the register
>> address is a real use case.  You pad after to give the device time to
>> respond, before doesn't make much sense to me on a register based device...
>> I'll change the naming to make it explicit that it is after the register though
>> so as to leave room for this to be added in future.
> 
> Some devices apparently use an entire byte or word with a magic flag in
> it to distinguish reads and writes.  These end up looking like 16 bit
> registers with only the lower 8 bits actually used in the current code.
Ah, fair enough there is a use case.  Can we add this as an additional patch
when needed?

Re: [PATCH 1/2] regmap: Support half writes and padding between register and value.

[Mark Brown]

On Fri, Sep 09, 2011 at 05:30:01PM +0100, Jonathan Cameron wrote:
> On 09/09/11 17:14, Mark Brown wrote:

> > Some devices apparently use an entire byte or word with a magic flag in
> > it to distinguish reads and writes.  These end up looking like 16 bit
> > registers with only the lower 8 bits actually used in the current code.

> Ah, fair enough there is a use case.  Can we add this as an additional patch
> when needed?

Yes, of course.  Only consideration is that the current option needs to
be clearly named to avoid colliding with such an addition.

[PATCH 2/2] staging:iio:imu:adis16400 regmap introduction.

[Jonathan Cameron]

Apply regmap for the basic register reads and writes.
Note not currently used at all for the mass reads
that occur in the buffer code.

Not signed off by: Jonathan Cameorn <jic23@cam.ac.uk>
---
 drivers/staging/iio/imu/Kconfig          |    1 +
 drivers/staging/iio/imu/adis16400.h      |    2 +
 drivers/staging/iio/imu/adis16400_core.c |  301 +++++++++++++++++++-----------
 3 files changed, 191 insertions(+), 113 deletions(-)

diff --git a/drivers/staging/iio/imu/Kconfig b/drivers/staging/iio/imu/Kconfig
index e0e0144..9b4b912 100644
--- a/drivers/staging/iio/imu/Kconfig
+++ b/drivers/staging/iio/imu/Kconfig
@@ -8,6 +8,7 @@ config ADIS16400
 	depends on SPI
 	select IIO_SW_RING if IIO_RING_BUFFER
 	select IIO_TRIGGER if IIO_RING_BUFFER
+	select REGMAP_SPI
 	help
 	  Say yes here to build support for Analog Devices adis16300, adis16350,
 	  adis16354, adis16355, adis16360, adis16362, adis16364, adis16365,
diff --git a/drivers/staging/iio/imu/adis16400.h b/drivers/staging/iio/imu/adis16400.h
index 1f8f0c6..b36dcbb 100644
--- a/drivers/staging/iio/imu/adis16400.h
+++ b/drivers/staging/iio/imu/adis16400.h
@@ -143,6 +143,7 @@ struct adis16400_chip_info {
 
 /**
  * struct adis16400_state - device instance specific data
+ * @regmap:		register map
  * @us:			actual spi_device
  * @trig:		data ready trigger registered with iio
  * @tx:			transmit buffer
@@ -150,6 +151,7 @@ struct adis16400_chip_info {
  * @buf_lock:		mutex to protect tx and rx
  **/
 struct adis16400_state {
+	struct regmap			*regmap;
 	struct spi_device		*us;
 	struct iio_trigger		*trig;
 	struct mutex			buf_lock;
diff --git a/drivers/staging/iio/imu/adis16400_core.c b/drivers/staging/iio/imu/adis16400_core.c
index b6d824f..66364f5 100644
--- a/drivers/staging/iio/imu/adis16400_core.c
+++ b/drivers/staging/iio/imu/adis16400_core.c
@@ -25,6 +25,8 @@
 #include <linux/sysfs.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/err.h>
+#include <linux/regmap.h>
 
 #include "../iio.h"
 #include "../sysfs.h"
@@ -43,71 +45,19 @@ enum adis16400_chip_variant {
 };
 
 /**
- * adis16400_spi_write_reg_8() - write single byte to a register
- * @dev: device associated with child of actual device (iio_dev or iio_trig)
- * @reg_address: the address of the register to be written
- * @val: the value to write
- */
-static int adis16400_spi_write_reg_8(struct iio_dev *indio_dev,
-				     u8 reg_address,
-				     u8 val)
-{
-	int ret;
-	struct adis16400_state *st = iio_priv(indio_dev);
-
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = ADIS16400_WRITE_REG(reg_address);
-	st->tx[1] = val;
-
-	ret = spi_write(st->us, st->tx, 2);
-	mutex_unlock(&st->buf_lock);
-
-	return ret;
-}
-
-/**
  * adis16400_spi_write_reg_16() - write 2 bytes to a pair of registers
  * @dev: device associated with child of actual device (iio_dev or iio_trig)
  * @reg_address: the address of the lower of the two registers. Second register
  *               is assumed to have address one greater.
  * @val: value to be written
- *
- * At the moment the spi framework doesn't allow global setting of cs_change.
- * This means that use cannot be made of spi_write.
  */
 static int adis16400_spi_write_reg_16(struct iio_dev *indio_dev,
 		u8 lower_reg_address,
 		u16 value)
 {
-	int ret;
-	struct spi_message msg;
 	struct adis16400_state *st = iio_priv(indio_dev);
-	struct spi_transfer xfers[] = {
-		{
-			.tx_buf = st->tx,
-			.bits_per_word = 8,
-			.len = 2,
-			.cs_change = 1,
-		}, {
-			.tx_buf = st->tx + 2,
-			.bits_per_word = 8,
-			.len = 2,
-		},
-	};
-
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = ADIS16400_WRITE_REG(lower_reg_address);
-	st->tx[1] = value & 0xFF;
-	st->tx[2] = ADIS16400_WRITE_REG(lower_reg_address + 1);
-	st->tx[3] = (value >> 8) & 0xFF;
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&xfers[0], &msg);
-	spi_message_add_tail(&xfers[1], &msg);
-	ret = spi_sync(st->us, &msg);
-	mutex_unlock(&st->buf_lock);
 
-	return ret;
+	return regmap_write(st->regmap, lower_reg_address, value);
 }
 
 /**
@@ -116,49 +66,21 @@ static int adis16400_spi_write_reg_16(struct iio_dev *indio_dev,
  * @reg_address: the address of the lower of the two registers. Second register
  *               is assumed to have address one greater.
  * @val: somewhere to pass back the value read
- *
- * At the moment the spi framework doesn't allow global setting of cs_change.
- * This means that use cannot be made of spi_read.
- **/
+ */
 static int adis16400_spi_read_reg_16(struct iio_dev *indio_dev,
 		u8 lower_reg_address,
 		u16 *val)
 {
-	struct spi_message msg;
 	struct adis16400_state *st = iio_priv(indio_dev);
 	int ret;
-	struct spi_transfer xfers[] = {
-		{
-			.tx_buf = st->tx,
-			.bits_per_word = 8,
-			.len = 2,
-			.cs_change = 1,
-		}, {
-			.rx_buf = st->rx,
-			.bits_per_word = 8,
-			.len = 2,
-		},
-	};
-
-	mutex_lock(&st->buf_lock);
-	st->tx[0] = ADIS16400_READ_REG(lower_reg_address);
-	st->tx[1] = 0;
-
-	spi_message_init(&msg);
-	spi_message_add_tail(&xfers[0], &msg);
-	spi_message_add_tail(&xfers[1], &msg);
-	ret = spi_sync(st->us, &msg);
-	if (ret) {
-		dev_err(&st->us->dev,
-			"problem when reading 16 bit register 0x%02X",
-			lower_reg_address);
-		goto error_ret;
-	}
-	*val = (st->rx[0] << 8) | st->rx[1];
+	unsigned int value;
 
-error_ret:
-	mutex_unlock(&st->buf_lock);
-	return ret;
+	ret = regmap_read(st->regmap, lower_reg_address, &value);
+	if (ret < 0)
+		return ret;
+	*val = value;
+	
+	return 0;
 }
 
 static ssize_t adis16400_read_frequency(struct device *dev,
@@ -172,7 +94,7 @@ static ssize_t adis16400_read_frequency(struct device *dev,
 	ret = adis16400_spi_read_reg_16(indio_dev,
 			ADIS16400_SMPL_PRD,
 			&t);
-	if (ret)
+	if (ret < 0)
 		return ret;
 	sps =  (t & ADIS16400_SMPL_PRD_TIME_BASE) ? 53 : 1638;
 	sps /= (t & ADIS16400_SMPL_PRD_DIV_MASK) + 1;
@@ -192,7 +114,7 @@ static ssize_t adis16400_write_frequency(struct device *dev,
 	u8 t;
 
 	ret = strict_strtol(buf, 10, &val);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	mutex_lock(&indio_dev->mlock);
@@ -206,7 +128,7 @@ static ssize_t adis16400_write_frequency(struct device *dev,
 	else
 		st->us->max_speed_hz = ADIS16400_SPI_FAST;
 
-	ret = adis16400_spi_write_reg_8(indio_dev,
+	ret = adis16400_spi_write_reg_16(indio_dev,
 			ADIS16400_SMPL_PRD,
 			t);
 
@@ -218,10 +140,10 @@ static ssize_t adis16400_write_frequency(struct device *dev,
 static int adis16400_reset(struct iio_dev *indio_dev)
 {
 	int ret;
-	ret = adis16400_spi_write_reg_8(indio_dev,
+	ret = adis16400_spi_write_reg_16(indio_dev,
 			ADIS16400_GLOB_CMD,
 			ADIS16400_GLOB_CMD_SW_RESET);
-	if (ret)
+	if (ret < 0)
 		dev_err(&indio_dev->dev, "problem resetting device");
 
 	return ret;
@@ -252,7 +174,7 @@ int adis16400_set_irq(struct iio_dev *indio_dev, bool enable)
 	u16 msc;
 
 	ret = adis16400_spi_read_reg_16(indio_dev, ADIS16400_MSC_CTRL, &msc);
-	if (ret)
+	if (ret < 0)
 		goto error_ret;
 
 	msc |= ADIS16400_MSC_CTRL_DATA_RDY_POL_HIGH;
@@ -262,7 +184,7 @@ int adis16400_set_irq(struct iio_dev *indio_dev, bool enable)
 		msc &= ~ADIS16400_MSC_CTRL_DATA_RDY_EN;
 
 	ret = adis16400_spi_write_reg_16(indio_dev, ADIS16400_MSC_CTRL, msc);
-	if (ret)
+	if (ret < 0)
 		goto error_ret;
 
 error_ret:
@@ -276,7 +198,7 @@ static int adis16400_stop_device(struct iio_dev *indio_dev)
 	u16 val = ADIS16400_SLP_CNT_POWER_OFF;
 
 	ret = adis16400_spi_write_reg_16(indio_dev, ADIS16400_SLP_CNT, val);
-	if (ret)
+	if (ret < 0)
 		dev_err(&indio_dev->dev,
 			"problem with turning device off: SLP_CNT");
 
@@ -338,7 +260,7 @@ static int adis16400_self_test(struct iio_dev *indio_dev)
 	ret = adis16400_spi_write_reg_16(indio_dev,
 			ADIS16400_MSC_CTRL,
 			ADIS16400_MSC_CTRL_MEM_TEST);
-	if (ret) {
+	if (ret < 0) {
 		dev_err(&indio_dev->dev, "problem starting self test");
 		goto err_ret;
 	}
@@ -362,24 +284,24 @@ static int adis16400_initial_setup(struct iio_dev *indio_dev)
 	spi_setup(st->us);
 
 	ret = adis16400_set_irq(indio_dev, false);
-	if (ret) {
+	if (ret < 0) {
 		dev_err(&indio_dev->dev, "disable irq failed");
 		goto err_ret;
 	}
 
 	ret = adis16400_self_test(indio_dev);
-	if (ret) {
+	if (ret < 0) {
 		dev_err(&indio_dev->dev, "self test failure");
 		goto err_ret;
 	}
 
 	ret = adis16400_check_status(indio_dev);
-	if (ret) {
+	if (ret < 0) {
 		adis16400_reset(indio_dev);
 		dev_err(&indio_dev->dev, "device not playing ball -> reset");
 		msleep(ADIS16400_STARTUP_DELAY);
 		ret = adis16400_check_status(indio_dev);
-		if (ret) {
+		if (ret < 0) {
 			dev_err(&indio_dev->dev, "giving up");
 			goto err_ret;
 		}
@@ -387,7 +309,7 @@ static int adis16400_initial_setup(struct iio_dev *indio_dev)
 	if (st->variant->flags & ADIS16400_HAS_PROD_ID) {
 		ret = adis16400_spi_read_reg_16(indio_dev,
 						ADIS16400_PRODUCT_ID, &prod_id);
-		if (ret)
+		if (ret < 0)
 			goto err_ret;
 
 		if ((prod_id & 0xF000) != st->variant->product_id)
@@ -492,7 +414,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
 		ret = adis16400_spi_read_reg_16(indio_dev,
 				adis16400_addresses[chan->address][0],
 				&val16);
-		if (ret) {
+		if (ret < 0) {
 			mutex_unlock(&indio_dev->mlock);
 			return ret;
 		}
@@ -539,7 +461,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
 				adis16400_addresses[chan->address][1],
 				&val16);
 		mutex_unlock(&indio_dev->mlock);
-		if (ret)
+		if (ret < 0)
 			return ret;
 		val16 = ((val16 & 0xFFF) << 4) >> 4;
 		*val = val16;
@@ -1016,6 +938,150 @@ static const struct iio_info adis16400_info = {
 	.attrs = &adis16400_attribute_group,
 };
 
+/* This may technically result in read attempts to undefined registers */
+static bool adis16400_reg_readable(struct device *dev,
+				   unsigned int reg)
+{
+	switch (reg) {
+	case ADIS16400_FLASH_CNT:
+	case ADIS16400_SUPPLY_OUT:
+	case ADIS16400_XGYRO_OUT:
+	case ADIS16400_YGYRO_OUT:
+	case ADIS16400_ZGYRO_OUT:
+	case ADIS16400_XACCL_OUT:
+	case ADIS16400_YACCL_OUT:
+	case ADIS16400_ZACCL_OUT:
+	case ADIS16400_XMAGN_OUT:
+	case ADIS16400_YMAGN_OUT:
+	case ADIS16400_ZMAGN_OUT:
+	case ADIS16400_TEMP_OUT:
+	case ADIS16400_AUX_ADC:
+	case ADIS16400_XGYRO_OFF:
+	case ADIS16400_YGYRO_OFF:
+	case ADIS16400_ZGYRO_OFF:
+	case ADIS16400_XACCL_OFF:
+	case ADIS16400_YACCL_OFF:
+	case ADIS16400_ZACCL_OFF:
+	case ADIS16400_XMAGN_HIF:
+	case ADIS16400_YMAGN_HIF:
+	case ADIS16400_ZMAGN_HIF:
+	case ADIS16400_XMAGN_SIF:
+	case ADIS16400_YMAGN_SIF:
+	case ADIS16400_ZMAGN_SIF:
+	case ADIS16400_GPIO_CTRL:
+	case ADIS16400_MSC_CTRL:
+	case ADIS16400_SMPL_PRD:
+	case ADIS16400_SENS_AVG:
+	case ADIS16400_DIAG_STAT:
+	case ADIS16400_ALM_MAG1:
+	case ADIS16400_ALM_MAG2:
+	case ADIS16400_ALM_SMPL1:
+	case ADIS16400_ALM_SMPL2:
+	case ADIS16400_ALM_CTRL:
+	case ADIS16400_AUX_DAC:
+	case ADIS16400_PRODUCT_ID:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool adis16400_reg_precious(struct device *dev,
+				   unsigned int reg)
+{
+	switch (reg) {
+	case ADIS16400_SUPPLY_OUT:
+	case ADIS16400_XGYRO_OUT:
+	case ADIS16400_YGYRO_OUT:
+	case ADIS16400_ZGYRO_OUT:
+	case ADIS16400_XACCL_OUT:
+	case ADIS16400_YACCL_OUT:
+	case ADIS16400_ZACCL_OUT:
+	case ADIS16400_XMAGN_OUT:
+	case ADIS16400_YMAGN_OUT:
+	case ADIS16400_ZMAGN_OUT:
+	case ADIS16400_TEMP_OUT:
+	case ADIS16400_AUX_ADC:
+	case ADIS16400_DIAG_STAT:
+		return true;
+	default:
+		return 0;
+	}
+}
+
+static bool adis16400_reg_volatile(struct device *dev,
+				   unsigned int reg)
+{
+	switch (reg) {
+	case ADIS16400_FLASH_CNT:
+	case ADIS16400_SUPPLY_OUT:
+	case ADIS16400_XGYRO_OUT:
+	case ADIS16400_YGYRO_OUT:
+	case ADIS16400_ZGYRO_OUT:
+	case ADIS16400_XACCL_OUT:
+	case ADIS16400_YACCL_OUT:
+	case ADIS16400_ZACCL_OUT:
+	case ADIS16400_XMAGN_OUT:
+	case ADIS16400_YMAGN_OUT:
+	case ADIS16400_ZMAGN_OUT:
+	case ADIS16400_TEMP_OUT:
+	case ADIS16400_AUX_ADC:
+	case ADIS16400_DIAG_STAT:
+
+		return true;
+	default:
+		return 0;
+	}
+}
+
+static bool adis16400_reg_writeable(struct device *dev,
+				    unsigned int reg)
+{
+	switch (reg) {
+	case ADIS16400_XGYRO_OFF:
+	case ADIS16400_YGYRO_OFF:
+	case ADIS16400_ZGYRO_OFF:
+	case ADIS16400_XACCL_OFF:
+	case ADIS16400_YACCL_OFF:
+	case ADIS16400_ZACCL_OFF:
+	case ADIS16400_XMAGN_HIF:
+	case ADIS16400_YMAGN_HIF:
+	case ADIS16400_ZMAGN_HIF:
+	case ADIS16400_XMAGN_SIF:
+	case ADIS16400_YMAGN_SIF:
+	case ADIS16400_ZMAGN_SIF:
+	case ADIS16400_GPIO_CTRL:
+	case ADIS16400_MSC_CTRL:
+	case ADIS16400_SMPL_PRD:
+	case ADIS16400_SENS_AVG:
+	case ADIS16400_SLP_CNT:
+	case ADIS16400_GLOB_CMD:
+	case ADIS16400_ALM_MAG1:
+	case ADIS16400_ALM_MAG2:
+	case ADIS16400_ALM_SMPL1:
+	case ADIS16400_ALM_SMPL2:
+	case ADIS16400_ALM_CTRL:
+	case ADIS16400_AUX_DAC:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config adis16400_regmap_config = {
+	.reg_bits = 8,
+	.reg_pad_bits = 8,
+	.val_bits = 16,
+	.half_write = true,
+	.writeable_reg = &adis16400_reg_writeable,
+	.readable_reg = &adis16400_reg_readable,
+	.precious_reg = &adis16400_reg_precious,
+	.volatile_reg = &adis16400_reg_volatile,
+	.max_register = 0x56,
+	.write_flag_mask = 0x80,
+	.read_flag_mask = 0,
+};
+
 static int __devinit adis16400_probe(struct spi_device *spi)
 {
 	int ret;
@@ -1026,9 +1092,14 @@ static int __devinit adis16400_probe(struct spi_device *spi)
 		goto error_ret;
 	}
 	st = iio_priv(indio_dev);
+	st->regmap = regmap_init_spi(spi, &adis16400_regmap_config);
+	if (IS_ERR(st->regmap)) {
+		ret = PTR_ERR(st->regmap);
+		goto error_free_dev;
+	}
 	/* this is only used for removal purposes */
 	spi_set_drvdata(spi, indio_dev);
-
+	spi->cs_between_transfers = 1;
 	st->us = spi;
 	mutex_init(&st->buf_lock);
 
@@ -1042,29 +1113,29 @@ static int __devinit adis16400_probe(struct spi_device *spi)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = adis16400_configure_ring(indio_dev);
-	if (ret)
-		goto error_free_dev;
+	if (ret < 0)
+		goto error_free_regmap;
 
 	ret = iio_ring_buffer_register(indio_dev,
 				       st->variant->channels,
 				       st->variant->num_channels);
-	if (ret) {
+	if (ret < 0) {
 		dev_err(&spi->dev, "failed to initialize the ring\n");
 		goto error_unreg_ring_funcs;
 	}
 
 	if (spi->irq) {
 		ret = adis16400_probe_trigger(indio_dev);
-		if (ret)
+		if (ret < 0)
 			goto error_uninitialize_ring;
 	}
 
 	/* Get the device into a sane initial state */
 	ret = adis16400_initial_setup(indio_dev);
-	if (ret)
+	if (ret < 0)
 		goto error_remove_trigger;
 	ret = iio_device_register(indio_dev);
-	if (ret)
+	if (ret < 0)
 		goto error_remove_trigger;
 
 	return 0;
@@ -1076,6 +1147,8 @@ error_uninitialize_ring:
 	iio_ring_buffer_unregister(indio_dev);
 error_unreg_ring_funcs:
 	adis16400_unconfigure_ring(indio_dev);
+error_free_regmap:
+	regmap_exit(st->regmap);
 error_free_dev:
 	iio_free_device(indio_dev);
 error_ret:
@@ -1087,14 +1160,16 @@ static int adis16400_remove(struct spi_device *spi)
 {
 	int ret;
 	struct iio_dev *indio_dev =  spi_get_drvdata(spi);
+	struct adis16400_state *st = iio_priv(indio_dev);
 
 	ret = adis16400_stop_device(indio_dev);
-	if (ret)
+	if (ret < 0)
 		goto err_ret;
 
 	adis16400_remove_trigger(indio_dev);
 	iio_ring_buffer_unregister(indio_dev);
 	adis16400_unconfigure_ring(indio_dev);
+	regmap_exit(st->regmap);
 	iio_device_unregister(indio_dev);
 
 	return 0;
-- 
1.7.3.4

Re: [PATCH 2/2] staging:iio:imu:adis16400 regmap introduction.

[Mark Brown]

On Thu, Sep 08, 2011 at 03:09:24PM +0100, Jonathan Cameron wrote:
> Apply regmap for the basic register reads and writes.
> Note not currently used at all for the mass reads
> that occur in the buffer code.

Looks good, shame adding the register access lists makes the diffstat
grow rather than shrink - a separate patch adding the access maps would
be good for showing the benefits :)

> +	case ADIS16400_DIAG_STAT:
> +		return true;
> +	default:
> +		return 0;

Should use either 1/0 or true/false consistently (probably the latter).

> +static const struct regmap_config adis16400_regmap_config = {
> +	.reg_bits = 8,
> +	.reg_pad_bits = 8,
> +	.val_bits = 16,
> +	.half_write = true,
> +	.writeable_reg = &adis16400_reg_writeable,
> +	.readable_reg = &adis16400_reg_readable,
> +	.precious_reg = &adis16400_reg_precious,
> +	.volatile_reg = &adis16400_reg_volatile,
> +	.max_register = 0x56,
> +	.write_flag_mask = 0x80,
> +	.read_flag_mask = 0,

No need to set things to zero explicitly.

> -
> +	spi->cs_between_transfers = 1;

Guess we need that change to go in first...