[alsa-devel] [PATCH 2/6] ASoC: Blackfin: new machine driver for ADAU1361 codecs

[Mark Brown]

On 7 Aug 2010, at 21:28, Mike Frysinger <vapier at gentoo.org> wrote:

> +config SND_BF5XX_SOC_ADAU1361
> +	tristate "SoC ADAU1361 Audio support"
> +	depends on SND_BF5XX_I2S
> +	select SND_BF5XX_SOC_I2S
> +	select SND_SOC_ADAU1361
> +	select I2C
> +	help
> +	  Say Y if you want to add support for ADAU1361 SoC audio.
> +

Identical comments to the last machine driver you posted here.

It'd be really helpful if you could do a bit more pre-review on the stuff you're posting, especially where you've got a bunch of drivers that follow similar patterns and are going to generate exactly the same feedback. Posting a large batch of patches at once isn't a problem but if you're sending additional patches later on it'd be helpful to take into account the feedback that has already been given on prior serieses.

> +	switch (params_rate(params)) {
> +	case 11025:
> +	case 22050:
> +	case 44100:
> +	case 88200:
> +		pll_out = ADAU1361_PLL_FREQ_441;
> +		break;
> +	case 8000:
> +	case 12000:
> +	case 16000:
> +	case 24000:
> +	case 32000:
> +	case 48000:
> +	case 64000:
> +	case 96000:
> +		pll_out = ADAU1361_PLL_FREQ_48;
> +		break;
> +	default:
> +		pll_out = ADAU1361_PLL_FREQ_441;
> +		break;
> +	}

It seems silly for the machine drivers to all have to manually do this lookup - it appears to be entirely and inflexibly related to the sample rate in use. May as well just specify the input clock to the device.

> +	/* set codec DAI configuration */
> +	ret = codec_dai->ops->set_fmt(codec_dai,
> +		SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> +		SND_SOC_DAIFMT_CBM_CFM);
> +	if (ret < 0)
> +		return ret;

There are accessor functions for this stuff. I've stopped reviewing the code at this point.