[PATCH 0/5] ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6

[PATCH 0/5] ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6

[Stephan Gerhold]

This series makes it possible to route audio through the combined
audio/modem DSP on MSM8916/APQ8016 devices instead of bypassing it using
the LPASS drivers. This is necessary to support certain functionality such
as voice call audio. See PATCH 4/5 for details.

Also, qcom,apq8016-sbc.txt is converted to DT schema by adding it to the
existing qcom,sm8250.yaml. The bindings are similar enough that it is easier
to share a single schema instead of duplicating everything into multiple ones.

Stephan Gerhold (5):
  ASoC: dt-bindings: qcom: sm8250: Drop redundant MultiMedia routes
  ASoC: dt-bindings: qcom: sm8250: Document "aux-devs"
  ASoC: dt-bindings: qcom: apq8016-sbc: Move to qcom,sm8250 DT schema
  ASoC: dt-bindings: qcom: Document qcom,msm8916-qdsp6-sndcard
    compatible
  ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6

 .../bindings/sound/qcom,apq8016-sbc.txt       |  96 -------------
 .../bindings/sound/qcom,sm8250.yaml           | 136 +++++++++++++++++-
 sound/soc/qcom/apq8016_sbc.c                  | 134 ++++++++++++++++-
 3 files changed, 259 insertions(+), 107 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt

-- 
2.34.1

[PATCH 1/5] ASoC: dt-bindings: qcom: sm8250: Drop redundant MultiMedia routes

[Stephan Gerhold]

The MultiMedia audio routes can be deduced from other parts of the
device tree (e.g. the definitions of the MultiMedia DAIs) and therefore
specifying them again in "audio-routing" is redundant and prone to
mistakes. This is no longer necessary since commit 6fd8d2d275f7
("ASoC: qcom: qdsp6: Move frontend AIFs to q6asm-dai").

Let's drop them from the example in the DT schema as well
to avoid confusion.

Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 Documentation/devicetree/bindings/sound/qcom,sm8250.yaml | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
index 7d57eb91657a..a0f1d7340eb5 100644
--- a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
+++ b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
@@ -86,10 +86,7 @@ examples:
         audio-routing = "SpkrLeft IN", "WSA_SPK1 OUT",
                     "SpkrRight IN", "WSA_SPK2 OUT",
                     "VA DMIC0", "vdd-micb",
-                    "VA DMIC1", "vdd-micb",
-                    "MM_DL1",  "MultiMedia1 Playback",
-                    "MM_DL2",  "MultiMedia2 Playback",
-                    "MultiMedia3 Capture", "MM_UL3";
+                    "VA DMIC1", "vdd-micb";
 
         mm1-dai-link {
             link-name = "MultiMedia0";
-- 
2.34.1

Re: [PATCH 1/5] ASoC: dt-bindings: qcom: sm8250: Drop redundant MultiMedia routes

[Rob Herring]

On Thu, 02 Dec 2021 15:55:01 +0100, Stephan Gerhold wrote:
> The MultiMedia audio routes can be deduced from other parts of the
> device tree (e.g. the definitions of the MultiMedia DAIs) and therefore
> specifying them again in "audio-routing" is redundant and prone to
> mistakes. This is no longer necessary since commit 6fd8d2d275f7
> ("ASoC: qcom: qdsp6: Move frontend AIFs to q6asm-dai").
> 
> Let's drop them from the example in the DT schema as well
> to avoid confusion.
> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  Documentation/devicetree/bindings/sound/qcom,sm8250.yaml | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 2/5] ASoC: dt-bindings: qcom: sm8250: Document "aux-devs"

[Stephan Gerhold]

The sm8250 audio driver uses the common Qualcomm device tree parser and
therefore already supports the "aux-devs" property that allows adding
additional auxiliary devices to the sound card (e.g. analog speaker
amplifiers that can be connected using "audio-routing").

Document the property in the DT schema for sm8250 as well. The description
is taken from simple-card.yaml which has a very similar property.

Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 Documentation/devicetree/bindings/sound/qcom,sm8250.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
index a0f1d7340eb5..3123382297b8 100644
--- a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
+++ b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
@@ -27,6 +27,12 @@ properties:
       being the connection's source. Valid names could be power supplies,
       MicBias of codec and the jacks on the board.
 
+  aux-devs:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: |
+      List of phandles pointing to auxiliary devices, such
+      as amplifiers, to be added to the sound card.
+
   model:
     $ref: /schemas/types.yaml#/definitions/string
     description: User visible long sound card name
-- 
2.34.1

Re: [PATCH 2/5] ASoC: dt-bindings: qcom: sm8250: Document "aux-devs"

[Rob Herring]

On Thu, 02 Dec 2021 15:55:02 +0100, Stephan Gerhold wrote:
> The sm8250 audio driver uses the common Qualcomm device tree parser and
> therefore already supports the "aux-devs" property that allows adding
> additional auxiliary devices to the sound card (e.g. analog speaker
> amplifiers that can be connected using "audio-routing").
> 
> Document the property in the DT schema for sm8250 as well. The description
> is taken from simple-card.yaml which has a very similar property.
> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  Documentation/devicetree/bindings/sound/qcom,sm8250.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 3/5] ASoC: dt-bindings: qcom: apq8016-sbc: Move to qcom,sm8250 DT schema

[Stephan Gerhold]

All the Qualcomm sound card drivers use the same common device tree
parsing code, so the allowed device tree nodes are almost the same
for all of them. Convert the qcom,apq8016-sbc-sndcard documentation
to a DT schema by adding it to the existing qcom,sm8250 schema.

The only speciality of qcom,apq8016-sbc-sndcard is that it has memory
resources for setting up an I/O mux. This can be handled using
a conditional if statement that only requires it for the apq8016-sbc
compatible.

Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Note: There will be a dtbs_check warning caused by this, because the
recommended naming for nodes is different in this DT schema (-dai-link
suffix). I will address this in a separate patch if this series is
accepted, simply by changing apq8016-sbc.dts to the example added
in this patch. The driver itself accepts any naming for those nodes.
---
 .../bindings/sound/qcom,apq8016-sbc.txt       | 96 -------------------
 .../bindings/sound/qcom,sm8250.yaml           | 83 +++++++++++++++-
 2 files changed, 81 insertions(+), 98 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt

