Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v11 0/3] Add documentation and machine driver for SC7180 sound card
@ 2020-09-14  8:06 Cheng-Yi Chiang
  2020-09-14  8:06 ` [PATCH v11 1/3] ASoC: hdmi-codec: Use set_jack ops to set jack Cheng-Yi Chiang
                   ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: Cheng-Yi Chiang @ 2020-09-14  8:06 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, Srini Kandagatla,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	dianders, dgreid, tzungbi, linux-arm-kernel, linux-arm-msm,
	devicetree, alsa-devel, linux-mediatek, linux-rockchip,
	Cheng-Yi Chiang

Note:
- 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.
- This patch series needs HDMI DAI name SC7180_LPASS_DP defined in sc7180-lpass.h.
  It will be posted in the newer patchset of https://patchwork.kernel.org/patch/11745565/

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.

Changes from v2 to v3:
- Ducumentation: Addressed suggestions from Srini.
- Machine driver:
  - Reuse qcom_snd_parse_of to parse properties.
  - Remove playback-only and capture-only.
  - Misc fixes to address comments.

Changes from v3 to v4:
- Ducumentation: Addressed suggestions from Rob.
 - Remove definition of dai.
 - Use 'sound-dai: true' for sound-dai schema.
 - Add reg property to pass 'make dt_binding_check' check although reg is not used in the driver.
- Machine driver:
 - Add Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>

Changes from v4 to v5:
- Documentation: Addressed suggestions from Rob.
 - Add definition for "#address-cells" and "#size-cells".
 - Add additionalProperties: false
 - Add required properties.

Changes from v5 to v6:
- Documentation: Addressed suggestions from Rob.
 - Drop contains in compatible strings.
 - Only allow dai-link@[0-9]
 - Remove reg ref since it has a type definition already.

Changes from v6 to v7
- Documentation:
  - Add headset-jack and hdmi-jack to specify the codec
    responsible for jack detection.
- HDMI codec driver:
  - Use component set_jack ops instead of exporting hdmi_codec_set_jack_detect.
- Machine driver:
  - Removed aux device following Stephan's suggestion.
  - Use headset-jack and hdmi-jack to specify the codec
    responsible for jack detection.
  - Add support for HDMI(actually DP) playback.

Changes from v7 to v8
- Documentation:
  - Remove headset-jack and hdmi-jack.
- Machine driver:
  - Let machine driver decide whether there is a jack on the DAI.

Changes from v8 to v9
- hdmi-codec driver:
  - Fixed the naming.
- Machine driver:
  - Fixed unused fields.
  - Moved snd_soc_card_set_drvdata
  - Keep the naming of HDMI as dai name until v5 of lpass-hdmi patches.

Changes from v9 to v10
- Documentation:
  - Let compatible string be more specific for board configuration to allow
    for future changes.
- Machine driver:
  - Fixed unused include and macro.
  - Add temporary macro SC7180_LPASS_DP for future change in sc7180-lpass.h.
  - Let sound card be dynamically allocated.
  - Change compatible string accordingly.

Changes from v10 to v11
- Machine driver:
  - Use temporary macro LPASS_DP_RX for future change in sc7180-lpass.h.

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

Cheng-Yi Chiang (2):
  ASoC: hdmi-codec: Use set_jack ops to set jack
  ASoC: qcom: dt-bindings: Add sc7180 machine bindings

 .../bindings/sound/qcom,sc7180.yaml           | 130 +++++++++
 include/sound/hdmi-codec.h                    |   3 -
 sound/soc/codecs/hdmi-codec.c                 |  12 +-
 sound/soc/mediatek/mt8173/mt8173-rt5650.c     |   5 +-
 .../mediatek/mt8183/mt8183-da7219-max98357.c  |   5 +-
 .../mt8183/mt8183-mt6358-ts3a227-max98357.c   |   5 +-
 sound/soc/qcom/Kconfig                        |  12 +
 sound/soc/qcom/Makefile                       |   2 +
 sound/soc/qcom/sc7180.c                       | 266 ++++++++++++++++++
 sound/soc/rockchip/rockchip_max98090.c        |   3 +-
 10 files changed, 421 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,sc7180.yaml
 create mode 100644 sound/soc/qcom/sc7180.c

-- 
2.28.0.618.gf4bc123cb7-goog


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

* [PATCH v11 1/3] ASoC: hdmi-codec: Use set_jack ops to set jack
  2020-09-14  8:06 [PATCH v11 0/3] Add documentation and machine driver for SC7180 sound card Cheng-Yi Chiang
@ 2020-09-14  8:06 ` Cheng-Yi Chiang
  2020-09-14  8:06 ` [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
  2020-09-14  8:06 ` [PATCH v11 3/3] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
  2 siblings, 0 replies; 23+ messages in thread
From: Cheng-Yi Chiang @ 2020-09-14  8:06 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, Srini Kandagatla,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	dianders, dgreid, tzungbi, linux-arm-kernel, linux-arm-msm,
	devicetree, alsa-devel, linux-mediatek, linux-rockchip,
	Cheng-Yi Chiang, Tzung-Bi Shih

Use set_jack ops to set jack so machine drivers do not need to include
hdmi-codec.h explicitly.

Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>
---
 include/sound/hdmi-codec.h                           |  3 ---
 sound/soc/codecs/hdmi-codec.c                        | 12 ++++--------
 sound/soc/mediatek/mt8173/mt8173-rt5650.c            |  5 ++---
 sound/soc/mediatek/mt8183/mt8183-da7219-max98357.c   |  5 ++---
 .../mediatek/mt8183/mt8183-mt6358-ts3a227-max98357.c |  5 ++---
 sound/soc/rockchip/rockchip_max98090.c               |  3 +--
 6 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
index 7754631a3102..b55970859a13 100644
--- a/include/sound/hdmi-codec.h
+++ b/include/sound/hdmi-codec.h
@@ -117,9 +117,6 @@ struct hdmi_codec_pdata {
 struct snd_soc_component;
 struct snd_soc_jack;
 
-int hdmi_codec_set_jack_detect(struct snd_soc_component *component,
-			       struct snd_soc_jack *jack);
-
 #define HDMI_CODEC_DRV_NAME "hdmi-audio-codec"
 
 #endif /* __HDMI_CODEC_H__ */
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index 8c6f540533ba..403d4c6a49a8 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -698,13 +698,9 @@ static void plugged_cb(struct device *dev, bool plugged)
 		hdmi_codec_jack_report(hcp, 0);
 }
 
-/**
- * hdmi_codec_set_jack_detect - register HDMI plugged callback
- * @component: the hdmi-codec instance
- * @jack: ASoC jack to report (dis)connection events on
- */
-int hdmi_codec_set_jack_detect(struct snd_soc_component *component,
-			       struct snd_soc_jack *jack)
+static int hdmi_codec_set_jack(struct snd_soc_component *component,
+			       struct snd_soc_jack *jack,
+			       void *data)
 {
 	struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
 	int ret = -EOPNOTSUPP;
@@ -720,7 +716,6 @@ int hdmi_codec_set_jack_detect(struct snd_soc_component *component,
 	}
 	return ret;
 }
-EXPORT_SYMBOL_GPL(hdmi_codec_set_jack_detect);
 
 static int hdmi_dai_spdif_probe(struct snd_soc_dai *dai)
 {
@@ -806,6 +801,7 @@ static const struct snd_soc_component_driver hdmi_driver = {
 	.use_pmdown_time	= 1,
 	.endianness		= 1,
 	.non_legacy_dai_naming	= 1,
+	.set_jack		= hdmi_codec_set_jack,
 };
 
 static int hdmi_codec_probe(struct platform_device *pdev)
diff --git a/sound/soc/mediatek/mt8173/mt8173-rt5650.c b/sound/soc/mediatek/mt8173/mt8173-rt5650.c
index 347b095d478d..c28ebf891cb0 100644
--- a/sound/soc/mediatek/mt8173/mt8173-rt5650.c
+++ b/sound/soc/mediatek/mt8173/mt8173-rt5650.c
@@ -11,7 +11,6 @@
 #include <linux/of_gpio.h>
 #include <sound/soc.h>
 #include <sound/jack.h>
-#include <sound/hdmi-codec.h>
 #include "../../codecs/rt5645.h"
 
 #define MCLK_FOR_CODECS		12288000
@@ -154,8 +153,8 @@ static int mt8173_rt5650_hdmi_init(struct snd_soc_pcm_runtime *rtd)
 	if (ret)
 		return ret;
 
-	return hdmi_codec_set_jack_detect(asoc_rtd_to_codec(rtd, 0)->component,
-					  &mt8173_rt5650_hdmi_jack);
+	return snd_soc_component_set_jack(asoc_rtd_to_codec(rtd, 0)->component,
+					  &mt8173_rt5650_hdmi_jack, NULL);
 }
 
 enum {
diff --git a/sound/soc/mediatek/mt8183/mt8183-da7219-max98357.c b/sound/soc/mediatek/mt8183/mt8183-da7219-max98357.c
index 06d0a4f80fc1..e53c6200d5a7 100644
--- a/sound/soc/mediatek/mt8183/mt8183-da7219-max98357.c
+++ b/sound/soc/mediatek/mt8183/mt8183-da7219-max98357.c
@@ -10,7 +10,6 @@
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/pinctrl/consumer.h>
-#include <sound/hdmi-codec.h>
 #include <sound/jack.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -369,8 +368,8 @@ static int mt8183_da7219_max98357_hdmi_init(struct snd_soc_pcm_runtime *rtd)
 	if (ret)
 		return ret;
 
-	return hdmi_codec_set_jack_detect(asoc_rtd_to_codec(rtd, 0)->component,
-					  &priv->hdmi_jack);
+	return snd_soc_component_set_jack(asoc_rtd_to_codec(rtd, 0)->component,
+					  &priv->hdmi_jack, NULL);
 }
 
 static struct snd_soc_dai_link mt8183_da7219_dai_links[] = {
diff --git a/sound/soc/mediatek/mt8183/mt8183-mt6358-ts3a227-max98357.c b/sound/soc/mediatek/mt8183/mt8183-mt6358-ts3a227-max98357.c
index 07410d7afaa9..327dfad41e31 100644
--- a/sound/soc/mediatek/mt8183/mt8183-mt6358-ts3a227-max98357.c
+++ b/sound/soc/mediatek/mt8183/mt8183-mt6358-ts3a227-max98357.c
@@ -9,7 +9,6 @@
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/pinctrl/consumer.h>
-#include <sound/hdmi-codec.h>
 #include <sound/jack.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -350,8 +349,8 @@ mt8183_mt6358_ts3a227_max98357_hdmi_init(struct snd_soc_pcm_runtime *rtd)
 	if (ret)
 		return ret;
 
-	return hdmi_codec_set_jack_detect(asoc_rtd_to_codec(rtd, 0)->component,
-					  &priv->hdmi_jack);
+	return snd_soc_component_set_jack(asoc_rtd_to_codec(rtd, 0)->component,
+					  &priv->hdmi_jack, NULL);
 }
 
 static struct snd_soc_dai_link mt8183_mt6358_ts3a227_dai_links[] = {
diff --git a/sound/soc/rockchip/rockchip_max98090.c b/sound/soc/rockchip/rockchip_max98090.c
index 9acfd024aa5d..c8f1a28a92b7 100644
--- a/sound/soc/rockchip/rockchip_max98090.c
+++ b/sound/soc/rockchip/rockchip_max98090.c
@@ -12,7 +12,6 @@
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
 #include <sound/core.h>
-#include <sound/hdmi-codec.h>
 #include <sound/jack.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -238,7 +237,7 @@ static int rk_hdmi_init(struct snd_soc_pcm_runtime *runtime)
 		return ret;
 	}
 
