On Fri, Mar 06, 2020 at 01:01:47PM +0000, Srinivas Kandagatla wrote:
From: Takahide Higuchi takahidehiguchi@gmail.com
This patch fixes a bug where playback on bidirectional I2S interface stops when we start recording on the same interface.
We use regmap_update_bits instead of regmap_write so that we will not clear SPKEN and SPKMODE bits when we start/stop recording.
Thanks for the patch! I don't have suitable hardware to test this feature, but I checked if playback-only is still working correctly:
With this patch I can only play audio once. Further playback never starts, it gets stuck immediately. Suggested fix below. (Also a minor code style suggestion...)
Signed-off-by: Takahide Higuchi takahidehiguchi@gmail.com Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org
sound/soc/qcom/lpass-apq8016.c | 2 ++ sound/soc/qcom/lpass-cpu.c | 24 ++++++++++++++++++------ sound/soc/qcom/lpass-lpaif-reg.h | 2 +- 3 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/sound/soc/qcom/lpass-apq8016.c b/sound/soc/qcom/lpass-apq8016.c index 6575da549237..85079c697faa 100644 --- a/sound/soc/qcom/lpass-apq8016.c +++ b/sound/soc/qcom/lpass-apq8016.c @@ -121,6 +121,8 @@ static struct snd_soc_dai_driver apq8016_lpass_cpu_dai_driver[] = { }, .probe = &asoc_qcom_lpass_cpu_dai_probe, .ops = &asoc_qcom_lpass_cpu_dai_ops,
.symmetric_samplebits = 1,.symmetric_rates = 1,};
diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c index dbce7e92baf3..dc8acb380b6f 100644 --- a/sound/soc/qcom/lpass-cpu.c +++ b/sound/soc/qcom/lpass-cpu.c @@ -72,6 +72,7 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream, snd_pcm_format_t format = params_format(params); unsigned int channels = params_channels(params); unsigned int rate = params_rate(params);
- unsigned int mask; unsigned int regval; int bitwidth, ret;
@@ -81,6 +82,9 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream, return bitwidth; }
- mask = LPAIF_I2SCTL_LOOPBACK_MASK |
LPAIF_I2SCTL_WSSRC_MASK |LPAIF_I2SCTL_BITWIDTH_MASK;
I understand the space after "mask" here (to align the =) ...
@@ -100,6 +104,7 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream, }
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
mask |= LPAIF_I2SCTL_SPKMODE_MASK | LPAIF_I2SCTL_SPKMONO_MASK;@@ -127,6 +132,7 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream, return -EINVAL; } } else {
mask |= LPAIF_I2SCTL_MICMODE_MASK | LPAIF_I2SCTL_MICMONO_MASK;
... but they don't make much sense to me in these two cases. It looks a bit weird in my opinion :)
@@ -155,9 +161,9 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream, } }
- ret = regmap_write(drvdata->lpaif_map,
LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),regval);
- ret = regmap_update_bits(drvdata->lpaif_map,
LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),mask, regval);@@ -178,11 +184,17 @@ static int lpass_cpu_daiops_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
- unsigned int mask; int ret;
- ret = regmap_write(drvdata->lpaif_map,
LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),0);
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
mask = LPAIF_I2SCTL_SPKMODE_MASK;- else
mask = LPAIF_I2SCTL_MICMODE_MASK;
Same here with the spaces after "mask", but more importantly:
This changes the behavior slightly. Previously we reset the entire register, now only the SPKMODE/MICMODE. This means that SPKEN/MICEN does not get reset to 0 when this function is called.
Everything seems to work fine again for me if I reset SPKEN/MICEN too:
diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c index dc8acb380b6f..49a36c1f5640 100644 --- a/sound/soc/qcom/lpass-cpu.c +++ b/sound/soc/qcom/lpass-cpu.c @@ -188,9 +188,9 @@ static int lpass_cpu_daiops_hw_free(struct snd_pcm_substream *substream, int ret;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - mask = LPAIF_I2SCTL_SPKMODE_MASK; + mask = LPAIF_I2SCTL_SPKEN_MASK | LPAIF_I2SCTL_SPKMODE_MASK; else - mask = LPAIF_I2SCTL_MICMODE_MASK; + mask = LPAIF_I2SCTL_MICEN_MASK | LPAIF_I2SCTL_MICMODE_MASK;
ret = regmap_update_bits(drvdata->lpaif_map, LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
After looking at the code a bit more I wonder why SPKEN/MICEN is set in prepare() at all? It is already set/un-set in trigger() on SNDRV_PCM_TRIGGER_START and SNDRV_PCM_TRIGGER_STOP.
In fact, my problem only happens because prepare() is called again after the stream was stopped (and therefore sets SPKEN again):
[ 32.809915] lpass_cpu_daiops_hw_params [ 32.810598] lpass_cpu_daiops_prepare [ 32.823092] lpass_cpu_daiops_prepare [ 32.837901] lpass_cpu_daiops_trigger: SNDRV_PCM_TRIGGER_START [ 37.565261] lpass_cpu_daiops_trigger: SNDRV_PCM_TRIGGER_STOP [ 37.565402] lpass_cpu_daiops_prepare [ 37.565854] lpass_cpu_daiops_hw_free
In other words, a much better fix in my opinion would be to simply remove(!) the entire lpass_cpu_daiops_prepare() method.
I tested it and that also fixes the problem, without my diff above to unset SPKEN/MICEN in lpass_cpu_daiops_hw_free().
Thanks, Stephan
- ret = regmap_update_bits(drvdata->lpaif_map,
LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),mask, 0);diff --git a/sound/soc/qcom/lpass-lpaif-reg.h b/sound/soc/qcom/lpass-lpaif-reg.h index 3d74ae123e9d..7a2b9cf99976 100644 --- a/sound/soc/qcom/lpass-lpaif-reg.h +++ b/sound/soc/qcom/lpass-lpaif-reg.h @@ -56,7 +56,7 @@ #define LPAIF_I2SCTL_MICMODE_6CH (7 << LPAIF_I2SCTL_MICMODE_SHIFT) #define LPAIF_I2SCTL_MICMODE_8CH (8 << LPAIF_I2SCTL_MICMODE_SHIFT)
-#define LPAIF_I2SCTL_MIMONO_MASK GENMASK(3, 3) +#define LPAIF_I2SCTL_MICMONO_MASK GENMASK(3, 3) #define LPAIF_I2SCTL_MICMONO_SHIFT 3 #define LPAIF_I2SCTL_MICMONO_STEREO (0 << LPAIF_I2SCTL_MICMONO_SHIFT)
#define LPAIF_I2SCTL_MICMONO_MONO (1 << LPAIF_I2SCTL_MICMONO_SHIFT)
2.21.0