Re: [RFC][PATCH 2/2 v2] ASoC: simple-card: add Device Tree support

[Stephen Warren <swarren@wwwdotorg.org>]

On 01/28/2013 07:14 PM, Kuninori Morimoto wrote:
> 
> Hi Stephen
> 
> Thank you for checking path
> 
>>> +Required properties:
>>> +
>>> +- compatible					: "simple-audio"
>>> +- simple-audio,card-name			: simple-audio card name
>>> +
>>> +- simple-audio,platform,controller		: phandle for platform
>>
>> Rename that simple-audio,dma-controller perhaps? "platform" is a word
>> specific to ASoC, and the bindings really should be generic across OSs.
>>
>> But I wonder why you'd even need the ASoC platform to be specified in
>> DT; instead, the following seem better:
>>
>> a) Have the CPU DAI's driver register the platform itself. Tegra does this.
>>
>> b) Assume the ASoC "platform" device simply does DMA via a standard
>> dmaengine driber, and instead refer to the DMA controller using DMA
>> engine DT bindings.
> 
> This is the feature of this "simple-audio" driver.
> "simple-audio" produces board/SoC specific relationship between codec/cpu.
> 
> For example, we are using FSI for cpu, and AK4642/WM8978/DA7210 for codec.
> In our old style, we created fsi-ak4642, fsi-wm8978, fsi-da7210...
> This means that new fsi-xxx driver is required whenever new boards were created.
> This simple-audio was created to avoid it

I still don't understand why the ASoC platform has to be exposed in
device tree; it seems like something completely internal to the ASoC
driver. Take a look at the Tegra ASoC DT bindings; the platform isn't in
DT at all there.

>>> +- simple-audio,platform,name			: simple-audio platform name
>>
>> Can you explain why you'd need the platform name in the DT? Doesn't the
>> phandle always uniquely identify it? The example doesn't use this property.
> 
> Ahh yes, this simple-audio supports both phandle and name matching for compatibility.
> example showed phandle matching only.

I think the DT binding should only support phandle-based resolution.
Name-based resolution would be quite odd for a device tree binding.

>>> +- simple-audio,cpu,controller			: phandle for CPU DAI
>>> +- simple-audio,cpu,dai,name			: simple-audio CPU DAI name
>>
>> It'd be a bit more typical of device-tree to have a single property that
>> defines both the controller and any properties of the controller at
>> once, e.g. something like:
>>
>> simple-audio,cpu-interface = <&codec_phandle AK4648_I2S_ITF_A>;
>>
>> where we assume something like:
>>
>> #define AK4648_I2S_ITF_A 0 // Interface A's ID
>>
>> That would remove the need to put string names into the DT.
> 
> Hmm... this "name" is required on ASoC matching...
> Especially, "codec dai name" is must item.
> 
> Should I modify ASoC itself ?

When I was thinking about a more generic DT binding for audio, quite a
while ago, I certainly was planning to have each CODEC expose some kind
of "of_xlate" function that could prase the integer interface ID in
device tree, and convert it to a string name for internal use by ASoC.

>>> +simple-audio,xxx,dai,clock-gating
>>> +	"continuous"
>>> +	"gated"
>>
>> Don't you need to use the common clock bindings to define which clock to
>> gate? Or, is the I2S/... node's binding supposed to provide that
>> information?
> 
> I guess it is dependent on SoC/board.
> No ?

OK, I guess if this is I2S bus clock rather than I2S module clock, then
this is fine as-is. Not all I2S controllers can implement this though.