On Tue, Dec 25, 2012 at 10:52:33PM -0800, Kuninori Morimoto wrote:

Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

Applied, thanks.

On 12/25/2012 11:53 PM, Kuninori Morimoto wrote:

Support for loading the simple-card module via devicetree. It requests platform/codec driver's DT blob for probing.

This binding proposal should be sent to devicetree-discuss@lists.ozlabs.org; that's where all DT bindings are dicussed.

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt

+Required properties: +for simple-card +- compatible : "asoc,simple-card"

I don't think "asoc," is a good vendor prefix; DT should strive to describe the hardware, not a particular OS's driver structure.

Perhaps "simple-audio" would be better.

+- simple,asoc,name : simple card name

What is "simple,asoc,"? I've never seen properties with two vendor prefixes before... Perhaps "simple-audio," would be better.

What kind of name? Perhaps "simple-audio,model" would be better, and more consistent with the use of the word "model" in other audio-related DT bindings? Describe it as "user-visible card name"?

+- simple,asoc,cpu : phandle for platform

"cpu" isn't a very good name; it couuld mean anything. "platform" is an ASoC-specific term. I think this would be better written as:

simple-audio,cpu-audio-controller : phandle of the CPU's audio controller

or perhaps:

simple-audio,cpu-port : phandle of the CPU's audio port

or since I assume this binding assumes I2S rather than say AC'97 or SlimBus, perhaps:

simple-audio,i2c-controller : phandle of the CPU's I2S controller

+- simple,asoc,codec : phandle for codec

CODEC should be capitalized in the plain-text description (but not DT property name). Same for DAI below.

+for platform +- simple,asoc,dai : dai name

+for codec +- simple,asoc,dai : dai name +- simple,asoc,name : codec name

Why are names needed? The existing audio-related DT bindings don't put any names into the device tree; everything binds with phandles.

+both platform/codec +- simple,asoc,daifmt,fmt : see snd_soc_of_parse_daifmt() +- simple,asoc,daifmt,clock : see snd_soc_of_parse_daifmt() +- simple,asoc,daifmt,bitclock_inversion : see snd_soc_of_parse_daifmt() +- simple,asoc,daifmt,bitclock_master : see snd_soc_of_parse_daifmt() +- simple,asoc,daifmt,frame_inversion : see snd_soc_of_parse_daifmt() +- simple,asoc,daifmt,frame_master : see snd_soc_of_parse_daifmt()

DT bindings should be self-contained, i.e. they should describe everything required to write the device tree (with the possible exception of referring to other existing manuals or documentation for the HW). In particular, they shouldn't refer to code in some specific OS. As such, please explain what all those mean here.

It is more typical to use - not _ as a word separator in DT property names.

"bitclock_master" doesn't really describe who is the master. Perhaps "cpu-bitclk-master"?

What is "clock"?

+- simple,asoc,sysclk : system clock rate

"clock-frequency" rather than "sysclk" would be more consistent with other bindings.

+Example:

+fsi_ak4642 {

• compatible = "asoc,simple-card";
• simple,asoc,name = "FSI2A-AK4648";
• simple,asoc,cpu = <&sh_fsi2>;
• simple,asoc,codec = <&ak4648>;

+};

+&i2c0 {

• ak4648: ak4648@0x12 {

• compatible = "asahi-kasei,ak4648";
  

• reg = <0x12>;
  

• simple,asoc,dai = "ak4642-hifi";
  

• simple,asoc,name = "ak4642-codec.0-0012";
  

• simple,asoc,daifmt,fmt = "left_j";
  

• simple,asoc,daifmt,bitclock_master;
  

• simple,asoc,daifmt,frame_master;
  

• simple,asoc,sysclk = <11289600>;
  

• };

+};

+&sh_fsi2 {

• simple,asoc,dai = "fsia-dai";
• simple,asoc,daifmt,fmt = "left_j";

+};

Oh, I see. Nothing in the text above implies that the DAI format properties would exist in the CODEC and I2S controller nodes. That's rather an imposition on the DT bindings for the CODEC and I2S controller; the individual bindings for those devices should define all the properties that go into those nodes, and those devices won't always be used with a "simple" sound card. It feels to me like the DAI configuration properties should be part of the sound node, not the I2S/CODEC nodes.

Hi Stephen, Mark

...

Support for loading the simple-card module via devicetree. It requests platform/codec driver's DT blob for probing.

This binding proposal should be sent to devicetree-discuss@lists.ozlabs.org; that's where all DT bindings are dicussed.

will do

...

+Required properties: +for simple-card +- compatible : "asoc,simple-card"

I don't think "asoc," is a good vendor prefix; DT should strive to describe the hardware, not a particular OS's driver structure.

Perhaps "simple-audio" would be better.

Thanks.

...

+- simple,asoc,cpu : phandle for platform

"cpu" isn't a very good name; it couuld mean anything. "platform" is an ASoC-specific term. I think this would be better written as:

simple-audio,cpu-audio-controller : phandle of the CPU's audio controller

or perhaps:

simple-audio,cpu-port : phandle of the CPU's audio port

Thank you. I would like to use "cpu-port" and "codec-port"

...

+for platform +- simple,asoc,dai : dai name

+for codec +- simple,asoc,dai : dai name +- simple,asoc,name : codec name

Why are names needed? The existing audio-related DT bindings don't put any names into the device tree; everything binds with phandles.

(snip)

Oh, I see. Nothing in the text above implies that the DAI format properties would exist in the CODEC and I2S controller nodes. That's rather an imposition on the DT bindings for the CODEC and I2S controller; the individual bindings for those devices should define all the properties that go into those nodes, and those devices won't always be used with a "simple" sound card. It feels to me like the DAI configuration properties should be part of the sound node, not the I2S/CODEC nodes.

Thank you for your advice. I modify this design.

...

+- simple,asoc,sysclk : system clock rate

"clock-frequency" rather than "sysclk" would be more consistent with other bindings.

(snip)

I think specifying the particular clock being referenced is useful here

• we may also end up wanting to specify the BCLK or sample rate due to

some hardware limitation on some system both of which are clocks too.

I will use "system-clock-frequency" here

Hi Mark

...

This patch adds snd_soc_of_parse_daifmt() and supports below style on DT.

Applied, thanks.

Thank you.

