* [PATCH] ASoC: qcom: lpass-cpu: support full duplex operation
@ 2020-03-06 13:01 ` Srinivas Kandagatla
0 siblings, 0 replies; 8+ messages in thread
From: Srinivas Kandagatla @ 2020-03-06 13:01 UTC (permalink / raw)
To: broonie; +Cc: Srinivas Kandagatla, alsa-devel, Takahide Higuchi, linux-kernel
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>
---
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)
--
2.21.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] ASoC: qcom: lpass-cpu: support full duplex operation
2020-03-06 13:01 ` Srinivas Kandagatla
@ 2020-04-16 8:05 ` Srinivas Kandagatla
-1 siblings, 0 replies; 8+ messages in thread
From: Srinivas Kandagatla @ 2020-04-16 8:05 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, linux-kernel, Takahide Higuchi
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)
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ASoC: qcom: lpass-cpu: support full duplex operation
@ 2020-04-16 8:05 ` Srinivas Kandagatla
0 siblings, 0 replies; 8+ messages in thread
From: Srinivas Kandagatla @ 2020-04-16 8:05 UTC (permalink / raw)
To: broonie; +Cc: alsa-devel, Takahide Higuchi, linux-kernel
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)
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ASoC: qcom: lpass-cpu: support full duplex operation
2020-04-16 8:05 ` Srinivas Kandagatla
@ 2020-04-16 9:35 ` Mark Brown
-1 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2020-04-16 9:35 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: alsa-devel, linux-kernel, Takahide Higuchi
[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: qcom: lpass-cpu: support full duplex operation
@ 2020-04-16 9:35 ` Mark Brown
0 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2020-04-16 9:35 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: alsa-devel, Takahide Higuchi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: qcom: lpass-cpu: support full duplex operation
2020-03-06 13:01 ` Srinivas Kandagatla
@ 2020-04-16 17:16 ` Stephan Gerhold
-1 siblings, 0 replies; 8+ messages in thread
From: Stephan Gerhold @ 2020-04-16 17:16 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: broonie, alsa-devel, Takahide Higuchi, linux-kernel
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
>
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] ASoC: qcom: lpass-cpu: support full duplex operation
@ 2020-04-16 17:16 ` Stephan Gerhold
0 siblings, 0 replies; 8+ messages in thread
From: Stephan Gerhold @ 2020-04-16 17:16 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: alsa-devel, broonie, Takahide Higuchi, linux-kernel
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
>
^ permalink raw reply related [flat|nested] 8+ messages in thread