[PATCH 0/4] ASoC: Intel: KMB: TDM Enablement patches

[PATCH 0/4] ASoC: Intel: KMB: TDM Enablement patches

[Michael Sit Wei Hong]

This patch series is to enable multiple features on the Keembay Platform

Michael Sit Wei Hong (4):
  ASoC: Intel: KMB: Add 8kHz audio support
  ASoC: Intel: KMB: Rework disable channel function
  ASoC: Intel: KMB: Enable TDM audio capture
  dt-bindings: sound: intel,keembay-i2s: Add channel-max property

 .../bindings/sound/intel,keembay-i2s.yaml     |   8 +
 sound/soc/intel/keembay/kmb_platform.c        | 137 +++++++++++++-----
 sound/soc/intel/keembay/kmb_platform.h        |   1 +
 3 files changed, 112 insertions(+), 34 deletions(-)

-- 
2.17.1

[PATCH 1/4] ASoC: Intel: KMB: Add 8kHz audio support

[Michael Sit Wei Hong]

Enable 8kHz audio support for Intel Keem Bay platform.

Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
Reviewed-by: Sia Jee Heng <jee.heng.sia@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/keembay/kmb_platform.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/keembay/kmb_platform.c b/sound/soc/intel/keembay/kmb_platform.c
index 2ce21336c06b..f94eb5b7598d 100644
--- a/sound/soc/intel/keembay/kmb_platform.c
+++ b/sound/soc/intel/keembay/kmb_platform.c
@@ -28,8 +28,10 @@ static const struct snd_pcm_hardware kmb_pcm_hardware = {
 		SNDRV_PCM_INFO_MMAP_VALID |
 		SNDRV_PCM_INFO_BATCH |
 		SNDRV_PCM_INFO_BLOCK_TRANSFER,
-	.rates = SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_48000,
-	.rate_min = 16000,
+	.rates = SNDRV_PCM_RATE_8000 |
+		 SNDRV_PCM_RATE_16000 |
+		 SNDRV_PCM_RATE_48000,
+	.rate_min = 8000,
 	.rate_max = 48000,
 	.formats = SNDRV_PCM_FMTBIT_S16_LE |
 		   SNDRV_PCM_FMTBIT_S24_LE |
@@ -533,8 +535,10 @@ static struct snd_soc_dai_driver intel_kmb_platform_dai[] = {
 		.playback = {
 			.channels_min = 2,
 			.channels_max = 2,
-			.rates = SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_48000,
-			.rate_min = 16000,
+			.rates = SNDRV_PCM_RATE_8000 |
+				 SNDRV_PCM_RATE_16000 |
+				 SNDRV_PCM_RATE_48000,
+			.rate_min = 8000,
 			.rate_max = 48000,
 			.formats = (SNDRV_PCM_FMTBIT_S32_LE |
 				    SNDRV_PCM_FMTBIT_S24_LE |
@@ -543,8 +547,14 @@ static struct snd_soc_dai_driver intel_kmb_platform_dai[] = {
 		.capture = {
 			.channels_min = 2,
 			.channels_max = 2,
-			.rates = SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_48000,
-			.rate_min = 16000,
+			/*
+			 * .channels_max will be overwritten
+			 * if provided by Device Tree
+			 */
+			.rates = SNDRV_PCM_RATE_8000 |
+				 SNDRV_PCM_RATE_16000 |
+				 SNDRV_PCM_RATE_48000,
+			.rate_min = 8000,
 			.rate_max = 48000,
 			.formats = (SNDRV_PCM_FMTBIT_S32_LE |
 				    SNDRV_PCM_FMTBIT_S24_LE |
-- 
2.17.1

[PATCH 2/4] ASoC: Intel: KMB: Rework disable channel function

[Michael Sit Wei Hong]

All channels are enabled at boot up, this patch ensures that all
channels are disabled at boot and whenever the function is called.

Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
Reviewed-by: Sia Jee Heng <jee.heng.sia@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/keembay/kmb_platform.c | 10 +++++++---
 sound/soc/intel/keembay/kmb_platform.h |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/keembay/kmb_platform.c b/sound/soc/intel/keembay/kmb_platform.c
index f94eb5b7598d..eaa4fd391171 100644
--- a/sound/soc/intel/keembay/kmb_platform.c
+++ b/sound/soc/intel/keembay/kmb_platform.c
@@ -109,14 +109,14 @@ static unsigned int kmb_pcm_rx_fn(struct kmb_i2s_info *kmb_i2s,
 static inline void kmb_i2s_disable_channels(struct kmb_i2s_info *kmb_i2s,
 					    u32 stream)
 {
-	struct i2s_clk_config_data *config = &kmb_i2s->config;
 	u32 i;
 
+	/* Disable all channels regardless of configuration*/
 	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		for (i = 0; i < config->chan_nr / 2; i++)
+		for (i = 0; i < MAX_ISR; i++)
 			writel(0, kmb_i2s->i2s_base + TER(i));
 	} else {
-		for (i = 0; i < config->chan_nr / 2; i++)
+		for (i = 0; i < MAX_ISR; i++)
 			writel(0, kmb_i2s->i2s_base + RER(i));
 	}
 }
@@ -637,6 +637,10 @@ static int kmb_plat_dai_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* To ensure none of the channels are enabled at boot up */
+	kmb_i2s_disable_channels(kmb_i2s, SNDRV_PCM_STREAM_PLAYBACK);
+	kmb_i2s_disable_channels(kmb_i2s, SNDRV_PCM_STREAM_CAPTURE);
+
 	dev_set_drvdata(dev, kmb_i2s);
 
 	return ret;
diff --git a/sound/soc/intel/keembay/kmb_platform.h b/sound/soc/intel/keembay/kmb_platform.h
index 29600652d8f4..da522b366995 100644
--- a/sound/soc/intel/keembay/kmb_platform.h
+++ b/sound/soc/intel/keembay/kmb_platform.h
@@ -90,6 +90,7 @@
 
 #define MAX_CHANNEL_NUM		8
 #define MIN_CHANNEL_NUM		2
+#define MAX_ISR			4
 
 #define TWO_CHANNEL_SUPPORT	2	/* up to 2.0 */
 #define FOUR_CHANNEL_SUPPORT	4	/* up to 3.1 */
-- 
2.17.1

[PATCH 3/4] ASoC: Intel: KMB: Enable TDM audio capture

[Michael Sit Wei Hong]

Enable I2S TDM audio capture for Intel Keem Bay platform.
The I2S TDM will support 4 channel and 8 channel audio capture only.
4 channel and 8 channel audio capture operates only in slave mode.

Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
Reviewed-by: Sia Jee Heng <jee.heng.sia@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/keembay/kmb_platform.c | 105 +++++++++++++++++++------
 1 file changed, 80 insertions(+), 25 deletions(-)

diff --git a/sound/soc/intel/keembay/kmb_platform.c b/sound/soc/intel/keembay/kmb_platform.c
index eaa4fd391171..8cb097ecf4bc 100644
--- a/sound/soc/intel/keembay/kmb_platform.c
+++ b/sound/soc/intel/keembay/kmb_platform.c
@@ -17,7 +17,7 @@
 #define PERIODS_MAX		48
 #define PERIOD_BYTES_MIN	4096
 #define BUFFER_BYTES_MAX	(PERIODS_MAX * PERIOD_BYTES_MIN)
-#define TDM_OPERATION		1
+#define TDM_OPERATION		5
 #define I2S_OPERATION		0
 #define DATA_WIDTH_CONFIG_BIT	6
 #define TDM_CHANNEL_CONFIG_BIT	3
@@ -37,7 +37,7 @@ static const struct snd_pcm_hardware kmb_pcm_hardware = {
 		   SNDRV_PCM_FMTBIT_S24_LE |
 		   SNDRV_PCM_FMTBIT_S32_LE,
 	.channels_min = 2,
-	.channels_max = 2,
+	/* channels_max removed to allow configurable max channels */
 	.buffer_bytes_max = BUFFER_BYTES_MAX,
 	.period_bytes_min = PERIOD_BYTES_MIN,
 	.period_bytes_max = BUFFER_BYTES_MAX / PERIODS_MIN,
@@ -82,19 +82,25 @@ static unsigned int kmb_pcm_rx_fn(struct kmb_i2s_info *kmb_i2s,
 {
 	unsigned int period_pos = rx_ptr % runtime->period_size;
 	void __iomem *i2s_base = kmb_i2s->i2s_base;
+	int chan = kmb_i2s->config.chan_nr;
 	void *buf = runtime->dma_area;
-	int i;
+	int i, j;
 
 	/* KMB i2s uses two separate L/R FIFO */
 	for (i = 0; i < kmb_i2s->fifo_th; i++) {
-		if (kmb_i2s->config.data_width == 16) {
-			((u16(*)[2])buf)[rx_ptr][0] = readl(i2s_base + LRBR_LTHR(0));
-			((u16(*)[2])buf)[rx_ptr][1] = readl(i2s_base + RRBR_RTHR(0));
-		} else {
-			((u32(*)[2])buf)[rx_ptr][0] = readl(i2s_base + LRBR_LTHR(0));
-			((u32(*)[2])buf)[rx_ptr][1] = readl(i2s_base + RRBR_RTHR(0));
+		for (j = 0; j < chan / 2; j++) {
+			if (kmb_i2s->config.data_width == 16) {
+				((u16 *)buf)[rx_ptr * chan + (j * 2)] =
+						readl(i2s_base + LRBR_LTHR(j));
+				((u16 *)buf)[rx_ptr * chan + ((j * 2) + 1)] =
+						readl(i2s_base + RRBR_RTHR(j));
+			} else {
+				((u32 *)buf)[rx_ptr * chan + (j * 2)] =
+						readl(i2s_base + LRBR_LTHR(j));
+				((u32 *)buf)[rx_ptr * chan + ((j * 2) + 1)] =
+						readl(i2s_base + RRBR_RTHR(j));
+			}
 		}
-
 		period_pos++;
 
 		if (++rx_ptr >= runtime->buffer_size)
@@ -238,6 +244,7 @@ static irqreturn_t kmb_i2s_irq_handler(int irq, void *dev_id)
 	struct kmb_i2s_info *kmb_i2s = dev_id;
 	struct i2s_clk_config_data *config = &kmb_i2s->config;
 	irqreturn_t ret = IRQ_NONE;
+	u32 tx_enabled = 0;
 	u32 isr[4];
 	int i;
 
@@ -246,22 +253,45 @@ static irqreturn_t kmb_i2s_irq_handler(int irq, void *dev_id)
 
 	kmb_i2s_clear_irqs(kmb_i2s, SNDRV_PCM_STREAM_PLAYBACK);
 	kmb_i2s_clear_irqs(kmb_i2s, SNDRV_PCM_STREAM_CAPTURE);
+	/* Only check TX interrupt if TX is active */
+	tx_enabled = readl(kmb_i2s->i2s_base + ITER);
+
+	/*
+	 * Data available. Retrieve samples from FIFO
+	 */
+
+	/*
+	 * 8 channel audio will have isr[0..2] triggered,
+	 * reading the specific isr based on the audio configuration,
+	 * to avoid reading the buffers too early.
+	 */
+	switch (config->chan_nr) {
+	case 2:
+		if (isr[0] & ISR_RXDA)
+			kmb_pcm_operation(kmb_i2s, false);
+		ret = IRQ_HANDLED;
+		break;
+	case 4:
+		if (isr[1] & ISR_RXDA)
+			kmb_pcm_operation(kmb_i2s, false);
+		ret = IRQ_HANDLED;
+		break;
+	case 8:
+		if (isr[3] & ISR_RXDA)
+			kmb_pcm_operation(kmb_i2s, false);
+		ret = IRQ_HANDLED;
+		break;
+	}
 
 	for (i = 0; i < config->chan_nr / 2; i++) {
 		/*
 		 * Check if TX fifo is empty. If empty fill FIFO with samples
 		 */
-		if ((isr[i] & ISR_TXFE)) {
+		if ((isr[i] & ISR_TXFE) && tx_enabled) {
 			kmb_pcm_operation(kmb_i2s, true);
 			ret = IRQ_HANDLED;
 		}
-		/*
-		 * Data available. Retrieve samples from FIFO
-		 */
-		if ((isr[i] & ISR_RXDA)) {
-			kmb_pcm_operation(kmb_i2s, false);
-			ret = IRQ_HANDLED;
-		}
+
 		/* Error Handling: TX */
 		if (isr[i] & ISR_TXFO) {
 			dev_dbg(kmb_i2s->dev, "TX overrun (ch_id=%d)\n", i);
@@ -445,7 +475,7 @@ static int kmb_dai_hw_params(struct snd_pcm_substream *substream,
 {
 	struct kmb_i2s_info *kmb_i2s = snd_soc_dai_get_drvdata(cpu_dai);
 	struct i2s_clk_config_data *config = &kmb_i2s->config;
-	u32 register_val, write_val;
+	u32 write_val;
 	int ret;
 
 	switch (params_format(hw_params)) {
@@ -472,16 +502,34 @@ static int kmb_dai_hw_params(struct snd_pcm_substream *substream,
 	config->chan_nr = params_channels(hw_params);
 
 	switch (config->chan_nr) {
-	/* TODO: This switch case will handle up to TDM8 in the near future */
-	case TWO_CHANNEL_SUPPORT:
+	case 8:
+	case 4:
+		/*
+		 * Platform is not capable of providing clocks for
+		 * multi channel audio
+		 */
+		if (kmb_i2s->master)
+			return -EINVAL;
+
 		write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) |
 				(config->data_width << DATA_WIDTH_CONFIG_BIT) |
-				MASTER_MODE | I2S_OPERATION;
+				!MASTER_MODE | TDM_OPERATION;
 
 		writel(write_val, kmb_i2s->pss_base + I2S_GEN_CFG_0);
+		break;
+	case 2:
+		/*
+		 * Platform is only capable of providing clocks need for
+		 * 2 channel master mode
+		 */
+		if (!(kmb_i2s->master))
+			return -EINVAL;
+
+		write_val = ((config->chan_nr / 2) << TDM_CHANNEL_CONFIG_BIT) |
+				(config->data_width << DATA_WIDTH_CONFIG_BIT) |
+				MASTER_MODE | I2S_OPERATION;
 
-		register_val = readl(kmb_i2s->pss_base + I2S_GEN_CFG_0);
-		dev_dbg(kmb_i2s->dev, "pss register = 0x%X", register_val);
+		writel(write_val, kmb_i2s->pss_base + I2S_GEN_CFG_0);
 		break;
 	default:
 		dev_dbg(kmb_i2s->dev, "channel not supported\n");
@@ -531,7 +579,7 @@ static struct snd_soc_dai_ops kmb_dai_ops = {
 
 static struct snd_soc_dai_driver intel_kmb_platform_dai[] = {
 	{
-		.name = "kmb-plat-dai",
+	/* .name is provided by Device Tree */
 		.playback = {
 			.channels_min = 2,
 			.channels_max = 2,
@@ -571,6 +619,7 @@ static int kmb_plat_dai_probe(struct platform_device *pdev)
 	struct kmb_i2s_info *kmb_i2s;
 	int ret, irq;
 	u32 comp1_reg;
+	u32 channel_max;
 
 	kmb_i2s = devm_kzalloc(dev, sizeof(*kmb_i2s), GFP_KERNEL);
 	if (!kmb_i2s)
@@ -629,6 +678,12 @@ static int kmb_plat_dai_probe(struct platform_device *pdev)
 
 	kmb_i2s->fifo_th = (1 << COMP1_FIFO_DEPTH(comp1_reg)) / 2;
 
+	ret = of_property_read_u32(dev->of_node, "channel-max", &channel_max);
+	if (ret < 0)
+		dev_err(dev, "Couldn't find channel-max\n");
+	else if (intel_kmb_platform_dai->capture.channels_max < channel_max)
+		intel_kmb_platform_dai->capture.channels_max = channel_max;
+
 	ret = devm_snd_soc_register_component(dev, &kmb_component,
 					      intel_kmb_platform_dai,
 				ARRAY_SIZE(intel_kmb_platform_dai));
-- 
2.17.1

[PATCH 4/4] dt-bindings: sound: intel, keembay-i2s: Add channel-max property

[Michael Sit Wei Hong]

Add a property that configures the interface to the desired max number
of capture channels. The platform may have multiple interfaces with
different number of capture channels.

Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
Reviewed-by: Sia Jee Heng <jee.heng.sia@intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 .../devicetree/bindings/sound/intel,keembay-i2s.yaml      | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml b/Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml
index 175e4fb0c315..9345fc3a2b95 100644
--- a/Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml
+++ b/Documentation/devicetree/bindings/sound/intel,keembay-i2s.yaml
@@ -43,6 +43,10 @@ properties:
       - const: osc
       - const: apb_clk
 
+  channel-max:
+    items:
+      - description: Maximum audio input channels
+
 required:
   - compatible
   - "#sound-dai-cells"
@@ -51,6 +55,9 @@ required:
   - clock-names
   - interrupts
 
+optional:
+  - channel-max
+
 examples:
   - |
      #include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -65,4 +72,5 @@ examples:
          interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
          clock-names = "osc", "apb_clk";
          clocks = <&scmi_clk KEEM_BAY_PSS_AUX_I2S3>, <&scmi_clk KEEM_BAY_PSS_I2S3>;
+         channel-max = <2>;
      };
-- 
2.17.1

Re: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add channel-max property

[Mark Brown]

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

On Thu, Jul 30, 2020 at 01:53:19PM +0800, Michael Sit Wei Hong wrote:

> Add a property that configures the interface to the desired max number
> of capture channels. The platform may have multiple interfaces with
> different number of capture channels.

Why?  None of the other platforms which support many channels need this
defining and the constraint code and/or machine driver would normally be
where this would be handled.

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

Re: [PATCH 0/4] ASoC: Intel: KMB: TDM Enablement patches

[Mark Brown]

On Thu, 30 Jul 2020 13:53:15 +0800, Michael Sit Wei Hong wrote:
> This patch series is to enable multiple features on the Keembay Platform
> 
> Michael Sit Wei Hong (4):
>   ASoC: Intel: KMB: Add 8kHz audio support
>   ASoC: Intel: KMB: Rework disable channel function
>   ASoC: Intel: KMB: Enable TDM audio capture
>   dt-bindings: sound: intel,keembay-i2s: Add channel-max property
> 
> [...]

Applied to

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

Thanks!

[1/2] ASoC: Intel: KMB: Add 8kHz audio support
      commit: b81f8df8039e6dca8f9533fe0b549139b907e61f
[2/2] ASoC: Intel: KMB: Rework disable channel function
      commit: d13389849caf05d79141d317bdf2f6b318e65f87

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

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

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

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

Thanks,
Mark

RE: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add channel-max property

[Sit, Michael Wei Hong]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Thursday, 30 July, 2020 7:30 PM
> To: Sit, Michael Wei Hong <michael.wei.hong.sit@intel.com>
> Cc: alsa-devel@alsa-project.org; tiwai@suse.com; pierre-
> louis.bossart@linux.intel.com; Rojewski, Cezary
> <cezary.rojewski@intel.com>; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; liam.r.girdwood@linux.intel.com; Sia,
> Jee Heng <jee.heng.sia@intel.com>
> Subject: Re: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add
> channel-max property
> 
> On Thu, Jul 30, 2020 at 01:53:19PM +0800, Michael Sit Wei Hong wrote:
> 
> > Add a property that configures the interface to the desired max number
> > of capture channels. The platform may have multiple interfaces with
> > different number of capture channels.
> 
> Why?  None of the other platforms which support many channels need
> this defining and the constraint code and/or machine driver would
> normally be where this would be handled.

The platform uses the audio-graph-card to create the dai-links, and doesn't use a specific machine driver code.
The platform also has 2 different interfaces which have different supported max-channels.
Using this value in the device-tree to determine the maximum supported channel of the interface.

Re: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add channel-max property

[Mark Brown]

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

On Mon, Aug 03, 2020 at 01:59:10AM +0000, Sit, Michael Wei Hong wrote:
> > On Thu, Jul 30, 2020 at 01:53:19PM +0800, Michael Sit Wei Hong wrote:

> > > Add a property that configures the interface to the desired max number
> > > of capture channels. The platform may have multiple interfaces with
> > > different number of capture channels.

> > Why?  None of the other platforms which support many channels need
> > this defining and the constraint code and/or machine driver would
> > normally be where this would be handled.

> The platform uses the audio-graph-card to create the dai-links, and doesn't use a specific machine driver code.

The audio-graph-card is very flexible and if it doesn't support
something which it is useful to configure per platform then that's the
place to add the extension, not some DAI specific driver.

> The platform also has 2 different interfaces which have different supported max-channels.
> Using this value in the device-tree to determine the maximum supported channel of the interface.

These should have different compatible strings, there are likely further
differences between them (even if they are not currently documented).

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

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

RE: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add channel-max property

[Sit, Michael Wei Hong]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Monday, 3 August, 2020 6:49 PM
> To: Sit, Michael Wei Hong <michael.wei.hong.sit@intel.com>
> Cc: alsa-devel@alsa-project.org; tiwai@suse.com; pierre-
> louis.bossart@linux.intel.com; Rojewski, Cezary
> <cezary.rojewski@intel.com>; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; liam.r.girdwood@linux.intel.com;
> Sia, Jee Heng <jee.heng.sia@intel.com>
> Subject: Re: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add
> channel-max property
> 
> On Mon, Aug 03, 2020 at 01:59:10AM +0000, Sit, Michael Wei Hong
> wrote:
> > > On Thu, Jul 30, 2020 at 01:53:19PM +0800, Michael Sit Wei Hong
> wrote:
> 
> > > > Add a property that configures the interface to the desired max
> > > > number of capture channels. The platform may have multiple
> > > > interfaces with different number of capture channels.
> 
> > > Why?  None of the other platforms which support many
> channels need
> > > this defining and the constraint code and/or machine driver
> would
> > > normally be where this would be handled.
> 
> > The platform uses the audio-graph-card to create the dai-links,
> and doesn't use a specific machine driver code.
> 
> The audio-graph-card is very flexible and if it doesn't support
> something which it is useful to configure per platform then that's
> the place to add the extension, not some DAI specific driver.
> 
> > The platform also has 2 different interfaces which have different
> supported max-channels.
> > Using this value in the device-tree to determine the maximum
> supported channel of the interface.
> 
> These should have different compatible strings, there are likely
> further differences between them (even if they are not currently
> documented).
> 
The 2 different I2S ports are from the same SoC which supports different
number of channels, do we need different compatible strings for this?
Considering the only difference is the maximum supported channels is 8 and 2?

> Please fix your mail client to word wrap within paragraphs at
> something substantially less than 80 columns.  Doing this makes
> your messages much easier to read and reply to.

Re: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add channel-max property

[Mark Brown]

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

On Tue, Aug 04, 2020 at 01:57:23AM +0000, Sit, Michael Wei Hong wrote:

> > > The platform also has 2 different interfaces which have different
> > supported max-channels.
> > > Using this value in the device-tree to determine the maximum
> > supported channel of the interface.

> > These should have different compatible strings, there are likely
> > further differences between them (even if they are not currently
> > documented).

> The 2 different I2S ports are from the same SoC which supports different
> number of channels, do we need different compatible strings for this?
> Considering the only difference is the maximum supported channels is 8 and 2?

Are you *sure* that's the only difference, or is that just the only
difference you know about right now?

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

RE: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add channel-max property

[Sit, Michael Wei Hong]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Tuesday, 4 August, 2020 7:50 PM
> To: Sit, Michael Wei Hong <michael.wei.hong.sit@intel.com>
> Cc: alsa-devel@alsa-project.org; tiwai@suse.com; pierre-
> louis.bossart@linux.intel.com; Rojewski, Cezary
> <cezary.rojewski@intel.com>; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; liam.r.girdwood@linux.intel.com;
> Sia, Jee Heng <jee.heng.sia@intel.com>
> Subject: Re: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add
> channel-max property
> 
> On Tue, Aug 04, 2020 at 01:57:23AM +0000, Sit, Michael Wei Hong
> wrote:
> 
> > > > The platform also has 2 different interfaces which have
> different
> > > supported max-channels.
> > > > Using this value in the device-tree to determine the maximum
> > > supported channel of the interface.
> 
> > > These should have different compatible strings, there are likely
> > > further differences between them (even if they are not currently
> > > documented).
> 
> > The 2 different I2S ports are from the same SoC which supports
> > different number of channels, do we need different compatible
> strings for this?
> > Considering the only difference is the maximum supported
> channels is 8 and 2?
> 
> Are you *sure* that's the only difference, or is that just the only
> difference you know about right now?

Yes, I am fairy sure that is the only difference according to the design, as per the platform use case.

Re: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add channel-max property

[Mark Brown]

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

On Wed, Aug 05, 2020 at 06:21:14AM +0000, Sit, Michael Wei Hong wrote:

> > Are you *sure* that's the only difference, or is that just the only
> > difference you know about right now?

> Yes, I am fairy sure that is the only difference according to the design, as per the platform use case.

It would still be safer to have a separate compatible, it wouldn't be
the first time that additional changes that the hardware people had
failed to communicate were discovered.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

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

RE: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add channel-max property

[Sit, Michael Wei Hong]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Friday, 7 August, 2020 10:31 PM
> To: Sit, Michael Wei Hong <michael.wei.hong.sit@intel.com>
> Cc: alsa-devel@alsa-project.org; tiwai@suse.com; pierre-
> louis.bossart@linux.intel.com; Rojewski, Cezary
> <cezary.rojewski@intel.com>; Shevchenko, Andriy
> <andriy.shevchenko@intel.com>; liam.r.girdwood@linux.intel.com;
> Sia, Jee Heng <jee.heng.sia@intel.com>
> Subject: Re: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add
> channel-max property
> 
> On Wed, Aug 05, 2020 at 06:21:14AM +0000, Sit, Michael Wei Hong
> wrote:
> 
> > > Are you *sure* that's the only difference, or is that just the only
> > > difference you know about right now?
> 
> > Yes, I am fairy sure that is the only difference according to the
> design, as per the platform use case.
> 
> It would still be safer to have a separate compatible, it wouldn't be
> the first time that additional changes that the hardware people had
> failed to communicate were discovered.
> 
I will create a separate compatible string in the next revision of the patch.
Since part of the patch series has already been merged, do I send in the new
patch as a new review or on top of these patch series?
> Please fix your mail client to word wrap within paragraphs at
> something substantially less than 80 columns.  Doing this makes
> your messages much easier to read and reply to.

Re: [PATCH 4/4] dt-bindings: sound: intel,keembay-i2s: Add channel-max property

[Mark Brown]

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

On Mon, Aug 10, 2020 at 09:47:15AM +0000, Sit, Michael Wei Hong wrote:

> Since part of the patch series has already been merged, do I send in the new
> patch as a new review or on top of these patch series?

Only send the patch that hasn't been applied yet, it should be based on
top of the current code (including the patches that have already been
applied) but no need to resend anything that was already applied.

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