diff --git a/Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt b/Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt
deleted file mode 100644
index 23998262a0a7..000000000000
--- a/Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt
+++ /dev/null
@@ -1,96 +0,0 @@
-* Qualcomm Technologies APQ8016 SBC ASoC machine driver
-
-This node models the Qualcomm Technologies APQ8016 SBC ASoC machine driver
-
-Required properties:
-
-- compatible		: "qcom,apq8016-sbc-sndcard"
-
-- pinctrl-N		: One property must exist for each entry in
-			  pinctrl-names.  See ../pinctrl/pinctrl-bindings.txt
-			  for details of the property values.
-- pinctrl-names		: Must contain a "default" entry.
-- reg			: Must contain an address for each entry in reg-names.
-- reg-names		: A list which must include the following entries:
-				* "mic-iomux"
-				* "spkr-iomux"
-- qcom,model		: Name of the sound card.
-
-- qcom,audio-routing	: 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. Valid names could be power supplies, MicBias
-			  of msm8x16_wcd codec and the jacks on the board:
-
-			  Power supplies:
-			  * MIC BIAS External1
-			  * MIC BIAS External2
-			  * MIC BIAS Internal1
-			  * MIC BIAS Internal2
-
-			  Board connectors:
-			  * Headset Mic
-			  * Secondary Mic
-			  * DMIC
-			  * Ext Spk
-
-Optional properties:
-
-- aux-devs		: A list of phandles for auxiliary devices (e.g. analog
-			  amplifiers) that do not appear directly within the DAI
-			  links. Should be connected to another audio component
-			  using "qcom,audio-routing".
-
-Dai-link subnode properties and subnodes:
-
-Required dai-link subnodes:
-
-- cpu					: CPU   sub-node
-- codec					: CODEC sub-node
-
-Required CPU/CODEC subnodes properties:
-
--link-name		: Name of the dai link.
--sound-dai		: phandle/s and port of CPU/CODEC
-
-Example:
-
-sound: sound {
-	compatible = "qcom,apq8016-sbc-sndcard";
-	reg = <0x07702000 0x4>, <0x07702004 0x4>;
-	reg-names = "mic-iomux", "spkr-iomux";
-	qcom,model = "DB410c";
-
-	qcom,audio-routing =
-		"MIC BIAS External1", "Handset Mic",
-		"MIC BIAS Internal2", "Headset Mic",
-		"MIC BIAS External1", "Secondary Mic",
-		"AMIC1", "MIC BIAS External1",
-		"AMIC2", "MIC BIAS Internal2",
-		"AMIC3", "MIC BIAS External1",
-		"DMIC1", "MIC BIAS Internal1",
-		"MIC BIAS Internal1", "Digital Mic1",
-		"DMIC2", "MIC BIAS Internal1",
-		"MIC BIAS Internal1", "Digital Mic2";
-
-	/* I2S - Internal codec */
-	internal-dai-link@0 {
-		cpu { /* PRIMARY */
-			sound-dai = <&lpass MI2S_PRIMARY>;
-		};
-		codec {
-			sound-dai = <&lpass_codec 0>, <&wcd_codec 0>;
-		};
-	};
-
-	/* External Primary or External Secondary -ADV7533 HDMI */
-	external-dai-link@0 {
-		link-name = "ADV7533";
-		cpu { /* QUAT */
-			sound-dai = <&lpass MI2S_QUATERNARY>;
-		};
-		codec {
-			sound-dai = <&adv_bridge 0>;
-		};
-	};
-};
diff --git a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
index 3123382297b8..97f13a0a71a9 100644
--- a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
+++ b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
@@ -4,18 +4,19 @@
 $id: http://devicetree.org/schemas/sound/qcom,sm8250.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Qualcomm Technologies Inc. SM8250 ASoC sound card driver
+title: Qualcomm Technologies Inc. ASoC sound card drivers
 
 maintainers:
   - Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
 
 description:
-  This bindings describes SC8250 SoC based sound cards
+  This bindings describes Qualcomm SoC based sound cards
   which uses LPASS internal codec for audio.
 
 properties:
   compatible:
     enum:
+      - qcom,apq8016-sbc-sndcard
       - qcom,sm8250-sndcard
       - qcom,qrb5165-rb5-sndcard
 
@@ -37,6 +38,10 @@ properties:
     $ref: /schemas/types.yaml#/definitions/string
     description: User visible long sound card name
 
+  # Only valid for some compatibles (see allOf if below)
+  reg: true
+  reg-names: true
+
 patternProperties:
   ".*-dai-link$":
     description:
@@ -79,6 +84,33 @@ required:
   - compatible
   - model
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,apq8016-sbc-sndcard
+    then:
+      properties:
+        reg:
+          items:
+            - description: Microphone I/O mux register address
+            - description: Speaker I/O mux register address
+        reg-names:
+          items:
+            - const: mic-iomux
+            - const: spkr-iomux
+      required:
+        - compatible
+        - model
+        - reg
+        - reg-names
+    else:
+      properties:
+        reg: false
+        reg-names: false
+
 additionalProperties: false
 
 examples:
@@ -160,3 +192,50 @@ examples:
             };
         };
     };
+
+  - |
+    #include <dt-bindings/sound/qcom,lpass.h>
+    sound@7702000 {
+        compatible = "qcom,apq8016-sbc-sndcard";
+        reg = <0x07702000 0x4>, <0x07702004 0x4>;
+        reg-names = "mic-iomux", "spkr-iomux";
+
+        model = "DB410c";
+        audio-routing =
+            "AMIC2", "MIC BIAS Internal2",
+            "AMIC3", "MIC BIAS External1";
+
+        pinctrl-0 = <&cdc_pdm_lines_act &ext_sec_tlmm_lines_act &ext_mclk_tlmm_lines_act>;
+        pinctrl-1 = <&cdc_pdm_lines_sus &ext_sec_tlmm_lines_sus &ext_mclk_tlmm_lines_sus>;
+        pinctrl-names = "default", "sleep";
+
+        quaternary-dai-link {
+            link-name = "ADV7533";
+            cpu {
+                sound-dai = <&lpass MI2S_QUATERNARY>;
+            };
+            codec {
+                sound-dai = <&adv_bridge 0>;
+            };
+        };
+
+        primary-dai-link {
+            link-name = "WCD";
+            cpu {
+                sound-dai = <&lpass MI2S_PRIMARY>;
+            };
+            codec {
+                sound-dai = <&lpass_codec 0>, <&wcd_codec 0>;
+            };
+        };
+
+        tertiary-dai-link {
+            link-name = "WCD-Capture";
+            cpu {
+                sound-dai = <&lpass MI2S_TERTIARY>;
+            };
+            codec {
+                sound-dai = <&lpass_codec 1>, <&wcd_codec 1>;
+            };
+        };
+    };
-- 
2.34.1

Re: [PATCH 3/5] ASoC: dt-bindings: qcom: apq8016-sbc: Move to qcom,sm8250 DT schema

[Rob Herring]

