[alsa-devel] [RFC PATCH] ARM ASoC: add sound driver for imx27pdk using mc13783 codec

[Mark Brown]

On Fri, Jul 08, 2011 at 03:24:51PM +0200, J??rgen Lambrecht wrote:

Please read and try to follow the guidelines in SubmittingPatches:
- Provide a sensible changelog for your patch
- Send copies to the relevant maintainers
- Word wrap within 80 columns
- Follow the kernel coding style.

>  
>  if SND_IMX_SOC
>  
> -config SND_MXC_SOC_SSI
> -	tristate
> -

This appeears to be unrelated to adding new machine support and should
be a separate patch.

> +config SND_SOC_IMX_MC13783
> +	tristate
> +

Err...  what is this for?

>  config SND_MXC_SOC_WM1133_EV1
>  	tristate "Audio on the the i.MX31ADS with WM1133-EV1 fitted"
>  	depends on MACH_MX31ADS_WM1133_EV1 && EXPERIMENTAL
>  	select SND_SOC_WM8350
> -	select SND_MXC_SOC_SSI
>  	select SND_MXC_SOC_FIQ
>  	help

Again, this and all the other similar hunks are totally unrelated to
adding a machine.

> +config SND_SOC_IMX27_MC13783
> +	tristate "SoC Audio support for i.MX27 platforms with a MC13783 codec"
> +	depends on MACH_MX27_3DS

The dependency and the text disagree with each other...

> +	/* set codec and cpu DAI configuration */
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		ret = snd_soc_dai_set_fmt(codec_dai, FMT_PLAYBACK);
> +		if (ret) {
> +			pr_err("%s: failed set codec dai format\n", __func__);
> +			return ret;
> +		}
> +		ret = snd_soc_dai_set_fmt(cpu_dai, FMT_PLAYBACK);
> +		if (ret) {
> +			pr_err("%s: failed set cpu dai format\n", __func__);
> +			return ret;

Can we have some blank lines for legibility please?

> +		ret = snd_soc_dai_set_tdm_slot(codec_dai, 0, 0, 4, 32); /* args are not used in mc13783 */
> +		if (ret)
> +			return ret;
> +	}

If the arguments are not used why are you supplying them, or even
calling this function?

> +//ok: checked; we use clia input pin; last arg is not used
> +	ret = snd_soc_dai_set_sysclk(codec_dai, MC13783_CLK_CLIA, 26000000, SND_SOC_CLOCK_OUT);
> +	if (ret) {
> +		pr_err("%s: failed setting codec sysclk\n", __func__);
> +		return ret;
> +	}

Coding style here and elsewhere.  You're also rather inconsistent about
error reporting.

> +static int imx_mc13783_audio_hifi_hw_free(struct snd_pcm_substream *substream)
> +{
> +	return 0;
> +}

Omit empty functions.

> +static int __init imx_mc13783_audio_init(void)
> +{
> +	int ret;
> +
> +	pr_info("IMX MC13783 Sound Card\n");

This is needlessly chatty.

> +	imx_mc13783_snd_device = platform_device_alloc("soc-audio", -1);
> +	if (!imx_mc13783_snd_device)
> +		return -ENOMEM;

Don't use soc-audio, use snd_soc_register_card().