[alsa-devel] [PATCH 5/5] ASoC: add support for the Freescale / iVeia P1022 RDK reference board

[Tabi Timur-B04825]

Mark Brown wrote:
> On Fri, Sep 14, 2012 at 04:14:38PM -0500, Timur Tabi wrote:
>
>>   sound/soc/fsl/Kconfig     |   14 ++
>>   sound/soc/fsl/Makefile    |    4 +
>>   sound/soc/fsl/p1022_rdk.c |  455 +++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 473 insertions(+), 0 deletions(-)
>>   create mode 100644 sound/soc/fsl/p1022_rdk.c
>
> This adds a new device tree binding so it should be documented.

It does?  Where?  It uses the same binding as p1022_ds.c.

>> +	/* Tell the codec driver what the serial protocol is. */
>> +	ret = snd_soc_dai_set_fmt(rtd->codec_dai, mdata->dai_format);
>> +	if (ret < 0) {
>> +		dev_err(dev, "could not set codec driver audio format\n");
>> +		return ret;
>> +	}
>
> This should normally be done via the dai_fmt member of the dai_link
> structure.

Ok, that's new.  The other PowerPC SSI drivers do it this way.

>> +	ret = snd_soc_dai_set_pll(rtd->codec_dai, 0, 0, mdata->clk_frequency,
>> +		mdata->clk_frequency);
>> +	if (ret < 0) {
>> +		dev_err(dev, "could not set codec PLL frequency\n");
>> +		return ret;
>> +	}
>
> We never stop the PLL - normally you'd want to stop it when the audio
> stops.  But..
>
>> +	/* Tell the WM8960 to set SYSCLK equal to MCLK input (bypass PLL) */
>> +	ret = snd_soc_dai_set_clkdiv(rtd->codec_dai, WM8960_SYSCLKDIV,
>> +		WM8960_SYSCLK_MCLK | WM8960_SYSCLK_DIV_1);
>> +	if (ret < 0) {
>> +		dev_err(dev, "could not set codec driver clock source\n");
>> +		return ret;
>> +	}
>
> ...we don't appear to use the PLL output anyway so why not just leave it
> off?  As this is fixed it's better style to do the setup in late_init()
> or the init function for the DAI link.

I just copied this code from iVeia BSP.  I'm not really sure what it does.

> Also we should log the return codes when we log failures.

You mean like this:

if (ret < 0) {
	dev_err(dev, "could not set codec driver clock source (ret=%i)\n", ret);
	return ret;
}

I almost never do this, and there are plenty of cases in my existing code 
where I don't.  Is this a new policy?

>> +	/* Get the serial format and clock direction. */
>> +	sprop = of_get_property(np, "fsl,mode", NULL);
>> +	if (!sprop) {
>> +		dev_err(&pdev->dev, "fsl,mode property not found\n");
>> +		ret = -EINVAL;
>> +		goto error;
>> +	}
>> +
>> +	if (strcasecmp(sprop, "i2s-slave") == 0) {
>> +		mdata->dai_format = SND_SOC_DAIFMT_NB_NF |
>> +			SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM;
>> +		mdata->codec_clk_direction = SND_SOC_CLOCK_OUT;
>> +		mdata->cpu_clk_direction = SND_SOC_CLOCK_IN;
>
> Why would this be something that individual systems would want to
> change, and if it is something it's useful to vary why not at runtime?
> I suspect the best thing here is just to pick a format and use it, if
> there is a reason to vary this it's probably a higher level thing.

This is the same code as my other two machine drivers that I've been using 
for years.  It's part of the SSI binding.  Do you want me to change the 
binding and drop the properties?

I understand what you're saying, and you're probably right that it's not 
really a configuration option.  But it's a change in the binding, so I 
can't just drop it here.  I have to fix the other two machine drivers.

>
>> +	snd_soc_unregister_card(card);
>> +	kfree(mdata);
>
> devm_kzalloc().

Ok.

-- 
Timur Tabi
Linux kernel developer at Freescale