Now, I'm afraid about this snd_soc_of_parse_daifmt() patch. I created this to be possible to share to get SND_SOC_xxxx flags between all drivers on DT. But now simple-card is the only user for it, and it seems can't use this snd_soc_of_parse_daifmt() directly. (snd_soc_of_parse_daifmt() will get all flags, but the flag field seems depends on driver/platform) of course, simple-card can use it with special mask, like flag = snd_soc_of_parse_daifmt() & SND_SOC_xxx_MASK.

I'm not sure what is the best way, but if it is no-sense to have this function under asoc-core, can you remove asoc/topic/of ?

Best regards --- Kuninori Morimoto

Hi Mark

Thank you for checking patch

...

+- simple-audio,cpu,dai,clock-gating : if needed, see below

A lot of these are listed as "if needed" - this means they should be listed separately as optional properties rather than in the required properties section.

Sorry, Does this means under Documentation ? Or driver ?

...

+- simple-audio,codec,controller : phandle for CODEC DAI

It feels like this should just be simple-audio,codec - the controller is just redudnant. Though for idiomatic DT we ought to write something like

simple-audio,codec { simple-audio,dev = &phandle; simple-audio,system-clock-frequency = 122880000; };

rather than have these very long prefixes to the individual property names.

I see. will do in v3

...

+- simple-audio,codec,dai,name : simple-audio CODEC DAI name +- simple-audio,codec,dai,format : see below +- simple-audio,codec,dai,clock-gating : if needed, see below +- simple-audio,codec,dai,bitclock-inversion : if needed +- simple-audio,codec,dai,bitclock-master : if needed +- simple-audio,codec,dai,frame-inversion : if needed +- simple-audio,codec,dai,frame-master : if needed +- simple-audio,codec,dai,system-clock-frequency : system clock rate if needed

I'm also thinking that for some of the above properties which really should be the same for both ends of the link we should just specify them at the card levle and copy them over. The format and inversion mainly.

I guess it can share format between codec/dai, but it is difficult for inversion. Because it is depends on its default clock polarity. of course we can do simple-audio,platform,dai-bitlock-inversion simple-audio,platform,codec-bitlock-inversion but it is same thing.

Best regards --- Kuninori Morimoto

Hi Mark

...

Sorry, Does this means under Documentation ? Or driver ?

In the binding document.

I see.

...

I guess it can share format between codec/dai, but it is difficult for inversion. Because it is depends on its default clock polarity. of course we can do simple-audio,platform,dai-bitlock-inversion simple-audio,platform,codec-bitlock-inversion but it is same thing.

Inverted and non-inverted always mean the same thing in terms of ASoC and should always mean the same thing in terms of the device tree, if two devices are directly connected you should always be able to use the same ASoC format for them.

Ahh indeed. sorry for my confusion. I will fix it on v3

Best regards --- Kuninori Morimoto

On 01/29/2013 03:00 AM, Kuninori Morimoto wrote:

Hi Mark, Stephen

...

...

simple-audio,codec { simple-audio,dev = &phandle; simple-audio,system-clock-frequency = 122880000; };

I would like to ask you before creating v3 patch. I got some opinions from you.

• cpu/codec style (above)
• "controller" naming
• it is using "name" property (it is must item for matching)

Then, what do you think about this style ?

fai_ak4642 {

The node would usually be named after the type of object, not the identity of the object, so more like "sound".

    compatible = "simple-audio";

    simple-audio,card-name = "FSI2A-AK4648";

OK.

    /* platform */
    simple-audio,platform {

I still don't understand why you need to even represent the platform in DT.

Introducing sub-nodes seems to just add to the complexity without much gain.

         compatible = "ak4642-hifi";  /* platform name (if needed) */

The "compatible" property has a very specific meaning in DT already. I don't think we should re-use that property name for other purposes.

         simple-audio,dev = <&sh_fsi2>;

         /* format and inversion are here */
         simple-audio,format = "left_j";
         simple-audio,bitclock-inversion;

Those properties appear more relevant to the I2S link than the ASoC platform object.

    }

    /* codec and cpu are same style */

    /* codec */
    simple-audio,codec {
         compatible = "ak4642-hifi";  /* codec dai name (this is must item on ASoC) */

If you absolutely must use named-based binding here rather than the standard phandle+args style, rename that property to something more like simple-audio,dai-name.

         simple-audio,dev = <&ak4648>;
         simple-audio,bitclock-master;

         /* clock gating is here */
        simple-audio,clock-gating = "continuous";

I think this should be a Boolean property; present means yes, missing means no.

    }

    /* cpu */
    simple-audio,cpu {
         compatible = "fsia-dai"; /* cpu dai name (if needed) */
         simple-audio,dev = <&sh_fsi2>;
         simple-audio,system-clock-frequency = <11289600>;
    }

}

On 01/29/2013 10:03 PM, Kuninori Morimoto wrote:

ASoC clock gate settings are continuous/gated only. This patch decides it as bool, then, gated clock will be default. Special thanks to Stephen

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c

• snprintf(prop, sizeof(prop), "%scontinuous-clock", prefix);
• if (of_get_property(np, prop, NULL))

That should really use of_property_read_bool(), although it makes little practical difference.

Hi Stephen

Thank you for checking patch

...

+Required properties:

+- compatible : "simple-audio" +- simple-audio,card-name : simple-audio card name +- simple-audio,format : see below

+Optional properties:

+- simple-audio,bitclock-inversion +- simple-audio,frame-inversion

+CPU / CODEC DAI is represented as a sub-node. +Then, sub-node name should be "simple-audio,cpu" or "simple-audio,codec".

Why require sub-nodes? it seem over-complex.

It was Mark's idea.

...

+- simple-audio,system-clock-frequency : system clock rate +- simple-audio,frame-master +- simple-audio,bitclock-master

Aren't those a property of the CPU<->CODEC link, hence the clock frequency is identical,

This clock connection/frequency depends on SoC/board. There is a case where system clock is not connected to both cpu/codec.

system clock bit/frame --------[codec]---------> [cpu]

and the two "master" properties are the inverse of each-other in between CPU and CODEC. Specifying these right in the top-level node seems simpler.

If I put these in top-level node, it needs

simple-audio,xxx,frame-master; simple-audio,xxx,bitclock-master; xxx = cpu / codec

This is same things ?

Of course I can do like

simple-audio,frame-master = "cpu"; simple-audio,bitclock-master = "cpu";

But, this style can't use snd_soc_of_parse_daifmt()

...