-	return hdmi_codec_set_jack_detect(component, &rk_hdmi_jack);
+	return snd_soc_component_set_jack(component, &rk_hdmi_jack, NULL);
 }
 
 /* max98090 dai_link */
-- 
2.28.0.618.gf4bc123cb7-goog


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

* [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-09-14  8:06 [PATCH v11 0/3] Add documentation and machine driver for SC7180 sound card Cheng-Yi Chiang
  2020-09-14  8:06 ` [PATCH v11 1/3] ASoC: hdmi-codec: Use set_jack ops to set jack Cheng-Yi Chiang
@ 2020-09-14  8:06 ` Cheng-Yi Chiang
  2020-09-14 17:48   ` Rob Herring
  2020-10-13 10:36   ` Srinivas Kandagatla
  2020-09-14  8:06 ` [PATCH v11 3/3] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
  2 siblings, 2 replies; 23+ messages in thread
From: Cheng-Yi Chiang @ 2020-09-14  8:06 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, Srini Kandagatla,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	dianders, dgreid, tzungbi, linux-arm-kernel, linux-arm-msm,
	devicetree, alsa-devel, linux-mediatek, linux-rockchip,
	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..b77311bb5190
--- /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.
+
+properties:
+  compatible:
+    const: qcom,sc7180-sndcard-rt5682-m98357-1mic
+
+  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
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+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.
+
+      cpu:
+        description: Holds subnode which indicates cpu dai.
+        type: object
+        properties:
+          sound-dai: true
+
+      codec:
+        description: Holds subnode which indicates codec dai.
+        type: object
+        properties:
+          sound-dai: true
+
+    required:
+      - link-name
+      - cpu
+      - codec
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - model
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+
+  - |
+    sound {
+        compatible = "qcom,sc7180-sndcard-rt5682-m98357-1mic";
+        model = "sc7180-snd-card";
+
+        audio-routing =
+                    "Headphone Jack", "HPOL",
+                    "Headphone Jack", "HPOR";
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        dai-link@0 {
+            link-name = "MultiMedia0";
+            reg = <0>;
+            cpu {
+                sound-dai = <&lpass_cpu 0>;
+            };
+
+            codec {
+                sound-dai = <&alc5682 0>;
+            };
+        };
+
+        dai-link@1 {
+            link-name = "MultiMedia1";
+            reg = <1>;
+            cpu {
+                sound-dai = <&lpass_cpu 1>;
+            };
+
+            codec {
+                sound-dai = <&max98357a>;
+            };
+        };
+
+        dai-link@2 {
+            link-name = "MultiMedia2";
+            reg = <2>;
+            cpu {
+                sound-dai = <&lpass_hdmi 0>;
+            };
+
+            codec {
+                sound-dai = <&msm_dp>;
+            };
+        };
+    };
-- 
2.28.0.618.gf4bc123cb7-goog


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

* [PATCH v11 3/3] ASoC: qcom: sc7180: Add machine driver for sound card registration
  2020-09-14  8:06 [PATCH v11 0/3] Add documentation and machine driver for SC7180 sound card Cheng-Yi Chiang
  2020-09-14  8:06 ` [PATCH v11 1/3] ASoC: hdmi-codec: Use set_jack ops to set jack Cheng-Yi Chiang
  2020-09-14  8:06 ` [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
@ 2020-09-14  8:06 ` Cheng-Yi Chiang
  2020-10-13  3:50   ` Cheng-yi Chiang
  2 siblings, 1 reply; 23+ messages in thread
From: Cheng-Yi Chiang @ 2020-09-14  8:06 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, Srini Kandagatla,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	dianders, dgreid, tzungbi, linux-arm-kernel, linux-arm-msm,
	devicetree, alsa-devel, linux-mediatek, linux-rockchip,
	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: Ajit Pandey <ajitp@codeaurora.org>
Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
---
 sound/soc/qcom/Kconfig  |  12 ++
 sound/soc/qcom/Makefile |   2 +
 sound/soc/qcom/sc7180.c | 266 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 280 insertions(+)
 create mode 100644 sound/soc/qcom/sc7180.c

diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
index a607ace8b089..0459185ee243 100644
--- a/sound/soc/qcom/Kconfig
+++ b/sound/soc/qcom/Kconfig
@@ -116,4 +116,16 @@ config SND_SOC_SDM845
 	  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 I2C
+	select SND_SOC_QCOM_COMMON
+	select SND_SOC_LPASS_SC7180
+	select SND_SOC_MAX98357A
+	select SND_SOC_RT5682_I2C
+	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.
+
 endif #SND_SOC_QCOM
diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
index 7972c9479ab0..0cdcbf367ef1 100644
--- a/sound/soc/qcom/Makefile
+++ b/sound/soc/qcom/Makefile
@@ -17,12 +17,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..0e90448523b0
--- /dev/null
+++ b/sound/soc/qcom/sc7180.c
@@ -0,0 +1,266 @@
+// 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/soc.h>
+#include <uapi/linux/input-event-codes.h>
+
+#include "../codecs/rt5682.h"
+#include "common.h"
+#include "lpass.h"
+
+#define DEFAULT_MCLK_RATE		19200000
+#define RT5682_PLL1_FREQ (48000 * 512)
+
+// This will be defined in include/dt-bindings/sound/sc7180-lpass.h
+#define LPASS_DP_RX 2
+
+struct sc7180_snd_data {
+	struct snd_soc_card card;
+	u32 pri_mi2s_clk_count;
+	struct snd_soc_jack hs_jack;
+	struct snd_soc_jack hdmi_jack;
+};
+
+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_pcm_runtime *rtd)
+{
+	struct snd_soc_card *card = rtd->card;
+	struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
+	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
+	struct snd_soc_component *component = codec_dai->component;
+	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->hs_jack, NULL, 0);
+
+	if (rval < 0) {
+		dev_err(card->dev, "Unable to add Headset Jack\n");
+		return rval;
+	}
+
+	jack = pdata->hs_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;
+
+	return snd_soc_component_set_jack(component, &pdata->hs_jack, NULL);
+}
+
+static int sc7180_hdmi_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_card *card = rtd->card;
+	struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
+	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
+	struct snd_soc_component *component = codec_dai->component;
+	struct snd_jack *jack;
+	int rval;
+
+	rval = snd_soc_card_jack_new(
+			card, "HDMI Jack",
+			SND_JACK_LINEOUT,
+			&pdata->hdmi_jack, NULL, 0);
+
+	if (rval < 0) {
+		dev_err(card->dev, "Unable to add HDMI Jack\n");
+		return rval;
+	}
+
+	jack = pdata->hdmi_jack.jack;
+	jack->private_data = component;
+	jack->private_free = sc7180_jack_free;
+
+	return snd_soc_component_set_jack(component, &pdata->hdmi_jack, NULL);
+}
+
+static int sc7180_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+
+	switch (cpu_dai->id) {
+	case MI2S_PRIMARY:
+		return sc7180_headset_init(rtd);
+	case MI2S_SECONDARY:
+		return 0;
+	case LPASS_DP_RX:
+		return sc7180_hdmi_init(rtd);
+	default:
+		dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
+			cpu_dai->id);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+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,
+				    SND_SOC_DAIFMT_CBS_CFS |
+				    SND_SOC_DAIFMT_NB_NF |
+				    SND_SOC_DAIFMT_I2S);
+
+		/* 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) {
+			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)
+			dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n",
+				ret);
+
+		break;
+	case MI2S_SECONDARY:
+		break;
+	case LPASS_DP_RX:
+		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;
+	case LPASS_DP_RX:
+		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 void sc7180_add_ops(struct snd_soc_card *card)
+{
+	struct snd_soc_dai_link *link;
+	int i;
+
+	for_each_card_prelinks(card, i, link) {
+		link->ops = &sc7180_ops;
+		link->init = sc7180_init;
+	}
+}
+
+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;
+
+	/* Allocate the private data */
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	card = &data->card;
+	snd_soc_card_set_drvdata(card, data);
+
+	card->owner = THIS_MODULE,
+	card->dev = dev;
+	card->dapm_widgets = sc7180_snd_widgets;
+	card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
+
+	ret = qcom_snd_parse_of(card);
+	if (ret)
+		return ret;
+
+	sc7180_add_ops(card);
+
+	return devm_snd_soc_register_card(dev, card);
+}
+
+static const struct of_device_id sc7180_snd_device_id[]  = {
+	{ .compatible = "qcom,sc7180-sndcard-rt5682-m98357-1mic" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sc7180_snd_device_id);
+
+static struct platform_driver sc7180_snd_driver = {
+	.probe = sc7180_snd_platform_probe,
+	.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.618.gf4bc123cb7-goog


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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-09-14  8:06 ` [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
@ 2020-09-14 17:48   ` Rob Herring
  2020-09-15 12:44     ` Cheng-yi Chiang
  2020-10-13 10:36   ` Srinivas Kandagatla
  1 sibling, 1 reply; 23+ messages in thread
From: Rob Herring @ 2020-09-14 17:48 UTC (permalink / raw)
  To: Cheng-Yi Chiang
  Cc: linux-kernel, dgreid, Bjorn Andersson, Takashi Iwai,
	linux-rockchip, alsa-devel, Patrick Lai, tzungbi,
	linux-arm-kernel, Mark Brown, Srinivasa Rao, Rob Herring,
	devicetree, Heiko Stuebner, linux-arm-msm, Matthias Brugger,
	Stephan Gerhold, dianders, Banajit Goswami, Liam Girdwood,
	Rohit kumar, Andy Gross, linux-mediatek, Taniya Das,
	Srini Kandagatla

On Mon, 14 Sep 2020 16:06:18 +0800, 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
> 


Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.


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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-09-14 17:48   ` Rob Herring
@ 2020-09-15 12:44     ` Cheng-yi Chiang
  2020-10-13  3:46       ` Cheng-yi Chiang
  0 siblings, 1 reply; 23+ messages in thread
From: Cheng-yi Chiang @ 2020-09-15 12:44 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, Dylan Reid, Bjorn Andersson, Takashi Iwai,
	open list:ARM/Rockchip SoC...,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	Patrick Lai, Tzung-Bi Shih, Linux ARM, Mark Brown, Srinivasa Rao,
	Rob Herring,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Heiko Stuebner, linux-arm-msm, Matthias Brugger, Stephan Gerhold,
	Doug Anderson, Banajit Goswami, Liam Girdwood, Rohit kumar,
	Andy Gross, moderated list:ARM/Mediatek SoC support, Taniya Das,
	Srini Kandagatla

On Tue, Sep 15, 2020 at 1:48 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, 14 Sep 2020 16:06:18 +0800, 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
> >
>
>
> Please add Acked-by/Reviewed-by tags when posting new versions. However,
> there's no need to repost patches *only* to add the tags. The upstream
> maintainer will do that for acks received on the version they apply.
>
> If a tag was not added on purpose, please state why and what changed.
>

Hi Rob,
There was a change between v9 and v10 on compatible string so I did
not add your Reviewed-by.
Now it is "qcom,sc7180-sndcard-rt5682-m98357-1mic" following Stephan's
comment in

https://patchwork.kernel.org/comment/23608881/

to make compatible string more specific to board configuration.
I only add the note to the cover letter. Sorry the cover letter became
too long to follow.
I will add the note in patch mail itself for future changes.
Thanks for taking a look again.

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-09-15 12:44     ` Cheng-yi Chiang
@ 2020-10-13  3:46       ` Cheng-yi Chiang
  0 siblings, 0 replies; 23+ messages in thread
From: Cheng-yi Chiang @ 2020-10-13  3:46 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, Dylan Reid, Bjorn Andersson, Takashi Iwai,
	open list:ARM/Rockchip SoC...,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	Patrick Lai, Tzung-Bi Shih, Linux ARM, Mark Brown, Srinivasa Rao,
	Rob Herring,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Heiko Stuebner, linux-arm-msm, Matthias Brugger, Stephan Gerhold,
	Doug Anderson, Banajit Goswami, Liam Girdwood, Rohit kumar,
	Andy Gross, moderated list:ARM/Mediatek SoC support, Taniya Das,
	Srini Kandagatla

On Tue, Sep 15, 2020 at 8:44 PM Cheng-yi Chiang <cychiang@chromium.org> wrote:
>
> On Tue, Sep 15, 2020 at 1:48 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, 14 Sep 2020 16:06:18 +0800, 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
> > >
> >
> >
> > Please add Acked-by/Reviewed-by tags when posting new versions. However,
> > there's no need to repost patches *only* to add the tags. The upstream
> > maintainer will do that for acks received on the version they apply.
> >
> > If a tag was not added on purpose, please state why and what changed.
> >
>
> Hi Rob,
> There was a change between v9 and v10 on compatible string so I did
> not add your Reviewed-by.
> Now it is "qcom,sc7180-sndcard-rt5682-m98357-1mic" following Stephan's
> comment in
>
> https://patchwork.kernel.org/comment/23608881/
>
> to make compatible string more specific to board configuration.
> I only add the note to the cover letter. Sorry the cover letter became
> too long to follow.
> I will add the note in patch mail itself for future changes.
> Thanks for taking a look again.


Hi Rob,
Could you please kindly review this patch ?

v11 contains the compatible string  "qcom,sc7180-sndcard-rt5682-m98357-1mic"
following Stephan's suggestion in

"[v9,3/3] ASoC: qcom: sc7180: Add machine driver for sound card registration"
( https://patchwork.kernel.org/patch/11770201/#23608881 )

to specify the combination of components.

Please let me know if this looks good to you.
Thanks!

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

* Re: [PATCH v11 3/3] ASoC: qcom: sc7180: Add machine driver for sound card registration
  2020-09-14  8:06 ` [PATCH v11 3/3] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
@ 2020-10-13  3:50   ` Cheng-yi Chiang
  2020-10-15  8:02     ` Cheng-yi Chiang
  0 siblings, 1 reply; 23+ messages in thread
From: Cheng-yi Chiang @ 2020-10-13  3:50 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, Srini Kandagatla,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	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...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajit Pandey

On Mon, Sep 14, 2020 at 4:06 PM Cheng-Yi Chiang <cychiang@chromium.org> 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: Ajit Pandey <ajitp@codeaurora.org>
> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> ---
>  sound/soc/qcom/Kconfig  |  12 ++
>  sound/soc/qcom/Makefile |   2 +
>  sound/soc/qcom/sc7180.c | 266 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 280 insertions(+)
>  create mode 100644 sound/soc/qcom/sc7180.c
>
> diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
> index a607ace8b089..0459185ee243 100644
> --- a/sound/soc/qcom/Kconfig
> +++ b/sound/soc/qcom/Kconfig
> @@ -116,4 +116,16 @@ config SND_SOC_SDM845
>           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 I2C
> +       select SND_SOC_QCOM_COMMON
> +       select SND_SOC_LPASS_SC7180
> +       select SND_SOC_MAX98357A
> +       select SND_SOC_RT5682_I2C
> +       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.
> +
>  endif #SND_SOC_QCOM
> diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
> index 7972c9479ab0..0cdcbf367ef1 100644
> --- a/sound/soc/qcom/Makefile
> +++ b/sound/soc/qcom/Makefile
> @@ -17,12 +17,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..0e90448523b0
> --- /dev/null
> +++ b/sound/soc/qcom/sc7180.c
> @@ -0,0 +1,266 @@
> +// 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/soc.h>
> +#include <uapi/linux/input-event-codes.h>
> +
> +#include "../codecs/rt5682.h"
> +#include "common.h"
> +#include "lpass.h"
> +
> +#define DEFAULT_MCLK_RATE              19200000
> +#define RT5682_PLL1_FREQ (48000 * 512)
> +
> +// This will be defined in include/dt-bindings/sound/sc7180-lpass.h
> +#define LPASS_DP_RX 2
> +
> +struct sc7180_snd_data {
> +       struct snd_soc_card card;
> +       u32 pri_mi2s_clk_count;
> +       struct snd_soc_jack hs_jack;
> +       struct snd_soc_jack hdmi_jack;
> +};
> +
> +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_pcm_runtime *rtd)
> +{
> +       struct snd_soc_card *card = rtd->card;
> +       struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> +       struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> +       struct snd_soc_component *component = codec_dai->component;
> +       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->hs_jack, NULL, 0);
> +
> +       if (rval < 0) {
> +               dev_err(card->dev, "Unable to add Headset Jack\n");
> +               return rval;
> +       }
> +
> +       jack = pdata->hs_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;
> +
> +       return snd_soc_component_set_jack(component, &pdata->hs_jack, NULL);
> +}
> +
> +static int sc7180_hdmi_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +       struct snd_soc_card *card = rtd->card;
> +       struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> +       struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> +       struct snd_soc_component *component = codec_dai->component;
> +       struct snd_jack *jack;
> +       int rval;
> +
> +       rval = snd_soc_card_jack_new(
> +                       card, "HDMI Jack",
> +                       SND_JACK_LINEOUT,
> +                       &pdata->hdmi_jack, NULL, 0);
> +
> +       if (rval < 0) {
> +               dev_err(card->dev, "Unable to add HDMI Jack\n");
> +               return rval;
> +       }
> +
> +       jack = pdata->hdmi_jack.jack;
> +       jack->private_data = component;
> +       jack->private_free = sc7180_jack_free;
> +
> +       return snd_soc_component_set_jack(component, &pdata->hdmi_jack, NULL);
> +}
> +
> +static int sc7180_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +       struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> +
> +       switch (cpu_dai->id) {
> +       case MI2S_PRIMARY:
> +               return sc7180_headset_init(rtd);
> +       case MI2S_SECONDARY:
> +               return 0;
> +       case LPASS_DP_RX:
> +               return sc7180_hdmi_init(rtd);
> +       default:
> +               dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> +                       cpu_dai->id);
> +               return -EINVAL;
> +       }
> +       return 0;
> +}
> +
> +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,
> +                                   SND_SOC_DAIFMT_CBS_CFS |
> +                                   SND_SOC_DAIFMT_NB_NF |
> +                                   SND_SOC_DAIFMT_I2S);
> +
> +               /* 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) {
> +                       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)
> +                       dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n",
> +                               ret);
> +
> +               break;
> +       case MI2S_SECONDARY:
> +               break;
> +       case LPASS_DP_RX:
> +               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;
> +       case LPASS_DP_RX:
> +               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 void sc7180_add_ops(struct snd_soc_card *card)
> +{
> +       struct snd_soc_dai_link *link;
> +       int i;
> +
> +       for_each_card_prelinks(card, i, link) {
> +               link->ops = &sc7180_ops;
> +               link->init = sc7180_init;
> +       }
> +}
> +
> +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;
> +
> +       /* Allocate the private data */
> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       card = &data->card;
> +       snd_soc_card_set_drvdata(card, data);
> +
> +       card->owner = THIS_MODULE,
> +       card->dev = dev;
> +       card->dapm_widgets = sc7180_snd_widgets;
> +       card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
> +
> +       ret = qcom_snd_parse_of(card);
> +       if (ret)
> +               return ret;
> +
> +       sc7180_add_ops(card);
> +
> +       return devm_snd_soc_register_card(dev, card);
> +}
> +
> +static const struct of_device_id sc7180_snd_device_id[]  = {
> +       { .compatible = "qcom,sc7180-sndcard-rt5682-m98357-1mic" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, sc7180_snd_device_id);
> +
> +static struct platform_driver sc7180_snd_driver = {
> +       .probe = sc7180_snd_platform_probe,
> +       .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.618.gf4bc123cb7-goog
>

Hi Srini and Stephan,
May I get your Reviewed-By for this patch ?
There are some patches on the machine driver for different
configurations pending on this one so I would hope to get this one
merged first.

Thanks a lot for reviewing it!

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-09-14  8:06 ` [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
  2020-09-14 17:48   ` Rob Herring
@ 2020-10-13 10:36   ` Srinivas Kandagatla
  2020-10-15  7:59     ` Cheng-yi Chiang
  1 sibling, 1 reply; 23+ messages in thread
From: Srinivas Kandagatla @ 2020-10-13 10:36 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, Stephan Gerhold,
	Matthias Brugger, Heiko Stuebner, Srinivasa Rao, dianders,
	dgreid, tzungbi, linux-arm-kernel, linux-arm-msm, devicetree,
	alsa-devel, linux-mediatek, linux-rockchip

Hi Cheng,

Sorry for such late review w.r.t compatibles,

On 14/09/2020 09:06, Cheng-Yi Chiang wrote:
> +---
> +$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.
> +
> +properties:
> +  compatible:
> +    const: qcom,sc7180-sndcard-rt5682-m98357-1mic

This information can come from the dai link description itself, why 
should compatible string have this information?

Can't we have better compatible string with actual board name or use the 
same compatible name as used by other boards?

Can you give us some details on the advantages of doing this way?

Or am I missing something?

AFAIU, you should add proper board name / model name to the compatible 
string rather than describe how its connected. Connection is already 
part of dai link definition.

On the other hand model property can include variant information.
This can also be used to set card long name which will help in UCM2.

The reason I had to bring this up is because the use-space (ucm in this 
case) will not be in a position to differentiate between different board 
variants to select correct mixer controls, so its going to be a pain!


Thanks,
srini

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-13 10:36   ` Srinivas Kandagatla
@ 2020-10-15  7:59     ` Cheng-yi Chiang
  2020-10-15 16:12       ` Mark Brown
  0 siblings, 1 reply; 23+ messages in thread
From: Cheng-yi Chiang @ 2020-10-15  7:59 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,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	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...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...

On Tue, Oct 13, 2020 at 6:36 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> Hi Cheng,
>
> Sorry for such late review w.r.t compatibles,
>
Hi Srini,
Thank you for taking another look!

> On 14/09/2020 09:06, Cheng-Yi Chiang wrote:
> > +---
> > +$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.
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,sc7180-sndcard-rt5682-m98357-1mic
>
> This information can come from the dai link description itself, why
> should compatible string have this information?


I think dailink description is not enough to specify everything
machine driver needs to know.
E.g. there is a variation where there are front mic and rear mic. We
need to tell the machine driver about it so
it can create proper widget, route, and controls.
The codec combination also matters. There will be a variation where
rt5682 is replaced with adau7002 for dmic.
Although machine driver can derive some information by looking at dailink,
I think specifying it explicitly in the compatible string is easier to
tell what machine driver should do, e.g.
setting PLL related to rt5682 or not.

>
> Can't we have better compatible string with actual board name or use the
> same compatible name as used by other boards?
>
>
> Can you give us some details on the advantages of doing this way?


Machine driver can easily tell what is expected when it sees the
compatible string (or model property, as you suggested below).
E.g. in 1-mic v.s. 2-mic case, the patch by Ajye Huang:

"[v1,2/2] ASoC: qcom: sc7180: Modify machine driver for 2mic"

You can see widget, route, controls are used according to the configuration.
The alternative approach is to check whether "dmic-gpio" property
exists to decide adding these stuff or not.
But it makes the intent less easier to understand.


>
> Or am I missing something?
>
> AFAIU, you should add proper board name / model name to the compatible
> string rather than describe how its connected. Connection is already
> part of dai link definition.
>
> On the other hand model property can include variant information.
> This can also be used to set card long name which will help in UCM2.
>
>
> The reason I had to bring this up is because the use-space (ucm in this
> case) will not be in a position to differentiate between different board
> variants to select correct mixer controls, so its going to be a pain!


I think your suggestions makes sense since we need to consider UCM.
Having the card with the same name doing different things will be
confusing to user (and to UCM).
I'll follow your suggestion to use the same compatible string, and put
the board variation information in card name using model property.
Thanks a lot for the great help!


>
>
>
> Thanks,
> srini

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

* Re: [PATCH v11 3/3] ASoC: qcom: sc7180: Add machine driver for sound card registration
  2020-10-13  3:50   ` Cheng-yi Chiang
@ 2020-10-15  8:02     ` Cheng-yi Chiang
  0 siblings, 0 replies; 23+ messages in thread
From: Cheng-yi Chiang @ 2020-10-15  8:02 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, Srini Kandagatla,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	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...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajit Pandey

On Tue, Oct 13, 2020 at 11:50 AM Cheng-yi Chiang <cychiang@chromium.org> wrote:
>
> On Mon, Sep 14, 2020 at 4:06 PM Cheng-Yi Chiang <cychiang@chromium.org> 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: Ajit Pandey <ajitp@codeaurora.org>
> > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> > ---
> >  sound/soc/qcom/Kconfig  |  12 ++
> >  sound/soc/qcom/Makefile |   2 +
> >  sound/soc/qcom/sc7180.c | 266 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 280 insertions(+)
> >  create mode 100644 sound/soc/qcom/sc7180.c
> >
> > diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
> > index a607ace8b089..0459185ee243 100644
> > --- a/sound/soc/qcom/Kconfig
> > +++ b/sound/soc/qcom/Kconfig
> > @@ -116,4 +116,16 @@ config SND_SOC_SDM845
> >           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 I2C
> > +       select SND_SOC_QCOM_COMMON
> > +       select SND_SOC_LPASS_SC7180
> > +       select SND_SOC_MAX98357A
> > +       select SND_SOC_RT5682_I2C
> > +       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.
> > +
> >  endif #SND_SOC_QCOM
> > diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
> > index 7972c9479ab0..0cdcbf367ef1 100644
> > --- a/sound/soc/qcom/Makefile
> > +++ b/sound/soc/qcom/Makefile
> > @@ -17,12 +17,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..0e90448523b0
> > --- /dev/null
> > +++ b/sound/soc/qcom/sc7180.c
> > @@ -0,0 +1,266 @@
> > +// 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/soc.h>
> > +#include <uapi/linux/input-event-codes.h>
> > +
> > +#include "../codecs/rt5682.h"
> > +#include "common.h"
> > +#include "lpass.h"
> > +
> > +#define DEFAULT_MCLK_RATE              19200000
> > +#define RT5682_PLL1_FREQ (48000 * 512)
> > +
> > +// This will be defined in include/dt-bindings/sound/sc7180-lpass.h
> > +#define LPASS_DP_RX 2
> > +
> > +struct sc7180_snd_data {
> > +       struct snd_soc_card card;
> > +       u32 pri_mi2s_clk_count;
> > +       struct snd_soc_jack hs_jack;
> > +       struct snd_soc_jack hdmi_jack;
> > +};
> > +
> > +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_pcm_runtime *rtd)
> > +{
> > +       struct snd_soc_card *card = rtd->card;
> > +       struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> > +       struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > +       struct snd_soc_component *component = codec_dai->component;
> > +       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->hs_jack, NULL, 0);
> > +
> > +       if (rval < 0) {
> > +               dev_err(card->dev, "Unable to add Headset Jack\n");
> > +               return rval;
> > +       }
> > +
> > +       jack = pdata->hs_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;
> > +
> > +       return snd_soc_component_set_jack(component, &pdata->hs_jack, NULL);
> > +}
> > +
> > +static int sc7180_hdmi_init(struct snd_soc_pcm_runtime *rtd)
> > +{
> > +       struct snd_soc_card *card = rtd->card;
> > +       struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> > +       struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> > +       struct snd_soc_component *component = codec_dai->component;
> > +       struct snd_jack *jack;
> > +       int rval;
> > +
> > +       rval = snd_soc_card_jack_new(
> > +                       card, "HDMI Jack",
> > +                       SND_JACK_LINEOUT,
> > +                       &pdata->hdmi_jack, NULL, 0);
> > +
> > +       if (rval < 0) {
> > +               dev_err(card->dev, "Unable to add HDMI Jack\n");
> > +               return rval;
> > +       }
> > +
> > +       jack = pdata->hdmi_jack.jack;
> > +       jack->private_data = component;
> > +       jack->private_free = sc7180_jack_free;
> > +
> > +       return snd_soc_component_set_jack(component, &pdata->hdmi_jack, NULL);
> > +}
> > +
> > +static int sc7180_init(struct snd_soc_pcm_runtime *rtd)
> > +{
> > +       struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> > +
> > +       switch (cpu_dai->id) {
> > +       case MI2S_PRIMARY:
> > +               return sc7180_headset_init(rtd);
> > +       case MI2S_SECONDARY:
> > +               return 0;
> > +       case LPASS_DP_RX:
> > +               return sc7180_hdmi_init(rtd);
> > +       default:
> > +               dev_err(rtd->dev, "%s: invalid dai id 0x%x\n", __func__,
> > +                       cpu_dai->id);
> > +               return -EINVAL;
> > +       }
> > +       return 0;
> > +}
> > +
> > +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,
> > +                                   SND_SOC_DAIFMT_CBS_CFS |
> > +                                   SND_SOC_DAIFMT_NB_NF |
> > +                                   SND_SOC_DAIFMT_I2S);
> > +
> > +               /* 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) {
> > +                       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)
> > +                       dev_err(rtd->dev, "snd_soc_dai_set_sysclk err = %d\n",
> > +                               ret);
> > +
> > +               break;
> > +       case MI2S_SECONDARY:
> > +               break;
> > +       case LPASS_DP_RX:
> > +               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;
> > +       case LPASS_DP_RX:
> > +               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 void sc7180_add_ops(struct snd_soc_card *card)
> > +{
> > +       struct snd_soc_dai_link *link;
> > +       int i;
> > +
> > +       for_each_card_prelinks(card, i, link) {
> > +               link->ops = &sc7180_ops;
> > +               link->init = sc7180_init;
> > +       }
> > +}
> > +
> > +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;
> > +
> > +       /* Allocate the private data */
> > +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return -ENOMEM;
> > +
> > +       card = &data->card;
> > +       snd_soc_card_set_drvdata(card, data);
> > +
> > +       card->owner = THIS_MODULE,
> > +       card->dev = dev;
> > +       card->dapm_widgets = sc7180_snd_widgets;
> > +       card->num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets);
> > +
> > +       ret = qcom_snd_parse_of(card);
> > +       if (ret)
> > +               return ret;
> > +
> > +       sc7180_add_ops(card);
> > +
> > +       return devm_snd_soc_register_card(dev, card);
> > +}
> > +
> > +static const struct of_device_id sc7180_snd_device_id[]  = {
> > +       { .compatible = "qcom,sc7180-sndcard-rt5682-m98357-1mic" },
> > +       {},
> > +};
> > +MODULE_DEVICE_TABLE(of, sc7180_snd_device_id);
> > +
> > +static struct platform_driver sc7180_snd_driver = {
> > +       .probe = sc7180_snd_platform_probe,
> > +       .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.618.gf4bc123cb7-goog
> >
>
> Hi Srini and Stephan,
> May I get your Reviewed-By for this patch ?
> There are some patches on the machine driver for different
> configurations pending on this one so I would hope to get this one
> merged first.
>
> Thanks a lot for reviewing it!

Thanks Srini for the suggestion in the review of compatible strings.
In the next version (v12), I will modify accordingly to use model
property rather than compatible string to specify board configuration.
Thanks!

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-15  7:59     ` Cheng-yi Chiang
@ 2020-10-15 16:12       ` Mark Brown
  2020-10-20 13:37         ` Cheng-yi Chiang
  0 siblings, 1 reply; 23+ messages in thread
From: Mark Brown @ 2020-10-15 16:12 UTC (permalink / raw)
  To: Cheng-yi Chiang
  Cc: Srinivas Kandagatla, linux-kernel, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm, Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...


[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

On Thu, Oct 15, 2020 at 03:59:26PM +0800, Cheng-yi Chiang wrote:
> On Tue, Oct 13, 2020 at 6:36 PM Srinivas Kandagatla

> > > +properties:
> > > +  compatible:
> > > +    const: qcom,sc7180-sndcard-rt5682-m98357-1mic

> > This information can come from the dai link description itself, why
> > should compatible string have this information?

> I think dailink description is not enough to specify everything
> machine driver needs to know.
> E.g. there is a variation where there are front mic and rear mic. We
> need to tell the machine driver about it so
> it can create proper widget, route, and controls.

That sounds like something that could be better described with
properties (including for example the existing bindings used for setting
up things like analogue outputs and DAPM routes)?

> The codec combination also matters. There will be a variation where
> rt5682 is replaced with adau7002 for dmic.
> Although machine driver can derive some information by looking at dailink,
> I think specifying it explicitly in the compatible string is easier to
> tell what machine driver should do, e.g.
> setting PLL related to rt5682 or not.

These feel more like things that fit with compatible, though please take
a look at Morimoto-san's (CCed) work on generic sound cards for more
complex devices:

   https://lore.kernel.org/alsa-devel/87imbeybq5.wl-kuninori.morimoto.gx@renesas.com/

This is not yet implemented but it'd be good to make sure that the
Qualcomm systems can be handled too in future.

> You can see widget, route, controls are used according to the configuration.
> The alternative approach is to check whether "dmic-gpio" property
> exists to decide adding these stuff or not.
> But it makes the intent less easier to understand.

OTOH if you have lots of compatibles then it can get hard to work out
exactly which one corresponds to a given board.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-15 16:12       ` Mark Brown
@ 2020-10-20 13:37         ` Cheng-yi Chiang
  2020-10-20 14:37           ` Mark Brown
  0 siblings, 1 reply; 23+ messages in thread
From: Cheng-yi Chiang @ 2020-10-20 13:37 UTC (permalink / raw)
  To: Mark Brown
  Cc: Srinivas Kandagatla, linux-kernel, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm, Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajye Huang

On Fri, Oct 16, 2020 at 12:13 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Oct 15, 2020 at 03:59:26PM +0800, Cheng-yi Chiang wrote:
> > On Tue, Oct 13, 2020 at 6:36 PM Srinivas Kandagatla
>
> > > > +properties:
> > > > +  compatible:
> > > > +    const: qcom,sc7180-sndcard-rt5682-m98357-1mic
>
> > > This information can come from the dai link description itself, why
> > > should compatible string have this information?
>
> > I think dailink description is not enough to specify everything
> > machine driver needs to know.
> > E.g. there is a variation where there are front mic and rear mic. We
> > need to tell the machine driver about it so
> > it can create proper widget, route, and controls.
>
> That sounds like something that could be better described with
> properties (including for example the existing bindings used for setting
> up things like analogue outputs and DAPM routes)?
>

Hi Mark, thank you for the comments.

May I know your suggestion on Ajye's patch "ASoC: qcom: sc7180: Modify
machine driver for 2mic" ?

https://lore.kernel.org/r/20200928063744.525700-3-ajye_huang@compal.corp-partner.google.com

I think adding code in the machine driver makes the intent straightforward.
If we want the machine driver to be fully configurable,
we can always add more code to handle properties like gpio, route,
widget (mux, text selection) passed in from the device tree.
But I feel that we don't need a machine driver to be that configurable
from the device tree.
I think having the logic scattered in various dtsi files and relying
on manual inspection to understand the usage would be less
maintainable than only exposing needed property like gpio.
Especially in the complicated case where we need to create a mux
widget with callback toggling the gpio like this:

+static const char * const dmic_mux_text[] = {
+       "Front Mic",
+       "Rear Mic",
+};
+
+static SOC_ENUM_SINGLE_DECL(sc7180_dmic_enum,
+                           SND_SOC_NOPM, 0, dmic_mux_text);
+
+static const struct snd_kcontrol_new sc7180_dmic_mux_control =
+       SOC_DAPM_ENUM_EXT("DMIC Select Mux", sc7180_dmic_enum,
+                         dmic_get, dmic_set);
+
+SND_SOC_DAPM_MUX("Dmic Mux", SND_SOC_NOPM, 0, 0, &sc7180_dmic_mux_control),

Passing all the logic along with the callback of dmic_get, dmic_set
from the device tree would be too hard to maintain.

> > The codec combination also matters. There will be a variation where
> > rt5682 is replaced with adau7002 for dmic.
> > Although machine driver can derive some information by looking at dailink,
> > I think specifying it explicitly in the compatible string is easier to
> > tell what machine driver should do, e.g.
> > setting PLL related to rt5682 or not.
>
> These feel more like things that fit with compatible, though please take
> a look at Morimoto-san's (CCed) work on generic sound cards for more
> complex devices:
>
>    https://lore.kernel.org/alsa-devel/87imbeybq5.wl-kuninori.morimoto.gx@renesas.com/
>
> This is not yet implemented but it'd be good to make sure that the
> Qualcomm systems can be handled too in future.

Yes, that should work to describe the dailink we are using.
But a more tricky issue is how to do calls like setting PLL in dai startup ops.

                /* Configure PLL1 for codec */
                ret = snd_soc_dai_set_pll(codec_dai, 0, RT5682_PLL1_S_MCLK,
                                          DEFAULT_MCLK_RATE, RT5682_PLL1_FREQ);

I think that asking a generic machine driver to do configuration like
this with only a limited interface of device property
might be too much of an ask for the machine driver.

>
> > You can see widget, route, controls are used according to the configuration.
> > The alternative approach is to check whether "dmic-gpio" property
> > exists to decide adding these stuff or not.
> > But it makes the intent less easier to understand.
>
> OTOH if you have lots of compatibles then it can get hard to work out
> exactly which one corresponds to a given board.

Totally agree. Glad we have only three variations up to now.

Would you mind if I simplify the compatible string like Srinivas
suggested, and send a v12?

As for other two kinds of variations that I am aware of:

1. front mic / rear mic
2. replace alc5682 with adau7002

We can set different board names and different compatible strings to
achieve such variation.
So that it would make sense to describe configuration in compatible
strings like you suggested, and also provides UCM a way to distinguish
different boards.
What do you think ?

Thanks!

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-20 13:37         ` Cheng-yi Chiang
@ 2020-10-20 14:37           ` Mark Brown
  2020-10-20 14:55             ` Srinivas Kandagatla
  2020-10-20 18:51             ` Cheng-yi Chiang
  0 siblings, 2 replies; 23+ messages in thread
From: Mark Brown @ 2020-10-20 14:37 UTC (permalink / raw)
  To: Cheng-yi Chiang
  Cc: Srinivas Kandagatla, linux-kernel, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm, Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajye Huang


[-- Attachment #1: Type: text/plain, Size: 2581 bytes --]

On Tue, Oct 20, 2020 at 09:37:05PM +0800, Cheng-yi Chiang wrote:

> May I know your suggestion on Ajye's patch "ASoC: qcom: sc7180: Modify
> machine driver for 2mic" ?

> https://lore.kernel.org/r/20200928063744.525700-3-ajye_huang@compal.corp-partner.google.com

> I think adding code in the machine driver makes the intent straightforward.
> If we want the machine driver to be fully configurable,
> we can always add more code to handle properties like gpio, route,
> widget (mux, text selection) passed in from the device tree.

If the device has both front and rear mics and only one can be active at
once that seems obvious and sensible.  If the devices only have one of
these then this seems like a bad idea.

> But I feel that we don't need a machine driver to be that configurable
> from the device tree.
> I think having the logic scattered in various dtsi files and relying
> on manual inspection to understand the usage would be less
> maintainable than only exposing needed property like gpio.
> Especially in the complicated case where we need to create a mux
> widget with callback toggling the gpio like this:

I don't understand what "logic scattered in various dtsi files" means,
sorry.

> Yes, that should work to describe the dailink we are using.
> But a more tricky issue is how to do calls like setting PLL in dai startup ops.

...

> I think that asking a generic machine driver to do configuration like
> this with only a limited interface of device property
> might be too much of an ask for the machine driver.

Richard was looking at some basic configuration for PLLs.

> Would you mind if I simplify the compatible string like Srinivas
> suggested, and send a v12?

> As for other two kinds of variations that I am aware of:

> 1. front mic / rear mic
> 2. replace alc5682 with adau7002

The CODEC change is going to be described in the DT no matter what -
you'll have a reference to the CODEC node but it may make sense if
there's enough custom code around it.  For front vs rear mic the
simplest thing would just be to not mention which if this is a hardware
fixed thing, otherwise a control.

> We can set different board names and different compatible strings to
> achieve such variation.
> So that it would make sense to describe configuration in compatible
> strings like you suggested, and also provides UCM a way to distinguish
> different boards.

I don't recall having suggested distinguishing these things with a
compatible string, especially not the microphones.  UCM can already use
the display names for the boards to distinguish things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-20 14:37           ` Mark Brown
@ 2020-10-20 14:55             ` Srinivas Kandagatla
  2020-10-20 18:54               ` Cheng-yi Chiang
  2020-10-20 18:51             ` Cheng-yi Chiang
  1 sibling, 1 reply; 23+ messages in thread
From: Srinivas Kandagatla @ 2020-10-20 14:55 UTC (permalink / raw)
  To: Mark Brown, Cheng-yi Chiang
  Cc: linux-kernel, Taniya Das, Rohit kumar, Banajit Goswami,
	Patrick Lai, Andy Gross, Bjorn Andersson, Liam Girdwood,
	Rob Herring, Jaroslav Kysela, Takashi Iwai, Stephan Gerhold,
	Matthias Brugger, Heiko Stuebner, Srinivasa Rao, Doug Anderson,
	Dylan Reid, Tzung-Bi Shih, Linux ARM, linux-arm-msm,
	Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajye Huang



On 20/10/2020 15:37, Mark Brown wrote:
> I don't understand what "logic scattered in various dtsi files" means,
> sorry.
> 
>> Yes, that should work to describe the dailink we are using.
>> But a more tricky issue is how to do calls like setting PLL in dai startup ops.
> ...
> 
>> I think that asking a generic machine driver to do configuration like
>> this with only a limited interface of device property
>> might be too much of an ask for the machine driver.
> Richard was looking at some basic configuration for PLLs.
> 
>> Would you mind if I simplify the compatible string like Srinivas
>> suggested, and send a v12?
>> As for other two kinds of variations that I am aware of:
>> 1. front mic / rear mic
>> 2. replace alc5682 with adau7002
> The CODEC change is going to be described in the DT no matter what -
> you'll have a reference to the CODEC node but it may make sense if
> there's enough custom code around it.  For front vs rear mic the
> simplest thing would just be to not mention which if this is a hardware
> fixed thing, otherwise a control.
> 
>> We can set different board names and different compatible strings to
>> achieve such variation.
>> So that it would make sense to describe configuration in compatible
>> strings like you suggested, and also provides UCM a way to distinguish
>> different boards.
> I don't recall having suggested distinguishing these things with a
> compatible string, especially not the microphones.  UCM can already use
> the display names for the boards to distinguish things.


Not with the compatible string!

Currently card name, and long name are exactly same in all Qualcomm 
soundcards, which makes it very difficult to identify how those boards 
re wired up at UCM2 level. So the plan is to properly populate card long 
name with "model" property which can include details on how things are 
wiredup on that board.

--srini

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-20 14:37           ` Mark Brown
  2020-10-20 14:55             ` Srinivas Kandagatla
@ 2020-10-20 18:51             ` Cheng-yi Chiang
  2020-10-20 19:39               ` Mark Brown
  1 sibling, 1 reply; 23+ messages in thread
From: Cheng-yi Chiang @ 2020-10-20 18:51 UTC (permalink / raw)
  To: Mark Brown
  Cc: Srinivas Kandagatla, linux-kernel, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm, Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajye Huang

On Tue, Oct 20, 2020 at 10:37 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Oct 20, 2020 at 09:37:05PM +0800, Cheng-yi Chiang wrote:
>
> > May I know your suggestion on Ajye's patch "ASoC: qcom: sc7180: Modify
> > machine driver for 2mic" ?
>
> > https://lore.kernel.org/r/20200928063744.525700-3-ajye_huang@compal.corp-partner.google.com
>
> > I think adding code in the machine driver makes the intent straightforward.
> > If we want the machine driver to be fully configurable,
> > we can always add more code to handle properties like gpio, route,
> > widget (mux, text selection) passed in from the device tree.
>
> If the device has both front and rear mics and only one can be active at
> once that seems obvious and sensible.  If the devices only have one of
> these then this seems like a bad idea.
>

trogdor board: only front mic.
pompom board: having both front mic and rear mic. Only one of them
will be used at a time. It is toggled by mixer control backed by a
gpio.

My proposed solution: instead of using compatible strings, expose only
dmic-gpio property.
When the machine driver sees this property, it uses the dapm widgets
and controls created in the machine driver.

> > But I feel that we don't need a machine driver to be that configurable
> > from the device tree.
> > I think having the logic scattered in various dtsi files and relying
> > on manual inspection to understand the usage would be less
> > maintainable than only exposing needed property like gpio.
> > Especially in the complicated case where we need to create a mux
> > widget with callback toggling the gpio like this:
>
> I don't understand what "logic scattered in various dtsi files" means,
> sorry.
>
I mean I don't want to use device property to pass in widget name,
type, text and callbacks.
Let me give an example:

- Board trogdor uses front mic, rt5682, and max98357a.
- Board pompom is based on board trogdor, but it has front mic and rear mic.
If we somehow managed to add the code to pass in widget, route, type,
text, and callbacks needed for dmic control, we will need to put a
bunch of properties in trogdor-pompom.dtsi file.
- Board ABC is based on trogdor as well, and it has front mic and rear
mic, but with a different speaker amp.

To use widget, route, type, text and callbacks for front mic and rear
mic, in trogdor-ABC.dtsi file we would copy some properties used in
trogdor-pompom.dtsi file. To support the different combination of
codec, we would need some modification of the route and widget.

Now the support of front mic and rear mic switch is scattered in
trogdor-ABC.dtsi and trogdor-pompom.dtsi files.
For example, when we change the code to parse or build the widget and
route, we need to fix both trogdor-pompom.dtsi and trogdor-ABC.dtsi.

Alternatively, if we only expose dmic-gpio property and put
surrounding code in the machine driver, we can use this dmic-gpio
property, plus the sound card name to identify the needed widget and
route.

> > Yes, that should work to describe the dailink we are using.
> > But a more tricky issue is how to do calls like setting PLL in dai startup ops.
>
> ...
>
> > I think that asking a generic machine driver to do configuration like
> > this with only a limited interface of device property
> > might be too much of an ask for the machine driver.
>
> Richard was looking at some basic configuration for PLLs.

That sounds promising. If we don't need to include the codec driver
header file explicitly, that can make machine drivers simpler.
Maybe for most of the simple cases we don't even need a dedicated
machine driver.

>
> > Would you mind if I simplify the compatible string like Srinivas
> > suggested, and send a v12?
>
> > As for other two kinds of variations that I am aware of:
>
> > 1. front mic / rear mic
> > 2. replace alc5682 with adau7002
>
> The CODEC change is going to be described in the DT no matter what -
> you'll have a reference to the CODEC node but it may make sense if
> there's enough custom code around it.  For front vs rear mic the
> simplest thing would just be to not mention which if this is a hardware
> fixed thing, otherwise a control.
>

Would you suggest checking whether the codec node is a rt5682 node,
and call required PLL calls accordingly ?

"For front vs rear mic the simplest thing would just be to not mention
which if this is a hardware fixed thing, otherwise a control."
Sorry I am not sure if I understand this correctly. Please correct me
if I am wrong.

- For default case having 1 mic: not mention this at all
- For front mic / rear mic case: see gpio property and use an
additional control.

> > We can set different board names and different compatible strings to
> > achieve such variation.
> > So that it would make sense to describe configuration in compatible
> > strings like you suggested, and also provides UCM a way to distinguish
> > different boards.
>
> I don't recall having suggested distinguishing these things with a
> compatible string, especially not the microphones.  UCM can already use
> the display names for the boards to distinguish things.

My apology that I made the wrong interpretation when I read your reply
"These feel more like things that fit with compatible" regarding
replacing alc5682 with adau7002. Please let me know which one solution
you prefer:
-  deriving this information from codec node
-  deriving this information from different sound card name

Thanks so much!

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-20 14:55             ` Srinivas Kandagatla
@ 2020-10-20 18:54               ` Cheng-yi Chiang
  2020-10-21 12:00                 ` Srinivas Kandagatla
  0 siblings, 1 reply; 23+ messages in thread
From: Cheng-yi Chiang @ 2020-10-20 18:54 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Mark Brown, linux-kernel, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm, Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajye Huang

On Tue, Oct 20, 2020 at 10:55 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 20/10/2020 15:37, Mark Brown wrote:
> > I don't understand what "logic scattered in various dtsi files" means,
> > sorry.
> >
> >> Yes, that should work to describe the dailink we are using.
> >> But a more tricky issue is how to do calls like setting PLL in dai startup ops.
> > ...
> >
> >> I think that asking a generic machine driver to do configuration like
> >> this with only a limited interface of device property
> >> might be too much of an ask for the machine driver.
> > Richard was looking at some basic configuration for PLLs.
> >
> >> Would you mind if I simplify the compatible string like Srinivas
> >> suggested, and send a v12?
> >> As for other two kinds of variations that I am aware of:
> >> 1. front mic / rear mic
> >> 2. replace alc5682 with adau7002
> > The CODEC change is going to be described in the DT no matter what -
> > you'll have a reference to the CODEC node but it may make sense if
> > there's enough custom code around it.  For front vs rear mic the
> > simplest thing would just be to not mention which if this is a hardware
> > fixed thing, otherwise a control.
> >
> >> We can set different board names and different compatible strings to
> >> achieve such variation.
> >> So that it would make sense to describe configuration in compatible
> >> strings like you suggested, and also provides UCM a way to distinguish
> >> different boards.
> > I don't recall having suggested distinguishing these things with a
> > compatible string, especially not the microphones.  UCM can already use
> > the display names for the boards to distinguish things.
>
>
> Not with the compatible string!
>
> Currently card name, and long name are exactly same in all Qualcomm
> soundcards, which makes it very difficult to identify how those boards
> re wired up at UCM2 level. So the plan is to properly populate card long
> name with "model" property which can include details on how things are
> wiredup on that board.
>
> --srini

Hi Srini,
Thanks for taking a look.
Let me try to clarify your comments in case there is any misunderstanding.

I understand your request on having different board variations using
different sound card names through model property, and I totally agree
with that.
As for compatible strings, do you insist on having all the board
variations using exactly the same compatible string ?

Thanks!

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-20 18:51             ` Cheng-yi Chiang
@ 2020-10-20 19:39               ` Mark Brown
  0 siblings, 0 replies; 23+ messages in thread
From: Mark Brown @ 2020-10-20 19:39 UTC (permalink / raw)
  To: Cheng-yi Chiang
  Cc: Srinivas Kandagatla, linux-kernel, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm, Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajye Huang


[-- Attachment #1: Type: text/plain, Size: 3651 bytes --]

On Wed, Oct 21, 2020 at 02:51:33AM +0800, Cheng-yi Chiang wrote:
> On Tue, Oct 20, 2020 at 10:37 PM Mark Brown <broonie@kernel.org> wrote:

> > If the device has both front and rear mics and only one can be active at
> > once that seems obvious and sensible.  If the devices only have one of
> > these then this seems like a bad idea.

> trogdor board: only front mic.
> pompom board: having both front mic and rear mic. Only one of them
> will be used at a time. It is toggled by mixer control backed by a
> gpio.

> My proposed solution: instead of using compatible strings, expose only
> dmic-gpio property.
> When the machine driver sees this property, it uses the dapm widgets
> and controls created in the machine driver.

Yes, that is what I would expect.

> > I don't understand what "logic scattered in various dtsi files" means,
> > sorry.

> I mean I don't want to use device property to pass in widget name,
> type, text and callbacks.
> Let me give an example:

> - Board trogdor uses front mic, rt5682, and max98357a.
> - Board pompom is based on board trogdor, but it has front mic and rear mic.
> If we somehow managed to add the code to pass in widget, route, type,
> text, and callbacks needed for dmic control, we will need to put a
> bunch of properties in trogdor-pompom.dtsi file.

Most of this code is already there as part of the generic card
infrastructure, the only thing that stands out for me is the GPIO to
switch between the front and rear mics.

> - Board ABC is based on trogdor as well, and it has front mic and rear
> mic, but with a different speaker amp.

> To use widget, route, type, text and callbacks for front mic and rear
> mic, in trogdor-ABC.dtsi file we would copy some properties used in
> trogdor-pompom.dtsi file. To support the different combination of
> codec, we would need some modification of the route and widget.

It shouldn't be hugely difficult to split the DT files up usually, and
ideally they'd be small enough that just having an entirely new sound
bit isn't the end of the world.  Again I'm just not clear what you're
seeing here.

> > The CODEC change is going to be described in the DT no matter what -
> > you'll have a reference to the CODEC node but it may make sense if
> > there's enough custom code around it.  For front vs rear mic the
> > simplest thing would just be to not mention which if this is a hardware
> > fixed thing, otherwise a control.

> Would you suggest checking whether the codec node is a rt5682 node,
> and call required PLL calls accordingly ?

Potentially, or there might be so little shared that it's just a
separate machine driver.

> "For front vs rear mic the simplest thing would just be to not mention
> which if this is a hardware fixed thing, otherwise a control."
> Sorry I am not sure if I understand this correctly. Please correct me
> if I am wrong.

> - For default case having 1 mic: not mention this at all
> - For front mic / rear mic case: see gpio property and use an
> additional control.

Yes.

> "These feel more like things that fit with compatible" regarding
> replacing alc5682 with adau7002. Please let me know which one solution
> you prefer:
> -  deriving this information from codec node
> -  deriving this information from different sound card name

To an extent this depends on how different the CODECs and general setup
are but a different CODEC is something that often justifies a separate
compatible.  Of course you also have an awful lot of systems that work
with the generic card drivers and all different kinds of CPU and CODEC,
usually because the driver doesn't need to know anything about the
implementation of either.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-20 18:54               ` Cheng-yi Chiang
@ 2020-10-21 12:00                 ` Srinivas Kandagatla
  2020-10-21 12:39                   ` Mark Brown
  0 siblings, 1 reply; 23+ messages in thread
From: Srinivas Kandagatla @ 2020-10-21 12:00 UTC (permalink / raw)
  To: Cheng-yi Chiang
  Cc: Mark Brown, linux-kernel, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm, Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajye Huang



On 20/10/2020 19:54, Cheng-yi Chiang wrote:
>> Not with the compatible string!
>>
>> Currently card name, and long name are exactly same in all Qualcomm
>> soundcards, which makes it very difficult to identify how those boards
>> re wired up at UCM2 level. So the plan is to properly populate card long
>> name with "model" property which can include details on how things are
>> wiredup on that board.
>>
>> --srini
> Hi Srini,
> Thanks for taking a look.
> Let me try to clarify your comments in case there is any misunderstanding.
> 
> I understand your request on having different board variations using
> different sound card names through model property, and I totally agree
> with that.
> As for compatible strings, do you insist on having all the board
> variations using exactly the same compatible string ?


For example if we set below property for sound card in Device tree
model = "RB5";

We will end up with

#   cat /proc/asound/cards
  0 [RB5            ]: RB5 - RB5
                       RB5

This is totally not very useful w.r.t UCM2 and makes it very difficult 
to common up parts of the configs.


My suggestions are.
1. set card->driver_name to something more sensible in your sound card 
driver.

ex:
	card->driver_name = "SM8250";

2. set long name in model DT property and set it as card long name
ex:
in DT:
	model = "Qualcomm-RB5-WSA8815-Speakers-DMIC0";

in sound driver or common.c:

of_property_read_string_index(np, "model", 0, &card->long_name);

With this set:

now
#   cat /proc/asound/cards
  0 [QualcommRB5WSA8]: SM8250 - Qualcomm-RB5-WSA8815-Speakers-D
                       Qualcomm-RB5-WSA8815-Speakers-DMIC0

This also means that in UCM2 we can have a top level SM8250 directory 
which can contain other board variants something like:

ucm2/Qualcomm/sm8250/Qualcomm-RB5-WSA8815-Speakers-DMIC0.conf
ucm2/Qualcomm/sm8250/Qualcomm-RB5-WSA8810-Speakers-DMIC123.conf
and so on!

Finally Only comment I had regarding compatible was not to encapsulate 
the connection details in it!. these can be made more sensible, 
something like
"qcom,sc7180-trogdor-v1", "qcom,sc7180-trogdor-v2".. and so on.

This compatible has nothing to do with driver or card short and long name.

Does that makes sense?


Thanks,
srini


with




Currently if

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-21 12:00                 ` Srinivas Kandagatla
@ 2020-10-21 12:39                   ` Mark Brown
  2020-10-22  3:29                     ` Tzung-Bi Shih
  0 siblings, 1 reply; 23+ messages in thread
From: Mark Brown @ 2020-10-21 12:39 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Cheng-yi Chiang, linux-kernel, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm, Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajye Huang


[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

On Wed, Oct 21, 2020 at 01:00:54PM +0100, Srinivas Kandagatla wrote:

> This is totally not very useful w.r.t UCM2 and makes it very difficult to
> common up parts of the configs.

> My suggestions are.
> 1. set card->driver_name to something more sensible in your sound card
> driver.

> 2. set long name in model DT property and set it as card long name

It's also worth taking a look at what Intel are doing here with their
cards.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-21 12:39                   ` Mark Brown
@ 2020-10-22  3:29                     ` Tzung-Bi Shih
  2020-10-22 10:12                       ` Srinivas Kandagatla
  0 siblings, 1 reply; 23+ messages in thread
From: Tzung-Bi Shih @ 2020-10-22  3:29 UTC (permalink / raw)
  To: Mark Brown
  Cc: Srinivas Kandagatla, Cheng-yi Chiang, linux-kernel, Taniya Das,
	Rohit kumar, Banajit Goswami, Patrick Lai, Andy Gross,
	Bjorn Andersson, Liam Girdwood, Rob Herring, Jaroslav Kysela,
	Takashi Iwai, Stephan Gerhold, Matthias Brugger, Heiko Stuebner,
	Srinivasa Rao, Doug Anderson, Dylan Reid, Tzung-Bi Shih,
	Linux ARM, linux-arm-msm, Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajye Huang

Hi, sorry for jumping into your discussion but I am trying to
summarize them to make sure we are on the same page.  Pardon me to
manually copy-and-paste partial sentences to quote.

ACK:
- Don't expose DAI connections in compatible strings.
- Use "model" DT property to make the card more UCM2-friendly.
- Expose new DT properties to distinguish different DMIC models.

NACK:
- All the board variations using exactly the same compatible string.
=> This is less realistic.  Although the CODECS information can be
retrieved from DT, it is inevitable to have some custom code for each
CODEC.

Per Mark's words:
> a different CODEC is something that often justifies a separate compatible
I think we should use different compatible strings for new CODECS
combinations.  And we should try to reuse the machine driver if they
share the most code.  In the worst case, introduce a new machine
driver for the new CODECS combinations.

- Srinivas's suggestion to set driver_name.
e.g. card->driver_name = "SM8250";
=> This sounds like a new DT property should be parsed in
sound/soc/qcom/common.c.  For example: "qcom,family"?  But as we do
less care about UCM2 for now, I would prefer to just leave it as is.


I would expect the following variants in DTS (just for example):

sound {
  compatible = "qcom,sc7180-trogdor";
  model = "sc7180-rt5682-max98357a-1mic";
}

sound {
  compatible = "qcom,sc7180-trogdor";
  model = "sc7180-rt5682-max98357a-2mic";
  dmic-gpio = ...
}

sound {
  compatible = "qcom,sc7180-pompom";
  model = "sc7180-adau7002-max98357a";
}


Please correct me if there is any misunderstanding.

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-22  3:29                     ` Tzung-Bi Shih
@ 2020-10-22 10:12                       ` Srinivas Kandagatla
  2020-10-23  7:55                         ` Cheng-yi Chiang
  0 siblings, 1 reply; 23+ messages in thread
From: Srinivas Kandagatla @ 2020-10-22 10:12 UTC (permalink / raw)
  To: Tzung-Bi Shih, Mark Brown
  Cc: Cheng-yi Chiang, linux-kernel, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm, Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajye Huang



On 22/10/2020 04:29, Tzung-Bi Shih wrote:
> Hi, sorry for jumping into your discussion but I am trying to
> summarize them to make sure we are on the same page.  Pardon me to
> manually copy-and-paste partial sentences to quote.
> 
> ACK:
> - Don't expose DAI connections in compatible strings.
> - Use "model" DT property to make the card more UCM2-friendly.
> - Expose new DT properties to distinguish different DMIC models.
> 
> NACK:
> - All the board variations using exactly the same compatible string.
> => This is less realistic.  Although the CODECS information can be
> retrieved from DT, it is inevitable to have some custom code for each
> CODEC.
> 
> Per Mark's words:
>> a different CODEC is something that often justifies a separate compatible
> I think we should use different compatible strings for new CODECS
> combinations.  And we should try to reuse the machine driver if they
> share the most code.  In the worst case, introduce a new machine
> driver for the new CODECS combinations.
> 
> - Srinivas's suggestion to set driver_name.
> e.g. card->driver_name = "SM8250";
> => This sounds like a new DT property should be parsed in
> sound/soc/qcom/common.c.  For example: "qcom,family"?  But as we do
> less care about UCM2 for now, I would prefer to just leave it as is.
> 
No, you can just hardcode this driver_name in your machine driver rather 
than getting it from DT, this is how everyone does!.
So need of adding anything to common.c

The thing that I suggested to add to common.c is setting card->long_name 
from "model" property.

> 
> I would expect the following variants in DTS (just for example):
> 
> sound {
>    compatible = "qcom,sc7180-trogdor";
Make sure that vendor name is correct here, am not sure if trogdor is 
qcom board or Google own board!

>    model = "sc7180-rt5682-max98357a-1mic";
> }
> 
> sound {
>    compatible = "qcom,sc7180-trogdor";
>    model = "sc7180-rt5682-max98357a-2mic";
>    dmic-gpio = ...
> }
> 
> sound {
>    compatible = "qcom,sc7180-pompom";
>    model = "sc7180-adau7002-max98357a";
> }
> 
> 
> Please correct me if there is any misunderstanding.

Looks good to me!
thanks for doing this!

--srini
> 

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

* Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
  2020-10-22 10:12                       ` Srinivas Kandagatla
@ 2020-10-23  7:55                         ` Cheng-yi Chiang
  0 siblings, 0 replies; 23+ messages in thread
From: Cheng-yi Chiang @ 2020-10-23  7:55 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Tzung-Bi Shih, Mark Brown, linux-kernel, Taniya Das, Rohit kumar,
	Banajit Goswami, Patrick Lai, Andy Gross, Bjorn Andersson,
	Liam Girdwood, Rob Herring, Jaroslav Kysela, Takashi Iwai,
	Stephan Gerhold, Matthias Brugger, Heiko Stuebner, Srinivasa Rao,
	Doug Anderson, Dylan Reid, Tzung-Bi Shih, Linux ARM,
	linux-arm-msm, Kuninori Morimoto,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	moderated list:ARM/Mediatek SoC support,
	open list:ARM/Rockchip SoC...,
	Ajye Huang, xuyuqing

Sorry for resending this mail.
I forgot to use plain text mode in the previous mail.
On Thu, Oct 22, 2020 at 6:12 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 22/10/2020 04:29, Tzung-Bi Shih wrote:
> > Hi, sorry for jumping into your discussion but I am trying to
> > summarize them to make sure we are on the same page.  Pardon me to
> > manually copy-and-paste partial sentences to quote.
> >
> > ACK:
> > - Don't expose DAI connections in compatible strings.
> > - Use "model" DT property to make the card more UCM2-friendly.
> > - Expose new DT properties to distinguish different DMIC models.
> >
> > NACK:
> > - All the board variations using exactly the same compatible string.
> > => This is less realistic.  Although the CODECS information can be
> > retrieved from DT, it is inevitable to have some custom code for each
> > CODEC.
> >
> > Per Mark's words:
> >> a different CODEC is something that often justifies a separate compatible
> > I think we should use different compatible strings for new CODECS
> > combinations.  And we should try to reuse the machine driver if they
> > share the most code.  In the worst case, introduce a new machine
> > driver for the new CODECS combinations.
> >
> > - Srinivas's suggestion to set driver_name.
> > e.g. card->driver_name = "SM8250";
> > => This sounds like a new DT property should be parsed in
> > sound/soc/qcom/common.c.  For example: "qcom,family"?  But as we do
> > less care about UCM2 for now, I would prefer to just leave it as is.
> >
> No, you can just hardcode this driver_name in your machine driver rather
> than getting it from DT, this is how everyone does!.
> So need of adding anything to common.c
>
ACK
> The thing that I suggested to add to common.c is setting card->long_name
> from "model" property.
>

NACK
I found that I don't need to set card->long_name in common.c because
soc-core.c already sets longname using card->name if
card->long_name is NULL.

        soc_setup_card_name(card->snd_card->longname,
                            card->long_name, card->name, 0);

So we can leave common.c as it is and still get long name.

> >
> > I would expect the following variants in DTS (just for example):
> >
> > sound {
> >    compatible = "qcom,sc7180-trogdor";
> Make sure that vendor name is correct here, am not sure if trogdor is
> qcom board or Google own board!
ACK
I should use "google,sc7180-trogdor" because google is the vendor.
>
> >    model = "sc7180-rt5682-max98357a-1mic";
> > }
> >
> > sound {
> >    compatible = "qcom,sc7180-trogdor";
> >    model = "sc7180-rt5682-max98357a-2mic";
> >    dmic-gpio = ...
> > }
> >
> > sound {
> >    compatible = "qcom,sc7180-pompom";
> >    model = "sc7180-adau7002-max98357a";
> > }
> >
> >
> > Please correct me if there is any misunderstanding.
>
> Looks good to me!
> thanks for doing this!
Thank you. I will collect the discussion result to send a v12, and
sync with variant board partners to submit following machine driver
changes.
We will make sure future projects follow this approach
>
> --srini
> >

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

end of thread, back to index

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-14  8:06 [PATCH v11 0/3] Add documentation and machine driver for SC7180 sound card Cheng-Yi Chiang
2020-09-14  8:06 ` [PATCH v11 1/3] ASoC: hdmi-codec: Use set_jack ops to set jack Cheng-Yi Chiang
2020-09-14  8:06 ` [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings Cheng-Yi Chiang
2020-09-14 17:48   ` Rob Herring
2020-09-15 12:44     ` Cheng-yi Chiang
2020-10-13  3:46       ` Cheng-yi Chiang
2020-10-13 10:36   ` Srinivas Kandagatla
2020-10-15  7:59     ` Cheng-yi Chiang
2020-10-15 16:12       ` Mark Brown
2020-10-20 13:37         ` Cheng-yi Chiang
2020-10-20 14:37           ` Mark Brown
2020-10-20 14:55             ` Srinivas Kandagatla
2020-10-20 18:54               ` Cheng-yi Chiang
2020-10-21 12:00                 ` Srinivas Kandagatla
2020-10-21 12:39                   ` Mark Brown
2020-10-22  3:29                     ` Tzung-Bi Shih
2020-10-22 10:12                       ` Srinivas Kandagatla
2020-10-23  7:55                         ` Cheng-yi Chiang
2020-10-20 18:51             ` Cheng-yi Chiang
2020-10-20 19:39               ` Mark Brown
2020-09-14  8:06 ` [PATCH v11 3/3] ASoC: qcom: sc7180: Add machine driver for sound card registration Cheng-Yi Chiang
2020-10-13  3:50   ` Cheng-yi Chiang
2020-10-15  8:02     ` 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