[alsa-devel] [patch 4/6] cs42l51: add asoc driver

[Mark Brown]

On Sat, May 15, 2010 at 05:30:02PM +0200, apatard at mandriva.com wrote:
> This patch is adding a ASoC driver for the cs42l51 from Cirrus Logic.
> Master mode and spi mode are not supported.
> 
> Signed-off-by: Arnaud Patard <apatard at mandriva.com>

This seems basically OK but there are a few things of varying severity
to fix below.  The main one is the namespacing of the constants in the
header file.

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

This really should be either chip default or user visible, but hopefully
when explicit control gets added users will be able to work out which
non-default value they were using so it'll be OK.

> +static const char *mic_boost[] = { "+16dB", "+32dB"};

> +	SOC_ENUM("Mic Boost", cs42l51_mic_boost),

This really should be a SOC_SINGLE_TLV() - that will mean that userspace
software like Pulse that tries to automate gain control over the full
path can include the gain from the boost amplifier.

> +static const char *cs42l51_dac_names[] = {"PCM->DAC",
> +	"PCM->SPE->DAC", "ADC->DAC"};

Conventionally these would have different names - something like "Direct
PCM", "DSP PCM" and "ADC" - since it looks a little nicer in the UI of
apps but it doesn't really matter since these are only ever likely to be
seen by people configuring systems and not end users.

> +#define DAPM_EVENT_PRE_POST_PMD	(SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD)

This really ought to get pushed into the main DAPM header file if it's
going to stick, though I'd be surprised to see other users so it's not
terribly urgent.

> +	if (!rates) {
> +		printk(KERN_ERR "cs42l51: could not find a valid sample rate\n");
> +		return -EINVAL;
> +	}

You could use dev_() printks based on codec->dev or dai->dev.

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

Not a big fan of chatty boots but it's no big deal.

> +#define CHIP_ID			0x1B
> +#define CHIP_REV_A		0x00
> +#define CHIP_REV_B		0x01

The macros in the header file should all be namespaced - they're likely
to collide with other things when the header is included in machine
drivers (eg, the registers for the CODEC).  Some of them are moderately
safe but things like these ones seem likely to run into trouble and it'd
be better to be consistent.