[PATCH] ASoC: spacemit: Remove redundant DAI field initialization

[PATCH] ASoC: spacemit: Remove redundant DAI field initialization

[phucduc.bui]

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

spacemit_i2s_init_dai() already initializes the playback and capture
fields after duplicating the static DAI template with devm_kmemdup().
Remove the duplicated initializers from the static template and keep
all field initialization in spacemit_i2s_init_dai().

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

diff --git a/sound/soc/spacemit/k1_i2s.c b/sound/soc/spacemit/k1_i2s.c
index 2d5ea1fd5d49..28a762769253 100644
--- a/sound/soc/spacemit/k1_i2s.c
+++ b/sound/soc/spacemit/k1_i2s.c
@@ -354,22 +354,6 @@ static const struct snd_soc_dai_ops spacemit_i2s_dai_ops = {
 
 static struct snd_soc_dai_driver spacemit_i2s_dai = {
 	.ops = &spacemit_i2s_dai_ops,
-	.playback = {
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = SPACEMIT_PCM_RATES,
-		.rate_min = SNDRV_PCM_RATE_8000,
-		.rate_max = SNDRV_PCM_RATE_48000,
-		.formats = SPACEMIT_PCM_FORMATS,
-	},
-	.capture = {
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = SPACEMIT_PCM_RATES,
-		.rate_min = SNDRV_PCM_RATE_8000,
-		.rate_max = SNDRV_PCM_RATE_48000,
-		.formats = SPACEMIT_PCM_FORMATS,
-	},
 	.symmetric_rate = 1,
 };
 
@@ -399,6 +383,8 @@ static int spacemit_i2s_init_dai(struct spacemit_i2s_dev *i2s,
 		dai->playback.channels_min = 1;
 		dai->playback.channels_max = 2;
 		dai->playback.rates = SPACEMIT_PCM_RATES;
+		dai->playback.rate_min = SNDRV_PCM_RATE_8000;
+		dai->playback.rate_max = SNDRV_PCM_RATE_48000;
 		dai->playback.formats = SPACEMIT_PCM_FORMATS;
 
 		i2s->playback_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
@@ -411,6 +397,8 @@ static int spacemit_i2s_init_dai(struct spacemit_i2s_dev *i2s,
 		dai->capture.channels_min = 1;
 		dai->capture.channels_max = 2;
 		dai->capture.rates = SPACEMIT_PCM_RATES;
+		dai->capture.rate_min = SNDRV_PCM_RATE_8000;
+		dai->capture.rate_max = SNDRV_PCM_RATE_48000;
 		dai->capture.formats = SPACEMIT_PCM_FORMATS;
 
 		i2s->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
-- 
2.43.0

Re: [PATCH] ASoC: spacemit: Remove redundant DAI field initialization

[Troy Mitchell]

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

> spacemit_i2s_init_dai() already initializes the playback and capture
> fields after duplicating the static DAI template with devm_kmemdup().
> Remove the duplicated initializers from the static template and keep
> all field initialization in spacemit_i2s_init_dai().

The code change is functionally correct, but the commit message currently
describes it only as removing redundant initialization. It also changes
which stream directions ASoC considers valid.

> @@ -354,22 +354,6 @@ static const struct snd_soc_dai_ops spacemit_i2s_dai_ops = {
>  
>  static struct snd_soc_dai_driver spacemit_i2s_dai = {
>  	.ops = &spacemit_i2s_dai_ops,
> -	.playback = {
> -		.channels_min = 1,
> -		.channels_max = 2,
> -		.rates = SPACEMIT_PCM_RATES,
> -		.rate_min = SNDRV_PCM_RATE_8000,
> -		.rate_max = SNDRV_PCM_RATE_48000,
> -		.formats = SPACEMIT_PCM_FORMATS,
> -	},
> -	.capture = {
> -		.channels_min = 1,
> -		.channels_max = 2,
> -		.rates = SPACEMIT_PCM_RATES,
> -		.rate_min = SNDRV_PCM_RATE_8000,
> -		.rate_max = SNDRV_PCM_RATE_48000,
> -		.formats = SPACEMIT_PCM_FORMATS,
> -	},
>  	.symmetric_rate = 1,
>  };

With the current static initialization, channels_min is nonzero for both
directions before dma-names is examined. snd_soc_dai_stream_valid()
therefore considers capture valid even when the device only provides a
"tx" DMA channel, which is permitted by the binding. The DMAengine PCM
code may then create a capture substream without an RX DMA channel and
fail during PCM creation.

> [...]
>
> @@ -399,6 +383,8 @@ static int spacemit_i2s_init_dai(struct spacemit_i2s_dev *i2s,
>  		dai->playback.channels_min = 1;
>  		dai->playback.channels_max = 2;
>  		dai->playback.rates = SPACEMIT_PCM_RATES;
> +		dai->playback.rate_min = SNDRV_PCM_RATE_8000;
> +		dai->playback.rate_max = SNDRV_PCM_RATE_48000;
>  		dai->playback.formats = SPACEMIT_PCM_FORMATS;
>  
>  		i2s->playback_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>
> [...]
>
> @@ -411,6 +397,8 @@ static int spacemit_i2s_init_dai(struct spacemit_i2s_dev *i2s,
>  		dai->capture.channels_min = 1;
>  		dai->capture.channels_max = 2;
>  		dai->capture.rates = SPACEMIT_PCM_RATES;
> +		dai->capture.rate_min = SNDRV_PCM_RATE_8000;
> +		dai->capture.rate_max = SNDRV_PCM_RATE_48000;
>  		dai->capture.formats = SPACEMIT_PCM_FORMATS;
>  
>  		i2s->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES;

Initializing these fields conditionally preserves the existing
capabilities for devices with both "tx" and "rx" DMA channels, while
correctly disabling directions without a corresponding DMA channel. The
added rate_min and rate_max assignments also preserve the existing rate
constraints.

Please update the subject and commit message to describe this functional
fix rather than only the removal of redundant initialization. A possible
subject is:

  ASoC: spacemit: advertise only DMA-backed DAI streams

Since the issue was introduced with the original driver, please also add:

  Fixes: fce217449075 ("ASoC: spacemit: add i2s support for K1 SoC")

                                            - Troy

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

Re: [PATCH] ASoC: spacemit: Remove redundant DAI field initialization

[Bui Duc Phuc]

Hi Troy,

Thank you for your feedback.

>
> Initializing these fields conditionally preserves the existing
> capabilities for devices with both "tx" and "rx" DMA channels, while
> correctly disabling directions without a corresponding DMA channel. The
> added rate_min and rate_max assignments also preserve the existing rate
> constraints.
>

Yes, I agree. That's also the conclusion I reached after reading
rockchip_i2s_init_dai().

>
> With the current static initialization, channels_min is nonzero for both
> directions before dma-names is examined. snd_soc_dai_stream_valid()
> therefore considers capture valid even when the device only provides a
> "tx" DMA channel, which is permitted by the binding. The DMAengine PCM
> code may then create a capture substream without an RX DMA channel and
> fail during PCM creation.
>

I traced the probe path and confirmed that snd_soc_dai_stream_valid()
is called from soc_new_pcm().
However, I still couldn't see where a nonzero channels_min alone leads
to creating a capture substream
without an RX DMA channel.
To avoid confusion, I'm referring specifically to the DMA-based
configuration, not PIO mode.
Could you point me to the relevant call path?

Best regards,
Phuc

Re: [PATCH] ASoC: spacemit: Remove redundant DAI field initialization

[Troy Mitchell]

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

> I traced the probe path and confirmed that snd_soc_dai_stream_valid()
> is called from soc_new_pcm().
> However, I still couldn't see where a nonzero channels_min alone leads
> to creating a capture substream
> without an RX DMA channel.
> To avoid confusion, I'm referring specifically to the DMA-based
> configuration, not PIO mode.
> Could you point me to the relevant call path?

More precisely, channels_min alone is not sufficient. The codec DAI must
also support capture (or be the dummy DAI for a dynamic link), and the link
must not be playback_only.

Under those conditions, the path is:

  soc_new_pcm()
    -> soc_get_playback_capture()
    -> soc_create_pcm(..., capture = 1)
    -> snd_pcm_new()
    -> snd_pcm_new_stream(CAPTURE, 1)
    -> snd_soc_pcm_component_new()
    -> dmaengine_pcm_new()

dmaengine_pcm_request_chan_of() tolerates a missing "rx" channel and leaves
pcm->chan[CAPTURE] NULL. Once the capture substream exists,
dmaengine_pcm_new() returns -EINVAL with
"Missing dma channel for stream: 1".
The conditional initialization prevents the CPU DAI from advertising that
direction for a tx-only node.

                                            - Troy

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

Re: [PATCH] ASoC: spacemit: Remove redundant DAI field initialization

[Troy Mitchell]

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

> I traced the probe path and confirmed that snd_soc_dai_stream_valid()
> is called from soc_new_pcm().
> However, I still couldn't see where a nonzero channels_min alone leads
> to creating a capture substream
> without an RX DMA channel.
> To avoid confusion, I'm referring specifically to the DMA-based
> configuration, not PIO mode.
> Could you point me to the relevant call path?

More precisely, channels_min alone is not sufficient. The codec DAI must
also support capture (or be the dummy DAI for a dynamic link), and the link
must not be playback_only.

Under those conditions, the path is:

  soc_new_pcm()
    -> soc_get_playback_capture()
    -> soc_create_pcm(..., capture = 1)
    -> snd_pcm_new()
    -> snd_pcm_new_stream(CAPTURE, 1)
    -> snd_soc_pcm_component_new()
    -> dmaengine_pcm_new()

dmaengine_pcm_request_chan_of() tolerates a missing "rx" channel and leaves
pcm->chan[CAPTURE] NULL. Once the capture substream exists,
dmaengine_pcm_new() returns -EINVAL with
"Missing dma channel for stream: 1".
The conditional initialization prevents the CPU DAI from advertising that
direction for a tx-only node.

                                            - Troy

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

Re: [PATCH] ASoC: spacemit: Remove redundant DAI field initialization

[Bui Duc Phuc]

Hi Troy,

>
> More precisely, channels_min alone is not sufficient. The codec DAI must
> also support capture (or be the dummy DAI for a dynamic link), and the link
> must not be playback_only.
>
> Under those conditions, the path is:
>
>   soc_new_pcm()
>     -> soc_get_playback_capture()
>     -> soc_create_pcm(..., capture = 1)
>     -> snd_pcm_new()
>     -> snd_pcm_new_stream(CAPTURE, 1)
>     -> snd_soc_pcm_component_new()
>     -> dmaengine_pcm_new()
>
> dmaengine_pcm_request_chan_of() tolerates a missing "rx" channel and leaves
> pcm->chan[CAPTURE] NULL. Once the capture substream exists,
> dmaengine_pcm_new() returns -EINVAL with
> "Missing dma channel for stream: 1".
> The conditional initialization prevents the CPU DAI from advertising that
> direction for a tx-only node.
>

Thanks for the detailed explanation and for tracing the call path.
I don't have Spacemit hardware available to verify this myself.
I'll update the commit message to reflect the behavior you've verified
on the hardware more accurately.

Best regards,
Phuc