On 01/07/15 17:08, Jean-Francois Moine wrote:

On Wed, 07 Jan 2015 14:39:13 +0000 Andrew Jackson Andrew.Jackson@arm.com wrote:

...

...

• • audio-ports: must contain one or two values selecting the source
• in the audio port.
• The source type is given by the corresponding entry in
• the audio-port-names property.

I think that this entry might benefit from a little more explanation. The value specified here selects which pins on the chip provide the audio input doesn't it? In the outline datasheet that I have these are listed in table 17:

Audio port Input configuration S/PDIF I2S-bus AP0 - WS (word select) AP1 S/PDIF input I2S-bus channel 0 AP2 S/PDIF input I2S-bus channel 1 AP3[1] I2S-bus channel 2 AP4[1] I2S-bus channel 3 ACLK - SCK (I2S-bus clock)

[1] Depending on package.

Your table is close to the one in the TDA9983B documentation I have, but the pins are not exactly the same:

AP0 WS (word select) AP1 I2S-bus port 0 AP2 I2S-bus port 1 AP3 I2S-bus port 2 AP4 I2S-bus port 3 AP5 MCLK (master clock for S/PDIF) AP6 S/PDIF input AP7 AUX (internal test) ACLK SCK (I2S-bus clock)

That's why I did not know clearly why I had to set AP2 for S/PDIF input and (AP0 + AP1) for I2S input in the Cubox.

Then, the only more explanation I could give is "have a look at the audio input format and at the register 0x1e page 0 in the documentation of the TDA998x chip".

BTW, the tda998x driver supports only the TDA9989, TDA19988 and TDA19989 chips. If the TDA9983B would be supported, the audio port definitions would be of no use.

So, what would you see as an explanation?

I understand your difficulty! I was just wanting something to clarify the meaning of the value without reference to the driver source.

You could add something like this to your existing explanation: "The value describes which audio input pins are selected; this varies depending on chip type so consult the section on audio port configuration in the relevant datasheet.".

Andrew

On Thu, Jan 08, 2015 at 05:42:57PM +0100, Jean-Francois Moine wrote:

Examples:

• for the Cubox:

audio-inputs = "i2s", "spdif";

• for some other board with I2S on the pins 3 and 4 only:

audio-inputs = "none", "none", "i2s", "i2s";

• for a fully wired TDA9983B (no driver yet):

audio-inputs = "i2s", "i2s", "i2s", "i2s", "spdif";

I think that mostly works, though I do wonder if we need a way to specify the ordering of the pins (if you can make pin 3 be the first two I2S channels for example)? Someone might choose a strange mapping for board routing reasons for example.

On 01/08/2015 06:42 PM, Jean-Francois Moine wrote:

On Thu, 8 Jan 2015 16:53:41 +0200 Jyri Sarha jsarha@ti.com wrote:

...

...

• • audio-ports: must contain one or two values selecting the source
• in the audio port.
• The source type is given by the corresponding entry in
• the audio-port-names property.

This binding does not allow multi channel i2s setups with multiple i2s pins. It would be nice to support that in the DT binding, even if the code is not yet ready for it.

How about having these two optional properties instead of audio-ports and audio-port-names:

audio-port-i2s: Upto 4 values for selecting pins for i2s port audio-port-spdif: Value for selecting input pin for spdif port

Presence of one of the properties would be mandatory and both are allowed.

Sorry to notice this only now, but I have not yet looked the drm side changes too closely.

From Andrew's datasheet, the TDA998x's which are handled by the tda998x driver have only 4 input audio pins, the first two ones being either S/PDIF or I2s, the last ones being I2S only.

AFAIK, SPDIF is always a single pin connection so only one pin needs to be selected. But i2s need for pins for full 8 channel output.

So, the DT description could be reduced to a simple list indexed by the pin number (= DAI number) and defining the protocol type.

Examples:

• for the Cubox:

  audio-inputs = "i2s", "spdif";

• for some other board with I2S on the pins 3 and 4 only:

  audio-inputs = "none", "none", "i2s", "i2s";

• for a fully wired TDA9983B (no driver yet):

  audio-inputs = "i2s", "i2s", "i2s", "i2s", "spdif";

If you want to go closer to the original paradigm, then how about defining following audio-port-names: i2s0, i2s1, i2s2, i2s3, and spdif. With this approach we could go with your original binding with only minor changes. A binding in your stereo i2s or spdif case would look like this:

audio-ports = <0x04>, <0x03>; audio-port-names = "spdif", "i2s0";

A full 8 channel i2s or spdif output would look like this:

audio-ports = <0x04>, <0x03>, <0x02>, <0x01>, <0x00>; audio-port-names = "spdif", "i2s0", "i2s1", "i2s2", "i2s3";

This would also indicate the channel mapping to the audio pins (i2s0 for first two channels, i2s1 for 3-4, etc.)

The code could for now just look for "i2s0" and the port names for channels 3-8 could be ignored until they are needed.

Best regards, Jyri

On Fri, Jan 09, 2015 at 12:30:36PM +0100, Jean-Francois Moine wrote:

In my original version, the audio-ports are a bitmap of the pins, the bit 0 being the WS used for I2S. A fully wired tda998x would have been as:

audio-ports = <0x03>, <0x04>, <0x09>, <0x11>; audio-port-names = "i2s", "spdif", "i2s", "i2s";

With the new version, it would simply become:

audio-inputs = "i2s", "spdif", "i2s", "i2s";

How do you know which i2s inputs to enable?

Does it make sense for the audio inputs to be mixed like that?

You will need to enable one i2s for front L+R, and increasingly others for the additional channels.

I think we need to understand exactly how the 998x map I2S inputs to the HDMI channels to avoid making a mistake with the binding; remember, the binding isn't something that can be easily "bug fixed" at a later date as anything we come up with now has to be supported long term by the kernel.

On Fri, Jan 09, 2015 at 01:54:01PM +0100, Jean-Francois Moine wrote:

On Fri, 9 Jan 2015 11:45:29 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:

...

I think we need to understand exactly how the 998x map I2S inputs to the HDMI channels to avoid making a mistake with the binding; remember, the binding isn't something that can be easily "bug fixed" at a later date as anything we come up with now has to be supported long term by the kernel.

The DT describes the hardware configuration.

You're missing my point.

How does the driver know which of the I2S pins to enable in I2S mode?

A fully wired tda998x could be a chip with the audio pins connected to:

• a kirkwood-like audio device with one I2S and one S/PDIF output,
• two other audio devices with one I2S each.

I don't think it's that simple. Since there is only one WS input to the 998x, the four I2S sources would need to synchronise somehow, and since the I2S source generates WS, connecting the 998x to multiple independent I2S sources, each with their own WS output, presents a hardware problem.

With S/PDIF, only one stereo channel may be sent, but with I2S, up to 4 stereo channels may be sent.

That statement is not entirely accurate. Yes, with S/PDIF, only one PCM L+R channel can be sent. However, S/PDIF also supports sending more channels using "non-audio" streams, with the data encoded as MPEG or AAC. This uses the HDMI data islands which would've been occupied by the Front L+R PCM channel.

These channels are extracted by the devices connected on the HDMI bus.

That's incorrect. Please read the HDMI 1.3 specification; channels are allocated for Front L+R, Centre, Sub, Rear L+R and there's no identification to indicate that there are two Front L+R channels which are different languages.

If you feel differently, please provide a reference to the HDMI specification which describes this feature.

An example could be the playing of a multi-language movie: each audio channel carries one language. From the computer view, the playing application sends each language to one sound card.

The selection of the language is done at the player, not by the display device.

So, this means that the tda998x driver should check that S/PDIF and I2S are not active at the same time, and it should also do a pin OR/AND on I2S start/stop.

That's correct: the TDA998x can operate in one of two modes: either S/PDIF _or_ I2S, but never both.

My question is: how do we know which I2S inputs to enable, or are you suggesting that all I2S inputs should be enabled if operating in I2S mode irrespective of whether they may be active?

On Fri, Jan 09, 2015 at 01:58:37PM +0000, Andrew Jackson wrote:

On 01/09/15 13:07, Russell King - ARM Linux wrote:

...

On Fri, Jan 09, 2015 at 01:54:01PM +0100, Jean-Francois Moine wrote:

...

On Fri, 9 Jan 2015 11:45:29 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:

...

I think we need to understand exactly how the 998x map I2S inputs to the HDMI channels to avoid making a mistake with the binding; remember, the binding isn't something that can be easily "bug fixed" at a later date as anything we come up with now has to be supported long term by the kernel.

The DT describes the hardware configuration.

You're missing my point.

How does the driver know which of the I2S pins to enable in I2S mode?

[snip]

...

My question is: how do we know which I2S inputs to enable, or are you suggesting that all I2S inputs should be enabled if operating in I2S mode irrespective of whether they may be active?

Isn't it the case that for I2S:

• Word Select (WS) is always required to disambiguate left/right. So WS need not be specified in any device tree configuration.

Yep.

• The audio inputs depend on a particular board but at least one is required. Fortunately, the TDA998x devices have all the same pin/input numbering so they /could/ be described as i2s0, i2s1, i2s2 and i2s3 as per J-F's earlier email. But tt isn't clear from my reading of the TDA19988 datasheet (for example) whether one can skip channels (so does channel 0 always have to be present?). If the channels must be enabled from 0 then one could simply specify the number of inputs. (The datasheets that I have don't indicate whether there's any channel remapping performed internally by the TDA998x).

Well, if we look at the HDMI specs, there is a field in the audio info frame (CA bits) which identifies the speaker allocation between the HDMI PCM channels and the physical speakers.

It describes a limited set - essentially though, channel 0 is always front left, and channel 1 is always front right. Channel 2 is always LFE, and channel 3 is always front centre.

Channel 0 and 1 are always allocated, but it's possible to have channels above 2 to be allocated independently of each other (eg, you could have 7, 6, 1, 0 allocated to front right centre, front left centre, right, left speakers - in that order.)

As you point out, we don't have documentation which tells us know how these PCM channels map to I2S inputs.

What we do know is that there is a fixed mapping between AP pins and I2S channels (which are not PCM channels), but as you point out, we don't have any documentation which describes how the I2S channels (each with their own L+R words) map to the PCM channels - and we don't know whether the CA_I2S bits in that same register in the TDA998x have an effect on this.

Does anyone have a TDA998x setup which has an I2S source connected to the TDA998x I2S channel 1, and who has a HDMI sink which will accept the LFE/FC channels? If so, producing a description of how the CA_I2S bits and enabling I2S input pins influences the mapping would be a good idea.

If we don't have that, I'd recommend splitting the DT property into "audio inputs for I2S" and "audio input for SPDIF" (only one can be active with SPDIF).

If we want to support more than one SPDIF input (which must be mutually exclusive) I'd recommend to look at the OF graph stuff we use in DRM - one port for each "mode" - eg, I2S, SPDIF in on AP2, SPDIF in on AP3. Each port node can specify the AP pins which should be enabled.

On Fri, Jan 09, 2015 at 06:38:57PM +0100, Jean-Francois Moine wrote:

On Fri, 9 Jan 2015 14:57:41 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:

...

What we do know is that there is a fixed mapping between AP pins and I2S channels (which are not PCM channels), but as you point out, we don't have any documentation which describes how the I2S channels (each with their own L+R words) map to the PCM channels - and we don't know whether the CA_I2S bits in that same register in the TDA998x have an effect on this.

HDMI talks about LPCM (Linear PCM) channels and TDA19988 talks about I2S-bus (stereo) channels. For me, it seems obvious that these channels are correlated:

• LPCM-0 is I2S-bus-0-left (FL)
• LPCM-1 is I2S-bus-0-right (FR)
• LPCM-2 is I2S-bus-1-left (LFE)
• LPCM-3 is I2S-bus-1-right (FC)

...

That's certainly a reasonable possibility, but we don't have a way to confirm it as I don't think anyone has access to a setup which uses I2S bus 1.

...

Does anyone have a TDA998x setup which has an I2S source connected to the TDA998x I2S channel 1, and who has a HDMI sink which will accept the LFE/FC channels? If so, producing a description of how the CA_I2S bits and enabling I2S input pins influences the mapping would be a good idea.

I could not find a description of these CA_I2S bits.

I'm willing to bet that when the audio is configured for layout 1, CA_I2S affects the mapping of individual I2S channels (what I call "words") to the HDMI channels.

...

If we don't have that, I'd recommend splitting the DT property into "audio inputs for I2S" and "audio input for SPDIF" (only one can be active with SPDIF).

If we want to support more than one SPDIF input (which must be mutually exclusive) I'd recommend to look at the OF graph stuff we use in DRM - one port for each "mode" - eg, I2S, SPDIF in on AP2, SPDIF in on AP3. Each port node can specify the AP pins which should be enabled.

I agree. There are one S/PDIF port and one I2S port.

So, which syntax?

I proposed:

audio-ports = <0x04>, <0x03>; audio-port-names = "spdif", "i2s";

Do you better like:

