[PATCH v2 0/6] ASoC: dwc: Fix several issues in the I2S driver

[PATCH v2 0/6] ASoC: dwc: Fix several issues in the I2S driver

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Hi all,

This series addresses several issues in the DesignWare I2S driver,
including clock rate calculation, DAI configuration, channel limits,
DMA FIFO sizing, and error handling.

Changes in v2:
 - Move the indentation and typo fixes into a separate commit.
 - Add a separate patch to propagate optional IRQ lookup errors.
 - Add Maxim Kochetkov's Reviewed-by tag.

Link v1: 
https://lore.kernel.org/all/20261106112940.53550-1-phucduc.bui@gmail.com/

Compile-tested only.

Best regards,
Phuc

bui duc phuc (6):
  ASoC: dwc: Fix bit clock rate in TDM mode
  ASoC: dwc: Fix error handling in dw_i2s_set_fmt()
  ASoC: dwc: Fix channels_max derived from COMP_PARAM_1
  ASoC: dwc: Fix DMA fifo_size calculation
  ASoC: dwc: Propagate optional IRQ lookup errors
  ASoC: dwc: Remove redundant error messages

 sound/soc/dwc/dwc-i2s.c | 57 +++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 34 deletions(-)

-- 
2.43.0

[PATCH v2 1/6] ASoC: dwc: Fix bit clock rate in TDM mode

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

The bit clock is computed as rate * data_width * 2, which only fits
a two-slot I2S frame. Use rate * data_width * tdm_slots in TDM mode.

Fixes: 221acc16aee1 ("ASoC: dwc: Add TDM mode support")
Reviewed-by: Maxim Kochetkov <fido_max@inbox.ru>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v2:
 - Move the indentation fixes into a separate commit.
 - Add Maxim Kochetkov's Reviewed-by tag.

 sound/soc/dwc/dwc-i2s.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index dbda943cfaf3..1f4d06a82333 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -332,6 +332,9 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
 		} else {
 			u32 bitclk = config->sample_rate *
 					config->data_width * 2;
+			if (dev->tdm_slots)
+				bitclk = config->sample_rate *
+					 config->data_width * dev->tdm_slots;
 
 			ret = clk_set_rate(dev->clk, bitclk);
 			if (ret) {
-- 
2.43.0

[PATCH v2 2/6] ASoC: dwc: Fix error handling in dw_i2s_set_fmt()

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Return early when an unsupported clock provider format is
encountered, preventing the function from updating frame_offset
before returning an error.

Fixes: 221acc16aee1 ("ASoC: dwc: Add TDM mode support")
Reviewed-by: Maxim Kochetkov <fido_max@inbox.ru>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v2:
 - Move the typo fixes into a separate commit.
 - Add Maxim Kochetkov's Reviewed-by tag.

 sound/soc/dwc/dwc-i2s.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index 1f4d06a82333..6a25d2a69ea7 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -390,29 +390,19 @@ static int dw_i2s_trigger(struct snd_pcm_substream *substream,
 static int dw_i2s_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
 {
 	struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(cpu_dai);
-	int ret = 0;
 
 	switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
 	case SND_SOC_DAIFMT_BC_FC:
-		if (dev->capability & DW_I2S_SLAVE)
-			ret = 0;
-		else
-			ret = -EINVAL;
+		if (!(dev->capability & DW_I2S_SLAVE))
+			return -EINVAL;
 		break;
 	case SND_SOC_DAIFMT_BP_FP:
-		if (dev->capability & DW_I2S_MASTER)
-			ret = 0;
-		else
-			ret = -EINVAL;
-		break;
-	case SND_SOC_DAIFMT_BC_FP:
-	case SND_SOC_DAIFMT_BP_FC:
-		ret = -EINVAL;
+		if (!(dev->capability & DW_I2S_MASTER))
+			return -EINVAL;
 		break;
 	default:
 		dev_dbg(dev->dev, "dwc : Invalid clock provider format\n");
-		ret = -EINVAL;
-		break;
+		return -EINVAL;
 	}
 
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
@@ -431,7 +421,7 @@ static int dw_i2s_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
 		return -EINVAL;
 	}
 
-	return ret;
+	return 0;
 }
 
 static int dw_i2s_set_tdm_slot(struct snd_soc_dai *cpu_dai,	unsigned int tx_mask,
-- 
2.43.0

[PATCH v2 3/6] ASoC: dwc: Fix channels_max derived from COMP_PARAM_1

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

The I2S_TX_CHANNELS (bits 10:9) and I2S_RX_CHANNELS (bits 8:7) fields
of I2S_COMP_PARAM_1 encode the number of hardware channels minus one,
i.e. values 0..3 mean 1..4 channels. In I2S mode each hardware channel
is one serial data line carrying a left/right pair, so the maximum
number of audio channels is 2 * (N + 1).

The current code uses 1 << (N + 1), which only produces the correct
result for N < 2. For N >= 2, it overestimates the maximum number of
audio channels.

Use 2 * (N + 1) for both playback and capture.

Fixes: b226efe5818b ("ASoC: dwc: Read I2S block configuration from registers")
Reviewed-by: Maxim Kochetkov <fido_max@inbox.ru>
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v2:
 - Add Maxim Kochetkov's Reviewed-by tag.

 sound/soc/dwc/dwc-i2s.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index 6a25d2a69ea7..c5773f191db7 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -605,7 +605,7 @@ static int dw_configure_dai(struct dw_i2s_dev *dev,
 			idx = 1;
 		dw_i2s_dai->playback.channels_min = MIN_CHANNEL_NUM;
 		dw_i2s_dai->playback.channels_max =
-				1 << (COMP1_TX_CHANNELS(comp1) + 1);
+				2 * (COMP1_TX_CHANNELS(comp1) + 1);
 		dw_i2s_dai->playback.formats = formats[idx];
 		dw_i2s_dai->playback.rates = rates;
 	}
@@ -619,7 +619,7 @@ static int dw_configure_dai(struct dw_i2s_dev *dev,
 			idx = 1;
 		dw_i2s_dai->capture.channels_min = MIN_CHANNEL_NUM;
 		dw_i2s_dai->capture.channels_max =
-				1 << (COMP1_RX_CHANNELS(comp1) + 1);
+				2 * (COMP1_RX_CHANNELS(comp1) + 1);
 		dw_i2s_dai->capture.formats = formats[idx];
 		dw_i2s_dai->capture.rates = rates;
 	}
-- 
2.43.0

[PATCH v2 4/6] ASoC: dwc: Fix DMA fifo_size calculation

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

fifo_size should be the FIFO size in bytes (depth * width / 8), but
the code shifts by 8 instead of 3. For capture the parentheses also
make it always 0.

Use depth * width / 8 for both directions.

Fixes: 52ea7c0543f8 ("ASoC: dwc: i2s: Add StarFive JH7110 SoC support")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/dwc/dwc-i2s.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index c5773f191db7..b0d3413c27d2 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -664,14 +664,14 @@ static int dw_configure_dai_by_pd(struct dw_i2s_dev *dev,
 			idx2 = COMP1_TX_WORDSIZE_0(comp1);
 			dev->play_dma_data.dt.addr = res->start + I2S_TXDMA;
 			dev->play_dma_data.dt.fifo_size = dev->fifo_th * 2 *
-				(fifo_width[idx2]) >> 8;
+							  fifo_width[idx2] / 8;
 			dev->play_dma_data.dt.maxburst = 16;
 		}
 		if (COMP1_RX_ENABLED(comp1)) {
 			idx2 = COMP2_RX_WORDSIZE_0(comp2);
 			dev->capture_dma_data.dt.addr = res->start + I2S_RXDMA;
 			dev->capture_dma_data.dt.fifo_size = dev->fifo_th * 2 *
-				(fifo_width[idx2] >> 8);
+							     fifo_width[idx2] / 8;
 			dev->capture_dma_data.dt.maxburst = 16;
 		}
 	} else {
@@ -711,7 +711,7 @@ static int dw_configure_dai_by_dt(struct dw_i2s_dev *dev,
 		dev->capability |= DWC_I2S_PLAY;
 		dev->play_dma_data.dt.addr = res->start + I2S_TXDMA;
 		dev->play_dma_data.dt.fifo_size = fifo_depth *
-			(fifo_width[idx2]) >> 8;
+						  fifo_width[idx2] / 8;
 		dev->play_dma_data.dt.maxburst = 16;
 	}
 	if (COMP1_RX_ENABLED(comp1)) {
@@ -720,7 +720,7 @@ static int dw_configure_dai_by_dt(struct dw_i2s_dev *dev,
 		dev->capability |= DWC_I2S_RECORD;
 		dev->capture_dma_data.dt.addr = res->start + I2S_RXDMA;
 		dev->capture_dma_data.dt.fifo_size = fifo_depth *
-			(fifo_width[idx2] >> 8);
+						     fifo_width[idx2] / 8;
 		dev->capture_dma_data.dt.maxburst = 16;
 	}
 
-- 
2.43.0

Re: [PATCH v2 4/6] ASoC: dwc: Fix DMA fifo_size calculation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] ASoC: dwc: missing Fixes trailer for dw_configure_dai_by_dt()
  fifo_size bug
--

commit 7aa9b7e8f65aeb4b8c0e3324d72ada2ec12b9b6b
Author: bui duc phuc <phucduc.bui@gmail.com>

> ASoC: dwc: Fix DMA fifo_size calculation
>
> fifo_size should be the FIFO size in bytes (depth * width / 8), but
> the code shifts by 8 instead of 3. For capture the parentheses also
> make it always 0.
>
> Use depth * width / 8 for both directions.
>
> Fixes: 52ea7c0543f8 ("ASoC: dwc: i2s: Add StarFive JH7110 SoC support")
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>

[Severity: Low]
While 52ea7c0543f8 ("ASoC: dwc: i2s: Add StarFive JH7110 SoC support")
added the calculation in dw_configure_dai_by_pd(), the broken fifo_size
calculation in dw_configure_dai_by_dt() was introduced earlier. Should
this also include:

  Fixes: 0d274544bfee ("ASoC: dwc: Add devicetree support for Designware I2S")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110095525.504660-1-phucduc.bui@gmail.com?part=4

Re: [PATCH v2 4/6] ASoC: dwc: Fix DMA fifo_size calculation

[Bui Duc Phuc]

>
> [Severity: Low]
> While 52ea7c0543f8 ("ASoC: dwc: i2s: Add StarFive JH7110 SoC support")
> added the calculation in dw_configure_dai_by_pd(), the broken fifo_size
> calculation in dw_configure_dai_by_dt() was introduced earlier. Should
> this also include:
>
>   Fixes: 0d274544bfee ("ASoC: dwc: Add devicetree support for Designware I2S")
>

Agreed. I'll add the Fixes tag in the next version.

[PATCH v2 5/6] ASoC: dwc: Propagate optional IRQ lookup errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

platform_get_irq_optional() can return errors other than
-EPROBE_DEFER. Propagate these errors instead of silently
continuing, while treating -ENXIO as the absence of an IRQ.

Use dev_err_probe() to report IRQ lookup failures.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/dwc/dwc-i2s.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index b0d3413c27d2..0d2be2d8d31a 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -961,8 +961,8 @@ static int dw_i2s_probe(struct platform_device *pdev)
 	}
 
 	irq = platform_get_irq_optional(pdev, 0);
-	if (irq == -EPROBE_DEFER)
-		return irq;
+	if (irq < 0 && irq != -ENXIO)
+		return dev_err_probe(&pdev->dev, irq, "failed to get IRQ\n");
 	if (irq > 0) {
 		ret = devm_request_irq(&pdev->dev, irq, i2s_irq_handler, 0,
 				pdev->name, dev);
-- 
2.43.0

Re: [PATCH v2 5/6] ASoC: dwc: Propagate optional IRQ lookup errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ASoC: dwc: unbalanced reset deassert in dw_i2s_probe() on IRQ
  error
--

commit f22e35683fe4bb23831ac84cbbbb8c5cfc290ae0
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: dwc: Propagate optional IRQ lookup errors

This commit updates dw_i2s_probe() to propagate errors other than -ENXIO
from platform_get_irq_optional() using dev_err_probe().

> diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
> index b0d3413c27d2b..0d2be2d8d31ae 100644
> --- a/sound/soc/dwc/dwc-i2s.c
> +++ b/sound/soc/dwc/dwc-i2s.c
> @@ -961,8 +961,8 @@ static int dw_i2s_probe(struct platform_device *pdev)
>  	}
>  
>  	irq = platform_get_irq_optional(pdev, 0);
> -	if (irq == -EPROBE_DEFER)
> -		return irq;
> +	if (irq < 0 && irq != -ENXIO)
> +		return dev_err_probe(&pdev->dev, irq, "failed to get IRQ\n");

[Severity: Medium]
Does returning directly here leave dev->reset deasserted when
platform_get_irq_optional() fails?

Just before the IRQ lookup, dw_i2s_probe() manually deasserts the shared
reset control via reset_control_deassert(dev->reset), and later failure
paths jump to err_assert_reset to call reset_control_assert(dev->reset).
Bypassing err_assert_reset leaves the hardware reset line deasserted and
leaks a deassert reference on the shared reset control.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110095525.504660-1-phucduc.bui@gmail.com?part=5

Re: [PATCH v2 5/6] ASoC: dwc: Propagate optional IRQ lookup errors

[Bui Duc Phuc]

> > diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
> > index b0d3413c27d2b..0d2be2d8d31ae 100644
> > --- a/sound/soc/dwc/dwc-i2s.c
> > +++ b/sound/soc/dwc/dwc-i2s.c
> > @@ -961,8 +961,8 @@ static int dw_i2s_probe(struct platform_device *pdev)
> >       }
> >
> >       irq = platform_get_irq_optional(pdev, 0);
> > -     if (irq == -EPROBE_DEFER)
> > -             return irq;
> > +     if (irq < 0 && irq != -ENXIO)
> > +             return dev_err_probe(&pdev->dev, irq, "failed to get IRQ\n");
>
> [Severity: Medium]
> Does returning directly here leave dev->reset deasserted when
> platform_get_irq_optional() fails?
>
> Just before the IRQ lookup, dw_i2s_probe() manually deasserts the shared
> reset control via reset_control_deassert(dev->reset), and later failure
> paths jump to err_assert_reset to call reset_control_assert(dev->reset).
> Bypassing err_assert_reset leaves the hardware reset line deasserted and
> leaks a deassert reference on the shared reset control.
>

Good catch.
I'll update the error handling to ensure the reset is asserted on this
failure path.

[PATCH v2 6/6] ASoC: dwc: Remove redundant error messages

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

The underlying functions already report these errors deeper in
the call chain, making the additional dev_err_probe() and
dev_err() calls unnecessary.

Also fix the indentation of the affected code and correct a typo
in the DAI format error message.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Changes in v2:
 - Add the indentation and typo fixes to this commit.

 sound/soc/dwc/dwc-i2s.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index 0d2be2d8d31a..d09cf1cd07f4 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -331,7 +331,7 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
 			}
 		} else {
 			u32 bitclk = config->sample_rate *
-					config->data_width * 2;
+				     config->data_width * 2;
 			if (dev->tdm_slots)
 				bitclk = config->sample_rate *
 					 config->data_width * dev->tdm_slots;
@@ -417,7 +417,7 @@ static int dw_i2s_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
 		dev->frame_offset = 0;
 		break;
 	default:
-		dev_err(dev->dev, "DAI format unsupported");
+		dev_err(dev->dev, "DAI format unsupported\n");
 		return -EINVAL;
 	}
 
@@ -752,7 +752,7 @@ static int jh7110_i2s_crg_master_init(struct dw_i2s_dev *dev)
 
 	ret = clk_bulk_get(dev->dev, ARRAY_SIZE(clks), clks);
 	if (ret)
-		return dev_err_probe(dev->dev, ret, "failed to get i2s clocks\n");
+		return ret;
 
 	mclk = clks[0].clk;
 	mclk_ext = clks[1].clk;
@@ -830,7 +830,7 @@ static int jh7110_i2s_crg_slave_init(struct dw_i2s_dev *dev)
 
 	ret = clk_bulk_get(dev->dev, ARRAY_SIZE(clks), clks);
 	if (ret)
-		return dev_err_probe(dev->dev, ret, "failed to get i2s clocks\n");
+		return ret;
 
 	mclk = clks[0].clk;
 	mclk_ext = clks[1].clk;
@@ -966,10 +966,8 @@ static int dw_i2s_probe(struct platform_device *pdev)
 	if (irq > 0) {
 		ret = devm_request_irq(&pdev->dev, irq, i2s_irq_handler, 0,
 				pdev->name, dev);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "failed to request irq\n");
+		if (ret < 0)
 			goto err_assert_reset;
-		}
 	}
 
 	dev->i2s_reg_comp1 = I2S_COMP_PARAM_1;
@@ -1010,10 +1008,8 @@ static int dw_i2s_probe(struct platform_device *pdev)
 	dev_set_drvdata(&pdev->dev, dev);
 	ret = devm_snd_soc_register_component(&pdev->dev, &dw_i2s_component,
 					 dw_i2s_dai, 1);
-	if (ret != 0) {
-		dev_err(&pdev->dev, "not able to register dai\n");
+	if (ret)
 		goto err_assert_reset;
-	}
 
 	if (!pdata || dev->is_jh7110) {
 		if (irq >= 0) {
-- 
2.43.0