[PATCH 1/2] regmap: core: Split out in place value parsing

[PATCH 1/2] regmap: core: Split out in place value parsing

[Mark Brown]

Currently the value parsing operations both return the parsed value and
modify the passed buffer. This precludes their use in places like the cache
code so split out the in place modification into a new parse_inplace()
operation.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/internal.h |    3 ++-
 drivers/base/regmap/regmap.c   |   52 +++++++++++++++++++++++++++-------------
 2 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 2aa8772..f0e0ca4 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -38,7 +38,8 @@ struct regmap_format {
 			     unsigned int reg, unsigned int val);
 	void (*format_reg)(void *buf, unsigned int reg, unsigned int shift);
 	void (*format_val)(void *buf, unsigned int val, unsigned int shift);
-	unsigned int (*parse_val)(void *buf);
+	unsigned int (*parse_val)(const void *buf);
+	void (*parse_inplace)(void *buf);
 };
 
 struct regmap_async {
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 7c6d3be..4b01404 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -228,30 +228,39 @@ static void regmap_format_32_native(void *buf, unsigned int val,
 	*(u32 *)buf = val << shift;
 }
 
-static unsigned int regmap_parse_8(void *buf)
+static void regmap_parse_inplace_noop(void *buf)
 {
-	u8 *b = buf;
+}
+
+static unsigned int regmap_parse_8(const void *buf)
+{
+	const u8 *b = buf;
 
 	return b[0];
 }
 
-static unsigned int regmap_parse_16_be(void *buf)
+static unsigned int regmap_parse_16_be(const void *buf)
+{
+	const __be16 *b = buf;
+
+	return be16_to_cpu(b[0]);
+}
+
+static void regmap_parse_16_be_inplace(void *buf)
 {
 	__be16 *b = buf;
 
 	b[0] = be16_to_cpu(b[0]);
-
-	return b[0];
 }
 
-static unsigned int regmap_parse_16_native(void *buf)
+static unsigned int regmap_parse_16_native(const void *buf)
 {
 	return *(u16 *)buf;
 }
 
-static unsigned int regmap_parse_24(void *buf)
+static unsigned int regmap_parse_24(const void *buf)
 {
-	u8 *b = buf;
+	const u8 *b = buf;
 	unsigned int ret = b[2];
 	ret |= ((unsigned int)b[1]) << 8;
 	ret |= ((unsigned int)b[0]) << 16;
@@ -259,16 +268,21 @@ static unsigned int regmap_parse_24(void *buf)
 	return ret;
 }
 
-static unsigned int regmap_parse_32_be(void *buf)
+static unsigned int regmap_parse_32_be(const void *buf)
+{
+	const __be32 *b = buf;
+
+	return be32_to_cpu(b[0]);
+}
+
+static void regmap_parse_32_be_inplace(void *buf)
 {
 	__be32 *b = buf;
 
 	b[0] = be32_to_cpu(b[0]);
-
-	return b[0];
 }
 
-static unsigned int regmap_parse_32_native(void *buf)
+static unsigned int regmap_parse_32_native(const void *buf)
 {
 	return *(u32 *)buf;
 }
@@ -555,16 +569,21 @@ struct regmap *regmap_init(struct device *dev,
 		goto err_map;
 	}
 
+	if (val_endian == REGMAP_ENDIAN_NATIVE)
+		map->format.parse_inplace = regmap_parse_inplace_noop;
+
 	switch (config->val_bits) {
 	case 8:
 		map->format.format_val = regmap_format_8;
 		map->format.parse_val = regmap_parse_8;
+		map->format.parse_inplace = regmap_parse_inplace_noop;
 		break;
 	case 16:
 		switch (val_endian) {
 		case REGMAP_ENDIAN_BIG:
 			map->format.format_val = regmap_format_16_be;
 			map->format.parse_val = regmap_parse_16_be;
+			map->format.parse_inplace = regmap_parse_16_be_inplace;
 			break;
 		case REGMAP_ENDIAN_NATIVE:
 			map->format.format_val = regmap_format_16_native;
@@ -585,6 +604,7 @@ struct regmap *regmap_init(struct device *dev,
 		case REGMAP_ENDIAN_BIG:
 			map->format.format_val = regmap_format_32_be;
 			map->format.parse_val = regmap_parse_32_be;
+			map->format.parse_inplace = regmap_parse_32_be_inplace;
 			break;
 		case REGMAP_ENDIAN_NATIVE:
 			map->format.format_val = regmap_format_32_native;
@@ -1242,7 +1262,7 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
 
 	if (!map->bus)
 		return -EINVAL;
-	if (!map->format.parse_val)
+	if (!map->format.parse_inplace)
 		return -EINVAL;
 	if (reg % map->reg_stride)
 		return -EINVAL;
@@ -1260,7 +1280,7 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
 			goto out;
 		}
 		for (i = 0; i < val_count * val_bytes; i += val_bytes)
-			map->format.parse_val(wval + i);
+			map->format.parse_inplace(wval + i);
 	}
 	/*
 	 * Some devices does not support bulk write, for
@@ -1521,7 +1541,7 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 
 	if (!map->bus)
 		return -EINVAL;
-	if (!map->format.parse_val)
+	if (!map->format.parse_inplace)
 		return -EINVAL;
 	if (reg % map->reg_stride)
 		return -EINVAL;
@@ -1548,7 +1568,7 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 		}
 
 		for (i = 0; i < val_count * val_bytes; i += val_bytes)
-			map->format.parse_val(val + i);
+			map->format.parse_inplace(val + i);
 	} else {
 		for (i = 0; i < val_count; i++) {
 			unsigned int ival;
-- 
1.7.10.4

[PATCH 2/2] regmap: cache: Store caches in native register format where possible

[Mark Brown]

This allows the cached data to be sent directly to the device when
we sync it.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/regcache.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 6948996..0f4fb8b 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -45,8 +45,8 @@ static int regcache_hw_init(struct regmap *map)
 		tmp_buf = kmalloc(map->cache_size_raw, GFP_KERNEL);
 		if (!tmp_buf)
 			return -EINVAL;
-		ret = regmap_bulk_read(map, 0, tmp_buf,
-				       map->num_reg_defaults_raw);
+		ret = regmap_raw_read(map, 0, tmp_buf,
+				      map->num_reg_defaults_raw);
 		map->cache_bypass = cache_bypass;
 		if (ret < 0) {
 			kfree(tmp_buf);
@@ -421,6 +421,13 @@ bool regcache_set_val(struct regmap *map, void *base, unsigned int idx,
 	if (regcache_get_val(map, base, idx) == val)
 		return true;
 
+	/* Use device native format if possible */
+	if (map->format.format_val) {
+		map->format.format_val(base + (map->cache_word_size * idx),
+				       val, 0);
+		return false;
+	}
+
 	switch (map->cache_word_size) {
 	case 1: {
 		u8 *cache = base;
@@ -449,6 +456,11 @@ unsigned int regcache_get_val(struct regmap *map, const void *base,
 	if (!base)
 		return -EINVAL;
 
+	/* Use device native format if possible */
+	if (map->format.parse_val)
+		return map->format.parse_val(base +
+					     (map->cache_word_size * idx));
+
 	switch (map->cache_word_size) {
 	case 1: {
 		const u8 *cache = base;
-- 
1.7.10.4

Re: [PATCH 1/2] regmap: core: Split out in place value parsing

[Stephen Warren]

On 03/03/2013 06:14 PM, Mark Brown wrote:
> Currently the value parsing operations both return the parsed value and
> modify the passed buffer. This precludes their use in places like the cache
> code so split out the in place modification into a new parse_inplace()
> operation.

Mark,

This change seems to break audio on my Tegra system with a WM8903 CODEC.
In next-20130318, reverting this plus "regmap: cache: Store caches in
native register format where possible" which depends on it does solve
the problem.

Looking at the raw WM8903 registers using i2cdump shows that for example
without these patches, register 39h is set to 0x0020, but with them it's
set to 0x0021, so the headphones are muted. In both cases, the regmap
debugfs file thinks the value last written is 0xa1 (the 0x80 bit isn't
actually state stored in HW, so the only relevant difference is in the
LSB). There are numerous other differences in i2cdump output.

It took a very quick look at the patch and couldn't see anything
actively wrong. I wonder if one of the existing unconverted users of
.parse_val() relies on the in-place modification of the buffer as well
as getting the result back, and so should have been converted to calling
both .parse_inplace() and then .parse_val()?

Re: [PATCH 1/2] regmap: core: Split out in place value parsing

[Stephen Warren]

On 03/19/2013 10:50 AM, Mark Brown wrote:
> On Mon, Mar 18, 2013 at 05:41:49PM -0600, Stephen Warren wrote:
> 
>> It took a very quick look at the patch and couldn't see anything 
>> actively wrong. I wonder if one of the existing unconverted users
>> of .parse_val() relies on the in-place modification of the buffer
>> as well as getting the result back, and so should have been
>> converted to calling both .parse_inplace() and then
>> .parse_val()?
> 
> Possibly, though you'd have thought that if it were just that one
> of the other users would have noticed - my primary development
> board uses regmap extensively for example.  Does seem the most
> likely option though.  Can't test anything again until Friday
> sadly.
> 
> Might also be some unusual code path WM8903 exercises, though again
> it's pretty simple.

I haven't thought through why the patch in question causes/exposes the
issue yet, but I have found out what the problem is.

_regmap_bus_raw_write() formats the value into work_buf + reg_bytes +
pad_bytes, i.e. work_buf + 1.

For the first regmap_write() that the WM8903 driver does, work_buf is
now xx 89 03.

_regmap_raw_write() then memcpy()s from val (i.e. work_buf + 1) to
workbuf, and parses the value to send to the caching code:

> if (!map->cache_bypass && map->format.parse_val) { unsigned int
> ival; int val_bytes = map->format.val_bytes; for (i = 0; i <
> val_len / val_bytes; i++) { memcpy(map->work_buf, val + (i *
> val_bytes), val_bytes); ival =
> map->format.parse_val(map->work_buf);

This corrupts that value at work_buf + 1 since it overlaps the copy
destination at work_buf. work_buf is now 89 03 03.

_regmap_raw_write() then formats the register address into work_buf,
yielding 00 03 03.

That data stream is then sent over I2C, causing a write to register 0
(correct) of value 03 03 (rather than 89 03).

If I comment out the memcpy, and instead pass the value address
directly to .parse_val(), everything works again.

Re: [PATCH 1/2] regmap: core: Split out in place value parsing

[Stephen Warren]

On 03/20/2013 04:19 PM, Stephen Warren wrote:
> On 03/19/2013 10:50 AM, Mark Brown wrote:
>> On Mon, Mar 18, 2013 at 05:41:49PM -0600, Stephen Warren wrote:
>>
>>> It took a very quick look at the patch and couldn't see anything 
>>> actively wrong. I wonder if one of the existing unconverted users
>>> of .parse_val() relies on the in-place modification of the buffer
>>> as well as getting the result back, and so should have been
>>> converted to calling both .parse_inplace() and then
>>> .parse_val()?
>>
>> Possibly, though you'd have thought that if it were just that one
>> of the other users would have noticed - my primary development
>> board uses regmap extensively for example.  Does seem the most
>> likely option though.  Can't test anything again until Friday
>> sadly.
>>
>> Might also be some unusual code path WM8903 exercises, though again
>> it's pretty simple.
> 
> I haven't thought through why the patch in question causes/exposes the
> issue yet, but I have found out what the problem is.
> 
> _regmap_bus_raw_write() formats the value into work_buf + reg_bytes +
> pad_bytes, i.e. work_buf + 1.
> 
> For the first regmap_write() that the WM8903 driver does, work_buf is
> now xx 89 03.
> 
> _regmap_raw_write() then memcpy()s from val (i.e. work_buf + 1) to
> workbuf, and parses the value to send to the caching code:
> 
>> if (!map->cache_bypass && map->format.parse_val) { unsigned int
>> ival; int val_bytes = map->format.val_bytes; for (i = 0; i <
>> val_len / val_bytes; i++) { memcpy(map->work_buf, val + (i *
>> val_bytes), val_bytes); ival =
>> map->format.parse_val(map->work_buf);
> 
> This corrupts that value at work_buf + 1 since it overlaps the copy
> destination at work_buf. work_buf is now 89 03 03.

Ah, here's why it used to work:

When parse_val used to both return the value and modify the buffer in
place, parse_val used to re-write the buffer from 89 03 03 to 03 89 03,
which is the same content that it originally had before the memcpy, at
least for the region that holds the value rather than the address, which
is all that's relevant since the address gets over-written later. The
combination of the memcpy-down-by-1-byte followed by swap-first-2-bytes,
just accidentally happened to leave the buffer in a valid state. Of
course, this only works for certain combinations of register address and
value sizes, I think...

So, I think that the fix I mentioned below really is valid, and what's
more is technically needed irrespective of whether "regmap: core: Split
out in place value parsing" is applied. I'll assume so and send out that
patch.

> _regmap_raw_write() then formats the register address into work_buf,
> yielding 00 03 03.
> 
> That data stream is then sent over I2C, causing a write to register 0
> (correct) of value 03 03 (rather than 89 03).
> 
> If I comment out the memcpy, and instead pass the value address
> directly to .parse_val(), everything works again.