audio-spdif-port = <0x04>; audio-i2s-port = <0x03>;

Neither; we know that there are TDA998x devices which allow SPDIF to be input via two separate pins, but only one to be active at any one time. Given that the hardware is flexible in that regard, a binding which artificially restricts that flexibility would seem unwise.

If we were to come across a setup which did route two SPDIF streams to the TDA998x, and we had to make the decision at run time which to route to the HDMI sink, we'd have to rework the binding, and we'd have to support the new binding and the old binding in the driver.

Can you please look at Documentation/devicetree/bindings/graph.txt ?

I think we may be able to use something like this:

tda998x: hdmi-encoder { compatible = "nxp,tda998x"; reg = <0x70>; video-ports = <0x234501>;

port { tda998x_video: endpoint { remote-endpoint = <&lcd0_rgb>; }; };

port { #address-cells = <1>; #size-cells = <0>;

tda998x_spdif0: endpoint@02 { reg = <0x02>; remote-endpoint = <&spdif0>; };

tda998x_spdif1: endpoint@04 { reg = <0x04>; remote-endpoint = <&spdif0>; };

tda998x_i2s: endpoint@03 { reg = <0x03>; remote-endpoint = <&i2s>; }; }; };

I'm adding Philipp Zabel for comment. The issue I see with this is that we have two ports here which are not mutually exclusive, and it's not obvious how they are dealt with.

Am Freitag, den 09.01.2015, 20:01 +0000 schrieb Russell King - ARM Linux: [...]

Neither; we know that there are TDA998x devices which allow SPDIF to be input via two separate pins, but only one to be active at any one time. Given that the hardware is flexible in that regard, a binding which artificially restricts that flexibility would seem unwise.

If we were to come across a setup which did route two SPDIF streams to the TDA998x, and we had to make the decision at run time which to route to the HDMI sink, we'd have to rework the binding, and we'd have to support the new binding and the old binding in the driver.

Can you please look at Documentation/devicetree/bindings/graph.txt ?

I think we may be able to use something like this:

tda998x: hdmi-encoder { compatible = "nxp,tda998x"; reg = <0x70>; video-ports = <0x234501>;

port { tda998x_video: endpoint { remote-endpoint = <&lcd0_rgb>; }; };

port { #address-cells = <1>; #size-cells = <0>;

tda998x_spdif0: endpoint@02 {
	reg = <0x02>;
	remote-endpoint = <&spdif0>;
};

tda998x_spdif1: endpoint@04 {
	reg = <0x04>;
	remote-endpoint = <&spdif0>;
};

tda998x_i2s: endpoint@03 {
	reg = <0x03>;
	remote-endpoint = <&i2s>;
};

}; };

I'm adding Philipp Zabel for comment. The issue I see with this is that we have two ports here which are not mutually exclusive, and it's not obvious how they are dealt with.

Jean-Francois' reply already reflects this, but the 'port' nodes should correspond to physical ports of the device if possible. If you can configure the device to have dedicated input pins for I2S, SPDIF0, and SPDIF1 at the same time, they should appear in the device tree as separate ports:

tda998x: hdmi-encoder { port@0 { /* pixel data according to video-ports */ reg = <0x00>; }; port@1 { /* AP1: SPDIF0 */ reg = <0x01>; }; port@2 { /* AP2: SPDIF1 */ reg = <0x02>; }; port@3 { /* AP3: I2S */ reg = <0x03>; }; };

The tda998x binding would define how the ports are numbered, some correspondence to the AP pin numbers would be good.

regards Philipp

Am Montag, den 12.01.2015, 12:25 +0000 schrieb Russell King - ARM Linux:

On Mon, Jan 12, 2015 at 10:25:28AM +0100, Philipp Zabel wrote:

...

Jean-Francois' reply already reflects this, but the 'port' nodes should correspond to physical ports of the device if possible. If you can configure the device to have dedicated input pins for I2S, SPDIF0, and SPDIF1 at the same time, they should appear in the device tree as separate ports:

tda998x: hdmi-encoder { port@0 { /* pixel data according to video-ports */ reg = <0x00>; }; port@1 { /* AP1: SPDIF0 */ reg = <0x01>; }; port@2 { /* AP2: SPDIF1 */ reg = <0x02>; }; port@3 { /* AP3: I2S */ reg = <0x03>; }; };

The tda998x binding would define how the ports are numbered, some correspondence to the AP pin numbers would be good.

