[PATCH 0/6] ASoC: samsung: fsd: audio support for FSD SoC

[PATCH 0/6] ASoC: samsung: fsd: audio support for FSD SoC

[Padmanabhan Rajanbabu]

The intention of this patch series is to enable audio support on FSD SoC.

The following features are added in samsung I2S interface:
1. Add TDM support on samsung I2S interface
2. Allow sound card to directly configure prescaler divider instead of
calculating it from frame clock

Also the sound card support for FSD SoC is added, which utilizes samsung
I2S interface as CPU DAI.

This patch is dependent on fsd-pinctrl fixes patch series [1]
[1]: https://lkml.org/lkml/2022/10/13/257

Padmanabhan Rajanbabu (6):
  ASoC: samsung: i2s: TDM Support for CPU DAI driver
  ASoC: samsung: i2s: configure PSR from sound card
  dt-bindings: sound: Add sound card bindings for Tesla FSD
  ASoC: samsung: fsd: Add FSD soundcard driver
  arm64: dts: fsd: Add I2S DAI node for Tesla FSD
  arm64: dts: fsd: Add sound card node for Tesla FSD

 .../bindings/sound/tesla,fsd-card.yaml        | 158 ++++++++
 arch/arm64/boot/dts/tesla/fsd-evb.dts         |  57 +++
 arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi    |  14 +
 arch/arm64/boot/dts/tesla/fsd.dtsi            |  41 ++
 sound/soc/samsung/Kconfig                     |  12 +
 sound/soc/samsung/Makefile                    |   2 +
 sound/soc/samsung/fsd-card.c                  | 349 ++++++++++++++++++
 sound/soc/samsung/i2s-regs.h                  |  17 +
 sound/soc/samsung/i2s.c                       | 120 +++++-
 sound/soc/samsung/i2s.h                       |   1 +
 10 files changed, 766 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
 create mode 100644 sound/soc/samsung/fsd-card.c

-- 
2.17.1

[PATCH 1/6] ASoC: samsung: i2s: TDM Support for CPU DAI driver

[Padmanabhan Rajanbabu]

Add support to configure samsung I2S CPU DAI in TDM mode.

Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
---
 sound/soc/samsung/i2s-regs.h | 15 +++++++
 sound/soc/samsung/i2s.c      | 84 +++++++++++++++++++++++++++++++++++-
 2 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h
index b4b5d6053503..cb2be4a3b70b 100644
--- a/sound/soc/samsung/i2s-regs.h
+++ b/sound/soc/samsung/i2s-regs.h
@@ -154,4 +154,19 @@
 #define I2SSIZE_TRNMSK		(0xffff)
 #define I2SSIZE_SHIFT		(16)
 
+#define TDM_LRCLK_WIDTH_SHIFT	12
+#define TDM_LRCLK_WIDTH_MASK	0xFF
+#define TDM_RX_SLOTS_SHIFT	8
+#define TDM_RX_SLOTS_MASK	7
+#define TDM_TX_SLOTS_SHIFT	4
+#define TDM_TX_SLOTS_MASK	7
+#define TDM_MODE_MASK		1
+#define TDM_MODE_SHIFT		1
+#define TDM_MODE_DSPA		0
+#define TDM_MODE_DSPB		1
+#define TDM_ENABLE		(1 << 0)
+
+/* stereo default */
+#define TDM_DEFAULT_SLOT_NUM_DIVIDER	2
+
 #endif /* __SND_SOC_SAMSUNG_I2S_REGS_H */
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 9505200f3d11..fb806b0af6ab 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -117,6 +117,8 @@ struct samsung_i2s_priv {
 	struct clk *clk_table[3];
 	struct clk_onecell_data clk_data;
 
+	int tdm_slots;
+
 	/* Spinlock protecting member fields below */
 	spinlock_t lock;
 
@@ -625,15 +627,19 @@ static int i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	struct samsung_i2s_priv *priv = snd_soc_dai_get_drvdata(dai);
 	struct i2s_dai *i2s = to_info(dai);
 	int lrp_shift, sdf_shift, sdf_mask, lrp_rlow, mod_slave;
+	int tdm_mod_mask, tdm_mod_shift;
+	u32 tdm = 0, tdm_tmp = 0;
 	u32 mod, tmp = 0;
 	unsigned long flags;
 
 	lrp_shift = priv->variant_regs->lrp_off;
 	sdf_shift = priv->variant_regs->sdf_off;
+	tdm_mod_shift = TDM_MODE_SHIFT;
 	mod_slave = 1 << priv->variant_regs->mss_off;
 
 	sdf_mask = MOD_SDF_MASK << sdf_shift;
 	lrp_rlow = MOD_LR_RLOW << lrp_shift;
+	tdm_mod_mask = TDM_MODE_MASK << tdm_mod_shift;
 
 	/* Format is priority */
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
@@ -648,6 +654,20 @@ static int i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	case SND_SOC_DAIFMT_I2S:
 		tmp |= (MOD_SDF_IIS << sdf_shift);
 		break;
+	case SND_SOC_DAIFMT_DSP_A:
+		if (!(priv->quirks & QUIRK_SUPPORTS_TDM)) {
+			dev_err(&i2s->pdev->dev, "TDM mode not supported\n");
+			return -EINVAL;
+		}
+		tdm_tmp |= (TDM_MODE_DSPA << tdm_mod_shift);
+		break;
+	case SND_SOC_DAIFMT_DSP_B:
+		if (!(priv->quirks & QUIRK_SUPPORTS_TDM)) {
+			dev_err(&i2s->pdev->dev, "TDM mode not supported\n");
+			return -EINVAL;
+		}
+		tdm_tmp |= (TDM_MODE_DSPB << tdm_mod_shift);
+		break;
 	default:
 		dev_err(&i2s->pdev->dev, "Format not supported\n");
 		return -EINVAL;
@@ -693,12 +713,17 @@ static int i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	pm_runtime_get_sync(dai->dev);
 	spin_lock_irqsave(&priv->lock, flags);
 	mod = readl(priv->addr + I2SMOD);
+
+	if (priv->quirks & QUIRK_SUPPORTS_TDM)
+		tdm = readl(priv->addr + I2STDM);
 	/*
 	 * Don't change the I2S mode if any controller is active on this
 	 * channel.
 	 */
 	if (any_active(i2s) &&
-		((mod & (sdf_mask | lrp_rlow | mod_slave)) != tmp)) {
+		(((mod & (sdf_mask | lrp_rlow | mod_slave)) != tmp) ||
+		((priv->quirks & QUIRK_SUPPORTS_TDM) &&
+		 ((tdm & tdm_mod_mask) != tdm_tmp)))) {
 		spin_unlock_irqrestore(&priv->lock, flags);
 		pm_runtime_put(dai->dev);
 		dev_err(&i2s->pdev->dev,
@@ -706,6 +731,12 @@ static int i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		return -EAGAIN;
 	}
 
+	if (priv->quirks & QUIRK_SUPPORTS_TDM) {
+		tdm &= ~(tdm_mod_mask);
+		tdm |= tdm_tmp;
+		writel(tdm, priv->addr + I2STDM);
+	}
+
 	mod &= ~(sdf_mask | lrp_rlow | mod_slave);
 	mod |= tmp;
 	writel(mod, priv->addr + I2SMOD);
@@ -812,6 +843,47 @@ static int i2s_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int i2s_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
+				unsigned int rx_mask, int slots, int slot_width)
+{
+	struct samsung_i2s_priv *priv = snd_soc_dai_get_drvdata(dai);
+	struct i2s_dai *i2s = to_info(dai);
+	u32 tdm = 0, mask = 0, val = 0;
+	unsigned long flags;
+
+	if (!(priv->quirks & QUIRK_SUPPORTS_TDM)) {
+		dev_err(&i2s->pdev->dev, "Invalid request: TDM not enabled\n");
+		return -EINVAL;
+	}
+
+	mask |= (TDM_ENABLE);
+	mask |= (TDM_TX_SLOTS_MASK << TDM_TX_SLOTS_SHIFT);
+	mask |= (TDM_RX_SLOTS_MASK << TDM_RX_SLOTS_SHIFT);
+
+	if (slots) {
+		val |= ((slots-1) & TDM_TX_SLOTS_MASK) << TDM_TX_SLOTS_SHIFT;
+		val |= ((slots-1) & TDM_RX_SLOTS_MASK) << TDM_RX_SLOTS_SHIFT;
+
+		dev_info(&i2s->pdev->dev,
+			"TDM Mode Configured - TX and RX Slots: %d\n", slots);
+
+		val |= TDM_ENABLE;
+
+		priv->tdm_slots = slots;
+	} else {
+		val = 0;
+		priv->tdm_slots = 0;
+	}
+
+	spin_lock_irqsave(&priv->lock, flags);
+	tdm = readl(priv->addr + I2STDM);
+	tdm = (tdm & ~mask) | val;
+	writel(tdm, priv->addr + I2STDM);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return 0;
+}
+
 /* We set constraints on the substream according to the version of I2S */
 static int i2s_startup(struct snd_pcm_substream *substream,
 	  struct snd_soc_dai *dai)
@@ -879,6 +951,9 @@ static int config_setup(struct i2s_dai *i2s)
 	if (!bfs && other)
 		bfs = other->bfs;
 
+	if (!bfs && (priv->quirks & QUIRK_SUPPORTS_TDM) && priv->tdm_slots)
+		bfs = blc * priv->tdm_slots;
+
 	/* Select least possible multiple(2) if no constraint set */
 	if (!bfs)
 		bfs = blc * 2;
@@ -899,6 +974,9 @@ static int config_setup(struct i2s_dai *i2s)
 			rfs = 256;
 		else
 			rfs = 384;
+
+		if ((priv->quirks & QUIRK_SUPPORTS_TDM) && priv->tdm_slots)
+			rfs /= (priv->tdm_slots / TDM_DEFAULT_SLOT_NUM_DIVIDER);
 	}
 
 	/* If already setup and running */
@@ -1110,6 +1188,7 @@ static const struct snd_soc_dai_ops samsung_i2s_dai_ops = {
 	.set_fmt = i2s_set_fmt,
 	.set_clkdiv = i2s_set_clkdiv,
 	.set_sysclk = i2s_set_sysclk,
+	.set_tdm_slot = i2s_set_tdm_slot,
 	.startup = i2s_startup,
 	.shutdown = i2s_shutdown,
 	.delay = i2s_delay,
@@ -1464,6 +1543,9 @@ static int samsung_i2s_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "failed to enable clock: %d\n", ret);
 		return ret;
 	}
+
+	priv->tdm_slots = 0;
+
 	pri_dai->dma_playback.addr = regs_base + I2STXD;
 	pri_dai->dma_capture.addr = regs_base + I2SRXD;
 	pri_dai->dma_playback.chan_name = "tx";
-- 
2.17.1

[PATCH 2/6] ASoC: samsung: i2s: configure PSR from sound card

[Padmanabhan Rajanbabu]

Currently the prescaler value in samsung I2S dai is calculated by
dividing the peripheral input clock frequency with frame clock
frequency and root clock frequency divider. This prescaler value is
used to divide the input clock to generate root clock (RCLK) from which
frame clock is generated for I2S communication.

However for the platforms which does not have a dedicated audio PLL as
an input clock source, the prescaler divider will not generate accurate
root clock frequency, which inturn affects sampling frequency also.

To overcome this scenario, support has been added to let the sound card
identify right prescaler divider value and configure the prescaler (PSR)
divider directly the from the sound card to achieve near accurate sample
frequencies

Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
---
 sound/soc/samsung/i2s-regs.h |  2 ++
 sound/soc/samsung/i2s.c      | 36 ++++++++++++++++++++++++++++++++----
 sound/soc/samsung/i2s.h      |  1 +
 3 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h
index cb2be4a3b70b..e2581dc73df2 100644
--- a/sound/soc/samsung/i2s-regs.h
+++ b/sound/soc/samsung/i2s-regs.h
@@ -132,6 +132,8 @@
 #define EXYNOS7_MOD_RCLK_192FS	7
 
 #define PSR_PSREN		(1 << 15)
+#define PSR_PSVAL_SHIFT		8
+#define PSR_PSVAL_MASK		0x3f
 
 #define FIC_TX2COUNT(x)		(((x) >>  24) & 0xf)
 #define FIC_TX1COUNT(x)		(((x) >>  16) & 0xf)
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index fb806b0af6ab..a96286b27f1d 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -59,10 +59,10 @@ struct i2s_dai {
 	/* Frame clock */
 	unsigned frmclk;
 	/*
-	 * Specifically requested RCLK, BCLK by machine driver.
+	 * Specifically requested RCLK, BCLK and PSR by machine driver.
 	 * 0 indicates CPU driver is free to choose any value.
 	 */
-	unsigned rfs, bfs;
+	unsigned int rfs, bfs, psr;
 	/* Pointer to the Primary_Fifo if this is Sec_Fifo, NULL otherwise */
 	struct i2s_dai *pri_dai;
 	/* Pointer to the Secondary_Fifo if it has one, NULL otherwise */
@@ -389,6 +389,17 @@ static inline int get_blc(struct i2s_dai *i2s)
 	}
 }
 