On Thu, 02 Dec 2021 15:55:03 +0100, Stephan Gerhold wrote:
> All the Qualcomm sound card drivers use the same common device tree
> parsing code, so the allowed device tree nodes are almost the same
> for all of them. Convert the qcom,apq8016-sbc-sndcard documentation
> to a DT schema by adding it to the existing qcom,sm8250 schema.
> 
> The only speciality of qcom,apq8016-sbc-sndcard is that it has memory
> resources for setting up an I/O mux. This can be handled using
> a conditional if statement that only requires it for the apq8016-sbc
> compatible.
> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> Note: There will be a dtbs_check warning caused by this, because the
> recommended naming for nodes is different in this DT schema (-dai-link
> suffix). I will address this in a separate patch if this series is
> accepted, simply by changing apq8016-sbc.dts to the example added
> in this patch. The driver itself accepts any naming for those nodes.
> ---
>  .../bindings/sound/qcom,apq8016-sbc.txt       | 96 -------------------
>  .../bindings/sound/qcom,sm8250.yaml           | 83 +++++++++++++++-
>  2 files changed, 81 insertions(+), 98 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 4/5] ASoC: dt-bindings: qcom: Document qcom,msm8916-qdsp6-sndcard compatible

[Stephan Gerhold]

There are two possible audio setups on MSM8916/APQ8016: Normally the audio
is routed through the audio/modem DSP (covered by the qdsp6 driver). During
upstreaming for the DragonBoard 410c it was decided to bypass it and
instead talk directly to the audio controller using the "lpass" driver.

Bypassing the DSP gives more control about the audio configuration but limits
the functionality: For example, routing audio through the audio/modem DSP is
strictly required for voice call audio. Also, without the special changes in
the DB410c firmware other MSM8916 devices can only use the bypass as long as
the modem DSP is not started. Otherwise, the firmware will assume control of
the LPASS hardware block and audio is no longer functional.

Add support for using the DSP audio setup instead using a new
"qcom,msm8916-qdsp6-sndcard" compatible. It is basically a mixture of
the apq8016-sbc-sndcard and the newer sm8250-sndcard, which uses
indirect QDSP6 DAI links instead of the direct LPASS DAI links.

Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 .../bindings/sound/qcom,sm8250.yaml           | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
index 97f13a0a71a9..e50964c54bb9 100644
--- a/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
+++ b/Documentation/devicetree/bindings/sound/qcom,sm8250.yaml
@@ -17,6 +17,7 @@ properties:
   compatible:
     enum:
       - qcom,apq8016-sbc-sndcard
+      - qcom,msm8916-qdsp6-sndcard
       - qcom,sm8250-sndcard
       - qcom,qrb5165-rb5-sndcard
 
@@ -91,6 +92,7 @@ allOf:
           contains:
             enum:
               - qcom,apq8016-sbc-sndcard
+              - qcom,msm8916-qdsp6-sndcard
     then:
       properties:
         reg:
@@ -239,3 +241,43 @@ examples:
             };
         };
     };
+
+  - |
+    #include <dt-bindings/sound/qcom,q6afe.h>
+    #include <dt-bindings/sound/qcom,q6asm.h>
+    sound@7702000 {
+        compatible = "qcom,msm8916-qdsp6-sndcard";
+        reg = <0x07702000 0x4>, <0x07702004 0x4>;
+        reg-names = "mic-iomux", "spkr-iomux";
+
+        model = "msm8916";
+        audio-routing =
+            "AMIC1", "MIC BIAS Internal1",
+            "AMIC2", "MIC BIAS Internal2",
+            "AMIC3", "MIC BIAS Internal3";
+        aux-devs = <&speaker_amp>;
+
+        pinctrl-names = "default", "sleep";
+        pinctrl-0 = <&cdc_pdm_lines_act>;
+        pinctrl-1 = <&cdc_pdm_lines_sus>;
+
+        mm1-dai-link {
+            link-name = "MultiMedia1";
+            cpu {
+                sound-dai = <&q6asmdai MSM_FRONTEND_DAI_MULTIMEDIA1>;
+            };
+        };
+
+        primary-dai-link {
+            link-name = "Primary MI2S";
+            cpu {
+                sound-dai = <&q6afedai PRIMARY_MI2S_RX>;
+            };
+            platform {
+                sound-dai = <&q6routing>;
+            };
+            codec {
+                sound-dai = <&lpass_codec 0>, <&wcd_codec 0>;
+            };
+        };
+    };
-- 
2.34.1

Re: [PATCH 4/5] ASoC: dt-bindings: qcom: Document qcom,msm8916-qdsp6-sndcard compatible

[Rob Herring]

On Thu, 02 Dec 2021 15:55:04 +0100, Stephan Gerhold wrote:
> There are two possible audio setups on MSM8916/APQ8016: Normally the audio
> is routed through the audio/modem DSP (covered by the qdsp6 driver). During
> upstreaming for the DragonBoard 410c it was decided to bypass it and
> instead talk directly to the audio controller using the "lpass" driver.
> 
> Bypassing the DSP gives more control about the audio configuration but limits
> the functionality: For example, routing audio through the audio/modem DSP is
> strictly required for voice call audio. Also, without the special changes in
> the DB410c firmware other MSM8916 devices can only use the bypass as long as
> the modem DSP is not started. Otherwise, the firmware will assume control of
> the LPASS hardware block and audio is no longer functional.
> 
> Add support for using the DSP audio setup instead using a new
> "qcom,msm8916-qdsp6-sndcard" compatible. It is basically a mixture of
> the apq8016-sbc-sndcard and the newer sm8250-sndcard, which uses
> indirect QDSP6 DAI links instead of the direct LPASS DAI links.
> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  .../bindings/sound/qcom,sm8250.yaml           | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 5/5] ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6

[Stephan Gerhold]

The apq8016-sbc-sndcard is designed to be used with the LPASS drivers
(bypassing the combined audio/modem DSP in MSM8916/APQ8016).
Make it possible to use QDSP6 audio instead for the msm8916-qdsp6-sndcard.

This only requires adding some additional hooks that set up the DPCM
backends correctly. Similar code is already used in drivers for newer
SoCs such as apq8096.c, sdm845.c and sm8250.c.

A slightly different initialization sequence is used for the apq8016-sbc
and msm8916-qdsp6 sound card by defining the apq8016_sbc_add_ops()
function as device match data.

Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 sound/soc/qcom/apq8016_sbc.c | 134 +++++++++++++++++++++++++++++++++--
 1 file changed, 129 insertions(+), 5 deletions(-)

diff --git a/sound/soc/qcom/apq8016_sbc.c b/sound/soc/qcom/apq8016_sbc.c
index ba2a98268ee4..f9d69375320e 100644
--- a/sound/soc/qcom/apq8016_sbc.c
+++ b/sound/soc/qcom/apq8016_sbc.c
@@ -17,6 +17,9 @@
 #include <uapi/linux/input-event-codes.h>
 #include <dt-bindings/sound/apq8016-lpass.h>
 #include "common.h"
