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

[Mark Brown]

> The PCM1795 is register compatible with the PCM1792A, but it encodes two
> of the register 18 FMT[2:0] values differently: code 001 selects 32-bit
> standard data instead of 20-bit, and code 100 selects 32-bit I2S instead
> of 16-bit I2S. It has no 20-bit format at all. The PCM1796 shares the
> PCM1792A encoding.

> +	{ .compatible = "ti,pcm1795", .data = (void *)PCM1795, },
> +	{ .compatible = "ti,pcm1796", .data = (void *)PCM1796, },
>  	{ }
>  };

>  static const struct spi_device_id pcm179x_spi_ids[] = {
>  	{ "pcm1792a", 0 },
> +	{ "pcm1795", 0 },

Don't we need to set the driver data to PCM1795 like we do for the other
DT based entries?

> +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 },

Does this line up with the constraints advertised by the driver?  LEFT_J
and S32 isn't here, but S16 is?  We're also not handling big endian.

> @@ -191,7 +241,7 @@ static struct snd_soc_dai_driver pcm179x_dai = {
>  		.rates = SNDRV_PCM_RATE_CONTINUOUS,
>  		.rate_min = 10000,
>  		.rate_max = 200000,
> -		.formats = PCM1792A_FORMATS, },
> +		.formats = PCM179X_FORMATS, },

It'd probably be easier to just register different constraints depending
on the device rather than having constraints in startup, you'd get much
better reuse of the core code that way.  Dynamic updates are usually
done for things that can vary at runtime like clock rates.

-- 
Mark Brown <broonie@xxxxxxxxxx>