On 22 April 2014 16:14, Mark Brown <broonie@xxxxxxxxxx> 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.
--
Tushar Behera
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
