Re: [alsa-devel] [PATCH] ASoC: qcom: add sdm845 sound card support

From: Rohit Kumar
Date: Tue Jun 19 2018 - 09:58:37 EST

Thanks Srinivas for reviewing.


On 6/19/2018 2:16 PM, Srinivas Kandagatla wrote:
Thanks Rohit for the patch!

On 18/06/18 12:16, Rohit kumar wrote:
This patch adds sdm845 audio machine driver support.

Signed-off-by: Rohit kumar <rohitkr@xxxxxxxxxxxxxx>
---
 .../devicetree/bindings/sound/qcom,sdm845.txt | 87 ++++
 sound/soc/qcom/Kconfig | 9 +
 sound/soc/qcom/Makefile | 2 +
 sound/soc/qcom/sdm845.c | 539 +++++++++++++++++++++
 4 files changed, 637 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,sdm845.txt

Split the bindings into a separate patch!

Sure, will do it in next spin.

 create mode 100644 sound/soc/qcom/sdm845.c

diff --git a/Documentation/devicetree/bindings/sound/qcom,sdm845.txt b/Documentation/devicetree/bindings/sound/qcom,sdm845.txt
new file mode 100644
index 0000000..fc0e98c
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/qcom,sdm845.txt
@@ -0,0 +1,87 @@
+* Qualcomm Technologies Inc. SDM845 ASoC sound card driver
+
+This binding describes the SDM845 sound card, which uses qdsp for audio.

[..]
+
+- codec:
+ÂÂÂ Usage: required
+ÂÂÂ Value type: <subnode>
+ÂÂÂ Definition: codec dai sub-node
+
+- platform:
+ÂÂÂ Usage: opional

Optional

