[alsa-devel] [PATCH 1/3] ASoC: Add WM8510 driver

[Takashi Iwai]

At Wed,  4 Jun 2008 17:33:29 +0100,
Mark Brown wrote:
> 
> +/*
> + * wm8510 register cache
> + * We can't read the WM8510 register space when we are
> + * using 2 wire for device control, so we cache them instead.
> + */
> +static const u16 wm8510_reg[WM8510_CACHEREGNUM] = {
> +    0x0000, 0x0000, 0x0000, 0x0000,

Use tabs here.

> +#define wm8510_reset(c)	wm8510_write(c, WM8510_RESET, 0)
> +
> +static const char *wm8510_companding[] = {"Off", "NC", "u-law", "A-law" };
> +static const char *wm8510_deemp[] = {"None", "32kHz", "44.1kHz", "48kHz" };
> +static const char *wm8510_alc[] = {"ALC", "Limiter" };

No space after '{' but a space before '}'?  Oh I'm too picky...

> +static int wm8510_pcm_hw_params(struct snd_pcm_substream *substream,
> +	struct snd_pcm_hw_params *params)
(snip)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_device *socdev = rtd->socdev;
> +	struct snd_soc_codec *codec = socdev->codec;
> +	u16 iface = wm8510_read_reg_cache(codec, WM8510_IFACE) & 0x19f;
> +	u16 adn = wm8510_read_reg_cache(codec, WM8510_ADD) & 0x1f1;
> +
> +	/* bit size */
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		break;
> +	case SNDRV_PCM_FORMAT_S20_3LE:
> +		iface |= 0x0020;
> +		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		iface |= 0x0040;
> +		break;
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +		iface |= 0x0060;
> +		break;
> +	}
> +
> +	/* filter coefficient */
> +	switch (params_rate(params)) {
> +	case SNDRV_PCM_RATE_8000:
> +		adn |= 0x5 << 1;
> +		break;
> +	case SNDRV_PCM_RATE_11025:
> +		adn |= 0x4 << 1;
> +		break;
> +	case SNDRV_PCM_RATE_16000:
> +		adn |= 0x3 << 1;
> +		break;
> +	case SNDRV_PCM_RATE_22050:
> +		adn |= 0x2 << 1;
> +		break;
> +	case SNDRV_PCM_RATE_32000:
> +		adn |= 0x1 << 1;
> +		break;
> +	case SNDRV_PCM_RATE_44100:
> +		break;

Just to be sure -- No 48000 case?

> +#define WM8510_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\
> +		SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
> +		SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000)

... since I find it here.

> +#define WM8510_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
> +	SNDRV_PCM_FMTBIT_S24_LE)

Don't you need SNDRV_PCM_FMTBIT_S32_LE?  It's handled in
wm8510_pcm_hw_params().


Also, is the patch author you or Liam?


thanks,

Takashi