+#include "qdsp6/q6afe.h"
+
+#define MI2S_COUNT  (MI2S_QUATERNARY + 1)
 
 struct apq8016_sbc_data {
 	struct snd_soc_card card;
@@ -24,6 +27,7 @@ struct apq8016_sbc_data {
 	void __iomem *spkr_iomux;
 	struct snd_soc_jack jack;
 	bool jack_setup;
+	int mi2s_clk_count[MI2S_COUNT];
 };
 
 #define MIC_CTRL_TER_WS_SLAVE_SEL	BIT(21)
@@ -38,10 +42,10 @@ struct apq8016_sbc_data {
 #define SPKR_CTL_TLMM_WS_EN_SEL_MASK	GENMASK(19, 18)
 #define SPKR_CTL_TLMM_WS_EN_SEL_SEC	BIT(18)
 #define DEFAULT_MCLK_RATE		9600000
+#define MI2S_BCLK_RATE			1536000
 
-static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd)
+static int apq8016_dai_init(struct snd_soc_pcm_runtime *rtd, int mi2s)
 {
-	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
 	struct snd_soc_dai *codec_dai;
 	struct snd_soc_component *component;
 	struct snd_soc_card *card = rtd->card;
@@ -49,7 +53,7 @@ static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd)
 	int i, rval;
 	u32 value;
 
-	switch (cpu_dai->id) {
+	switch (mi2s) {
 	case MI2S_PRIMARY:
 		writel(readl(pdata->spkr_iomux) | SPKR_CTL_PRI_WS_SLAVE_SEL_11,
 			pdata->spkr_iomux);
@@ -128,6 +132,13 @@ static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd)
 	return 0;
 }
 
+static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+
+	return apq8016_dai_init(rtd, cpu_dai->id);
+}
+
 static void apq8016_sbc_add_ops(struct snd_soc_card *card)
 {
 	struct snd_soc_dai_link *link;
@@ -137,6 +148,113 @@ static void apq8016_sbc_add_ops(struct snd_soc_card *card)
 		link->init = apq8016_sbc_dai_init;
 }
 