+Required subnode properties: +- simple-audio,dev : phandle for CPU/CODEC

+Optional subnode properties:

+- simple-audio,dai-name : simple-audio CPU/CODEC DAI name

OK, I see those two are CPU-/CODEC-specific. However, you could easily just have different property names for the two.

I assume you didn't like my idea of not putting the DAI names into the DT, but using extra arguments after the phandles instead? Doing so would be more alike most existing DT bindings.

I'm confusing about this. What does this "different property name" mean ?

If you agree with "sub-node", then I can use below style. it can remove "dai-name" ?

ak4642-hifi { simple-audio,type = "codec"; simple-audio,dev = <&xxx>; ... }

fsia-dai { simple-audio,type = "cpu"; simple-audio,dev = <&xxx>; ... }

Best regards --- Kuninori Morimoto

On Thu, Jan 31, 2013 at 10:15:55AM -0700, Stephen Warren wrote:

In that diagram, it looks like the CPU-side simply doesn't receive a system clock at all, so it's recovering any clock by multiplying up the bit clock.

Practically speaking this is the case for most simple CPU DAIs, unless you're doing DSP or generating the clocks you just need a clock that is strictly faster than the data rate, synchronisation is irrelevant.

On 01/30/2013 06:35 PM, Mark Brown wrote:

On Wed, Jan 30, 2013 at 01:14:07PM -0700, Stephen Warren wrote:

...

...

+CPU / CODEC DAI is represented as a sub-node. +Then, sub-node name should be "simple-audio,cpu" or "simple-audio,codec".

...

Why require sub-nodes? it seem over-complex.

I suggested this because it seemed more legible to repeat the properties by having a named structure for each property rather than by repeating them all with slightly different names.

So the only need to repeat them at all was if the two DAIs needed different values for some of the properties rather than being identical (e.g. signal polarity) or precise inversions (e.g. which end is master), right?

Is that actually likely - I can understand that for e.g. plain old GPIOs board designers might end up sticking random inverters between two devices for various reasons, but surely this wouldn't happen for an I2S interface where the polarities etc. mean something relative to the other signals? I would almost argue that since the simple case is for the signals to be connected 1:1 between the two DAIs, that if a particular board doesn't conform to that, it isn't a simple-audio case. That all said, we could extend simple-audio to handle it still without duplicating everything by specifying the DAI format for the CPU DAI once in the simple-audio node, "inverting" that when configuring the CODEC DAI, and then adding extra boolean properties to the DT later in a backwards-compatible fashion, for any strange cases the driver can end up handling.

On 01/31/2013 10:11 AM, Mark Brown wrote:

On Thu, Jan 31, 2013 at 10:09:37AM -0700, Stephen Warren wrote:

...

On 01/30/2013 06:35 PM, Mark Brown wrote:

...

...

I suggested this because it seemed more legible to repeat the properties by having a named structure for each property rather than by repeating them all with slightly different names.

...

So the only need to repeat them at all was if the two DAIs needed different values for some of the properties rather than being identical (e.g. signal polarity) or precise inversions (e.g. which end is master), right?

There's more attributes that we might want to configure on the devices than just the actual DAI link. Clocks for example.

Sure, but this binding is *simple*-audio, right. If the intent is to create some more general/all-encompassing binding structure, I'd want to think about it and review it in a different way.

Hi Mark, Stephen

...

...

There's more attributes that we might want to configure on the devices than just the actual DAI link. Clocks for example.

...

Sure, but this binding is *simple*-audio, right. If the intent is to create some more general/all-encompassing binding structure, I'd want to think about it and review it in a different way.

Right, but things like the clocks are basic enough.

I agree that "simple"-audio has "simple" settings.

But, there is a board which gets some data delay via external factor. Then, it needs special inversion, especially bit clock. Without this settings, that board can't get correct data. This is maybe special case, so, I need cpu/codec special settings as "option".

If you can agree this, "simple-audio,xxx = " will be common (for cpu/codec) settings, and "simple-audio,cpu,xxx" or "simple-audio,codec,xxx" can be used as "option" settings.

Best regards --- Kuninori Morimoto

On Mon, Feb 04, 2013 at 03:23:49PM -0700, Stephen Warren wrote:

It seems extremely odd to me that the data delay would just happen to end up meaning that inverting the clock works out to magically make everything work.

Still, if there really is a use-case for this in otherwise simple audio systems, then I guess specifying all the DAI properties in sub-nodes is indeed to simplest/most-flexible way to go.

I've heard of this before when things get routed through an inverter for buffering (or sometimes a FPGA for something more fancy).

Hi Stephen

...

Current ASoC always request codec_dai_name for matching, but it can be option. This patch set codec_dai_name as option.

This seems reasonable to me. However, I don't think it helps solve the case you mentioned in your earlier email:

...

+-------------+ | FSI port A |--> ak4642 | port B |--> HDMI +-------------+

In that case there are two DAIs, and with this patch and no specified DAI name, you won't be able to control which of those two DAIs gets used by the DAI link.

In such case, first port (port A this case) is always selected.

So while I can see the utility of this patch in some cases, I think you really want to explicitly specify the DAI name (or DAI ID) in the device tree for the simple-audio use-case.

Some boards doesn't use port B. I guess port B selection can be "option" ?

Best regards --- Kuninori Morimoto

On 01/31/2013 06:05 PM, Kuninori Morimoto wrote:

Hi Stephen, Mark

...

...

+- simple-audio,dai-name : simple-audio CPU/CODEC DAI name

OK, I see those two are CPU-/CODEC-specific. However, you could easily just have different property names for the two.

I assume you didn't like my idea of not putting the DAI names into the DT, but using extra arguments after the phandles instead? Doing so would be more alike most existing DT bindings.

The reason why I need "dai-name" is that phandle can select "cpu/codec device" itself, but, it can't select "port". This port is called/controlled as "xxx_dai_name" on ASoC I think.

like this

+-------------+ | FSI port A |--> ak4642 | port B |--> HDMI +-------------+

Then, I would like to ask Stephen, Mark

Yes it can.

In most DT bindings where phandles are used, you specify both a phandle and some arguments related to that phandle. The arguments could easily encode the port ID. In other words, instead of:

simple-audio,codec = <&phandle>;

you could write:

simple-audio,codec = <&phandle PORT_NUMBER>;

