2021-10-01 12:50:14

by Srinivasa Rao Mandadapu

[permalink] [raw]
Subject: [PATCH 0/8] Add support for audio on SC7280 based targets

This patch set is to add support for Audio over wcd codec,
digital mics, through digital codecs and without ADSP.
This patch set depends on:
-- https://patchwork.kernel.org/project/alsa-devel/list/?series=549577
-- https://patchwork.kernel.org/project/alsa-devel/list/?series=548765
-- https://patchwork.kernel.org/project/alsa-devel/list/?series=543829

Srinivasa Rao Mandadapu (8):
ASoC: qcom: Move lpass_pcm_data structure to lpass header
ASoC: qcom: lpass: Add dma fields for codec dma lpass interface
ASoC: qcom: Add register definition for codec rddma and wrdma
ASoC: qcom: Add lapss CPU driver for codec dma control
ASoC: qcom: Add support for codec dma driver
ASoC: dt-bindings: Add SC7280 sound card bindings
ASoC: qcom: lpass-sc7280: Add platform driver for lpass audio
ASoc: qcom: lpass: Add suspend and resume for sc7280 platform

.../devicetree/bindings/sound/qcom,lpass-cpu.yaml | 69 ++-
sound/soc/qcom/lpass-cdc-dma.c | 263 +++++++++
sound/soc/qcom/lpass-cpu.c | 334 +++++++++++-
sound/soc/qcom/lpass-lpaif-reg.h | 103 +++-
sound/soc/qcom/lpass-platform.c | 504 +++++++++++++++--
sound/soc/qcom/lpass-sc7280.c | 602 +++++++++++++++++++++
sound/soc/qcom/lpass.h | 156 ++++++
7 files changed, 1974 insertions(+), 57 deletions(-)
create mode 100644 sound/soc/qcom/lpass-cdc-dma.c
create mode 100644 sound/soc/qcom/lpass-sc7280.c

--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.


2021-10-01 12:50:53

by Srinivasa Rao Mandadapu

[permalink] [raw]
Subject: [PATCH 1/8] ASoC: qcom: Move lpass_pcm_data structure to lpass header

Declare lpass_pcm_data structure in lpass header file instead of
platform source file to make common use of it by other drivers

Signed-off-by: Venkata Prasad Potturu <[email protected]>
Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
---
sound/soc/qcom/lpass-platform.c | 5 -----
sound/soc/qcom/lpass.h | 5 +++++
2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c
index f9df76d..a000041 100644
--- a/sound/soc/qcom/lpass-platform.c
+++ b/sound/soc/qcom/lpass-platform.c
@@ -18,11 +18,6 @@

#define DRV_NAME "lpass-platform"

-struct lpass_pcm_data {
- int dma_ch;
- int i2s_port;
-};
-
#define LPASS_PLATFORM_BUFFER_SIZE (24 * 2 * 1024)
#define LPASS_PLATFORM_PERIODS 2

diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
index 67ef497..63aaa6f 100644
--- a/sound/soc/qcom/lpass.h
+++ b/sound/soc/qcom/lpass.h
@@ -256,6 +256,11 @@ struct lpass_variant {
int num_clks;
};

+struct lpass_pcm_data {
+ int dma_ch;
+ int i2s_port;
+};
+
/* register the platform driver from the CPU DAI driver */
int asoc_qcom_lpass_platform_register(struct platform_device *);
int asoc_qcom_lpass_cpu_platform_remove(struct platform_device *pdev);
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

2021-10-01 12:57:59

by Srinivasa Rao Mandadapu

[permalink] [raw]
Subject: [PATCH 4/8] ASoC: qcom: Add lapss CPU driver for codec dma control

Add lpass cpu driver to support audio over codec dma for
ADSP bypass usecase.

Signed-off-by: Venkata Prasad Potturu <[email protected]>
Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
---
sound/soc/qcom/lpass-cdc-dma.c | 263 +++++++++++++++++++++++++++++++++++++++++
sound/soc/qcom/lpass.h | 1 +
2 files changed, 264 insertions(+)
create mode 100644 sound/soc/qcom/lpass-cdc-dma.c

