Re: [PATCH v2 5/5] ASoC: pcm179x: Add support for pcm1975

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] sound: pcm1795 misidentified as pcm1792a on spi/i2c due to missing driver_data
- [Medium] ASoC: pcm179x: 16-bit format missing for PCM1795
- [Medium] ASoC: pcm1795: hw_params fails with -EINVAL for 32-bit LEFT_J streams
--

commit 82f31b72b82184d1d5d7c469d77cb1c8fab51b6d
Author: Michael Trimarchi <michael@amarulasolutions.com>
Subject: ASoC: pcm179x: Add support for pcm1975

The commit adds support for the PCM1795 audio codec, introducing a new codec
model differentiation based on device match data. It adds device match tables
for OF along with SPI ID tables, and separate format handling tables and
startup constraints for the new model.

> diff --git a/sound/soc/codecs/pcm179x-spi.c b/sound/soc/codecs/pcm179x-spi.c
> index 4b873f2986c77..a6c6ac1eb97b2 100644
> --- a/sound/soc/codecs/pcm179x-spi.c
> +++ b/sound/soc/codecs/pcm179x-spi.c
[ ... ]
> @@ -31,13 +31,15 @@ static int pcm179x_spi_probe(struct spi_device *spi)
[ ... ]
>  static const struct spi_device_id pcm179x_spi_ids[] = {
>  	{ "pcm1792a", 0 },
> +	{ "pcm1795", 0 },
>  	{ "pcm1796", 0 },
>  	{ "pcm179x", 0 },
>  	{ },

[Severity: Medium]
Since the driver differentiates models using enum pcm179x_type where
PCM1795 is 1, will leaving the driver_data as 0 cause non-OF setups to
misidentify the PCM1795 as a PCM1792A?

When device_get_match_data() is called in pcm179x_common_init(), it will
resolve to 0 for these legacy boards. This could break audio playback
for formats encoded differently between models, such as 32-bit I2S.

Additionally, should the new models be added to the I2C ID table in
pcm179x-i2c.c for the same reason?

> diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
> index fae5de318b6ea..8e9b9e98c8dff 100644
> --- a/sound/soc/codecs/pcm179x.c
> +++ b/sound/soc/codecs/pcm179x.c
[ ... ]
> @@ -82,16 +94,53 @@ static const struct pcm179x_fmt_table pcm1792a_fmt[] = {
>  	{ SND_SOC_DAIFMT_I2S,     32, 5 },
>  };
>  
> +static const struct pcm179x_fmt_table pcm1795_fmt[] = {
> +	{ SND_SOC_DAIFMT_RIGHT_J, 16, 0 },
> +	{ SND_SOC_DAIFMT_RIGHT_J, 24, 2 },
> +	{ SND_SOC_DAIFMT_RIGHT_J, 32, 1 },
> +	{ SND_SOC_DAIFMT_LEFT_J,  24, 3 },
> +	{ SND_SOC_DAIFMT_I2S,     24, 5 },
> +	{ SND_SOC_DAIFMT_I2S,     32, 4 },
> +};

[Severity: Medium]
Is there a missing entry here for { SND_SOC_DAIFMT_LEFT_J, 32, 3 }?

Because PCM1795_FORMATS includes SNDRV_PCM_FMTBIT_S32_LE and the ASoC core
allows LEFT_J for all models, userspace can request 32-bit LEFT_J streams.

Without an entry in this table, pcm179x_fmt_value() will fail and return
-EINVAL during pcm179x_hw_params(), causing playback to fail at runtime.

> diff --git a/sound/soc/codecs/pcm179x.h b/sound/soc/codecs/pcm179x.h
> index 0039ca8ee7420..33971fa935469 100644
> --- a/sound/soc/codecs/pcm179x.h
> +++ b/sound/soc/codecs/pcm179x.h
[ ... ]
> @@ -8,8 +8,16 @@
>  #ifndef __PCM179X_H__
>  #define __PCM179X_H__
>  
> -#define PCM1792A_FORMATS (SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE | \
> -			  SNDRV_PCM_FMTBIT_S16_LE)
> +enum pcm179x_type {
> +	PCM1792A,
> +	PCM1795,
> +	PCM1796,
> +};
> +
> +#define PCM179X_FORMATS (SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE | \
> +			 SNDRV_PCM_FMTBIT_S16_LE)
> +
> +#define PCM1795_FORMATS (SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE)

[Severity: Medium]
Does the PCM1795 support 16-bit audio streams?

The pcm1795_fmt table in pcm179x.c contains an entry for 16-bit Right-J:

        { SND_SOC_DAIFMT_RIGHT_J, 16, 0 },

By excluding SNDRV_PCM_FMTBIT_S16_LE from PCM1795_FORMATS, the ALSA core
unconditionally rejects all 16-bit streams at open time. This forces
userspace to upsample or pad streams, and makes the 16-bit Right-J
table entry unreachable dead code.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-work-v2-0-9340b9555fbb@amarulasolutions.com?part=5