Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/2] Add documentation and machine driver for SC7180 sound card
@ 2020-07-21 10:44 Cheng-Yi Chiang
  2020-07-21 10:44 ` [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
  2020-07-21 10:44 ` [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
  0 siblings, 2 replies; 11+ messages in thread
From: Cheng-Yi Chiang @ 2020-07-21 10:44 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Brown, Taniya Das, Rohit kumar, Banajit Goswami,
	Patrick Lai, Andy Gross, Bjorn Andersson, Liam Girdwood,
	Rob Herring, Jaroslav Kysela, Takashi Iwai, dianders, dgreid,
	tzungbi, linux-arm-kernel, linux-arm-msm, devicetree, alsa-devel,
	Cheng-Yi Chiang

Note:
- The machine driver patch depends on LPASS patch series so it is not ready to be merged now.
  ASoC: qcom: Add support for SC7180 lpass variant https://patchwork.kernel.org/cover/11650649/
- The machine driver patch is made by the collaboration of
  Cheng-Yi Chiang <cychiang@chromium.org>
  Rohit kumar <rohitkr@codeaurora.org>
  Ajit Pandey <ajitp@codeaurora.org>
  But Ajit has left codeaurora.

Changes from v1 to v2:
- Ducumentation: Addressed all suggestions from Doug.
- Machine driver:
  - Fix comment style for license.
  - Sort includes.
  - Remove sc7180_snd_hw_params.
  - Remove sc7180_dai_init and use aux device instead for headset jack registration.
  - Statically define format for Primary MI2S.
  - Atomic is not a concern because there is mutex in card to make sure
    startup and shutdown happen sequentially.
  - Fix missing return -EINVAL in startup.
  - Use static sound card.
  - Use devm_kzalloc to avoid kfree.

Thanks for the review!

Ajit Pandey (1):
  ASoC: qcom: sc7180: Add machine driver for sound card registration

Cheng-Yi Chiang (1):
  ASoC: qcom: dt-bindings: Add sc7180 machine bindings

 .../bindings/sound/qcom,sc7180.yaml           | 130 ++++++
 sound/soc/qcom/Kconfig                        |  11 +
 sound/soc/qcom/Makefile                       |   2 +
 sound/soc/qcom/sc7180.c                       | 380 ++++++++++++++++++
 4 files changed, 523 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
 create mode 100644 sound/soc/qcom/sc7180.c

-- 
2.28.0.rc0.105.gf9edc3c819-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-07-21 10:44 [PATCH v2 0/2] Add documentation and machine driver for SC7180 sound card Cheng-Yi Chiang
@ 2020-07-21 10:44 ` Cheng-Yi Chiang
  2020-07-22  9:25   ` Srinivas Kandagatla
  2020-07-21 10:44 ` [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
  1 sibling, 1 reply; 11+ messages in thread
From: Cheng-Yi Chiang @ 2020-07-21 10:44 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Brown, Taniya Das, Rohit kumar, Banajit Goswami,
	Patrick Lai, Andy Gross, Bjorn Andersson, Liam Girdwood,
	Rob Herring, Jaroslav Kysela, Takashi Iwai, dianders, dgreid,
	tzungbi, linux-arm-kernel, linux-arm-msm, devicetree, alsa-devel,
	Cheng-Yi Chiang

Add devicetree bindings documentation file for sc7180 sound card.

Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
---
 .../bindings/sound/qcom,sc7180.yaml           | 130 ++++++++++++++++++
 1 file changed, 130 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml

diff --git a/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
new file mode 100644
index 000000000000..82f9483276eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/qcom,sc7180.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies Inc. SC7180 ASoC sound card driver
+
+maintainers:
+  - Rohit kumar <rohitkr@codeaurora.org>
+  - Cheng-Yi Chiang <cychiang@chromium.org>
+
+description:
+  This binding describes the SC7180 sound card which uses LPASS for audio.
+
+definitions:
+
+  dai:
+    type: object
+    properties:
+      sound-dai:
+        maxItems: 1
+        $ref: /schemas/types.yaml#/definitions/phandle-array
+        description: phandle array of the codec or CPU DAI
+
+    required:
+      - sound-dai
+
+properties:
+  compatible:
+    contains:
+      const: qcom,sc7180-sndcard
+
+  audio-routing:
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+    description:
+      A list of the connections between audio components. Each entry is a
+      pair of strings, the first being the connection's sink, the second
+      being the connection's source.
+
+  model:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: User specified audio sound card name
+
+  aux-dev:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle of the codec for headset detection
+
+patternProperties:
+  "^dai-link(@[0-9]+)?$":
+    description:
+      Each subnode represents a dai link. Subnodes of each dai links would be
+      cpu/codec dais.
+
+    type: object
+
+    properties:
+      link-name:
+        description: Indicates dai-link name and PCM stream name.
+        $ref: /schemas/types.yaml#/definitions/string
+        maxItems: 1
+
+      reg:
+        description: dai link address.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        maxItems: 1
+
+      playback-only:
+        description: Specify that the dai link is only for playback.
+        $ref: /schemas/types.yaml#/definitions/flag
+
+      capture-only:
+        description: Specify that the dai link is only for capture.
+        $ref: /schemas/types.yaml#/definitions/flag
+
+      cpu:
+        $ref: "#/definitions/dai"
+
+      codec:
+        $ref: "#/definitions/dai"
+
+    required:
+      - link-name
+      - reg
+      - cpu
+      - codec
+
+    additionalProperties: false
+
+examples:
+
+  - |
+    sound {
+        compatible = "qcom,sc7180-sndcard";
+        model = "sc7180-snd-card";
+
+        audio-routing =
+                    "Headphone Jack", "HPOL",
+                    "Headphone Jack", "HPOR";
+
+        aux-dev = <&alc5682>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        dai-link@0 {
+            reg = <0>;
+            link-name = "MultiMedia0";
+            cpu {
+                sound-dai = <&lpass_cpu 0>;
+            };
+
+            codec {
+                sound-dai = <&alc5682 0>;
+            };
+        };
+
+        dai-link@1 {
+            reg = <1>;
+            link-name = "MultiMedia1";
+            playback-only;
+            cpu {
+                sound-dai = <&lpass_cpu 1>;
+            };
+
+            codec {
+                sound-dai = <&max98357a>;
+            };
+        };
+    };
-- 
2.28.0.rc0.105.gf9edc3c819-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
  2020-07-21 10:44 [PATCH v2 0/2] Add documentation and machine driver for SC7180 sound card Cheng-Yi Chiang
  2020-07-21 10:44 ` [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
@ 2020-07-21 10:44 ` Cheng-Yi Chiang
  2020-07-22  3:15   ` Tzung-Bi Shih
  2020-07-22  9:25   ` Srinivas Kandagatla
  1 sibling, 2 replies; 11+ messages in thread
From: Cheng-Yi Chiang @ 2020-07-21 10:44 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Brown, Taniya Das, Rohit kumar, Banajit Goswami,
	Patrick Lai, Andy Gross, Bjorn Andersson, Liam Girdwood,
	Rob Herring, Jaroslav Kysela, Takashi Iwai, dianders, dgreid,
	tzungbi, linux-arm-kernel, linux-arm-msm, devicetree, alsa-devel,
	Ajit Pandey, Cheng-Yi Chiang

From: Ajit Pandey <ajitp@codeaurora.org>

Add new driver to register sound card on sc7180 trogdor board and
do the required configuration for lpass cpu dai and external codecs
connected over MI2S interfaces.

Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
---
 sound/soc/qcom/Kconfig  |  11 ++
 sound/soc/qcom/Makefile |   2 +
 sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 393 insertions(+)
 create mode 100644 sound/soc/qcom/sc7180.c

diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
index cfca0f730c61..1b02f521f355 100644
--- a/sound/soc/qcom/Kconfig
+++ b/sound/soc/qcom/Kconfig
@@ -109,3 +109,14 @@ config SND_SOC_SDM845
 	  To add support for audio on Qualcomm Technologies Inc.
 	  SDM845 SoC-based systems.
 	  Say Y if you want to use audio device on this SoCs.
+
+config SND_SOC_SC7180
+	tristate "SoC Machine driver for SC7180 boards"
+	depends on SND_SOC_QCOM
+	select SND_SOC_LPASS_SC7180
+	select SND_SOC_MAX98357A
+	select SND_SOC_RT5682
+	help
+	 To add support for audio on Qualcomm Technologies Inc.
+	 SC7180 SoC-based systems.
+	 Say Y if you want to use audio device on this SoCs.
diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
index 41b2c7a23a4d..3f6275d90526 100644
--- a/sound/soc/qcom/Makefile
+++ b/sound/soc/qcom/Makefile
@@ -15,12 +15,14 @@ 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
+snd-soc-sc7180-objs := sc7180.o
 snd-soc-qcom-common-objs := common.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
+obj-$(CONFIG_SND_SOC_SC7180) += snd-soc-sc7180.o
 obj-$(CONFIG_SND_SOC_QCOM_COMMON) += snd-soc-qcom-common.o
 
 #DSP lib
diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
new file mode 100644
index 000000000000..3beb2b129d01
--- /dev/null
+++ b/sound/soc/qcom/sc7180.c
@@ -0,0 +1,380 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+//Copyright (c) 2020, The Linux Foundation. All rights reserved.
+//
+//sc7180.c -- ALSA SoC Machine driver for SC7180
+
+#include <dt-bindings/sound/sc7180-lpass.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <sound/core.h>
+#include <sound/jack.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <uapi/linux/input-event-codes.h>
+
+#include "../codecs/rt5682.h"
+#include "common.h"
+#include "lpass.h"
+
+#define DEFAULT_SAMPLE_RATE_48K		48000
+#define DEFAULT_MCLK_RATE		19200000
+#define RT5682_PLL1_FREQ (48000 * 512)
+
+static int sc7180_headset_init(struct snd_soc_component *component);
+
+static struct snd_soc_aux_dev sc7180_headset_dev = {
+	.dlc = COMP_EMPTY(),
+	.init = sc7180_headset_init,
+};
+
+struct sc7180_snd_data {
+	struct snd_soc_jack jack;
+	struct snd_soc_card *card;
+	u32 pri_mi2s_clk_count;
+};
+
+static void sc7180_jack_free(struct snd_jack *jack)
+{
+	struct snd_soc_component *component = jack->private_data;
+
+	snd_soc_component_set_jack(component, NULL, NULL);
+}
+
+static int sc7180_headset_init(struct snd_soc_component *component)
+{
+	struct snd_soc_card *card = component->card;
+	struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
+	struct snd_jack *jack;
+	int rval;
+
+	rval = snd_soc_card_jack_new(
+			card, "Headset Jack",
+			SND_JACK_HEADSET |
+			SND_JACK_HEADPHONE |
+			SND_JACK_BTN_0 | SND_JACK_BTN_1 |
+			SND_JACK_BTN_2 | SND_JACK_BTN_3,
+			&pdata->jack, NULL, 0);
+
+	if (rval < 0) {
+		dev_err(card->dev, "Unable to add Headset Jack\n");
+		return rval;
+	}
+
+	jack = pdata->jack.jack;
+
+	snd_jack_set_key(jack, SND_JACK_BTN_0, KEY_PLAYPAUSE);
+	snd_jack_set_key(jack, SND_JACK_BTN_1, KEY_VOICECOMMAND);
+	snd_jack_set_key(jack, SND_JACK_BTN_2, KEY_VOLUMEUP);
+	snd_jack_set_key(jack, SND_JACK_BTN_3, KEY_VOLUMEDOWN);
+
+	jack->private_data = component;
+	jack->private_free = sc7180_jack_free;
+
+	rval = snd_soc_component_set_jack(component,
+					  &pdata->jack, NULL);
+	if (rval != 0 && rval != -EOPNOTSUPP) {
+		dev_warn(card->dev, "Failed to set jack: %d\n", rval);
+		return rval;
+	}
+
+	return 0;
+}
+
+
+static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
+				      SND_SOC_DAIFMT_NB_NF |
+				      SND_SOC_DAIFMT_I2S;
+
+static int sc7180_snd_startup(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_card *card = rtd->card;
+	struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);
+	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
+	int ret;
+
+	switch (cpu_dai->id) {
+	case MI2S_PRIMARY:
+		if (++data->pri_mi2s_clk_count == 1) {
+			snd_soc_dai_set_sysclk(cpu_dai,
+					       LPASS_MCLK0,
+					       DEFAULT_MCLK_RATE,
+					       SNDRV_PCM_STREAM_PLAYBACK);
+		}
+		snd_soc_dai_set_fmt(codec_dai, primary_dai_fmt);
+
+		/* Configure PLL1 for codec */
+		ret = snd_soc_dai_set_pll(codec_dai, 0, RT5682_PLL1_S_MCLK,
+					  DEFAULT_MCLK_RATE, RT5682_PLL1_FREQ);
+		if (ret < 0) {
+			dev_err(rtd->dev, "can't set codec pll: %d\n", ret);
+			return ret;
+		}
+
+		/* Configure sysclk for codec */
+		ret = snd_soc_dai_set_sysclk(codec_dai, RT5682_SCLK_S_PLL1,
+					     RT5682_PLL1_FREQ,
+					     SND_SOC_CLOCK_IN);
+		if (ret < 0)
+			dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n",
+				ret);
+
+		break;
+	case MI2S_SECONDARY:
+		break;
+	default:
+		dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
+			cpu_dai->id);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static void sc7180_snd_shutdown(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_card *card = rtd->card;
+	struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);
+	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+
+	switch (cpu_dai->id) {
+	case MI2S_PRIMARY:
+		if (--data->pri_mi2s_clk_count == 0) {
+			snd_soc_dai_set_sysclk(cpu_dai,
+					       LPASS_MCLK0,
+					       0,
+					       SNDRV_PCM_STREAM_PLAYBACK);
+		}
+		break;
+	case MI2S_SECONDARY:
+		break;
+	default:
+		dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
+			cpu_dai->id);
+		break;
+	}
+}
+
+static const struct snd_soc_ops sc7180_ops = {
+	.startup = sc7180_snd_startup,
+	.shutdown = sc7180_snd_shutdown,
+};
+
+static const struct snd_soc_dapm_widget sc7180_snd_widgets[] = {
+	SND_SOC_DAPM_HP("Headphone Jack", NULL),
+	SND_SOC_DAPM_MIC("Headset Mic", NULL),
+};
+
+static struct snd_soc_card sc7180_card = {
+	.owner = THIS_MODULE,
+	.aux_dev = &sc7180_headset_dev,
+	.num_aux_devs = 1,
+	.dapm_widgets = sc7180_snd_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
+};
+
+static int sc7180_parse_of(struct snd_soc_card *card)
+{
+	struct device_node *np;
+	struct device_node *codec = NULL;
+	struct device_node *platform = NULL;
+	struct device_node *cpu = NULL;
+	struct device *dev = card->dev;
+	struct snd_soc_dai_link *link;
+	struct of_phandle_args args;
+	struct snd_soc_dai_link_component *dlc;
+	int ret, num_links;
+
+	ret = snd_soc_of_parse_card_name(card, "model");
+	if (ret) {
+		dev_err(dev, "Error parsing card name: %d\n", ret);
+		return ret;
+	}
+
+	/* DAPM routes */
+	if (of_property_read_bool(dev->of_node, "audio-routing")) {
+		ret = snd_soc_of_parse_audio_routing(card,
+						     "audio-routing");
+		if (ret)
+			return ret;
+	}
+
+	/* headset aux dev. */
+	sc7180_headset_dev.dlc.of_node = of_parse_phandle(
+			dev->of_node, "aux-dev", 0);
+	if (!sc7180_headset_dev.dlc.of_node) {
+		dev_err(dev,
+			"Property 'aux-dev' missing/invalid\n");
+		return -EINVAL;
+	}
+
+	/* Populate links */
+	num_links = of_get_child_count(dev->of_node);
+
+	/* Allocate the DAI link array */
+	card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
+				      GFP_KERNEL);
+	if (!card->dai_link)
+		return -ENOMEM;
+
+	card->num_links = num_links;
+	link = card->dai_link;
+
+	for_each_child_of_node(dev->of_node, np) {
+		dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
+		if (!dlc)
+			return -ENOMEM;
+
+		link->cpus	= &dlc[0];
+		link->platforms	= &dlc[1];
+
+		link->num_cpus		= 1;
+		link->num_platforms	= 1;
+
+		ret = of_property_read_string(np, "link-name", &link->name);
+		if (ret) {
+			dev_err(card->dev,
+				"error getting codec dai_link name\n");
+			goto err;
+		}
+
+		link->playback_only = of_property_read_bool(np,
+							    "playback_only");
+		link->capture_only = of_property_read_bool(np,
+							   "capture_only");
+
+		if (link->playback_only && link->capture_only) {
+			dev_err(card->dev,
+				"getting both playback and capture only\n");
+			goto err;
+		}
+
+		cpu = of_get_child_by_name(np, "cpu");
+		codec = of_get_child_by_name(np, "codec");
+
+		if (!cpu) {
+			dev_err(dev, "%s: Can't find cpu DT node\n",
+				link->name);
+			ret = -EINVAL;
+			goto err;
+		}
+
+		ret = of_parse_phandle_with_args(cpu, "sound-dai",
+						 "#sound-dai-cells", 0, &args);
+		if (ret) {
+			dev_err(card->dev, "%s: error getting cpu phandle\n",
+				link->name);
+			goto err;
+		}
+		link->cpus->of_node = args.np;
+		link->id = args.args[0];
+
+		ret = snd_soc_of_get_dai_name(cpu, &link->cpus->dai_name);
+		if (ret) {
+			dev_err(card->dev, "%s: error getting cpu dai name\n",
+				link->name);
+			goto err;
+		}
+
+		if (codec) {
+			ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
+			if (ret < 0) {
+				dev_err(card->dev, "%s: codec dai not found\n",
+					link->name);
+				goto err;
+			}
+		} else {
+			dlc = devm_kzalloc(dev, sizeof(*dlc), GFP_KERNEL);
+			if (!dlc)
+				return -ENOMEM;
+
+			link->codecs	 = dlc;
+			link->num_codecs = 1;
+
+			link->codecs->dai_name = "snd-soc-dummy-dai";
+			link->codecs->name = "snd-soc-dummy";
+		}
+
+		link->platforms->of_node = link->cpus->of_node;
+		link->stream_name = link->name;
+		link->ops = &sc7180_ops;
+		link++;
+
+		of_node_put(cpu);
+		of_node_put(codec);
+	}
+
+	return 0;
+err:
+	of_node_put(np);
+	of_node_put(cpu);
+	of_node_put(codec);
+	of_node_put(platform);
+	return ret;
+}
+
+static int sc7180_snd_platform_probe(struct platform_device *pdev)
+{
+	struct snd_soc_card *card;
+	struct sc7180_snd_data *data;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	card = &sc7180_card;
+
+	/* Allocate the private data */
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	card->dapm_widgets = sc7180_snd_widgets;
+	card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
+	card->dev = dev;
+	dev_set_drvdata(dev, card);
+	ret = sc7180_parse_of(card);
+	if (ret) {
+		dev_err(dev, "Error parsing OF data\n");
+		return ret;
+	}
+
+	data->card = card;
+	snd_soc_card_set_drvdata(card, data);
+
+	ret = snd_soc_register_card(card);
+	if (ret) {
+		dev_err(dev, "Sound card registration failed\n");
+		return ret;
+	}
+	return ret;
+}
+
+static int sc7180_snd_platform_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
+
+	snd_soc_unregister_card(card);
+	return 0;
+}
+
+static const struct of_device_id sc7180_snd_device_id[]  = {
+	{ .compatible = "qcom,sc7180-sndcard" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sc7180_snd_device_id);
+
+static struct platform_driver sc7180_snd_driver = {
+	.probe = sc7180_snd_platform_probe,
+	.remove = sc7180_snd_platform_remove,
+	.driver = {
+		.name = "msm-snd-sc7180",
+		.of_match_table = sc7180_snd_device_id,
+	},
+};
+module_platform_driver(sc7180_snd_driver);
+
+MODULE_DESCRIPTION("sc7180 ASoC Machine Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.28.0.rc0.105.gf9edc3c819-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
  2020-07-21 10:44 ` [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
@ 2020-07-22  3:15   ` Tzung-Bi Shih
  2020-07-31  8:55     ` Cheng-yi Chiang
  2020-07-22  9:25   ` Srinivas Kandagatla
  1 sibling, 1 reply; 11+ messages in thread
From: Tzung-Bi Shih @ 2020-07-22  3:15 UTC (permalink / raw)
  To: Cheng-Yi Chiang
  Cc: Linux Kernel Mailing List, Mark Brown, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Douglas Anderson, dgreid, Tzung-Bi Shih, linux-arm-kernel,
	linux-arm-msm, devicetree, ALSA development, Ajit Pandey

On Tue, Jul 21, 2020 at 6:44 PM Cheng-Yi Chiang <cychiang@chromium.org> wrote:
> diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> new file mode 100644
> index 000000000000..3beb2b129d01
> --- /dev/null
> +++ b/sound/soc/qcom/sc7180.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +//Copyright (c) 2020, The Linux Foundation. All rights reserved.
> +//
> +//sc7180.c -- ALSA SoC Machine driver for SC7180
Insert an extra space between // and text to make it look better.

> +static int sc7180_headset_init(struct snd_soc_component *component);
> +
> +static struct snd_soc_aux_dev sc7180_headset_dev = {
> +       .dlc = COMP_EMPTY(),
> +       .init = sc7180_headset_init,
> +};
Move definition of sc7180_headset_dev after sc7180_headset_init( ) so
that you don't need forward declaration of sc7180_headset_init( ).

> +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> +                                     SND_SOC_DAIFMT_NB_NF |
> +                                     SND_SOC_DAIFMT_I2S;
> +
> +static int sc7180_snd_startup(struct snd_pcm_substream *substream)
> +{
> +       struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +       struct snd_soc_card *card = rtd->card;
> +       struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);
> +       struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> +       struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> +       int ret;
> +
> +       switch (cpu_dai->id) {
> +       case MI2S_PRIMARY:
> +               if (++data->pri_mi2s_clk_count == 1) {
> +                       snd_soc_dai_set_sysclk(cpu_dai,
> +                                              LPASS_MCLK0,
> +                                              DEFAULT_MCLK_RATE,
> +                                              SNDRV_PCM_STREAM_PLAYBACK);
> +               }
> +               snd_soc_dai_set_fmt(codec_dai, primary_dai_fmt);
My comment on the previous thread may mislead.  My original intent:
move the DAIFMT setting to DAI link->dai_fmt in sc7180_parse_of( ).

If you need to keep it as is: inline the SND_SOC_DAIFMT_* into
snd_soc_dai_set_fmt( ) (i.e. eliminate primary_dai_fmt).

> +static int sc7180_parse_of(struct snd_soc_card *card)
> +{
> +       struct device_node *np;
> +       struct device_node *codec = NULL;
> +       struct device_node *platform = NULL;
The function doesn't use platform.

> +       struct device_node *cpu = NULL;
> +       struct device *dev = card->dev;
> +       struct snd_soc_dai_link *link;
> +       struct of_phandle_args args;
> +       struct snd_soc_dai_link_component *dlc;
> +       int ret, num_links;
> +
> +       ret = snd_soc_of_parse_card_name(card, "model");
> +       if (ret) {
> +               dev_err(dev, "Error parsing card name: %d\n", ret);
> +               return ret;
> +       }
> +
> +       /* DAPM routes */
> +       if (of_property_read_bool(dev->of_node, "audio-routing")) {
> +               ret = snd_soc_of_parse_audio_routing(card,
> +                                                    "audio-routing");
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       /* headset aux dev. */
> +       sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> +                       dev->of_node, "aux-dev", 0);
> +       if (!sc7180_headset_dev.dlc.of_node) {
> +               dev_err(dev,
> +                       "Property 'aux-dev' missing/invalid\n");
> +               return -EINVAL;
> +       }
> +
> +       /* Populate links */
> +       num_links = of_get_child_count(dev->of_node);
Eliminate num_links but use card->num_links directly.

> +
> +       /* Allocate the DAI link array */
> +       card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
> +                                     GFP_KERNEL);
> +       if (!card->dai_link)
> +               return -ENOMEM;
> +
> +       card->num_links = num_links;
Ditto, eliminate it.

> +       link = card->dai_link;
> +
Eliminate the blank line to make "link = card->dai_link" and the
following for-loop "a whole thing".

> +       for_each_child_of_node(dev->of_node, np) {
> +               dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
> +               if (!dlc)
> +                       return -ENOMEM;
> +
> +               link->cpus      = &dlc[0];
> +               link->platforms = &dlc[1];
> +
> +               link->num_cpus          = 1;
> +               link->num_platforms     = 1;
> +
> +               ret = of_property_read_string(np, "link-name", &link->name);
> +               if (ret) {
> +                       dev_err(card->dev,
> +                               "error getting codec dai_link name\n");
> +                       goto err;
> +               }
> +
> +               link->playback_only = of_property_read_bool(np,
> +                                                           "playback_only");
> +               link->capture_only = of_property_read_bool(np,
> +                                                          "capture_only");
> +
> +               if (link->playback_only && link->capture_only) {
> +                       dev_err(card->dev,
> +                               "getting both playback and capture only\n");
ret = -EINVAL;

> +                       goto err;
> +               }
> +
> +               cpu = of_get_child_by_name(np, "cpu");
> +               codec = of_get_child_by_name(np, "codec");
Move to below.

> +
> +               if (!cpu) {
> +                       dev_err(dev, "%s: Can't find cpu DT node\n",
> +                               link->name);
> +                       ret = -EINVAL;
> +                       goto err;
> +               }
> +
> +               ret = of_parse_phandle_with_args(cpu, "sound-dai",
> +                                                "#sound-dai-cells", 0, &args);
I may overlook it but I failed to find "#sound-dai-cells" in the
dt-binding example.  I think it should be in DTS?

> +               if (ret) {
> +                       dev_err(card->dev, "%s: error getting cpu phandle\n",
> +                               link->name);
> +                       goto err;
> +               }
> +               link->cpus->of_node = args.np;
> +               link->id = args.args[0];
I am not quite sure what it will be.  I guess one of the following
comes from DTS node name.
#define MI2S_PRIMARY 0
#define MI2S_SECONDARY 1

> +
> +               ret = snd_soc_of_get_dai_name(cpu, &link->cpus->dai_name);
> +               if (ret) {
> +                       dev_err(card->dev, "%s: error getting cpu dai name\n",
> +                               link->name);
> +                       goto err;
> +               }
> +

Move "codec = of_get_child_by_name(np, "codec");" to here.
> +               if (codec) {
> +                       ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
> +                       if (ret < 0) {
> +                               dev_err(card->dev, "%s: codec dai not found\n",
> +                                       link->name);
> +                               goto err;
> +                       }
> +               } else {
> +                       dlc = devm_kzalloc(dev, sizeof(*dlc), GFP_KERNEL);
> +                       if (!dlc)
> +                               return -ENOMEM;
> +
> +                       link->codecs     = dlc;
> +                       link->num_codecs = 1;
> +
> +                       link->codecs->dai_name = "snd-soc-dummy-dai";
> +                       link->codecs->name = "snd-soc-dummy";
> +               }
> +
> +               link->platforms->of_node = link->cpus->of_node;
> +               link->stream_name = link->name;
> +               link->ops = &sc7180_ops;
> +               link++;
> +
> +               of_node_put(cpu);
> +               of_node_put(codec);
cpu = NULL;
codec = NULL;
In case of double of_node_put( ).

> +       }
> +
> +       return 0;
> +err:
> +       of_node_put(np);
I guess you don't need this.

> +       of_node_put(cpu);
> +       of_node_put(codec);
> +       of_node_put(platform);
Eliminate it, not used.

> +static int sc7180_snd_platform_probe(struct platform_device *pdev)
> +{
> +       struct snd_soc_card *card;
> +       struct sc7180_snd_data *data;
> +       struct device *dev = &pdev->dev;
> +       int ret;
> +
> +       card = &sc7180_card;
In this case, inline the initialization while declaration.

> +
> +       /* Allocate the private data */
> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       card->dapm_widgets = sc7180_snd_widgets;
> +       card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
Remove them.

> +       card->dev = dev;
> +       dev_set_drvdata(dev, card);
I guess you don't need this if using devm_snd_soc_register_card(...).

Insert a blank line.
> +       ret = sc7180_parse_of(card);
> +       if (ret) {
> +               dev_err(dev, "Error parsing OF data\n");
> +               return ret;
> +       }
> +
> +       data->card = card;
Looks like data->card is not used.

> +       snd_soc_card_set_drvdata(card, data);
> +
> +       ret = snd_soc_register_card(card);
> +       if (ret) {
> +               dev_err(dev, "Sound card registration failed\n");
> +               return ret;
> +       }
> +       return ret;
Just return devm_snd_soc_register_card(...);

> +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> +{
> +       struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> +
> +       snd_soc_unregister_card(card);
> +       return 0;
> +}
Can be removed if using devm_snd_soc_register_card( ).


I didn't go through all the cases.  But it would be better if all "if
(ret < 0)" can be replaced to "if (ret)".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
  2020-07-21 10:44 ` [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
  2020-07-22  3:15   ` Tzung-Bi Shih
@ 2020-07-22  9:25   ` Srinivas Kandagatla
  2020-07-23 18:44     ` Stephan Gerhold
  2020-07-31  8:55     ` Cheng-yi Chiang
  1 sibling, 2 replies; 11+ messages in thread
From: Srinivas Kandagatla @ 2020-07-22  9:25 UTC (permalink / raw)
  To: Cheng-Yi Chiang, linux-kernel
  Cc: Mark Brown, Taniya Das, Rohit kumar, Banajit Goswami,
	Patrick Lai, Andy Gross, Bjorn Andersson, Liam Girdwood,
	Rob Herring, Jaroslav Kysela, Takashi Iwai, dianders, dgreid,
	tzungbi, linux-arm-kernel, linux-arm-msm, devicetree, alsa-devel,
	Ajit Pandey

Few comments apart from Tzung-Bi comments,

On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> From: Ajit Pandey <ajitp@codeaurora.org>
> 
> Add new driver to register sound card on sc7180 trogdor board and
> do the required configuration for lpass cpu dai and external codecs
> connected over MI2S interfaces.
> 
> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> ---
>   sound/soc/qcom/Kconfig  |  11 ++
>   sound/soc/qcom/Makefile |   2 +
>   sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 393 insertions(+)
>   create mode 100644 sound/soc/qcom/sc7180.c
> 

> diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> new file mode 100644
> index 000000000000..3beb2b129d01
> --- /dev/null
> +++ b/sound/soc/qcom/sc7180.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +//Copyright (c) 2020, The Linux Foundation. All rights reserved.
> +//
> +//sc7180.c -- ALSA SoC Machine driver for SC7180
> +
> +#include <dt-bindings/sound/sc7180-lpass.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <sound/core.h>
> +#include <sound/jack.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <uapi/linux/input-event-codes.h>
> +
> +#include "../codecs/rt5682.h"
> +#include "common.h"

What is that you are using from this header?

> +#include "lpass.h"
> +
> +#define DEFAULT_SAMPLE_RATE_48K		48000
> +#define DEFAULT_MCLK_RATE		19200000
> +#define RT5682_PLL1_FREQ (48000 * 512)
> +
> +static int sc7180_headset_init(struct snd_soc_component *component);
> +
> +static struct snd_soc_aux_dev sc7180_headset_dev = {
> +	.dlc = COMP_EMPTY(),
> +	.init = sc7180_headset_init,
> +};

Can you explain why can you not use snd_soc_component_set_jack() on the 
codec component from link->init() callback?

> +
> +struct sc7180_snd_data {
> +	struct snd_soc_jack jack;
> +	struct snd_soc_card *card;
> +	u32 pri_mi2s_clk_count;
> +};
> +
> +static void sc7180_jack_free(struct snd_jack *jack)
> +{
> +	struct snd_soc_component *component = jack->private_data;
> +
> +	snd_soc_component_set_jack(component, NULL, NULL);
> +}
> +
> +static int sc7180_headset_init(struct snd_soc_component *component)
> +{
> +	struct snd_soc_card *card = component->card;
> +	struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> +	struct snd_jack *jack;
> +	int rval;
> +
> +	rval = snd_soc_card_jack_new(
> +			card, "Headset Jack",
> +			SND_JACK_HEADSET |
> +			SND_JACK_HEADPHONE |
> +			SND_JACK_BTN_0 | SND_JACK_BTN_1 |
> +			SND_JACK_BTN_2 | SND_JACK_BTN_3,
> +			&pdata->jack, NULL, 0);
> +
> +	if (rval < 0) {
> +		dev_err(card->dev, "Unable to add Headset Jack\n");
> +		return rval;
> +	}
> +
> +	jack = pdata->jack.jack;
> +
> +	snd_jack_set_key(jack, SND_JACK_BTN_0, KEY_PLAYPAUSE);
> +	snd_jack_set_key(jack, SND_JACK_BTN_1, KEY_VOICECOMMAND);
> +	snd_jack_set_key(jack, SND_JACK_BTN_2, KEY_VOLUMEUP);
> +	snd_jack_set_key(jack, SND_JACK_BTN_3, KEY_VOLUMEDOWN);
> +
> +	jack->private_data = component;
> +	jack->private_free = sc7180_jack_free;
> +
> +	rval = snd_soc_component_set_jack(component,
> +					  &pdata->jack, NULL);
> +	if (rval != 0 && rval != -EOPNOTSUPP) {
> +		dev_warn(card->dev, "Failed to set jack: %d\n", rval);
> +		return rval;
> +	}
> +
> +	return 0;
> +}
> +
> +
> +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> +				      SND_SOC_DAIFMT_NB_NF |
> +				      SND_SOC_DAIFMT_I2S;
> +
> +static int sc7180_snd_startup(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_card *card = rtd->card;
> +	struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);
> +	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> +	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> +	int ret;
> +
> +	switch (cpu_dai->id) {
> +	case MI2S_PRIMARY:
> +		if (++data->pri_mi2s_clk_count == 1) {
> +			snd_soc_dai_set_sysclk(cpu_dai,
> +					       LPASS_MCLK0,
> +					       DEFAULT_MCLK_RATE,
> +					       SNDRV_PCM_STREAM_PLAYBACK);
> +		}
> +		snd_soc_dai_set_fmt(codec_dai, primary_dai_fmt);
> +
> +		/* Configure PLL1 for codec */
> +		ret = snd_soc_dai_set_pll(codec_dai, 0, RT5682_PLL1_S_MCLK,
> +					  DEFAULT_MCLK_RATE, RT5682_PLL1_FREQ);
> +		if (ret < 0) {
> +			dev_err(rtd->dev, "can't set codec pll: %d\n", ret);
> +			return ret;
> +		}
> +
> +		/* Configure sysclk for codec */
> +		ret = snd_soc_dai_set_sysclk(codec_dai, RT5682_SCLK_S_PLL1,
> +					     RT5682_PLL1_FREQ,
> +					     SND_SOC_CLOCK_IN);
> +		if (ret < 0)
> +			dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n",
> +				ret);
> +
> +		break;
> +	case MI2S_SECONDARY:
> +		break;
> +	default:
> +		dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> +			cpu_dai->id);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static void sc7180_snd_shutdown(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_card *card = rtd->card;
> +	struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);
> +	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> +
> +	switch (cpu_dai->id) {
> +	case MI2S_PRIMARY:
> +		if (--data->pri_mi2s_clk_count == 0) {
> +			snd_soc_dai_set_sysclk(cpu_dai,
> +					       LPASS_MCLK0,
> +					       0,
> +					       SNDRV_PCM_STREAM_PLAYBACK);
> +		}
> +		break;
> +	case MI2S_SECONDARY:
> +		break;
> +	default:
> +		dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> +			cpu_dai->id);
> +		break;
> +	}
> +}
> +
> +static const struct snd_soc_ops sc7180_ops = {
> +	.startup = sc7180_snd_startup,
> +	.shutdown = sc7180_snd_shutdown,
> +};
> +
> +static const struct snd_soc_dapm_widget sc7180_snd_widgets[] = {
> +	SND_SOC_DAPM_HP("Headphone Jack", NULL),
> +	SND_SOC_DAPM_MIC("Headset Mic", NULL),
> +};
> +
> +static struct snd_soc_card sc7180_card = {
> +	.owner = THIS_MODULE,
> +	.aux_dev = &sc7180_headset_dev,
> +	.num_aux_devs = 1,
> +	.dapm_widgets = sc7180_snd_widgets,
> +	.num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
> +};
> +
> +static int sc7180_parse_of(struct snd_soc_card *card)
> +{

This code is getting duplicated in various places like 
apq8016_sbc_parse_of, it will be nice to common this up, if possible!


> +	struct device_node *np;
> +	struct device_node *codec = NULL;
> +	struct device_node *platform = NULL;
> +	struct device_node *cpu = NULL;
> +	struct device *dev = card->dev;
> +	struct snd_soc_dai_link *link;
> +	struct of_phandle_args args;
> +	struct snd_soc_dai_link_component *dlc;
> +	int ret, num_links;
> +
> +	ret = snd_soc_of_parse_card_name(card, "model");
> +	if (ret) {
> +		dev_err(dev, "Error parsing card name: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* DAPM routes */
> +	if (of_property_read_bool(dev->of_node, "audio-routing")) {
> +		ret = snd_soc_of_parse_audio_routing(card,
> +						     "audio-routing");
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* headset aux dev. */
> +	sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> +			dev->of_node, "aux-dev", 0);
> +	if (!sc7180_headset_dev.dlc.of_node) {
> +		dev_err(dev,
> +			"Property 'aux-dev' missing/invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Populate links */
> +	num_links = of_get_child_count(dev->of_node);
> +
> +	/* Allocate the DAI link array */
> +	card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
> +				      GFP_KERNEL);
> +	if (!card->dai_link)
> +		return -ENOMEM;
> +
> +	card->num_links = num_links;
> +	link = card->dai_link;
> +
> +	for_each_child_of_node(dev->of_node, np) {
> +		dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
> +		if (!dlc)
> +			return -ENOMEM;
> +
> +		link->cpus	= &dlc[0];
> +		link->platforms	= &dlc[1];
> +
> +		link->num_cpus		= 1;
> +		link->num_platforms	= 1;
> +
> +		ret = of_property_read_string(np, "link-name", &link->name);
> +		if (ret) {
> +			dev_err(card->dev,
> +				"error getting codec dai_link name\n");
> +			goto err;
> +		}
> +
> +		link->playback_only = of_property_read_bool(np,
> +							    "playback_only");
> +		link->capture_only = of_property_read_bool(np,
> +							   "capture_only");

Does this even work??
You have replaced "-" with "_" for property name?

> +
> +		if (link->playback_only && link->capture_only) {
> +			dev_err(card->dev,
> +				"getting both playback and capture only\n");
> +			goto err;
> +		}
> +
> +		cpu = of_get_child_by_name(np, "cpu");
> +		codec = of_get_child_by_name(np, "codec");
> +
> +		if (!cpu) {
> +			dev_err(dev, "%s: Can't find cpu DT node\n",
> +				link->name);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +
> +		ret = of_parse_phandle_with_args(cpu, "sound-dai",
> +						 "#sound-dai-cells", 0, &args);
> +		if (ret) {
> +			dev_err(card->dev, "%s: error getting cpu phandle\n",
> +				link->name);
> +			goto err;
> +		}
> +		link->cpus->of_node = args.np;
> +		link->id = args.args[0];
> +
> +		ret = snd_soc_of_get_dai_name(cpu, &link->cpus->dai_name);
> +		if (ret) {
> +			dev_err(card->dev, "%s: error getting cpu dai name\n",
> +				link->name);
> +			goto err;
> +		}
> +
> +		if (codec) {
> +			ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
> +			if (ret < 0) {
> +				dev_err(card->dev, "%s: codec dai not found\n",
> +					link->name);
> +				goto err;
> +			}
> +		} else {
> +			dlc = devm_kzalloc(dev, sizeof(*dlc), GFP_KERNEL);
> +			if (!dlc)
> +				return -ENOMEM;
> +
> +			link->codecs	 = dlc;
> +			link->num_codecs = 1;
> +
> +			link->codecs->dai_name = "snd-soc-dummy-dai";
> +			link->codecs->name = "snd-soc-dummy";
> +		}
> +
> +		link->platforms->of_node = link->cpus->of_node;
> +		link->stream_name = link->name;
> +		link->ops = &sc7180_ops;
> +		link++;
> +
> +		of_node_put(cpu);
> +		of_node_put(codec);
> +	}
> +
> +	return 0;
> +err:
> +	of_node_put(np);
> +	of_node_put(cpu);
> +	of_node_put(codec);
> +	of_node_put(platform);
> +	return ret;
> +}
> +
> +static int sc7180_snd_platform_probe(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card;
> +	struct sc7180_snd_data *data;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	card = &sc7180_card;
> +
> +	/* Allocate the private data */
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	card->dapm_widgets = sc7180_snd_widgets;
> +	card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
> +	card->dev = dev;
> +	dev_set_drvdata(dev, card);
> +	ret = sc7180_parse_of(card);
> +	if (ret) {
> +		dev_err(dev, "Error parsing OF data\n");
> +		return ret;
> +	}
> +
> +	data->card = card;
> +	snd_soc_card_set_drvdata(card, data);
> +
> +	ret = snd_soc_register_card(card);

devm_snd_soc_register_card()??

> +	if (ret) {
> +		dev_err(dev, "Sound card registration failed\n");
> +		return ret;
> +	}
> +	return ret;
> +}
> +
> +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> +
> +	snd_soc_unregister_card(card);
> +	return 0;
> +}
> +
> +static const struct of_device_id sc7180_snd_device_id[]  = {
> +	{ .compatible = "qcom,sc7180-sndcard" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sc7180_snd_device_id);
> +
> +static struct platform_driver sc7180_snd_driver = {
> +	.probe = sc7180_snd_platform_probe,
> +	.remove = sc7180_snd_platform_remove,
> +	.driver = {
> +		.name = "msm-snd-sc7180",
> +		.of_match_table = sc7180_snd_device_id,
> +	},
> +};
> +module_platform_driver(sc7180_snd_driver);
> +
> +MODULE_DESCRIPTION("sc7180 ASoC Machine Driver");
> +MODULE_LICENSE("GPL v2");
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-07-21 10:44 ` [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
@ 2020-07-22  9:25   ` Srinivas Kandagatla
  2020-07-31  8:55     ` Cheng-yi Chiang
  0 siblings, 1 reply; 11+ messages in thread
From: Srinivas Kandagatla @ 2020-07-22  9:25 UTC (permalink / raw)
  To: Cheng-Yi Chiang, linux-kernel
  Cc: Mark Brown, Taniya Das, Rohit kumar, Banajit Goswami,
	Patrick Lai, Andy Gross, Bjorn Andersson, Liam Girdwood,
	Rob Herring, Jaroslav Kysela, Takashi Iwai, dianders, dgreid,
	tzungbi, linux-arm-kernel, linux-arm-msm, devicetree, alsa-devel,
	pierre-louis.bossart



On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> Add devicetree bindings documentation file for sc7180 sound card.
> 
> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> ---
>   .../bindings/sound/qcom,sc7180.yaml           | 130 ++++++++++++++++++
>   1 file changed, 130 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> new file mode 100644
> index 000000000000..82f9483276eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> @@ -0,0 +1,130 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/qcom,sc7180.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies Inc. SC7180 ASoC sound card driver
> +
> +maintainers:
> +  - Rohit kumar <rohitkr@codeaurora.org>
> +  - Cheng-Yi Chiang <cychiang@chromium.org>
> +
> +description:
> +  This binding describes the SC7180 sound card which uses LPASS for audio.
> +
> +definitions:
> +
> +  dai:
> +    type: object
> +    properties:
> +      sound-dai:
> +        maxItems: 1
> +        $ref: /schemas/types.yaml#/definitions/phandle-array
> +        description: phandle array of the codec or CPU DAI
> +
> +    required:
> +      - sound-dai
> +
> +properties:
> +  compatible:
> +    contains:
> +      const: qcom,sc7180-sndcard
> +
> +  audio-routing:
> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> +    description:
> +      A list of the connections between audio components. Each entry is a
> +      pair of strings, the first being the connection's sink, the second
> +      being the connection's source.
> +
> +  model:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: User specified audio sound card name
> +
> +  aux-dev:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle of the codec for headset detection


Why do we need this? You should be able to set the jack for codec 
snd_soc_component_set_jack()?


> +
> +patternProperties:
> +  "^dai-link(@[0-9]+)?$":
> +    description:
> +      Each subnode represents a dai link. Subnodes of each dai links would be
> +      cpu/codec dais.
> +
> +    type: object
> +
> +    properties:
> +      link-name:
> +        description: Indicates dai-link name and PCM stream name.
> +        $ref: /schemas/types.yaml#/definitions/string
> +        maxItems: 1
> +
> +      reg:
> +        description: dai link address.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        maxItems: 1

Why do we need this?? I have not seen the parsing code using this.


> +
> +      playback-only:
> +        description: Specify that the dai link is only for playback.
> +        $ref: /schemas/types.yaml#/definitions/flag
> +
> +      capture-only:
> +        description: Specify that the dai link is only for capture.
> +        $ref: /schemas/types.yaml#/definitions/flag
> +

Are these because the cpu/codec dais are single directional?

If so you can extend snd_soc_dai_link_set_capabilities() and use this 
function.


--srini

> +      cpu:
> +        $ref: "#/definitions/dai"
> +
> +      codec:
> +        $ref: "#/definitions/dai"
> +
> +    required:
> +      - link-name
> +      - reg
> +      - cpu
> +      - codec
> +
> +    additionalProperties: false
> +
> +examples:
> +
> +  - |
> +    sound {
> +        compatible = "qcom,sc7180-sndcard";
> +        model = "sc7180-snd-card";
> +
> +        audio-routing =
> +                    "Headphone Jack", "HPOL",
> +                    "Headphone Jack", "HPOR";
> +
> +        aux-dev = <&alc5682>;
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        dai-link@0 {
> +            reg = <0>;
> +            link-name = "MultiMedia0";
> +            cpu {
> +                sound-dai = <&lpass_cpu 0>;
> +            };
> +
> +            codec {
> +                sound-dai = <&alc5682 0>;
> +            };
> +        };
> +
> +        dai-link@1 {
> +            reg = <1>;
> +            link-name = "MultiMedia1";
> +            playback-only;
> +            cpu {
> +                sound-dai = <&lpass_cpu 1>;
> +            };
> +
> +            codec {
> +                sound-dai = <&max98357a>;
> +            };
> +        };
> +    };
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
  2020-07-22  9:25   ` Srinivas Kandagatla
@ 2020-07-23 18:44     ` Stephan Gerhold
  2020-07-31  8:55       ` Cheng-yi Chiang
  2020-07-31  8:55     ` Cheng-yi Chiang
  1 sibling, 1 reply; 11+ messages in thread
From: Stephan Gerhold @ 2020-07-23 18:44 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Cheng-Yi Chiang, linux-kernel, Mark Brown, Taniya Das,
	Rohit kumar, Banajit Goswami, Patrick Lai, Andy Gross,
	Bjorn Andersson, Liam Girdwood, Rob Herring, Jaroslav Kysela,
	Takashi Iwai, dianders, dgreid, tzungbi, linux-arm-kernel,
	linux-arm-msm, devicetree, alsa-devel, Ajit Pandey

On Wed, Jul 22, 2020 at 10:25:14AM +0100, Srinivas Kandagatla wrote:
> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
> 
> This code is getting duplicated in various places like apq8016_sbc_parse_of,
> it will be nice to common this up, if possible!
> 

FYI, I started work on making apq8016_sbc use qcom_snd_parse_of()
a while ago already, but didn't find the time to finish it up.
I have now sent it, this should make it possible to use the common
qcom_snd_parse_of() function in this driver as well.

See: https://lore.kernel.org/alsa-devel/20200723183904.321040-1-stephan@gerhold.net/

Stephan


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-07-22  9:25   ` Srinivas Kandagatla
@ 2020-07-31  8:55     ` Cheng-yi Chiang
  0 siblings, 0 replies; 11+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31  8:55 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: linux-kernel, Mark Brown, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	Pierre-Louis Bossart

On Wed, Jul 22, 2020 at 5:25 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> > Add devicetree bindings documentation file for sc7180 sound card.
> >
> > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> > ---
> >   .../bindings/sound/qcom,sc7180.yaml           | 130 ++++++++++++++++++
> >   1 file changed, 130 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> > new file mode 100644
> > index 000000000000..82f9483276eb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
> > @@ -0,0 +1,130 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/qcom,sc7180.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Technologies Inc. SC7180 ASoC sound card driver
> > +
> > +maintainers:
> > +  - Rohit kumar <rohitkr@codeaurora.org>
> > +  - Cheng-Yi Chiang <cychiang@chromium.org>
> > +
> > +description:
> > +  This binding describes the SC7180 sound card which uses LPASS for audio.
> > +
> > +definitions:
> > +
> > +  dai:
> > +    type: object
> > +    properties:
> > +      sound-dai:
> > +        maxItems: 1
> > +        $ref: /schemas/types.yaml#/definitions/phandle-array
> > +        description: phandle array of the codec or CPU DAI
> > +
> > +    required:
> > +      - sound-dai
> > +
> > +properties:
> > +  compatible:
> > +    contains:
> > +      const: qcom,sc7180-sndcard
> > +
> > +  audio-routing:
> > +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> > +    description:
> > +      A list of the connections between audio components. Each entry is a
> > +      pair of strings, the first being the connection's sink, the second
> > +      being the connection's source.
> > +
> > +  model:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description: User specified audio sound card name
> > +
> > +  aux-dev:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: phandle of the codec for headset detection
>
>
> Why do we need this? You should be able to set the jack for codec
> snd_soc_component_set_jack()?
>
>
I put my explanation in the machine driver patch since there is more
context there.

> > +
> > +patternProperties:
> > +  "^dai-link(@[0-9]+)?$":
> > +    description:
> > +      Each subnode represents a dai link. Subnodes of each dai links would be
> > +      cpu/codec dais.
> > +
> > +    type: object
> > +
> > +    properties:
> > +      link-name:
> > +        description: Indicates dai-link name and PCM stream name.
> > +        $ref: /schemas/types.yaml#/definitions/string
> > +        maxItems: 1
> > +
> > +      reg:
> > +        description: dai link address.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        maxItems: 1
>
> Why do we need this?? I have not seen the parsing code using this.
>
>
When checking the yaml file using dt_binding_check, I got this warnings:

$  make dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
  CHKDT   Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
  SCHEMA  Documentation/devicetree/bindings/processed-schema-examples.yaml
  DTC     Documentation/devicetree/bindings/sound/qcom,sc7180.example.dt.yaml
Documentation/devicetree/bindings/sound/qcom,sc7180.example.dts:32.24-41.15:
Warning (unit_address_vs_reg): /example-0/sound/dai-link@0: node has a
unit name, but no reg or ranges property
Documentation/devicetree/bindings/sound/qcom,sc7180.example.dts:43.24-52.15:
Warning (unit_address_vs_reg): /example-0/sound/dai-link@1: node has a
unit name, but no reg or ranges property
  CHECK   Documentation/devicetree/bindings/sound/qcom,sc7180.example.dt.yaml

Should I ignore these warnings because reg is not used in the driver ?

> > +
> > +      playback-only:
> > +        description: Specify that the dai link is only for playback.
> > +        $ref: /schemas/types.yaml#/definitions/flag
> > +
> > +      capture-only:
> > +        description: Specify that the dai link is only for capture.
> > +        $ref: /schemas/types.yaml#/definitions/flag
> > +
>
> Are these because the cpu/codec dais are single directional?
>
> If so you can extend snd_soc_dai_link_set_capabilities() and use this
> function.

I found that this is not needed since soc_new_pcm already takes care
of checking capture/playback capability in non-dpcm cases.

>
>
> --srini
>

Thanks for reviewing the patch!



> > +      cpu:
> > +        $ref: "#/definitions/dai"
> > +
> > +      codec:
> > +        $ref: "#/definitions/dai"
> > +
> > +    required:
> > +      - link-name
> > +      - reg
> > +      - cpu
> > +      - codec
> > +
> > +    additionalProperties: false
> > +
> > +examples:
> > +
> > +  - |
> > +    sound {
> > +        compatible = "qcom,sc7180-sndcard";
> > +        model = "sc7180-snd-card";
> > +
> > +        audio-routing =
> > +                    "Headphone Jack", "HPOL",
> > +                    "Headphone Jack", "HPOR";
> > +
> > +        aux-dev = <&alc5682>;
> > +
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        dai-link@0 {
> > +            reg = <0>;
> > +            link-name = "MultiMedia0";
> > +            cpu {
> > +                sound-dai = <&lpass_cpu 0>;
> > +            };
> > +
> > +            codec {
> > +                sound-dai = <&alc5682 0>;
> > +            };
> > +        };
> > +
> > +        dai-link@1 {
> > +            reg = <1>;
> > +            link-name = "MultiMedia1";
> > +            playback-only;
> > +            cpu {
> > +                sound-dai = <&lpass_cpu 1>;
> > +            };
> > +
> > +            codec {
> > +                sound-dai = <&max98357a>;
> > +            };
> > +        };
> > +    };
> >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
  2020-07-22  3:15   ` Tzung-Bi Shih
@ 2020-07-31  8:55     ` Cheng-yi Chiang
  0 siblings, 0 replies; 11+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31  8:55 UTC (permalink / raw)
  To: Tzung-Bi Shih
  Cc: Linux Kernel Mailing List, Mark Brown, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Douglas Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	ALSA development, Ajit Pandey

On Wed, Jul 22, 2020 at 11:15 AM Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Tue, Jul 21, 2020 at 6:44 PM Cheng-Yi Chiang <cychiang@chromium.org> wrote:
> > diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> > new file mode 100644
> > index 000000000000..3beb2b129d01
> > --- /dev/null
> > +++ b/sound/soc/qcom/sc7180.c
> > @@ -0,0 +1,380 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +//Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > +//
> > +//sc7180.c -- ALSA SoC Machine driver for SC7180
> Insert an extra space between // and text to make it look better.
>
Fixed in v3.
> > +static int sc7180_headset_init(struct snd_soc_component *component);
> > +
> > +static struct snd_soc_aux_dev sc7180_headset_dev = {
> > +       .dlc = COMP_EMPTY(),
> > +       .init = sc7180_headset_init,
> > +};
> Move definition of sc7180_headset_dev after sc7180_headset_init( ) so
> that you don't need forward declaration of sc7180_headset_init( ).
>
Fixed in v3.
> > +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> > +                                     SND_SOC_DAIFMT_NB_NF |
> > +                                     SND_SOC_DAIFMT_I2S;
> > +
> > +static int sc7180_snd_startup(struct snd_pcm_substream *substream)
> > +{
> > +       struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +       struct snd_soc_card *card = rtd->card;
> > +       struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);
> > +       struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> > +       struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > +       int ret;
> > +
> > +       switch (cpu_dai->id) {
> > +       case MI2S_PRIMARY:
> > +               if (++data->pri_mi2s_clk_count == 1) {
> > +                       snd_soc_dai_set_sysclk(cpu_dai,
> > +                                              LPASS_MCLK0,
> > +                                              DEFAULT_MCLK_RATE,
> > +                                              SNDRV_PCM_STREAM_PLAYBACK);
> > +               }
> > +               snd_soc_dai_set_fmt(codec_dai, primary_dai_fmt);
> My comment on the previous thread may mislead.  My original intent:
> move the DAIFMT setting to DAI link->dai_fmt in sc7180_parse_of( ).
>
> If you need to keep it as is: inline the SND_SOC_DAIFMT_* into
> snd_soc_dai_set_fmt( ) (i.e. eliminate primary_dai_fmt).
>
Fixed in v3. Since in v3 I am reusing parse function in common.c, I
will keep the format setting in startup.

> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
> > +       struct device_node *np;
> > +       struct device_node *codec = NULL;
> > +       struct device_node *platform = NULL;
> The function doesn't use platform.
>

sc7180_parse_of is removed in v3.

> > +       struct device_node *cpu = NULL;
> > +       struct device *dev = card->dev;
> > +       struct snd_soc_dai_link *link;
> > +       struct of_phandle_args args;
> > +       struct snd_soc_dai_link_component *dlc;
> > +       int ret, num_links;
> > +
> > +       ret = snd_soc_of_parse_card_name(card, "model");
> > +       if (ret) {
> > +               dev_err(dev, "Error parsing card name: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       /* DAPM routes */
> > +       if (of_property_read_bool(dev->of_node, "audio-routing")) {
> > +               ret = snd_soc_of_parse_audio_routing(card,
> > +                                                    "audio-routing");
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       /* headset aux dev. */
> > +       sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> > +                       dev->of_node, "aux-dev", 0);
> > +       if (!sc7180_headset_dev.dlc.of_node) {
> > +               dev_err(dev,
> > +                       "Property 'aux-dev' missing/invalid\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       /* Populate links */
> > +       num_links = of_get_child_count(dev->of_node);
> Eliminate num_links but use card->num_links directly.
>
> > +
> > +       /* Allocate the DAI link array */
> > +       card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
> > +                                     GFP_KERNEL);
> > +       if (!card->dai_link)
> > +               return -ENOMEM;
> > +
> > +       card->num_links = num_links;
> Ditto, eliminate it.
>
> > +       link = card->dai_link;
> > +
> Eliminate the blank line to make "link = card->dai_link" and the
> following for-loop "a whole thing".
>
> > +       for_each_child_of_node(dev->of_node, np) {
> > +               dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
> > +               if (!dlc)
> > +                       return -ENOMEM;
> > +
> > +               link->cpus      = &dlc[0];
> > +               link->platforms = &dlc[1];
> > +
> > +               link->num_cpus          = 1;
> > +               link->num_platforms     = 1;
> > +
> > +               ret = of_property_read_string(np, "link-name", &link->name);
> > +               if (ret) {
> > +                       dev_err(card->dev,
> > +                               "error getting codec dai_link name\n");
> > +                       goto err;
> > +               }
> > +
> > +               link->playback_only = of_property_read_bool(np,
> > +                                                           "playback_only");
> > +               link->capture_only = of_property_read_bool(np,
> > +                                                          "capture_only");
> > +
> > +               if (link->playback_only && link->capture_only) {
> > +                       dev_err(card->dev,
> > +                               "getting both playback and capture only\n");
> ret = -EINVAL;
>
> > +                       goto err;
> > +               }
> > +
> > +               cpu = of_get_child_by_name(np, "cpu");
> > +               codec = of_get_child_by_name(np, "codec");
> Move to below.
>
> > +
> > +               if (!cpu) {
> > +                       dev_err(dev, "%s: Can't find cpu DT node\n",
> > +                               link->name);
> > +                       ret = -EINVAL;
> > +                       goto err;
> > +               }
> > +
> > +               ret = of_parse_phandle_with_args(cpu, "sound-dai",
> > +                                                "#sound-dai-cells", 0, &args);
> I may overlook it but I failed to find "#sound-dai-cells" in the
> dt-binding example.  I think it should be in DTS?
>
> > +               if (ret) {
> > +                       dev_err(card->dev, "%s: error getting cpu phandle\n",
> > +                               link->name);
> > +                       goto err;
> > +               }
> > +               link->cpus->of_node = args.np;
> > +               link->id = args.args[0];
> I am not quite sure what it will be.  I guess one of the following
> comes from DTS node name.
> #define MI2S_PRIMARY 0
> #define MI2S_SECONDARY 1
>
> > +
> > +               ret = snd_soc_of_get_dai_name(cpu, &link->cpus->dai_name);
> > +               if (ret) {
> > +                       dev_err(card->dev, "%s: error getting cpu dai name\n",
> > +                               link->name);
> > +                       goto err;
> > +               }
> > +
>
> Move "codec = of_get_child_by_name(np, "codec");" to here.
> > +               if (codec) {
> > +                       ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
> > +                       if (ret < 0) {
> > +                               dev_err(card->dev, "%s: codec dai not found\n",
> > +                                       link->name);
> > +                               goto err;
> > +                       }
> > +               } else {
> > +                       dlc = devm_kzalloc(dev, sizeof(*dlc), GFP_KERNEL);
> > +                       if (!dlc)
> > +                               return -ENOMEM;
> > +
> > +                       link->codecs     = dlc;
> > +                       link->num_codecs = 1;
> > +
> > +                       link->codecs->dai_name = "snd-soc-dummy-dai";
> > +                       link->codecs->name = "snd-soc-dummy";
> > +               }
> > +
> > +               link->platforms->of_node = link->cpus->of_node;
> > +               link->stream_name = link->name;
> > +               link->ops = &sc7180_ops;
> > +               link++;
> > +
> > +               of_node_put(cpu);
> > +               of_node_put(codec);
> cpu = NULL;
> codec = NULL;
> In case of double of_node_put( ).
>
> > +       }
> > +
> > +       return 0;
> > +err:
> > +       of_node_put(np);
> I guess you don't need this.
>
> > +       of_node_put(cpu);
> > +       of_node_put(codec);
> > +       of_node_put(platform);
> Eliminate it, not used.
>
> > +static int sc7180_snd_platform_probe(struct platform_device *pdev)
> > +{
> > +       struct snd_soc_card *card;
> > +       struct sc7180_snd_data *data;
> > +       struct device *dev = &pdev->dev;
> > +       int ret;
> > +
> > +       card = &sc7180_card;
> In this case, inline the initialization while declaration.
>
Fixed in v3.
> > +
> > +       /* Allocate the private data */
> > +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return -ENOMEM;
> > +
> > +       card->dapm_widgets = sc7180_snd_widgets;
> > +       card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
> Remove them.
>
Fixed in v3.
> > +       card->dev = dev;
> > +       dev_set_drvdata(dev, card);
> I guess you don't need this if using devm_snd_soc_register_card(...).
>
> Insert a blank line.

Fixed in v3.
> > +       ret = sc7180_parse_of(card);
> > +       if (ret) {
> > +               dev_err(dev, "Error parsing OF data\n");
> > +               return ret;
> > +       }
> > +
> > +       data->card = card;
> Looks like data->card is not used.
>
Removed in v3.
> > +       snd_soc_card_set_drvdata(card, data);
> > +
> > +       ret = snd_soc_register_card(card);
> > +       if (ret) {
> > +               dev_err(dev, "Sound card registration failed\n");
> > +               return ret;
> > +       }
> > +       return ret;
> Just return devm_snd_soc_register_card(...);
>
Fixed in v3.
> > +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> > +{
> > +       struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> > +
> > +       snd_soc_unregister_card(card);
> > +       return 0;
> > +}
> Can be removed if using devm_snd_soc_register_card( ).
>
>
Removed in v3.
> I didn't go through all the cases.  But it would be better if all "if
> (ret < 0)" can be replaced to "if (ret)".
Fixed in v3.

Thanks for the review!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
  2020-07-22  9:25   ` Srinivas Kandagatla
  2020-07-23 18:44     ` Stephan Gerhold
@ 2020-07-31  8:55     ` Cheng-yi Chiang
  1 sibling, 0 replies; 11+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31  8:55 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: linux-kernel, Mark Brown, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	Ajit Pandey

On Wed, Jul 22, 2020 at 5:25 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> Few comments apart from Tzung-Bi comments,
>
Thanks a lot for reviewing the patch!

> On 21/07/2020 11:44, Cheng-Yi Chiang wrote:
> > From: Ajit Pandey <ajitp@codeaurora.org>
> >
> > Add new driver to register sound card on sc7180 trogdor board and
> > do the required configuration for lpass cpu dai and external codecs
> > connected over MI2S interfaces.
> >
> > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> > ---
> >   sound/soc/qcom/Kconfig  |  11 ++
> >   sound/soc/qcom/Makefile |   2 +
> >   sound/soc/qcom/sc7180.c | 380 ++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 393 insertions(+)
> >   create mode 100644 sound/soc/qcom/sc7180.c
> >
>
> > diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> > new file mode 100644
> > index 000000000000..3beb2b129d01
> > --- /dev/null
> > +++ b/sound/soc/qcom/sc7180.c
> > @@ -0,0 +1,380 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +//Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > +//
> > +//sc7180.c -- ALSA SoC Machine driver for SC7180
> > +
> > +#include <dt-bindings/sound/sc7180-lpass.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <sound/core.h>
> > +#include <sound/jack.h>
> > +#include <sound/pcm.h>
> > +#include <sound/pcm_params.h>
> > +#include <sound/soc.h>
> > +#include <uapi/linux/input-event-codes.h>
> > +
> > +#include "../codecs/rt5682.h"
> > +#include "common.h"
>
> What is that you are using from this header?
>
It is not used in v2, but will be used in v3.

> > +#include "lpass.h"
> > +
> > +#define DEFAULT_SAMPLE_RATE_48K              48000
> > +#define DEFAULT_MCLK_RATE            19200000
> > +#define RT5682_PLL1_FREQ (48000 * 512)
> > +
> > +static int sc7180_headset_init(struct snd_soc_component *component);
> > +
> > +static struct snd_soc_aux_dev sc7180_headset_dev = {
> > +     .dlc = COMP_EMPTY(),
> > +     .init = sc7180_headset_init,
> > +};
>
> Can you explain why can you not use snd_soc_component_set_jack() on the
> codec component from link->init() callback?
>
I am using snd_soc_component_set_jack, but only on aux device.
As Tzungbi suggested in his comment on v1 machine driver patch, the
current approach in other qcom driver like sdm845.c and apq8016_sbc.c
of setting jack is not clean.
For example, in sdm845.c, a bool jack_etup is needed in
sdm845_snd_data so that jack will be created once.
Also, the machine driver assumes that snd_soc_component_set_jack is
used when cpu_dai->id == PRIMARY_MI2S_RX, which is not easy to
understand.
In apq8016_sbc.c, apq8016_sbc_dai_init tries to set jack on every
codec_dai which does not make sense.
These downsides can be avoided by using aux-dev so that the logic of
setting jack is easy to understand, and dai_init function can be much
cleaner.
> > +
> > +struct sc7180_snd_data {
> > +     struct snd_soc_jack jack;
> > +     struct snd_soc_card *card;
> > +     u32 pri_mi2s_clk_count;
> > +};
> > +
> > +static void sc7180_jack_free(struct snd_jack *jack)
> > +{
> > +     struct snd_soc_component *component = jack->private_data;
> > +
> > +     snd_soc_component_set_jack(component, NULL, NULL);
> > +}
> > +
> > +static int sc7180_headset_init(struct snd_soc_component *component)
> > +{
> > +     struct snd_soc_card *card = component->card;
> > +     struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> > +     struct snd_jack *jack;
> > +     int rval;
> > +
> > +     rval = snd_soc_card_jack_new(
> > +                     card, "Headset Jack",
> > +                     SND_JACK_HEADSET |
> > +                     SND_JACK_HEADPHONE |
> > +                     SND_JACK_BTN_0 | SND_JACK_BTN_1 |
> > +                     SND_JACK_BTN_2 | SND_JACK_BTN_3,
> > +                     &pdata->jack, NULL, 0);
> > +
> > +     if (rval < 0) {
> > +             dev_err(card->dev, "Unable to add Headset Jack\n");
> > +             return rval;
> > +     }
> > +
> > +     jack = pdata->jack.jack;
> > +
> > +     snd_jack_set_key(jack, SND_JACK_BTN_0, KEY_PLAYPAUSE);
> > +     snd_jack_set_key(jack, SND_JACK_BTN_1, KEY_VOICECOMMAND);
> > +     snd_jack_set_key(jack, SND_JACK_BTN_2, KEY_VOLUMEUP);
> > +     snd_jack_set_key(jack, SND_JACK_BTN_3, KEY_VOLUMEDOWN);
> > +
> > +     jack->private_data = component;
> > +     jack->private_free = sc7180_jack_free;
> > +
> > +     rval = snd_soc_component_set_jack(component,
> > +                                       &pdata->jack, NULL);
> > +     if (rval != 0 && rval != -EOPNOTSUPP) {
> > +             dev_warn(card->dev, "Failed to set jack: %d\n", rval);
> > +             return rval;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +
> > +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> > +                                   SND_SOC_DAIFMT_NB_NF |
> > +                                   SND_SOC_DAIFMT_I2S;
> > +
> > +static int sc7180_snd_startup(struct snd_pcm_substream *substream)
> > +{
> > +     struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +     struct snd_soc_card *card = rtd->card;
> > +     struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);
> > +     struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> > +     struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > +     int ret;
> > +
> > +     switch (cpu_dai->id) {
> > +     case MI2S_PRIMARY:
> > +             if (++data->pri_mi2s_clk_count == 1) {
> > +                     snd_soc_dai_set_sysclk(cpu_dai,
> > +                                            LPASS_MCLK0,
> > +                                            DEFAULT_MCLK_RATE,
> > +                                            SNDRV_PCM_STREAM_PLAYBACK);
> > +             }
> > +             snd_soc_dai_set_fmt(codec_dai, primary_dai_fmt);
> > +
> > +             /* Configure PLL1 for codec */
> > +             ret = snd_soc_dai_set_pll(codec_dai, 0, RT5682_PLL1_S_MCLK,
> > +                                       DEFAULT_MCLK_RATE, RT5682_PLL1_FREQ);
> > +             if (ret < 0) {
> > +                     dev_err(rtd->dev, "can't set codec pll: %d\n", ret);
> > +                     return ret;
> > +             }
> > +
> > +             /* Configure sysclk for codec */
> > +             ret = snd_soc_dai_set_sysclk(codec_dai, RT5682_SCLK_S_PLL1,
> > +                                          RT5682_PLL1_FREQ,
> > +                                          SND_SOC_CLOCK_IN);
> > +             if (ret < 0)
> > +                     dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n",
> > +                             ret);
> > +
> > +             break;
> > +     case MI2S_SECONDARY:
> > +             break;
> > +     default:
> > +             dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> > +                     cpu_dai->id);
> > +             return -EINVAL;
> > +     }
> > +     return 0;
> > +}
> > +
> > +static void sc7180_snd_shutdown(struct snd_pcm_substream *substream)
> > +{
> > +     struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +     struct snd_soc_card *card = rtd->card;
> > +     struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);
> > +     struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> > +
> > +     switch (cpu_dai->id) {
> > +     case MI2S_PRIMARY:
> > +             if (--data->pri_mi2s_clk_count == 0) {
> > +                     snd_soc_dai_set_sysclk(cpu_dai,
> > +                                            LPASS_MCLK0,
> > +                                            0,
> > +                                            SNDRV_PCM_STREAM_PLAYBACK);
> > +             }
> > +             break;
> > +     case MI2S_SECONDARY:
> > +             break;
> > +     default:
> > +             dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> > +                     cpu_dai->id);
> > +             break;
> > +     }
> > +}
> > +
> > +static const struct snd_soc_ops sc7180_ops = {
> > +     .startup = sc7180_snd_startup,
> > +     .shutdown = sc7180_snd_shutdown,
> > +};
> > +
> > +static const struct snd_soc_dapm_widget sc7180_snd_widgets[] = {
> > +     SND_SOC_DAPM_HP("Headphone Jack", NULL),
> > +     SND_SOC_DAPM_MIC("Headset Mic", NULL),
> > +};
> > +
> > +static struct snd_soc_card sc7180_card = {
> > +     .owner = THIS_MODULE,
> > +     .aux_dev = &sc7180_headset_dev,
> > +     .num_aux_devs = 1,
> > +     .dapm_widgets = sc7180_snd_widgets,
> > +     .num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
> > +};
> > +
> > +static int sc7180_parse_of(struct snd_soc_card *card)
> > +{
>
> This code is getting duplicated in various places like
> apq8016_sbc_parse_of, it will be nice to common this up, if possible!
>
>
Thanks for the refactoring work of Stephan I can reuse qcom_snd_parse_of.

> > +     struct device_node *np;
> > +     struct device_node *codec = NULL;
> > +     struct device_node *platform = NULL;
> > +     struct device_node *cpu = NULL;
> > +     struct device *dev = card->dev;
> > +     struct snd_soc_dai_link *link;
> > +     struct of_phandle_args args;
> > +     struct snd_soc_dai_link_component *dlc;
> > +     int ret, num_links;
> > +
> > +     ret = snd_soc_of_parse_card_name(card, "model");
> > +     if (ret) {
> > +             dev_err(dev, "Error parsing card name: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     /* DAPM routes */
> > +     if (of_property_read_bool(dev->of_node, "audio-routing")) {
> > +             ret = snd_soc_of_parse_audio_routing(card,
> > +                                                  "audio-routing");
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     /* headset aux dev. */
> > +     sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> > +                     dev->of_node, "aux-dev", 0);
> > +     if (!sc7180_headset_dev.dlc.of_node) {
> > +             dev_err(dev,
> > +                     "Property 'aux-dev' missing/invalid\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     /* Populate links */
> > +     num_links = of_get_child_count(dev->of_node);
> > +
> > +     /* Allocate the DAI link array */
> > +     card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
> > +                                   GFP_KERNEL);
> > +     if (!card->dai_link)
> > +             return -ENOMEM;
> > +
> > +     card->num_links = num_links;
> > +     link = card->dai_link;
> > +
> > +     for_each_child_of_node(dev->of_node, np) {
> > +             dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
> > +             if (!dlc)
> > +                     return -ENOMEM;
> > +
> > +             link->cpus      = &dlc[0];
> > +             link->platforms = &dlc[1];
> > +
> > +             link->num_cpus          = 1;
> > +             link->num_platforms     = 1;
> > +
> > +             ret = of_property_read_string(np, "link-name", &link->name);
> > +             if (ret) {
> > +                     dev_err(card->dev,
> > +                             "error getting codec dai_link name\n");
> > +                     goto err;
> > +             }
> > +
> > +             link->playback_only = of_property_read_bool(np,
> > +                                                         "playback_only");
> > +             link->capture_only = of_property_read_bool(np,
> > +                                                        "capture_only");
>
> Does this even work??
> You have replaced "-" with "_" for property name?
>
This does not work because of the typo. But it is not needed so I
removed it in v3.
soc_new_pcm already takes care of capability in non-dpcm case.
> > +
> > +             if (link->playback_only && link->capture_only) {
> > +                     dev_err(card->dev,
> > +                             "getting both playback and capture only\n");
> > +                     goto err;
> > +             }
> > +
> > +             cpu = of_get_child_by_name(np, "cpu");
> > +             codec = of_get_child_by_name(np, "codec");
> > +
> > +             if (!cpu) {
> > +                     dev_err(dev, "%s: Can't find cpu DT node\n",
> > +                             link->name);
> > +                     ret = -EINVAL;
> > +                     goto err;
> > +             }
> > +
> > +             ret = of_parse_phandle_with_args(cpu, "sound-dai",
> > +                                              "#sound-dai-cells", 0, &args);
> > +             if (ret) {
> > +                     dev_err(card->dev, "%s: error getting cpu phandle\n",
> > +                             link->name);
> > +                     goto err;
> > +             }
> > +             link->cpus->of_node = args.np;
> > +             link->id = args.args[0];
> > +
> > +             ret = snd_soc_of_get_dai_name(cpu, &link->cpus->dai_name);
> > +             if (ret) {
> > +                     dev_err(card->dev, "%s: error getting cpu dai name\n",
> > +                             link->name);
> > +                     goto err;
> > +             }
> > +
> > +             if (codec) {
> > +                     ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
> > +                     if (ret < 0) {
> > +                             dev_err(card->dev, "%s: codec dai not found\n",
> > +                                     link->name);
> > +                             goto err;
> > +                     }
> > +             } else {
> > +                     dlc = devm_kzalloc(dev, sizeof(*dlc), GFP_KERNEL);
> > +                     if (!dlc)
> > +                             return -ENOMEM;
> > +
> > +                     link->codecs     = dlc;
> > +                     link->num_codecs = 1;
> > +
> > +                     link->codecs->dai_name = "snd-soc-dummy-dai";
> > +                     link->codecs->name = "snd-soc-dummy";
> > +             }
> > +
> > +             link->platforms->of_node = link->cpus->of_node;
> > +             link->stream_name = link->name;
> > +             link->ops = &sc7180_ops;
> > +             link++;
> > +
> > +             of_node_put(cpu);
> > +             of_node_put(codec);
> > +     }
> > +
> > +     return 0;
> > +err:
> > +     of_node_put(np);
> > +     of_node_put(cpu);
> > +     of_node_put(codec);
> > +     of_node_put(platform);
> > +     return ret;
> > +}
> > +
> > +static int sc7180_snd_platform_probe(struct platform_device *pdev)
> > +{
> > +     struct snd_soc_card *card;
> > +     struct sc7180_snd_data *data;
> > +     struct device *dev = &pdev->dev;
> > +     int ret;
> > +
> > +     card = &sc7180_card;
> > +
> > +     /* Allocate the private data */
> > +     data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     card->dapm_widgets = sc7180_snd_widgets;
> > +     card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
> > +     card->dev = dev;
> > +     dev_set_drvdata(dev, card);
> > +     ret = sc7180_parse_of(card);
> > +     if (ret) {
> > +             dev_err(dev, "Error parsing OF data\n");
> > +             return ret;
> > +     }
> > +
> > +     data->card = card;
> > +     snd_soc_card_set_drvdata(card, data);
> > +
> > +     ret = snd_soc_register_card(card);
>
> devm_snd_soc_register_card()??
>
Fixed in v3.

> > +     if (ret) {
> > +             dev_err(dev, "Sound card registration failed\n");
> > +             return ret;
> > +     }
> > +     return ret;
> > +}
> > +
> > +static int sc7180_snd_platform_remove(struct platform_device *pdev)
> > +{
> > +     struct snd_soc_card *card = dev_get_drvdata(&pdev->dev);
> > +
> > +     snd_soc_unregister_card(card);
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id sc7180_snd_device_id[]  = {
> > +     { .compatible = "qcom,sc7180-sndcard" },
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, sc7180_snd_device_id);
> > +
> > +static struct platform_driver sc7180_snd_driver = {
> > +     .probe = sc7180_snd_platform_probe,
> > +     .remove = sc7180_snd_platform_remove,
> > +     .driver = {
> > +             .name = "msm-snd-sc7180",
> > +             .of_match_table = sc7180_snd_device_id,
> > +     },
> > +};
> > +module_platform_driver(sc7180_snd_driver);
> > +
> > +MODULE_DESCRIPTION("sc7180 ASoC Machine Driver");
> > +MODULE_LICENSE("GPL v2");
> >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
  2020-07-23 18:44     ` Stephan Gerhold
@ 2020-07-31  8:55       ` Cheng-yi Chiang
  0 siblings, 0 replies; 11+ messages in thread
From: Cheng-yi Chiang @ 2020-07-31  8:55 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: Srinivas Kandagatla, linux-kernel, Mark Brown, Taniya Das,
	Rohit kumar, Banajit Goswami, Patrick Lai, Andy Gross,
	Bjorn Andersson, Liam Girdwood, Rob Herring, Jaroslav Kysela,
	Takashi Iwai, Doug Anderson, Dylan Reid, Tzung-Bi Shih,
	Linux ARM, linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	Ajit Pandey

On Fri, Jul 24, 2020 at 2:45 AM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Wed, Jul 22, 2020 at 10:25:14AM +0100, Srinivas Kandagatla wrote:
> > > +static int sc7180_parse_of(struct snd_soc_card *card)
> > > +{
> >
> > This code is getting duplicated in various places like apq8016_sbc_parse_of,
> > it will be nice to common this up, if possible!
> >
>
> FYI, I started work on making apq8016_sbc use qcom_snd_parse_of()
> a while ago already, but didn't find the time to finish it up.
> I have now sent it, this should make it possible to use the common
> qcom_snd_parse_of() function in this driver as well.
>
> See: https://lore.kernel.org/alsa-devel/20200723183904.321040-1-stephan@gerhold.net/
>
> Stephan
>
Hi Stephan, thanks a lot for jumping on this to help.
It indeed makes this new driver much cleaner.
I have tested with your patches and it works great.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 10:44 [PATCH v2 0/2] Add documentation and machine driver for SC7180 sound card Cheng-Yi Chiang
2020-07-21 10:44 ` [PATCH v2 1/2] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
2020-07-22  9:25   ` Srinivas Kandagatla
2020-07-31  8:55     ` Cheng-yi Chiang
2020-07-21 10:44 ` [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
2020-07-22  3:15   ` Tzung-Bi Shih
2020-07-31  8:55     ` Cheng-yi Chiang
2020-07-22  9:25   ` Srinivas Kandagatla
2020-07-23 18:44     ` Stephan Gerhold
2020-07-31  8:55       ` Cheng-yi Chiang
2020-07-31  8:55     ` Cheng-yi Chiang

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git