On 22 April 2014 16:14, Mark Brown broonie@kernel.org wrote:
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?
I didn't get this point. Would you please elaborate?
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.
Okay, I will update the CC list as per get_maintainer script during next revision.
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.
Ok.
However, setting this in dai_link is not working if I2S is running is master mode. The bug is with I2S driver as it is setting rclk_srcrate as 0 during startup. I will fix that in another patch.
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...
Should I create a late_probe call for this (in line with tobermory.c)?
Setting BCLK was an oversight, that is not required. I will remove that.
+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.
Sure.
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.
I will keep this as DT only, will remove the above check for platform data.
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().
Ok.
Thanks for reviewing.