where PORT_NUMBER could be 0, 1, 2, or any other integer value or sequence of integer values, which the driver for the CODEC would interpret itself, and return an ASoC-internal port name.

Hi Stephen, Mark

These are v4 of simple-card DT support

Kuninori Morimoto (3): ASoC: use list_add_tail() instead of list_add() for platform/codec/dai list ASoC: add snd_soc_of_get_port_dai_name() ASoC: simple-card: add Device Tree support

#1, #2 enable to get cpu/codec_dai_name from device node and specific port number. #3 adds simple-card.

Best regards --- Kuninori Morimoto

On 02/05/2013 03:11 AM, Kuninori Morimoto wrote:

Current ASoC is using list_add() in order to add new list to platform/codec/dai list. But it breaks added list's order. This patch use list_add_tail() instead of list_add() to solve this issue.

Do these lists have a defined order, such that changing the order does actually /break/ something?

I'm just curious what the motivation for this patch is really; is it just cosmetic?

On 02/05/2013 03:11 AM, Kuninori Morimoto wrote:

This patch adds snd_soc_of_get_port_dai_name() to get dai name from device_node and port number.

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c

+const char *snd_soc_of_get_port_dai_name(struct device_node *of_node, int port)

OK, that function prototype seems like /almost/ the right kind of thing to be doing.

"int port" should be "const struct of_phandle_args *portspec", since the port ID could be encoded into device tree with more than one cell, and the cells are uint32_t not int.

+{

• struct snd_soc_dai *dai;
• int i = 0;
• list_for_each_entry(dai, &dai_list, list) {

• if (dai->dev->of_node == of_node) {
  

• 	if (port == i)
  

• 		return dai->name;
  

Is this the reason for the previous patch? This is horribly wrong. The only way this could work is with internal knowledge of how ASoC works, yet the device tree must be written in a completely OS-/driver-agnostic fashion, purely describing hardware.

This function should be implemented as follows:

1) Find the driver for dai->dev

2) Call an "of_xlate" function on that driver, passing it the "portspec" I mentioned above; the driver will interpret the portspec, maps it to the ASoC port name, and return it.

The DT binding for each audio component must define the list of valid port IDs, and the number of cells used to encode the value.

See for example the GPIO and PWM subsystem's of_xlate functions.

• 	i++;
  

• }
  

• }
• return NULL;

+} +EXPORT_SYMBOL_GPL(snd_soc_of_get_port_dai_name);

On 02/11/2013 10:37 PM, Kuninori Morimoto wrote:

Hi Stephen

...

2. Call an "of_xlate" function on that driver, passing it the "portspec"

I mentioned above; the driver will interpret the portspec, maps it to the ASoC port name, and return it.

The DT binding for each audio component must define the list of valid port IDs, and the number of cells used to encode the value.

See for example the GPIO and PWM subsystem's of_xlate functions.

Hmm... I guess this means struct snd_soc_dai_driver or similar has .of_xlate() callback ?

Yes, something like that.

But, this simple-audio is "independent" from cpu/codec.

Well, any driver that could be used with it will need to implement the .of_xlate() callback. The simple-audio code would still be 100% independent from any CPU or CODEC, since of_xlate would be a standard interface/API that any CPU or CODEC driver could implement, and any ASoC machine driver could call.

For example, basically, we can use this simple-audio for FSI-wm8903 matching. But, this wm8903 is used on tegra too. Then, FSI-wm8903 (which use simple-audio) needs .of_xlate on wm8903 driver to get interpreted port name, but tegra-wm8903 don't need it.

Yes, .of_xlate() wouldn't have to be implemented by the WM8903 CODEC driver for the tegra-wm8903 machine driver to use the WM8903 CODEC. However, if it /was/ implemented, it wouldn't stop tegra-wm8903 from working. And later, it may help unify all the simple Tegra machine drivers into one, since they could also become CODEC-independent, and hence become unified (or at least mostly unified) code.

On 02/13/2013 06:16 PM, Kuninori Morimoto wrote:

Hi Stephen

Thank you for your explain. I think I could understand, but not 100%.

...

Well, any driver that could be used with it will need to implement the .of_xlate() callback. The simple-audio code would still be 100% independent from any CPU or CODEC, since of_xlate would be a standard interface/API that any CPU or CODEC driver could implement, and any ASoC machine driver could call.

(snip)

...

Yes, .of_xlate() wouldn't have to be implemented by the WM8903 CODEC driver for the tegra-wm8903 machine driver to use the WM8903 CODEC. However, if it /was/ implemented, it wouldn't stop tegra-wm8903 from working. And later, it may help unify all the simple Tegra machine drivers into one, since they could also become CODEC-independent, and hence become unified (or at least mostly unified) code.

I guess, this .of_xlate should has "public" interface for all drivers, not simple-card special.

Yes exactly. And this means that it always returns a particular type of object, not something different depending on who calls it; this is relevant to the discussion below.

Now, simple-card needs "dai name" from struct of_phandle_args.

Yes.

This .of_xlate can be used from other driver if there is such kind of driver. Now, as example, I assumed this "other driver" needs something other data pointer from .of_xlate here.

Well, .of_xlate is /defined/ as taking the of_phandle_args and returning the DAI name that the of_phandle_args represents. If there's ever a need to translate some of_phandle_args to something else, that should be a different function.

To that end, perhaps calling the function .of_xlate_dai_name might be more future-proof; if we ever needed to translate say a CODEC widget name, we could add a separate .of_xlate_widget_name function to do that later, with zero effect on the .of_xlate_dai_name function.

Then can .of_xlate has *void pointer for return ?

I think it would always return a string; the function parameter would be "char **dai_name".

In my check, the parameter of .of_xlate in gpio_chip and pmw has "driver specific" parameter.

It shouldn't. A GPIO driver's .of_xlate should /always/ take an of_phandle_args, and translate it to a Linux (or perhaps chip-relative) GPIO number, plus Linux GPIO flags. There should be nothing driver-specific about the data it returns. The format of the device tree cells that represent that data can be driver- (really, DT binding-) specific; the whole point of the .of_xlate function is to translate that binding-specific format into some standard internal Linux format.

My pseudo code now is below. But is this correct ??

simple-card OF has <&device port> : of_phandle_args will be used as "port" spec other driver OF has <xxxx yyy zzz> : of_phandle_args will be used as "something" spec

-- asoc -- struct snd_soc_dai_driver { ... int (*of_xlate)(struct snd_soc_dai_driver *driver, const struct of_phandle_args *spec, // driver specific spec void *data); // for return data

As I mentioned above, that last parameter should be "char **dai_name", and the function name probably of_xlate_dai_name.

   ...

}

-- MULTI .of_xlate support codec driver ---

#ifdef CONFIG_ASOC_SIMPLE_AUDIO_OF

No need to the ifdef; just provide a single definition which does one thing.

int codec_of_xlate(struct snd_soc_dai_driver *driver, const struct of_phandle_args *portspec, void *data); { /* * for simple-card which needs "dai name" * * of_phandle_args is used as "port" spec */ *data = port_to_dai_name(portspec);

Yes, that's about right. Given how simple the implementation of port_to_dai_name() is, I would simply write the code directly in the implementation of of_xlate(). I assume you were writing pseudo-code simply to avoid spelling out the implementation though.

    return 0;

} #elif CONFIG_OTHER_DIRVER_IT_NEEDS_xxx_POINTER_OF int codec_of_xlate(struct snd_soc_dai_driver *driver, const struct of_phandle_args *something_spec, void *data); { /* * for "other" driver which needs something pointer * * of_phandle_args is used as "something" spec */ *data = something_necessary_pointer(something_spec); return 0; } #else #define codec_of_xlate() NULL #endif

struct snd_soc_dai_driver codec_driver = { ... .of_xlate = codec_of_xlate, ... }

Hi Mar, Stephen

These are v6 of simple-card DT support

Kuninori Morimoto (4): ASoC: add .of_xlate_dai_name callback on struct snd_soc_dai_driver ASoC: ak4642: enable .of_xlate_dai_name on struct snd_soc_dai_driver ASoC: fsi: enable .of_xlate_dai_name on struct snd_soc_dai_driver ASoC: simple-card: add Device Tree support

Documentation/devicetree/bindings/sound/ak4642.txt | 5 + .../devicetree/bindings/sound/renesas,fsi.txt | 9 ++ .../devicetree/bindings/sound/simple-card.txt | 75 +++++++++++++ include/sound/soc-dai.h | 4 + include/sound/soc.h | 5 + sound/soc/codecs/ak4642.c | 1 + sound/soc/generic/simple-card.c | 118 +++++++++++++++++++- sound/soc/sh/fsi.c | 2 + sound/soc/soc-core.c | 40 +++++++ 9 files changed, 254 insertions(+), 5 deletions(-)

#1 adds .of_xlate_dai_name on snd_soc_dai_driver, #2,#3 add .of_xlate_dai_name support on FSI/AK4642 #4 adds simple-card DT support

Best regards --- Kuninori Morimoto

Hi Mark, Stephen

Sorry, this is self response, but...

ASoC sound driver requires CPU/CODEC drivers for probing, and each CPU/CODEC has some DAI on it. Then, "dai name matching" have been used to identify CPU-CODEC DAI pair on ASoC.

But, the "dai port number matching" is now required from DeviceTree. The solution of this issue is to replace the dai port number into dai name, and it needs some kind of .of_xlate function on each driver.

This patch adds .of_xlate_dai_name callback interface on struct snd_soc_dai_driver, very basic/common snd_soc_common_of_xlate_dai_name() which replace the dai port number into dai name, and snd_soc_of_get_port_dai_name() which is using .of_xlate_dai_name.

Then, #sound-dai-cells which enables DAI specifier is required on CPU/CODEC device tree properties.

Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

(snip)

+const char *snd_soc_common_of_xlate_dai_name(struct snd_soc_dai *dai,

• 			const struct of_phandle_args *args)
  

+{

• if (dai->id != args->args[0])

• return NULL;
  

• return dai->driver->name;

+} +EXPORT_SYMBOL_GPL(snd_soc_common_of_xlate_dai_name);

I used *common* for this function name, but *simple* or *basic* seems better name ?

Best regards --- Kuninori Morimoto

Hi Lars

Thank you for checking patch

...

• list_for_each_entry(dai, &dai_list, list) {

• if (dai->dev->of_node != args.np)
  

• 	continue;
  

• if (dai->driver->of_xlate_dai_name) {
  

• 	name = dai->driver->of_xlate_dai_name(dai, &args);
  

• 	if (name)
  

• 		return name;
  

• }
  

Hm, this is not really a translate function, but rather a match function. It iterates over all dais of the device and returns the name of the first one which matches. But there is no translation going on. If the dai matches the callback will always want to return dai->driver->name. Anything else doesn't make much sense.

I re-consider this including Stephen's opinion. Thank you

Best regards --- Kuninori Morimoto

On 02/19/2013 01:53 PM, Stephen Warren wrote:

On 02/14/2013 02:21 AM, Kuninori Morimoto wrote:

...

ASoC sound driver requires CPU/CODEC drivers for probing, and each CPU/CODEC has some DAI on it. Then, "dai name matching" have been used to identify CPU-CODEC DAI pair on ASoC.

But, the "dai port number matching" is now required from DeviceTree. The solution of this issue is to replace the dai port number into dai name, and it needs some kind of .of_xlate function on each driver.

This patch adds .of_xlate_dai_name callback interface on struct snd_soc_dai_driver, very basic/common snd_soc_common_of_xlate_dai_name() which replace the dai port number into dai name, and snd_soc_of_get_port_dai_name() which is using .of_xlate_dai_name.

Then, #sound-dai-cells which enables DAI specifier is required on CPU/CODEC device tree properties.

...

...

+const char *snd_soc_of_get_port_dai_name(struct device_node *of_node,

• 			 const char *prop)
  

+{

• struct snd_soc_dai *dai;
• struct of_phandle_args args;
• const char *name;
• int ret;
• ret = of_parse_phandle_with_args(of_node, prop,

• 			 "#sound-dai-cells", 0, &args);
  

• if (ret)

• return NULL;
  

• of_node_put(args.np);
• list_for_each_entry(dai, &dai_list, list) {

I think this should iterate over the list of CODEC drivers that are registered, and then ask the driver to translate from the args to the DAI name. Here, you're iterating over the list of registered DAIs, which is something entirely different.

Ah. I guess here is the issue:

For CODECs, ASoC actually knows about a CODEC object, so you can imagine asking the CODEC object to translate from some a DT DAI specifier to a DAI name, and then look up the DAI using the combination of CODEC plus DAI name.

However, the CPU DAIs, there is no object that contains the DAIs; only the DAIs exist. Thus, there's nothing to ask "which of your DAIs does this DT DAI specifier correspond to?".

Now presumably there is a platform device for the FSI device, just like there's a platform device for the AK4642 CODEC. Can we create an ASoC "CPU" device for the FSI device too, to act as the DAI container? Or can we alter this patch series to query the platform device rather than some ASoC device in order to do the DT DAI specifier to DAI name translation?

As an aside, I now recall this lack of symmetry from when I was working on enhancing the Tegra30 ASoC driver to support DPCM and similar; I ended up pretending that the CPU I2S interface was actually a CODEC with 1 DAI in order to model the CPU DAIs and real CODEC DAIs in a more similar fashion.

On 02/19/2013 05:59 PM, Kuninori Morimoto wrote:

Hi Stephen

...

...

...

+const char *snd_soc_of_get_port_dai_name(struct device_node *of_node,

• 			 const char *prop)
  

+{

• struct snd_soc_dai *dai;
• struct of_phandle_args args;
• const char *name;
• int ret;
• ret = of_parse_phandle_with_args(of_node, prop,

• 			 "#sound-dai-cells", 0, &args);
  

• if (ret)

• return NULL;
  

• of_node_put(args.np);
• list_for_each_entry(dai, &dai_list, list) {

I think this should iterate over the list of CODEC drivers that are registered, and then ask the driver to translate from the args to the DAI name. Here, you're iterating over the list of registered DAIs, which is something entirely different.

Ah. I guess here is the issue:

For CODECs, ASoC actually knows about a CODEC object, so you can imagine asking the CODEC object to translate from some a DT DAI specifier to a DAI name, and then look up the DAI using the combination of CODEC plus DAI name.

However, the CPU DAIs, there is no object that contains the DAIs; only the DAIs exist. Thus, there's nothing to ask "which of your DAIs does this DT DAI specifier correspond to?".

Now presumably there is a platform device for the FSI device, just like there's a platform device for the AK4642 CODEC. Can we create an ASoC "CPU" device for the FSI device too, to act as the DAI container? Or can we alter this patch series to query the platform device rather than some ASoC device in order to do the DT DAI specifier to DAI name translation?

As an aside, I now recall this lack of symmetry from when I was working on enhancing the Tegra30 ASoC driver to support DPCM and similar; I ended up pretending that the CPU I2S interface was actually a CODEC with 1 DAI in order to model the CPU DAIs and real CODEC DAIs in a more similar fashion.

Thank you for understanding my last resort :)

I don't want to create new feature (= snd_soc_register_cpu) on ASoC only for simple-card. But now, simple-card is assuming CPU device and Platform device is same one.

If my understanding was correct, struct snd_soc_platform_driver

I don't think a platform device would want an of_xlate_dai_name function; a platform device is not what provides the DAI and hence it shouldn't be involved in translating a DT DAI specifier into a DAI name.

struct snd_soc_codec_driver will have new ".of_xlate_dai_name".

Yes, putting the function into the CODEC driver makes sense.

I guess that snd_soc_register_dai() and snd_soc_register_dais() already are the equivalent of snd_soc_register_cpu(); they are already passed the struct device that implements the DAIs, and hence if you pass that struct back to the DAI driver's of_xlate_dai_name function, it should have enough information to do the mapping: the dev contains the platform data the DAI driver can use to perform the mapping.

I guess the algorithm should be:

* Find a CODEC device that was instantiated from the DT node referenced by the phandle.

* If found, ask the CODEC to translate, and use the result. This function would be passed the CODEC pointer and phandle args.

* Find any DAI, that is not part of a CODEC, that was instantiated from the DT node referenced by the phandle.

* If found, ask the driver of the DAI to translate. This function would probably have to be passed the DT node as well as the first DAI you found, plus the phandle args.

* If nothing found above, fail.

On 02/20/2013 02:04 AM, Kuninori Morimoto wrote:

Hi Stephen

...

Hmm. I think that the .of_xlate_dai_name should be something related to the object (e.g. CODEC) that contains the DAIs, rather than the DAIs themselves. The intent is to ask some object (e.g. CODEC) which of its DAIs is represented by the DT DAI specifier, not to ask each DAI whether it is represented by that particular DT DAI specifier.

Aside from that, I'd expect the patch above to be something more like:

const char *fsi_xlate_dai_name(struct snd_soc_dai *dai, const struct of_phandle_args *args) { switch (args[0]) { case 0: return fsi_soc_dai[0].name; case 1: return fsi_soc_dai[1].name; default: return NULL; } }

Thank you for pointing it. I use this style in next patch.

I don't think this makes any more sense than the current version. You still iterate over all dai instances and call the xlate function for the first dai where the _parent_ device's of_node matches. You also pass in the this dai as the first parameter to the xlate function, which makes even less sense since you can use it to lookup the correct name, which may either be the dai's name or any of its siblings.

If you want to use the xlate paradigm you really need to keep track of the cpu devices and make the xlate callback a property of the cpu device. Well or use a match paradigm instead of xlate.

- Lars

On 02/20/2013 01:48 AM, Kuninori Morimoto wrote:

Hi Lars

Thank you for checking patch

...

...

+- simple-audio,dev : phandle and port for CPU/CODEC +- simple-audio,frame-master : frame master +- simple-audio,bitclock-master : bitclock master +#sound-dai-cells integer is required on simple-audio,dev phandle's node

Shouldn't the names of '#sound-dai-cells' and 'simple-audio,dev' kind of match? E.g. '#sound-dai-cells' and 'sound-dai'. Maybe drop the sound, since the a in dai kind of implies this.

Thank you, but I would like to keep "simple-audio" name for it too. So, can I use simple-audio-dai #simple-audio-dai-cells

The requirement to be able to find the DAI via devicetree is not specific to the simple-audio driver. I'd prefer if we could come up with a common binding which can be used by multiple drivers. Since you also add the xlate (or match) function to the sound core it makes sense - in my option - to also have a generic phandle -> DAI look-up method in the core.

- Lars

...

...

+static struct device_node* +__asoc_simple_card_parse_of(struct device_node *np,

• 	    struct asoc_simple_dai *dai)
  

+{

• struct device_node *node;
• char prop[128];
• /* get "simple-audio,dev = <&phandle port>" */
• snprintf(prop, sizeof(prop), "simple-audio,dev");

Why do you need the extra buffer? Can you just pass, "simple-audio,dev" directly to of_parse_phandle?

Indeed, thank you.

...

...

• node = of_parse_phandle(np, prop, 0);
• if (!node)

• return NULL;
  

• of_node_put(node);

You shouldn't drop the reference until you are done processing it. Which in this case is only after the device has been unregistered, since you pass the node on to the ASoC core.

I see, will fix

Best regards

Kuninori Morimoto _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Hi Lars, Stephen

Thank you for your advice

...

The requirement to be able to find the DAI via devicetree is not specific to the simple-audio driver. I'd prefer if we could come up with a common binding which can be used by multiple drivers. Since you also add the xlate (or match) function to the sound core it makes sense - in my option - to also have a generic phandle -> DAI look-up method in the core.

I agree here; the whole reason I've been pushing on creating a standardized of_xlate_dai_name function and a standardized #sound-dai-cells property is to allow the exact same property name and function to be used for any audio-related DT binding.

I see. I will try it. But it will be next week

Best regards --- Kuninori Morimoto

Hi Stephen

On 02/25/2013 01:51 AM, Kuninori Morimoto wrote:

...

Current ASoC has register function for platform/codec/dai/card, but doesn't have for cpu. It often produces confusion and fault on ASoC. This patch adds very basic register function for cpu

This seems reasonable to me.

I can't recall how much influence the existing CODEC objects have on the various routing/matching decisions inside the ASoC core. While this patch does register and unregister CPU objects, I wonder if it should have more impact on any of the existing core code? Certainly I'd expect the CPU objects to show up in ASoC's debugfs and any similar files...

This patch doesn't have any impact to existing drivers. It just added new cpu list.

So as far as it goes, Reviewed-by: Stephen Warren swarren@nvidia.com

But I suspect this patch might need to do a bunch of other stuff too, or add a bunch of follow-on patches for that?

If Mark can accept this new feature (= register CPU), I think we can start to switch to use it. But current struct CPU is very very simple, and it doesn't have any effect for existing code

Best regards --- Kuninori Morimoto

On 02/28/2013 08:14 PM, Stephen Warren wrote:

On 02/27/2013 05:42 PM, Kuninori Morimoto wrote:

...

Hi Stephen

...

On 02/25/2013 01:51 AM, Kuninori Morimoto wrote:

...

Current ASoC has register function for platform/codec/dai/card, but doesn't have for cpu. It often produces confusion and fault on ASoC. This patch adds very basic register function for cpu

This seems reasonable to me.

I can't recall how much influence the existing CODEC objects have on the various routing/matching decisions inside the ASoC core. While this patch does register and unregister CPU objects, I wonder if it should have more impact on any of the existing core code? Certainly I'd expect the CPU objects to show up in ASoC's debugfs and any similar files...

This patch doesn't have any impact to existing drivers. It just added new cpu list.

Sure. My point was that now you've added the list, perhaps the core ASoC code might want to make use of it in the same way it makes use of CODEC objects, since it's logically equivalent.

In my opinion it makes sense to get rid of the distinction between CPU objects and CODEC objects in this case and instead introduce some kind of DAI container object. I mean the distinction is mostly for historic reasons from a time where it was only possible to bind a the cpu_dai of a link to a CPU DAI and the codec_dai to a CODEC DAI. But these dais both can for example be bound CODEC DAIs. I think to have a common base here would allow us to do some cleanups to the ASoC core and also avoid the duplication with snd_soc_of_get_cpu_dai_name and snd_soc_of_get_codec_dai_name which was introduced in this series.

- Lars

Hi Stephen

Thank you for your reply

...

Support for loading the simple-card module via devicetree. It requests cpu/codec information, and .of_xlate_dai_name support on each driver for probing.

...

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt

...

+Optional properties:

...

...

+- simple-audio,bitclock-inversion : bit clock inversion for both CPU/CODEC +- simple-audio,frame-inversion : frame inversion for both CPU/CODEC

I think that Mark pointed out those would be best stored in the CPU and CODEC child nodes, since random buffering/inverting on the board might make the values at the two DAIs the opposite of each-other.

Sub node has same properties too, and this is hybrid result of your/Mark comments.

If "sound node" has these properties, these will go to both CPU/CODEC, this means "cpu/codec common properties" if "cpu/codec sub node" has it, it will go to itself only. I guess "common property" is useful ?

Thank you for other comments, I will fix these in next version. But I'm busy now, so, it will be next week.

Best regards --- Kuninori Morimoto

Hi Kuninori,

On 25.02.2013 09:49, Kuninori Morimoto wrote:

Hi Mark, Stephen, Lars

These are v7 of simple-card DT support. I well considered your comments/advices. Then, I noticed that creating snd_soc_register_cpu() is better solution. It is very basic/simple version at this point, and I added [RFC] on these patches. I hope it is accepted by you, and updated to be more useful. Now, snd_soc_codec, and snd_soc_cpu has .of_xlate_dai_name() callback.

Kuninori Morimoto (5): ASoC: add snd_soc_register_cpu() ASoC: add .of_xlate_dai_name on snd_soc_cpu_cpu/codec_driver ASoC: simple-card: add Device Tree support ASoC: ak4642: add .of_xlate_dai_name support ASoC: fsi: add .of_xlate_dai_name support

May I ask what happened to this approach and the patch set?

Thanks, Daniel

Documentation/devicetree/bindings/sound/ak4642.txt | 5 + .../devicetree/bindings/sound/renesas,fsi.txt | 9 ++ .../devicetree/bindings/sound/simple-card.txt | 75 +++++++++++ include/sound/soc.h | 33 +++++ sound/soc/codecs/ak4642.c | 11 ++ sound/soc/generic/simple-card.c | 139 +++++++++++++++++++- sound/soc/sh/fsi.c | 22 +++- sound/soc/soc-core.c | 118 +++++++++++++++++ 8 files changed, 405 insertions(+), 7 deletions(-)

Best regards

Kuninori Morimoto _______________________________________________ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss

On 02/06/2013 05:55 PM, Kuninori Morimoto wrote:

Support for loading the simple-card module via devicetree. It requests cpu/codec information for probing.

+Simple-Card:

+Required properties:

• [prefix] means cpu/codec here

+- compatible : "simple-audio" +- simple-audio,card-name : simple-audio card name +- simple-audio,format : see below

+- simple-audio,[prefix],dai : phandle and port for CPU/CODEC +- simple-audio,[prefix],frame-master : frame master +- simple-audio,[prefix],bitclock-master : bitclock master

+Optional properties:

+- simple-audio,system-clock-frequency : system clock rate if it is connected to both CPU/CODEC +- simple-audio,bitclock-inversion : bit clock inversion for both CPU/CODEC +- simple-audio,frame-inversion : frame inversion for both CPU/CODEC

+- simple-audio,[prefix],bitclock-inversion : if CPU/CODEC needs clock inversion +- simple-audio,[prefix],frame-inversion : if CPU/CODEC needs frame inversion +- simple-audio,[prefix],system-clock-frequency : system clock rate for each CPU/CODEC

I know Mark had a preference to store DAI-specific data in sub-nodes rather than using this "[prefix]" thing... If that doesn't end up happening though, rather than "[prefix],", I think it'd be more typical to use "[prefix]-" since I've never seen a DT property name with two ","; "-" is typically used as the word separator in DT property names.

+sound {

• compatible = "simple-audio";

...

• simple-audio,codec,dai = <&ak4648 0>;

...

+sh_fsi2: sh_fsi2@0xec230000 {

• compatible = "renesas,sh_fsi2";
• reg = <0xec230000 0x400>;
• interrupt-parent = <&gic>;
• interrupts = <0 146 0x4>;

+};

Note that the DT binding documentation for renesas,sh_fsi2 needs to define:

a) The value of the #sound-dai-cells property that's missing from the sh_fsi2 node.

b) The legal values for the "0" in the "simple-audio,codec,dai" property in the "sound" node above, and which DAI on the device each value represents.

On 02/11/2013 09:48 PM, Kuninori Morimoto wrote:

Hi Stephen

...

...

+sound {

• compatible = "simple-audio";

...

...

• simple-audio,codec,dai = <&ak4648 0>;

...

...

+sh_fsi2: sh_fsi2@0xec230000 {

• compatible = "renesas,sh_fsi2";
• reg = <0xec230000 0x400>;
• interrupt-parent = <&gic>;
• interrupts = <0 146 0x4>;

+};

Note that the DT binding documentation for renesas,sh_fsi2 needs to define:

a) The value of the #sound-dai-cells property that's missing from the sh_fsi2 node.

b) The legal values for the "0" in the "simple-audio,codec,dai" property in the "sound" node above, and which DAI on the device each value represents.

Sorry, I couldn't understand this. Do you mean like this ?

This example is assuming like this, and shows FSI-ak4642 sound

FSI port A -- ak4642 port B -- HDMI

sh_fsi2 has #sound-dai-cells, and ak4642 doesn't have it. Does this solved a) ?

There should be a file in Documentation/devicetree/bindings that describes how to create a "renesas,sh_fsi2" node. The document should specify what value that node's #sound-dai-cells property must contain.

Do you mean sh_fsi2 needs something like "port-a = xxx" settings for b) ??

The binding documentation must specify what values go into the "DAI specifier" DT cells. Something like:

Required properties: - #sound-dai-cells Integer, must be 1. Valid values for the DAI specifier are: 0: FSI port A 1: FSI port B

sound { compatible = "simple-audio";

simple-audio,card-name = "FSI2A-AK4648"; simple-audio,format = "left_j"; simple-audio,cpu,dai = <&sh_fsi2 0>; simple-audio,codec,dai = <&ak4648>; simple-audio,codec,bitclock-master; simple-audio,codec,frame-master; simple-audio,codec,system-clock-frequency = <11289600>; };

&i2c0 { ak4648: ak4648@0x12 { compatible = "asahi-kasei,ak4648"; reg = <0x12>;

There should be a #sound-dai-cells property in this node too, since it needs to be linked into the sound node.

Does this HW only have a single DAI? If so, the value of #sound-dai-cells can be 0, since there's no need to distinguish between different DAIs.

}; };

sh_fsi2: sh_fsi2@0xec230000 { #sound-dai-cells = <2>;

Shouldn't that be 1? This is specifying the number of cells that appear after the phandle in the simple-audio,cpu,dai property above, not the number of valid values that you could put into the cell.

compatible = "renesas,sh_fsi2"; reg = <0xec230000 0x400>; interrupt-parent = <&gic>; interrupts = <0 146 0x4>; };

On 02/12/2013 06:13 PM, Kuninori Morimoto wrote:

Hi Stephen

...

...

+- simple-audio,[prefix],bitclock-inversion : if CPU/CODEC needs clock inversion +- simple-audio,[prefix],frame-inversion : if CPU/CODEC needs frame inversion +- simple-audio,[prefix],system-clock-frequency : system clock rate for each CPU/CODEC

I know Mark had a preference to store DAI-specific data in sub-nodes rather than using this "[prefix]" thing... If that doesn't end up happening though, rather than "[prefix],", I think it'd be more typical to use "[prefix]-" since I've never seen a DT property name with two ","; "-" is typically used as the word separator in DT property names.

Hmm... If I can use sub-node, I can use below style for getting dai name ? I guess it is easy/simple, but, what do you think about this ?

sound { ak4642-hifi {

If you want the "simple-audio" binding and driver to be completely generic, you had better name this node something generic like "codec".

simple-audio,codec;

Then, you wouldn't need that property to identify what type of child node this is.

simple-audio,dev = <&ak4642>;

Make that value <&ak4642 0> in order to select which AK4642 port you want to connect.

...

} fsia-dai {

Name this node something generic like "CPU".

simple-audio,cpu;
simple-audio,dev = <&sh_fsi2>;
...

} }

On 01/14/2013 07:40 PM, Kuninori Morimoto wrote:

Support for loading the simple-card module via devicetree. It requests cpu/codec/platform information for probing.

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt

+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.

+- 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.

+- 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.

+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?

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

...

+- 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.

...

+- 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 ?

...

+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 ?

Best regards --- Kuninori Morimoto

Hi Stephen, Mark

Thank you for your checking

...

...

Rename that simple-audio,dma-controller perhaps? "platform" is a word specific to ASoC, and the bindings really should be generic across OSs.

(snip)

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.

OK, I checked Tegra sounds, and it is using same device pointer for platform and cpu. My FSI is same style. But I wonder is this normal style ? >> Mark If it is very normal style, I can remove "platform" from simple-audio.

Thank you

...

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.

I see. I will remove name-base matching from simple-audio

Best regards --- Kuninori Morimoto