Re: [PATCH] ASoC: add SSM2604 codec driver

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

On Sat, Aug 07, 2010 at 01:54:07AM -0400, Mike Frysinger wrote:
> From: Cliff Cai <cliff.cai@analog.com>
> 
> Signed-off-by: Cliff Cai <cliff.cai@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

This needs to be updated to the multi-component branch.  There's a few
other issues as well.

> +#define SSM2604_VERSION "0.1"

Drop this, just track the version through git and kernel releases.

> +static const struct soc_enum ssm2604_enum[] = {
> +	SOC_ENUM_SINGLE(SSM2604_APDIGI, 1, 4, ssm2604_deemph),
> +};

Use individual variables for enums rather than stuffing them all in an
array.  The arrays are just hard to read and error prone...

> +SOC_ENUM("Capture Source", ssm2604_enum[0]),
> +
> +SOC_ENUM("Playback De-emphasis", ssm2604_enum[1]),

...are you sure this code runs correctly?  I see only one element in the
array.  It's worth checking this in your other drivers as well.

> +	/* The DAI has shared clocks so if we already have a playback or
> +	 * capture going then constrain this substream to match it.
> +	 * TODO: the ssm2604 allows pairs of non-matching PB/REC rates
> +	 */
> +	if (ssm2604->master_substream) {

Set the symmetric_rates flag on the DAI rather than open coding this.

> +	/* deactivate */
> +	if (!codec->active)
> +		snd_soc_write(codec, SSM2604_ACTIVE, 0);

This probably wants to be managed by DAPM (possibly a supply widget for
the DAC and ADC).

> +#define SSM2604_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_32000 |\
> +		SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\
> +		SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)

SNDRV_PCM_RATE_8000_96000

> +	snd_soc_write(codec, SSM2604_APANA, APANA_SELECT_DAC);

This could use a little explanation?

> +struct ssm2604_setup_data {
> +	int i2c_bus;
> +	unsigned short i2c_address;
> +};

This is definitely not needed, even with the current code.