| Message ID | 20200306130147.27452-1-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: qcom: lpass-cpu: support full duplex operation | expand |
Hi Mark, On 06/03/2020 13:01, 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. > > Signed-off-by: Takahide Higuchi <takahidehiguchi@gmail.com> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Looks like this patch was missed in last cycle, Should I resend this one or are you okay to apply this. This patch is required to get full duplex on msm8916 based platforms. thanks, srini > --- > 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; > regval = LPAIF_I2SCTL_LOOPBACK_DISABLE | > LPAIF_I2SCTL_WSSRC_INTERNAL; > > @@ -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; > switch (channels) { > case 1: > regval |= LPAIF_I2SCTL_SPKMODE_SD0; > @@ -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; > switch (channels) { > case 1: > regval |= LPAIF_I2SCTL_MICMODE_SD0; > @@ -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); > if (ret) { > dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret); > return ret; > @@ -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; > + > + ret = regmap_update_bits(drvdata->lpaif_map, > + LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id), > + mask, 0); > if (ret) > dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret); > > 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) >
On Thu, Apr 16, 2020 at 09:05:55AM +0100, Srinivas Kandagatla wrote: > Looks like this patch was missed in last cycle, Should I resend this one or > are you okay to apply this. This patch is required to get full duplex on > msm8916 based platforms. Please don't send content free pings and please allow a reasonable time for review. People get busy, go on holiday, attend conferences and so on so unless there is some reason for urgency (like critical bug fixes) please allow at least a couple of weeks for review. If there have been review comments then people may be waiting for those to be addressed. Sending content free pings adds to the mail volume (if they are seen at all) which is often the problem and since they can't be reviewed directly if something has gone wrong you'll have to resend the patches anyway, so sending again is generally a better approach though there are some other maintainers who like them - if in doubt look at how patches for the subsystem are normally handled.
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; > regval = LPAIF_I2SCTL_LOOPBACK_DISABLE | > LPAIF_I2SCTL_WSSRC_INTERNAL; > 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; > switch (channels) { > case 1: > regval |= LPAIF_I2SCTL_SPKMODE_SD0; > @@ -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; > switch (channels) { > case 1: > regval |= LPAIF_I2SCTL_MICMODE_SD0; ... 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); > if (ret) { > dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret); > return ret; > @@ -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); > if (ret) > dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret); > > 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 >
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; regval = LPAIF_I2SCTL_LOOPBACK_DISABLE | LPAIF_I2SCTL_WSSRC_INTERNAL; @@ -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; switch (channels) { case 1: regval |= LPAIF_I2SCTL_SPKMODE_SD0; @@ -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; switch (channels) { case 1: regval |= LPAIF_I2SCTL_MICMODE_SD0; @@ -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); if (ret) { dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret); return ret; @@ -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; + + ret = regmap_update_bits(drvdata->lpaif_map, + LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id), + mask, 0); if (ret) dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret); 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)