[alsa-devel] [RFC/RFT PATCH] Revert "ASoC: fsl_esai: ETDR and TX0~5 registers are non volatile"
This reverts commit 8973112aa41b8ad956a5b47f2fe17bc2a5cf2645.
ETDR and TX0~5 are TX data registers. There are a couple of reasons to revert the change: 1) Though ETDR and TX0~5 are not volatile but write-only registers, they should not be cached either. According to the definition of "volatile_reg", one should be put in the volatile list if it can not be cached. 2) When doing regcache_sync(), the operation may accidentally write some "dirty" data into these registers, in case that cached data happen to be different from the default ones. It may also result in a channel shift/swap situation since the number of write-via- sync operations at ETDR would unlikely match the channel number.
Note: this revert is not a complete revert as it keeps those macros of registers remaining in the default value list while the original commit also changed other entries in the list. And this patch isn't very necessary to Cc stable tree since there has been always a FIFO reset operation around the regcache_sync() call, even prior to this reverted commit.
Signed-off-by: Nicolin Chen nicoleotsuka@gmail.com Cc: Shengjiu Wang shengjiu.wang@nxp.com --- Hi Mark, In case there's no objection against the patch, I'd still like to wait for a Tested-by from NXP folks before submitting it. Thanks!
sound/soc/fsl/fsl_esai.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index 10d2210c91ef..8f0a86335f73 100644 --- a/sound/soc/fsl/fsl_esai.c +++ b/sound/soc/fsl/fsl_esai.c @@ -652,16 +652,9 @@ static const struct snd_soc_component_driver fsl_esai_component = { };
static const struct reg_default fsl_esai_reg_defaults[] = { - {REG_ESAI_ETDR, 0x00000000}, {REG_ESAI_ECR, 0x00000000}, {REG_ESAI_TFCR, 0x00000000}, {REG_ESAI_RFCR, 0x00000000}, - {REG_ESAI_TX0, 0x00000000}, - {REG_ESAI_TX1, 0x00000000}, - {REG_ESAI_TX2, 0x00000000}, - {REG_ESAI_TX3, 0x00000000}, - {REG_ESAI_TX4, 0x00000000}, - {REG_ESAI_TX5, 0x00000000}, {REG_ESAI_TSR, 0x00000000}, {REG_ESAI_SAICR, 0x00000000}, {REG_ESAI_TCR, 0x00000000}, @@ -711,10 +704,17 @@ static bool fsl_esai_readable_reg(struct device *dev, unsigned int reg) static bool fsl_esai_volatile_reg(struct device *dev, unsigned int reg) { switch (reg) { + case REG_ESAI_ETDR: case REG_ESAI_ERDR: case REG_ESAI_ESR: case REG_ESAI_TFSR: case REG_ESAI_RFSR: + case REG_ESAI_TX0: + case REG_ESAI_TX1: + case REG_ESAI_TX2: + case REG_ESAI_TX3: + case REG_ESAI_TX4: + case REG_ESAI_TX5: case REG_ESAI_RX0: case REG_ESAI_RX1: case REG_ESAI_RX2:
On Thu, Jun 06, 2019 at 04:01:05PM -0700, Nicolin Chen wrote:
This reverts commit 8973112aa41b8ad956a5b47f2fe17bc2a5cf2645.
Please use subject lines matching the style for the subsystem. This makes it easier for people to identify relevant patches.
- Though ETDR and TX0~5 are not volatile but write-only registers, they should not be cached either. According to the definition of "volatile_reg", one should be put in the volatile list if it can not be cached.
There's no problem with caching write only registers, having a cache allows one to do read/modify/write cycles on them and can help with debugging. The original reason we had cache code in ASoC was for write only devices.
Hello Mark,
On Fri, Jun 07, 2019 at 12:12:44PM +0100, Mark Brown wrote:
On Thu, Jun 06, 2019 at 04:01:05PM -0700, Nicolin Chen wrote:
This reverts commit 8973112aa41b8ad956a5b47f2fe17bc2a5cf2645.
Please use subject lines matching the style for the subsystem. This makes it easier for people to identify relevant patches.
- Though ETDR and TX0~5 are not volatile but write-only registers, they should not be cached either. According to the definition of "volatile_reg", one should be put in the volatile list if it can not be cached.
There's no problem with caching write only registers, having a cache allows one to do read/modify/write cycles on them and can help with debugging. The original reason we had cache code in ASoC was for write only devices.
Maybe because my paragraph doesn't state it clearly -- it's nothing wrong with regmap caching write-only registers; but it caching data registers would potentially cause dirty data or channel swap/shift. So the reason (1) here is "cannot cached" == "should be volatile".
I will revise the commit message for review and fix the subject.
Thank you Nicolin
participants (2)
-
Mark Brown -
Nicolin Chen