It's not quite that simple, because the SPDIF AP pins are multiplexed with the I2S pins - and there is variation between chip models and packages.

So, it's probably best if port@0 is the video port, and then port@1..n can describe the audio inputs, including a property which specifies whether they are I2S or SPDIF, and the value to be programmed into the AP enable register (which is a bit field of the AP pins which should be unmasked.) I guess we can re-use the reg= property for that value, since video will always be zero.

Note that of_graph_parse_endpoint interprets the port node's reg property as port id. And the unit address part of the node name should match the first address in the reg property.

regards Philipp

On Mon, 12 Jan 2015 14:04:56 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:

On Mon, Jan 12, 2015 at 02:59:57PM +0100, Philipp Zabel wrote:

...

Am Montag, den 12.01.2015, 12:25 +0000 schrieb Russell King - ARM Linux:

...

It's not quite that simple, because the SPDIF AP pins are multiplexed with the I2S pins - and there is variation between chip models and packages.

So, it's probably best if port@0 is the video port, and then port@1..n can describe the audio inputs, including a property which specifies whether they are I2S or SPDIF, and the value to be programmed into the AP enable register (which is a bit field of the AP pins which should be unmasked.) I guess we can re-use the reg= property for that value, since video will always be zero.

Note that of_graph_parse_endpoint interprets the port node's reg property as port id. And the unit address part of the node name should match the first address in the reg property.

This is not the case in vexpress-v2p-ca15_a7.dts.

So that's not going to work very well... because the AP register is a bitmask.

I guess we could specify a node unit and reg, which the code otherwise ignores, and specify a philipps,ap-mask = property for the audio ports instead.

Also, the video and audio ports must be distinguished. They could be defined in different port groups.

Example from the Cubox:

video-ports: ports@0 { port { tda998x_video: endpoint { remote-endpoint = <&lcd0_0>; nxp,video-port = <0x230145>; }; }; }; audio-ports: ports@1 { port@0 { /* AP1 = I2S */ tda998x_i2s: endpoint@0 { remote-endpoint = <&audio1_i2s>; nxp,audio-port = <0x03>; }; }; port@1 { /* AP2 = S/PDIF */ tda998x_spdif: endpoint@1 { remote-endpoint = <&audio1_spdif1>; nxp,audio-port = <0x04>; }; }; };

The port type is identified by the bit AP0.

On Mon, 12 Jan 2015 17:57:06 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:

I don't particularly like that - that makes the assumption that AP0 always means I2S. What if a future chip decides to allow SPDIF on AP0? Why should we need to re-invent the binding?

IMHO, it would be much better to make this explicit.

OK.

Note that the "video-ports" and "audio-ports" are just labels in the DT file; they aren't carried through to the resulting DT binary file, so they don't have any meaning to the kernel.

Right, so, either the port type must be explicitly defined, or the name of the property giving the port value also gives the port type (nxp,video-port / nxp,audio-port).

On Tue, Jan 13, 2015 at 01:21:58PM +0100, Philipp Zabel wrote:

I wonder if it wouldn't be nicer to have the AP# and type in the device tree and then calculate the register value from that in the driver.

port@1 { reg = <1>; /* AP1 */ nxp,audio-port = "i2s"; tda998x_i2s: endpoint { remote-endpoint = <&audio1_i2s>; }; };

What about the case where we have 4 I2S streams being supplied to the device on four separate AP inputs?

On Tue, Jan 13, 2015 at 04:54:11PM +0100, Jean-Francois Moine wrote:

4 streams on 4 different APs (sources) should work, but 4 streams from a same source should be detailed.

I'd like to know how you intend to wire up four different I2S sources to the TDA998x.

Remember, an I2S source produces the I2S data and the word clock - that's two outputs. You can't electronically wire the word clocks together.

On Tue, Jan 13, 2015 at 08:02:52PM +0100, Jean-Francois Moine wrote:

On Tue, 13 Jan 2015 16:03:13 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:

...

On Tue, Jan 13, 2015 at 04:54:11PM +0100, Jean-Francois Moine wrote:

...

4 streams on 4 different APs (sources) should work, but 4 streams from a same source should be detailed.

I'd like to know how you intend to wire up four different I2S sources to the TDA998x.

Remember, an I2S source produces the I2S data and the word clock - that's two outputs. You can't electronically wire the word clocks together.

From the spec, the tda998x gets independently the serial clock and the serial word select from each I2S (stereo) input channel (= audio pin), so, you may have 4 audio chips giving 4 independent audio streams. I don't know what can be the result in HDMI if these streams are actived at the same time!

In the other configuration, an audio chip may have 4 synchronized stereo channels (software PCMs). These ones may be considered as one link (one port out from the audio chip to one port in to the tda998x), the AP configuration being 0x1f.

Let me try to be clearer. Here's four independent I2S blocks which have been arranged to be clocked by the same I2S bus clock. Each I2S block produces its own word clock and data stream:

SCLK | +-------+ +->| I2S +------> WS1 | | src 1 +------> I2S1 | +-------+ | | +-------+ +->| I2S +------> WS2 | | src 2 +------> I2S2 | +-------+ | | +-------+ +->| I2S +------> WS3 | | src 3 +------> I2S3 | +-------+ | | +-------+ +->| I2S +------> WS4 | | src 4 +------> I2S4 | +-------+ | | +---------+ | | TDA998x | `------->+ ACLK | WS --->+ AP0 | I2S1 --->+ AP1 | I2S2 --->+ AP2 | I2S3 --->+ AP3 | I2S4 --->+ AP4 | +---------+

The arrows point from the output (driver of the signal) to the input. It is illegal in electronics to wire two outputs (which are not able to disable their output drivers) together.

How do you arrange for WS1..WS4 to be connected to the TDA998x WS input?

My answer is: this is an impossible setup.

Since these are independent blocks, there is no guarantee that the WS1..WS4 outputs will be synchronised to each other, since that depends upon the SCLK edge that each unit becomes enabled. That also determines the bit position relative to SCLK of the first bit output.

Hence, you could very well end up with something like this for a 16-bit I2S output from each I2S block:

SCLK: _~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_ WS1: __~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~________________________________~ I2S1: llmm............................llmm............................llm WS2: ________________~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~___________________ I2S2: ..............llmm............................llmm................. WS3: ________~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~___________________________ I2S3: ......llmm............................llmm......................... WS4: ______~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~_____________________________ I2S4: ....llmm............................llmm...........................

where 'l' indicates the LSB of the sample, and 'm' indicates the MSB.

The problem is that the TDA998x expects each I2S input to be synchronised. In other words, the WS input is shared between each and every separate I2S input, which means the MSB of each I2S input needs to be supplied at exactly the same SCLK edge. In other words:

SCLK: _~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_ WS: __~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~________________________________~ I2S1: llmm............................llmm............................llm I2S2: llmm............................llmm............................llm I2S3: llmm............................llmm............................llm I2S4: llmm............................llmm............................llm

So, what I'm saying is that it is _impossible_ to drive the TDA998x using multiple I2S streams which are not produced by the same I2S block.

On Tue, Jan 13, 2015 at 09:41:01PM +0200, Jyri Sarha wrote:

On 01/13/2015 09:26 PM, Russell King - ARM Linux wrote:

...

SCLK: _~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_ WS: __~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~________________________________~ I2S1: llmm............................llmm............................llm I2S2: llmm............................llmm............................llm I2S3: llmm............................llmm............................llm I2S4: llmm............................llmm............................llm

So, what I'm saying is that it is_impossible_ to drive the TDA998x using multiple I2S streams which are not produced by the same I2S block.

This is besides the point, but it is possible that one of the multiple I2S blocks is the bit-clock and frame-clock master to the i2s bus and the others are slaves to it (banging their bits according to SCLK and WS of the I2S master). However, in this situation there really is only one i2s bus with multiple data pins.

Just my 0.02€ to this discussion.

Right, that's about the only way it could work.

To represent that in DT, I would imagine we'd need something like this:

#address-cells = <1>; #size-cells = <0>; ... port@1 { /* AP1,2 = I2S */ #address-cells = <1>; #size-cells = <0>; port-type = "i2s"; reg = <0x01>; /* WS */ tda998x_i2s1: endpoint@2 { reg = <0x02>; /* AP1 */ remote-endpoint = <&audio1_i2s>; }; tda998x_i2s2: endpoint@4 { reg = <0x04>; /* AP2 */ remote-endpoint = <&audio2_i2s>; }; };

where audio1_i2s is operating in master mode, and audio2_i2s is operating in slave mode for both WS and SCLK.

If we can agree on that, then I'm happy with the proposed binding. (Remember that #address-cells and #size-cells are required in the parent where we have reg= in the child.)

On Wed, Jan 14, 2015 at 08:55:01AM +0100, Jean-Francois Moine wrote:

On Tue, 13 Jan 2015 19:54:15 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:

...

On Tue, Jan 13, 2015 at 09:41:01PM +0200, Jyri Sarha wrote:

...

On 01/13/2015 09:26 PM, Russell King - ARM Linux wrote:

...

SCLK: _~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_ WS: __~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~________________________________~ I2S1: llmm............................llmm............................llm I2S2: llmm............................llmm............................llm I2S3: llmm............................llmm............................llm I2S4: llmm............................llmm............................llm

So, what I'm saying is that it is_impossible_ to drive the TDA998x using multiple I2S streams which are not produced by the same I2S block.

This is besides the point, but it is possible that one of the multiple I2S blocks is the bit-clock and frame-clock master to the i2s bus and the others are slaves to it (banging their bits according to SCLK and WS of the I2S master). However, in this situation there really is only one i2s bus with multiple data pins.

Just my 0.02€ to this discussion.

Right, that's about the only way it could work.

To represent that in DT, I would imagine we'd need something like this:

#address-cells = <1>; #size-cells = <0>; ... port@1 { /* AP1,2 = I2S */ #address-cells = <1>; #size-cells = <0>; port-type = "i2s"; reg = <0x01>; /* WS */ tda998x_i2s1: endpoint@2 { reg = <0x02>; /* AP1 */ remote-endpoint = <&audio1_i2s>; }; tda998x_i2s2: endpoint@4 { reg = <0x04>; /* AP2 */ remote-endpoint = <&audio2_i2s>; }; };

where audio1_i2s is operating in master mode, and audio2_i2s is operating in slave mode for both WS and SCLK.

If we can agree on that, then I'm happy with the proposed binding. (Remember that #address-cells and #size-cells are required in the parent where we have reg= in the child.)

#address-cells and #size-cells may be defined in any of the upper parent, so, as it is defined in the device, it is not needed in the port (see of_n_addr_cells in drivers/of/base.c).

That may be, but the documentation specifies that it should be given. See Documentation/devicetree/bindings/graph.txt - maybe the docs need fixing?

Well, I am a bit lost between (only one source / many channels - I2S busses) and (many sources / one I2s bus!).

I think that's a matter of how the problem is viewed. :)

Anyway, I don't understand your example. I'd expect that a port would be a DAI.

I view a DAI as a Linux abstraction, which doesn't really have any place in DT. I'm looking at this problem from the electronics point of view.

If yes, when playing is active, both endpoints receive an audio stream from the remote audio devices, and the AP register is always set to 0x07 (= 0x01 | 0x02 | 0x04). Then, it would have been simpler to have:

#address-cells = <1>; #size-cells = <0>; ... port@7 { /* AP1,2 = I2S */ port-type = "i2s"; reg = <0x07>; /* WS + AP1 + AP2 */ tda998x_i2s1: endpoint@1 { remote-endpoint = <&audio1_i2s>; }; tda998x_i2s2: endpoint@2 { remote-endpoint = <&audio2_i2s>; }; };

If no, the two DAIs must be created from the endpoints, permitting streaming on i2s1 alone, i2s2 alone or both i2s1+i2s2 at the same time.

How does that work - I mean, if we have i2s1 as the SCLK and WS master, how can i2s2 run without i2s1?

I suppose I2S2 could be reprogrammed to be the master for both signals, provided that I2S1 is programmed to be a slave before hand, but that seems to be making things excessively complex (we'd need some way for I2S1 and I2S2 to comunicate that state between themselves and negotiate which to be the master.)

However, I'd suggest that if audio1_i2s always maps to HDMI front left/right, audio1_i2s must always be enabled when audio playback is required; there is no CA bit combination provided in HDMI where the front L/R channels are optional. Hence, I'd suggest that audio1_i2s must always be the master.

As we don't know, I'd suggest that we permit flexibility here. We can use the reg property as you have below, and we just bitwise-or the of_endpoint port/id together, along with that result from other active audio endpoints. The advantage of that is we can use any of these representations, and it should result in a working setup - and when we do have the necessary information to make a properly informed decision, we can update the DT and binding doc accordingly and we won't have to update the driver (nor will we break backwards compat.)

Sound sane?

On Wed, Jan 14, 2015 at 11:46:58AM +0100, Philipp Zabel wrote:

So the question is mostly whether four I2S data pins with a single shared WS/SCK input should be called "four I2S ports with shared clocks" or "one I2S port with up to four data lanes". I'd lean towards the latter.

Yes, this is what we're doing for the Samsung CPU side controllers which implement this natively and seems to make sense here too - the different channels can't really work as separate interfaces in any practical way.

How audio2_i2s is forced to synchronize its clock output to audio1_i2s is a problem their bindings will have to handle.

Trying to hook up a controller that doesn't natively support this format to a device that uses it is definitely tricky, as well as describing the physical hookup we also need to worry about how things look to userspace - it's ideally going to want a single multi-channel audio stream.

I think from a binding point of view we need a way to say "this endpoint on the link is constructed from these DAIs on the device side" and say if this is TDM or multi-data, and what's driving the clocks.

On Fri, 9 Jan 2015 12:24:12 +0200 Jyri Sarha jsarha@ti.com wrote:

...

select SND_SOC_TDA998X if SND_SOC

should work in all cases.

I think that would still fail if DRM and TDA998x is built in and SND_SOC is built as modules. A request_module() call before tda9998x_codec_register() should help. Or could could write:

select SND_SOC_TDA998X if (SND_SOC=DRM_I2C_NXP_TDA998X || SND_SOC=y)

request_module() is not needed because there are functions calls from the transmitter to the codec.

With the simple "if SND_SOC", make menuconfig refuses to have TDA998x built in and SND_SOC built as modules, as it should.

On Wed, Jan 07, 2015 at 03:10:47PM +0000, Andrew Jackson wrote:

On 01/07/15 10:51, Jean-Francois Moine wrote:

...

This patch adds a CODEC to the NXP TDA998x HDMI transmitter.

The CODEC handles both I2S and S/PDIF inputs.

Please delete unneeded context from your messages, it makes it much easier to find any new content you've added.

On 01/07/15 10:51, Jean-Francois Moine wrote:

This patch adds a CODEC to the NXP TDA998x HDMI transmitter.

The CODEC handles both I2S and S/PDIF inputs. It maintains the audio format and rate constraints according to the HDMI device parameters (EDID) and sets dynamically the input ports in the TDA998x I2C driver on start/stop audio streaming.

Signed-off-by: Jean-Francois Moine moinejf@free.fr

[snip]

+static int tda998x_codec_startup(struct snd_pcm_substream *substream,

•                            struct snd_soc_dai *dai)
  

+{

•   struct snd_pcm_runtime *runtime = substream->runtime;
  

•   struct snd_pcm_hw_constraint_list *rate_constraints;
  

•   u8 *sad;                /* Short Audio Descriptor (3 bytes) */
  

[...]

•   snd_pcm_hw_constraint_minmax(runtime,
  

•                           SNDRV_PCM_HW_PARAM_CHANNELS,
  

•                           1, sad[SAD_MX_CHAN_I]);
  

In the light of our discussions elsewhere [1], shouldn't this be constrained by the number of hardware channels that the TDA998x supports too? That is, the maximum number of channels should be the lesser of sd[SAD_MX_CHAN_I] and number_of_I2S channels (or S/PDIF channels if so configured).

Andrew

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-January/086090.htm...

On Fri, 09 Jan 2015 17:39:08 +0000 Andrew Jackson Andrew.Jackson@arm.com wrote:

...

•   snd_pcm_hw_constraint_minmax(runtime,
  

•                           SNDRV_PCM_HW_PARAM_CHANNELS,
  

•                           1, sad[SAD_MX_CHAN_I]);  
  

In the light of our discussions elsewhere [1], shouldn't this be constrained by the number of hardware channels that the TDA998x supports too? That is, the maximum number of channels should be the lesser of sd[SAD_MX_CHAN_I] and number_of_I2S channels (or S/PDIF channels if so configured).

snd_pcm_hw_constraint_minmax() does the job from the min/max numbers of channels in the DAI definition and the real max number of channels will be set from audio ports declared in the DT.

On 01/08/2015 10:05 PM, Mark Brown wrote:

On Thu, Jan 08, 2015 at 04:53:04PM +0200, Jyri Sarha wrote:

...

BTW, the HDMI codec is now completely unused, since my OMAP4+ HDMI audio patches were merged (it uses dummy codec) and my BBB HDMI audio patches were never merged.

Might those BBB ones turn up sometime?

My intention is to take Jean-Francoises patches into use (I am currently working on it). I abandon my generic hdmi codec for now. In any case I should not need the dummy HDMI codec for anything.

Best regards, Jyri