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

["Troy Mitchell" <troy.mitchell@linux.spacemit.com>]

[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 248 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv