Re: [patch 3/5] cs42l51: add asoc driver

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

On Tue, May 25, 2010 at 02:22:39PM +0200, apatard@mandriva.com wrote:

> +	/* route microphone */
> +	ret = snd_soc_write(codec, CS42L51_ADC_INPUT, 0xF0);
> +	if (ret < 0)
> +		goto error_alloc;

As I said last time this really should be visible to userspace rather
than fixed function in the driver.

> +#define DAPM_EVENT_PRE_POST_PMD	(SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD)

As I said last time this should be in soc-dapm.h rather than in the
driver.

> +	SND_SOC_DAPM_DAC_E("Left DAC", "Left HiFi Playback",
> +		CS42L51_POWER_CTL1, 5, 1,
> +		cs42l51_pdn_event, DAPM_EVENT_PRE_POST_PMD),
> +	SND_SOC_DAPM_DAC_E("Right DAC", "Right HiFi Playback",
> +		CS42L51_POWER_CTL1, 6, 1,
> +		cs42l51_pdn_event, DAPM_EVENT_PRE_POST_PMD),

Your event here uses snd_soc_write() so for a stereo enable/disable
you'll do extra writes.  If you changed it to use snd_soc_update_bits()
then these would be suppressed.

> +	switch (format & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBM_CFM:
> +		cs42l51->func = MODE_MASTER;
> +		break;
> +	default:
> +	case SND_SOC_DAIFMT_CBS_CFS:
> +		cs42l51->func = MODE_SLAVE_AUTO;
> +		break;

Is the device really capable of automatically coping with mixed master
buses automatically, or should the default be to return an error?

> +	dev_info(&pdev->dev, "CS42L51 ALSA SoC Codec\n");

I really would prefer to remove this.

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

This is unused and could be removed.