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

[Timur Tabi <timur@freescale.com>]

Mark Brown wrote:

> It's adding a new machine driver, that generally means a new binding.

Not for the way I defined the SSI bindings.  All of the information is
stored in the SSI and codec nodes.  That's why I did it that way.  There
is no binding for the machine driver.

> If it's really an identical binding then it ought to at least add a new
> compatible property to the existing binding (and extend the set of
> supported CODECs for that binding).

Here's the device tree:

http://git.kernel.org/?p=linux/kernel/git/galak/powerpc.git;a=blob;f=arch/powerpc/boot/dts/p1022rdk.dts;h=51d82de223f37c12dfbb33906cac89ce551abcd2;hb=34f84b5b5bc83f4fc208cc278f572e6d926f976b

The only thing that's new is a node for the wm8960.  I could create a
binding for that.  I asked you in a previous email about the binding for
that codec, but you never replied.

> Personally given the thing with the interface formats and the fact that
> it's not sharing code with the other bindings for implementation of the
> binding I'd be inclined to just document it as a new binding and use it
> as the model going forwards, leaving the older bindings as legacy.

Again, the only new binding is the codec.

>>>> +	/* 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.
> 
> You should just be able to drop the call to set_pll(), I expect you'll
> find that all it does is consume extra power.

Ok, I'll try it.  I have no way to measure power consumption, however.

>> I almost never do this, and there are plenty of cases in my existing code 
>> where I don't.  Is this a new policy?
> 
> It's not new, it's always been good practice - if something fails we
> should do as much as possible to tell the user why.

Ok.

>> 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?
> 
> Well, the existing bindings are out there so can't really be changed but
> we should do something better going forwards.

Fair enough.  I created those bindings because, back then, I had no idea
whether they would be needed or not.  Now that I've created three boards
that use the SSI, I see that it's not helpful.  I can fix that.

-- 
Timur Tabi
Linux kernel developer at Freescale