Re: [PATCH 1/4] ASoC sst v2: Add sn95031 codec driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Tue, Jan 04, 2011 at 08:16:07PM +0530, Koul, Vinod wrote:

A few comments below; depending on what Liam feels I think we can
possibly merge this as-is on the basis that it's going to get future
updates.

> +	/* PCM interface config
> +	 * This sets the pcm rx slot conguration to max 6 slots
> +	 * for max 4 dais (2 stereo and 2 mono)
> +	 */
> +	snd_soc_write(codec, SN95031_PCM2RXSLOT01, 0x10);
> +	snd_soc_write(codec, SN95031_PCM2RXSLOT23, 0x32);
> +	snd_soc_write(codec, SN95031_PCM2RXSLOT45, 0x54);
> +	/* pcm port setting
> +	 * This sets the pcm port to slave and clock at 19.2Mhz which
> +	 * can support 6slots, sampling rate set per stream in hw-params
> +	 */
> +	snd_soc_write(codec, SN95031_PCM1C1, 0x00);
> +	snd_soc_write(codec, SN95031_PCM2C1, 0x01);
> +	snd_soc_write(codec, SN95031_PCM2C2, 0x0A);
> +	snd_soc_write(codec, SN95031_HSMIXER, BIT(0)|BIT(4));

This stuff should all be dynamically configured at runtime - the clocks
should be being managed with set_sysclk() and the slot configuration
with the TDM API or dynamic routing depending on what the actual control
is.

I guess this is OK for now, though.

> +	/* soft mute ramp time */
> +	snd_soc_write(codec, SN95031_SOFTMUTE, 0x3);

Ideally this should be user controllable.

> +	/* dac mode and lineout workaround */
> +	snd_soc_write(codec, SN95031_SSR2, 0x10);
> +	snd_soc_write(codec, SN95031_SSR3, 0x40);

DAC mode?

> +struct snd_soc_codec_driver sn95031_codec = {
> +	.probe =	sn95031_codec_probe,
> +	.remove =	sn95031_codec_remove,
> +	.read		= sn95031_read,
> +	.write		= sn95031_write,
> +	.set_bias_level	= sn95031_set_vaud_bias,
> +};

The formatting of the = should be consistent here.

> +MODULE_DESCRIPTION("ASoC Intel(R) SN95031 codec driver");

I thought you said this was a TI chip?  For example...

> + *  sn95031.h - TI sn95031 Codec driver