okay
+ÂÂÂ Value type: <subnode>
+ÂÂÂ Definition: platform dai sub-node
+
+- sound-dai:
+ÂÂÂ Usage: required
+ÂÂÂ Value type: <phandle>
+ÂÂÂ Definition: dai phandle/s and port of CPU/CODEC/PLATFORM node.
+
+Example:
+
+audio {
+ÂÂÂ compatible = "qcom,sdm845-sndcard";
+ÂÂÂ qcom,model = "sdm845-snd-card";
+ÂÂÂ pinctrl-names = "pri_active", "pri_sleep";
+ÂÂÂ pinctrl-0 = <&pri_mi2s_active &pri_mi2s_ws_active>;
+ÂÂÂ pinctrl-1 = <&pri_mi2s_sleep &pri_mi2s_ws_sleep>;
+
+ÂÂÂ qcom,audio-routing = "PRI_MI2S_RX Audio Mixer", "Pri-mi2s-gpio";
+
+ÂÂÂ cdc-vdd-supply = <&pm8998_l14>;
+
+ÂÂÂ fe@1 {
Lets not use fe or be reference here, its just a link.
okay


+ÂÂÂÂÂÂÂ link-name = "MultiMedia1";
+ÂÂÂÂÂÂÂ cpu {
+ÂÂÂÂÂÂÂÂÂÂÂ sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA1>;
+ÂÂÂÂÂÂÂ };
+ÂÂÂÂÂÂÂ platform {
+ÂÂÂÂÂÂÂÂÂÂÂ sound-dai = <&q6asmdai>;
+ÂÂÂÂÂÂÂ };
asmdai has sound-cell specifier as 1, so this will dtc will throw warning for this.

have a look at how 8996 is done.

ok, sure

+ÂÂÂ };
+
+ÂÂÂ be@1 {

[..]
diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
index 206945b..ac9609e 100644
--- a/sound/soc/qcom/Makefile
+++ b/sound/soc/qcom/Makefile
@@ -14,10 +14,12 @@ obj-$(CONFIG_SND_SOC_LPASS_APQ8016) += snd-soc-lpass-apq8016.o
 snd-soc-storm-objs := storm.o
 snd-soc-apq8016-sbc-objs := apq8016_sbc.o
 snd-soc-apq8096-objs := apq8096.o
+snd-soc-sdm845-objs := sdm845.o
  obj-$(CONFIG_SND_SOC_STORM) += snd-soc-storm.o
 obj-$(CONFIG_SND_SOC_APQ8016_SBC) += snd-soc-apq8016-sbc.o
 obj-$(CONFIG_SND_SOC_MSM8996) += snd-soc-apq8096.o
++obj-$(CONFIG_SND_SOC_SDM845) += snd-soc-sdm845.o

?? looks like typo here.

Right. Will update.

  #DSP lib
 obj-$(CONFIG_SND_SOC_QDSP6) += qdsp6/
diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c
new file mode 100644
index 0000000..70d2611
--- /dev/null
+++ b/sound/soc/qcom/sdm845.c
@@ -0,0 +1,539 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/component.h>
[..]
+}
+
+static int sdm845_snd_startup(struct snd_pcm_substream *substream)
+{
+ÂÂÂ unsigned int fmt = SND_SOC_DAIFMT_CBS_CFS;
+ÂÂÂ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ÂÂÂ struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+
+ÂÂÂ pr_debug("%s: dai_id: 0x%x\n", __func__, cpu_dai->id);
+ÂÂÂ switch (cpu_dai->id) {
+ÂÂÂ case PRIMARY_MI2S_RX:
+ÂÂÂ case PRIMARY_MI2S_TX:
+ÂÂÂÂÂÂÂ mutex_lock(&pri_mi2s_res_lock);

Mutex and atomic variables looks redundant here.
Can you explain why do you need both?
Right. Only mutex is required here. I will make count as non-atomic.


...
+
+static int sdm845_sbc_parse_of(struct snd_soc_card *card)
+{
+ÂÂÂ struct device *dev = card->dev;
+ÂÂÂ struct snd_soc_dai_link *link;
+ÂÂÂ struct device_node *np, *codec, *platform, *cpu, *node;
+ÂÂÂ int ret, num_links;
+ÂÂÂ struct sdm845_snd_data *data;
+
+ÂÂÂ ret = snd_soc_of_parse_card_name(card, "qcom,model");
+ÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂ dev_err(dev, "Error parsing card name: %d\n", ret);
+ÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
+
+ÂÂÂ node = dev->of_node;
+
+ÂÂÂ /* DAPM routes */
+ÂÂÂ if (of_property_read_bool(node, "qcom,audio-routing")) {
+ÂÂÂÂÂÂÂ ret = snd_soc_of_parse_audio_routing(card,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "qcom,audio-routing");
+ÂÂÂÂÂÂÂ if (ret)
+ÂÂÂÂÂÂÂÂÂÂÂ return ret;
+ÂÂÂ }
+
+ÂÂÂ /* Populate links */
+ÂÂÂ num_links = of_get_child_count(node);
+
+ÂÂÂ dev_info(dev, "Found %d child audio dai links..\n", num_links);

Looks unnessary!

Ok . Will remove it in next patchset.

+
+ÂÂÂÂÂÂÂ link->platform_of_node = of_parse_phandle(platform,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "sound-dai", 0);
+ÂÂÂÂÂÂÂ if (!link->platform_of_node) {
+ÂÂÂÂÂÂÂÂÂÂÂ dev_err(card->dev, "error getting platform phandle\n");
+ÂÂÂÂÂÂÂÂÂÂÂ ret = -EINVAL;
+ÂÂÂÂÂÂÂÂÂÂÂ goto fail;
+ÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂ ret = snd_soc_of_get_dai_name(cpu, &link->cpu_dai_name);
+ÂÂÂÂÂÂÂ if (ret) {
+ÂÂÂÂÂÂÂÂÂÂÂ dev_err(card->dev, "error getting cpu dai name\n");
+ÂÂÂÂÂÂÂÂÂÂÂ goto fail;
+ÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂ if (codec) {
+ÂÂÂÂÂÂÂÂÂÂÂ ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
+ÂÂÂÂÂÂÂÂÂÂÂ if (ret < 0) {
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(card->dev, "error getting codec dai name\n");
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto fail;
+ÂÂÂÂÂÂÂÂÂÂÂ }
+ÂÂÂÂÂÂÂÂÂÂÂ link->no_pcm = 1;
+ÂÂÂÂÂÂÂÂÂÂÂ link->ignore_suspend = 1;
+ÂÂÂÂÂÂÂÂÂÂÂ link->ignore_pmdown_time = 1;
+ÂÂÂÂÂÂÂÂÂÂÂ link->ops = &sdm845_be_ops;
+ÂÂÂÂÂÂÂÂÂÂÂ link->be_hw_params_fixup = sdm845_be_hw_params_fixup;
+ÂÂÂÂÂÂÂ } else {
+ÂÂÂÂÂÂÂÂÂÂÂ link->dynamic = 1;
+ÂÂÂÂÂÂÂÂÂÂÂ link->codec_dai_name = "snd-soc-dummy-dai";
+ÂÂÂÂÂÂÂÂÂÂÂ link->codec_name = "snd-soc-dummy";
+ÂÂÂÂÂÂÂ }
+

You could optimize this code, have a look at apq8096.c which does exactly same thing.


Okay. will check and update.
...
+
+static void sdm845_unbind(struct device *dev)
+{
+ÂÂÂ struct snd_soc_card *card = dev_get_drvdata(dev);
+ÂÂÂ struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
+
+ÂÂÂ mutex_destroy(&pri_mi2s_res_lock);
+ÂÂÂ mutex_destroy(&quat_tdm_res_lock);
+ÂÂÂ if (data->vdd_supply)
+ÂÂÂÂÂÂÂ regulator_put(data->vdd_supply);
+ÂÂÂ component_unbind_all(dev, card);
+ÂÂÂ snd_soc_unregister_card(card);
+ÂÂÂ kfree(data);
+ÂÂÂ kfree(card);
+}
+
+static const struct component_master_ops sdm845_ops = {
+ÂÂÂ .bind = sdm845_bind,
+ÂÂÂ .unbind = sdm845_unbind,
+};
+
+static int sdm845_runtime_resume(struct device *dev)
+{
+ÂÂÂ struct snd_soc_card *card = dev_get_drvdata(dev);
+ÂÂÂ struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
+
+ÂÂÂ if (!data->vdd_supply) {
+ÂÂÂÂÂÂÂ dev_dbg(dev, "no supplies defined\n");
+ÂÂÂÂÂÂÂ return 0;
+ÂÂÂ }
+
+ÂÂÂ if (regulator_enable(data->vdd_supply))
+ÂÂÂÂÂÂÂ dev_err(dev, "Enable regulator supply failed\n");
+
+ÂÂÂ return 0;
+}
+
+static struct platform_driver sdm845_snd_driver = {
+ÂÂÂ .probe = sdm845_snd_platform_probe,
+ÂÂÂ .remove = sdm845_snd_platform_remove,
+ÂÂÂ .driver = {
+ÂÂÂÂÂÂÂ .name = "msm-snd-sdm845",
+ÂÂÂÂÂÂÂ .pm = &sdm845_pm_ops,
+ÂÂÂÂÂÂÂ .owner = THIS_MODULE,
not necessary!

Okay. Will remove it.
+ÂÂÂÂÂÂÂ .of_match_table = of_match_ptr(sdm845_snd_device_id),
+ÂÂÂ },
+};
+module_platform_driver(sdm845_snd_driver);
+
+MODULE_DESCRIPTION("sdm845 ASoC Machine Driver");
+MODULE_LICENSE("GPL v2");

_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Regards,
Rohit

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