+static inline unsigned int get_psval(struct i2s_dai *i2s)
+{
+	struct samsung_i2s_priv *priv = i2s->priv;
+	u32 psr;
+
+	psr = readl(priv->addr + I2SPSR) >> PSR_PSVAL_SHIFT;
+	psr &= PSR_PSVAL_MASK;
+
+	return (psr + 1);
+}
+
 /* TX channel control */
 static void i2s_txctrl(struct i2s_dai *i2s, int on)
 {
@@ -994,7 +1005,11 @@ static int config_setup(struct i2s_dai *i2s)
 		return 0;
 
 	if (!(priv->quirks & QUIRK_NO_MUXPSR)) {
-		psr = priv->rclk_srcrate / i2s->frmclk / rfs;
+		if (i2s->psr)
+			psr = i2s->psr;
+		else
+			psr = priv->rclk_srcrate / i2s->frmclk / rfs;
+
 		writel(((psr - 1) << 8) | PSR_PSREN, priv->addr + I2SPSR);
 		dev_dbg(&i2s->pdev->dev,
 			"RCLK_SRC=%luHz PSR=%u, RCLK=%dfs, BCLK=%dfs\n",
@@ -1072,6 +1087,18 @@ static int i2s_set_clkdiv(struct snd_soc_dai *dai,
 		i2s->bfs = div;
 		pm_runtime_put(dai->dev);
 		break;
+	case SAMSUNG_I2S_DIV_RCLK:
+		pm_runtime_get_sync(dai->dev);
+		if ((any_active(i2s) && div && (get_psval(i2s) != div))
+			|| (other && other->psr && (other->psr != div))) {
+			pm_runtime_put(dai->dev);
+			dev_err(&i2s->pdev->dev,
+				"%s:%d Other DAI busy\n", __func__, __LINE__);
+			return -EAGAIN;
+		}
+		i2s->psr = div;
+		pm_runtime_put(dai->dev);
+		break;
 	default:
 		dev_err(&i2s->pdev->dev,
 			"Invalid clock divider(%d)\n", div_id);
@@ -1140,9 +1167,10 @@ static int samsung_i2s_dai_probe(struct snd_soc_dai *dai)
 					   other->idma_playback.addr);
 	}
 
-	/* Reset any constraint on RFS and BFS */
+	/* Reset any constraint on RFS, BFS and PSR*/
 	i2s->rfs = 0;
 	i2s->bfs = 0;
+	i2s->psr = 0;
 
 	spin_lock_irqsave(&priv->lock, flags);
 	i2s_txctrl(i2s, 0);
diff --git a/sound/soc/samsung/i2s.h b/sound/soc/samsung/i2s.h
index 78b475ef98d9..e783d33fdfac 100644
--- a/sound/soc/samsung/i2s.h
+++ b/sound/soc/samsung/i2s.h
@@ -13,6 +13,7 @@
 #define SAMSUNG_I2S_DAI_SEC    "samsung-i2s-sec"
 
 #define SAMSUNG_I2S_DIV_BCLK		1
+#define SAMSUNG_I2S_DIV_RCLK		2
 
 #define SAMSUNG_I2S_RCLKSRC_0		0
 #define SAMSUNG_I2S_RCLKSRC_1		1
-- 
2.17.1

Re: [PATCH 2/6] ASoC: samsung: i2s: configure PSR from sound card

[Mark Brown]

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

On Fri, Oct 14, 2022 at 03:51:47PM +0530, Padmanabhan Rajanbabu wrote:
> Currently the prescaler value in samsung I2S dai is calculated by
> dividing the peripheral input clock frequency with frame clock
> frequency and root clock frequency divider. This prescaler value is
> used to divide the input clock to generate root clock (RCLK) from which
> frame clock is generated for I2S communication.
> 
> However for the platforms which does not have a dedicated audio PLL as
> an input clock source, the prescaler divider will not generate accurate
> root clock frequency, which inturn affects sampling frequency also.
> 
> To overcome this scenario, support has been added to let the sound card
> identify right prescaler divider value and configure the prescaler (PSR)
> divider directly the from the sound card to achieve near accurate sample
> frequencies

It's not clear to me why the solution here is to move the configuration
to the sound card rather than to improve the I2S driver to be able to
cope with whatever the restrictions are on the PSR in these systems - it
seems more cumbersome for system integrators, especially since you've
not documented the issues or how to configure it.  Could you expand on
what the constraints are here and why it's not possible for the driver
to figure things out (given some quirk information)?

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

RE: [PATCH 2/6] ASoC: samsung: i2s: configure PSR from sound card

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: 14 October 2022 05:32 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> Cc: lgirdwood@gmail.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com; alsa-devel@alsa-project.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org
> Subject: Re: [PATCH 2/6] ASoC: samsung: i2s: configure PSR from sound card
> 
> On Fri, Oct 14, 2022 at 03:51:47PM +0530, Padmanabhan Rajanbabu wrote:
> > Currently the prescaler value in samsung I2S dai is calculated by
> > dividing the peripheral input clock frequency with frame clock
> > frequency and root clock frequency divider. This prescaler value is
> > used to divide the input clock to generate root clock (RCLK) from
> > which frame clock is generated for I2S communication.
> >
> > However for the platforms which does not have a dedicated audio PLL as
> > an input clock source, the prescaler divider will not generate
> > accurate root clock frequency, which inturn affects sampling frequency
> also.
> >
> > To overcome this scenario, support has been added to let the sound
> > card identify right prescaler divider value and configure the
> > prescaler (PSR) divider directly the from the sound card to achieve
> > near accurate sample frequencies
> 
> It's not clear to me why the solution here is to move the configuration to
the
> sound card rather than to improve the I2S driver to be able to cope with
> whatever the restrictions are on the PSR in these systems - it seems more
> cumbersome for system integrators, especially since you've not documented
> the issues or how to configure it.  Could you expand on what the
constraints
> are here and why it's not possible for the driver to figure things out
(given
> some quirk information)?

Thank you for reviewing the patch.

In Samsung I2S CPU controller, to derive the frame clock, we are supposed to
configure the PSR and RFS internal dividers. i.e.

OPCLK -> PSR -> RCLK -> RFS -> Frame clock

Where:
OPCLK - Operational clock
PSR - Operational clock prescaler
RCLK - Root Clock (derived from OPCLK based on PSR)
RFS - Root frequency selection (divider)
Frame clock - Sample frequency (derived from RCLK based on RFS)

Ultimately,

PSR = OPCLK / Frame clock / RFS

Unlike other platforms utilizing Samsung CPU DAI, FSD SoC has a limitation
on
operational clock, where the clock frequency is fixed (66 MHz) and cannot be
modified. 

Assuming that an userspace application wants perform playback @44100 Hz
and assuming that RFS divider value is configured as 256, the PSR value will
yield to

66 MHz / 44.1 KHz / 256 = 5

However if HW uses PSR = 5 to derive the frame clock from operational clock,
then

RCLK = OPCLK / PSR = 66 MHz / 5 = 13.2 MHz
Frame clock = RCLK / RFS = 13.2 MHz / 256 = 51562 Hz

The actual frame clock derived based on PSR is now different from what user
application has intended. The situation did not improve even if the RFS is
swept throughout the entire valid range.

We can overcome this scenario to an extent if we can get a flexibility to
Configure both PSR as well as RFS.

i.e. to achieve frame clock of 44100 Hz, if PSR  = 23 and RFS = 64
then frame clock = 66 MHz / 23 / 64 = 44836 Hz 

Although the sample frequency is not precise, it is very much closer to the
Intended frequency, when compared to that of the existing solution. Since
this
scenario is specific to FSD SoC and has no changes in the Samsung I2S CPU
DAI,
the configuration is being done from the sound card of FSD SoC during
hw_params.

Please let me know if you think this scenario can be approached in any other
way possible, rather than configuring from sound card.

Re: [PATCH 2/6] ASoC: samsung: i2s: configure PSR from sound card

[Mark Brown]

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

On Fri, Oct 21, 2022 at 01:30:25PM +0530, Padmanabhan Rajanbabu wrote:

> We can overcome this scenario to an extent if we can get a flexibility to
> Configure both PSR as well as RFS.

Why does it make sense for the machine driver to worry about this rather
than having the I2S controller driver configure the clock tree?

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

RE: [PATCH 2/6] ASoC: samsung: i2s: configure PSR from sound card

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: 21 October 2022 05:24 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> Cc: lgirdwood@gmail.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com; alsa-devel@alsa-project.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org
> Subject: Re: [PATCH 2/6] ASoC: samsung: i2s: configure PSR from sound card
> 
> On Fri, Oct 21, 2022 at 01:30:25PM +0530, Padmanabhan Rajanbabu wrote:
> 
> > We can overcome this scenario to an extent if we can get a flexibility
> > to Configure both PSR as well as RFS.
> 
> Why does it make sense for the machine driver to worry about this rather
> than having the I2S controller driver configure the clock tree?


                       ____________________________________________
_____           |                  __
| 
|         |	        |	             |   \
|
|CMU|	        |	             |     \
|
|FSD  |-  |---|-|--------->|       \        _________    _________
|
|___  |    |    | |op_clk0|         |      |               |     |
|             |
	  |    | |	             |MUX|----|  PSR       |----|  RFS
|--cdclk  |
	  |    | |              |         |      |_______|     |_______|
|
	  |    | |--------->|        /
|
	  |    |  op_clk1 |      /
|
	  |    | 	             |_ /
|
	  |    |___________________________________________|
	  |
	  |-----> To other FSD SoC Peripherals
	 
In FSD I2S, the clock source is not an independent source but a common clock
source being shared by many IPs in the same domain.

Changing the clock tree will impact other IPs in the domain as they are
dependent on the same source for functionality.

We can understand your point to bring the PSR changes under the I2S CPU DAI
driver by adding a separate compatible and data for the FSD SoC. But If we
take
the example of existing sound cards such as sound/soc/samsung/tm2_wm5110.c,
the op_clk is supplied via external audio pll to the controller and PLL
configuration
is taken care by the sound card. Since the configuration of PLL is more
specific to
the tm2 platform, it makes use of the flexibility of changing the RFS and
BFS
using the sysclk and clkdiv hooks provided by exynos7-i2s CPU DAI along with
PLL tuning for precise sampling frequency.

Similar to the above example, the choice of clock source under discussion is
not a
limitation of exynos7-i2s controller, but instead is a limitation on the FSD
SoC.
By using the proposed change, we can ensure that the exynos CPU DAI driver
is
giving additional hooks similar to existing hooks for BFS, RFS and CDCLK
direction
so that sound cards can use snd_soc_dai_set_sysclk and
snd_soc_dai_set_clkdiv
to customize the same.

An alternative approach is to use the cpu dai as bit clock and frame clock
consumer (i.e. in slave mode) so that codec can supply the MCLK to FSD for
playback and capture. But this will deviate from the actual usecase for FSD
SoC, where the CPU DAI is intended to function as master.

Re: [PATCH 2/6] ASoC: samsung: i2s: configure PSR from sound card

[Mark Brown]

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

On Tue, Nov 08, 2022 at 10:53:40AM +0530, Padmanabhan Rajanbabu wrote:

> > > We can overcome this scenario to an extent if we can get a flexibility
> > > to Configure both PSR as well as RFS.

> > Why does it make sense for the machine driver to worry about this rather
> > than having the I2S controller driver configure the clock tree?

> _____           |                  __
> | 
> |         |	        |	             |   \
> |
> |CMU|	        |	             |     \
> |
> |FSD  |-  |---|-|--------->|       \        _________    _________
> |
> |___  |    |    | |op_clk0|         |      |               |     |
> |             |
> 	  |    | |	             |MUX|----|  PSR       |----|  RFS
> |--cdclk  |
> 	  |    | |              |         |      |_______|     |_______|
> |
> 	  |    | |--------->|        /
> |
> 	  |    |  op_clk1 |      /
> |
> 	  |    | 	             |_ /
> |
> 	  |    |___________________________________________|
> 	  |
> 	  |-----> To other FSD SoC Peripherals

> In FSD I2S, the clock source is not an independent source but a common clock
> source being shared by many IPs in the same domain.

> Changing the clock tree will impact other IPs in the domain as they are
> dependent on the same source for functionality.

I'm not sure I follow.  Perhaps your diagram is unclear but it looks
like PSR and RFS are both after a mux which appears to select which
clock is going to be used by the I2S controller?  The usage by other
clocks appears to be upstream of the mux and dividers.

> We can understand your point to bring the PSR changes under the I2S CPU DAI
> driver by adding a separate compatible and data for the FSD SoC. But If we
> take
> the example of existing sound cards such as sound/soc/samsung/tm2_wm5110.c,
> the op_clk is supplied via external audio pll to the controller and PLL
> configuration
> is taken care by the sound card. Since the configuration of PLL is more
> specific to
> the tm2 platform, it makes use of the flexibility of changing the RFS and
> BFS
> using the sysclk and clkdiv hooks provided by exynos7-i2s CPU DAI along with
> PLL tuning for precise sampling frequency.

The big reason for the clocking control (and indeed having a custom
machine driver) with the WM5110 is that it has multiple clocks to
control and a good deal of flexibility with placing them in clock
domains and so on which have power and performance impacts.  It's
frankly a bit unclear to me if the CPU I2S controller even needs the
bitclock configuring given that the clocks are being driven by the CODEC
there, but regardless it's not clear to me why the I2S controller would
need anything other than the input clock to the block configuring? 

> Similar to the above example, the choice of clock source under discussion is
> not a
> limitation of exynos7-i2s controller, but instead is a limitation on the FSD
> SoC.
> By using the proposed change, we can ensure that the exynos CPU DAI driver
> is
> giving additional hooks similar to existing hooks for BFS, RFS and CDCLK
> direction
> so that sound cards can use snd_soc_dai_set_sysclk and
> snd_soc_dai_set_clkdiv
> to customize the same.

I'm still not seeing anything that articulates why pushing the
configuration of the dividers within the block into the machine driver
solves a problem here.  Again, what's the upside to configuring clocks
that are purely within the block?

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

RE: [PATCH 2/6] ASoC: samsung: i2s: configure PSR from sound card

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: 09 November 2022 11:09 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> Cc: lgirdwood@gmail.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com; alsa-devel@alsa-project.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org
> Subject: Re: [PATCH 2/6] ASoC: samsung: i2s: configure PSR from sound card
> 
> On Tue, Nov 08, 2022 at 10:53:40AM +0530, Padmanabhan Rajanbabu wrote:
> 
> > > > We can overcome this scenario to an extent if we can get a
> > > > flexibility to Configure both PSR as well as RFS.
> 
> > > Why does it make sense for the machine driver to worry about this
> > > rather than having the I2S controller driver configure the clock tree?
> 
> > _____           |                  __
> > |
> > |         |	        |	             |   \
> > |
> > |CMU|	        |	             |     \
> > |
> > |FSD  |-  |---|-|--------->|       \        _________    _________
> > |
> > |___  |    |    | |op_clk0|         |      |               |     |
> > |             |
> > 	  |    | |	             |MUX|----|  PSR       |----|  RFS
> > |--cdclk  |
> > 	  |    | |              |         |      |_______|     |_______|
> > |
> > 	  |    | |--------->|        /
> > |
> > 	  |    |  op_clk1 |      /
> > |
> > 	  |    | 	             |_ /
> > |
> > 	  |    |___________________________________________|
> > 	  |
> > 	  |-----> To other FSD SoC Peripherals
> 
> > In FSD I2S, the clock source is not an independent source but a common
> > clock source being shared by many IPs in the same domain.
> 
> > Changing the clock tree will impact other IPs in the domain as they
> > are dependent on the same source for functionality.
> 
> I'm not sure I follow.  Perhaps your diagram is unclear but it looks like
PSR and
> RFS are both after a mux which appears to select which clock is going to
be
> used by the I2S controller?  The usage by other clocks appears to be
> upstream of the mux and dividers.
> 
> > We can understand your point to bring the PSR changes under the I2S
> > CPU DAI driver by adding a separate compatible and data for the FSD
> > SoC. But If we take the example of existing sound cards such as
> > sound/soc/samsung/tm2_wm5110.c, the op_clk is supplied via external
> > audio pll to the controller and PLL configuration is taken care by the
> > sound card. Since the configuration of PLL is more specific to the tm2
> > platform, it makes use of the flexibility of changing the RFS and BFS
> > using the sysclk and clkdiv hooks provided by exynos7-i2s CPU DAI
> > along with PLL tuning for precise sampling frequency.
> 
> The big reason for the clocking control (and indeed having a custom
machine
> driver) with the WM5110 is that it has multiple clocks to control and a
good
> deal of flexibility with placing them in clock domains and so on which
have
> power and performance impacts.  It's frankly a bit unclear to me if the
CPU
> I2S controller even needs the bitclock configuring given that the clocks
are
> being driven by the CODEC there, but regardless it's not clear to me why
the
> I2S controller would need anything other than the input clock to the block
> configuring?
> 
> > Similar to the above example, the choice of clock source under
> > discussion is not a limitation of exynos7-i2s controller, but instead
> > is a limitation on the FSD SoC.
> > By using the proposed change, we can ensure that the exynos CPU DAI
> > driver is giving additional hooks similar to existing hooks for BFS,
> > RFS and CDCLK direction so that sound cards can use
> > snd_soc_dai_set_sysclk and snd_soc_dai_set_clkdiv to customize the
> > same.
> 
> I'm still not seeing anything that articulates why pushing the
configuration of
> the dividers within the block into the machine driver solves a problem
here.
> Again, what's the upside to configuring clocks that are purely within the
> block?

Okay, I can understand the reason for de-linking these changes from the
machine
Driver. I'll post the v2 patches integrating the PSR changes into cpu dai
driver.

Thanks,
Padmanabhan R.

Re: [PATCH 2/6] ASoC: samsung: i2s: configure PSR from sound card

[Krzysztof Kozlowski]

On 21/10/2022 04:00, Padmanabhan Rajanbabu wrote:
>> It's not clear to me why the solution here is to move the configuration to
> the
>> sound card rather than to improve the I2S driver to be able to cope with
>> whatever the restrictions are on the PSR in these systems - it seems more
>> cumbersome for system integrators, especially since you've not documented
>> the issues or how to configure it.  Could you expand on what the
> constraints
>> are here and why it's not possible for the driver to figure things out
> (given
>> some quirk information)?
> 
> Thank you for reviewing the patch.
> 
> In Samsung I2S CPU controller, to derive the frame clock, we are supposed to
> configure the PSR and RFS internal dividers. i.e.
> 
> OPCLK -> PSR -> RCLK -> RFS -> Frame clock
> 
> Where:
> OPCLK - Operational clock
> PSR - Operational clock prescaler
> RCLK - Root Clock (derived from OPCLK based on PSR)
> RFS - Root frequency selection (divider)
> Frame clock - Sample frequency (derived from RCLK based on RFS)
> 
> Ultimately,
> 
> PSR = OPCLK / Frame clock / RFS
> 
> Unlike other platforms utilizing Samsung CPU DAI, FSD SoC has a limitation
> on
> operational clock, where the clock frequency is fixed (66 MHz) and cannot be
> modified. 
> 
> Assuming that an userspace application wants perform playback @44100 Hz
> and assuming that RFS divider value is configured as 256, the PSR value will
> yield to
> 
> 66 MHz / 44.1 KHz / 256 = 5
> 
> However if HW uses PSR = 5 to derive the frame clock from operational clock,
> then
> 
> RCLK = OPCLK / PSR = 66 MHz / 5 = 13.2 MHz
> Frame clock = RCLK / RFS = 13.2 MHz / 256 = 51562 Hz
> 
> The actual frame clock derived based on PSR is now different from what user
> application has intended. The situation did not improve even if the RFS is
> swept throughout the entire valid range.
> 
> We can overcome this scenario to an extent if we can get a flexibility to
> Configure both PSR as well as RFS.
> 
> i.e. to achieve frame clock of 44100 Hz, if PSR  = 23 and RFS = 64
> then frame clock = 66 MHz / 23 / 64 = 44836 Hz 
> 
> Although the sample frequency is not precise, it is very much closer to the
> Intended frequency, when compared to that of the existing solution. Since
> this
> scenario is specific to FSD SoC and has no changes in the Samsung I2S CPU
> DAI,
> the configuration is being done from the sound card of FSD SoC during
> hw_params.
> 
> Please let me know if you think this scenario can be approached in any other
> way possible, rather than configuring from sound card.

Entire new driver for this, instead of improving existing Samsung
drivers... no, it is no the way. If you followed this approach you would
send 20 drivers for each "specific" quirk difference.

Best regards,
Krzysztof

[PATCH 3/6] dt-bindings: sound: Add sound card bindings for Tesla FSD

[Padmanabhan Rajanbabu]

Add dt-binding reference document to configure the DAI link for Tesla
FSD sound card driver.

Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
---
 .../bindings/sound/tesla,fsd-card.yaml        | 158 ++++++++++++++++++
 1 file changed, 158 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml

diff --git a/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
new file mode 100644
index 000000000000..4bd590f4ee27
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
@@ -0,0 +1,158 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2022 Samsung Electronics Co. Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/tesla,fsd-card.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Tesla FSD ASoC sound card driver
+
+maintainers:
+  - Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
+
+description: |
+  This binding describes the node properties and essential DT entries of FSD
+  SoC sound card node
+
+select: false
+
+properties:
+  compatible:
+    enum:
+      - tesla,fsd-sndcard
+
+  model:
+    description: Describes the Name of the sound card
+    $ref: /schemas/types.yaml#/definitions/string
+
+  widgets:
+    description: A list of DAPM widgets in the sound card. Each entry is a pair
+      of strings, the first being the widget name and the second being the
+      widget alias
+    $ref: /schemas/types.yaml#/definitions/string-array
+
+  audio-routing:
+    description: A list of the connections between audio components. Each entry
+      is a pair of strings, the first being the connection's sink, the second
+      being the connection's source
+    $ref: /schemas/types.yaml#/definitions/string-array
+
+  dai-tdm-slot-num:
+    description: Enables TDM mode and specifies the number of TDM slots to be
+      enabled
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8]
+    default: 2
+
+  dai-tdm-slot-width:
+    description: Specifies the slot width of each TDm slot enabled
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [8, 16, 24]
+    default: 16
+
+  dai-link:
+    description: Holds the DAI link data between CPU, Codec and Platform
+    type: object
+    properties:
+      link-name:
+        description: Specifies the name of the DAI link
+        $ref: /schemas/types.yaml#/definitions/string
+
+      dai-format:
+        description: Specifies the serial data format of CPU DAI
+        $ref: /schemas/types.yaml#/definitions/string
+
+      tesla,bitclock-master:
+        description: Specifies the phandle of bitclock master DAI
+        $ref: /schemas/types.yaml#/definitions/phandle
+
+      tesla,frame-master:
+        description: Specifies the phandle of frameclock master DAI
+        $ref: /schemas/types.yaml#/definitions/phandle
+
+      cpu:
+        description: Holds the CPU DAI node and instance
+        type: object
+        properties:
+          sound-dai:
+            description: Specifies the phandle of CPU DAI node
+            $ref: /schemas/types.yaml#/definitions/phandle-array
+
+        required:
+          - sound-dai
+
+      codec:
+        description: Holds the Codec DAI node and instance
+        type: object
+        properties:
+          sound-dai:
+            description: Specifies the phandle of Codec DAI node
+            $ref: /schemas/types.yaml#/definitions/phandle-array
+
+        required:
+          - sound-dai
+
+    required:
+      - link-name
+      - dai-format
+      - tesla,bitclock-master
+      - tesla,frame-master
+      - cpu
+
+dependencies:
+  dai-tdm-slot-width: [ 'dai-tdm-slot-num' ]
+
+required:
+  - compatible
+  - model
+  - dai-link
+
+additionalProperties: false
+
+examples:
+  - |
+    sound {
+        compatible = "tesla,fsd-sndcard";
+        status = "disabled";
+        model = "fsd-i2s";
+
+        primary-dai-link-0 {
+            link-name = "fsd-primary-0";
+            dai-format = "i2s";
+            tesla,bitclock-master = <&tdm_0>;
+            tesla,frame-master = <&tdm_0>;
+            cpu {
+                sound-dai = <&tdm_0 0>;
+            };
+        };
+
+        secondary-dai-link-0 {
+            link-name = "fsd-secondary-0";
+            dai-format = "i2s";
+            tesla,bitclock-master = <&tdm_0>;
+            tesla,frame-master = <&tdm_0>;
+            cpu {
+                sound-dai = <&tdm_0 1>;
+            };
+        };
+
+        primary-dai-link-1 {
+            link-name = "fsd-primary-1";
+            dai-format = "i2s";
+            tesla,bitclock-master = <&tdm_1>;
+            tesla,frame-master = <&tdm_1>;
+            cpu {
+                sound-dai = <&tdm_1 0>;
+            };
+        };
+
+        secondary-dai-link-1 {
+            link-name = "fsd-secondary-1";
+            dai-format = "i2s";
+            tesla,bitclock-master = <&tdm_1>;
+            tesla,frame-master = <&tdm_1>;
+            cpu {
+                sound-dai = <&tdm_1 1>;
+            };
+        };
+    };
-- 
2.17.1

Re: [PATCH 3/6] dt-bindings: sound: Add sound card bindings for Tesla FSD

[Rob Herring]

On Fri, Oct 14, 2022 at 03:51:48PM +0530, Padmanabhan Rajanbabu wrote:
> Add dt-binding reference document to configure the DAI link for Tesla
> FSD sound card driver.

The simple-card or graph-card bindings don't work for you?

> 
> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> ---
>  .../bindings/sound/tesla,fsd-card.yaml        | 158 ++++++++++++++++++
>  1 file changed, 158 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> new file mode 100644
> index 000000000000..4bd590f4ee27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> @@ -0,0 +1,158 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2022 Samsung Electronics Co. Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/tesla,fsd-card.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Tesla FSD ASoC sound card driver
> +
> +maintainers:
> +  - Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> +
> +description: |
> +  This binding describes the node properties and essential DT entries of FSD
> +  SoC sound card node
> +
> +select: false

Never apply this schema. Why?

> +
> +properties:
> +  compatible:
> +    enum:
> +      - tesla,fsd-sndcard
> +
> +  model:
> +    description: Describes the Name of the sound card
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +  widgets:
> +    description: A list of DAPM widgets in the sound card. Each entry is a pair
> +      of strings, the first being the widget name and the second being the
> +      widget alias
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +
> +  audio-routing:
> +    description: A list of the connections between audio components. Each entry
> +      is a pair of strings, the first being the connection's sink, the second
> +      being the connection's source
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +
> +  dai-tdm-slot-num:
> +    description: Enables TDM mode and specifies the number of TDM slots to be
> +      enabled
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8]

