All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] ASoC: qcom: qdsp6: Add MI2S clock control
@ 2026-06-08  2:30 Mohammad Rafi Shaik
  2026-06-08  2:30 ` [PATCH v2 1/3] ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode Mohammad Rafi Shaik
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Mohammad Rafi Shaik @ 2026-06-08  2:30 UTC (permalink / raw)
  To: Srinivas Kandagatla, Liam Girdwood, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela, Takashi Iwai
  Cc: Krzysztof Kozlowski, linux-arm-msm, linux-sound, devicetree,
	linux-kernel

Add support for MI2S clock control within q6apm-lpass DAIs, including
handling of MCLK, BCLK, and ECLK via the DAI .set_sysclk callback.
Each MI2S port now retrieves its clock handles from the device tree,
allowing per-port clock configuration and proper enable/disable during
startup and shutdown.

On platforms such as Monaco and Lemans, third-party codecs are
hardware-wired to the SoC and do not always have an in-tree codec
driver to manage their clocks. For these designs, clock line
enablement must be driven from the platform side, and this
series provides the necessary support for that.

On QAIF-based platforms such as Shikra and Hawi, responsibility
for voting I2S MCLK and bit-clock has moved from the DSP to the
kernel. This series introduces the required device tree binding
support to represent and vote for these clocks from the kernel.

Enhances the sc8280xp machine driver to set the boards spacific
configurations.

This series depends on:
- https://lore.kernel.org/all/20260607-rubikpi-next-20260605-v1-3-7f334e16fea6@thundersoft.com/

---
Changes in v3:
- Added a detailed commit description to clearly explain the need for this change.
- Improved the machine driver based on Neil’s feedback.
- Link to v1: https://lore.kernel.org/all/20260309111300.2484262-1-mohammad.rafi.shaik@oss.qualcomm.com/
---
Mohammad Rafi Shaik (3):
  ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode
  ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control
  ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for
    board-specific config

 .../bindings/sound/qcom,q6apm-lpass-dais.yaml |  57 +++++
 sound/soc/qcom/qdsp6/q6apm-lpass-dais.c       | 137 ++++++++++-
 sound/soc/qcom/qdsp6/q6prm.h                  |   4 +
 sound/soc/qcom/sc8280xp.c                     | 213 ++++++++++++++++--
 4 files changed, 391 insertions(+), 20 deletions(-)


base-commit: 6e845bcb78c95af935094040bd4edc3c2b6dd784
prerequisite-patch-id: 2f1bd3efac328030dd8efe28fb95f84603868043
-- 
2.34.1


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

* [PATCH v2 1/3] ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode
  2026-06-08  2:30 [PATCH v2 0/3] ASoC: qcom: qdsp6: Add MI2S clock control Mohammad Rafi Shaik
@ 2026-06-08  2:30 ` Mohammad Rafi Shaik
  2026-06-08  4:44   ` sashiko-bot
  2026-06-08  9:59   ` Krzysztof Kozlowski
  2026-06-08  2:30 ` [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control Mohammad Rafi Shaik
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 12+ messages in thread
From: Mohammad Rafi Shaik @ 2026-06-08  2:30 UTC (permalink / raw)
  To: Srinivas Kandagatla, Liam Girdwood, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela, Takashi Iwai
  Cc: Krzysztof Kozlowski, linux-arm-msm, linux-sound, devicetree,
	linux-kernel, Srinivas Kandagatla

Extend the qcom,q6apm-lpass-dais device tree binding to explicitly
describe Digital Audio Interface (DAI) child nodes.

Add #address-cells and #size-cells to allow representation of multiple
DAI instances as child nodes, and define a dai@<id> pattern to document
per-DAI properties such as the interface ID and associated clocks.

On platforms such as Monaco and Lemans, third-party codecs are hardware
wired to the SoC and do not always have an in-tree codec driver to manage
their clocks. For these designs, clock line enablement must be driven
from the platform side, and this series provides the necessary support
for that.

On QAIF-based platforms such as Shikra and Hawi, responsibility for voting
I2S MCLK and BCLK has moved from the DSP to the kernel. This series
introduces the required device tree binding support to represent and
vote for these clocks from the kernel.

Co-developed-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 .../bindings/sound/qcom,q6apm-lpass-dais.yaml | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/qcom,q6apm-lpass-dais.yaml b/Documentation/devicetree/bindings/sound/qcom,q6apm-lpass-dais.yaml
index 2fb95544d..4c4c73778 100644
--- a/Documentation/devicetree/bindings/sound/qcom,q6apm-lpass-dais.yaml
+++ b/Documentation/devicetree/bindings/sound/qcom,q6apm-lpass-dais.yaml
@@ -21,6 +21,48 @@ properties:
   '#sound-dai-cells':
     const: 1
 
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+# Digital Audio Interfaces
+patternProperties:
+  '^dai@[0-9]+$':
+    type: object
+    description:
+      Q6DSP Digital Audio Interfaces.
+
+    properties:
+      reg:
+        maxItems: 1
+        description:
+          Digital Audio Interface ID
+
+      clocks:
+        minItems: 1
+        items:
+          - description: MI2S master clock
+          - description: MI2S bit clock
+          - description: MI2S external bit clock
+
+      clock-names:
+        minItems: 1
+        items:
+          - const: mclk
+          - const: bclk
+          - const: eclk
+
+    dependencies:
+      clocks: [ clock-names ]
+      clock-names: [ clocks ]
+
+    required:
+      - reg
+
+    additionalProperties: false
+
 required:
   - compatible
   - '#sound-dai-cells'
@@ -29,7 +71,22 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/sound/qcom,q6afe.h>
+
     dais {
         compatible = "qcom,q6apm-lpass-dais";
         #sound-dai-cells = <1>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        dai@16 {
+           reg = <PRIMARY_MI2S_RX>;
+           clocks = <&q6prmcc LPASS_CLK_ID_MCLK_1
+                              LPASS_CLK_ATTRIBUTE_COUPLE_NO>,
+                    <&q6prmcc LPASS_CLK_ID_PRI_MI2S_IBIT
+                              LPASS_CLK_ATTRIBUTE_COUPLE_NO>,
+                    <&q6prmcc LPASS_CLK_ID_PRI_MI2S_EBIT
+                              LPASS_CLK_ATTRIBUTE_COUPLE_NO>;
+           clock-names = "mclk", "bclk", "eclk";
+        };
     };
-- 
2.34.1


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

* [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control
  2026-06-08  2:30 [PATCH v2 0/3] ASoC: qcom: qdsp6: Add MI2S clock control Mohammad Rafi Shaik
  2026-06-08  2:30 ` [PATCH v2 1/3] ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode Mohammad Rafi Shaik
