[PATCH 0/2] ASoC/regmap: rt5640: Fix resume

[PATCH 0/2] ASoC/regmap: rt5640: Fix resume

[Jarkko Nikula]

Realtek RT5642 audio codec fails to resume because regmap/regcache tries
to sync consecutive registers using block writes but RT5642 doesn't support
autoincrementing I2C writes according to debugging and oscilloscope
measurments.

I'm not sure is there regression after 75a5f89f635c ("regmap: cache: Write
consecutive registers in a single block write") or was RT564x resume ever
working in mainline so I didn't mark stable yet.

Both patches are independent from each other and can be applied into their
own subsystems separately.

Jarkko Nikula (2):
  regmap: cache: Fix regcache_sync_block for non-autoincrementing
    devices
  ASoC: rt5640: Do not allow regmap to use bulk read-write operations

 drivers/base/regmap/regcache.c | 14 +++++++++++++-
 sound/soc/codecs/rt5640.c      |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

-- 
2.1.0

[PATCH 1/2] regmap: cache: Fix regcache_sync_block for non-autoincrementing devices

[Jarkko Nikula]

Commit 75a5f89f635c ("regmap: cache: Write consecutive registers in a single
block write") expected that autoincrementing writes are supported if
hardware has a register format which can support raw writes.

This is not necessarily true and thus for instance rbtree sync can fail when
there is need to sync multiple consecutive registers but block write to
device fails due not supported autoincrementing writes.

Fix this by spliting raw block sync to series of single register writes for
devices that don't support autoincrementing writes.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
I noticed this with Realtek RT5642 audio codec which didn't resume properly
since first block write having more data than for single register failed to
not acknowledged I2C write during regcache_sync(). Chip acknowledges device
address, register address and two data bytes for its word size registers but
next data byte is not which then causes aborted I2C transfer and aborted
register sync.
---
 drivers/base/regmap/regcache.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 29b4128da0b0..54707e586ac8 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -629,6 +629,7 @@ static int regcache_sync_block_raw_flush(struct regmap *map, const void **data,
 {
 	size_t val_bytes = map->format.val_bytes;
 	int ret, count;
+	unsigned int i;
 
 	if (*data == NULL)
 		return 0;
@@ -640,7 +641,18 @@ static int regcache_sync_block_raw_flush(struct regmap *map, const void **data,
 
 	map->cache_bypass = 1;
 
-	ret = _regmap_raw_write(map, base, *data, count * val_bytes);
+	if (!map->use_single_rw) {
+		ret = _regmap_raw_write(map, base, *data, count * val_bytes);
+	} else {
+		for (i = 0; i < count; i++) {
+			ret = _regmap_raw_write(map,
+						base + (i * map->reg_stride),
+						*data + (i * val_bytes),
+						val_bytes);
+			if (ret != 0)
+				break;
+		}
+	}
 
 	map->cache_bypass = 0;
 
-- 
2.1.0

[PATCH 2/2] ASoC: rt5640: Do not allow regmap to use bulk read-write operations

[Jarkko Nikula]

Debugging showed Realtek RT5642 doesn't support autoincrementing writes so
driver should set the use_single_rw flag for regmap.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Bard Liao <bardliao@realtek.com>
Cc: Oder Chiou <oder_chiou@realtek.com>
---
I'm not sure is this specific only to RT5642? I was thinking that because
commit 4c9185be5e8e ("ASoC: rt5640: Move cache sync() to resume()") is way
after 75a5f89f635c ("regmap: cache: Write consecutive registers in a single
block write") which started to use block writes during rbtree sync.
Or maybe 4c9185be5e8e was done on top of older kernel?
---
 sound/soc/codecs/rt5640.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
index 6bc6efdec550..f1ec6e6bd08a 100644
--- a/sound/soc/codecs/rt5640.c
+++ b/sound/soc/codecs/rt5640.c
@@ -2059,6 +2059,7 @@ static struct snd_soc_codec_driver soc_codec_dev_rt5640 = {
 static const struct regmap_config rt5640_regmap = {
 	.reg_bits = 8,
 	.val_bits = 16,
+	.use_single_rw = true,
 
 	.max_register = RT5640_VENDOR_ID2 + 1 + (ARRAY_SIZE(rt5640_ranges) *
 					       RT5640_PR_SPACING),
-- 
2.1.0

Re: [alsa-devel] [PATCH 1/2] regmap: cache: Fix regcache_sync_block for non-autoincrementing devices

[Takashi Iwai]

At Tue, 26 Aug 2014 17:03:12 +0300,
Jarkko Nikula wrote:
> 
> Commit 75a5f89f635c ("regmap: cache: Write consecutive registers in a single
> block write") expected that autoincrementing writes are supported if
> hardware has a register format which can support raw writes.
> 
> This is not necessarily true and thus for instance rbtree sync can fail when
> there is need to sync multiple consecutive registers but block write to
> device fails due not supported autoincrementing writes.
> 
> Fix this by spliting raw block sync to series of single register writes for
> devices that don't support autoincrementing writes.

Wouldn't it suffice to correct regmap_can_raw_write() to return false
if map->use_single_rw is set?


Takashi

> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> I noticed this with Realtek RT5642 audio codec which didn't resume properly
> since first block write having more data than for single register failed to
> not acknowledged I2C write during regcache_sync(). Chip acknowledges device
> address, register address and two data bytes for its word size registers but
> next data byte is not which then causes aborted I2C transfer and aborted
> register sync.
> ---
>  drivers/base/regmap/regcache.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
> index 29b4128da0b0..54707e586ac8 100644
> --- a/drivers/base/regmap/regcache.c
> +++ b/drivers/base/regmap/regcache.c
> @@ -629,6 +629,7 @@ static int regcache_sync_block_raw_flush(struct regmap *map, const void **data,
>  {
>  	size_t val_bytes = map->format.val_bytes;
>  	int ret, count;
> +	unsigned int i;
>  
>  	if (*data == NULL)
>  		return 0;
> @@ -640,7 +641,18 @@ static int regcache_sync_block_raw_flush(struct regmap *map, const void **data,
>  
>  	map->cache_bypass = 1;
>  
> -	ret = _regmap_raw_write(map, base, *data, count * val_bytes);
> +	if (!map->use_single_rw) {
> +		ret = _regmap_raw_write(map, base, *data, count * val_bytes);
> +	} else {
> +		for (i = 0; i < count; i++) {
> +			ret = _regmap_raw_write(map,
> +						base + (i * map->reg_stride),
> +						*data + (i * val_bytes),
> +						val_bytes);
> +			if (ret != 0)
> +				break;
> +		}
> +	}
>  
>  	map->cache_bypass = 0;
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [alsa-devel] [PATCH 1/2] regmap: cache: Fix regcache_sync_block for non-autoincrementing devices

[Jarkko Nikula]

On 08/26/2014 05:21 PM, Takashi Iwai wrote:
> At Tue, 26 Aug 2014 17:03:12 +0300,
> Jarkko Nikula wrote:
>> Commit 75a5f89f635c ("regmap: cache: Write consecutive registers in a single
>> block write") expected that autoincrementing writes are supported if
>> hardware has a register format which can support raw writes.
>>
>> This is not necessarily true and thus for instance rbtree sync can fail when
>> there is need to sync multiple consecutive registers but block write to
>> device fails due not supported autoincrementing writes.
>>
>> Fix this by spliting raw block sync to series of single register writes for
>> devices that don't support autoincrementing writes.
> Wouldn't it suffice to correct regmap_can_raw_write() to return false
> if map->use_single_rw is set?
>
I don't know. I was thinking that also but was unsure about it since 
regcache_sync_block_raw() and regcache_sync_block_single() code paths 
use different regmap write functions. regcache_sync_block_raw() ends up 
calling _regmap_raw_write() which takes care of page select operation 
when needed and regcache_sync_block_single() uses _regmap_write() which 
doesn't.

Which makes me thinking should the regcache_sync_block_single() also use 
_regmap_raw_write() in order to take care of page selects?

-- 
Jarkko

Re: [alsa-devel] [PATCH 1/2] regmap: cache: Fix regcache_sync_block for non-autoincrementing devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

On Wed, Aug 27, 2014 at 08:52:16AM +0300, Jarkko Nikula wrote:

> I don't know. I was thinking that also but was unsure about it since
> regcache_sync_block_raw() and regcache_sync_block_single() code paths use
> different regmap write functions. regcache_sync_block_raw() ends up calling
> _regmap_raw_write() which takes care of page select operation when needed
> and regcache_sync_block_single() uses _regmap_write() which doesn't.

> Which makes me thinking should the regcache_sync_block_single() also use
> _regmap_raw_write() in order to take care of page selects?

We can't use raw_write() for everything since not every bus can do a raw
write.  We probably need to push the select_page() operation into the
_regmap_write() path though, it looks like it's getting missed at the
minute.  I ought to redo a lot of that code to simplify it, it's got too
many tentacles at the minute.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] regmap: cache: Fix regcache_sync_block for non-autoincrementing devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

On Tue, Aug 26, 2014 at 05:03:12PM +0300, Jarkko Nikula wrote:
> Commit 75a5f89f635c ("regmap: cache: Write consecutive registers in a single
> block write") expected that autoincrementing writes are supported if
> hardware has a register format which can support raw writes.
> 
> This is not necessarily true and thus for instance rbtree sync can fail when
> there is need to sync multiple consecutive registers but block write to
> device fails due not supported autoincrementing writes.
> 
> Fix this by spliting raw block sync to series of single register writes for
> devices that don't support autoincrementing writes.

This seems like the wrong place for this - we already have a one
register at a time mechanism for syncing the registers, making the block
write scheme able to do it as well is rather missing the point.  Better
to move the check into sync_block() instead.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ASoC: rt5640: Do not allow regmap to use bulk read-write operations

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 213 bytes --]

On Tue, Aug 26, 2014 at 05:03:13PM +0300, Jarkko Nikula wrote:
> Debugging showed Realtek RT5642 doesn't support autoincrementing writes so
> driver should set the use_single_rw flag for regmap.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]