maximum: 8

> +    default: 2
> +
> +  dai-tdm-slot-width:
> +    description: Specifies the slot width of each TDm slot enabled
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [8, 16, 24]
> +    default: 16

All the above have types defined. You should not be redefining the 
types.

> +
> +  dai-link:
> +    description: Holds the DAI link data between CPU, Codec and Platform
> +    type: object

       additionalProperties: false

> +    properties:
> +      link-name:
> +        description: Specifies the name of the DAI link
> +        $ref: /schemas/types.yaml#/definitions/string
> +
> +      dai-format:
> +        description: Specifies the serial data format of CPU DAI
> +        $ref: /schemas/types.yaml#/definitions/string
> +
> +      tesla,bitclock-master:
> +        description: Specifies the phandle of bitclock master DAI
> +        $ref: /schemas/types.yaml#/definitions/phandle
> +
> +      tesla,frame-master:
> +        description: Specifies the phandle of frameclock master DAI
> +        $ref: /schemas/types.yaml#/definitions/phandle

These are common properties. Drop 'tesla'.

> +
> +      cpu:
> +        description: Holds the CPU DAI node and instance
> +        type: object

           additionalProperties: false

> +        properties:
> +          sound-dai:
> +            description: Specifies the phandle of CPU DAI node
> +            $ref: /schemas/types.yaml#/definitions/phandle-array
> +
> +        required:
> +          - sound-dai
> +
> +      codec:
> +        description: Holds the Codec DAI node and instance
> +        type: object

           additionalProperties: false

> +        properties:
> +          sound-dai:
> +            description: Specifies the phandle of Codec DAI node
> +            $ref: /schemas/types.yaml#/definitions/phandle-array

Already has a type. Need to define how many entries (maxItems: 1 ?).

> +
> +        required:
> +          - sound-dai
> +
> +    required:
> +      - link-name
> +      - dai-format
> +      - tesla,bitclock-master
> +      - tesla,frame-master
> +      - cpu
> +
> +dependencies:
> +  dai-tdm-slot-width: [ 'dai-tdm-slot-num' ]

This should be captured with tdm-slot.txt converted to schema.

> +
> +required:
> +  - compatible
> +  - model
> +  - dai-link
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    sound {
> +        compatible = "tesla,fsd-sndcard";
> +        status = "disabled";

Why have a disabled example? Other than your example won't pass your 
schema.

> +        model = "fsd-i2s";
> +
> +        primary-dai-link-0 {
> +            link-name = "fsd-primary-0";
> +            dai-format = "i2s";
> +            tesla,bitclock-master = <&tdm_0>;
> +            tesla,frame-master = <&tdm_0>;
> +            cpu {
> +                sound-dai = <&tdm_0 0>;
> +            };
> +        };
> +
> +        secondary-dai-link-0 {
> +            link-name = "fsd-secondary-0";
> +            dai-format = "i2s";
> +            tesla,bitclock-master = <&tdm_0>;
> +            tesla,frame-master = <&tdm_0>;
> +            cpu {
> +                sound-dai = <&tdm_0 1>;
> +            };
> +        };
> +
> +        primary-dai-link-1 {
> +            link-name = "fsd-primary-1";
> +            dai-format = "i2s";
> +            tesla,bitclock-master = <&tdm_1>;
> +            tesla,frame-master = <&tdm_1>;
> +            cpu {
> +                sound-dai = <&tdm_1 0>;
> +            };
> +        };
> +
> +        secondary-dai-link-1 {
> +            link-name = "fsd-secondary-1";
> +            dai-format = "i2s";
> +            tesla,bitclock-master = <&tdm_1>;
> +            tesla,frame-master = <&tdm_1>;
> +            cpu {
> +                sound-dai = <&tdm_1 1>;
> +            };
> +        };
> +    };
> -- 
> 2.17.1
> 
> 

RE: [PATCH 3/6] dt-bindings: sound: Add sound card bindings for Tesla FSD

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: 14 October 2022 08:43 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> Cc: lgirdwood@gmail.com; broonie@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com; alsa-devel@alsa-project.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org
> Subject: Re: [PATCH 3/6] dt-bindings: sound: Add sound card bindings for
> Tesla FSD
> 
> On Fri, Oct 14, 2022 at 03:51:48PM +0530, Padmanabhan Rajanbabu wrote:
> > Add dt-binding reference document to configure the DAI link for Tesla
> > FSD sound card driver.
> 
> The simple-card or graph-card bindings don't work for you?
Thank you for reviewing the patch.

The actual reason for having a custom sound card driver lies in the fact
that the Samsung i2s cpu dai requires configuration of some Samsung
specific properties like rfs, bfs, codec clock direction and root clock
source. We do not have flexibility of configuring the same in simple card
driver (sound/soc/generic/simple-card.c) or audio graph card driver 
(sound/soc/generic/audio-graph-card.c). The binding has been added to
support the custom sound card driver.

I understand from your query that the newly added card has device tree
nodes and properties which are used in simple card as well, but having a
different or new prefixes. I believe to address that, we can restructure
the string names to generic ones. But I would like to understand, to reuse
the simple card or audio graph card bindings, can we add secondary
compatible strings in the simple card dt-binding for the custom sound card
to probe ?
> 
> >
> > Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> > ---
> >  .../bindings/sound/tesla,fsd-card.yaml        | 158 ++++++++++++++++++
> >  1 file changed, 158 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> > b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> > new file mode 100644
> > index 000000000000..4bd590f4ee27
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> > @@ -0,0 +1,158 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # Copyright
> > +2022 Samsung Electronics Co. Ltd.
> > +%YAML 1.2
> > +---
> > +$id:
> > +https://protect2.fireeye.com/v1/url?k=4ae54403-157e7d1c-4ae4cf4c-
> 000b
> > +abdfecba-9eb398ea304f8ae8&q=1&e=4935bed0-ce62-47dd-8faf-
> 4750b01e22d3&
> >
> +u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fsound%2Ftesla%2Cfsd-
> card.ya
> > +ml%23
> > +$schema:
> > +https://protect2.fireeye.com/v1/url?k=8c72226e-d3e91b71-8c73a921-
> 000b
> > +abdfecba-3ce999f6c052255b&q=1&e=4935bed0-ce62-47dd-8faf-
> 4750b01e22d3&
> > +u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
> > +
> > +title: Tesla FSD ASoC sound card driver
> > +
> > +maintainers:
> > +  - Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> > +
> > +description: |
> > +  This binding describes the node properties and essential DT entries
> > +of FSD
> > +  SoC sound card node
> > +
> > +select: false
> 
> Never apply this schema. Why?
Sorry about it, let me change the select property to true in the next
patchset
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - tesla,fsd-sndcard
> > +
> > +  model:
> > +    description: Describes the Name of the sound card
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +
> > +  widgets:
> > +    description: A list of DAPM widgets in the sound card. Each entry
is a pair
> > +      of strings, the first being the widget name and the second being
the
> > +      widget alias
> > +    $ref: /schemas/types.yaml#/definitions/string-array
> > +
> > +  audio-routing:
> > +    description: A list of the connections between audio components.
Each entry
> > +      is a pair of strings, the first being the connection's sink, the
second
> > +      being the connection's source
> > +    $ref: /schemas/types.yaml#/definitions/string-array
> > +
> > +  dai-tdm-slot-num:
> > +    description: Enables TDM mode and specifies the number of TDM slots
to be
> > +      enabled
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8]
> 
> maximum: 8
Okay
> 
> > +    default: 2
> > +
> > +  dai-tdm-slot-width:
> > +    description: Specifies the slot width of each TDm slot enabled
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [8, 16, 24]
> > +    default: 16
> 
> All the above have types defined. You should not be redefining the types.
Okay
> 
> > +
> > +  dai-link:
> > +    description: Holds the DAI link data between CPU, Codec and
Platform
> > +    type: object
> 
>        additionalProperties: false
okay
> 
> > +    properties:
> > +      link-name:
> > +        description: Specifies the name of the DAI link
> > +        $ref: /schemas/types.yaml#/definitions/string
> > +
> > +      dai-format:
> > +        description: Specifies the serial data format of CPU DAI
> > +        $ref: /schemas/types.yaml#/definitions/string
> > +
> > +      tesla,bitclock-master:
> > +        description: Specifies the phandle of bitclock master DAI
> > +        $ref: /schemas/types.yaml#/definitions/phandle
> > +
> > +      tesla,frame-master:
> > +        description: Specifies the phandle of frameclock master DAI
> > +        $ref: /schemas/types.yaml#/definitions/phandle
> 
> These are common properties. Drop 'tesla'.
Okay
> 
> > +
> > +      cpu:
> > +        description: Holds the CPU DAI node and instance
> > +        type: object
> 
>            additionalProperties: false
Okay
> 
> > +        properties:
> > +          sound-dai:
> > +            description: Specifies the phandle of CPU DAI node
> > +            $ref: /schemas/types.yaml#/definitions/phandle-array
> > +
> > +        required:
> > +          - sound-dai
> > +
> > +      codec:
> > +        description: Holds the Codec DAI node and instance
> > +        type: object
> 
>            additionalProperties: false
Okay
> 
> > +        properties:
> > +          sound-dai:
> > +            description: Specifies the phandle of Codec DAI node
> > +            $ref: /schemas/types.yaml#/definitions/phandle-array
> 
> Already has a type. Need to define how many entries (maxItems: 1 ?).
Okay. I'll update in the upcoming patch set
> 
> > +
> > +        required:
> > +          - sound-dai
> > +
> > +    required:
> > +      - link-name
> > +      - dai-format
> > +      - tesla,bitclock-master
> > +      - tesla,frame-master
> > +      - cpu
> > +
> > +dependencies:
> > +  dai-tdm-slot-width: [ 'dai-tdm-slot-num' ]
> 
> This should be captured with tdm-slot.txt converted to schema.
Okay
> 
> > +
> > +required:
> > +  - compatible
> > +  - model
> > +  - dai-link
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    sound {
> > +        compatible = "tesla,fsd-sndcard";
> > +        status = "disabled";
> 
> Why have a disabled example? Other than your example won't pass your
> schema.
Thanks for noticing, I'll double check and change the example keeping the
status 
as enabled
> 
> > +        model = "fsd-i2s";
> > +
> > +        primary-dai-link-0 {
> > +            link-name = "fsd-primary-0";
> > +            dai-format = "i2s";
> > +            tesla,bitclock-master = <&tdm_0>;
> > +            tesla,frame-master = <&tdm_0>;
> > +            cpu {
> > +                sound-dai = <&tdm_0 0>;
> > +            };
> > +        };
> > +
> > +        secondary-dai-link-0 {
> > +            link-name = "fsd-secondary-0";
> > +            dai-format = "i2s";
> > +            tesla,bitclock-master = <&tdm_0>;
> > +            tesla,frame-master = <&tdm_0>;
> > +            cpu {
> > +                sound-dai = <&tdm_0 1>;
> > +            };
> > +        };
> > +
> > +        primary-dai-link-1 {
> > +            link-name = "fsd-primary-1";
> > +            dai-format = "i2s";
> > +            tesla,bitclock-master = <&tdm_1>;
> > +            tesla,frame-master = <&tdm_1>;
> > +            cpu {
> > +                sound-dai = <&tdm_1 0>;
> > +            };
> > +        };
> > +
> > +        secondary-dai-link-1 {
> > +            link-name = "fsd-secondary-1";
> > +            dai-format = "i2s";
> > +            tesla,bitclock-master = <&tdm_1>;
> > +            tesla,frame-master = <&tdm_1>;
> > +            cpu {
> > +                sound-dai = <&tdm_1 1>;
> > +            };
> > +        };
> > +    };
> > --
> > 2.17.1
> >
> >

Re: [PATCH 3/6] dt-bindings: sound: Add sound card bindings for Tesla FSD

[Krzysztof Kozlowski]

On 21/10/2022 04:44, Padmanabhan Rajanbabu wrote:
>> On Fri, Oct 14, 2022 at 03:51:48PM +0530, Padmanabhan Rajanbabu wrote:
>>> Add dt-binding reference document to configure the DAI link for Tesla
>>> FSD sound card driver.
>>
>> The simple-card or graph-card bindings don't work for you?
> Thank you for reviewing the patch.
> 
> The actual reason for having a custom sound card driver lies in the fact
> that the Samsung i2s cpu dai requires configuration of some Samsung
> specific properties like rfs, bfs, codec clock direction and root clock
> source. We do not have flexibility of configuring the same in simple card
> driver (sound/soc/generic/simple-card.c) or audio graph card driver 
> (sound/soc/generic/audio-graph-card.c). The binding has been added to
> support the custom sound card driver.
> 
> I understand from your query that the newly added card has device tree
> nodes and properties which are used in simple card as well, but having a
> different or new prefixes. I believe to address that, we can restructure
> the string names to generic ones. 

You must use generic, existing properties where applicable.

> But I would like to understand, to reuse
> the simple card or audio graph card bindings, can we add secondary
> compatible strings in the simple card dt-binding for the custom sound card
> to probe ?

If you see other vendor compatibles there, then yes... But there aren't
any, right?

>>
>>>
>>> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
>>> ---
>>>  .../bindings/sound/tesla,fsd-card.yaml        | 158 ++++++++++++++++++
>>>  1 file changed, 158 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
>>> b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
>>> new file mode 100644
>>> index 000000000000..4bd590f4ee27
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
>>> @@ -0,0 +1,158 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # Copyright
>>> +2022 Samsung Electronics Co. Ltd.
>>> +%YAML 1.2
>>> +---
>>> +$id:
>>> +https://protect2.fireeye.com/v1/url?k=4ae54403-157e7d1c-4ae4cf4c-
>> 000b
>>> +abdfecba-9eb398ea304f8ae8&q=1&e=4935bed0-ce62-47dd-8faf-
>> 4750b01e22d3&
>>>
>> +u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fsound%2Ftesla%2Cfsd-
>> card.ya
>>> +ml%23
>>> +$schema:
>>> +https://protect2.fireeye.com/v1/url?k=8c72226e-d3e91b71-8c73a921-
>> 000b
>>> +abdfecba-3ce999f6c052255b&q=1&e=4935bed0-ce62-47dd-8faf-
>> 4750b01e22d3&
>>> +u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>>> +
>>> +title: Tesla FSD ASoC sound card driver
>>> +
>>> +maintainers:
>>> +  - Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
>>> +
>>> +description: |
>>> +  This binding describes the node properties and essential DT entries
>>> +of FSD
>>> +  SoC sound card node
>>> +
>>> +select: false
>>
>> Never apply this schema. Why?
> Sorry about it, let me change the select property to true in the next
> patchset

No, just drop it. Look at other bindings or at example-schema

