[alsa-devel] [PATCH 0/3] ASoC: lpass-platform: Fixes and Cleanup.
Hi Mark,
This patchset fixes broken lpass-platform driver in v4.9-rc1, and also fixes problem while using lpass as kernel module. There is also a cleanup patch in this series just to tidy up the code in the same area where there is regression.
Tested this patchset with msm8916 codec patchset in linux-next on v4.9-rc1 on DB410c.
Without this patchset lpass-platform driver can not play audio or even load/unload as module.
Thanks, srini
Srinivas Kandagatla (3): ASoC: lpass-platform: use dma_ch instead of rdma_ch/wrdma_ch ASoC: lpass-platform: Fix broken pcm data usage ASoC: lpass-cpu: add module licence and description
sound/soc/qcom/lpass-cpu.c | 3 + sound/soc/qcom/lpass-platform.c | 182 ++++++++++++++++------------------------ sound/soc/qcom/lpass.h | 1 - 3 files changed, 76 insertions(+), 110 deletions(-)
-- 2.10.1
This patch cleans up usage of wrdma_ch and rdma_ch variables into a common variable dma_ch, As there is no real use of tracking the dma channel in two different variables based on stream direction.
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/qcom/lpass-platform.c | 53 +++++++++++++---------------------------- 1 file changed, 17 insertions(+), 36 deletions(-)
diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c index e2ff538..2570c96 100644 --- a/sound/soc/qcom/lpass-platform.c +++ b/sound/soc/qcom/lpass-platform.c @@ -25,8 +25,7 @@ #include "lpass.h"
struct lpass_pcm_data { - int rdma_ch; - int wrdma_ch; + int dma_ch; int i2s_port; };
@@ -95,10 +94,7 @@ static int lpass_platform_pcmops_hw_params(struct snd_pcm_substream *substream, int bitwidth; int ret, dma_port = pcm_data->i2s_port + v->dmactl_audif_start;
- if (dir == SNDRV_PCM_STREAM_PLAYBACK) - ch = pcm_data->rdma_ch; - else - ch = pcm_data->wrdma_ch; + ch = pcm_data->dma_ch;
bitwidth = snd_pcm_format_width(format); if (bitwidth < 0) { @@ -184,11 +180,7 @@ static int lpass_platform_pcmops_hw_free(struct snd_pcm_substream *substream) unsigned int reg; int ret;
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - reg = LPAIF_RDMACTL_REG(v, pcm_data->rdma_ch); - else - reg = LPAIF_WRDMACTL_REG(v, pcm_data->wrdma_ch); - + reg = LPAIF_DMACTL_REG(v, pcm_data->dma_ch, substream->stream); ret = regmap_write(drvdata->lpaif_map, reg, 0); if (ret) dev_err(soc_runtime->dev, "%s() error writing to rdmactl reg: %d\n", @@ -207,10 +199,7 @@ static int lpass_platform_pcmops_prepare(struct snd_pcm_substream *substream) struct lpass_variant *v = drvdata->variant; int ret, ch, dir = substream->stream;
- if (dir == SNDRV_PCM_STREAM_PLAYBACK) - ch = pcm_data->rdma_ch; - else - ch = pcm_data->wrdma_ch; + ch = pcm_data->dma_ch;
ret = regmap_write(drvdata->lpaif_map, LPAIF_DMABASE_REG(v, ch, dir), @@ -261,10 +250,7 @@ static int lpass_platform_pcmops_trigger(struct snd_pcm_substream *substream, struct lpass_variant *v = drvdata->variant; int ret, ch, dir = substream->stream;
- if (dir == SNDRV_PCM_STREAM_PLAYBACK) - ch = pcm_data->rdma_ch; - else - ch = pcm_data->wrdma_ch; + ch = pcm_data->dma_ch;
switch (cmd) { case SNDRV_PCM_TRIGGER_START: @@ -338,10 +324,7 @@ static snd_pcm_uframes_t lpass_platform_pcmops_pointer( unsigned int base_addr, curr_addr; int ret, ch, dir = substream->stream;
- if (dir == SNDRV_PCM_STREAM_PLAYBACK) - ch = pcm_data->rdma_ch; - else - ch = pcm_data->wrdma_ch; + ch = pcm_data->dma_ch;
ret = regmap_read(drvdata->lpaif_map, LPAIF_DMABASE_REG(v, ch, dir), &base_addr); @@ -488,13 +471,13 @@ static int lpass_platform_pcm_new(struct snd_soc_pcm_runtime *soc_runtime) psubstream = pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream; if (psubstream) { if (v->alloc_dma_channel) - data->rdma_ch = v->alloc_dma_channel(drvdata, + data->dma_ch = v->alloc_dma_channel(drvdata, SNDRV_PCM_STREAM_PLAYBACK);
- if (data->rdma_ch < 0) - return data->rdma_ch; + if (data->dma_ch < 0) + return data->dma_ch;
- drvdata->substream[data->rdma_ch] = psubstream; + drvdata->substream[data->dma_ch] = psubstream;
ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, soc_runtime->platform->dev, @@ -503,7 +486,7 @@ static int lpass_platform_pcm_new(struct snd_soc_pcm_runtime *soc_runtime) goto playback_alloc_err;
ret = regmap_write(drvdata->lpaif_map, - LPAIF_RDMACTL_REG(v, data->rdma_ch), 0); + LPAIF_RDMACTL_REG(v, data->dma_ch), 0); if (ret) { dev_err(soc_runtime->dev, "%s() error writing to rdmactl reg: %d\n", @@ -515,15 +498,15 @@ static int lpass_platform_pcm_new(struct snd_soc_pcm_runtime *soc_runtime) csubstream = pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream; if (csubstream) { if (v->alloc_dma_channel) - data->wrdma_ch = v->alloc_dma_channel(drvdata, + data->dma_ch = v->alloc_dma_channel(drvdata, SNDRV_PCM_STREAM_CAPTURE);
- if (data->wrdma_ch < 0) { - ret = data->wrdma_ch; + if (data->dma_ch < 0) { + ret = data->dma_ch; goto capture_alloc_err; }
- drvdata->substream[data->wrdma_ch] = csubstream; + drvdata->substream[data->dma_ch] = csubstream;
ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, soc_runtime->platform->dev, @@ -532,7 +515,7 @@ static int lpass_platform_pcm_new(struct snd_soc_pcm_runtime *soc_runtime) goto capture_alloc_err;
ret = regmap_write(drvdata->lpaif_map, - LPAIF_WRDMACTL_REG(v, data->wrdma_ch), 0); + LPAIF_WRDMACTL_REG(v, data->dma_ch), 0); if (ret) { dev_err(soc_runtime->dev, "%s() error writing to wrdmactl reg: %d\n", @@ -573,9 +556,7 @@ static void lpass_platform_pcm_free(struct snd_pcm *pcm) drvdata = snd_soc_platform_get_drvdata(rt->platform); data = drvdata->private_data;
- ch = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - ? data->rdma_ch - : data->wrdma_ch; + ch = data->dma_ch; v = drvdata->variant; drvdata->substream[ch] = NULL; if (v->free_dma_channel)
On Fri, Oct 28, 2016 at 04:32:18PM +0100, Srinivas Kandagatla wrote:
This patch cleans up usage of wrdma_ch and rdma_ch variables into a common variable dma_ch, As there is no real use of tracking the dma channel in two different variables based on stream direction.
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org
sound/soc/qcom/lpass-platform.c | 53 +++++++++++++---------------------------- 1 file changed, 17 insertions(+), 36 deletions(-)
Acked-by: Kenneth Westfield kwestfie@codeaurora.org
This patch fixes lpass-platform driver which was broken in v4.9-rc1. lpass_pcm_data data structure holds information specific to stream. Holding a single private pointer to it in global lpass_data will not work, because it would be overwritten by for each pcm instance.
This code was breaking playback when we have both playback and capture pcm streams, as playback settings are over written by capture settings.
Fix this by moving channel allocation logic out of pcm_new to pcm_open so that we can store the stream specific information private_data of snd_pcm_runtime.
Fixes: 6adcbdcd4b6e ("ASoC: lpass-platform: don't use snd_soc_pcm_set_drvdata()") Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org --- sound/soc/qcom/lpass-platform.c | 151 ++++++++++++++++++---------------------- sound/soc/qcom/lpass.h | 1 - 2 files changed, 67 insertions(+), 85 deletions(-)
diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c index 2570c96..4554cae 100644 --- a/sound/soc/qcom/lpass-platform.c +++ b/sound/soc/qcom/lpass-platform.c @@ -60,7 +60,36 @@ static int lpass_platform_pcmops_open(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct snd_soc_pcm_runtime *soc_runtime = substream->private_data; - int ret; + struct snd_soc_dai *cpu_dai = soc_runtime->cpu_dai; + struct lpass_data *drvdata = + snd_soc_platform_get_drvdata(soc_runtime->platform); + struct lpass_variant *v = drvdata->variant; + int ret, dir = substream->stream; + struct lpass_pcm_data *data; + + data = devm_kzalloc(soc_runtime->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->i2s_port = cpu_dai->driver->id; + runtime->private_data = data; + + if (v->alloc_dma_channel) + data->dma_ch = v->alloc_dma_channel(drvdata, dir); + if (data->dma_ch < 0) + return data->dma_ch; + + drvdata->substream[data->dma_ch] = substream; + + ret = regmap_write(drvdata->lpaif_map, + LPAIF_DMACTL_REG(v, data->dma_ch, dir), 0); + if (ret) { + dev_err(soc_runtime->dev, + "%s() error writing to rdmactl reg: %d\n", + __func__, ret); + return ret; + } +
snd_soc_set_runtime_hwparams(substream, &lpass_platform_pcm_hardware);
@@ -79,13 +108,32 @@ static int lpass_platform_pcmops_open(struct snd_pcm_substream *substream) return 0; }
+static int lpass_platform_pcmops_close(struct snd_pcm_substream *substream) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct snd_soc_pcm_runtime *soc_runtime = substream->private_data; + struct lpass_data *drvdata = + snd_soc_platform_get_drvdata(soc_runtime->platform); + struct lpass_variant *v = drvdata->variant; + struct lpass_pcm_data *data; + + data = runtime->private_data; + v = drvdata->variant; + drvdata->substream[data->dma_ch] = NULL; + if (v->free_dma_channel) + v->free_dma_channel(drvdata, data->dma_ch); + + return 0; +} + static int lpass_platform_pcmops_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { struct snd_soc_pcm_runtime *soc_runtime = substream->private_data; struct lpass_data *drvdata = snd_soc_platform_get_drvdata(soc_runtime->platform); - struct lpass_pcm_data *pcm_data = drvdata->private_data; + struct snd_pcm_runtime *rt = substream->runtime; + struct lpass_pcm_data *pcm_data = rt->private_data; struct lpass_variant *v = drvdata->variant; snd_pcm_format_t format = params_format(params); unsigned int channels = params_channels(params); @@ -175,7 +223,8 @@ static int lpass_platform_pcmops_hw_free(struct snd_pcm_substream *substream) struct snd_soc_pcm_runtime *soc_runtime = substream->private_data; struct lpass_data *drvdata = snd_soc_platform_get_drvdata(soc_runtime->platform); - struct lpass_pcm_data *pcm_data = drvdata->private_data; + struct snd_pcm_runtime *rt = substream->runtime; + struct lpass_pcm_data *pcm_data = rt->private_data; struct lpass_variant *v = drvdata->variant; unsigned int reg; int ret; @@ -195,7 +244,8 @@ static int lpass_platform_pcmops_prepare(struct snd_pcm_substream *substream) struct snd_soc_pcm_runtime *soc_runtime = substream->private_data; struct lpass_data *drvdata = snd_soc_platform_get_drvdata(soc_runtime->platform); - struct lpass_pcm_data *pcm_data = drvdata->private_data; + struct snd_pcm_runtime *rt = substream->runtime; + struct lpass_pcm_data *pcm_data = rt->private_data; struct lpass_variant *v = drvdata->variant; int ret, ch, dir = substream->stream;
@@ -246,7 +296,8 @@ static int lpass_platform_pcmops_trigger(struct snd_pcm_substream *substream, struct snd_soc_pcm_runtime *soc_runtime = substream->private_data; struct lpass_data *drvdata = snd_soc_platform_get_drvdata(soc_runtime->platform); - struct lpass_pcm_data *pcm_data = drvdata->private_data; + struct snd_pcm_runtime *rt = substream->runtime; + struct lpass_pcm_data *pcm_data = rt->private_data; struct lpass_variant *v = drvdata->variant; int ret, ch, dir = substream->stream;
@@ -319,7 +370,8 @@ static snd_pcm_uframes_t lpass_platform_pcmops_pointer( struct snd_soc_pcm_runtime *soc_runtime = substream->private_data; struct lpass_data *drvdata = snd_soc_platform_get_drvdata(soc_runtime->platform); - struct lpass_pcm_data *pcm_data = drvdata->private_data; + struct snd_pcm_runtime *rt = substream->runtime; + struct lpass_pcm_data *pcm_data = rt->private_data; struct lpass_variant *v = drvdata->variant; unsigned int base_addr, curr_addr; int ret, ch, dir = substream->stream; @@ -357,6 +409,7 @@ static int lpass_platform_pcmops_mmap(struct snd_pcm_substream *substream,
static const struct snd_pcm_ops lpass_platform_pcm_ops = { .open = lpass_platform_pcmops_open, + .close = lpass_platform_pcmops_close, .ioctl = snd_pcm_lib_ioctl, .hw_params = lpass_platform_pcmops_hw_params, .hw_free = lpass_platform_pcmops_hw_free, @@ -453,115 +506,45 @@ static int lpass_platform_pcm_new(struct snd_soc_pcm_runtime *soc_runtime) { struct snd_pcm *pcm = soc_runtime->pcm; struct snd_pcm_substream *psubstream, *csubstream; - struct snd_soc_dai *cpu_dai = soc_runtime->cpu_dai; - struct lpass_data *drvdata = - snd_soc_platform_get_drvdata(soc_runtime->platform); - struct lpass_variant *v = drvdata->variant; int ret = -EINVAL; - struct lpass_pcm_data *data; size_t size = lpass_platform_pcm_hardware.buffer_bytes_max;
- data = devm_kzalloc(soc_runtime->dev, sizeof(*data), GFP_KERNEL); - if (!data) - return -ENOMEM; - - data->i2s_port = cpu_dai->driver->id; - drvdata->private_data = data; - psubstream = pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream; if (psubstream) { - if (v->alloc_dma_channel) - data->dma_ch = v->alloc_dma_channel(drvdata, - SNDRV_PCM_STREAM_PLAYBACK); - - if (data->dma_ch < 0) - return data->dma_ch; - - drvdata->substream[data->dma_ch] = psubstream; - ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, soc_runtime->platform->dev, size, &psubstream->dma_buffer); - if (ret) - goto playback_alloc_err; - - ret = regmap_write(drvdata->lpaif_map, - LPAIF_RDMACTL_REG(v, data->dma_ch), 0); if (ret) { - dev_err(soc_runtime->dev, - "%s() error writing to rdmactl reg: %d\n", - __func__, ret); - goto capture_alloc_err; + dev_err(soc_runtime->dev, "Cannot allocate buffer(s)\n"); + return ret; } }
csubstream = pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream; if (csubstream) { - if (v->alloc_dma_channel) - data->dma_ch = v->alloc_dma_channel(drvdata, - SNDRV_PCM_STREAM_CAPTURE); - - if (data->dma_ch < 0) { - ret = data->dma_ch; - goto capture_alloc_err; - } - - drvdata->substream[data->dma_ch] = csubstream; - ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, soc_runtime->platform->dev, size, &csubstream->dma_buffer); - if (ret) - goto capture_alloc_err; - - ret = regmap_write(drvdata->lpaif_map, - LPAIF_WRDMACTL_REG(v, data->dma_ch), 0); if (ret) { - dev_err(soc_runtime->dev, - "%s() error writing to wrdmactl reg: %d\n", - __func__, ret); - goto capture_reg_err; + dev_err(soc_runtime->dev, "Cannot allocate buffer(s)\n"); + if (psubstream) + snd_dma_free_pages(&psubstream->dma_buffer); + return ret; } + }
return 0; - -capture_reg_err: - if (csubstream) - snd_dma_free_pages(&csubstream->dma_buffer); - -capture_alloc_err: - if (psubstream) - snd_dma_free_pages(&psubstream->dma_buffer); - - playback_alloc_err: - dev_err(soc_runtime->dev, "Cannot allocate buffer(s)\n"); - - return ret; }
static void lpass_platform_pcm_free(struct snd_pcm *pcm) { - struct snd_soc_pcm_runtime *rt; - struct lpass_data *drvdata; - struct lpass_pcm_data *data; - struct lpass_variant *v; struct snd_pcm_substream *substream; - int ch, i; + int i;
for (i = 0; i < ARRAY_SIZE(pcm->streams); i++) { substream = pcm->streams[i].substream; if (substream) { - rt = substream->private_data; - drvdata = snd_soc_platform_get_drvdata(rt->platform); - data = drvdata->private_data; - - ch = data->dma_ch; - v = drvdata->variant; - drvdata->substream[ch] = NULL; - if (v->free_dma_channel) - v->free_dma_channel(drvdata, ch); - snd_dma_free_pages(&substream->dma_buffer); substream->dma_buffer.area = NULL; substream->dma_buffer.addr = 0; diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h index 35b3cea..924971b 100644 --- a/sound/soc/qcom/lpass.h +++ b/sound/soc/qcom/lpass.h @@ -59,7 +59,6 @@ struct lpass_data { struct clk *pcnoc_mport_clk; struct clk *pcnoc_sway_clk;
- void *private_data; };
/* Vairant data per each SOC */
On Fri, Oct 28, 2016 at 04:32:19PM +0100, Srinivas Kandagatla wrote:
This patch fixes lpass-platform driver which was broken in v4.9-rc1. lpass_pcm_data data structure holds information specific to stream. Holding a single private pointer to it in global lpass_data will not work, because it would be overwritten by for each pcm instance.
This is a bug fix but you've made it depend on patch 1 which is just a refactoring and should therefore not be sent to Linus as a fix. Please don't do this, fix bugs first then do any cleanups and new functionality. That way we can send fixes to Linus without needing to pull in extra stuff.
On 28/10/16 19:26, Mark Brown wrote:
On Fri, Oct 28, 2016 at 04:32:19PM +0100, Srinivas Kandagatla wrote:
This patch fixes lpass-platform driver which was broken in v4.9-rc1. lpass_pcm_data data structure holds information specific to stream. Holding a single private pointer to it in global lpass_data will not work, because it would be overwritten by for each pcm instance.
This is a bug fix but you've made it depend on patch 1 which is just a refactoring and should therefore not be sent to Linus as a fix. Please don't do this, fix bugs first then do any cleanups and new functionality. That way we can send fixes to Linus without needing to pull in extra stuff.
Sorry about that, I will resend the fix patch without any dependencies.
Thanks, srini
On Fri, Oct 28, 2016 at 04:32:19PM +0100, Srinivas Kandagatla wrote:
This patch fixes lpass-platform driver which was broken in v4.9-rc1. lpass_pcm_data data structure holds information specific to stream. Holding a single private pointer to it in global lpass_data will not work, because it would be overwritten by for each pcm instance.
This code was breaking playback when we have both playback and capture pcm streams, as playback settings are over written by capture settings.
Fix this by moving channel allocation logic out of pcm_new to pcm_open so that we can store the stream specific information private_data of snd_pcm_runtime.
Fixes: 6adcbdcd4b6e ("ASoC: lpass-platform: don't use snd_soc_pcm_set_drvdata()") Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org
sound/soc/qcom/lpass-platform.c | 151 ++++++++++++++++++---------------------- sound/soc/qcom/lpass.h | 1 - 2 files changed, 67 insertions(+), 85 deletions(-)
After you address Mark's comments:
Acked-by: Kenneth Westfield kwestfie@codeaurora.org
This patch adds module licence to lpass-cpu driver, without this patch lpass-cpu module would taint with below error:
snd_soc_lpass_cpu: module license 'unspecified' taints kernel. Disabling lock debugging due to kernel taint snd_soc_lpass_cpu: Unknown symbol regmap_write (err 0) snd_soc_lpass_cpu: Unknown symbol devm_kmalloc (err 0) ...
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org Acked-by: Kenneth Westfield kwestfie@codeaurora.org --- sound/soc/qcom/lpass-cpu.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c index 3cde9fb..eff3f9a 100644 --- a/sound/soc/qcom/lpass-cpu.c +++ b/sound/soc/qcom/lpass-cpu.c @@ -586,3 +586,6 @@ int asoc_qcom_lpass_cpu_platform_remove(struct platform_device *pdev) return 0; } EXPORT_SYMBOL_GPL(asoc_qcom_lpass_cpu_platform_remove); + +MODULE_DESCRIPTION("QTi LPASS CPU Driver"); +MODULE_LICENSE("GPL v2");
participants (3)
-
Kenneth Westfield
-
Mark Brown
-
Srinivas Kandagatla