diff --git a/sound/soc/qcom/lpass-cdc-dma.c b/sound/soc/qcom/lpass-cdc-dma.c
new file mode 100644
index 0000000..56b3791
--- /dev/null
+++ b/sound/soc/qcom/lpass-cdc-dma.c
@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021 The Linux Foundation. All rights reserved.
+ *
+ * lpass-cdc-dma.c -- ALSA SoC WCD -CPU DAI driver for QTi LPASS WCD
+ */
+
+#include <linux/module.h>
+#include <sound/soc.h>
+#include <sound/soc-dai.h>
+
+#include "lpass-lpaif-reg.h"
+#include "lpass.h"
+
+static int __lpass_platform_codec_intf_init(struct snd_soc_dai *dai,
+ struct snd_pcm_substream *substream)
+{
+ struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
+ struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
+ struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
+ struct snd_pcm_runtime *rt = substream->runtime;
+ struct lpass_pcm_data *pcm_data = rt->private_data;
+ struct lpass_variant *v = drvdata->variant;
+ struct lpaif_dmactl *dmactl;
+ struct regmap *map;
+ int dir = substream->stream;
+ int ret, id;
+ unsigned int dai_id = cpu_dai->driver->id;
+
+ if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
+ dmactl = drvdata->rxtx_rd_dmactl;
+ map = drvdata->rxtx_lpaif_map;
+ id = pcm_data->dma_ch;
+ } else {
+ if (dai_id == LPASS_CDC_DMA_TX3) {
+ dmactl = drvdata->rxtx_wr_dmactl;
+ map = drvdata->rxtx_lpaif_map;
+ id = pcm_data->dma_ch - v->rxtx_wrdma_channel_start;
+ } else if (dai_id == LPASS_CDC_DMA_VA_TX0) {
+ dmactl = drvdata->va_wr_dmactl;
+ map = drvdata->va_lpaif_map;
+ id = pcm_data->dma_ch - v->va_wrdma_channel_start;
+ }
+ }
+
+ if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
+ ret = regmap_fields_write(dmactl->codec_intf, id, LPASS_CDC_DMA_RX0_INTERFACE);
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to rdmactl reg: %d\n", ret);
+ return ret;
+ }
+ ret = regmap_fields_write(dmactl->codec_fs_sel, id, 0x0);
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to rdmactl reg: %d\n", ret);
+ return ret;
+ }
+ ret = regmap_fields_write(dmactl->codec_fs_delay, id, 0x0);
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to rdmactl codec_fs_delay reg field: %d\n", ret);
+ return ret;
+ }
+ ret = regmap_fields_write(dmactl->codec_pack, id, 0x1);
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to rdmactl codec_pack reg field: %d\n", ret);
+ return ret;
+ }
+ ret = regmap_fields_write(dmactl->codec_enable, id, LPAIF_DMACTL_ENABLE_ON);
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to rdmactl reg: %d\n", ret);
+ return ret;
+ }
+
+ } else {
+ ret = regmap_fields_write(dmactl->codec_intf, id, LPASS_CDC_DMA_INTERFACE(dai_id));
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to wrdmactl codec_intf reg field: %d\n", ret);
+ return ret;
+ }
+ ret = regmap_fields_write(dmactl->codec_fs_sel, id, 0x0);
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to wrdmactl codec_fs_sel reg field: %d\n", ret);
+ return ret;
+ }
+ ret = regmap_fields_write(dmactl->codec_fs_delay, id, 0x0);
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to wrdmactl codec_fs_delay reg field: %d\n", ret);
+ return ret;
+ }
+ ret = regmap_fields_write(dmactl->codec_pack, id, 0x1);
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to wrdmactl codec_pack reg field: %d\n", ret);
+ return ret;
+ }
+ ret = regmap_fields_write(dmactl->codec_enable, id, LPAIF_DMACTL_ENABLE_ON);
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to wrdmactl codec_enable reg field: %d\n", ret);
+ return ret;
+ }
+ }
+ return 0;
+}
+
+static int lpass_wcd_daiops_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
+ int ret, i;
+
+ for (i = 0; i < drvdata->cdc_num_clks; i++) {
+ ret = clk_prepare_enable(drvdata->cdc_dma_clks[i]);
+ if (ret) {
+ dev_err(dai->dev, "error in enabling cdc dma clks: %d\n", ret);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static void lpass_wcd_daiops_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ int i;
+ struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
+
+ for (i = 0; i < drvdata->cdc_num_clks; i++)
+ clk_disable_unprepare(drvdata->cdc_dma_clks[i]);
+}
+
+static int lpass_wcd_daiops_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
+{
+ struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
+ struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
+ struct lpaif_dmactl *dmactl;
+ struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
+ struct snd_pcm_runtime *rt = substream->runtime;
+ struct lpass_pcm_data *pcm_data = rt->private_data;
+ struct lpass_variant *v = drvdata->variant;
+ struct regmap *map;
+ int dir = substream->stream;
+ unsigned int ret, regval;
+ unsigned int channels = params_channels(params);
+ int id;
+ unsigned int dai_id = cpu_dai->driver->id;
+
+ if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
+ dmactl = drvdata->rxtx_rd_dmactl;
+ map = drvdata->rxtx_lpaif_map;
+ id = pcm_data->dma_ch;
+ } else {
+ if (dai_id == LPASS_CDC_DMA_TX3) {
+ dmactl = drvdata->rxtx_wr_dmactl;
+ map = drvdata->rxtx_lpaif_map;
+ id = pcm_data->dma_ch - v->rxtx_wrdma_channel_start;
+ } else if (dai_id == LPASS_CDC_DMA_VA_TX0) {
+ dmactl = drvdata->va_wr_dmactl;
+ map = drvdata->va_lpaif_map;
+ id = pcm_data->dma_ch - v->va_wrdma_channel_start;
+ }
+ }
+
+ switch (channels) {
+ case 1:
+ regval = LPASS_CDC_DMA_INTF_ONE_CHANNEL;
+ break;
+ case 2:
+ regval = LPASS_CDC_DMA_INTF_TWO_CHANNEL;
+ break;
+ case 4:
+ regval = LPASS_CDC_DMA_INTF_FOUR_CHANNEL;
+ break;
+ case 6:
+ regval = LPASS_CDC_DMA_INTF_SIX_CHANNEL;
+ break;
+ case 8:
+ regval = LPASS_CDC_DMA_INTF_EIGHT_CHANNEL;
+ break;
+ default:
+ dev_err(soc_runtime->dev, "invalid PCM config\n");
+ return -EINVAL;
+ }
+
+ ret = regmap_fields_write(dmactl->codec_channel, id, regval);
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to rdmactl codec_channel reg field: %d\n", ret);
+ return ret;
+ }
+ return ret;
+}
+
+static int lpass_wcd_daiops_trigger(struct snd_pcm_substream *substream,
+ int cmd, struct snd_soc_dai *dai)
+{
+ struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
+ struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
+ struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
+ struct snd_pcm_runtime *rt = substream->runtime;
+ struct lpass_pcm_data *pcm_data = rt->private_data;
+ struct lpass_variant *v = drvdata->variant;
+ int dir = substream->stream;
+ struct lpaif_dmactl *dmactl;
+ struct regmap *map;
+ unsigned int dai_id = cpu_dai->driver->id;
+ int ret = 0, id;
+
+ if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
+ dmactl = drvdata->rxtx_rd_dmactl;
+ map = drvdata->rxtx_lpaif_map;
+ id = pcm_data->dma_ch;
+ } else {
+ if (dai_id == LPASS_CDC_DMA_TX3) {
+ dmactl = drvdata->rxtx_wr_dmactl;
+ map = drvdata->rxtx_lpaif_map;
+ id = pcm_data->dma_ch - v->rxtx_wrdma_channel_start;
+ } else if (dai_id == LPASS_CDC_DMA_VA_TX0) {
+ dmactl = drvdata->va_wr_dmactl;
+ map = drvdata->va_lpaif_map;
+ id = pcm_data->dma_ch - v->va_wrdma_channel_start;
+ }
+ }
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ __lpass_platform_codec_intf_init(dai, substream);
+ break;
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ ret = regmap_fields_write(dmactl->codec_enable, id, LPAIF_DMACTL_ENABLE_OFF);
+ if (ret) {
+ dev_err(soc_runtime->dev,
+ "error writing to rdmactl reg: %d\n", ret);
+ return ret;
+ }
+
+ break;
+ }
+ return ret;
+}
+
+const struct snd_soc_dai_ops asoc_qcom_lpass_wcd_dai_ops = {
+ .startup = lpass_wcd_daiops_startup,
+ .shutdown = lpass_wcd_daiops_shutdown,
+ .hw_params = lpass_wcd_daiops_hw_params,
+ .trigger = lpass_wcd_daiops_trigger,
+};
+EXPORT_SYMBOL_GPL(asoc_qcom_lpass_wcd_dai_ops);
+
+MODULE_DESCRIPTION("QTi LPASS CDC DMA Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
index 058b42d..e0ea698 100644
--- a/sound/soc/qcom/lpass.h
+++ b/sound/soc/qcom/lpass.h
@@ -418,5 +418,6 @@ int asoc_qcom_lpass_cpu_dai_probe(struct snd_soc_dai *dai);
extern const struct snd_soc_dai_ops asoc_qcom_lpass_cpu_dai_ops;
int lpass_cpu_pcm_new(struct snd_soc_pcm_runtime *rtd,
struct snd_soc_dai *dai);
+extern const struct snd_soc_dai_ops asoc_qcom_lpass_wcd_dai_ops;

#endif /* __LPASS_H__ */
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

2021-10-01 16:09:48

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH 4/8] ASoC: qcom: Add lapss CPU driver for codec dma control