>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - tesla,fsd-sndcard
>>> +
>>> +  model:
>>> +    description: Describes the Name of the sound card
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +
>>> +  widgets:
>>> +    description: A list of DAPM widgets in the sound card. Each entry
> is a pair
>>> +      of strings, the first being the widget name and the second being
> the
>>> +      widget alias
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>> +
>>> +  audio-routing:
>>> +    description: A list of the connections between audio components.
> Each entry
>>> +      is a pair of strings, the first being the connection's sink, the
> second
>>> +      being the connection's source
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>> +
>>> +  dai-tdm-slot-num:
>>> +    description: Enables TDM mode and specifies the number of TDM slots
> to be
>>> +      enabled
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8]
>>
>> maximum: 8
> Okay
>>
>>> +    default: 2
>>> +
>>> +  dai-tdm-slot-width:
>>> +    description: Specifies the slot width of each TDm slot enabled
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [8, 16, 24]
>>> +    default: 16
>>
>> All the above have types defined. You should not be redefining the types.
> Okay
>>
>>> +
>>> +  dai-link:
>>> +    description: Holds the DAI link data between CPU, Codec and
> Platform
>>> +    type: object
>>
>>        additionalProperties: false
> okay
>>
>>> +    properties:
>>> +      link-name:
>>> +        description: Specifies the name of the DAI link
>>> +        $ref: /schemas/types.yaml#/definitions/string
>>> +
>>> +      dai-format:
>>> +        description: Specifies the serial data format of CPU DAI
>>> +        $ref: /schemas/types.yaml#/definitions/string
>>> +
>>> +      tesla,bitclock-master:
>>> +        description: Specifies the phandle of bitclock master DAI
>>> +        $ref: /schemas/types.yaml#/definitions/phandle
>>> +
>>> +      tesla,frame-master:
>>> +        description: Specifies the phandle of frameclock master DAI
>>> +        $ref: /schemas/types.yaml#/definitions/phandle
>>
>> These are common properties. Drop 'tesla'.
> Okay
>>
>>> +
>>> +      cpu:
>>> +        description: Holds the CPU DAI node and instance
>>> +        type: object
>>
>>            additionalProperties: false
> Okay
>>
>>> +        properties:
>>> +          sound-dai:
>>> +            description: Specifies the phandle of CPU DAI node
>>> +            $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +
>>> +        required:
>>> +          - sound-dai
>>> +
>>> +      codec:
>>> +        description: Holds the Codec DAI node and instance
>>> +        type: object
>>
>>            additionalProperties: false
> Okay
>>
>>> +        properties:
>>> +          sound-dai:
>>> +            description: Specifies the phandle of Codec DAI node
>>> +            $ref: /schemas/types.yaml#/definitions/phandle-array
>>
>> Already has a type. Need to define how many entries (maxItems: 1 ?).
> Okay. I'll update in the upcoming patch set
>>
>>> +
>>> +        required:
>>> +          - sound-dai
>>> +
>>> +    required:
>>> +      - link-name
>>> +      - dai-format
>>> +      - tesla,bitclock-master
>>> +      - tesla,frame-master
>>> +      - cpu
>>> +
>>> +dependencies:
>>> +  dai-tdm-slot-width: [ 'dai-tdm-slot-num' ]
>>
>> This should be captured with tdm-slot.txt converted to schema.
> Okay
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - model
>>> +  - dai-link
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    sound {
>>> +        compatible = "tesla,fsd-sndcard";
>>> +        status = "disabled";
>>
>> Why have a disabled example? Other than your example won't pass your
>> schema.
> Thanks for noticing, I'll double check and change the example keeping the
> status 
> as enabled

No, just drop. Start with example-schema instead.

Best regards,
Krzysztof

RE: [PATCH 3/6] dt-bindings: sound: Add sound card bindings for Tesla FSD

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 22 October 2022 10:19 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>; 'Rob Herring'
> <robh@kernel.org>
> Cc: lgirdwood@gmail.com; broonie@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com; alsa-devel@alsa-project.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org
> Subject: Re: [PATCH 3/6] dt-bindings: sound: Add sound card bindings for
> Tesla FSD
> 
> On 21/10/2022 04:44, Padmanabhan Rajanbabu wrote:
> >> On Fri, Oct 14, 2022 at 03:51:48PM +0530, Padmanabhan Rajanbabu wrote:
> >>> Add dt-binding reference document to configure the DAI link for
> >>> Tesla FSD sound card driver.
> >>
> >> The simple-card or graph-card bindings don't work for you?
> > Thank you for reviewing the patch.
> >
> > The actual reason for having a custom sound card driver lies in the
> > fact that the Samsung i2s cpu dai requires configuration of some
> > Samsung specific properties like rfs, bfs, codec clock direction and
> > root clock source. We do not have flexibility of configuring the same
> > in simple card driver (sound/soc/generic/simple-card.c) or audio graph
> > card driver (sound/soc/generic/audio-graph-card.c). The binding has
> > been added to support the custom sound card driver.
> >
> > I understand from your query that the newly added card has device tree
> > nodes and properties which are used in simple card as well, but having
> > a different or new prefixes. I believe to address that, we can
> > restructure the string names to generic ones.
> 
> You must use generic, existing properties where applicable.

Okay

> 
> > But I would like to understand, to reuse the simple card or audio
> > graph card bindings, can we add secondary compatible strings in the
> > simple card dt-binding for the custom sound card to probe ?
> 
> If you see other vendor compatibles there, then yes... But there aren't any,
> right?

Yes you are right, we don't see other vendor compatibles. But, am I allowed
to add such secondary compatibles so that we can extend the simple card
and its utilities to a large extent?

If no, then I believe we will need a separate binding to extend the generic
properties.
> 
> >>
> >>>
> >>> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> >>> ---
> >>>  .../bindings/sound/tesla,fsd-card.yaml        | 158 ++++++++++++++++++
> >>>  1 file changed, 158 insertions(+)
> >>>  create mode 100644
> >>> Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> >>> b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> >>> new file mode 100644
> >>> index 000000000000..4bd590f4ee27
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/sound/tesla,fsd-card.yaml
> >>> @@ -0,0 +1,158 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) #
> >>> +Copyright
> >>> +2022 Samsung Electronics Co. Ltd.
> >>> +%YAML 1.2
> >>> +---
> >>> +$id:
> >>> +https://protect2.fireeye.com/v1/url?k=4ae54403-157e7d1c-4ae4cf4c-
> >> 000b
> >>> +abdfecba-9eb398ea304f8ae8&q=1&e=4935bed0-ce62-47dd-8faf-
> >> 4750b01e22d3&
> >>>
> >>
> +u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fsound%2Ftesla%2Cfsd-
> >> card.ya
> >>> +ml%23
> >>> +$schema:
> >>> +https://protect2.fireeye.com/v1/url?k=8c72226e-d3e91b71-8c73a921-
> >> 000b
> >>> +abdfecba-3ce999f6c052255b&q=1&e=4935bed0-ce62-47dd-8faf-
> >> 4750b01e22d3&
> >>> +u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
> >>> +
> >>> +title: Tesla FSD ASoC sound card driver
> >>> +
> >>> +maintainers:
> >>> +  - Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> >>> +
> >>> +description: |
> >>> +  This binding describes the node properties and essential DT
> >>> +entries of FSD
> >>> +  SoC sound card node
> >>> +
> >>> +select: false
> >>
> >> Never apply this schema. Why?
> > Sorry about it, let me change the select property to true in the next
> > patchset
> 
> No, just drop it. Look at other bindings or at example-schema
> 
> >>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - tesla,fsd-sndcard
> >>> +
> >>> +  model:
> >>> +    description: Describes the Name of the sound card
> >>> +    $ref: /schemas/types.yaml#/definitions/string
> >>> +
> >>> +  widgets:
> >>> +    description: A list of DAPM widgets in the sound card. Each
> >>> + entry
> > is a pair
> >>> +      of strings, the first being the widget name and the second
> >>> + being
> > the
> >>> +      widget alias
> >>> +    $ref: /schemas/types.yaml#/definitions/string-array
> >>> +
> >>> +  audio-routing:
> >>> +    description: A list of the connections between audio components.
> > Each entry
> >>> +      is a pair of strings, the first being the connection's sink,
> >>> + the
> > second
> >>> +      being the connection's source
> >>> +    $ref: /schemas/types.yaml#/definitions/string-array
> >>> +
> >>> +  dai-tdm-slot-num:
> >>> +    description: Enables TDM mode and specifies the number of TDM
> >>> + slots
> > to be
> >>> +      enabled
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7, 8]
> >>
> >> maximum: 8
> > Okay
> >>
> >>> +    default: 2
> >>> +
> >>> +  dai-tdm-slot-width:
> >>> +    description: Specifies the slot width of each TDm slot enabled
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    enum: [8, 16, 24]
> >>> +    default: 16
> >>
> >> All the above have types defined. You should not be redefining the types.
> > Okay
> >>
> >>> +
> >>> +  dai-link:
> >>> +    description: Holds the DAI link data between CPU, Codec and
> > Platform
> >>> +    type: object
> >>
> >>        additionalProperties: false
> > okay
> >>
> >>> +    properties:
> >>> +      link-name:
> >>> +        description: Specifies the name of the DAI link
> >>> +        $ref: /schemas/types.yaml#/definitions/string
> >>> +
> >>> +      dai-format:
> >>> +        description: Specifies the serial data format of CPU DAI
> >>> +        $ref: /schemas/types.yaml#/definitions/string
> >>> +
> >>> +      tesla,bitclock-master:
> >>> +        description: Specifies the phandle of bitclock master DAI
> >>> +        $ref: /schemas/types.yaml#/definitions/phandle
> >>> +
> >>> +      tesla,frame-master:
> >>> +        description: Specifies the phandle of frameclock master DAI
> >>> +        $ref: /schemas/types.yaml#/definitions/phandle
> >>
> >> These are common properties. Drop 'tesla'.
> > Okay
> >>
> >>> +
> >>> +      cpu:
> >>> +        description: Holds the CPU DAI node and instance
> >>> +        type: object
> >>
> >>            additionalProperties: false
> > Okay
> >>
> >>> +        properties:
> >>> +          sound-dai:
> >>> +            description: Specifies the phandle of CPU DAI node
> >>> +            $ref: /schemas/types.yaml#/definitions/phandle-array
> >>> +
> >>> +        required:
> >>> +          - sound-dai
> >>> +
> >>> +      codec:
> >>> +        description: Holds the Codec DAI node and instance
> >>> +        type: object
> >>
> >>            additionalProperties: false
> > Okay
> >>
> >>> +        properties:
> >>> +          sound-dai:
> >>> +            description: Specifies the phandle of Codec DAI node
> >>> +            $ref: /schemas/types.yaml#/definitions/phandle-array
> >>
> >> Already has a type. Need to define how many entries (maxItems: 1 ?).
> > Okay. I'll update in the upcoming patch set
> >>
> >>> +
> >>> +        required:
> >>> +          - sound-dai
> >>> +
> >>> +    required:
> >>> +      - link-name
> >>> +      - dai-format
> >>> +      - tesla,bitclock-master
> >>> +      - tesla,frame-master
> >>> +      - cpu
> >>> +
> >>> +dependencies:
> >>> +  dai-tdm-slot-width: [ 'dai-tdm-slot-num' ]
> >>
> >> This should be captured with tdm-slot.txt converted to schema.
> > Okay
> >>
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - model
> >>> +  - dai-link
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    sound {
> >>> +        compatible = "tesla,fsd-sndcard";
> >>> +        status = "disabled";
> >>
> >> Why have a disabled example? Other than your example won't pass your
> >> schema.
> > Thanks for noticing, I'll double check and change the example keeping
> > the status as enabled
> 
> No, just drop. Start with example-schema instead.
> 
> Best regards,
> Krzysztof

Re: [PATCH 3/6] dt-bindings: sound: Add sound card bindings for Tesla FSD

[Krzysztof Kozlowski]

On 08/11/2022 06:33, Padmanabhan Rajanbabu wrote:
>>>
>>> The actual reason for having a custom sound card driver lies in the
>>> fact that the Samsung i2s cpu dai requires configuration of some
>>> Samsung specific properties like rfs, bfs, codec clock direction and
>>> root clock source. We do not have flexibility of configuring the same
>>> in simple card driver (sound/soc/generic/simple-card.c) or audio graph
>>> card driver (sound/soc/generic/audio-graph-card.c). The binding has
>>> been added to support the custom sound card driver.
>>>
>>> I understand from your query that the newly added card has device tree
>>> nodes and properties which are used in simple card as well, but having
>>> a different or new prefixes. I believe to address that, we can
>>> restructure the string names to generic ones.
>>
>> You must use generic, existing properties where applicable.
> 
> Okay
> 
>>
>>> But I would like to understand, to reuse the simple card or audio
>>> graph card bindings, can we add secondary compatible strings in the
>>> simple card dt-binding for the custom sound card to probe ?
>>
>> If you see other vendor compatibles there, then yes... But there aren't any,
>> right?
> 
> Yes you are right, we don't see other vendor compatibles. But, am I allowed
> to add such secondary compatibles so that we can extend the simple card
> and its utilities to a large extent?
> 
> If no, then I believe we will need a separate binding to extend the generic
> properties.

If you have any custom properties, then yes. But you don't have.


Best regards,
Krzysztof

[PATCH 4/6] ASoC: samsung: fsd: Add FSD soundcard driver

[Padmanabhan Rajanbabu]

Add a soundcard driver for FSD audio interface to bridge the
CPU DAI of samsung I2S with the codec and platform driver.

Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
---
 sound/soc/samsung/Kconfig    |  12 ++
 sound/soc/samsung/Makefile   |   2 +
 sound/soc/samsung/fsd-card.c | 349 +++++++++++++++++++++++++++++++++++
 3 files changed, 363 insertions(+)
 create mode 100644 sound/soc/samsung/fsd-card.c

diff --git a/sound/soc/samsung/Kconfig b/sound/soc/samsung/Kconfig
index 2a61e620cd3b..344503522bd3 100644
--- a/sound/soc/samsung/Kconfig
+++ b/sound/soc/samsung/Kconfig
@@ -239,4 +239,16 @@ config SND_SOC_SAMSUNG_MIDAS_WM1811
 	help
 	  Say Y if you want to add support for SoC audio on the Midas boards.
 
+config SND_SOC_TESLA_FSD
+	tristate "SoC I2S Audio support for Tesla FSD board"
+	depends on SND_SOC_SAMSUNG
+	select SND_SIMPLE_CARD_UTILS
+	select SND_SAMSUNG_I2S
+	help
+	  Say Y if you want to add support for SOC audio on the FSD board.
+	  This will select the SND_SIMPLE_CARD_UTILS to use the dummy
+	  codec driver, incase the codec node is not specified in the device
+	  tree. This sound card driver uses Samsung I2S controller as CPU
+	  and platform DAI.
+
 endif #SND_SOC_SAMSUNG
diff --git a/sound/soc/samsung/Makefile b/sound/soc/samsung/Makefile
index 398e843f388c..0dad88fdb1a8 100644
--- a/sound/soc/samsung/Makefile
+++ b/sound/soc/samsung/Makefile
@@ -43,6 +43,7 @@ snd-soc-arndale-objs := arndale.o
 snd-soc-tm2-wm5110-objs := tm2_wm5110.o
 snd-soc-aries-wm8994-objs := aries_wm8994.o
 snd-soc-midas-wm1811-objs := midas_wm1811.o
+snd-soc-fsd-objs := fsd-card.o
 
 obj-$(CONFIG_SND_SOC_SAMSUNG_JIVE_WM8750) += snd-soc-jive-wm8750.o
 obj-$(CONFIG_SND_SOC_SAMSUNG_NEO1973_WM8753) += snd-soc-neo1973-wm8753.o
@@ -68,3 +69,4 @@ obj-$(CONFIG_SND_SOC_ARNDALE) += snd-soc-arndale.o
 obj-$(CONFIG_SND_SOC_SAMSUNG_TM2_WM5110) += snd-soc-tm2-wm5110.o
 obj-$(CONFIG_SND_SOC_SAMSUNG_ARIES_WM8994) += snd-soc-aries-wm8994.o
 obj-$(CONFIG_SND_SOC_SAMSUNG_MIDAS_WM1811) += snd-soc-midas-wm1811.o
+obj-$(CONFIG_SND_SOC_TESLA_FSD) += snd-soc-fsd.o
diff --git a/sound/soc/samsung/fsd-card.c b/sound/soc/samsung/fsd-card.c
new file mode 100644
index 000000000000..806a4d3b99fd
--- /dev/null
+++ b/sound/soc/samsung/fsd-card.c
@@ -0,0 +1,349 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ALSA SoC Audio Layer - FSD Soundcard driver
+ *
+ * Copyright (c) 2022 Samsung Electronics Co. Ltd.
+ *	Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
+ */
+
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include <sound/initval.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/soc-dai.h>
+#include <sound/simple_card_utils.h>
+
+#include "i2s.h"
+#include "i2s-regs.h"
+
+#define FSD_PREFIX		"tesla,"
+#define FSD_DAI_SRC_PCLK	3
+#define FSD_DAI_RFS_192		192
+#define FSD_DAI_BFS_48		48
+#define FSD_DAI_BFS_96		96
+#define FSD_DAI_BFS_192		192
+
+struct fsd_card_priv {
+	struct snd_soc_card card;
+	struct snd_soc_dai_link *dai_link;
+	u32 tdm_slots;
+	u32 tdm_slot_width;
+};
+
+static unsigned int fsd_card_get_rfs(unsigned int rate)
+{
+	return FSD_DAI_RFS_192;
+}
+
+static unsigned int fsd_card_get_bfs(unsigned int channels)
+{
+	switch (channels) {
+	case 1:
+	case 2:
+		return FSD_DAI_BFS_48;
+	case 3:
+	case 4:
+		return FSD_DAI_BFS_96;
+	case 5:
+	case 6:
+	case 7:
+	case 8:
+		return FSD_DAI_BFS_192;
+	default:
+		return FSD_DAI_BFS_48;
+	}
+}
+
+static unsigned int fsd_card_get_psr(unsigned int rate)
+{
+	switch (rate) {
+	case 8000:	return 43;
+	case 11025:	return 31;
+	case 16000:	return 21;
+	case 22050:	return 16;
+	case 32000:	return 11;
+	case 44100:	return 8;
+	case 48000:	return 7;
+	case 64000:	return 5;
+	case 88200:	return 4;
+	case 96000:	return 4;
+	case 192000:	return 2;
+	default:	return 1;
+	}
+}
+
+static int fsd_card_hw_params(struct snd_pcm_substream *substream,
+					struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd	= substream->private_data;
+	struct snd_soc_card *card	= rtd->card;
+	struct snd_soc_dai *cpu_dai	= asoc_rtd_to_cpu(rtd, 0);
+	struct snd_soc_dai_link *link	= rtd->dai_link;
+	struct fsd_card_priv *priv	= snd_soc_card_get_drvdata(card);
+	unsigned int rfs, bfs, psr;
+	int ret = 0, cdclk_dir;
+
+	rfs = fsd_card_get_rfs(params_rate(params));
+	bfs = fsd_card_get_bfs(params_channels(params));
+	psr = fsd_card_get_psr(params_rate(params));
+
+	/* Configure the Root Clock Source */
+	ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_OPCLK,
+					false, FSD_DAI_SRC_PCLK);
+	if (ret < 0) {
+		dev_err(card->dev, "Failed to set OPCLK: %d\n", ret);
+		goto err;
+	}
+
+	ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_RCLKSRC_0,
+					false, false);
+	if (ret < 0) {
+		dev_err(card->dev, "Failed to set RCLKSRC: %d\n", ret);
+		goto err;
+	}
+
+	/* Set CPU DAI configuration */
+	ret = snd_soc_dai_set_fmt(cpu_dai, link->dai_fmt);
+	if (ret < 0) {
+		dev_err(card->dev, "Failed to set DAIFMT: %d\n", ret);
+		goto err;
+	}
+
+	if (link->dai_fmt & SND_SOC_DAIFMT_CBC_CFC) {
+		cdclk_dir = SND_SOC_CLOCK_OUT;
+	} else if (link->dai_fmt & SND_SOC_DAIFMT_CBP_CFP) {
+		cdclk_dir = SND_SOC_CLOCK_IN;
+	} else {
+		dev_err(card->dev, "Missing Clock Master information\n");
+		goto err;
+	}
+
+	/* Set Clock Source for CDCLK */
+	ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_CDCLK,
+					rfs, cdclk_dir);
+	if (ret < 0) {
+		dev_err(card->dev, "Failed to set CDCLK: %d\n", ret);
+		goto err;
+	}
+
+	ret = snd_soc_dai_set_clkdiv(cpu_dai, SAMSUNG_I2S_DIV_RCLK, psr);
+	if (ret < 0) {
+		dev_err(card->dev, "Failed to set PSR: %d\n", ret);
+		goto err;
+	}
+
+	ret = snd_soc_dai_set_clkdiv(cpu_dai, SAMSUNG_I2S_DIV_BCLK, bfs);
+	if (ret < 0) {
+		dev_err(card->dev, "Failed to set BCLK: %d\n", ret);
+		goto err;
+	}
+
+	if (priv->tdm_slots) {
+		ret = snd_soc_dai_set_tdm_slot(cpu_dai, false, false,
+				priv->tdm_slots, priv->tdm_slot_width);
+		if (ret < 0) {
+			dev_err(card->dev,
+				"Failed to configure in TDM mode:%d\n", ret);
+			goto err;
+		}
+	}
+
+err:
+	return ret;
+}
+
+static const struct snd_soc_ops fsd_card_ops = {
+	.hw_params	= fsd_card_hw_params,
+};
+
+static struct fsd_card_priv *fsd_card_parse_of(struct snd_soc_card *card)
+{
+	struct fsd_card_priv *priv;
+	struct snd_soc_dai_link *link;
+	struct device *dev = card->dev;
+	struct device_node *node = dev->of_node;
+	struct device_node *np, *cpu_node, *codec_node;
+	struct snd_soc_dai_link_component *dlc;
+	int ret, id = 0, num_links;
+
+	ret = snd_soc_of_parse_card_name(card, "model");
+	if (ret) {
+		dev_err(dev, "Error parsing card name: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	if (of_property_read_bool(dev->of_node, "widgets")) {
+		ret = snd_soc_of_parse_audio_simple_widgets(card, "widgets");
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
+	/* Add DAPM routes to the card */
+	if (of_property_read_bool(node, "audio-routing")) {
+		ret = snd_soc_of_parse_audio_routing(card, "audio-routing");
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
+	num_links = of_get_child_count(node);
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return ERR_PTR(-ENOMEM);
+
+	priv->dai_link = devm_kzalloc(dev, num_links * sizeof(*priv->dai_link),
+								GFP_KERNEL);
+	if (!priv->dai_link)
+		return ERR_PTR(-ENOMEM);
+
+	priv->tdm_slots = 0;
+	priv->tdm_slot_width = 0;
+
+	snd_soc_of_parse_tdm_slot(node, NULL, NULL, &priv->tdm_slots,
+			&priv->tdm_slot_width);
+
+	link = priv->dai_link;
+
+	for_each_child_of_node(node, np) {
+		dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
+		if (!dlc)
+			return ERR_PTR(-ENOMEM);
+
+		link->id		= id;
+		link->cpus		= &dlc[0];
+		link->platforms		= &dlc[1];
+		link->num_cpus		= 1;
+		link->num_codecs	= 1;
+		link->num_platforms	= 1;
+
+		cpu_node = of_get_child_by_name(np, "cpu");
+		if (!cpu_node) {
+			dev_err(dev, "Missing CPU/Codec node\n");
+			ret = -EINVAL;
+			goto err_cpu_node;
+		}
+
+		ret = snd_soc_of_get_dai_link_cpus(dev, cpu_node, link);
+		if (ret < 0) {
+			dev_err(dev, "Error Parsing CPU DAI name\n");
+			goto err_cpu_name;
+		}
+
+		link->platforms->of_node = link->cpus->of_node;
+
+		codec_node = of_get_child_by_name(np, "codec");
+		if (codec_node) {
+			ret = snd_soc_of_get_dai_link_codecs(dev, codec_node,
+					link);
+			if (ret < 0) {
+				dev_err(dev, "Error Parsing Codec DAI name\n");
+				goto err_codec_name;
+			}
+		} else {
+			dlc = devm_kzalloc(dev, sizeof(*dlc), GFP_KERNEL);
+			if (!dlc) {
+				ret = -ENOMEM;
+				goto err_cpu_name;
+			}
+
+			link->codecs = dlc;
+
+			link->codecs->dai_name = "snd-soc-dummy-dai";
+			link->codecs->name = "snd-soc-dummy";
+			link->dynamic = 1;
+
+			snd_soc_dai_link_set_capabilities(link);
+			link->ignore_suspend = 1;
+			link->nonatomic = 1;
+		}
+
+		ret = asoc_simple_parse_daifmt(dev, np, codec_node,
+					FSD_PREFIX, &link->dai_fmt);
+		if (ret)
+			link->dai_fmt = SND_SOC_DAIFMT_NB_NF |
+					SND_SOC_DAIFMT_CBC_CFC |
+					SND_SOC_DAIFMT_I2S;
+
+		ret = of_property_read_string(np, "link-name", &link->name);
+		if (ret) {
+			dev_err(card->dev, "Error Parsing link name\n");
+			goto err_codec_name;
+		}
+
+		link->stream_name = link->name;
+		link->ops = &fsd_card_ops;
+
+		link++;
+		id++;
+
+		of_node_put(cpu_node);
+		of_node_put(codec_node);
+	}
+
+	card->dai_link = priv->dai_link;
+	card->num_links = num_links;
+
+	return priv;
+
+err_codec_name:
+	of_node_put(codec_node);
+err_cpu_name:
+	of_node_put(cpu_node);
+err_cpu_node:
+	of_node_put(np);
+	return ERR_PTR(ret);
+}
+
+static int fsd_platform_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct snd_soc_card *card;
+	struct fsd_card_priv *fsd_priv;
+
+	card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL);
+	if (!card)
+		return -ENOMEM;
+
+	card->dev	= dev;
+	fsd_priv	= fsd_card_parse_of(card);
+
+	if (IS_ERR(fsd_priv)) {
+		dev_err(&pdev->dev, "Error Parsing DAI Link: %ld\n",
+				PTR_ERR(fsd_priv));
+		return PTR_ERR(fsd_priv);
+	}
+
+	snd_soc_card_set_drvdata(card, fsd_priv);
+
+	return devm_snd_soc_register_card(&pdev->dev, card);
+}
+
+static const struct of_device_id fsd_device_id[] = {
+	{ .compatible = "tesla,fsd-card" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, fsd_device_id);
+
+static struct platform_driver fsd_platform_driver = {
+	.driver = {
+		.name = "fsd-card",
+		.of_match_table = of_match_ptr(fsd_device_id),
+	},
+	.probe = fsd_platform_probe,
+};
+module_platform_driver(fsd_platform_driver);
+
+MODULE_AUTHOR("Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>");
+MODULE_DESCRIPTION("FSD ASoC Soundcard Driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH 4/6] ASoC: samsung: fsd: Add FSD soundcard driver

[Mark Brown]

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

On Fri, Oct 14, 2022 at 03:51:49PM +0530, Padmanabhan Rajanbabu wrote:

> +++ b/sound/soc/samsung/fsd-card.c
> @@ -0,0 +1,349 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ALSA SoC Audio Layer - FSD Soundcard driver
> + *
> + * Copyright (c) 2022 Samsung Electronics Co. Ltd.
> + *	Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>

Please make the entire comment a C++ one so things look more
intentional.

> +	if (link->dai_fmt & SND_SOC_DAIFMT_CBC_CFC) {
> +		cdclk_dir = SND_SOC_CLOCK_OUT;
> +	} else if (link->dai_fmt & SND_SOC_DAIFMT_CBP_CFP) {
> +		cdclk_dir = SND_SOC_CLOCK_IN;
> +	} else {
> +		dev_err(card->dev, "Missing Clock Master information\n");
> +		goto err;
> +	}

We're trying to modernise the langauge around clock providers, please
use that term rather than the outdated terminology here.

> +	if (priv->tdm_slots) {
> +		ret = snd_soc_dai_set_tdm_slot(cpu_dai, false, false,
> +				priv->tdm_slots, priv->tdm_slot_width);
> +		if (ret < 0) {
> +			dev_err(card->dev,
> +				"Failed to configure in TDM mode:%d\n", ret);
> +			goto err;
> +		}
> +	}

Just set things once on probe if they don't depend on the configuration,
it's neater and marginally faster if nothing else.

> +	if (of_property_read_bool(dev->of_node, "widgets")) {
> +		ret = snd_soc_of_parse_audio_simple_widgets(card, "widgets");
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
> +	/* Add DAPM routes to the card */
> +	if (of_property_read_bool(node, "audio-routing")) {
> +		ret = snd_soc_of_parse_audio_routing(card, "audio-routing");
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}

Just fix the library functions to handle missing properties gracefully,
every card is going to need the same code here.

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

RE: [PATCH 4/6] ASoC: samsung: fsd: Add FSD soundcard driver

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: 14 October 2022 05:53 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> Cc: lgirdwood@gmail.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com; alsa-devel@alsa-project.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org
> Subject: Re: [PATCH 4/6] ASoC: samsung: fsd: Add FSD soundcard driver
> 
> On Fri, Oct 14, 2022 at 03:51:49PM +0530, Padmanabhan Rajanbabu wrote:
> 
> > +++ b/sound/soc/samsung/fsd-card.c
> > @@ -0,0 +1,349 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ALSA SoC Audio Layer - FSD Soundcard driver
> > + *
> > + * Copyright (c) 2022 Samsung Electronics Co. Ltd.
> > + *	Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> 
> Please make the entire comment a C++ one so things look more intentional.
okay
> 
> > +	if (link->dai_fmt & SND_SOC_DAIFMT_CBC_CFC) {
> > +		cdclk_dir = SND_SOC_CLOCK_OUT;
> > +	} else if (link->dai_fmt & SND_SOC_DAIFMT_CBP_CFP) {
> > +		cdclk_dir = SND_SOC_CLOCK_IN;
> > +	} else {
> > +		dev_err(card->dev, "Missing Clock Master information\n");
> > +		goto err;
> > +	}
> 
> We're trying to modernise the langauge around clock providers, please use
> that term rather than the outdated terminology here.
Okay, I'll make the necessary change for the clock terminologies in the next
patch set
> 
> > +	if (priv->tdm_slots) {
> > +		ret = snd_soc_dai_set_tdm_slot(cpu_dai, false, false,
> > +				priv->tdm_slots, priv->tdm_slot_width);
> > +		if (ret < 0) {
> > +			dev_err(card->dev,
> > +				"Failed to configure in TDM mode:%d\n",
> ret);
> > +			goto err;
> > +		}
> > +	}
> 
> Just set things once on probe if they don't depend on the configuration,
it's
> neater and marginally faster if nothing else.
Okay
> 
> > +	if (of_property_read_bool(dev->of_node, "widgets")) {
> > +		ret = snd_soc_of_parse_audio_simple_widgets(card,
> "widgets");
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +	}
> > +
> > +	/* Add DAPM routes to the card */
> > +	if (of_property_read_bool(node, "audio-routing")) {
> > +		ret = snd_soc_of_parse_audio_routing(card, "audio-
> routing");
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +	}
> 
> Just fix the library functions to handle missing properties gracefully,
every
> card is going to need the same code here.
Okay

Thank you for reviewing the patch

Re: [PATCH 4/6] ASoC: samsung: fsd: Add FSD soundcard driver

[Krzysztof Kozlowski]

On 14/10/2022 06:21, Padmanabhan Rajanbabu wrote:
> Add a soundcard driver for FSD audio interface to bridge the
> CPU DAI of samsung I2S with the codec and platform driver.
> 

Thank you for your patch. There is something to discuss/improve.

> +
> +#define FSD_PREFIX		"tesla,"
> +#define FSD_DAI_SRC_PCLK	3
> +#define FSD_DAI_RFS_192		192
> +#define FSD_DAI_BFS_48		48
> +#define FSD_DAI_BFS_96		96
> +#define FSD_DAI_BFS_192		192
> +
> +struct fsd_card_priv {
> +	struct snd_soc_card card;
> +	struct snd_soc_dai_link *dai_link;
> +	u32 tdm_slots;
> +	u32 tdm_slot_width;
> +};
> +
> +static unsigned int fsd_card_get_rfs(unsigned int rate)
> +{
> +	return FSD_DAI_RFS_192;

This wrapper is a bit silly...

> +}
> +
> +static unsigned int fsd_card_get_bfs(unsigned int channels)
> +{
> +	switch (channels) {
> +	case 1:
> +	case 2:
> +		return FSD_DAI_BFS_48;
> +	case 3:
> +	case 4:
> +		return FSD_DAI_BFS_96;
> +	case 5:
> +	case 6:
> +	case 7:
> +	case 8:
> +		return FSD_DAI_BFS_192;
> +	default:
> +		return FSD_DAI_BFS_48;
> +	}
> +}
> +
> +static unsigned int fsd_card_get_psr(unsigned int rate)
> +{
> +	switch (rate) {
> +	case 8000:	return 43;
> +	case 11025:	return 31;
> +	case 16000:	return 21;
> +	case 22050:	return 16;
> +	case 32000:	return 11;
> +	case 44100:	return 8;
> +	case 48000:	return 7;
> +	case 64000:	return 5;
> +	case 88200:	return 4;
> +	case 96000:	return 4;
> +	case 192000:	return 2;
> +	default:	return 1;
> +	}
> +}
> +
> +static int fsd_card_hw_params(struct snd_pcm_substream *substream,
> +					struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd	= substream->private_data;
> +	struct snd_soc_card *card	= rtd->card;
> +	struct snd_soc_dai *cpu_dai	= asoc_rtd_to_cpu(rtd, 0);
> +	struct snd_soc_dai_link *link	= rtd->dai_link;
> +	struct fsd_card_priv *priv	= snd_soc_card_get_drvdata(card);
> +	unsigned int rfs, bfs, psr;
> +	int ret = 0, cdclk_dir;
> +
> +	rfs = fsd_card_get_rfs(params_rate(params));
> +	bfs = fsd_card_get_bfs(params_channels(params));
> +	psr = fsd_card_get_psr(params_rate(params));
> +
> +	/* Configure the Root Clock Source */
> +	ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_OPCLK,
> +					false, FSD_DAI_SRC_PCLK);
> +	if (ret < 0) {
> +		dev_err(card->dev, "Failed to set OPCLK: %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_RCLKSRC_0,
> +					false, false);
> +	if (ret < 0) {
> +		dev_err(card->dev, "Failed to set RCLKSRC: %d\n", ret);

Don't you need to cleanup on error paths?

> +		goto err;
> +	}
> +
> +	/* Set CPU DAI configuration */
> +	ret = snd_soc_dai_set_fmt(cpu_dai, link->dai_fmt);
> +	if (ret < 0) {
> +		dev_err(card->dev, "Failed to set DAIFMT: %d\n", ret);
> +		goto err;
> +	}
> +
> +	if (link->dai_fmt & SND_SOC_DAIFMT_CBC_CFC) {
> +		cdclk_dir = SND_SOC_CLOCK_OUT;
> +	} else if (link->dai_fmt & SND_SOC_DAIFMT_CBP_CFP) {
> +		cdclk_dir = SND_SOC_CLOCK_IN;
> +	} else {
> +		dev_err(card->dev, "Missing Clock Master information\n");
> +		goto err;
> +	}
> +
> +	/* Set Clock Source for CDCLK */
> +	ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_CDCLK,
> +					rfs, cdclk_dir);
> +	if (ret < 0) {
> +		dev_err(card->dev, "Failed to set CDCLK: %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = snd_soc_dai_set_clkdiv(cpu_dai, SAMSUNG_I2S_DIV_RCLK, psr);
> +	if (ret < 0) {
> +		dev_err(card->dev, "Failed to set PSR: %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = snd_soc_dai_set_clkdiv(cpu_dai, SAMSUNG_I2S_DIV_BCLK, bfs);
> +	if (ret < 0) {
> +		dev_err(card->dev, "Failed to set BCLK: %d\n", ret);
> +		goto err;
> +	}
> +
> +	if (priv->tdm_slots) {
> +		ret = snd_soc_dai_set_tdm_slot(cpu_dai, false, false,
> +				priv->tdm_slots, priv->tdm_slot_width);
> +		if (ret < 0) {
> +			dev_err(card->dev,
> +				"Failed to configure in TDM mode:%d\n", ret);
> +			goto err;
> +		}
> +	}
> +
> +err:
> +	return ret;
> +}
> +
> +static const struct snd_soc_ops fsd_card_ops = {
> +	.hw_params	= fsd_card_hw_params,
> +};
> +
> +static struct fsd_card_priv *fsd_card_parse_of(struct snd_soc_card *card)
> +{
> +	struct fsd_card_priv *priv;
> +	struct snd_soc_dai_link *link;
> +	struct device *dev = card->dev;
> +	struct device_node *node = dev->of_node;
> +	struct device_node *np, *cpu_node, *codec_node;
> +	struct snd_soc_dai_link_component *dlc;
> +	int ret, id = 0, num_links;
> +
> +	ret = snd_soc_of_parse_card_name(card, "model");
> +	if (ret) {
> +		dev_err(dev, "Error parsing card name: %d\n", ret);
> +		return ERR_PTR(ret);

return dev_err_probe

> +	}
> +
> +	if (of_property_read_bool(dev->of_node, "widgets")) {
> +		ret = snd_soc_of_parse_audio_simple_widgets(card, "widgets");
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
> +	/* Add DAPM routes to the card */
> +	if (of_property_read_bool(node, "audio-routing")) {
> +		ret = snd_soc_of_parse_audio_routing(card, "audio-routing");
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
> +	num_links = of_get_child_count(node);
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return ERR_PTR(-ENOMEM);
> +
> +	priv->dai_link = devm_kzalloc(dev, num_links * sizeof(*priv->dai_link),
> +								GFP_KERNEL);
> +	if (!priv->dai_link)
> +		return ERR_PTR(-ENOMEM);
> +
> +	priv->tdm_slots = 0;
> +	priv->tdm_slot_width = 0;
> +
> +	snd_soc_of_parse_tdm_slot(node, NULL, NULL, &priv->tdm_slots,
> +			&priv->tdm_slot_width);
> +
> +	link = priv->dai_link;
> +
> +	for_each_child_of_node(node, np) {
> +		dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
> +		if (!dlc)
> +			return ERR_PTR(-ENOMEM);
> +
> +		link->id		= id;
> +		link->cpus		= &dlc[0];
> +		link->platforms		= &dlc[1];
> +		link->num_cpus		= 1;
> +		link->num_codecs	= 1;
> +		link->num_platforms	= 1;
> +
> +		cpu_node = of_get_child_by_name(np, "cpu");
> +		if (!cpu_node) {
> +			dev_err(dev, "Missing CPU/Codec node\n");
> +			ret = -EINVAL;
> +			goto err_cpu_node;
> +		}
> +
> +		ret = snd_soc_of_get_dai_link_cpus(dev, cpu_node, link);
> +		if (ret < 0) {
> +			dev_err(dev, "Error Parsing CPU DAI name\n");
> +			goto err_cpu_name;
> +		}
> +
> +		link->platforms->of_node = link->cpus->of_node;
> +
> +		codec_node = of_get_child_by_name(np, "codec");
> +		if (codec_node) {
> +			ret = snd_soc_of_get_dai_link_codecs(dev, codec_node,
> +					link);
> +			if (ret < 0) {
> +				dev_err(dev, "Error Parsing Codec DAI name\n");
> +				goto err_codec_name;
> +			}
> +		} else {
> +			dlc = devm_kzalloc(dev, sizeof(*dlc), GFP_KERNEL);
> +			if (!dlc) {
> +				ret = -ENOMEM;
> +				goto err_cpu_name;
> +			}
> +
> +			link->codecs = dlc;
> +
> +			link->codecs->dai_name = "snd-soc-dummy-dai";
> +			link->codecs->name = "snd-soc-dummy";
> +			link->dynamic = 1;
> +
> +			snd_soc_dai_link_set_capabilities(link);
> +			link->ignore_suspend = 1;
> +			link->nonatomic = 1;
> +		}
> +
> +		ret = asoc_simple_parse_daifmt(dev, np, codec_node,
> +					FSD_PREFIX, &link->dai_fmt);
> +		if (ret)
> +			link->dai_fmt = SND_SOC_DAIFMT_NB_NF |
> +					SND_SOC_DAIFMT_CBC_CFC |
> +					SND_SOC_DAIFMT_I2S;
> +
> +		ret = of_property_read_string(np, "link-name", &link->name);
> +		if (ret) {
> +			dev_err(card->dev, "Error Parsing link name\n");
> +			goto err_codec_name;
> +		}
> +
> +		link->stream_name = link->name;
> +		link->ops = &fsd_card_ops;
> +
> +		link++;
> +		id++;
> +
> +		of_node_put(cpu_node);
> +		of_node_put(codec_node);
> +	}
> +
> +	card->dai_link = priv->dai_link;
> +	card->num_links = num_links;
> +
> +	return priv;
> +
> +err_codec_name:
> +	of_node_put(codec_node);
> +err_cpu_name:
> +	of_node_put(cpu_node);
> +err_cpu_node:
> +	of_node_put(np);
> +	return ERR_PTR(ret);
> +}
> +
> +static int fsd_platform_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct snd_soc_card *card;
> +	struct fsd_card_priv *fsd_priv;
> +
> +	card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL);
> +	if (!card)
> +		return -ENOMEM;
> +
> +	card->dev	= dev;
> +	fsd_priv	= fsd_card_parse_of(card);

Drop the indentation before =

> +
> +	if (IS_ERR(fsd_priv)) {
> +		dev_err(&pdev->dev, "Error Parsing DAI Link: %ld\n",
> +				PTR_ERR(fsd_priv));
> +		return PTR_ERR(fsd_priv);

return dev_err_probe

> +	}
> +
> +	snd_soc_card_set_drvdata(card, fsd_priv);
> +
> +	return devm_snd_soc_register_card(&pdev->dev, card);
> +}
> +
> +static const struct of_device_id fsd_device_id[] = {
> +	{ .compatible = "tesla,fsd-card" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, fsd_device_id);
> +
> +static struct platform_driver fsd_platform_driver = {
> +	.driver = {
> +		.name = "fsd-card",
> +		.of_match_table = of_match_ptr(fsd_device_id),

of_match_ptr comes with maybe_unused. Or drop it.



Best regards,
Krzysztof

RE: [PATCH 4/6] ASoC: samsung: fsd: Add FSD soundcard driver

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 16 October 2022 08:48 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>;
> lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com
> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
> Subject: Re: [PATCH 4/6] ASoC: samsung: fsd: Add FSD soundcard driver
> 
> On 14/10/2022 06:21, Padmanabhan Rajanbabu wrote:
> > Add a soundcard driver for FSD audio interface to bridge the CPU DAI
> > of samsung I2S with the codec and platform driver.
> >
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +
> > +#define FSD_PREFIX		"tesla,"
> > +#define FSD_DAI_SRC_PCLK	3
> > +#define FSD_DAI_RFS_192		192
> > +#define FSD_DAI_BFS_48		48
> > +#define FSD_DAI_BFS_96		96
> > +#define FSD_DAI_BFS_192		192
> > +
> > +struct fsd_card_priv {
> > +	struct snd_soc_card card;
> > +	struct snd_soc_dai_link *dai_link;
> > +	u32 tdm_slots;
> > +	u32 tdm_slot_width;
> > +};
> > +
> > +static unsigned int fsd_card_get_rfs(unsigned int rate) {
> > +	return FSD_DAI_RFS_192;
> 
> This wrapper is a bit silly...
okay, will remove the wrapper and assign the macro value directly
> 
> > +}
> > +
> > +static unsigned int fsd_card_get_bfs(unsigned int channels) {
> > +	switch (channels) {
> > +	case 1:
> > +	case 2:
> > +		return FSD_DAI_BFS_48;
> > +	case 3:
> > +	case 4:
> > +		return FSD_DAI_BFS_96;
> > +	case 5:
> > +	case 6:
> > +	case 7:
> > +	case 8:
> > +		return FSD_DAI_BFS_192;
> > +	default:
> > +		return FSD_DAI_BFS_48;
> > +	}
> > +}
> > +
> > +static unsigned int fsd_card_get_psr(unsigned int rate) {
> > +	switch (rate) {
> > +	case 8000:	return 43;
> > +	case 11025:	return 31;
> > +	case 16000:	return 21;
> > +	case 22050:	return 16;
> > +	case 32000:	return 11;
> > +	case 44100:	return 8;
> > +	case 48000:	return 7;
> > +	case 64000:	return 5;
> > +	case 88200:	return 4;
> > +	case 96000:	return 4;
> > +	case 192000:	return 2;
> > +	default:	return 1;
> > +	}
> > +}
> > +
> > +static int fsd_card_hw_params(struct snd_pcm_substream *substream,
> > +					struct snd_pcm_hw_params
> *params) {
> > +	struct snd_soc_pcm_runtime *rtd	= substream->private_data;
> > +	struct snd_soc_card *card	= rtd->card;
> > +	struct snd_soc_dai *cpu_dai	= asoc_rtd_to_cpu(rtd, 0);
> > +	struct snd_soc_dai_link *link	= rtd->dai_link;
> > +	struct fsd_card_priv *priv	= snd_soc_card_get_drvdata(card);
> > +	unsigned int rfs, bfs, psr;
> > +	int ret = 0, cdclk_dir;
> > +
> > +	rfs = fsd_card_get_rfs(params_rate(params));
> > +	bfs = fsd_card_get_bfs(params_channels(params));
> > +	psr = fsd_card_get_psr(params_rate(params));
> > +
> > +	/* Configure the Root Clock Source */
> > +	ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_OPCLK,
> > +					false, FSD_DAI_SRC_PCLK);
> > +	if (ret < 0) {
> > +		dev_err(card->dev, "Failed to set OPCLK: %d\n", ret);
> > +		goto err;
> > +	}
> > +
> > +	ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_RCLKSRC_0,
> > +					false, false);
> > +	if (ret < 0) {
> > +		dev_err(card->dev, "Failed to set RCLKSRC: %d\n", ret);
> 
> Don't you need to cleanup on error paths?
we might not be needing, since the sound card neither configures any
clock sources directly nor involves in any memory allocation during
hw_params.
> 
> > +		goto err;
> > +	}
> > +
> > +	/* Set CPU DAI configuration */
> > +	ret = snd_soc_dai_set_fmt(cpu_dai, link->dai_fmt);
> > +	if (ret < 0) {
> > +		dev_err(card->dev, "Failed to set DAIFMT: %d\n", ret);
> > +		goto err;
> > +	}
> > +
> > +	if (link->dai_fmt & SND_SOC_DAIFMT_CBC_CFC) {
> > +		cdclk_dir = SND_SOC_CLOCK_OUT;
> > +	} else if (link->dai_fmt & SND_SOC_DAIFMT_CBP_CFP) {
> > +		cdclk_dir = SND_SOC_CLOCK_IN;
> > +	} else {
> > +		dev_err(card->dev, "Missing Clock Master information\n");
> > +		goto err;
> > +	}
> > +
> > +	/* Set Clock Source for CDCLK */
> > +	ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_CDCLK,
> > +					rfs, cdclk_dir);
> > +	if (ret < 0) {
> > +		dev_err(card->dev, "Failed to set CDCLK: %d\n", ret);
> > +		goto err;
> > +	}
> > +
> > +	ret = snd_soc_dai_set_clkdiv(cpu_dai, SAMSUNG_I2S_DIV_RCLK, psr);
> > +	if (ret < 0) {
> > +		dev_err(card->dev, "Failed to set PSR: %d\n", ret);
> > +		goto err;
> > +	}
> > +
> > +	ret = snd_soc_dai_set_clkdiv(cpu_dai, SAMSUNG_I2S_DIV_BCLK, bfs);
> > +	if (ret < 0) {
> > +		dev_err(card->dev, "Failed to set BCLK: %d\n", ret);
> > +		goto err;
> > +	}
> > +
> > +	if (priv->tdm_slots) {
> > +		ret = snd_soc_dai_set_tdm_slot(cpu_dai, false, false,
> > +				priv->tdm_slots, priv->tdm_slot_width);
> > +		if (ret < 0) {
> > +			dev_err(card->dev,
> > +				"Failed to configure in TDM mode:%d\n", ret);
> > +			goto err;
> > +		}
> > +	}
> > +
> > +err:
> > +	return ret;
> > +}
> > +
> > +static const struct snd_soc_ops fsd_card_ops = {
> > +	.hw_params	= fsd_card_hw_params,
> > +};
> > +
> > +static struct fsd_card_priv *fsd_card_parse_of(struct snd_soc_card
> > +*card) {
> > +	struct fsd_card_priv *priv;
> > +	struct snd_soc_dai_link *link;
> > +	struct device *dev = card->dev;
> > +	struct device_node *node = dev->of_node;
> > +	struct device_node *np, *cpu_node, *codec_node;
> > +	struct snd_soc_dai_link_component *dlc;
> > +	int ret, id = 0, num_links;
> > +
> > +	ret = snd_soc_of_parse_card_name(card, "model");
> > +	if (ret) {
> > +		dev_err(dev, "Error parsing card name: %d\n", ret);
> > +		return ERR_PTR(ret);
> 
> return dev_err_probe
Okay
> 
> > +	}
> > +
> > +	if (of_property_read_bool(dev->of_node, "widgets")) {
> > +		ret = snd_soc_of_parse_audio_simple_widgets(card, "widgets");
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +	}
> > +
> > +	/* Add DAPM routes to the card */
> > +	if (of_property_read_bool(node, "audio-routing")) {
> > +		ret = snd_soc_of_parse_audio_routing(card, "audio-routing");
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +	}
> > +
> > +	num_links = of_get_child_count(node);
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	priv->dai_link = devm_kzalloc(dev, num_links * sizeof(*priv-
> >dai_link),
> > + 	GFP_KERNEL);
> > +	if (!priv->dai_link)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	priv->tdm_slots = 0;
> > +	priv->tdm_slot_width = 0;
> > +
> > +	snd_soc_of_parse_tdm_slot(node, NULL, NULL, &priv->tdm_slots,
> > +			&priv->tdm_slot_width);
> > +
> > +	link = priv->dai_link;
> > +
> > +	for_each_child_of_node(node, np) {
> > +		dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
> > +		if (!dlc)
> > +			return ERR_PTR(-ENOMEM);
> > +
> > +		link->id		= id;
> > +		link->cpus		= &dlc[0];
> > +		link->platforms		= &dlc[1];
> > +		link->num_cpus		= 1;
> > +		link->num_codecs	= 1;
> > +		link->num_platforms	= 1;
> > +
> > +		cpu_node = of_get_child_by_name(np, "cpu");
> > +		if (!cpu_node) {
> > +			dev_err(dev, "Missing CPU/Codec node\n");
> > +			ret = -EINVAL;
> > +			goto err_cpu_node;
> > +		}
> > +
> > +		ret = snd_soc_of_get_dai_link_cpus(dev, cpu_node, link);
> > +		if (ret < 0) {
> > +			dev_err(dev, "Error Parsing CPU DAI name\n");
> > +			goto err_cpu_name;
> > +		}
> > +
> > +		link->platforms->of_node = link->cpus->of_node;
> > +
> > +		codec_node = of_get_child_by_name(np, "codec");
> > +		if (codec_node) {
> > +			ret = snd_soc_of_get_dai_link_codecs(dev, codec_node,
> > +					link);
> > +			if (ret < 0) {
> > +				dev_err(dev, "Error Parsing Codec DAI name\n");
> > +				goto err_codec_name;
> > +			}
> > +		} else {
> > +			dlc = devm_kzalloc(dev, sizeof(*dlc), GFP_KERNEL);
> > +			if (!dlc) {
> > +				ret = -ENOMEM;
> > +				goto err_cpu_name;
> > +			}
> > +
> > +			link->codecs = dlc;
> > +
> > +			link->codecs->dai_name = "snd-soc-dummy-dai";
> > +			link->codecs->name = "snd-soc-dummy";
> > +			link->dynamic = 1;
> > +
> > +			snd_soc_dai_link_set_capabilities(link);
> > +			link->ignore_suspend = 1;
> > +			link->nonatomic = 1;
> > +		}
> > +
> > +		ret = asoc_simple_parse_daifmt(dev, np, codec_node,
> > +					FSD_PREFIX, &link->dai_fmt);
> > +		if (ret)
> > +			link->dai_fmt = SND_SOC_DAIFMT_NB_NF |
> > +					SND_SOC_DAIFMT_CBC_CFC |
> > +					SND_SOC_DAIFMT_I2S;
> > +
> > +		ret = of_property_read_string(np, "link-name", &link-
> >name);
> > +		if (ret) {
> > +			dev_err(card->dev, "Error Parsing link name\n");
> > +			goto err_codec_name;
> > +		}
> > +
> > +		link->stream_name = link->name;
> > +		link->ops = &fsd_card_ops;
> > +
> > +		link++;
> > +		id++;
> > +
> > +		of_node_put(cpu_node);
> > +		of_node_put(codec_node);
> > +	}
> > +
> > +	card->dai_link = priv->dai_link;
> > +	card->num_links = num_links;
> > +
> > +	return priv;
> > +
> > +err_codec_name:
> > +	of_node_put(codec_node);
> > +err_cpu_name:
> > +	of_node_put(cpu_node);
> > +err_cpu_node:
> > +	of_node_put(np);
> > +	return ERR_PTR(ret);
> > +}
> > +
> > +static int fsd_platform_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct snd_soc_card *card;
> > +	struct fsd_card_priv *fsd_priv;
> > +
> > +	card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL);
> > +	if (!card)
> > +		return -ENOMEM;
> > +
> > +	card->dev	= dev;
> > +	fsd_priv	= fsd_card_parse_of(card);
> 
> Drop the indentation before =
Okay
> 
> > +
> > +	if (IS_ERR(fsd_priv)) {
> > +		dev_err(&pdev->dev, "Error Parsing DAI Link: %ld\n",
> > +				PTR_ERR(fsd_priv));
> > +		return PTR_ERR(fsd_priv);
> 
> return dev_err_probe
Okay
> 
> > +	}
> > +
> > +	snd_soc_card_set_drvdata(card, fsd_priv);
> > +
> > +	return devm_snd_soc_register_card(&pdev->dev, card); }
> > +
> > +static const struct of_device_id fsd_device_id[] = {
> > +	{ .compatible = "tesla,fsd-card" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, fsd_device_id);
> > +
> > +static struct platform_driver fsd_platform_driver = {
> > +	.driver = {
> > +		.name = "fsd-card",
> > +		.of_match_table = of_match_ptr(fsd_device_id),
> 
> of_match_ptr comes with maybe_unused. Or drop it.
Okay
> 
> 
> 
> Best regards,
> Krzysztof
Thank you for reviewing the patch.

[PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD

[Padmanabhan Rajanbabu]

Add device tree node for I2S0 and I2S1 CPU DAI instances for Tesla
FSD board

Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
---
 arch/arm64/boot/dts/tesla/fsd-evb.dts      |  8 +++++
 arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 14 ++++++++
 arch/arm64/boot/dts/tesla/fsd.dtsi         | 38 ++++++++++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts b/arch/arm64/boot/dts/tesla/fsd-evb.dts
index 1db6ddf03f01..c0a4509499ab 100644
--- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
+++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
@@ -41,3 +41,11 @@
 &ufs {
 	status = "okay";
 };
+
+&tdm_0 {
+	status = "okay";
+};
+
+&tdm_1 {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
index e3852c946352..ff6f5d4b16dd 100644
--- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
@@ -339,6 +339,20 @@
 		samsung,pin-pud = <FSD_PIN_PULL_UP>;
 		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
 	};
+
+	i2s0_bus: i2s0-bus {
+		samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2", "gpd1-3", "gpd1-4";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
+	};
+
+	i2s1_bus: i2s1-bus {
+		samsung,pins = "gpd2-0", "gpd2-1", "gpd2-2", "gpd2-3", "gpd2-4";
+		samsung,pin-function = <FSD_PIN_FUNC_2>;
+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
+	};
 };
 
 &pinctrl_pmu {
diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
index f35bc5a288c2..5decad45a1b6 100644
--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
@@ -32,6 +32,8 @@
 		spi0 = &spi_0;
 		spi1 = &spi_1;
 		spi2 = &spi_2;
+		tdm0 = &tdm_0;
+		tdm1 = &tdm_1;
 	};
 
 	cpus {
@@ -809,6 +811,42 @@
 			status = "disabled";
 		};
 
+		tdm_0: tdm@140e0000 {
+			compatible = "samsung,exynos7-i2s";
+			reg = <0x0 0x140E0000 0x0 0x100>;
+			interrupts = <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>;
+			dmas = <&pdma1 14>, <&pdma1 13>, <&pdma1 12>;
+			dma-names = "tx", "rx", "tx-sec";
+			#clock-cells = <1>;
+			#sound-dai-cells = <1>;
+			clocks = <&clock_peric PERIC_HCLK_TDM0>,
+				 <&clock_peric PERIC_HCLK_TDM0>,
+				 <&clock_peric PERIC_PCLK_TDM0>;
+			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2s0_bus>;
+			samsung,sec-dai-id = <0>;
+			status = "disabled";
+		};
+
+		tdm_1: tdm@140f0000 {
+			compatible = "samsung,exynos7-i2s";
+			reg = <0x0 0x140F0000 0x0 0x100>;
+			interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>;
+			dmas = <&pdma1 17>, <&pdma1 16>, <&pdma1 15>;
+			dma-names = "tx", "rx", "tx-sec";
+			#clock-cells = <1>;
+			#sound-dai-cells = <1>;
+			clocks = <&clock_peric PERIC_HCLK_TDM1>,
+				 <&clock_peric PERIC_HCLK_TDM1>,
+				 <&clock_peric PERIC_PCLK_TDM1>;
+			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
+			pinctrl-names = "default";
+			pinctrl-0 = <&i2s1_bus>;
+			samsung,sec-dai-id = <1>;
+			status = "disabled";
+		};
+
 		timer@10040000 {
 			compatible = "tesla,fsd-mct", "samsung,exynos4210-mct";
 			reg = <0x0 0x10040000 0x0 0x800>;
-- 
2.17.1

RE: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD

[Alim Akhtar]

>-----Original Message-----
>From: Padmanabhan Rajanbabu [mailto:p.rajanbabu@samsung.com]
>Sent: Friday, October 14, 2022 3:52 PM
>To: lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
>krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
>perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
>alim.akhtar@samsung.com; rcsekar@samsung.com;
>aswani.reddy@samsung.com
>Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; Padmanabhan
>Rajanbabu <p.rajanbabu@samsung.com>
>Subject: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD
>
>Add device tree node for I2S0 and I2S1 CPU DAI instances for Tesla FSD board
>
>Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
>---
> arch/arm64/boot/dts/tesla/fsd-evb.dts      |  8 +++++
> arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 14 ++++++++
> arch/arm64/boot/dts/tesla/fsd.dtsi         | 38 ++++++++++++++++++++++
> 3 files changed, 60 insertions(+)
>
>diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts
>b/arch/arm64/boot/dts/tesla/fsd-evb.dts
>index 1db6ddf03f01..c0a4509499ab 100644
>--- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
>+++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
>@@ -41,3 +41,11 @@
> &ufs {
> 	status = "okay";
> };
>+
>+&tdm_0 {
>+	status = "okay";
>+};
>+
>+&tdm_1 {
>+	status = "okay";
>+};
>diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
>b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
>index e3852c946352..ff6f5d4b16dd 100644
>--- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
>+++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
>@@ -339,6 +339,20 @@
> 		samsung,pin-pud = <FSD_PIN_PULL_UP>;
> 		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> 	};
>+
>+	i2s0_bus: i2s0-bus {
>+		samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2", "gpd1-3",
>"gpd1-4";
>+		samsung,pin-function = <FSD_PIN_FUNC_2>;
>+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
>+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
>+	};
>+
>+	i2s1_bus: i2s1-bus {
>+		samsung,pins = "gpd2-0", "gpd2-1", "gpd2-2", "gpd2-3",
>"gpd2-4";
>+		samsung,pin-function = <FSD_PIN_FUNC_2>;
>+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
>+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
>+	};
> };
>
> &pinctrl_pmu {
>diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi
>b/arch/arm64/boot/dts/tesla/fsd.dtsi
>index f35bc5a288c2..5decad45a1b6 100644
>--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
>+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
>@@ -32,6 +32,8 @@
> 		spi0 = &spi_0;
> 		spi1 = &spi_1;
> 		spi2 = &spi_2;
>+		tdm0 = &tdm_0;
>+		tdm1 = &tdm_1;
> 	};
>
> 	cpus {
>@@ -809,6 +811,42 @@
> 			status = "disabled";
> 		};
>
>+		tdm_0: tdm@140e0000 {
>+			compatible = "samsung,exynos7-i2s";
>+			reg = <0x0 0x140E0000 0x0 0x100>;
Address should be all in small caps
Make sure you have run 'make dtbs_check' 

>+			interrupts = <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>;
>+			dmas = <&pdma1 14>, <&pdma1 13>, <&pdma1 12>;
>+			dma-names = "tx", "rx", "tx-sec";
>+			#clock-cells = <1>;
>+			#sound-dai-cells = <1>;
>+			clocks = <&clock_peric PERIC_HCLK_TDM0>,
>+				 <&clock_peric PERIC_HCLK_TDM0>,
>+				 <&clock_peric PERIC_PCLK_TDM0>;
>+			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
>+			pinctrl-names = "default";
>+			pinctrl-0 = <&i2s0_bus>;
>+			samsung,sec-dai-id = <0>;
>+			status = "disabled";
>+		};
>+
>+		tdm_1: tdm@140f0000 {
>+			compatible = "samsung,exynos7-i2s";
>+			reg = <0x0 0x140F0000 0x0 0x100>;
Same as above

>+			interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>;
>+			dmas = <&pdma1 17>, <&pdma1 16>, <&pdma1 15>;
>+			dma-names = "tx", "rx", "tx-sec";
>+			#clock-cells = <1>;
>+			#sound-dai-cells = <1>;
>+			clocks = <&clock_peric PERIC_HCLK_TDM1>,
>+				 <&clock_peric PERIC_HCLK_TDM1>,
>+				 <&clock_peric PERIC_PCLK_TDM1>;
>+			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
>+			pinctrl-names = "default";
>+			pinctrl-0 = <&i2s1_bus>;
>+			samsung,sec-dai-id = <1>;
>+			status = "disabled";
>+		};
>+
> 		timer@10040000 {
> 			compatible = "tesla,fsd-mct", "samsung,exynos4210-
>mct";
> 			reg = <0x0 0x10040000 0x0 0x800>;
>--
>2.17.1

RE: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Alim Akhtar [mailto:alim.akhtar@samsung.com]
> Sent: 14 October 2022 06:54 PM
> To: 'Padmanabhan Rajanbabu' <p.rajanbabu@samsung.com>;
> lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> rcsekar@samsung.com; aswani.reddy@samsung.com
> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
> Subject: RE: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD
> 
> 
> 
> >-----Original Message-----
> >From: Padmanabhan Rajanbabu [mailto:p.rajanbabu@samsung.com]
> >Sent: Friday, October 14, 2022 3:52 PM
> >To: lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> >krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> >perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> >alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com
> >Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> >kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org;
> Padmanabhan
> >Rajanbabu <p.rajanbabu@samsung.com>
> >Subject: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD
> >
> >Add device tree node for I2S0 and I2S1 CPU DAI instances for Tesla FSD
> >board
> >
> >Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> >---
> > arch/arm64/boot/dts/tesla/fsd-evb.dts      |  8 +++++
> > arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 14 ++++++++
> > arch/arm64/boot/dts/tesla/fsd.dtsi         | 38 ++++++++++++++++++++++
> > 3 files changed, 60 insertions(+)
> >
> >diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >index 1db6ddf03f01..c0a4509499ab 100644
> >--- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >+++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >@@ -41,3 +41,11 @@
> > &ufs {
> > 	status = "okay";
> > };
> >+
> >+&tdm_0 {
> >+	status = "okay";
> >+};
> >+
> >+&tdm_1 {
> >+	status = "okay";
> >+};
> >diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> >b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> >index e3852c946352..ff6f5d4b16dd 100644
> >--- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> >+++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> >@@ -339,6 +339,20 @@
> > 		samsung,pin-pud = <FSD_PIN_PULL_UP>;
> > 		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> > 	};
> >+
> >+	i2s0_bus: i2s0-bus {
> >+		samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2", "gpd1-3",
> >"gpd1-4";
> >+		samsung,pin-function = <FSD_PIN_FUNC_2>;
> >+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> >+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> >+	};
> >+
> >+	i2s1_bus: i2s1-bus {
> >+		samsung,pins = "gpd2-0", "gpd2-1", "gpd2-2", "gpd2-3",
> >"gpd2-4";
> >+		samsung,pin-function = <FSD_PIN_FUNC_2>;
> >+		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> >+		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> >+	};
> > };
> >
> > &pinctrl_pmu {
> >diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi
> >b/arch/arm64/boot/dts/tesla/fsd.dtsi
> >index f35bc5a288c2..5decad45a1b6 100644
> >--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> >+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> >@@ -32,6 +32,8 @@
> > 		spi0 = &spi_0;
> > 		spi1 = &spi_1;
> > 		spi2 = &spi_2;
> >+		tdm0 = &tdm_0;
> >+		tdm1 = &tdm_1;
> > 	};
> >
> > 	cpus {
> >@@ -809,6 +811,42 @@
> > 			status = "disabled";
> > 		};
> >
> >+		tdm_0: tdm@140e0000 {
> >+			compatible = "samsung,exynos7-i2s";
> >+			reg = <0x0 0x140E0000 0x0 0x100>;
> Address should be all in small caps
> Make sure you have run 'make dtbs_check'
Okay
> 
> >+			interrupts = <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>;
> >+			dmas = <&pdma1 14>, <&pdma1 13>, <&pdma1 12>;
> >+			dma-names = "tx", "rx", "tx-sec";
> >+			#clock-cells = <1>;
> >+			#sound-dai-cells = <1>;
> >+			clocks = <&clock_peric PERIC_HCLK_TDM0>,
> >+				 <&clock_peric PERIC_HCLK_TDM0>,
> >+				 <&clock_peric PERIC_PCLK_TDM0>;
> >+			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
> >+			pinctrl-names = "default";
> >+			pinctrl-0 = <&i2s0_bus>;
> >+			samsung,sec-dai-id = <0>;
> >+			status = "disabled";
> >+		};
> >+
> >+		tdm_1: tdm@140f0000 {
> >+			compatible = "samsung,exynos7-i2s";
> >+			reg = <0x0 0x140F0000 0x0 0x100>;
> Same as above
Okay
> 
> >+			interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>;
> >+			dmas = <&pdma1 17>, <&pdma1 16>, <&pdma1 15>;
> >+			dma-names = "tx", "rx", "tx-sec";
> >+			#clock-cells = <1>;
> >+			#sound-dai-cells = <1>;
> >+			clocks = <&clock_peric PERIC_HCLK_TDM1>,
> >+				 <&clock_peric PERIC_HCLK_TDM1>,
> >+				 <&clock_peric PERIC_PCLK_TDM1>;
> >+			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
> >+			pinctrl-names = "default";
> >+			pinctrl-0 = <&i2s1_bus>;
> >+			samsung,sec-dai-id = <1>;
> >+			status = "disabled";
> >+		};
> >+
> > 		timer@10040000 {
> > 			compatible = "tesla,fsd-mct", "samsung,exynos4210-
> mct";
> > 			reg = <0x0 0x10040000 0x0 0x800>;
> >--
> >2.17.1
> 

Re: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD

[Krzysztof Kozlowski]

On 14/10/2022 06:21, Padmanabhan Rajanbabu wrote:
> Add device tree node for I2S0 and I2S1 CPU DAI instances for Tesla
> FSD board
> 
> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> ---
>  arch/arm64/boot/dts/tesla/fsd-evb.dts      |  8 +++++
>  arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 14 ++++++++
>  arch/arm64/boot/dts/tesla/fsd.dtsi         | 38 ++++++++++++++++++++++
>  3 files changed, 60 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> index 1db6ddf03f01..c0a4509499ab 100644
> --- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> +++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> @@ -41,3 +41,11 @@
>  &ufs {
>  	status = "okay";
>  };
> +
> +&tdm_0 {

Alphabetical order against other label-overrides.

> +	status = "okay";
> +};
> +
> +&tdm_1 {
> +	status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> index e3852c946352..ff6f5d4b16dd 100644
> --- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> +++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> @@ -339,6 +339,20 @@
>  		samsung,pin-pud = <FSD_PIN_PULL_UP>;
>  		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
>  	};
> +
> +	i2s0_bus: i2s0-bus {

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

> +		samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2", "gpd1-3", "gpd1-4";
> +		samsung,pin-function = <FSD_PIN_FUNC_2>;
> +		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> +		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> +	};
> +
> +	i2s1_bus: i2s1-bus {
> +		samsung,pins = "gpd2-0", "gpd2-1", "gpd2-2", "gpd2-3", "gpd2-4";
> +		samsung,pin-function = <FSD_PIN_FUNC_2>;
> +		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> +		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> +	};
>  };
>  
>  &pinctrl_pmu {
> diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
> index f35bc5a288c2..5decad45a1b6 100644
> --- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> +++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> @@ -32,6 +32,8 @@
>  		spi0 = &spi_0;
>  		spi1 = &spi_1;
>  		spi2 = &spi_2;
> +		tdm0 = &tdm_0;
> +		tdm1 = &tdm_1;

Why?

>  	};
>  
>  	cpus {
> @@ -809,6 +811,42 @@
>  			status = "disabled";
>  		};
>  
> +		tdm_0: tdm@140e0000 {

Node names should be generic, so this looks like i2s.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +			compatible = "samsung,exynos7-i2s";
> +			reg = <0x0 0x140E0000 0x0 0x100>;
> +			interrupts = <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>;
> +			dmas = <&pdma1 14>, <&pdma1 13>, <&pdma1 12>;
> +			dma-names = "tx", "rx", "tx-sec";
> +			#clock-cells = <1>;
> +			#sound-dai-cells = <1>;
> +			clocks = <&clock_peric PERIC_HCLK_TDM0>,
> +				 <&clock_peric PERIC_HCLK_TDM0>,
> +				 <&clock_peric PERIC_PCLK_TDM0>;
> +			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

Best regards,
Krzysztof

RE: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 16 October 2022 08:44 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>;
> lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com
> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
> Subject: Re: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD
> 
> On 14/10/2022 06:21, Padmanabhan Rajanbabu wrote:
> > Add device tree node for I2S0 and I2S1 CPU DAI instances for Tesla FSD
> > board
> >
> > Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> > ---
> >  arch/arm64/boot/dts/tesla/fsd-evb.dts      |  8 +++++
> >  arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi | 14 ++++++++
> >  arch/arm64/boot/dts/tesla/fsd.dtsi         | 38 ++++++++++++++++++++++
> >  3 files changed, 60 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> > b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> > index 1db6ddf03f01..c0a4509499ab 100644
> > --- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> > +++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> > @@ -41,3 +41,11 @@
> >  &ufs {
> >  	status = "okay";
> >  };
> > +
> > +&tdm_0 {
> 
> Alphabetical order against other label-overrides.
Okay
> 
> > +	status = "okay";
> > +};
> > +
> > +&tdm_1 {
> > +	status = "okay";
> > +};
> > diff --git a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> > b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> > index e3852c946352..ff6f5d4b16dd 100644
> > --- a/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> > +++ b/arch/arm64/boot/dts/tesla/fsd-pinctrl.dtsi
> > @@ -339,6 +339,20 @@
> >  		samsung,pin-pud = <FSD_PIN_PULL_UP>;
> >  		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> >  	};
> > +
> > +	i2s0_bus: i2s0-bus {
> 
> Does not look like you tested the DTS against bindings. Please run `make
> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
> for instructions).
I'll double check and run dtbs_check to see if I'm hitting any errors.
> 
> > +		samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2", "gpd1-3",
> "gpd1-4";
> > +		samsung,pin-function = <FSD_PIN_FUNC_2>;
> > +		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> > +		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> > +	};
> > +
> > +	i2s1_bus: i2s1-bus {
> > +		samsung,pins = "gpd2-0", "gpd2-1", "gpd2-2", "gpd2-3",
> "gpd2-4";
> > +		samsung,pin-function = <FSD_PIN_FUNC_2>;
> > +		samsung,pin-pud = <FSD_PIN_PULL_DOWN>;
> > +		samsung,pin-drv = <FSD_PIN_DRV_LV4>;
> > +	};
> >  };
> >
> >  &pinctrl_pmu {
> > diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi
> > b/arch/arm64/boot/dts/tesla/fsd.dtsi
> > index f35bc5a288c2..5decad45a1b6 100644
> > --- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> > +++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> > @@ -32,6 +32,8 @@
> >  		spi0 = &spi_0;
> >  		spi1 = &spi_1;
> >  		spi2 = &spi_2;
> > +		tdm0 = &tdm_0;
> > +		tdm1 = &tdm_1;
> 
> Why?
Sorry, these aliases are not used right now. I'll remove it.

> 
> >  	};
> >
> >  	cpus {
> > @@ -809,6 +811,42 @@
> >  			status = "disabled";
> >  		};
> >
> > +		tdm_0: tdm@140e0000 {
> 
> Node names should be generic, so this looks like i2s.
> https://protect2.fireeye.com/v1/url?k=2cfaa5af-4d874de8-2cfb2ee0-
> 74fe485fff30-cb16acc0c0c574e9&q=1&e=fc8e3b54-a0ef-475e-a4f2-
> 83626a86ac8a&u=https%3A%2F%2Fdevicetree-
> specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-
> basics.html%23generic-names-recommendation

Thank you for the link. I could only find audio-controller in the list and
not i2s. so I believe I can use audio-controller node name. Please correct
me otherwise.
> 
> > +			compatible = "samsung,exynos7-i2s";
> > +			reg = <0x0 0x140E0000 0x0 0x100>;
> > +			interrupts = <GIC_SPI 206 IRQ_TYPE_LEVEL_HIGH>;
> > +			dmas = <&pdma1 14>, <&pdma1 13>, <&pdma1 12>;
> > +			dma-names = "tx", "rx", "tx-sec";
> > +			#clock-cells = <1>;
> > +			#sound-dai-cells = <1>;
> > +			clocks = <&clock_peric PERIC_HCLK_TDM0>,
> > +				 <&clock_peric PERIC_HCLK_TDM0>,
> > +				 <&clock_peric PERIC_PCLK_TDM0>;
> > +			clock-names = "i2s_opclk0", "i2s_opclk1", "iis";
> 
> Does not look like you tested the DTS against bindings. Please run `make
> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
> for instructions).
I'll double check and run dtbs_check to see if I'm hitting any errors.
> 
> Best regards,
> Krzysztof
Thank you for reviewing the patch

Re: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD

[Krzysztof Kozlowski]

On 21/10/2022 04:49, Padmanabhan Rajanbabu wrote:
>>>
>>>  	cpus {
>>> @@ -809,6 +811,42 @@
>>>  			status = "disabled";
>>>  		};
>>>
>>> +		tdm_0: tdm@140e0000 {
>>
>> Node names should be generic, so this looks like i2s.
>> https://protect2.fireeye.com/v1/url?k=2cfaa5af-4d874de8-2cfb2ee0-
>> 74fe485fff30-cb16acc0c0c574e9&q=1&e=fc8e3b54-a0ef-475e-a4f2-
>> 83626a86ac8a&u=https%3A%2F%2Fdevicetree-
>> specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-
>> basics.html%23generic-names-recommendation
> 
> Thank you for the link. I could only find audio-controller in the list and
> not i2s. so I believe I can use audio-controller node name. Please correct
> me otherwise.

All I2S controllers use node name "i2s", so if this is I2S, then use "i2s".


Best regards,
Krzysztof

RE: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 21 October 2022 06:32 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>;
> lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com
> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
> Subject: Re: [PATCH 5/6] arm64: dts: fsd: Add I2S DAI node for Tesla FSD
> 
> On 21/10/2022 04:49, Padmanabhan Rajanbabu wrote:
> >>>
> >>>  	cpus {
> >>> @@ -809,6 +811,42 @@
> >>>  			status = "disabled";
> >>>  		};
> >>>
> >>> +		tdm_0: tdm@140e0000 {
> >>
> >> Node names should be generic, so this looks like i2s.
> >> https://protect2.fireeye.com/v1/url?k=2cfaa5af-4d874de8-2cfb2ee0-
> >> 74fe485fff30-cb16acc0c0c574e9&q=1&e=fc8e3b54-a0ef-475e-a4f2-
> >> 83626a86ac8a&u=https%3A%2F%2Fdevicetree-
> >> specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-
> >> basics.html%23generic-names-recommendation
> >
> > Thank you for the link. I could only find audio-controller in the list
> > and not i2s. so I believe I can use audio-controller node name. Please
> > correct me otherwise.
> 
> All I2S controllers use node name "i2s", so if this is I2S, then use "i2s".

Okay, I'll ensure the next version will use "i2s" node name

> 
> 
> Best regards,
> Krzysztof

[PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla FSD

[Padmanabhan Rajanbabu]

Add device tree node support for sound card on Tesla FSD board

Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
---
 arch/arm64/boot/dts/tesla/fsd-evb.dts | 49 +++++++++++++++++++++++++++
 arch/arm64/boot/dts/tesla/fsd.dtsi    |  3 ++
 2 files changed, 52 insertions(+)

diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts b/arch/arm64/boot/dts/tesla/fsd-evb.dts
index c0a4509499ab..ecaa3c2e3045 100644
--- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
+++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
@@ -49,3 +49,52 @@
 &tdm_1 {
 	status = "okay";
 };
+
+&sound {
+	compatible = "tesla,fsd-sndcard";
+	status = "okay";
+	model = "fsd-i2s";
+	widgets =
+		"Speaker", "MAIN SPK",
+		"Microphone", "MAIN MIC";
+
+	primary-dai-link-0 {
+		link-name = "fsd-primary-0";
+		dai-format = "i2s";
+		tesla,bitclock-master = <&tdm_0>;
+		tesla,frame-master = <&tdm_0>;
+		cpu {
+			sound-dai = <&tdm_0 0>;
+		};
+	};
+
+	secondary-dai-link-0 {
+		link-name = "fsd-secondary-0";
+		dai-format = "i2s";
+		tesla,bitclock-master = <&tdm_0>;
+		tesla,frame-master = <&tdm_0>;
+		cpu {
+			sound-dai = <&tdm_0 1>;
+		};
+	};
+
+	primary-dai-link-1 {
+		link-name = "fsd-primary-1";
+		dai-format = "i2s";
+		tesla,bitclock-master = <&tdm_1>;
+		tesla,frame-master = <&tdm_1>;
+		cpu {
+			sound-dai = <&tdm_1 0>;
+		};
+	};
+
+	secondary-dai-link-1 {
+		link-name = "fsd-secondary-1";
+		dai-format = "i2s";
+		tesla,bitclock-master = <&tdm_1>;
+		tesla,frame-master = <&tdm_1>;
+		cpu {
+			sound-dai = <&tdm_1 1>;
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
index 5decad45a1b6..fc8931f830a7 100644
--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
@@ -847,6 +847,9 @@
 			status = "disabled";
 		};
 
+		sound: sound {
+		};
+
 		timer@10040000 {
 			compatible = "tesla,fsd-mct", "samsung,exynos4210-mct";
 			reg = <0x0 0x10040000 0x0 0x800>;
-- 
2.17.1

RE: [PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla FSD

[Alim Akhtar]

>-----Original Message-----
>From: Padmanabhan Rajanbabu [mailto:p.rajanbabu@samsung.com]
>Sent: Friday, October 14, 2022 3:52 PM
>To: lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
>krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
>perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
>alim.akhtar@samsung.com; rcsekar@samsung.com;
>aswani.reddy@samsung.com
>Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org; Padmanabhan
>Rajanbabu <p.rajanbabu@samsung.com>
>Subject: [PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla FSD
>
>Add device tree node support for sound card on Tesla FSD board
>
>Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
>---
> arch/arm64/boot/dts/tesla/fsd-evb.dts | 49
>+++++++++++++++++++++++++++
> arch/arm64/boot/dts/tesla/fsd.dtsi    |  3 ++
> 2 files changed, 52 insertions(+)
>
>diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts
>b/arch/arm64/boot/dts/tesla/fsd-evb.dts
>index c0a4509499ab..ecaa3c2e3045 100644
>--- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
>+++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
>@@ -49,3 +49,52 @@
> &tdm_1 {
> 	status = "okay";
> };
>+
>+&sound {
>+	compatible = "tesla,fsd-sndcard";
>+	status = "okay";
>+	model = "fsd-i2s";
>+	widgets =
>+		"Speaker", "MAIN SPK",
>+		"Microphone", "MAIN MIC";
>+
>+	primary-dai-link-0 {
>+		link-name = "fsd-primary-0";
>+		dai-format = "i2s";
>+		tesla,bitclock-master = <&tdm_0>;
>+		tesla,frame-master = <&tdm_0>;
>+		cpu {
>+			sound-dai = <&tdm_0 0>;
>+		};
>+	};
>+
>+	secondary-dai-link-0 {
>+		link-name = "fsd-secondary-0";
>+		dai-format = "i2s";
>+		tesla,bitclock-master = <&tdm_0>;
>+		tesla,frame-master = <&tdm_0>;
>+		cpu {
>+			sound-dai = <&tdm_0 1>;
>+		};
>+	};
>+
>+	primary-dai-link-1 {
>+		link-name = "fsd-primary-1";
>+		dai-format = "i2s";
>+		tesla,bitclock-master = <&tdm_1>;
>+		tesla,frame-master = <&tdm_1>;
>+		cpu {
>+			sound-dai = <&tdm_1 0>;
>+		};
>+	};
>+
>+	secondary-dai-link-1 {
>+		link-name = "fsd-secondary-1";
>+		dai-format = "i2s";
>+		tesla,bitclock-master = <&tdm_1>;
>+		tesla,frame-master = <&tdm_1>;
>+		cpu {
>+			sound-dai = <&tdm_1 1>;
>+		};
>+	};
>+};
>diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi
>b/arch/arm64/boot/dts/tesla/fsd.dtsi
>index 5decad45a1b6..fc8931f830a7 100644
>--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
>+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
>@@ -847,6 +847,9 @@
> 			status = "disabled";
> 		};
>
>+		sound: sound {
>+		};
>+
Why to have an empty node in dtsi?

> 		timer@10040000 {
> 			compatible = "tesla,fsd-mct", "samsung,exynos4210-
>mct";
> 			reg = <0x0 0x10040000 0x0 0x800>;
>--
>2.17.1

RE: [PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla FSD

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Alim Akhtar [mailto:alim.akhtar@samsung.com]
> Sent: 14 October 2022 06:59 PM
> To: 'Padmanabhan Rajanbabu' <p.rajanbabu@samsung.com>;
> lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> rcsekar@samsung.com; aswani.reddy@samsung.com
> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
> Subject: RE: [PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla FSD
> 
> 
> 
> >-----Original Message-----
> >From: Padmanabhan Rajanbabu [mailto:p.rajanbabu@samsung.com]
> >Sent: Friday, October 14, 2022 3:52 PM
> >To: lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> >krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> >perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> >alim.akhtar@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com
> >Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> >kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org;
> Padmanabhan
> >Rajanbabu <p.rajanbabu@samsung.com>
> >Subject: [PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla FSD
> >
> >Add device tree node support for sound card on Tesla FSD board
> >
> >Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> >---
> > arch/arm64/boot/dts/tesla/fsd-evb.dts | 49
> >+++++++++++++++++++++++++++
> > arch/arm64/boot/dts/tesla/fsd.dtsi    |  3 ++
> > 2 files changed, 52 insertions(+)
> >
> >diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >index c0a4509499ab..ecaa3c2e3045 100644
> >--- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >+++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >@@ -49,3 +49,52 @@
> > &tdm_1 {
> > 	status = "okay";
> > };
> >+
> >+&sound {
> >+	compatible = "tesla,fsd-sndcard";
> >+	status = "okay";
> >+	model = "fsd-i2s";
> >+	widgets =
> >+		"Speaker", "MAIN SPK",
> >+		"Microphone", "MAIN MIC";
> >+
> >+	primary-dai-link-0 {
> >+		link-name = "fsd-primary-0";
> >+		dai-format = "i2s";
> >+		tesla,bitclock-master = <&tdm_0>;
> >+		tesla,frame-master = <&tdm_0>;
> >+		cpu {
> >+			sound-dai = <&tdm_0 0>;
> >+		};
> >+	};
> >+
> >+	secondary-dai-link-0 {
> >+		link-name = "fsd-secondary-0";
> >+		dai-format = "i2s";
> >+		tesla,bitclock-master = <&tdm_0>;
> >+		tesla,frame-master = <&tdm_0>;
> >+		cpu {
> >+			sound-dai = <&tdm_0 1>;
> >+		};
> >+	};
> >+
> >+	primary-dai-link-1 {
> >+		link-name = "fsd-primary-1";
> >+		dai-format = "i2s";
> >+		tesla,bitclock-master = <&tdm_1>;
> >+		tesla,frame-master = <&tdm_1>;
> >+		cpu {
> >+			sound-dai = <&tdm_1 0>;
> >+		};
> >+	};
> >+
> >+	secondary-dai-link-1 {
> >+		link-name = "fsd-secondary-1";
> >+		dai-format = "i2s";
> >+		tesla,bitclock-master = <&tdm_1>;
> >+		tesla,frame-master = <&tdm_1>;
> >+		cpu {
> >+			sound-dai = <&tdm_1 1>;
> >+		};
> >+	};
> >+};
> >diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi
> >b/arch/arm64/boot/dts/tesla/fsd.dtsi
> >index 5decad45a1b6..fc8931f830a7 100644
> >--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> >+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> >@@ -847,6 +847,9 @@
> > 			status = "disabled";
> > 		};
> >
> >+		sound: sound {
> >+		};
> >+
> Why to have an empty node in dtsi?
This is required as every node we use in dts should have the same declared in
dtsi. Sound nodes in most of the platform is only declared (dummy node) in
dtsi and defining only in dts. Thus we are following the same.
> 
> > 		timer@10040000 {
> > 			compatible = "tesla,fsd-mct", "samsung,exynos4210-
> mct";
> > 			reg = <0x0 0x10040000 0x0 0x800>;
> >--
> >2.17.1
> 
Thank you for reviewing the patch

Re: [PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla FSD

[Krzysztof Kozlowski]

On 21/10/2022 04:12, Padmanabhan Rajanbabu wrote:
> 
> 
>> -----Original Message-----
>> From: Alim Akhtar [mailto:alim.akhtar@samsung.com]
>> Sent: 14 October 2022 06:59 PM
>> To: 'Padmanabhan Rajanbabu' <p.rajanbabu@samsung.com>;
>> lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
>> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
>> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
>> rcsekar@samsung.com; aswani.reddy@samsung.com
>> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
>> Subject: RE: [PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla FSD
>>
>>
>>
>>> -----Original Message-----
>>> From: Padmanabhan Rajanbabu [mailto:p.rajanbabu@samsung.com]
>>> Sent: Friday, October 14, 2022 3:52 PM
>>> To: lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
>>> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
>>> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
>>> alim.akhtar@samsung.com; rcsekar@samsung.com;
>> aswani.reddy@samsung.com
>>> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
>>> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org;
>> Padmanabhan
>>> Rajanbabu <p.rajanbabu@samsung.com>
>>> Subject: [PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla FSD
>>>
>>> Add device tree node support for sound card on Tesla FSD board
>>>
>>> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
>>> ---
>>> arch/arm64/boot/dts/tesla/fsd-evb.dts | 49
>>> +++++++++++++++++++++++++++
>>> arch/arm64/boot/dts/tesla/fsd.dtsi    |  3 ++
>>> 2 files changed, 52 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts
>>> b/arch/arm64/boot/dts/tesla/fsd-evb.dts
>>> index c0a4509499ab..ecaa3c2e3045 100644
>>> --- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
>>> +++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
>>> @@ -49,3 +49,52 @@
>>> &tdm_1 {
>>> 	status = "okay";
>>> };
>>> +
>>> +&sound {
>>> +	compatible = "tesla,fsd-sndcard";
>>> +	status = "okay";
>>> +	model = "fsd-i2s";
>>> +	widgets =
>>> +		"Speaker", "MAIN SPK",
>>> +		"Microphone", "MAIN MIC";
>>> +
>>> +	primary-dai-link-0 {
>>> +		link-name = "fsd-primary-0";
>>> +		dai-format = "i2s";
>>> +		tesla,bitclock-master = <&tdm_0>;
>>> +		tesla,frame-master = <&tdm_0>;
>>> +		cpu {
>>> +			sound-dai = <&tdm_0 0>;
>>> +		};
>>> +	};
>>> +
>>> +	secondary-dai-link-0 {
>>> +		link-name = "fsd-secondary-0";
>>> +		dai-format = "i2s";
>>> +		tesla,bitclock-master = <&tdm_0>;
>>> +		tesla,frame-master = <&tdm_0>;
>>> +		cpu {
>>> +			sound-dai = <&tdm_0 1>;
>>> +		};
>>> +	};
>>> +
>>> +	primary-dai-link-1 {
>>> +		link-name = "fsd-primary-1";
>>> +		dai-format = "i2s";
>>> +		tesla,bitclock-master = <&tdm_1>;
>>> +		tesla,frame-master = <&tdm_1>;
>>> +		cpu {
>>> +			sound-dai = <&tdm_1 0>;
>>> +		};
>>> +	};
>>> +
>>> +	secondary-dai-link-1 {
>>> +		link-name = "fsd-secondary-1";
>>> +		dai-format = "i2s";
>>> +		tesla,bitclock-master = <&tdm_1>;
>>> +		tesla,frame-master = <&tdm_1>;
>>> +		cpu {
>>> +			sound-dai = <&tdm_1 1>;
>>> +		};
>>> +	};
>>> +};
>>> diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi
>>> b/arch/arm64/boot/dts/tesla/fsd.dtsi
>>> index 5decad45a1b6..fc8931f830a7 100644
>>> --- a/arch/arm64/boot/dts/tesla/fsd.dtsi
>>> +++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
>>> @@ -847,6 +847,9 @@
>>> 			status = "disabled";
>>> 		};
>>>
>>> +		sound: sound {
>>> +		};
>>> +
>> Why to have an empty node in dtsi?
> This is required as every node we use in dts should have the same declared in

I see no reason why this is required.

> dtsi. Sound nodes in most of the platform is only declared (dummy node) in
> dtsi and defining only in dts. Thus we are following the same.

Can you point me to Samsung platform doing this?

Keep the code consistent with Exynos style.

Best regards,
Krzysztof

RE: [PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla FSD

[Padmanabhan Rajanbabu]

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 21 October 2022 06:24 PM
> To: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>; 'Alim Akhtar'
> <alim.akhtar@samsung.com>; lgirdwood@gmail.com; broonie@kernel.org;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> s.nawrocki@samsung.com; perex@perex.cz; tiwai@suse.com;
> pankaj.dubey@samsung.com; rcsekar@samsung.com;
> aswani.reddy@samsung.com
> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
> Subject: Re: [PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla FSD
> 
> On 21/10/2022 04:12, Padmanabhan Rajanbabu wrote:
> >
> >
> >> -----Original Message-----
> >> From: Alim Akhtar [mailto:alim.akhtar@samsung.com]
> >> Sent: 14 October 2022 06:59 PM
> >> To: 'Padmanabhan Rajanbabu' <p.rajanbabu@samsung.com>;
> >> lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> >> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> >> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> >> rcsekar@samsung.com; aswani.reddy@samsung.com
> >> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org
> >> Subject: RE: [PATCH 6/6] arm64: dts: fsd: Add sound card node for
> >> Tesla FSD
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Padmanabhan Rajanbabu [mailto:p.rajanbabu@samsung.com]
> >>> Sent: Friday, October 14, 2022 3:52 PM
> >>> To: lgirdwood@gmail.com; broonie@kernel.org; robh+dt@kernel.org;
> >>> krzysztof.kozlowski+dt@linaro.org; s.nawrocki@samsung.com;
> >>> perex@perex.cz; tiwai@suse.com; pankaj.dubey@samsung.com;
> >>> alim.akhtar@samsung.com; rcsekar@samsung.com;
> >> aswani.reddy@samsung.com
> >>> Cc: alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-
> >>> kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org;
> >> Padmanabhan
> >>> Rajanbabu <p.rajanbabu@samsung.com>
> >>> Subject: [PATCH 6/6] arm64: dts: fsd: Add sound card node for Tesla
> >>> FSD
> >>>
> >>> Add device tree node support for sound card on Tesla FSD board
> >>>
> >>> Signed-off-by: Padmanabhan Rajanbabu <p.rajanbabu@samsung.com>
> >>> ---
> >>> arch/arm64/boot/dts/tesla/fsd-evb.dts | 49
> >>> +++++++++++++++++++++++++++
> >>> arch/arm64/boot/dts/tesla/fsd.dtsi    |  3 ++
> >>> 2 files changed, 52 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >>> b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >>> index c0a4509499ab..ecaa3c2e3045 100644
> >>> --- a/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >>> +++ b/arch/arm64/boot/dts/tesla/fsd-evb.dts
> >>> @@ -49,3 +49,52 @@
> >>> &tdm_1 {
> >>> 	status = "okay";
> >>> };
> >>> +
> >>> +&sound {
> >>> +	compatible = "tesla,fsd-sndcard";
> >>> +	status = "okay";
> >>> +	model = "fsd-i2s";
> >>> +	widgets =
> >>> +		"Speaker", "MAIN SPK",
> >>> +		"Microphone", "MAIN MIC";
> >>> +
> >>> +	primary-dai-link-0 {
> >>> +		link-name = "fsd-primary-0";
> >>> +		dai-format = "i2s";
> >>> +		tesla,bitclock-master = <&tdm_0>;
> >>> +		tesla,frame-master = <&tdm_0>;
> >>> +		cpu {
> >>> +			sound-dai = <&tdm_0 0>;
> >>> +		};
> >>> +	};
> >>> +
> >>> +	secondary-dai-link-0 {
> >>> +		link-name = "fsd-secondary-0";
> >>> +		dai-format = "i2s";
> >>> +		tesla,bitclock-master = <&tdm_0>;
> >>> +		tesla,frame-master = <&tdm_0>;
> >>> +		cpu {
> >>> +			sound-dai = <&tdm_0 1>;
> >>> +		};
> >>> +	};
> >>> +
> >>> +	primary-dai-link-1 {
> >>> +		link-name = "fsd-primary-1";
> >>> +		dai-format = "i2s";
> >>> +		tesla,bitclock-master = <&tdm_1>;
> >>> +		tesla,frame-master = <&tdm_1>;
> >>> +		cpu {
> >>> +			sound-dai = <&tdm_1 0>;
> >>> +		};
> >>> +	};
> >>> +
> >>> +	secondary-dai-link-1 {
> >>> +		link-name = "fsd-secondary-1";
> >>> +		dai-format = "i2s";
> >>> +		tesla,bitclock-master = <&tdm_1>;
> >>> +		tesla,frame-master = <&tdm_1>;
> >>> +		cpu {
> >>> +			sound-dai = <&tdm_1 1>;
> >>> +		};
> >>> +	};
> >>> +};
> >>> diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi
> >>> b/arch/arm64/boot/dts/tesla/fsd.dtsi
> >>> index 5decad45a1b6..fc8931f830a7 100644
> >>> --- a/arch/arm64/boot/dts/tesla/fsd.dtsi
> >>> +++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
> >>> @@ -847,6 +847,9 @@
> >>> 			status = "disabled";
> >>> 		};
> >>>
> >>> +		sound: sound {
> >>> +		};
> >>> +
> >> Why to have an empty node in dtsi?
> > This is required as every node we use in dts should have the same
> > declared in
> 
> I see no reason why this is required.
> 
> > dtsi. Sound nodes in most of the platform is only declared (dummy
> > node) in dtsi and defining only in dts. Thus we are following the same.
> 
> Can you point me to Samsung platform doing this?
> 
> Keep the code consistent with Exynos style.

Okay, will add the sound node in accordance with Exynos style

> 
> Best regards,
> Krzysztof