[alsa-devel] [PATCH 0/2] ASoC/regmap: rt5640: Fix resume
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(-)
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;
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
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?
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),
participants (2)
-
Jarkko Nikula -
Takashi Iwai