@ 2026-06-08  2:30 ` Mohammad Rafi Shaik
  2026-06-08  2:38   ` sashiko-bot
                     ` (2 more replies)
  2026-06-08  2:30 ` [PATCH v2 3/3] ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config Mohammad Rafi Shaik
  2026-06-08  8:23 ` [PATCH v2 0/3] ASoC: qcom: qdsp6: Add MI2S clock control Neil Armstrong
  3 siblings, 3 replies; 12+ messages in thread
From: Mohammad Rafi Shaik @ 2026-06-08  2:30 UTC (permalink / raw)
  To: Srinivas Kandagatla, Liam Girdwood, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela, Takashi Iwai
  Cc: Krzysztof Kozlowski, linux-arm-msm, linux-sound, devicetree,
	linux-kernel, Srinivas Kandagatla

Add support for MI2S clock control within q6apm-lpass DAIs, including
handling of MCLK, BCLK, and ECLK via the DAI .set_sysclk callback.
Each MI2S port now retrieves its clock handles from the device tree,
allowing per-port clock configuration and proper enable/disable during
startup and shutdown.

Co-developed-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 sound/soc/qcom/qdsp6/q6apm-lpass-dais.c | 137 +++++++++++++++++++++++-
 sound/soc/qcom/qdsp6/q6prm.h            |   4 +
 2 files changed, 139 insertions(+), 2 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
index 006b28348..143750afb 100644
--- a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
+++ b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
@@ -2,10 +2,12 @@
 // Copyright (c) 2021, Linaro Limited
 
 #include <dt-bindings/sound/qcom,q6dsp-lpass-ports.h>
+#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <sound/pcm.h>
@@ -15,13 +17,22 @@
 #include "q6dsp-common.h"
 #include "audioreach.h"
 #include "q6apm.h"
+#include "q6prm.h"
 
 #define AUDIOREACH_BE_PCM_BASE	16
 
+struct q6apm_dai_priv_data {
+	struct clk *mclk;
+	struct clk *bclk;
+	struct clk *eclk;
+	bool mclk_enabled, bclk_enabled, eclk_enabled;
+};
+
 struct q6apm_lpass_dai_data {
 	struct q6apm_graph *graph[APM_PORT_MAX];
 	bool is_port_started[APM_PORT_MAX];
 	struct audioreach_module_config module_config[APM_PORT_MAX];
+	struct q6apm_dai_priv_data priv[APM_PORT_MAX];
 };
 
 static int q6dma_set_channel_map(struct snd_soc_dai *dai,
@@ -251,6 +262,70 @@ static int q6apm_lpass_dai_startup(struct snd_pcm_substream *substream, struct s
 	return 0;
 }
 
+static int q6i2s_dai_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
+{
+	return q6apm_lpass_dai_startup(substream, dai);
+}
+
+static void q6i2s_lpass_dai_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
+{
+	struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(dai->dev);
+
+	if (dai_data->priv[dai->id].mclk_enabled) {
+		clk_disable_unprepare(dai_data->priv[dai->id].mclk);
+		dai_data->priv[dai->id].mclk_enabled = false;
+	}
+
+	if (dai_data->priv[dai->id].bclk_enabled) {
+		clk_disable_unprepare(dai_data->priv[dai->id].bclk);
+		dai_data->priv[dai->id].bclk_enabled = false;
+	}
+
+	if (dai_data->priv[dai->id].eclk_enabled) {
+		clk_disable_unprepare(dai_data->priv[dai->id].eclk);
+		dai_data->priv[dai->id].eclk_enabled = false;
+	}
+	q6apm_lpass_dai_shutdown(substream, dai);
+}
+
+static int q6i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir)
+{
+	struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(dai->dev);
+	struct clk *sysclk;
+	bool *enabled;
+	int ret = 0;
+
+	switch (clk_id) {
+	case LPAIF_MI2S_MCLK:
+		sysclk = dai_data->priv[dai->id].mclk;
+		enabled = &dai_data->priv[dai->id].mclk_enabled;
+		break;
+	case LPAIF_MI2S_BCLK:
+		sysclk = dai_data->priv[dai->id].bclk;
+		enabled = &dai_data->priv[dai->id].bclk_enabled;
+		break;
+	case LPAIF_MI2S_ECLK:
+		sysclk = dai_data->priv[dai->id].eclk;
+		enabled = &dai_data->priv[dai->id].eclk_enabled;
+		break;
+	default:
+		break;
+	}
+
+	if (sysclk) {
+		clk_set_rate(sysclk, freq);
+		ret = clk_prepare_enable(sysclk);
+		if (ret) {
+			dev_err(dai->dev, "Error, Unable to prepare (%d) sysclk\n", clk_id);
+			return ret;
+		}
+
+		*enabled = true;
+	}
+
+	return ret;
+}
+
 static int q6i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 {
 	struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(dai->dev);
@@ -272,11 +347,12 @@ static const struct snd_soc_dai_ops q6dma_ops = {
 
 static const struct snd_soc_dai_ops q6i2s_ops = {
 	.prepare	= q6apm_lpass_dai_prepare,
-	.startup	= q6apm_lpass_dai_startup,
-	.shutdown	= q6apm_lpass_dai_shutdown,
+	.startup	= q6i2s_dai_startup,
+	.shutdown	= q6i2s_lpass_dai_shutdown,
 	.set_channel_map  = q6dma_set_channel_map,
 	.hw_params        = q6dma_hw_params,
 	.set_fmt	= q6i2s_set_fmt,
+	.set_sysclk	= q6i2s_set_sysclk,
 	.trigger	= q6apm_lpass_dai_trigger,
 };
 
@@ -297,6 +373,59 @@ static const struct snd_soc_component_driver q6apm_lpass_dai_component = {
 	.remove_order   = SND_SOC_COMP_ORDER_FIRST,
 };
 
+static int of_q6apm_parse_dai_data(struct device *dev,
+				   struct q6apm_lpass_dai_data *data)
+{
+	struct device_node *node;
+	int ret;
+
+	for_each_child_of_node(dev->of_node, node) {
+		struct q6apm_dai_priv_data *priv;
+		int id;
+
+		ret = of_property_read_u32(node, "reg", &id);
+		if (ret || id < 0 || id >= APM_PORT_MAX) {
+			dev_err(dev, "valid dai id not found:%d\n", ret);
+			continue;
+		}
+
+		switch (id) {
+		/* MI2S specific properties */
+		case PRIMARY_MI2S_RX ... QUATERNARY_MI2S_TX:
+		case QUINARY_MI2S_RX ... QUINARY_MI2S_TX:
+			priv = &data->priv[id];
+			priv->mclk = of_clk_get_by_name(node, "mclk");
+			if (IS_ERR(priv->mclk)) {
+				if (PTR_ERR(priv->mclk) == -EPROBE_DEFER)
+					return dev_err_probe(dev, PTR_ERR(priv->mclk),
+							     "unable to get mi2s mclk\n");
+				priv->mclk = NULL;
+			}
+
+			priv->bclk = of_clk_get_by_name(node, "bclk");
+			if (IS_ERR(priv->bclk)) {
+				if (PTR_ERR(priv->bclk) == -EPROBE_DEFER)
+					return dev_err_probe(dev, PTR_ERR(priv->bclk),
+							     "unable to get mi2s bclk\n");
+				priv->bclk = NULL;
+			}
+
+			priv->eclk = of_clk_get_by_name(node, "eclk");
+			if (IS_ERR(priv->eclk)) {
+				if (PTR_ERR(priv->eclk) == -EPROBE_DEFER)
+					return dev_err_probe(dev, PTR_ERR(priv->eclk),
+							     "unable to get mi2s eclk\n");
+				priv->eclk = NULL;
+			}
+			break;
+		default:
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static int q6apm_lpass_dai_dev_probe(struct platform_device *pdev)
 {
 	struct q6dsp_audio_port_dai_driver_config cfg;
@@ -304,12 +433,16 @@ static int q6apm_lpass_dai_dev_probe(struct platform_device *pdev)
 	struct snd_soc_dai_driver *dais;
 	struct device *dev = &pdev->dev;
 	int num_dais;
+	int ret;
 
 	dai_data = devm_kzalloc(dev, sizeof(*dai_data), GFP_KERNEL);
 	if (!dai_data)
 		return -ENOMEM;
 
 	dev_set_drvdata(dev, dai_data);
+	ret = of_q6apm_parse_dai_data(dev, dai_data);
+	if (ret)
+		return ret;
 
 	memset(&cfg, 0, sizeof(cfg));
 	cfg.q6i2s_ops = &q6i2s_ops;
diff --git a/sound/soc/qcom/qdsp6/q6prm.h b/sound/soc/qcom/qdsp6/q6prm.h
index 6917e70bc..7b751486c 100644
--- a/sound/soc/qcom/qdsp6/q6prm.h
+++ b/sound/soc/qcom/qdsp6/q6prm.h
@@ -3,6 +3,10 @@
 #ifndef __Q6PRM_H__
 #define __Q6PRM_H__
 
+#define LPAIF_MI2S_MCLK 1
+#define LPAIF_MI2S_BCLK 2
+#define LPAIF_MI2S_ECLK 3
+
 /* Clock ID for Primary I2S IBIT */
 #define Q6PRM_LPASS_CLK_ID_PRI_MI2S_IBIT                          0x100
 /* Clock ID for Primary I2S EBIT */
-- 
2.34.1


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

* [PATCH v2 3/3] ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config
  2026-06-08  2:30 [PATCH v2 0/3] ASoC: qcom: qdsp6: Add MI2S clock control Mohammad Rafi Shaik
  2026-06-08  2:30 ` [PATCH v2 1/3] ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode Mohammad Rafi Shaik
  2026-06-08  2:30 ` [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control Mohammad Rafi Shaik
@ 2026-06-08  2:30 ` Mohammad Rafi Shaik
  2026-06-08  2:41   ` sashiko-bot
  2026-06-08 12:08   ` Mark Brown
  2026-06-08  8:23 ` [PATCH v2 0/3] ASoC: qcom: qdsp6: Add MI2S clock control Neil Armstrong
  3 siblings, 2 replies; 12+ messages in thread
From: Mohammad Rafi Shaik @ 2026-06-08  2:30 UTC (permalink / raw)
  To: Srinivas Kandagatla, Liam Girdwood, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela, Takashi Iwai
  Cc: Krzysztof Kozlowski, linux-arm-msm, linux-sound, devicetree,
	linux-kernel

The sc8280xp machine driver is currently written with a largely
SoC-centric view and assumes a uniform audio topology across all boards.
In practice, multiple products based on the same SoC use different board
designs and external audio components, which require board-specific
configuration to function correctly.

Several Qualcomm platforms like talos integrate third-party audio codecs
or use different external audio paths. These designs often require
additional configuration such as explicit MI2S MCLK settings for audio
to work.

This change enhances the sc8280xp machine driver to support board-specific
configuration such as allowing each board variant to provide its own DAPM
widgets and routes, reflecting the actual audio components and connectors
present and enabling MI2S MCLK programming for boards that use external
codecs requiring a stable master clock.

Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 sound/soc/qcom/sc8280xp.c | 213 ++++++++++++++++++++++++++++++++++----
 1 file changed, 195 insertions(+), 18 deletions(-)

diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c
index 7925aa3f6..1f3afc6d0 100644
--- a/sound/soc/qcom/sc8280xp.c
+++ b/sound/soc/qcom/sc8280xp.c
@@ -12,17 +12,77 @@
 #include <sound/jack.h>
 #include <linux/input-event-codes.h>
 #include "qdsp6/q6afe.h"
+#include "qdsp6/q6apm.h"
+#include "qdsp6/q6prm.h"
 #include "common.h"
 #include "sdw.h"
 
+#define I2S_MCLKFS 256
+
+#define I2S_MCLK_RATE(rate) \
+	((rate) * (I2S_MCLKFS))
+#define I2S_BIT_RATE(rate, channels, format) \
+	((rate) * (channels) * (format))
+
+static struct snd_soc_dapm_widget sc8280xp_dapm_widgets[] = {
+	SND_SOC_DAPM_HP("Headphone Jack", NULL),
+	SND_SOC_DAPM_MIC("Mic Jack", NULL),
+	SND_SOC_DAPM_SPK("DP0 Jack", NULL),
+	SND_SOC_DAPM_SPK("DP1 Jack", NULL),
+	SND_SOC_DAPM_SPK("DP2 Jack", NULL),
+	SND_SOC_DAPM_SPK("DP3 Jack", NULL),
+	SND_SOC_DAPM_SPK("DP4 Jack", NULL),
+	SND_SOC_DAPM_SPK("DP5 Jack", NULL),
+	SND_SOC_DAPM_SPK("DP6 Jack", NULL),
+	SND_SOC_DAPM_SPK("DP7 Jack", NULL),
+};
+
+struct snd_soc_common {
+	const char *driver_name;
+	const struct snd_soc_dapm_widget *dapm_widgets;
+	int num_dapm_widgets;
+	const struct snd_soc_dapm_route *dapm_routes;
+	int num_dapm_routes;
+	const struct snd_kcontrol_new *controls;
+	int num_controls;
+	unsigned int codec_dai_fmt;
+	bool codec_sysclk_set;
+	bool mi2s_mclk_enable;
+	bool mi2s_bclk_enable;
+};
+
 struct sc8280xp_snd_data {
 	bool stream_prepared[AFE_PORT_MAX];
 	struct snd_soc_card *card;
 	struct snd_soc_jack jack;
 	struct snd_soc_jack dp_jack[8];
+	struct snd_soc_common *snd_soc_common_priv;
 	bool jack_setup;
 };
 
+static inline int sc8280xp_get_mclk_freq(struct snd_pcm_hw_params *params)
+{
+	int rate = params_rate(params);
+
+	switch (rate) {
+	case SNDRV_PCM_RATE_11025:
+	case SNDRV_PCM_RATE_44100:
+	case SNDRV_PCM_RATE_88200:
+		return I2S_MCLK_RATE(44100);
+	default:
+		break;
+	}
+
+	return I2S_MCLK_RATE(rate);
+}
+
+static inline int sc8280xp_get_bclk_freq(struct snd_pcm_hw_params *params)
+{
+	return I2S_BIT_RATE(params_rate(params),
+			    params_channels(params),
+			    snd_pcm_format_width(params_format(params)));
+}
+
 static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd)
 {
 	struct sc8280xp_snd_data *data = snd_soc_card_get_drvdata(rtd->card);
@@ -32,10 +92,6 @@ static int sc8280xp_snd_init(struct snd_soc_pcm_runtime *rtd)
 	int dp_pcm_id = 0;
 
 	switch (cpu_dai->id) {
-	case PRIMARY_MI2S_RX...QUATERNARY_MI2S_TX:
-	case QUINARY_MI2S_RX...QUINARY_MI2S_TX:
-		snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_BP_FP);
-		break;
 	case WSA_CODEC_DMA_RX_0:
 	case WSA_CODEC_DMA_RX_1:
 		/*
@@ -96,6 +152,47 @@ static int sc8280xp_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
 	return 0;
 }
 
+static int sc8280xp_snd_hw_params(struct snd_pcm_substream *substream,
+				  struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
+	struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0);
+	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
+	struct sc8280xp_snd_data *data = snd_soc_card_get_drvdata(rtd->card);
+	int mclk_freq = sc8280xp_get_mclk_freq(params);
+	int bclk_freq = sc8280xp_get_bclk_freq(params);
+
+	switch (cpu_dai->id) {
+	case PRIMARY_MI2S_RX ... QUATERNARY_MI2S_TX:
+	case QUINARY_MI2S_RX ... QUINARY_MI2S_TX:
+		snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_BP_FP);
+
+		if (data->snd_soc_common_priv->codec_dai_fmt)
+			snd_soc_dai_set_fmt(codec_dai,
+					    data->snd_soc_common_priv->codec_dai_fmt);
+
+		if (data->snd_soc_common_priv->mi2s_mclk_enable)
+			snd_soc_dai_set_sysclk(cpu_dai,
+					       LPAIF_MI2S_MCLK, mclk_freq,
+					       SND_SOC_CLOCK_IN);
+
+		if (data->snd_soc_common_priv->mi2s_bclk_enable)
+			snd_soc_dai_set_sysclk(cpu_dai,
+					       LPAIF_MI2S_BCLK, bclk_freq,
+					       SND_SOC_CLOCK_IN);
+
+		if (data->snd_soc_common_priv->codec_sysclk_set)
+			snd_soc_dai_set_sysclk(cpu_dai,
+					       0, mclk_freq,
+					       SND_SOC_CLOCK_IN);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static int sc8280xp_snd_prepare(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
@@ -117,6 +214,7 @@ static int sc8280xp_snd_hw_free(struct snd_pcm_substream *substream)
 static const struct snd_soc_ops sc8280xp_be_ops = {
 	.startup = qcom_snd_sdw_startup,
 	.shutdown = qcom_snd_sdw_shutdown,
+	.hw_params = sc8280xp_snd_hw_params,
 	.hw_free = sc8280xp_snd_hw_free,
 	.prepare = sc8280xp_snd_prepare,
 };
@@ -145,37 +243,116 @@ static int sc8280xp_platform_probe(struct platform_device *pdev)
 	card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL);
 	if (!card)
 		return -ENOMEM;
-	card->owner = THIS_MODULE;
+
 	/* Allocate the private data */
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
+	data->snd_soc_common_priv = (struct snd_soc_common *)of_device_get_match_data(dev);
+	if (!data->snd_soc_common_priv)
+		return -ENOMEM;
+
+	card->owner = THIS_MODULE;
 	card->dev = dev;
 	dev_set_drvdata(dev, card);
 	snd_soc_card_set_drvdata(card, data);
+	card->dapm_widgets = data->snd_soc_common_priv->dapm_widgets;
+	card->num_dapm_widgets = data->snd_soc_common_priv->num_dapm_widgets;
+	card->dapm_routes = data->snd_soc_common_priv->dapm_routes;
+	card->num_dapm_routes = data->snd_soc_common_priv->num_dapm_routes;
+	card->controls = data->snd_soc_common_priv->controls;
+	card->num_controls = data->snd_soc_common_priv->num_controls;
+
 	ret = qcom_snd_parse_of(card);
 	if (ret)
 		return ret;
 
-	card->driver_name = of_device_get_match_data(dev);
+	card->driver_name = data->snd_soc_common_priv->driver_name;
 	sc8280xp_add_be_ops(card);
 	return devm_snd_soc_register_card(dev, card);
 }
 
+static struct snd_soc_common kaanapali_priv_data = {
+	.driver_name = "kaanapali",
+	.dapm_widgets = sc8280xp_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+};
+
+static struct snd_soc_common qcs9100_priv_data = {
+	.driver_name = "sa8775p",
+	.dapm_widgets = sc8280xp_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+};
+
+static struct snd_soc_common qcs615_priv_data = {
+	.driver_name = "qcs615",
+	.dapm_widgets = sc8280xp_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+	.mi2s_mclk_enable = true,
+};
+
+static struct snd_soc_common qcm6490_priv_data = {
+	.driver_name = "qcm6490",
+	.dapm_widgets = sc8280xp_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+};
+
+static struct snd_soc_common qcs6490_priv_data = {
+	.driver_name = "qcs6490",
+	.dapm_widgets = sc8280xp_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+};
+
+static struct snd_soc_common qcs8275_priv_data = {
+	.driver_name = "qcs8300",
+	.dapm_widgets = sc8280xp_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+};
+
+static struct snd_soc_common sc8280xp_priv_data = {
+	.driver_name = "sc8280xp",
+	.dapm_widgets = sc8280xp_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+};
+
+static struct snd_soc_common sm8450_priv_data = {
+	.driver_name = "sm8450",
+	.dapm_widgets = sc8280xp_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+};
+
+static struct snd_soc_common sm8550_priv_data = {
+	.driver_name = "sm8550",
+	.dapm_widgets = sc8280xp_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+};
+
+static struct snd_soc_common sm8650_priv_data = {
+	.driver_name = "sm8650",
+	.dapm_widgets = sc8280xp_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+};
+
+static struct snd_soc_common sm8750_priv_data = {
+	.driver_name = "sm8750",
+	.dapm_widgets = sc8280xp_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+};
+
 static const struct of_device_id snd_sc8280xp_dt_match[] = {
-	{.compatible = "qcom,kaanapali-sndcard", "kaanapali"},
-	{.compatible = "qcom,qcm6490-idp-sndcard", "qcm6490"},
-	{.compatible = "qcom,qcs615-sndcard", "qcs615"},
-	{.compatible = "qcom,qcs6490-rb3gen2-sndcard", "qcs6490"},
-	{.compatible = "qcom,qcs8275-sndcard", "qcs8300"},
-	{.compatible = "qcom,qcs9075-sndcard", "sa8775p"},
-	{.compatible = "qcom,qcs9100-sndcard", "sa8775p"},
-	{.compatible = "qcom,sc8280xp-sndcard", "sc8280xp"},
-	{.compatible = "qcom,sm8450-sndcard", "sm8450"},
-	{.compatible = "qcom,sm8550-sndcard", "sm8550"},
-	{.compatible = "qcom,sm8650-sndcard", "sm8650"},
-	{.compatible = "qcom,sm8750-sndcard", "sm8750"},
+	{.compatible = "qcom,kaanapali-sndcard", .data = &kaanapali_priv_data},
+	{.compatible = "qcom,qcm6490-idp-sndcard", .data = &qcm6490_priv_data},
+	{.compatible = "qcom,qcs615-sndcard", .data = &qcs615_priv_data},
+	{.compatible = "qcom,qcs6490-rb3gen2-sndcard", .data = &qcs6490_priv_data},
+	{.compatible = "qcom,qcs8275-sndcard", .data = &qcs8275_priv_data},
+	{.compatible = "qcom,qcs9075-sndcard", .data = &qcs9100_priv_data},
+	{.compatible = "qcom,qcs9100-sndcard", .data = &qcs9100_priv_data},
+	{.compatible = "qcom,sc8280xp-sndcard", .data = &sc8280xp_priv_data},
+	{.compatible = "qcom,sm8450-sndcard", .data = &sm8450_priv_data},
+	{.compatible = "qcom,sm8550-sndcard", .data = &sm8550_priv_data},
+	{.compatible = "qcom,sm8650-sndcard", .data = &sm8650_priv_data},
+	{.compatible = "qcom,sm8750-sndcard", .data = &sm8750_priv_data},
 	{}
 };
 
-- 
2.34.1


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

* Re: [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control
  2026-06-08  2:30 ` [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control Mohammad Rafi Shaik
@ 2026-06-08  2:38   ` sashiko-bot
  2026-06-08  2:49   ` Val Packett
  2026-06-08 12:01   ` Mark Brown
  2 siblings, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-06-08  2:38 UTC (permalink / raw)
  To: Mohammad Rafi Shaik; +Cc: devicetree, robh, conor+dt

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Uninitialized pointers `sysclk` and `enabled` in `q6i2s_set_sysclk` can cause arbitrary memory writes and kernel crashes.
- [High] Clock references acquired via `of_clk_get_by_name()` are never released with `clk_put()`, causing permanent resource leaks.
- [High] Unbalanced clock enable/disable logic between `.set_sysclk` and `.shutdown` causes clock enable count leaks and hardware state desync.
- [Medium] Missing `of_node_put()` on early return inside OF node iterator loop causes device node reference leak.
--

commit bb83de598c284e0e9f9b088d18eb243bc21c7bbd
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control
    
Add support for MI2S clock control within q6apm-lpass DAIs, including
handling of MCLK, BCLK, and ECLK via the DAI .set_sysclk callback.
Each MI2S port now retrieves its clock handles from the device tree,
allowing per-port clock configuration and proper enable/disable during
startup and shutdown.

> --- a/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-lpass-dais.c

[ ... ]

> +static void q6i2s_lpass_dai_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(dai->dev);
> +
> +	if (dai_data->priv[dai->id].mclk_enabled) {
> +		clk_disable_unprepare(dai_data->priv[dai->id].mclk);
> +		dai_data->priv[dai->id].mclk_enabled = false;
> +	}
> +
> +	if (dai_data->priv[dai->id].bclk_enabled) {
> +		clk_disable_unprepare(dai_data->priv[dai->id].bclk);
> +		dai_data->priv[dai->id].bclk_enabled = false;
> +	}
> +
> +	if (dai_data->priv[dai->id].eclk_enabled) {
> +		clk_disable_unprepare(dai_data->priv[dai->id].eclk);
> +		dai_data->priv[dai->id].eclk_enabled = false;
> +	}
> +	q6apm_lpass_dai_shutdown(substream, dai);
> +}
> +
> +static int q6i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir)
> +{
> +	struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(dai->dev);
> +	struct clk *sysclk;
> +	bool *enabled;
> +	int ret = 0;
> +
> +	switch (clk_id) {
> +	case LPAIF_MI2S_MCLK:
> +		sysclk = dai_data->priv[dai->id].mclk;
> +		enabled = &dai_data->priv[dai->id].mclk_enabled;
> +		break;
> +	case LPAIF_MI2S_BCLK:
> +		sysclk = dai_data->priv[dai->id].bclk;
> +		enabled = &dai_data->priv[dai->id].bclk_enabled;
> +		break;
> +	case LPAIF_MI2S_ECLK:
> +		sysclk = dai_data->priv[dai->id].eclk;
> +		enabled = &dai_data->priv[dai->id].eclk_enabled;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (sysclk) {

[Severity: Critical]
Are the pointers sysclk and enabled initialized when clk_id falls into the
default case?

If an unsupported clk_id is provided, the switch statement completes without
assigning values to sysclk and enabled. The local stack memory could
evaluate as non-NULL, causing clk_set_rate() to be called with a garbage
pointer.

Additionally, does this lead to memory corruption when it executes
*enabled = true; writing a boolean to an arbitrary, uninitialized address?

> +		clk_set_rate(sysclk, freq);
> +		ret = clk_prepare_enable(sysclk);

[Severity: High]
Can this lead to unbalanced clock enable/disable counts and broken concurrent
streams?

In q6i2s_set_sysclk(), clk_prepare_enable() is called unconditionally,
meaning multiple invocations will increment the enable count multiple times.

However, q6i2s_lpass_dai_shutdown() runs per-substream. If both playback
and capture are active on the same DAI, shutting down the first stream
will disable the shared clock and set the enabled boolean to false. Does
this prematurely kill the clock for the remaining active stream?

Similarly, if the enable count was incremented multiple times, but shutdown
only disables it once before clearing the flag, does this leak the clock
enable refcount?

> +		if (ret) {
> +			dev_err(dai->dev, "Error, Unable to prepare (%d) sysclk\n", clk_id);
> +			return ret;
> +		}
> +
> +		*enabled = true;
> +	}
> +
> +	return ret;
> +}

[ ... ]

> +static int of_q6apm_parse_dai_data(struct device *dev,
> +				   struct q6apm_lpass_dai_data *data)
> +{
> +	struct device_node *node;
> +	int ret;
> +
> +	for_each_child_of_node(dev->of_node, node) {
> +		struct q6apm_dai_priv_data *priv;
> +		int id;
> +
> +		ret = of_property_read_u32(node, "reg", &id);
> +		if (ret || id < 0 || id >= APM_PORT_MAX) {
> +			dev_err(dev, "valid dai id not found:%d\n", ret);
> +			continue;
> +		}
> +
> +		switch (id) {
> +		/* MI2S specific properties */
> +		case PRIMARY_MI2S_RX ... QUATERNARY_MI2S_TX:
> +		case QUINARY_MI2S_RX ... QUINARY_MI2S_TX:
> +			priv = &data->priv[id];
> +			priv->mclk = of_clk_get_by_name(node, "mclk");

[Severity: High]
Are these clock references ever released?

The clocks are acquired here via of_clk_get_by_name(), which increments the
underlying refcount and requires a matching clk_put().

If a later clock lookup within this loop fails with -EPROBE_DEFER, the
function returns early without freeing previously acquired clocks. Furthermore,
because these are not managed via devm_ and there is no teardown logic or
.remove callback provided, does this cause a permanent resource leak when
the device unbinds?

> +			if (IS_ERR(priv->mclk)) {
> +				if (PTR_ERR(priv->mclk) == -EPROBE_DEFER)
> +					return dev_err_probe(dev, PTR_ERR(priv->mclk),
> +							     "unable to get mi2s mclk\n");

[Severity: Medium]
Does returning here leak the device node reference?

The for_each_child_of_node() macro automatically manages reference counts
during iteration, but an early return requires a manual of_node_put(node)
before exiting. If this loop exits early via return, will the device node
reference be leaked?

> +				priv->mclk = NULL;
> +			}
> +
> +			priv->bclk = of_clk_get_by_name(node, "bclk");
> +			if (IS_ERR(priv->bclk)) {
> +				if (PTR_ERR(priv->bclk) == -EPROBE_DEFER)
> +					return dev_err_probe(dev, PTR_ERR(priv->bclk),
> +							     "unable to get mi2s bclk\n");
> +				priv->bclk = NULL;
> +			}
> +
> +			priv->eclk = of_clk_get_by_name(node, "eclk");
> +			if (IS_ERR(priv->eclk)) {
> +				if (PTR_ERR(priv->eclk) == -EPROBE_DEFER)
> +					return dev_err_probe(dev, PTR_ERR(priv->eclk),
> +							     "unable to get mi2s eclk\n");
> +				priv->eclk = NULL;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608023011.942228-1-mohammad.rafi.shaik@oss.qualcomm.com?part=2

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

* Re: [PATCH v2 3/3] ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config
  2026-06-08  2:30 ` [PATCH v2 3/3] ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config Mohammad Rafi Shaik
@ 2026-06-08  2:41   ` sashiko-bot
  2026-06-08 12:08   ` Mark Brown
  1 sibling, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-06-08  2:41 UTC (permalink / raw)
  To: Mohammad Rafi Shaik; +Cc: robh, conor+dt, devicetree

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `sc8280xp_get_mclk_freq()` incorrectly compares integer sample rate values in Hz with ALSA `SNDRV_PCM_RATE_*` bitmask constants.
- [Medium] The `codec_sysclk_set` flag incorrectly applies the sysclk configuration to the CPU DAI instead of the CODEC DAI.
- [Low] Returning `-ENOMEM` when `of_device_get_match_data()` returns NULL.
--

commit 66516d9230e47a6cd893799b841db83183cc7051
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config

This commit enhances the sc8280xp machine driver to support board-specific
configurations. It allows each board variant to provide its own DAPM widgets
and routes, and enables MI2S MCLK programming for external codecs that
require a stable master clock.

> diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c
> index 7925aa3f63ba0..1f3afc6d015cf 100644
> --- a/sound/soc/qcom/sc8280xp.c
> +++ b/sound/soc/qcom/sc8280xp.c
[ ... ]
> @@ -12,17 +12,77 @@
[ ... ]
> +static inline int sc8280xp_get_mclk_freq(struct snd_pcm_hw_params *params)
> +{
> +	int rate = params_rate(params);
> +
> +	switch (rate) {
> +	case SNDRV_PCM_RATE_11025:
> +	case SNDRV_PCM_RATE_44100:
> +	case SNDRV_PCM_RATE_88200:

[Severity: High]
Does this switch statement compare integer frequencies in Hz against ALSA rate
bitmask macros?

The function params_rate(params) returns the actual sample rate in Hz
(e.g., 44100), but macros like SNDRV_PCM_RATE_44100 evaluate to bitmasks
(e.g., 1U << 6, or 64). Because an integer frequency will never equal its
bitmask value, it seems this switch will always fall through to the default
case.

This will accidentally yield the correct multiplier for 44100 Hz streams, but
will incorrectly calculate the MCLK rate for 11025 Hz and 88200 Hz streams.

> +		return I2S_MCLK_RATE(44100);
> +	default:
> +		break;
> +	}
> +
> +	return I2S_MCLK_RATE(rate);
> +}
[ ... ]
> @@ -96,6 +152,47 @@ static int sc8280xp_be_hw_params_fixup(struct snd_soc_pcm_runtime *rtd,
[ ... ]
> +static int sc8280xp_snd_hw_params(struct snd_pcm_substream *substream,
> +				  struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0);
> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
[ ... ]
> +		if (data->snd_soc_common_priv->codec_sysclk_set)
> +			snd_soc_dai_set_sysclk(cpu_dai,
> +					       0, mclk_freq,
> +					       SND_SOC_CLOCK_IN);

[Severity: Medium]
Should this call configure the system clock for the codec_dai instead of the
cpu_dai?

When the codec_sysclk_set flag is true, it appears the intent is to configure
the external codec's clock. However, applying it to the cpu_dai pointer might
leave the external codec unconfigured while inadvertently assigning an
unintended clock configuration to the CPU DAI.

> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> @@ -145,37 +243,116 @@ static int sc8280xp_platform_probe(struct platform_device *pdev)
[ ... ]
> +	data->snd_soc_common_priv = (struct snd_soc_common *)of_device_get_match_data(dev);
> +	if (!data->snd_soc_common_priv)
> +		return -ENOMEM;

[Severity: Low]
Would it be better to return -EINVAL or -ENODEV here instead of -ENOMEM?

A missing device tree match or absent platform data does not seem like a memory
allocation failure, and returning -ENOMEM might create confusing kernel logs
during debugging.

> +
> +	card->owner = THIS_MODULE;
>  	card->dev = dev;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608023011.942228-1-mohammad.rafi.shaik@oss.qualcomm.com?part=3

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

* Re: [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control
  2026-06-08  2:30 ` [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control Mohammad Rafi Shaik
  2026-06-08  2:38   ` sashiko-bot
@ 2026-06-08  2:49   ` Val Packett
  2026-06-08 12:01   ` Mark Brown
  2 siblings, 0 replies; 12+ messages in thread
From: Val Packett @ 2026-06-08  2:49 UTC (permalink / raw)
  To: Mohammad Rafi Shaik, Srinivas Kandagatla, Liam Girdwood,
	Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Jaroslav Kysela, Takashi Iwai
  Cc: Krzysztof Kozlowski, linux-arm-msm, linux-sound, devicetree,
	linux-kernel, Srinivas Kandagatla


On 6/7/26 11:30 PM, Mohammad Rafi Shaik wrote:
> Add support for MI2S clock control within q6apm-lpass DAIs, including
> handling of MCLK, BCLK, and ECLK via the DAI .set_sysclk callback.
> Each MI2S port now retrieves its clock handles from the device tree,
> allowing per-port clock configuration and proper enable/disable during
> startup and shutdown.
> [..]
> @@ -297,6 +373,59 @@ static const struct snd_soc_component_driver q6apm_lpass_dai_component = {
>   	.remove_order   = SND_SOC_COMP_ORDER_FIRST,
>   };
>   
> +static int of_q6apm_parse_dai_data(struct device *dev,
> +				   struct q6apm_lpass_dai_data *data)
> +{
> +	struct device_node *node;
> +	int ret;
> +
> +	for_each_child_of_node(dev->of_node, node) {
> +		struct q6apm_dai_priv_data *priv;
> +		int id;
> +
> +		ret = of_property_read_u32(node, "reg", &id);
> +		if (ret || id < 0 || id >= APM_PORT_MAX) {
> +			dev_err(dev, "valid dai id not found:%d\n", ret);
> +			continue;
> +		}
> +
> +		switch (id) {
> +		/* MI2S specific properties */
> +		case PRIMARY_MI2S_RX ... QUATERNARY_MI2S_TX:
> +		case QUINARY_MI2S_RX ... QUINARY_MI2S_TX:

SENARY is also a thing these days btw..


~val


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

* Re: [PATCH v2 1/3] ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode
  2026-06-08  2:30 ` [PATCH v2 1/3] ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode Mohammad Rafi Shaik
@ 2026-06-08  4:44   ` sashiko-bot
  2026-06-08  9:59   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 12+ messages in thread
From: sashiko-bot @ 2026-06-08  4:44 UTC (permalink / raw)
  To: Mohammad Rafi Shaik; +Cc: conor+dt, robh, devicetree

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The schema uses an invalid regex `^dai@[0-9]+$` that rejects valid hex unit addresses containing `a-f`, and the example incorrectly uses the decimal unit address `dai@16` instead of the expected hexadecimal `dai@10`.
- [Low] The `clock-names` and `clocks` properties define a strictly ordered array sequence, preventing developers from defining subsets of clocks (e.g., omitting `bclk` while supplying `eclk`).
--

commit 3b274d792c2aeffee37c0e3a9b1930eed072c203
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode

This commit extends the qcom,q6apm-lpass-dais device tree binding to
explicitly describe DAI child nodes. It adds address and size cells,
along with a dai pattern property to define interface IDs and clocks.

> diff --git a/Documentation/devicetree/bindings/sound/qcom,q6apm-lpass-dais.yaml b/Documentation/devicetree/bindings/sound/qcom,q6apm-lpass-dais.yaml
> index 2fb95544db8b5..4c4c73778e238 100644
> --- a/Documentation/devicetree/bindings/sound/qcom,q6apm-lpass-dais.yaml
> +++ b/Documentation/devicetree/bindings/sound/qcom,q6apm-lpass-dais.yaml
[ ... ]
> +# Digital Audio Interfaces
> +patternProperties:
> +  '^dai@[0-9]+$':

[Severity: Low]
Does this regex pattern incorrectly block valid hexadecimal device tree unit
addresses?

Device tree unit addresses are typically expected to be lowercase
hexadecimal. This pattern rejects valid hex addresses containing alphabetical
characters like a-f.

> +    type: object
> +    description:
> +      Q6DSP Digital Audio Interfaces.
[ ... ]
> +      clock-names:
> +        minItems: 1
> +        items:
> +          - const: mclk
> +          - const: bclk
> +          - const: eclk

[Severity: Low]
By using a strict array sequence here, does this prevent developers from
defining flexible clock subsets?

The driver retrieves clocks individually by name and appears to handle
omitted clocks gracefully. If a hardware design omits bclk but uses eclk,
this strict sequence constraint would cause schema validation to fail.

[ ... ]
>  examples:
>    - |
> +    #include <dt-bindings/sound/qcom,q6afe.h>
> +
>      dais {
>          compatible = "qcom,q6apm-lpass-dais";
>          #sound-dai-cells = <1>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        dai@16 {
> +           reg = <PRIMARY_MI2S_RX>;

[Severity: Low]
Should this example use a hexadecimal unit address instead of decimal?

PRIMARY_MI2S_RX evaluates to decimal 16, which means the unit address
should be formatted as dai@10 to match the reg property and avoid
unit_address_vs_reg warnings during dtc checks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608023011.942228-1-mohammad.rafi.shaik@oss.qualcomm.com?part=1

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

* Re: [PATCH v2 0/3] ASoC: qcom: qdsp6: Add MI2S clock control
  2026-06-08  2:30 [PATCH v2 0/3] ASoC: qcom: qdsp6: Add MI2S clock control Mohammad Rafi Shaik
                   ` (2 preceding siblings ...)
  2026-06-08  2:30 ` [PATCH v2 3/3] ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config Mohammad Rafi Shaik
@ 2026-06-08  8:23 ` Neil Armstrong
  3 siblings, 0 replies; 12+ messages in thread
From: Neil Armstrong @ 2026-06-08  8:23 UTC (permalink / raw)
  To: Mohammad Rafi Shaik, Srinivas Kandagatla, Liam Girdwood,
	Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Jaroslav Kysela, Takashi Iwai
  Cc: Krzysztof Kozlowski, linux-arm-msm, linux-sound, devicetree,
	linux-kernel

On 6/8/26 04:30, Mohammad Rafi Shaik wrote:
> Add support for MI2S clock control within q6apm-lpass DAIs, including
> handling of MCLK, BCLK, and ECLK via the DAI .set_sysclk callback.
> Each MI2S port now retrieves its clock handles from the device tree,
> allowing per-port clock configuration and proper enable/disable during
> startup and shutdown.
> 
> On platforms such as Monaco and Lemans, third-party codecs are
> hardware-wired to the SoC and do not always have an in-tree codec
> driver to manage their clocks. For these designs, clock line
> enablement must be driven from the platform side, and this
> series provides the necessary support for that.
> 
> On QAIF-based platforms such as Shikra and Hawi, responsibility
> for voting I2S MCLK and bit-clock has moved from the DSP to the
> kernel. This series introduces the required device tree binding
> support to represent and vote for these clocks from the kernel.
> 
> Enhances the sc8280xp machine driver to set the boards spacific
> configurations.
> 
> This series depends on:
> - https://lore.kernel.org/all/20260607-rubikpi-next-20260605-v1-3-7f334e16fea6@thundersoft.com/
> 
> ---
> Changes in v3:
> - Added a detailed commit description to clearly explain the need for this change.
> - Improved the machine driver based on Neil’s feedback.
> - Link to v1: https://lore.kernel.org/all/20260309111300.2484262-1-mohammad.rafi.shaik@oss.qualcomm.com/
> ---
> Mohammad Rafi Shaik (3):
>    ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode
>    ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control
>    ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for
>      board-specific config
> 
>   .../bindings/sound/qcom,q6apm-lpass-dais.yaml |  57 +++++
>   sound/soc/qcom/qdsp6/q6apm-lpass-dais.c       | 137 ++++++++++-
>   sound/soc/qcom/qdsp6/q6prm.h                  |   4 +
>   sound/soc/qcom/sc8280xp.c                     | 213 ++++++++++++++++--
>   4 files changed, 391 insertions(+), 20 deletions(-)
> 
> 
> base-commit: 6e845bcb78c95af935094040bd4edc3c2b6dd784
> prerequisite-patch-id: 2f1bd3efac328030dd8efe28fb95f84603868043

Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-HDK

With:
```
diff --git a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
index eabc828c05b4..69327c4857e1 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
@@ -218,6 +218,22 @@ platform {
  				sound-dai = <&q6apm>;
  			};
  		};
+
+		pri-mi2s-dai-link {
+			link-name = "HDMI Playback";
+
+			cpu {
+				sound-dai = <&q6apmbedai PRIMARY_MI2S_RX>;
+			};
+
+			codec {
+				sound-dai = <&lt9611_codec 0>;
+			};
+
+			platform {
+				sound-dai = <&q6apm>;
+			};
+		};
  	};

  	vph_pwr: regulator-vph-pwr {
@@ -853,6 +869,7 @@ &i2c6 {
  	lt9611_codec: hdmi-bridge@2b {
  		compatible = "lontium,lt9611uxc";
  		reg = <0x2b>;
+		#sound-dai-cells = <1>;

  		interrupts-extended = <&tlmm 85 IRQ_TYPE_EDGE_FALLING>;

@@ -861,7 +878,10 @@ lt9611_codec: hdmi-bridge@2b {
  		vdd-supply = <&lt9611_1v2>;
  		vcc-supply = <&lt9611_3v3>;

-		pinctrl-0 = <&lt9611_irq_pin>, <&lt9611_rst_pin>;
+		pinctrl-0 = <&lt9611_irq_pin>,
+			    <&lt9611_rst_pin>,
+			    <&i2s0_default_state>,
+			    <&audio_mclk0_default_state>;
  		pinctrl-names = "default";

  		ports {
@@ -1072,6 +1092,19 @@ &pon_resin {
  	status = "okay";
  };

+&q6apmbedai {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	dai@16 {
+		reg = <PRIMARY_MI2S_RX>;
+		clocks = <&q6prmcc LPASS_CLK_ID_MCLK_1 LPASS_CLK_ATTRIBUTE_COUPLE_NO>,
+			 <&q6prmcc LPASS_CLK_ID_PRI_MI2S_IBIT LPASS_CLK_ATTRIBUTE_COUPLE_NO>;
+		clock-names = "mclk",
+			      "bclk";
+	};
+};
+
  &qup_i2c3_data_clk {
  	/* Use internal I2C pull-up */
  	bias-pull-up = <2200>;
diff --git a/sound/soc/qcom/sc8280xp.c b/sound/soc/qcom/sc8280xp.c
index 1f3afc6d015c..7b34ff533e0d 100644
--- a/sound/soc/qcom/sc8280xp.c
+++ b/sound/soc/qcom/sc8280xp.c
@@ -320,18 +320,36 @@ static struct snd_soc_common sm8450_priv_data = {
  	.driver_name = "sm8450",
  	.dapm_widgets = sc8280xp_dapm_widgets,
  	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+	/* I2S Connected to HDMI */
+	.mi2s_mclk_enable = true,
+	.mi2s_bclk_enable = true,
+	.codec_dai_fmt = SND_SOC_DAIFMT_BC_FC |
+			 SND_SOC_DAIFMT_NB_NF |
+			 SND_SOC_DAIFMT_I2S,
  };

  static struct snd_soc_common sm8550_priv_data = {
  	.driver_name = "sm8550",
  	.dapm_widgets = sc8280xp_dapm_widgets,
  	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+	/* I2S Connected to HDMI */
+	.mi2s_mclk_enable = true,
+	.mi2s_bclk_enable = true,
+	.codec_dai_fmt = SND_SOC_DAIFMT_BC_FC |
+			 SND_SOC_DAIFMT_NB_NF |
+			 SND_SOC_DAIFMT_I2S,
  };

  static struct snd_soc_common sm8650_priv_data = {
  	.driver_name = "sm8650",
  	.dapm_widgets = sc8280xp_dapm_widgets,
  	.num_dapm_widgets = ARRAY_SIZE(sc8280xp_dapm_widgets),
+	/* I2S Connected to HDMI */
+	.mi2s_mclk_enable = true,
+	.mi2s_bclk_enable = true,
+	.codec_dai_fmt = SND_SOC_DAIFMT_BC_FC |
+			 SND_SOC_DAIFMT_NB_NF |
+			 SND_SOC_DAIFMT_I2S,
  };

  static struct snd_soc_common sm8750_priv_data = {
```

Thanks,
Neil

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

* Re: [PATCH v2 1/3] ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode
  2026-06-08  2:30 ` [PATCH v2 1/3] ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode Mohammad Rafi Shaik
  2026-06-08  4:44   ` sashiko-bot
@ 2026-06-08  9:59   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 12+ messages in thread
From: Krzysztof Kozlowski @ 2026-06-08  9:59 UTC (permalink / raw)
  To: Mohammad Rafi Shaik
  Cc: Srinivas Kandagatla, Liam Girdwood, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela, Takashi Iwai,
	linux-arm-msm, linux-sound, devicetree, linux-kernel,
	Srinivas Kandagatla

On Mon, Jun 08, 2026 at 08:00:09AM +0530, Mohammad Rafi Shaik wrote:
> +      clocks:
> +        minItems: 1
> +        items:
> +          - description: MI2S master clock
> +          - description: MI2S bit clock
> +          - description: MI2S external bit clock
> +
> +      clock-names:
> +        minItems: 1
> +        items:
> +          - const: mclk
> +          - const: bclk
> +          - const: eclk
> +
> +    dependencies:
> +      clocks: [ clock-names ]

This you can keep

> +      clock-names: [ clocks ]

But this is not needed, core handles it.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof


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

* Re: [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control
  2026-06-08  2:30 ` [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control Mohammad Rafi Shaik
  2026-06-08  2:38   ` sashiko-bot
  2026-06-08  2:49   ` Val Packett
@ 2026-06-08 12:01   ` Mark Brown
  2 siblings, 0 replies; 12+ messages in thread
From: Mark Brown @ 2026-06-08 12:01 UTC (permalink / raw)
  To: Mohammad Rafi Shaik
  Cc: Srinivas Kandagatla, Liam Girdwood, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela, Takashi Iwai,
	Krzysztof Kozlowski, linux-arm-msm, linux-sound, devicetree,
	linux-kernel, Srinivas Kandagatla

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

On Mon, Jun 08, 2026 at 08:00:10AM +0530, Mohammad Rafi Shaik wrote:
> Add support for MI2S clock control within q6apm-lpass DAIs, including
> handling of MCLK, BCLK, and ECLK via the DAI .set_sysclk callback.

> +static int q6i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id, unsigned int freq, int dir)
> +{
> +	struct q6apm_lpass_dai_data *dai_data = dev_get_drvdata(dai->dev);
> +	struct clk *sysclk;
> +	bool *enabled;

Neither sysclk nor enabled are initialised by default...

> +	switch (clk_id) {

> +	case LPAIF_MI2S_ECLK:
> +		sysclk = dai_data->priv[dai->id].eclk;
> +		enabled = &dai_data->priv[dai->id].eclk_enabled;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (sysclk) {

...but we just fall through the switch statement if the clock ID is
invalid and use the values.

> +	for_each_child_of_node(dev->of_node, node) {

> +			if (IS_ERR(priv->mclk)) {
> +				if (PTR_ERR(priv->mclk) == -EPROBE_DEFER)
> +					return dev_err_probe(dev, PTR_ERR(priv->mclk),
> +							     "unable to get mi2s mclk\n");
> +				priv->mclk = NULL;
> +			}

Do we need to drop any OF references on early return?

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

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

* Re: [PATCH v2 3/3] ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config
  2026-06-08  2:30 ` [PATCH v2 3/3] ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config Mohammad Rafi Shaik
  2026-06-08  2:41   ` sashiko-bot
@ 2026-06-08 12:08   ` Mark Brown
  1 sibling, 0 replies; 12+ messages in thread
From: Mark Brown @ 2026-06-08 12:08 UTC (permalink / raw)
  To: Mohammad Rafi Shaik
  Cc: Srinivas Kandagatla, Liam Girdwood, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Jaroslav Kysela, Takashi Iwai,
	Krzysztof Kozlowski, linux-arm-msm, linux-sound, devicetree,
	linux-kernel

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

On Mon, Jun 08, 2026 at 08:00:11AM +0530, Mohammad Rafi Shaik wrote:
> The sc8280xp machine driver is currently written with a largely
> SoC-centric view and assumes a uniform audio topology across all boards.

> +static inline int sc8280xp_get_mclk_freq(struct snd_pcm_hw_params *params)
> +{
> +	int rate = params_rate(params);
> +
> +	switch (rate) {
> +	case SNDRV_PCM_RATE_11025:
> +	case SNDRV_PCM_RATE_44100:
> +	case SNDRV_PCM_RATE_88200:

rate is in Hz but these are bitmasks.

> +		return I2S_MCLK_RATE(44100);
> +	default:
> +		break;
> +	}

The function only works since it ignores invalid values.

> +static int sc8280xp_snd_hw_params(struct snd_pcm_substream *substream,
> +				  struct snd_pcm_hw_params *params)
> +{

> +
> +		if (data->snd_soc_common_priv->codec_dai_fmt)
> +			snd_soc_dai_set_fmt(codec_dai,
> +					    data->snd_soc_common_priv->codec_dai_fmt);

Should we error check the functions we call here?

> +		if (data->snd_soc_common_priv->mi2s_mclk_enable)
> +			snd_soc_dai_set_sysclk(cpu_dai,
> +					       LPAIF_MI2S_MCLK, mclk_freq,
> +					       SND_SOC_CLOCK_IN);
> +
> +		if (data->snd_soc_common_priv->mi2s_bclk_enable)
> +			snd_soc_dai_set_sysclk(cpu_dai,
> +					       LPAIF_MI2S_BCLK, bclk_freq,
> +					       SND_SOC_CLOCK_IN);

Is SND_SOC_CLK_IN right here?  The flag sounds like it's enabling the
clock on the DAI but this is configuring the DAI to consume a clock.

> +		if (data->snd_soc_common_priv->codec_sysclk_set)
> +			snd_soc_dai_set_sysclk(cpu_dai,
> +					       0, mclk_freq,
> +					       SND_SOC_CLOCK_IN);

This is configuring the CPU not CODEC.

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

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

end of thread, other threads:[~2026-06-08 12:09 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-08  2:30 [PATCH v2 0/3] ASoC: qcom: qdsp6: Add MI2S clock control Mohammad Rafi Shaik
2026-06-08  2:30 ` [PATCH v2 1/3] ASoC: dt-bindings: qcom,q6apm-lpass-dais: Document DAI subnode Mohammad Rafi Shaik
2026-06-08  4:44   ` sashiko-bot
2026-06-08  9:59   ` Krzysztof Kozlowski
2026-06-08  2:30 ` [PATCH v2 2/3] ASoC: qcom: q6apm-lpass-dais: Add MI2S clock control Mohammad Rafi Shaik
2026-06-08  2:38   ` sashiko-bot
2026-06-08  2:49   ` Val Packett
2026-06-08 12:01   ` Mark Brown
2026-06-08  2:30 ` [PATCH v2 3/3] ASoC: qcom: sc8280xp: ASoC: qcom: sc8280xp: enhance machine driver for board-specific config Mohammad Rafi Shaik
2026-06-08  2:41   ` sashiko-bot
2026-06-08 12:08   ` Mark Brown
2026-06-08  8:23 ` [PATCH v2 0/3] ASoC: qcom: qdsp6: Add MI2S clock control Neil Armstrong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.