On Fri, Oct 01, 2021 at 04:51:28PM +0530, Srinivasa Rao Mandadapu wrote:
> Subject: ASoC: qcom: Add lapss CPU driver for codec dma control

nit: s/lapss/lpass/

>
> Add lpass cpu driver to support audio over codec dma for
> ADSP bypass usecase.
>
> Signed-off-by: Venkata Prasad Potturu <[email protected]>
> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
> ---
> sound/soc/qcom/lpass-cdc-dma.c | 263 +++++++++++++++++++++++++++++++++++++++++
> sound/soc/qcom/lpass.h | 1 +
> 2 files changed, 264 insertions(+)
> create mode 100644 sound/soc/qcom/lpass-cdc-dma.c
>
> diff --git a/sound/soc/qcom/lpass-cdc-dma.c b/sound/soc/qcom/lpass-cdc-dma.c
> new file mode 100644
> index 0000000..56b3791
> --- /dev/null
> +++ b/sound/soc/qcom/lpass-cdc-dma.c
> @@ -0,0 +1,263 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021 The Linux Foundation. All rights reserved.
> + *
> + * lpass-cdc-dma.c -- ALSA SoC WCD -CPU DAI driver for QTi LPASS WCD
> + */
> +
> +#include <linux/module.h>
> +#include <sound/soc.h>
> +#include <sound/soc-dai.h>
> +
> +#include "lpass-lpaif-reg.h"
> +#include "lpass.h"
> +
> +static int __lpass_platform_codec_intf_init(struct snd_soc_dai *dai,
> + struct snd_pcm_substream *substream)
> +{
> + struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
> + struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
> + struct snd_pcm_runtime *rt = substream->runtime;
> + struct lpass_pcm_data *pcm_data = rt->private_data;
> + struct lpass_variant *v = drvdata->variant;
> + struct lpaif_dmactl *dmactl;
> + struct regmap *map;
> + int dir = substream->stream;
> + int ret, id;
> + unsigned int dai_id = cpu_dai->driver->id;
> +
> + if (dir == SNDRV_PCM_STREAM_PLAYBACK) {

s/ == / == /

> + dmactl = drvdata->rxtx_rd_dmactl;
> + map = drvdata->rxtx_lpaif_map;
> + id = pcm_data->dma_ch;
> + } else {
> + if (dai_id == LPASS_CDC_DMA_TX3) {
> + dmactl = drvdata->rxtx_wr_dmactl;
> + map = drvdata->rxtx_lpaif_map;
> + id = pcm_data->dma_ch - v->rxtx_wrdma_channel_start;
> + } else if (dai_id == LPASS_CDC_DMA_VA_TX0) {
> + dmactl = drvdata->va_wr_dmactl;
> + map = drvdata->va_lpaif_map;
> + id = pcm_data->dma_ch - v->va_wrdma_channel_start;
> + }
> + }

'map' is assigned here, but not used in the rest of the function, drop it.

> +
> + if (dir == SNDRV_PCM_STREAM_PLAYBACK) {

s/ == / == /

> + ret = regmap_fields_write(dmactl->codec_intf, id, LPASS_CDC_DMA_RX0_INTERFACE);
> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to rdmactl reg: %d\n", ret);

Some of the logs print the register name, others don't, why not log it
always?

> + return ret;
> + }
> + ret = regmap_fields_write(dmactl->codec_fs_sel, id, 0x0);

fix indentation

> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to rdmactl reg: %d\n", ret);
> + return ret;
> + }
> + ret = regmap_fields_write(dmactl->codec_fs_delay, id, 0x0);
> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to rdmactl codec_fs_delay reg field: %d\n", ret);
> + return ret;
> + }
> + ret = regmap_fields_write(dmactl->codec_pack, id, 0x1);
> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to rdmactl codec_pack reg field: %d\n", ret);
> + return ret;
> + }
> + ret = regmap_fields_write(dmactl->codec_enable, id, LPAIF_DMACTL_ENABLE_ON);
> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to rdmactl reg: %d\n", ret);
> + return ret;
> + }
> +

