Re: [PATCH] ASoC: SAMSUNG: Add sound card driver for Snow board

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Apr 22, 2014 at 01:33:54PM +0530, Tushar Behera wrote:

> Added machine driver to instantiate I2S based sound card on Snow
> board. It has MAX98095 audio codec on board.

In general this isn't up to modern standards, please do try to check
that new code is following best practices.  Did the support for setting
the clocking up in the device tree get merged already?

Please do also pay attention to the CC lists when posting patches, this
seems to have been sent to a fairly random selection of people and
lists.

> +	switch (params_format(params)) {

params_width().

> +	ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
> +					     SND_SOC_DAIFMT_NB_NF |
> +					     SND_SOC_DAIFMT_CBS_CFS);

Set this in the dai_link.

> +	ret = snd_soc_dai_set_sysclk(codec_dai, 0,
> +					FIN_PLL_RATE, SND_SOC_CLOCK_IN);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = snd_soc_dai_set_sysclk(cpu_dai, SAMSUNG_I2S_CDCLK,
> +					0, SND_SOC_CLOCK_OUT);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = snd_soc_dai_set_clkdiv(cpu_dai, SAMSUNG_I2S_DIV_BCLK, bfs);
> +	if (ret < 0)
> +		return ret;

Set this stuff up on probe.  I'm surprised that you need to set BCLK at
all...

> +static int snow_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct snd_soc_codec *codec = rtd->codec;
> +	struct snd_soc_dapm_context *dapm = &codec->dapm;
> +
> +	snd_soc_dapm_sync(dapm);
> +
> +	return 0;
> +}

This does nothing, remove it.

> +	if (!pdev->dev.platform_data && !pdev->dev.of_node) {
> +		dev_err(&pdev->dev, "No platform data supplied\n");
> +		return -EINVAL;
> +	}
> +
> +	name = of_get_property(pdev->dev.of_node, "card-name", NULL);
> +	if (name)
> +		card->name = name;
> +
> +	i2s_node = of_parse_phandle(pdev->dev.of_node,
> +				    "samsung,i2s-controller", 0);
> +	if (!i2s_node) {
> +		dev_err(&pdev->dev,
> +			"Property 'i2s-controller' missing or invalid\n");
> +		return -EINVAL;
> +	}

You're allowing platform data above but I see DT is mandatory here.

> +	ret = snd_soc_register_card(card);
> +	if (ret) {
> +		dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
> +		return ret;
> +	}

devm_snd_soc_register_card().

Attachment: signature.asc
Description: Digital signature


[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux