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

--61ecdfd0fe19057f00ac6f6f76c3e71ea0f2054d2589209ef8ab808b217a
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8

> 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 =3D {
> =20
>  static struct snd_soc_dai_driver spacemit_i2s_dai =3D {
>  	.ops =3D &spacemit_i2s_dai_ops,
> -	.playback =3D {
> -		.channels_min =3D 1,
> -		.channels_max =3D 2,
> -		.rates =3D SPACEMIT_PCM_RATES,
> -		.rate_min =3D SNDRV_PCM_RATE_8000,
> -		.rate_max =3D SNDRV_PCM_RATE_48000,
> -		.formats =3D SPACEMIT_PCM_FORMATS,
> -	},
> -	.capture =3D {
> -		.channels_min =3D 1,
> -		.channels_max =3D 2,
> -		.rates =3D SPACEMIT_PCM_RATES,
> -		.rate_min =3D SNDRV_PCM_RATE_8000,
> -		.rate_max =3D SNDRV_PCM_RATE_48000,
> -		.formats =3D SPACEMIT_PCM_FORMATS,
> -	},
>  	.symmetric_rate =3D 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 =3D 1;
>  		dai->playback.channels_max =3D 2;
>  		dai->playback.rates =3D SPACEMIT_PCM_RATES;
> +		dai->playback.rate_min =3D SNDRV_PCM_RATE_8000;
> +		dai->playback.rate_max =3D SNDRV_PCM_RATE_48000;
>  		dai->playback.formats =3D SPACEMIT_PCM_FORMATS;
> =20
>  		i2s->playback_dma_data.addr_width =3D DMA_SLAVE_BUSWIDTH_2_BYTES;
>
> [...]
>
> @@ -411,6 +397,8 @@ static int spacemit_i2s_init_dai(struct spacemit_i2s_=
dev *i2s,
>  		dai->capture.channels_min =3D 1;
>  		dai->capture.channels_max =3D 2;
>  		dai->capture.rates =3D SPACEMIT_PCM_RATES;
> +		dai->capture.rate_min =3D SNDRV_PCM_RATE_8000;
> +		dai->capture.rate_max =3D SNDRV_PCM_RATE_48000;
>  		dai->capture.formats =3D SPACEMIT_PCM_FORMATS;
> =20
>  		i2s->capture_dma_data.addr_width =3D 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

--61ecdfd0fe19057f00ac6f6f76c3e71ea0f2054d2589209ef8ab808b217a
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iIMEABYKACsWIQSL4Ay2cExaPXAQcU2YCe+A+TM0LwUCanGJKw0caUB0cm95LXku
b3JnAAoJEJgJ74D5MzQvrYMBAMbOjR8cB3gZnCCwoFuogtbhkkXZKuHUVOalvNnO
xLXaAP9cyjXtTVuRiipuj1Wc5jAjZomGvTqPv1ctpbtf1xqFBA==
=BzH6
-----END PGP SIGNATURE-----

--61ecdfd0fe19057f00ac6f6f76c3e71ea0f2054d2589209ef8ab808b217a--