Remove empty line

> + } else {
> + ret = regmap_fields_write(dmactl->codec_intf, id, LPASS_CDC_DMA_INTERFACE(dai_id));
> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to wrdmactl codec_intf reg field: %d\n", ret);
> + return ret;
> + }
> + ret = regmap_fields_write(dmactl->codec_fs_sel, id, 0x0);

fix indentation

> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to wrdmactl codec_fs_sel reg field: %d\n", ret);
> + return ret;
> + }
> + ret = regmap_fields_write(dmactl->codec_fs_delay, id, 0x0);
> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to wrdmactl codec_fs_delay reg field: %d\n", ret);
> + return ret;
> + }
> + ret = regmap_fields_write(dmactl->codec_pack, id, 0x1);
> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to wrdmactl codec_pack reg field: %d\n", ret);
> + return ret;
> + }
> + ret = regmap_fields_write(dmactl->codec_enable, id, LPAIF_DMACTL_ENABLE_ON);
> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to wrdmactl codec_enable reg field: %d\n", ret);
> + return ret;
> + }

The last four register writes are exactly the same in both branches, only the
logging distinguishes between 'rdmactl' and 'wrdmactl'. Those should be in the
main branch, if you really need to distinguish between 'rdmactl' and 'wrdmactl'
in the logging you can assign a string pointer in the 'if (dir == SNDRV_PCM_STREAM_PLAYBACK)'
branches.

> + }
> + return 0;
> +}
> +
> +static int lpass_wcd_daiops_startup(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
> + int ret, i;
> +
> + for (i = 0; i < drvdata->cdc_num_clks; i++) {
> + ret = clk_prepare_enable(drvdata->cdc_dma_clks[i]);
> + if (ret) {
> + dev_err(dai->dev, "error in enabling cdc dma clks: %d\n", ret);
> + return ret;

Disable clocks that have been enabled previously?

> + }
> + }
> +
> + return 0;
> +}
> +
> +static void lpass_wcd_daiops_shutdown(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + int i;
> + struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
> +
> + for (i = 0; i < drvdata->cdc_num_clks; i++)
> + clk_disable_unprepare(drvdata->cdc_dma_clks[i]);
> +}
> +
> +static int lpass_wcd_daiops_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
> +{
> + struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
> + struct lpaif_dmactl *dmactl;
> + struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
> + struct snd_pcm_runtime *rt = substream->runtime;
> + struct lpass_pcm_data *pcm_data = rt->private_data;
> + struct lpass_variant *v = drvdata->variant;
> + struct regmap *map;
> + int dir = substream->stream;
> + unsigned int ret, regval;
> + unsigned int channels = params_channels(params);
> + int id;
> + unsigned int dai_id = cpu_dai->driver->id;
> +
> + if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
> + dmactl = drvdata->rxtx_rd_dmactl;
> + map = drvdata->rxtx_lpaif_map;
> + id = pcm_data->dma_ch;
> + } else {
> + if (dai_id == LPASS_CDC_DMA_TX3) {
> + dmactl = drvdata->rxtx_wr_dmactl;
> + map = drvdata->rxtx_lpaif_map;
> + id = pcm_data->dma_ch - v->rxtx_wrdma_channel_start;
> + } else if (dai_id == LPASS_CDC_DMA_VA_TX0) {
> + dmactl = drvdata->va_wr_dmactl;
> + map = drvdata->va_lpaif_map;
> + id = pcm_data->dma_ch - v->va_wrdma_channel_start;
> + }
> + }

The exact same code block is used 3 times in this driver. Add a helper with
'substream' and 'dai' as parameters, which determines 'dmactl and 'id'.
That would also make the variable declaration/assignment section of these
functions signifcantly shorter.

> +
> + switch (channels) {
> + case 1:
> + regval = LPASS_CDC_DMA_INTF_ONE_CHANNEL;
> + break;
> + case 2:
> + regval = LPASS_CDC_DMA_INTF_TWO_CHANNEL;
> + break;
> + case 4:
> + regval = LPASS_CDC_DMA_INTF_FOUR_CHANNEL;
> + break;
> + case 6:
> + regval = LPASS_CDC_DMA_INTF_SIX_CHANNEL;
> + break;
> + case 8:
> + regval = LPASS_CDC_DMA_INTF_EIGHT_CHANNEL;
> + break;
> + default:
> + dev_err(soc_runtime->dev, "invalid PCM config\n");
> + return -EINVAL;
> + }
> +
> + ret = regmap_fields_write(dmactl->codec_channel, id, regval);
> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to rdmactl codec_channel reg field: %d\n", ret);

not necessarily 'rdmactl'.

> + return ret;
> + }
> + return ret;

return 0

> +}
> +
> +static int lpass_wcd_daiops_trigger(struct snd_pcm_substream *substream,
> + int cmd, struct snd_soc_dai *dai)
> +{
> + struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
> + struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
> + struct snd_pcm_runtime *rt = substream->runtime;
> + struct lpass_pcm_data *pcm_data = rt->private_data;
> + struct lpass_variant *v = drvdata->variant;
> + int dir = substream->stream;
> + struct lpaif_dmactl *dmactl;
> + struct regmap *map;
> + unsigned int dai_id = cpu_dai->driver->id;
> + int ret = 0, id;
> +
> + if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
> + dmactl = drvdata->rxtx_rd_dmactl;
> + map = drvdata->rxtx_lpaif_map;
> + id = pcm_data->dma_ch;
> + } else {
> + if (dai_id == LPASS_CDC_DMA_TX3) {
> + dmactl = drvdata->rxtx_wr_dmactl;
> + map = drvdata->rxtx_lpaif_map;
> + id = pcm_data->dma_ch - v->rxtx_wrdma_channel_start;
> + } else if (dai_id == LPASS_CDC_DMA_VA_TX0) {
> + dmactl = drvdata->va_wr_dmactl;
> + map = drvdata->va_lpaif_map;
> + id = pcm_data->dma_ch - v->va_wrdma_channel_start;
> + }
> + }

see above

> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_START:
> + case SNDRV_PCM_TRIGGER_RESUME:
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + __lpass_platform_codec_intf_init(dai, substream);
> + break;
> + case SNDRV_PCM_TRIGGER_STOP:
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + ret = regmap_fields_write(dmactl->codec_enable, id, LPAIF_DMACTL_ENABLE_OFF);
> + if (ret) {
> + dev_err(soc_runtime->dev,
> + "error writing to rdmactl reg: %d\n", ret);

not necessarily rdmactl, better log that it's the codec enable reg.

> + return ret;
> + }
> +
> + break;
> + }
> + return ret;
> +}
> +
> +const struct snd_soc_dai_ops asoc_qcom_lpass_wcd_dai_ops = {
> + .startup = lpass_wcd_daiops_startup,
> + .shutdown = lpass_wcd_daiops_shutdown,
> + .hw_params = lpass_wcd_daiops_hw_params,
> + .trigger = lpass_wcd_daiops_trigger,
> +};
> +EXPORT_SYMBOL_GPL(asoc_qcom_lpass_wcd_dai_ops);
> +
> +MODULE_DESCRIPTION("QTi LPASS CDC DMA Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
> index 058b42d..e0ea698 100644
> --- a/sound/soc/qcom/lpass.h
> +++ b/sound/soc/qcom/lpass.h
> @@ -418,5 +418,6 @@ int asoc_qcom_lpass_cpu_dai_probe(struct snd_soc_dai *dai);
> extern const struct snd_soc_dai_ops asoc_qcom_lpass_cpu_dai_ops;
> int lpass_cpu_pcm_new(struct snd_soc_pcm_runtime *rtd,
> struct snd_soc_dai *dai);
> +extern const struct snd_soc_dai_ops asoc_qcom_lpass_wcd_dai_ops;
>
> #endif /* __LPASS_H__ */
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>

2021-10-04 17:55:43

by Srinivasa Rao Mandadapu

[permalink] [raw]
Subject: Re: [PATCH 4/8] ASoC: qcom: Add lapss CPU driver for codec dma control


On 10/1/2021 9:22 PM, Matthias Kaehlcke wrote:
Thanks for Your time Matthias!!!
> On Fri, Oct 01, 2021 at 04:51:28PM +0530, Srinivasa Rao Mandadapu wrote:
>> Subject: ASoC: qcom: Add lapss CPU driver for codec dma control
> nit: s/lapss/lpass/
Okay. Will change.
>> Add lpass cpu driver to support audio over codec dma for
>> ADSP bypass usecase.
>>
>> Signed-off-by: Venkata Prasad Potturu <[email protected]>
>> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
>> ---
>> sound/soc/qcom/lpass-cdc-dma.c | 263 +++++++++++++++++++++++++++++++++++++++++
>> sound/soc/qcom/lpass.h | 1 +
>> 2 files changed, 264 insertions(+)
>> create mode 100644 sound/soc/qcom/lpass-cdc-dma.c
>>
>> diff --git a/sound/soc/qcom/lpass-cdc-dma.c b/sound/soc/qcom/lpass-cdc-dma.c
>> new file mode 100644
>> index 0000000..56b3791
>> --- /dev/null
>> +++ b/sound/soc/qcom/lpass-cdc-dma.c
>> @@ -0,0 +1,263 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2021 The Linux Foundation. All rights reserved.
>> + *
>> + * lpass-cdc-dma.c -- ALSA SoC WCD -CPU DAI driver for QTi LPASS WCD
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <sound/soc.h>
>> +#include <sound/soc-dai.h>
>> +
>> +#include "lpass-lpaif-reg.h"
>> +#include "lpass.h"
>> +
>> +static int __lpass_platform_codec_intf_init(struct snd_soc_dai *dai,
>> + struct snd_pcm_substream *substream)
>> +{
>> + struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
>> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
>> + struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
>> + struct snd_pcm_runtime *rt = substream->runtime;
>> + struct lpass_pcm_data *pcm_data = rt->private_data;
>> + struct lpass_variant *v = drvdata->variant;
>> + struct lpaif_dmactl *dmactl;
>> + struct regmap *map;
>> + int dir = substream->stream;
>> + int ret, id;
>> + unsigned int dai_id = cpu_dai->driver->id;
>> +
>> + if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
> s/ == / == /
Okay. Will change it.
>> + dmactl = drvdata->rxtx_rd_dmactl;
>> + map = drvdata->rxtx_lpaif_map;
>> + id = pcm_data->dma_ch;
>> + } else {
>> + if (dai_id == LPASS_CDC_DMA_TX3) {
>> + dmactl = drvdata->rxtx_wr_dmactl;
>> + map = drvdata->rxtx_lpaif_map;
>> + id = pcm_data->dma_ch - v->rxtx_wrdma_channel_start;
>> + } else if (dai_id == LPASS_CDC_DMA_VA_TX0) {
>> + dmactl = drvdata->va_wr_dmactl;
>> + map = drvdata->va_lpaif_map;
>> + id = pcm_data->dma_ch - v->va_wrdma_channel_start;
>> + }
>> + }
> 'map' is assigned here, but not used in the rest of the function, drop it.
Okay. will remove it.
>> +
>> + if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
> s/ == / == /
Okay. Will change it.
>> + ret = regmap_fields_write(dmactl->codec_intf, id, LPASS_CDC_DMA_RX0_INTERFACE);
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to rdmactl reg: %d\n", ret);
> Some of the logs print the register name, others don't, why not log it
> always?
>
>> + return ret;
>> + }
>> + ret = regmap_fields_write(dmactl->codec_fs_sel, id, 0x0);
> fix indentation
Okay. will change it.
>
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to rdmactl reg: %d\n", ret);
>> + return ret;
>> + }
>> + ret = regmap_fields_write(dmactl->codec_fs_delay, id, 0x0);
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to rdmactl codec_fs_delay reg field: %d\n", ret);
>> + return ret;
>> + }
>> + ret = regmap_fields_write(dmactl->codec_pack, id, 0x1);
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to rdmactl codec_pack reg field: %d\n", ret);
>> + return ret;
>> + }
>> + ret = regmap_fields_write(dmactl->codec_enable, id, LPAIF_DMACTL_ENABLE_ON);
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to rdmactl reg: %d\n", ret);
>> + return ret;
>> + }
>> +
> Remove empty line
Okay. will remove it.
>
>> + } else {
>> + ret = regmap_fields_write(dmactl->codec_intf, id, LPASS_CDC_DMA_INTERFACE(dai_id));
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to wrdmactl codec_intf reg field: %d\n", ret);
>> + return ret;
>> + }
>> + ret = regmap_fields_write(dmactl->codec_fs_sel, id, 0x0);
> fix indentation
Okay. Will change it.
>
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to wrdmactl codec_fs_sel reg field: %d\n", ret);
>> + return ret;
>> + }
>> + ret = regmap_fields_write(dmactl->codec_fs_delay, id, 0x0);
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to wrdmactl codec_fs_delay reg field: %d\n", ret);
>> + return ret;
>> + }
>> + ret = regmap_fields_write(dmactl->codec_pack, id, 0x1);
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to wrdmactl codec_pack reg field: %d\n", ret);
>> + return ret;
>> + }
>> + ret = regmap_fields_write(dmactl->codec_enable, id, LPAIF_DMACTL_ENABLE_ON);
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to wrdmactl codec_enable reg field: %d\n", ret);
>> + return ret;
>> + }
> The last four register writes are exactly the same in both branches, only the
> logging distinguishes between 'rdmactl' and 'wrdmactl'. Those should be in the
> main branch, if you really need to distinguish between 'rdmactl' and 'wrdmactl'
> in the logging you can assign a string pointer in the 'if (dir == SNDRV_PCM_STREAM_PLAYBACK)'
> branches.
Okay. Will optimize code. I don't think rdmactl/wrdmactl is distinguish
required.
>> + }
>> + return 0;
>> +}
>> +
>> +static int lpass_wcd_daiops_startup(struct snd_pcm_substream *substream,
>> + struct snd_soc_dai *dai)
>> +{
>> + struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
>> + int ret, i;
>> +
>> + for (i = 0; i < drvdata->cdc_num_clks; i++) {
>> + ret = clk_prepare_enable(drvdata->cdc_dma_clks[i]);
>> + if (ret) {
>> + dev_err(dai->dev, "error in enabling cdc dma clks: %d\n", ret);
>> + return ret;
> Disable clocks that have been enabled previously?
Okay. will use bulk clock API here.
>
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void lpass_wcd_daiops_shutdown(struct snd_pcm_substream *substream,
>> + struct snd_soc_dai *dai)
>> +{
>> + int i;
>> + struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
>> +
>> + for (i = 0; i < drvdata->cdc_num_clks; i++)
>> + clk_disable_unprepare(drvdata->cdc_dma_clks[i]);
>> +}
>> +
>> +static int lpass_wcd_daiops_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
>> +{
>> + struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
>> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
>> + struct lpaif_dmactl *dmactl;
>> + struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
>> + struct snd_pcm_runtime *rt = substream->runtime;
>> + struct lpass_pcm_data *pcm_data = rt->private_data;
>> + struct lpass_variant *v = drvdata->variant;
>> + struct regmap *map;
>> + int dir = substream->stream;
>> + unsigned int ret, regval;
>> + unsigned int channels = params_channels(params);
>> + int id;
>> + unsigned int dai_id = cpu_dai->driver->id;
>> +
>> + if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
>> + dmactl = drvdata->rxtx_rd_dmactl;
>> + map = drvdata->rxtx_lpaif_map;
>> + id = pcm_data->dma_ch;
>> + } else {
>> + if (dai_id == LPASS_CDC_DMA_TX3) {
>> + dmactl = drvdata->rxtx_wr_dmactl;
>> + map = drvdata->rxtx_lpaif_map;
>> + id = pcm_data->dma_ch - v->rxtx_wrdma_channel_start;
>> + } else if (dai_id == LPASS_CDC_DMA_VA_TX0) {
>> + dmactl = drvdata->va_wr_dmactl;
>> + map = drvdata->va_lpaif_map;
>> + id = pcm_data->dma_ch - v->va_wrdma_channel_start;
>> + }
>> + }
> The exact same code block is used 3 times in this driver. Add a helper with
> 'substream' and 'dai' as parameters, which determines 'dmactl and 'id'.
> That would also make the variable declaration/assignment section of these
> functions signifcantly shorter.
Okay. will optimize it.
>> +
>> + switch (channels) {
>> + case 1:
>> + regval = LPASS_CDC_DMA_INTF_ONE_CHANNEL;
>> + break;
>> + case 2:
>> + regval = LPASS_CDC_DMA_INTF_TWO_CHANNEL;
>> + break;
>> + case 4:
>> + regval = LPASS_CDC_DMA_INTF_FOUR_CHANNEL;
>> + break;
>> + case 6:
>> + regval = LPASS_CDC_DMA_INTF_SIX_CHANNEL;
>> + break;
>> + case 8:
>> + regval = LPASS_CDC_DMA_INTF_EIGHT_CHANNEL;
>> + break;
>> + default:
>> + dev_err(soc_runtime->dev, "invalid PCM config\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = regmap_fields_write(dmactl->codec_channel, id, regval);
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to rdmactl codec_channel reg field: %d\n", ret);
> not necessarily 'rdmactl'.
Okay. will change it.
>> + return ret;
>> + }
>> + return ret;
> return 0
>
>> +}
>> +
>> +static int lpass_wcd_daiops_trigger(struct snd_pcm_substream *substream,
>> + int cmd, struct snd_soc_dai *dai)
>> +{
>> + struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
>> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
>> + struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
>> + struct snd_pcm_runtime *rt = substream->runtime;
>> + struct lpass_pcm_data *pcm_data = rt->private_data;
>> + struct lpass_variant *v = drvdata->variant;
>> + int dir = substream->stream;
>> + struct lpaif_dmactl *dmactl;
>> + struct regmap *map;
>> + unsigned int dai_id = cpu_dai->driver->id;
>> + int ret = 0, id;
>> +
>> + if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
>> + dmactl = drvdata->rxtx_rd_dmactl;
>> + map = drvdata->rxtx_lpaif_map;
>> + id = pcm_data->dma_ch;
>> + } else {
>> + if (dai_id == LPASS_CDC_DMA_TX3) {
>> + dmactl = drvdata->rxtx_wr_dmactl;
>> + map = drvdata->rxtx_lpaif_map;
>> + id = pcm_data->dma_ch - v->rxtx_wrdma_channel_start;
>> + } else if (dai_id == LPASS_CDC_DMA_VA_TX0) {
>> + dmactl = drvdata->va_wr_dmactl;
>> + map = drvdata->va_lpaif_map;
>> + id = pcm_data->dma_ch - v->va_wrdma_channel_start;
>> + }
>> + }
> see above
okay.
>> + switch (cmd) {
>> + case SNDRV_PCM_TRIGGER_START:
>> + case SNDRV_PCM_TRIGGER_RESUME:
>> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> + __lpass_platform_codec_intf_init(dai, substream);
>> + break;
>> + case SNDRV_PCM_TRIGGER_STOP:
>> + case SNDRV_PCM_TRIGGER_SUSPEND:
>> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>> + ret = regmap_fields_write(dmactl->codec_enable, id, LPAIF_DMACTL_ENABLE_OFF);
>> + if (ret) {
>> + dev_err(soc_runtime->dev,
>> + "error writing to rdmactl reg: %d\n", ret);
> not necessarily rdmactl, better log that it's the codec enable reg.
okay. will change it.
>
>> + return ret;
>> + }
>> +
>> + break;
>> + }
>> + return ret;
>> +}
>> +
>> +const struct snd_soc_dai_ops asoc_qcom_lpass_wcd_dai_ops = {
>> + .startup = lpass_wcd_daiops_startup,
>> + .shutdown = lpass_wcd_daiops_shutdown,
>> + .hw_params = lpass_wcd_daiops_hw_params,
>> + .trigger = lpass_wcd_daiops_trigger,
>> +};
>> +EXPORT_SYMBOL_GPL(asoc_qcom_lpass_wcd_dai_ops);
>> +
>> +MODULE_DESCRIPTION("QTi LPASS CDC DMA Driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
>> index 058b42d..e0ea698 100644
>> --- a/sound/soc/qcom/lpass.h
>> +++ b/sound/soc/qcom/lpass.h
>> @@ -418,5 +418,6 @@ int asoc_qcom_lpass_cpu_dai_probe(struct snd_soc_dai *dai);
>> extern const struct snd_soc_dai_ops asoc_qcom_lpass_cpu_dai_ops;
>> int lpass_cpu_pcm_new(struct snd_soc_pcm_runtime *rtd,
>> struct snd_soc_dai *dai);
>> +extern const struct snd_soc_dai_ops asoc_qcom_lpass_wcd_dai_ops;
>>
>> #endif /* __LPASS_H__ */
>> --
>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
>> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>>
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.