+static int qdsp6_dai_get_lpass_id(struct snd_soc_dai *cpu_dai)
+{
+	switch (cpu_dai->id) {
+	case PRIMARY_MI2S_RX:
+	case PRIMARY_MI2S_TX:
+		return MI2S_PRIMARY;
+	case SECONDARY_MI2S_RX:
+	case SECONDARY_MI2S_TX:
+		return MI2S_SECONDARY;
+	case TERTIARY_MI2S_RX:
+	case TERTIARY_MI2S_TX:
+		return MI2S_TERTIARY;
+	case QUATERNARY_MI2S_RX:
+	case QUATERNARY_MI2S_TX:
+		return MI2S_QUATERNARY;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int msm8916_qdsp6_dai_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+
+	snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_CBS_CFS);
+	return apq8016_dai_init(rtd, qdsp6_dai_get_lpass_id(cpu_dai));
+}
+
+static int msm8916_qdsp6_startup(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_card *card = rtd->card;
+	struct apq8016_sbc_data *data = snd_soc_card_get_drvdata(card);
+	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+	int mi2s, ret;
+
+	mi2s = qdsp6_dai_get_lpass_id(cpu_dai);
+	if (mi2s < 0)
+		return mi2s;
+
+	if (++data->mi2s_clk_count[mi2s] > 1)
+		return 0;
+
+	ret = snd_soc_dai_set_sysclk(cpu_dai, LPAIF_BIT_CLK, MI2S_BCLK_RATE, 0);
+	if (ret)
+		dev_err(card->dev, "Failed to enable LPAIF bit clk: %d\n", ret);
+	return ret;
+}
+
+static void msm8916_qdsp6_shutdown(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_card *card = rtd->card;
+	struct apq8016_sbc_data *data = snd_soc_card_get_drvdata(card);
+	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+	int mi2s, ret;
+
+	mi2s = qdsp6_dai_get_lpass_id(cpu_dai);
+	if (mi2s < 0)
+		return;
+
+	if (--data->mi2s_clk_count[mi2s] > 0)
+		return;
+
+	ret = snd_soc_dai_set_sysclk(cpu_dai, LPAIF_BIT_CLK, 0, 0);
+	if (ret)
+		dev_err(card->dev, "Failed to disable LPAIF bit clk: %d\n", ret);
+}
+
+static const struct snd_soc_ops msm8916_qdsp6_be_ops = {
+	.startup = msm8916_qdsp6_startup,
+	.shutdown = msm8916_qdsp6_shutdown,
+};
+
+static int msm8916_qdsp6_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
+					    struct snd_pcm_hw_params *params)
+{
+	struct snd_interval *rate = hw_param_interval(params,
+					SNDRV_PCM_HW_PARAM_RATE);
+	struct snd_interval *channels = hw_param_interval(params,
+					SNDRV_PCM_HW_PARAM_CHANNELS);
+	struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
+
+	rate->min = rate->max = 48000;
+	channels->min = channels->max = 2;
+	snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
+
+	return 0;
+}
+
+static void msm8916_qdsp6_add_ops(struct snd_soc_card *card)
+{
+	struct snd_soc_dai_link *link;
+	int i;
+
+	/* Make it obvious to userspace that QDSP6 is used */
+	card->components = "qdsp6";
+
+	for_each_card_prelinks(card, i, link) {
+		if (link->no_pcm) {
+			link->init = msm8916_qdsp6_dai_init;
+			link->ops = &msm8916_qdsp6_be_ops;
+			link->be_hw_params_fixup = msm8916_qdsp6_be_hw_params_fixup;
+		}
+	}
+}
+
 static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = {
 
 	SND_SOC_DAPM_MIC("Handset Mic", NULL),
@@ -148,11 +266,16 @@ static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = {
 
 static int apq8016_sbc_platform_probe(struct platform_device *pdev)
 {
+	void (*add_ops)(struct snd_soc_card *card);
 	struct device *dev = &pdev->dev;
 	struct snd_soc_card *card;
 	struct apq8016_sbc_data *data;
 	int ret;
 
+	add_ops = device_get_match_data(&pdev->dev);
+	if (!add_ops)
+		return -EINVAL;
+
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
@@ -177,12 +300,13 @@ static int apq8016_sbc_platform_probe(struct platform_device *pdev)
 
 	snd_soc_card_set_drvdata(card, data);
 
-	apq8016_sbc_add_ops(card);
+	add_ops(card);
 	return devm_snd_soc_register_card(&pdev->dev, card);
 }
 
 static const struct of_device_id apq8016_sbc_device_id[] __maybe_unused = {
-	{ .compatible = "qcom,apq8016-sbc-sndcard" },
+	{ .compatible = "qcom,apq8016-sbc-sndcard", .data = apq8016_sbc_add_ops },
+	{ .compatible = "qcom,msm8916-qdsp6-sndcard", .data = msm8916_qdsp6_add_ops },
 	{},
 };
 MODULE_DEVICE_TABLE(of, apq8016_sbc_device_id);
-- 
2.34.1

Re: [PATCH 5/5] ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6

[Srinivas Kandagatla]

Thanks Stephan for doing this,

I have tested DB410c this use case in the past using similar patch [1].

Over all it looks good, except one comment.

On 02/12/2021 14:55, Stephan Gerhold wrote:
> The apq8016-sbc-sndcard is designed to be used with the LPASS drivers
> (bypassing the combined audio/modem DSP in MSM8916/APQ8016).
> Make it possible to use QDSP6 audio instead for the msm8916-qdsp6-sndcard.
> 
> This only requires adding some additional hooks that set up the DPCM
> backends correctly. Similar code is already used in drivers for newer
> SoCs such as apq8096.c, sdm845.c and sm8250.c.
> 
> A slightly different initialization sequence is used for the apq8016-sbc
> and msm8916-qdsp6 sound card by defining the apq8016_sbc_add_ops()
> function as device match data.

Other alternative is to reuse card->name populated from "qcom,model" 
property to differentiate between both of these.

This should also help in differentiating UCM configs.


> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Few minor nits, other than that it LGTM,

Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> ---
>   sound/soc/qcom/apq8016_sbc.c | 134 +++++++++++++++++++++++++++++++++--
>   1 file changed, 129 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/qcom/apq8016_sbc.c b/sound/soc/qcom/apq8016_sbc.c
> index ba2a98268ee4..f9d69375320e 100644
> --- a/sound/soc/qcom/apq8016_sbc.c
> +++ b/sound/soc/qcom/apq8016_sbc.c
> @@ -17,6 +17,9 @@
>   #include <uapi/linux/input-event-codes.h>
>   #include <dt-bindings/sound/apq8016-lpass.h>
>   #include "common.h"
> +#include "qdsp6/q6afe.h"
> +
> +#define MI2S_COUNT  (MI2S_QUATERNARY + 1)
>   
>   struct apq8016_sbc_data {
>   	struct snd_soc_card card;
> @@ -24,6 +27,7 @@ struct apq8016_sbc_data {
>   	void __iomem *spkr_iomux;
>   	struct snd_soc_jack jack;
>   	bool jack_setup;
> +	int mi2s_clk_count[MI2S_COUNT];
>   };
>   
>   #define MIC_CTRL_TER_WS_SLAVE_SEL	BIT(21)
> @@ -38,10 +42,10 @@ struct apq8016_sbc_data {
>   #define SPKR_CTL_TLMM_WS_EN_SEL_MASK	GENMASK(19, 18)
>   #define SPKR_CTL_TLMM_WS_EN_SEL_SEC	BIT(18)
>   #define DEFAULT_MCLK_RATE		9600000
> +#define MI2S_BCLK_RATE			1536000
>   
> -static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd)
> +static int apq8016_dai_init(struct snd_soc_pcm_runtime *rtd, int mi2s)
>   {
> -	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
>   	struct snd_soc_dai *codec_dai;
>   	struct snd_soc_component *component;
>   	struct snd_soc_card *card = rtd->card;
> @@ -49,7 +53,7 @@ static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd)
>   	int i, rval;
>   	u32 value;
>   
> -	switch (cpu_dai->id) {
> +	switch (mi2s) {
>   	case MI2S_PRIMARY:
>   		writel(readl(pdata->spkr_iomux) | SPKR_CTL_PRI_WS_SLAVE_SEL_11,
>   			pdata->spkr_iomux);
> @@ -128,6 +132,13 @@ static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd)
>   	return 0;
>   }
>   
> +static int apq8016_sbc_dai_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> +
> +	return apq8016_dai_init(rtd, cpu_dai->id);
> +}
> +
>   static void apq8016_sbc_add_ops(struct snd_soc_card *card)
>   {
>   	struct snd_soc_dai_link *link;
> @@ -137,6 +148,113 @@ static void apq8016_sbc_add_ops(struct snd_soc_card *card)
>   		link->init = apq8016_sbc_dai_init;
>   }
>   
> +static int qdsp6_dai_get_lpass_id(struct snd_soc_dai *cpu_dai)
> +{
> +	switch (cpu_dai->id) {
> +	case PRIMARY_MI2S_RX:
> +	case PRIMARY_MI2S_TX:
> +		return MI2S_PRIMARY;
> +	case SECONDARY_MI2S_RX:
> +	case SECONDARY_MI2S_TX:
> +		return MI2S_SECONDARY;
> +	case TERTIARY_MI2S_RX:
> +	case TERTIARY_MI2S_TX:
> +		return MI2S_TERTIARY;
> +	case QUATERNARY_MI2S_RX:
> +	case QUATERNARY_MI2S_TX:
> +		return MI2S_QUATERNARY;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int msm8916_qdsp6_dai_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> +
> +	snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_CBS_CFS);
> +	return apq8016_dai_init(rtd, qdsp6_dai_get_lpass_id(cpu_dai));
> +}
> +
> +static int msm8916_qdsp6_startup(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_card *card = rtd->card;
> +	struct apq8016_sbc_data *data = snd_soc_card_get_drvdata(card);
> +	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> +	int mi2s, ret;
> +
> +	mi2s = qdsp6_dai_get_lpass_id(cpu_dai);
> +	if (mi2s < 0)
> +		return mi2s;
> +
> +	if (++data->mi2s_clk_count[mi2s] > 1)
> +		return 0;
> +

Am assuming that as you are not setting any DIGITAL CDC clock here you 
might be using an external codec.

> +	ret = snd_soc_dai_set_sysclk(cpu_dai, LPAIF_BIT_CLK, MI2S_BCLK_RATE, 0);
> +	if (ret)
> +		dev_err(card->dev, "Failed to enable LPAIF bit clk: %d\n", ret);
> +	return ret;
> +}
> +
> +static void msm8916_qdsp6_shutdown(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_card *card = rtd->card;
> +	struct apq8016_sbc_data *data = snd_soc_card_get_drvdata(card);
> +	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> +	int mi2s, ret;
> +
> +	mi2s = qdsp6_dai_get_lpass_id(cpu_dai);
> +	if (mi2s < 0)
> +		return;
> +
> +	if (--data->mi2s_clk_count[mi2s] > 0)
> +		return;
> +
> +	ret = snd_soc_dai_set_sysclk(cpu_dai, LPAIF_BIT_CLK, 0, 0);
> +	if (ret)
> +		dev_err(card->dev, "Failed to disable LPAIF bit clk: %d\n", ret);
> +}
> +
> +static const struct snd_soc_ops msm8916_qdsp6_be_ops = {
> +	.startup = msm8916_qdsp6_startup,
> +	.shutdown = msm8916_qdsp6_shutdown,
> +};
> +
> +static int msm8916_qdsp6_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
> +					    struct snd_pcm_hw_params *params)
> +{
> +	struct snd_interval *rate = hw_param_interval(params,
> +					SNDRV_PCM_HW_PARAM_RATE);
> +	struct snd_interval *channels = hw_param_interval(params,
> +					SNDRV_PCM_HW_PARAM_CHANNELS);
> +	struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
> +
> +	rate->min = rate->max = 48000;
> +	channels->min = channels->max = 2;
> +	snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
> +
> +	return 0;
> +}
> +
> +static void msm8916_qdsp6_add_ops(struct snd_soc_card *card)
> +{
> +	struct snd_soc_dai_link *link;
> +	int i;
> +
> +	/* Make it obvious to userspace that QDSP6 is used */
> +	card->components = "qdsp6";
> +
> +	for_each_card_prelinks(card, i, link) {
> +		if (link->no_pcm) {
> +			link->init = msm8916_qdsp6_dai_init;
> +			link->ops = &msm8916_qdsp6_be_ops;
> +			link->be_hw_params_fixup = msm8916_qdsp6_be_hw_params_fixup;
> +		}
> +	}
> +}
> +
>   static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = {
>   
>   	SND_SOC_DAPM_MIC("Handset Mic", NULL),
> @@ -148,11 +266,16 @@ static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = {
>   
>   static int apq8016_sbc_platform_probe(struct platform_device *pdev)
>   {
> +	void (*add_ops)(struct snd_soc_card *card);
>   	struct device *dev = &pdev->dev;
>   	struct snd_soc_card *card;
>   	struct apq8016_sbc_data *data;
>   	int ret;
>   
> +	add_ops = device_get_match_data(&pdev->dev);
> +	if (!add_ops)
> +		return -EINVAL;

We will never hit the error case here because without a match we can not 
even enter the probe function.

--srini
> +
>   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;
> @@ -177,12 +300,13 @@ static int apq8016_sbc_platform_probe(struct platform_device *pdev)
>   
>   	snd_soc_card_set_drvdata(card, data);
>   
> -	apq8016_sbc_add_ops(card);
> +	add_ops(card);
>   	return devm_snd_soc_register_card(&pdev->dev, card);
>   }
>   
>   static const struct of_device_id apq8016_sbc_device_id[] __maybe_unused = {
> -	{ .compatible = "qcom,apq8016-sbc-sndcard" },
> +	{ .compatible = "qcom,apq8016-sbc-sndcard", .data = apq8016_sbc_add_ops },
> +	{ .compatible = "qcom,msm8916-qdsp6-sndcard", .data = msm8916_qdsp6_add_ops },
>   	{},
>   };
>   MODULE_DEVICE_TABLE(of, apq8016_sbc_device_id);
> 
[1]: 
https://git.linaro.org/people/srinivas.kandagatla/linux.git/commit/?h=q6dsp-db410c-v5.11-rc6&id=9d62822a5f66cd06a5e6b31f6b841a25c143926c

Re: [PATCH 5/5] ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6

[Stephan Gerhold]

Hi Srinivas,

Thanks for your review!

On Fri, Dec 03, 2021 at 10:35:08AM +0000, Srinivas Kandagatla wrote:
> I have tested DB410c this use case in the past using similar patch [1].
> 

Did you use a different modem DSP firmware? (An older one maybe?)
In my tests the newer ones seem to have QDSP6 audio completely broken,
my DB410c simply rebooted when I tried it.

> On 02/12/2021 14:55, Stephan Gerhold wrote:
> > The apq8016-sbc-sndcard is designed to be used with the LPASS drivers
> > (bypassing the combined audio/modem DSP in MSM8916/APQ8016).
> > Make it possible to use QDSP6 audio instead for the msm8916-qdsp6-sndcard.
> > 
> > This only requires adding some additional hooks that set up the DPCM
> > backends correctly. Similar code is already used in drivers for newer
> > SoCs such as apq8096.c, sdm845.c and sm8250.c.
> > 
> > A slightly different initialization sequence is used for the apq8016-sbc
> > and msm8916-qdsp6 sound card by defining the apq8016_sbc_add_ops()
> > function as device match data.
> 
> Other alternative is to reuse card->name populated from "qcom,model"
> property to differentiate between both of these.
> 
> This should also help in differentiating UCM configs.
> 

I have "qdsp6" in card->components to differentiate the setups in UCM
configs. I think this is a more flexible approach than adding it to the
card model. It can be checked in UCM using ${CardComponents}.

In my setup the card "model" identifies the device in use (e.g.
smartphone X with a stereo speaker setup). This device might use the
DSP bypass (apq8016-sbc-sndcard) or QDSP6 (msm8916-qdsp6-sndcard),
depending on user preference. In UCM this is detected by checking
if ${CardComponents} contains "qdsp6" or not.

The reason for supporting both setups is that they both have their
advantages and disadvantages. The DSP must be used when the modem is
needed, but otherwise the LPASS driver tends to give more easy control
about sample rate, latency etc.

> 
> > 
> > Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> 
> Few minor nits, other than that it LGTM,
> 
> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> > ---
> >   sound/soc/qcom/apq8016_sbc.c | 134 +++++++++++++++++++++++++++++++++--
> >   1 file changed, 129 insertions(+), 5 deletions(-)
> > 
> > diff --git a/sound/soc/qcom/apq8016_sbc.c b/sound/soc/qcom/apq8016_sbc.c
> > index ba2a98268ee4..f9d69375320e 100644
> > --- a/sound/soc/qcom/apq8016_sbc.c
> > +++ b/sound/soc/qcom/apq8016_sbc.c
> > [...]
> > +static int msm8916_qdsp6_startup(struct snd_pcm_substream *substream)
> > +{
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct snd_soc_card *card = rtd->card;
> > +	struct apq8016_sbc_data *data = snd_soc_card_get_drvdata(card);
> > +	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> > +	int mi2s, ret;
> > +
> > +	mi2s = qdsp6_dai_get_lpass_id(cpu_dai);
> > +	if (mi2s < 0)
> > +		return mi2s;
> > +
> > +	if (++data->mi2s_clk_count[mi2s] > 1)
> > +		return 0;
> > +
> 
> Am assuming that as you are not setting any DIGITAL CDC clock here you might
> be using an external codec.
> 

For apq8016-sbc the digital clock is controlled by msm8916-wcd-digital
through the reference in the device tree (<&gcc GCC_CODEC_DIGCODEC_CLK>).
It must be carefully managed, because it is needed for register access
in that driver.

Since QDSP6 also allows controlling this clock though LPAIF_DIG_CLK
it is a bit of a hack to bypass it using the GCC driver. However, I kept
the same setup for simplicity and it has been working just fine so far.

AFAICT in your commit you simply turn on the clock twice, once directly
using GCC and once indirectly via LPAIF_DIG_CLK in QDSP6. :-)

> > +	ret = snd_soc_dai_set_sysclk(cpu_dai, LPAIF_BIT_CLK, MI2S_BCLK_RATE, 0);
> > +	if (ret)
> > +		dev_err(card->dev, "Failed to enable LPAIF bit clk: %d\n", ret);
> > +	return ret;
> > +}
> > +
> > [...]
> > @@ -148,11 +266,16 @@ static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = {
> >   static int apq8016_sbc_platform_probe(struct platform_device *pdev)
> >   {
> > +	void (*add_ops)(struct snd_soc_card *card);
> >   	struct device *dev = &pdev->dev;
> >   	struct snd_soc_card *card;
> >   	struct apq8016_sbc_data *data;
> >   	int ret;
> > +	add_ops = device_get_match_data(&pdev->dev);
> > +	if (!add_ops)
> > +		return -EINVAL;
> 
> We will never hit the error case here because without a match we can not
> even enter the probe function.
> 

Theoretically it's possible to create platform devices through other
ways than the device tree (think of old board C files for example).
I agree that nobody should do that, but having this check here
at least avoids a NULL pointer dereference in this unlikely scenario.

Please let me know if I should remove it anyway, that's fine for me!

Thanks,
Stephan 

Re: [PATCH 5/5] ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6

[Srinivas Kandagatla]

On 03/12/2021 14:36, Stephan Gerhold wrote:
> Hi Srinivas,
> 
> Thanks for your review!
> 
> On Fri, Dec 03, 2021 at 10:35:08AM +0000, Srinivas Kandagatla wrote:
>> I have tested DB410c this use case in the past using similar patch [1].
>>
> 
> Did you use a different modem DSP firmware? (An older one maybe?)

It was very old which came with some Android release I guess.


> In my tests the newer ones seem to have QDSP6 audio completely broken,
> my DB410c simply rebooted when I tried it.
> 
>> On 02/12/2021 14:55, Stephan Gerhold wrote:
>>> The apq8016-sbc-sndcard is designed to be used with the LPASS drivers
>>> (bypassing the combined audio/modem DSP in MSM8916/APQ8016).
>>> Make it possible to use QDSP6 audio instead for the msm8916-qdsp6-sndcard.
>>>
>>> This only requires adding some additional hooks that set up the DPCM
>>> backends correctly. Similar code is already used in drivers for newer
>>> SoCs such as apq8096.c, sdm845.c and sm8250.c.
>>>
>>> A slightly different initialization sequence is used for the apq8016-sbc
>>> and msm8916-qdsp6 sound card by defining the apq8016_sbc_add_ops()
>>> function as device match data.
>>
>> Other alternative is to reuse card->name populated from "qcom,model"
>> property to differentiate between both of these.
>>
>> This should also help in differentiating UCM configs.
>>
> 
> I have "qdsp6" in card->components to differentiate the setups in UCM
> configs. I think this is a more flexible approach than adding it to the
> card model. It can be checked in UCM using ${CardComponents}.
> 
> In my setup the card "model" identifies the device in use (e.g.
> smartphone X with a stereo speaker setup). This device might use the
> DSP bypass (apq8016-sbc-sndcard) or QDSP6 (msm8916-qdsp6-sndcard),
> depending on user preference. In UCM this is detected by checking
> if ${CardComponents} contains "qdsp6" or not.

That is other way to do it too.


> 
> The reason for supporting both setups is that they both have their
> advantages and disadvantages. The DSP must be used when the modem is
> needed, but otherwise the LPASS driver tends to give more easy control
> about sample rate, latency etc.
> 
>>
>>>
>>> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>>
>> Few minor nits, other than that it LGTM,
>>
>> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>>> ---
>>>    sound/soc/qcom/apq8016_sbc.c | 134 +++++++++++++++++++++++++++++++++--
>>>    1 file changed, 129 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/sound/soc/qcom/apq8016_sbc.c b/sound/soc/qcom/apq8016_sbc.c
>>> index ba2a98268ee4..f9d69375320e 100644
>>> --- a/sound/soc/qcom/apq8016_sbc.c
>>> +++ b/sound/soc/qcom/apq8016_sbc.c
>>> [...]
>>> +static int msm8916_qdsp6_startup(struct snd_pcm_substream *substream)
>>> +{
>>> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>>> +	struct snd_soc_card *card = rtd->card;
>>> +	struct apq8016_sbc_data *data = snd_soc_card_get_drvdata(card);
>>> +	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
>>> +	int mi2s, ret;
>>> +
>>> +	mi2s = qdsp6_dai_get_lpass_id(cpu_dai);
>>> +	if (mi2s < 0)
>>> +		return mi2s;
>>> +
>>> +	if (++data->mi2s_clk_count[mi2s] > 1)
>>> +		return 0;
>>> +
>>
>> Am assuming that as you are not setting any DIGITAL CDC clock here you might
>> be using an external codec.
>>
> 
> For apq8016-sbc the digital clock is controlled by msm8916-wcd-digital
> through the reference in the device tree (<&gcc GCC_CODEC_DIGCODEC_CLK>).
> It must be carefully managed, because it is needed for register access
> in that driver.
> 
> Since QDSP6 also allows controlling this clock though LPAIF_DIG_CLK
> it is a bit of a hack to bypass it using the GCC driver. However, I kept
> the same setup for simplicity and it has been working just fine so far.

Nice!

> 
> AFAICT in your commit you simply turn on the clock twice, once directly
> using GCC and once indirectly via LPAIF_DIG_CLK in QDSP6. :-)

Yes, that was bit of test code TBH.

> 
>>> +	ret = snd_soc_dai_set_sysclk(cpu_dai, LPAIF_BIT_CLK, MI2S_BCLK_RATE, 0);
>>> +	if (ret)
>>> +		dev_err(card->dev, "Failed to enable LPAIF bit clk: %d\n", ret);
>>> +	return ret;
>>> +}
>>> +
>>> [...]
>>> @@ -148,11 +266,16 @@ static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = {
>>>    static int apq8016_sbc_platform_probe(struct platform_device *pdev)
>>>    {
>>> +	void (*add_ops)(struct snd_soc_card *card);
>>>    	struct device *dev = &pdev->dev;
>>>    	struct snd_soc_card *card;
>>>    	struct apq8016_sbc_data *data;
>>>    	int ret;
>>> +	add_ops = device_get_match_data(&pdev->dev);
>>> +	if (!add_ops)
>>> +		return -EINVAL;
>>
>> We will never hit the error case here because without a match we can not
>> even enter the probe function.
>>
> 
> Theoretically it's possible to create platform devices through other
> ways than the device tree (think of old board C files for example).
> I agree that nobody should do that, but having this check here
> at least avoids a NULL pointer dereference in this unlikely scenario.
> 
> Please let me know if I should remove it anyway, that's fine for me!

TBH, I don't have very strong opinion on this.


--srini
> 
> Thanks,
> Stephan
> 

Re: [PATCH 5/5] ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6

[Stephan Gerhold]

On Fri, Dec 03, 2021 at 02:52:43PM +0000, Srinivas Kandagatla wrote:
> On 03/12/2021 14:36, Stephan Gerhold wrote:
> > On Fri, Dec 03, 2021 at 10:35:08AM +0000, Srinivas Kandagatla wrote:
> > > I have tested DB410c this use case in the past using similar patch [1].
> > > 
> > 
> > Did you use a different modem DSP firmware? (An older one maybe?)
> 
> It was very old which came with some Android release I guess.
> 

Right, that should be similar to the ones used on MSM8916
smartphones/tablets. I was really glad that the qdsp6 drivers (q6asm,
q6afe, ...) worked without any changes on MSM8916 by the way, thanks a
lot for all your work on them!

> > > > @@ -148,11 +266,16 @@ static const struct snd_soc_dapm_widget apq8016_sbc_dapm_widgets[] = {
> > > >    static int apq8016_sbc_platform_probe(struct platform_device *pdev)
> > > >    {
> > > > +	void (*add_ops)(struct snd_soc_card *card);
> > > >    	struct device *dev = &pdev->dev;
> > > >    	struct snd_soc_card *card;
> > > >    	struct apq8016_sbc_data *data;
> > > >    	int ret;
> > > > +	add_ops = device_get_match_data(&pdev->dev);
> > > > +	if (!add_ops)
> > > > +		return -EINVAL;
> > > 
> > > We will never hit the error case here because without a match we can not
> > > even enter the probe function.
> > > 
> > 
> > Theoretically it's possible to create platform devices through other
> > ways than the device tree (think of old board C files for example).
> > I agree that nobody should do that, but having this check here
> > at least avoids a NULL pointer dereference in this unlikely scenario.
> > 
> > Please let me know if I should remove it anyway, that's fine for me!
> 
> TBH, I don't have very strong opinion on this.
> 

Great, can I assume your Reviewed-by: applies without any changes then?

Thanks,
Stephan

Re: [PATCH 5/5] ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6

[Srinivas Kandagatla]

On 03/12/2021 15:09, Stephan Gerhold wrote:
>>>>> +	add_ops = device_get_match_data(&pdev->dev);
>>>>> +	if (!add_ops)
>>>>> +		return -EINVAL;
>>>> We will never hit the error case here because without a match we can not
>>>> even enter the probe function.
>>>>
>>> Theoretically it's possible to create platform devices through other
>>> ways than the device tree (think of old board C files for example).
>>> I agree that nobody should do that, but having this check here
>>> at least avoids a NULL pointer dereference in this unlikely scenario.
>>>
>>> Please let me know if I should remove it anyway, that's fine for me!
>> TBH, I don't have very strong opinion on this.
>>
> Great, can I assume your Reviewed-by: applies without any changes then?

yes sure.

--srini

Re: [PATCH 0/5] ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6

[Srinivas Kandagatla]

Hi Stephan,

On 02/12/2021 14:55, Stephan Gerhold wrote:
> This series makes it possible to route audio through the combined
> audio/modem DSP on MSM8916/APQ8016 devices instead of bypassing it using
> the LPASS drivers. This is necessary to support certain functionality such
> as voice call audio. See PATCH 4/5 for details.
> 
> Also, qcom,apq8016-sbc.txt is converted to DT schema by adding it to the
> existing qcom,sm8250.yaml. The bindings are similar enough that it is easier
> to share a single schema instead of duplicating everything into multiple ones.

Yes this is really a good start, we should at some point in time move to 
a common yaml file, currently most of the dsp based dt-bindings very 
simillar, except some codec specific routings.


> 
> Stephan Gerhold (5):
>    ASoC: dt-bindings: qcom: sm8250: Drop redundant MultiMedia routes
>    ASoC: dt-bindings: qcom: sm8250: Document "aux-devs"
>    ASoC: dt-bindings: qcom: apq8016-sbc: Move to qcom,sm8250 DT schema
>    ASoC: dt-bindings: qcom: Document qcom,msm8916-qdsp6-sndcard
>      compatible
>    ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6
> 


--srini
>   .../bindings/sound/qcom,apq8016-sbc.txt       |  96 -------------
>   .../bindings/sound/qcom,sm8250.yaml           | 136 +++++++++++++++++-
>   sound/soc/qcom/apq8016_sbc.c                  | 134 ++++++++++++++++-
>   3 files changed, 259 insertions(+), 107 deletions(-)
>   delete mode 100644 Documentation/devicetree/bindings/sound/qcom,apq8016-sbc.txt
> 

Re: [PATCH 0/5] ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6

[Mark Brown]

On Thu, 2 Dec 2021 15:55:00 +0100, Stephan Gerhold wrote:
> This series makes it possible to route audio through the combined
> audio/modem DSP on MSM8916/APQ8016 devices instead of bypassing it using
> the LPASS drivers. This is necessary to support certain functionality such
> as voice call audio. See PATCH 4/5 for details.
> 
> Also, qcom,apq8016-sbc.txt is converted to DT schema by adding it to the
> existing qcom,sm8250.yaml. The bindings are similar enough that it is easier
> to share a single schema instead of duplicating everything into multiple ones.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/5] ASoC: dt-bindings: qcom: sm8250: Drop redundant MultiMedia routes
      commit: c55676ec292e0ff83261eb61efaf99a91079a3b8
[2/5] ASoC: dt-bindings: qcom: sm8250: Document "aux-devs"
      commit: 1875ae76f82c5c9acd7b7f44bd9226fbcbe858b7
[3/5] ASoC: dt-bindings: qcom: apq8016-sbc: Move to qcom,sm8250 DT schema
      commit: b7875d88bf70100d2fe0dc08072018f994ccd6c4
[4/5] ASoC: dt-bindings: qcom: Document qcom,msm8916-qdsp6-sndcard compatible
      commit: 38192dc36f1fe1615b7a12cc78b9354d6b4ba8b7
[5/5] ASoC: qcom: apq8016_sbc: Allow routing audio through QDSP6
      commit: 190357e1e09f271566e91fcd108c161f7e25a253

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark