[RFC,v5,4/8] drm/i2c: tda998x: Add support of a DT graph of ports

Commit Message

Jyri Sarha Feb. 17, 2016, 2:49 p.m. UTC

From: Jean-Francois Moine <[e-mail]>

Two kinds of ports may be declared in a DT graph of ports: video and audio.
This patch accepts the port value from a video port as an alternative
to the video-ports property.
It also accepts audio ports in the case the transmitter is not used as
a slave encoder.
The new file include/sound/tda998x.h prepares to the definition of
a tda998x CODEC.

Signed-off-by: Jean-Francois Moine <[e-mail]>
Signed-off-by: Jyri Sarha <[e-mail]>
---
 .../devicetree/bindings/display/bridge/tda998x.txt | 51 ++++++++++++
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 90 +++++++++++++++++++---
 include/sound/tda998x.h                            |  8 ++
 3 files changed, 140 insertions(+), 9 deletions(-)
 create mode 100644 include/sound/tda998x.h

Comments

Rob Herring Feb. 18, 2016, 2:35 p.m. UTC | #1

On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
> From: Jean-Francois Moine <[e-mail]>
> 
> Two kinds of ports may be declared in a DT graph of ports: video and audio.
> This patch accepts the port value from a video port as an alternative
> to the video-ports property.
> It also accepts audio ports in the case the transmitter is not used as
> a slave encoder.
> The new file include/sound/tda998x.h prepares to the definition of
> a tda998x CODEC.
> 
> Signed-off-by: Jean-Francois Moine <[e-mail]>
> Signed-off-by: Jyri Sarha <[e-mail]>
> ---
>  .../devicetree/bindings/display/bridge/tda998x.txt | 51 ++++++++++++
>  drivers/gpu/drm/i2c/tda998x_drv.c                  | 90 +++++++++++++++++++---
>  include/sound/tda998x.h                            |  8 ++
>  3 files changed, 140 insertions(+), 9 deletions(-)
>  create mode 100644 include/sound/tda998x.h
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> index e9e4bce..35f6a80 100644
> --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> @@ -16,6 +16,35 @@ Optional properties:
>  
>    - video-ports: 24 bits value which defines how the video controller
>  	output is wired to the TDA998x input - default: <0x230145>
> +	This property is not used when ports are defined.
> +
> +Optional nodes:
> +
> +  - port: up to three ports.
> +	The ports are defined according to [1].
> +
> +    Video port.
> +	There may be only one video port.
> +	This one must contain the following property:
> +
> +	- port-type: must be "rgb"

This should be implied from the port unit address. In other words, 
port@0 is defined to be the rgb port. Now, if this is one of several 
modes for the video port, then that is a different story.

> +	and may contain the optional property:
> +
> +	- reg: 24 bits value which defines how the video controller
> +		output is wired to the TDA998x input (video pins)
> +		When absent, the default value is <0x230145>.

This is not really how reg is intended to be used. Can you explain how 
this value is determined?

> +    Audio ports.
> +	There may be one or two audio ports.
> +	These ones must contain the following properties:
> +
> +	- port-type: must be "i2s" or "spdif"
> +
> +	- reg: 8 bits value which defines how the audio controller
> +		output is wired to the TDA998x input (audio pins)

Same here.

Rob

Jean-Francois Moine Feb. 18, 2016, 3:18 p.m. UTC | #2

On Thu, 18 Feb 2016 08:35:30 -0600
Rob Herring <[e-mail]> wrote:

> On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
> > From: Jean-Francois Moine <[e-mail]>
> > 
> > Two kinds of ports may be declared in a DT graph of ports: video and audio.
> > This patch accepts the port value from a video port as an alternative
> > to the video-ports property.
> > It also accepts audio ports in the case the transmitter is not used as
> > a slave encoder.
> > The new file include/sound/tda998x.h prepares to the definition of
> > a tda998x CODEC.
> > 
> > Signed-off-by: Jean-Francois Moine <[e-mail]>
> > Signed-off-by: Jyri Sarha <[e-mail]>
> > ---
> >  .../devicetree/bindings/display/bridge/tda998x.txt | 51 ++++++++++++
> >  drivers/gpu/drm/i2c/tda998x_drv.c                  | 90 +++++++++++++++++++---
> >  include/sound/tda998x.h                            |  8 ++
> >  3 files changed, 140 insertions(+), 9 deletions(-)
> >  create mode 100644 include/sound/tda998x.h
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > index e9e4bce..35f6a80 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > +++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > @@ -16,6 +16,35 @@ Optional properties:
> >  
> >    - video-ports: 24 bits value which defines how the video controller
> >  	output is wired to the TDA998x input - default: <0x230145>
> > +	This property is not used when ports are defined.
> > +
> > +Optional nodes:
> > +
> > +  - port: up to three ports.
> > +	The ports are defined according to [1].
> > +
> > +    Video port.
> > +	There may be only one video port.
> > +	This one must contain the following property:
> > +
> > +	- port-type: must be "rgb"
> 
> This should be implied from the port unit address. In other words, 
> port@0 is defined to be the rgb port. Now, if this is one of several 
> modes for the video port, then that is a different story.
> 
> > +	and may contain the optional property:
> > +
> > +	- reg: 24 bits value which defines how the video controller
> > +		output is wired to the TDA998x input (video pins)
> > +		When absent, the default value is <0x230145>.
> 
> This is not really how reg is intended to be used. Can you explain how 
> this value is determined?
> 
> > +    Audio ports.
> > +	There may be one or two audio ports.
> > +	These ones must contain the following properties:
> > +
> > +	- port-type: must be "i2s" or "spdif"
> > +
> > +	- reg: 8 bits value which defines how the audio controller
> > +		output is wired to the TDA998x input (audio pins)
> 
> Same here.

Hi Rob,

These audio/video port definitions were discussed in the thread
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-August/095836.html
but, you are right, and as you already said in the thread 
https://lists.freedesktop.org/archives/dri-devel/2015-September/090544.html
'reg' should not be used here.

I would have changed to 'port-value', but as I will not update the
kernel of my Dove Cubox anymore, I will not propose a new version of
this patch (and, sorry Jyri, I will not test your patch series).

Russell King - ARM Linux Feb. 18, 2016, 3:32 p.m. UTC | #3

On Thu, Feb 18, 2016 at 04:18:23PM +0100, Jean-Francois Moine wrote:
> On Thu, 18 Feb 2016 08:35:30 -0600
> Rob Herring <[e-mail]> wrote:
> 
> > On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
> > > From: Jean-Francois Moine <[e-mail]>
> > > 
> > > Two kinds of ports may be declared in a DT graph of ports: video and audio.
> > > This patch accepts the port value from a video port as an alternative
> > > to the video-ports property.
> > > It also accepts audio ports in the case the transmitter is not used as
> > > a slave encoder.
> > > The new file include/sound/tda998x.h prepares to the definition of
> > > a tda998x CODEC.
> > > 
> > > Signed-off-by: Jean-Francois Moine <[e-mail]>
> > > Signed-off-by: Jyri Sarha <[e-mail]>
> > > ---
> > >  .../devicetree/bindings/display/bridge/tda998x.txt | 51 ++++++++++++
> > >  drivers/gpu/drm/i2c/tda998x_drv.c                  | 90 +++++++++++++++++++---
> > >  include/sound/tda998x.h                            |  8 ++
> > >  3 files changed, 140 insertions(+), 9 deletions(-)
> > >  create mode 100644 include/sound/tda998x.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > > index e9e4bce..35f6a80 100644
> > > --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > > +++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > > @@ -16,6 +16,35 @@ Optional properties:
> > >  
> > >    - video-ports: 24 bits value which defines how the video controller
> > >  	output is wired to the TDA998x input - default: <0x230145>
> > > +	This property is not used when ports are defined.
> > > +
> > > +Optional nodes:
> > > +
> > > +  - port: up to three ports.
> > > +	The ports are defined according to [1].
> > > +
> > > +    Video port.
> > > +	There may be only one video port.
> > > +	This one must contain the following property:
> > > +
> > > +	- port-type: must be "rgb"
> > 
> > This should be implied from the port unit address. In other words, 
> > port@0 is defined to be the rgb port. Now, if this is one of several 
> > modes for the video port, then that is a different story.
> > 
> > > +	and may contain the optional property:
> > > +
> > > +	- reg: 24 bits value which defines how the video controller
> > > +		output is wired to the TDA998x input (video pins)
> > > +		When absent, the default value is <0x230145>.
> > 
> > This is not really how reg is intended to be used. Can you explain how 
> > this value is determined?
> > 
> > > +    Audio ports.
> > > +	There may be one or two audio ports.
> > > +	These ones must contain the following properties:
> > > +
> > > +	- port-type: must be "i2s" or "spdif"
> > > +
> > > +	- reg: 8 bits value which defines how the audio controller
> > > +		output is wired to the TDA998x input (audio pins)
> > 
> > Same here.
> 
> Hi Rob,
> 
> These audio/video port definitions were discussed in the thread
> http://mailman.alsa-project.org/pipermail/alsa-devel/2015-August/095836.html
> but, you are right, and as you already said in the thread 
> https://lists.freedesktop.org/archives/dri-devel/2015-September/090544.html
> 'reg' should not be used here.

However, ePAPR requires that a node name with @n also have a reg property
with the same n value.  See ePAPR 2.2.1.1.  This statement in it seems
to be particularly clear on this subject:

"The unit-address must match the first address specified in the reg
property of the node. If the node has no reg property, the @ and
unit-address must be omitted and the node-name alone differentiates
the node from other nodes at the same level in the tree."

Jyri Sarha Feb. 25, 2016, 1:42 p.m. UTC | #4

On 02/18/16 16:35, Rob Herring wrote:
> On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
>> From: Jean-Francois Moine <[e-mail]>
>>
>> Two kinds of ports may be declared in a DT graph of ports: video and audio.
>> This patch accepts the port value from a video port as an alternative
>> to the video-ports property.
>> It also accepts audio ports in the case the transmitter is not used as
>> a slave encoder.
>> The new file include/sound/tda998x.h prepares to the definition of
>> a tda998x CODEC.
>>
>> Signed-off-by: Jean-Francois Moine <[e-mail]>
>> Signed-off-by: Jyri Sarha <[e-mail]>
>> ---
>>   .../devicetree/bindings/display/bridge/tda998x.txt | 51 ++++++++++++
>>   drivers/gpu/drm/i2c/tda998x_drv.c                  | 90 +++++++++++++++++++---
>>   include/sound/tda998x.h                            |  8 ++
>>   3 files changed, 140 insertions(+), 9 deletions(-)
>>   create mode 100644 include/sound/tda998x.h
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
>> index e9e4bce..35f6a80 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
>> @@ -16,6 +16,35 @@ Optional properties:
>>
>>     - video-ports: 24 bits value which defines how the video controller
>>   	output is wired to the TDA998x input - default: <0x230145>
>> +	This property is not used when ports are defined.
>> +
>> +Optional nodes:
>> +
>> +  - port: up to three ports.
>> +	The ports are defined according to [1].
>> +
>> +    Video port.
>> +	There may be only one video port.
>> +	This one must contain the following property:
>> +
>> +	- port-type: must be "rgb"
>
> This should be implied from the port unit address. In other words,
> port@0 is defined to be the rgb port. Now, if this is one of several
> modes for the video port, then that is a different story.
>

Do you suggest that also the audio i2s and s/p-dif port-types should be 
coded in the port unit addresses? Something like: port@0 is always rgb, 
port@1 is i2s, and port@2 is spdif?

Having the port-type information explicitly written serves the purpose 
keeping the dts files human readable. Is saving couple of bytes this 
important or is there some other reason to not to have the port-type 
property?

>> +	and may contain the optional property:
>> +
>> +	- reg: 24 bits value which defines how the video controller
>> +		output is wired to the TDA998x input (video pins)
>> +		When absent, the default value is <0x230145>.
>
> This is not really how reg is intended to be used. Can you explain how
> this value is determined?
>

I never liked this unorthodox usage of reg property either. I'll replace 
the reg -properties with something more explicit.

>> +    Audio ports.
>> +	There may be one or two audio ports.
>> +	These ones must contain the following properties:
>> +
>> +	- port-type: must be "i2s" or "spdif"
>> +
>> +	- reg: 8 bits value which defines how the audio controller
>> +		output is wired to the TDA998x input (audio pins)
>
> Same here.
>
> Rob
>

Russell King - ARM Linux Feb. 26, 2016, 12:43 a.m. UTC | #5

On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
> On 02/18/16 16:35, Rob Herring wrote:
> >This should be implied from the port unit address. In other words,
> >port@0 is defined to be the rgb port. Now, if this is one of several
> >modes for the video port, then that is a different story.
> >
> 
> Do you suggest that also the audio i2s and s/p-dif port-types should be
> coded in the port unit addresses? Something like: port@0 is always rgb,
> port@1 is i2s, and port@2 is spdif?

For the audio inputs, the port address corresponds to the input pin.
TDA998x devices can have multiple streams routed to the pins, and can
select between them.

For example, there may be four I2S data pins and one I2S clock pin.
When using stereo, you can select which of the four I2S data pins
carries the audio data.

When using SPDIF, there may be two SPDIF inputs, and you can select
which SPDIF input is used.

So, "reg" may not be an address in terms of a CPU visible address, but
it's an address as far as selecting the appropriate input - and it
fits in with the requirements of ePAPR, which are that if you have
a unit-address (which is required to distinguish different port nodes)
then you must have a matching "reg" property.

I don't particularly like the video node using the RGB routing register
value either for the reg property, but I've kept quiet because I have
nothing to offer there: again, this comes down to ePAPR requirements
and the need to specify multiple "port { }" nodes.  You can't have two
"port { }" nodes without using a unit-address, and we'd need to chose
a unit-address for it which doesn't conflict with the audio ports...
so there's a kind of logic to using the RGB routing value, which will
never conflict.

Jyri Sarha Feb. 26, 2016, 10:14 a.m. UTC | #6

On 02/26/16 02:43, Russell King - ARM Linux wrote:
> On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
>> On 02/18/16 16:35, Rob Herring wrote:
>>> This should be implied from the port unit address. In other words,
>>> port@0 is defined to be the rgb port. Now, if this is one of several
>>> modes for the video port, then that is a different story.
>>>
>>
>> Do you suggest that also the audio i2s and s/p-dif port-types should be
>> coded in the port unit addresses? Something like: port@0 is always rgb,
>> port@1 is i2s, and port@2 is spdif?
>
> For the audio inputs, the port address corresponds to the input pin.
> TDA998x devices can have multiple streams routed to the pins, and can
> select between them.
>
> For example, there may be four I2S data pins and one I2S clock pin.
> When using stereo, you can select which of the four I2S data pins
> carries the audio data.
>

Sure, but I do not think that would be the usual setup. The only 
"normal" situation I can think for having a need to have two alternative 
audio setups would one for i2s and another for s/p-dif. But then again 
it is possible to come up with a design with multiple alternative audio 
wirings and it relatively simple handle that in DT binding, so why not.

> When using SPDIF, there may be two SPDIF inputs, and you can select
> which SPDIF input is used.
>
> So, "reg" may not be an address in terms of a CPU visible address, but
> it's an address as far as selecting the appropriate input - and it
> fits in with the requirements of ePAPR, which are that if you have
> a unit-address (which is required to distinguish different port nodes)
> then you must have a matching "reg" property.
>

Still I do not see why it is desirable to reuse reg property, when we 
can introduce new property for describing the audio wiring.

> I don't particularly like the video node using the RGB routing register
> value either for the reg property, but I've kept quiet because I have
> nothing to offer there: again, this comes down to ePAPR requirements
> and the need to specify multiple "port { }" nodes.  You can't have two
> "port { }" nodes without using a unit-address, and we'd need to chose
> a unit-address for it which doesn't conflict with the audio ports...
> so there's a kind of logic to using the RGB routing value, which will
> never conflict.
>

If we after all decide to go with using reg property for audio wiring 
(and essentially writing the value directly to AP_ENA register), then we 
could also agree that video port's unit address is always 0 as it 
corresponds to audio disabled in AP_ENA register and would not collide 
with any audio "address". Then we could keep the old video-ports 
property to configure the video wiring. How does this sound?

Best regards,
Jyri

ps. Then we still have the alternative of not using the graph/ports 
binding for audio at all. No other audio driver is currently using that 
and the graph binding is not particularly well suited for i2s 
connections as there may be more than two components connected to the 
same wires and sharing the bandwidth with both TDM and different data 
wires. But I am not sure if I want to go there any more as I have a 
feeling that I have been running in circles for couple of years with 
this now.

Russell King - ARM Linux Feb. 26, 2016, 11:21 a.m. UTC | #7

On Fri, Feb 26, 2016 at 12:14:44PM +0200, Jyri Sarha wrote:
> On 02/26/16 02:43, Russell King - ARM Linux wrote:
> >On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
> >>On 02/18/16 16:35, Rob Herring wrote:
> >>>This should be implied from the port unit address. In other words,
> >>>port@0 is defined to be the rgb port. Now, if this is one of several
> >>>modes for the video port, then that is a different story.
> >>>
> >>
> >>Do you suggest that also the audio i2s and s/p-dif port-types should be
> >>coded in the port unit addresses? Something like: port@0 is always rgb,
> >>port@1 is i2s, and port@2 is spdif?
> >
> >For the audio inputs, the port address corresponds to the input pin.
> >TDA998x devices can have multiple streams routed to the pins, and can
> >select between them.
> >
> >For example, there may be four I2S data pins and one I2S clock pin.
> >When using stereo, you can select which of the four I2S data pins
> >carries the audio data.
> >
> 
> Sure, but I do not think that would be the usual setup. The only "normal"
> situation I can think for having a need to have two alternative audio setups
> would one for i2s and another for s/p-dif. But then again it is possible to
> come up with a design with multiple alternative audio wirings and it
> relatively simple handle that in DT binding, so why not.

There's another reason: if you want to support 8 channel audio using I2S
rather than SPDIF, then you need to use four I2S data inputs.  Each I2S
data input can support only two channels.

> >When using SPDIF, there may be two SPDIF inputs, and you can select
> >which SPDIF input is used.
> >
> >So, "reg" may not be an address in terms of a CPU visible address, but
> >it's an address as far as selecting the appropriate input - and it
> >fits in with the requirements of ePAPR, which are that if you have
> >a unit-address (which is required to distinguish different port nodes)
> >then you must have a matching "reg" property.
> 
> Still I do not see why it is desirable to reuse reg property, when we can
> introduce new property for describing the audio wiring.

Different people have different opinions.  Your opinion is just another
example of someone holding a different view.

You _have_ to have a unit address, and therefore you _have_ to have a
reg property.  If you want to use some other property to describe the
audio input pin, then you will need to make up a totally ficticious
unit-address and reg property for each audio input pin.

That's adding complexity, arguably unnecessary complexity, and making
the binding unnecessarily more complex for no good reason.

> >I don't particularly like the video node using the RGB routing register
> >value either for the reg property, but I've kept quiet because I have
> >nothing to offer there: again, this comes down to ePAPR requirements
> >and the need to specify multiple "port { }" nodes.  You can't have two
> >"port { }" nodes without using a unit-address, and we'd need to chose
> >a unit-address for it which doesn't conflict with the audio ports...
> >so there's a kind of logic to using the RGB routing value, which will
> >never conflict.
> >
> 
> If we after all decide to go with using reg property for audio wiring (and
> essentially writing the value directly to AP_ENA register), then we could
> also agree that video port's unit address is always 0 as it corresponds to
> audio disabled in AP_ENA register and would not collide with any audio
> "address". Then we could keep the old video-ports property to configure the
> video wiring. How does this sound?

Sub-standard :)

This has actually been discussed before.  See the thread:

"[PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio"

from January 2015.

Jyri Sarha Feb. 29, 2016, 10:36 a.m. UTC | #8

On 02/26/16 13:21, Russell King - ARM Linux wrote:
> On Fri, Feb 26, 2016 at 12:14:44PM +0200, Jyri Sarha wrote:
>> On 02/26/16 02:43, Russell King - ARM Linux wrote:
>>> On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
>>>> On 02/18/16 16:35, Rob Herring wrote:
>>>>> This should be implied from the port unit address. In other words,
>>>>> port@0 is defined to be the rgb port. Now, if this is one of several
>>>>> modes for the video port, then that is a different story.
>>>>>
>>>>
>>>> Do you suggest that also the audio i2s and s/p-dif port-types should be
>>>> coded in the port unit addresses? Something like: port@0 is always rgb,
>>>> port@1 is i2s, and port@2 is spdif?
>>>
>>> For the audio inputs, the port address corresponds to the input pin.
>>> TDA998x devices can have multiple streams routed to the pins, and can
>>> select between them.
>>>
>>> For example, there may be four I2S data pins and one I2S clock pin.
>>> When using stereo, you can select which of the four I2S data pins
>>> carries the audio data.
>>>
>>
>> Sure, but I do not think that would be the usual setup. The only "normal"
>> situation I can think for having a need to have two alternative audio setups
>> would one for i2s and another for s/p-dif. But then again it is possible to
>> come up with a design with multiple alternative audio wirings and it
>> relatively simple handle that in DT binding, so why not.
>
> There's another reason: if you want to support 8 channel audio using I2S
> rather than SPDIF, then you need to use four I2S data inputs.  Each I2S
> data input can support only two channels.
>

Yes, but surely in the situation where there is 4 data wires, those 
should all be seen as a single audio port, as they are all sharing the 
same bit and frame clock wires. In such a situation I can't see why we 
would ever want to configure anything less that all four connected 
wires. ALSA and CPU—DAI driver can then deal with the situations when we 
only have two channels of data, or the HDMI sink can only play stereo.

If we start to think how to describe multiple alternative endpoints for 
I have to have a unit address, but do I have to have a reg property? I 
did not know about such a rule when I already made an alternative 
binding and implementation that appears to work just fine with multiple 
ports without reg property, but maybe I have over looked some piece of 
specification that forbids that.essentially the same entity (=the single 
i2s port, with up to 4 data wires), then there is no upper limit to the 
amount of ports we'd need to support.

On the other hand, if there are multiple i2s components sharing the same 
clock wires (and maybe some data wires with TDM too), then graph-style 
binding is probably not the best way to describe that kind of setup.

The more I think about this the more I think the graph binding is a bad 
choice to for i2s connections. For SPDIF the graph works better, but why 
use something that does not work for every DAI format?

>>> When using SPDIF, there may be two SPDIF inputs, and you can select
>>> which SPDIF input is used.
>>>
>>> So, "reg" may not be an address in terms of a CPU visible address, but
>>> it's an address as far as selecting the appropriate input - and it
>>> fits in with the requirements of ePAPR, which are that if you have
>>> a unit-address (which is required to distinguish different port nodes)
>>> then you must have a matching "reg" property.
>>
>> Still I do not see why it is desirable to reuse reg property, when we can
>> introduce new property for describing the audio wiring.
>
> Different people have different opinions.  Your opinion is just another
> example of someone holding a different view.
>
> You _have_ to have a unit address, and therefore you _have_ to have a
> reg property.  If you want to use some other property to describe the

I have to have a unit address, but do I have to have a reg property? I 
did not know about such a rule when I already made an alternative 
binding and implementation that appears to work just fine with multiple 
ports without reg property, but maybe I have over looked some piece of 
specification that forbids that.

> audio input pin, then you will need to make up a totally ficticious
> unit-address and reg property for each audio input pin.
>
> That's adding complexity, arguably unnecessary complexity, and making
> the binding unnecessarily more complex for no good reason.
>
>>> I don't particularly like the video node using the RGB routing register
>>> value either for the reg property, but I've kept quiet because I have
>>> nothing to offer there: again, this comes down to ePAPR requirements
>>> and the need to specify multiple "port { }" nodes.  You can't have two
>>> "port { }" nodes without using a unit-address, and we'd need to chose
>>> a unit-address for it which doesn't conflict with the audio ports...
>>> so there's a kind of logic to using the RGB routing value, which will
>>> never conflict.
>>>
>>
>> If we after all decide to go with using reg property for audio wiring (and
>> essentially writing the value directly to AP_ENA register), then we could
>> also agree that video port's unit address is always 0 as it corresponds to
>> audio disabled in AP_ENA register and would not collide with any audio
>> "address". Then we could keep the old video-ports property to configure the
>> video wiring. How does this sound?
>
> Sub-standard :)
>
> This has actually been discussed before.  See the thread:
>
> "[PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio"
>
> from January 2015.
>

Yes, this started sound strangely familiar :). Well, I've been trying to 
come up with an upstremable implementation for Beaglebone-Black HDMI 
audio for almost three years already. At this point I am ready to make 
almost what ever compromise if I just could get the code in. I just want 
to explain my point of view.

Luckily the HDMI-codec part is mostly agnostic about these things so 
maybe I can have life of its own.

Best regards,
Jyri

Jyri Sarha March 1, 2016, 2:26 p.m. UTC | #9

Ok, here is just one more simple alternative for tda998x audio binding. 
I feel that the graph ports binding for audio does not make sense 
without a graph based ASoC machine driver implementation. The ASoC 
simple-card is already here and it so widely used that there is no 
getting rid of that any time soon. This proposal provides the same 
functionality as the patch in the root of this thread, but in a simpler 
way and is equally compatible with simple-card.

The example assumes a dt-include file with integer definitions for 
TDA998x_SPDIF and TDA998x_I2S.


	tda19988 {
		compatible = "nxp,tda998x";
		reg = <0x70>;
		pinctrl-names = "default", "off";
		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
		pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
		video-ports = <0x230145>;

		/* Required by simple-card. The number of sound-dai
		   cells must agree with the number of audio ports
		   described in audio-ports property. */
		#sound-dai-cells = <2>;

			     /*	DAI-format	AP_ENA reg value */
		audio-ports = <	TDA998x_SPDIF	0x04
				TDA998x_I2S	0x03>;

		port {
			hdmi_0: endpoint@0 {
				remote-endpoint = <&lcdc_0>;
			};
		};
	};

Russell, Rob? Which one would you prefer, the old graph ports binding or 
this simpler version?

Best regards,
Jyri

Jean-Francois Moine March 1, 2016, 3:35 p.m. UTC | #10

On Tue, 1 Mar 2016 16:26:50 +0200
Jyri Sarha <[e-mail]> wrote:

> Ok, here is just one more simple alternative for tda998x audio binding. 
> I feel that the graph ports binding for audio does not make sense 
> without a graph based ASoC machine driver implementation. The ASoC 
> simple-card is already here and it so widely used that there is no 
> getting rid of that any time soon. This proposal provides the same 
> functionality as the patch in the root of this thread, but in a simpler 
> way and is equally compatible with simple-card.

Hi Jyri,

The graph port binding for the tda998x works fine with the simple card
driver when multi-codec support is added. See
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-January/085855.html

Jyri Sarha March 1, 2016, 3:51 p.m. UTC | #11

On 03/01/16 17:35, Jean-Francois Moine wrote:
> On Tue, 1 Mar 2016 16:26:50 +0200
> Jyri Sarha <[e-mail]> wrote:
>
>> Ok, here is just one more simple alternative for tda998x audio binding.
>> I feel that the graph ports binding for audio does not make sense
>> without a graph based ASoC machine driver implementation. The ASoC
>> simple-card is already here and it so widely used that there is no
>> getting rid of that any time soon. This proposal provides the same
>> functionality as the patch in the root of this thread, but in a simpler
>> way and is equally compatible with simple-card.
>
> Hi Jyri,
>
> The graph port binding for the tda998x works fine with the simple card
> driver when multi-codec support is added. See
> http://mailman.alsa-project.org/pipermail/alsa-devel/2015-January/085855.html
>

I know that it works, I have used it myself until now, but it is not 
needed and there is no driver that parses audio port endpoints. I see no 
point specifying something in the binding that is not used and there no 
specific plan to ever use it.

AFAIU my proposed binding should work equally well with simple-card, 
with or without multi-codec support.

Best regards,
Jyri

Jean-Francois Moine March 1, 2016, 4:16 p.m. UTC | #12

On Tue, 1 Mar 2016 17:51:09 +0200
Jyri Sarha <[e-mail]> wrote:

> I know that it works, I have used it myself until now, but it is not 
> needed and there is no driver that parses audio port endpoints. I see no 
> point specifying something in the binding that is not used and there no 
> specific plan to ever use it.
> 
> AFAIU my proposed binding should work equally well with simple-card, 
> with or without multi-codec support.

As told many times, the simple card is a pure Linux specific entity.
It does not describe any hardware. It should not appear in a DT, or,
if it does, its compatible should be "linux, simple-audio-card".
Then, how can the other OSs know the links between the audio
devices and the audio encoders/connectors?

On the other way, the audio graph does not impose any particular
software design. It just describes the links between the different
hardware components and each OS is free to implement its own layout.

Jyri Sarha March 1, 2016, 6:29 p.m. UTC | #13

On 03/01/16 18:16, Jean-Francois Moine wrote:
> On Tue, 1 Mar 2016 17:51:09 +0200
> Jyri Sarha <[e-mail]> wrote:
>
>> I know that it works, I have used it myself until now, but it is not
>> needed and there is no driver that parses audio port endpoints. I see no
>> point specifying something in the binding that is not used and there no
>> specific plan to ever use it.
>>
>> AFAIU my proposed binding should work equally well with simple-card,
>> with or without multi-codec support.
>
> As told many times, the simple card is a pure Linux specific entity.
> It does not describe any hardware. It should not appear in a DT, or,
> if it does, its compatible should be "linux, simple-audio-card".
> Then, how can the other OSs know the links between the audio
> devices and the audio encoders/connectors?
>

I understand the short comings of simple-card and it's binding. However, 
the binding is documented and it is feasible to extract the audio 
connections from a simple-card binding too. In fact it models the I2S 
connections better than straight out of the box graph binding. Actually 
a graph is not the best way describe an i2s-bus with multiple DAIs 
(codec or CPU) connected to it.

> On the other way, the audio graph does not impose any particular
> software design. It just describes the links between the different
> hardware components and each OS is free to implement its own layout.
>

That is true. In the most narrow sense the i2s protocol details, or even 
TDM time-slot selections should not be in the dtb. However, is not 
feasible to write a generic machine driver that would deduce the ideal 
audio configuration just based on the i2s wiring between the audio 
components simply because that is not enough information*. So to put it 
simply the simple-card is not the perfect solution for the problem, but 
even with its flaws it is better than straight out of the box graph 
binding, and it is still entirely feasible to extract all needed 
information for any audio implementation from that binding.

Still even with my proposed binding there is nothing that prevents 
adding the graph binding on top of that if it is ever needed.

Best regards,
Jyri

* With a complete set of information of all audio wiring and component 
capabilities, including the analog only components, it would probably be 
possible to deduce a generic configuration that would work in the most 
common - simple cases, but let's not go there now.

Jean-Francois Moine March 1, 2016, 7:26 p.m. UTC | #14

On Tue, 1 Mar 2016 20:29:17 +0200
Jyri Sarha <[e-mail]> wrote:

> I understand the short comings of simple-card and it's binding. However, 
> the binding is documented and it is feasible to extract the audio 
> connections from a simple-card binding too. In fact it models the I2S 
> connections better than straight out of the box graph binding. Actually 
> a graph is not the best way describe an i2s-bus with multiple DAIs 
> (codec or CPU) connected to it.

I still don't understand your problem. You want something like:

	audio-ports = <	TDA998x_SPDIF	0x04
			TDA998x_I2S	0x03>;

and the graph definition would be:

	port@03 {
		reg = <0x03>;
		port-type = "audio-i2s";
		...
	};

	port@04 {
		reg = <0x04>;
		port-type = "audio-spdif";
		...
	};

Apart the syntax, I don't really see the difference.

Mark Brown March 2, 2016, 4:29 a.m. UTC | #15

On Tue, Mar 01, 2016 at 05:16:30PM +0100, Jean-Francois Moine wrote:

> As told many times, the simple card is a pure Linux specific entity.
> It does not describe any hardware. It should not appear in a DT, or,

The physical integration of audio systems is meaningful hardware that
physically exists and matters to software.  I am completely fed up of 
having to go through this, I'm fairly sure I've been through it with you
before.

> if it does, its compatible should be "linux, simple-audio-card".

Documentation/SubmittingPatches.

> Then, how can the other OSs know the links between the audio
> devices and the audio encoders/connectors?

Documentation/devicetree/bindings/sound/simple-card.txt

> On the other way, the audio graph does not impose any particular
> software design. It just describes the links between the different
> hardware components and each OS is free to implement its own layout.

So long as there is no effort on actually upstreaming a graph based card
that shows realistic signs of getting merged in a useful form it doesn't
meaningfully exist.  Right now nobody is even trying to do that.

If you are concerned about simple-card being too Linux specific then by
all means add board specific bindings, preferably ones that can just be
added to simple-card as a compatible string.

Jyri Sarha March 2, 2016, 8:34 a.m. UTC | #16

On 03/01/16 21:26, Jean-Francois Moine wrote:
> On Tue, 1 Mar 2016 20:29:17 +0200
> Jyri Sarha <[e-mail]> wrote:
>
>> I understand the short comings of simple-card and it's binding. However,
>> the binding is documented and it is feasible to extract the audio
>> connections from a simple-card binding too. In fact it models the I2S
>> connections better than straight out of tehe box graph binding. Actually
>> a graph is not the best way describe an i2s-bus with multiple DAIs
>> (codec or CPU) connected to it.
>
> I still don't understand your problem. You want something like:
>

The problem is adding redundant unused details into binding without any 
plan of ever using them.

Fundamentally my problem is finding some consensus on the tda998x ASoC 
implementation. I've been reusing your binding for couple of review 
rounds and there has been some well justified critique towards it. I 
feel stupid in pushing forward something that I do not completely agree 
myself, so I decided to try something else.

> 	audio-ports = <	TDA998x_SPDIF	0x04
> 			TDA998x_I2S	0x03>;
>
> and the graph definition would be:
>
> 	port@03 {
> 		reg = <0x03>;
> 		port-type = "audio-i2s";
> 		...
> 	};
>
> 	port@04 {
> 		reg = <0x04>;
> 		port-type = "audio-spdif";
> 		...
> 	};
>
> Apart the syntax, I don't really see the difference.
>

Yes, the necessary information is contained in both bindings. I can live 
with either one of them, but I would prefer my version. Essentially I 
would just like to move forward.

Best regards,
Jyri

Patch

diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
index e9e4bce..35f6a80 100644
--- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt
+++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
@@ -16,6 +16,35 @@  Optional properties:
 
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
+	This property is not used when ports are defined.
+
+Optional nodes:
+
+  - port: up to three ports.
+	The ports are defined according to [1].
+
+    Video port.
+	There may be only one video port.
+	This one must contain the following property:
+
+	- port-type: must be "rgb"
+
+	and may contain the optional property:
+
+	- reg: 24 bits value which defines how the video controller
+		output is wired to the TDA998x input (video pins)
+		When absent, the default value is <0x230145>.
+
+    Audio ports.
+	There may be one or two audio ports.
+	These ones must contain the following properties:
+
+	- port-type: must be "i2s" or "spdif"
+
+	- reg: 8 bits value which defines how the audio controller
+		output is wired to the TDA998x input (audio pins)
+
+[1] Documentation/devicetree/bindings/graph.txt
 
 Example:
 
@@ -26,4 +55,26 @@  Example:
 		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;
 		pinctrl-names = "default";
+
+		port@230145 {
+			port-type = "rgb";
+			reg = <0x230145>;
+			hdmi_0: endpoint {
+				remote-endpoint = <&lcd0_0>;
+			};
+		};
+		port@3 {			/* AP1 = I2S */
+			port-type = "i2s";
+			reg = <0x03>;
+			tda998x_i2s: endpoint {
+				remote-endpoint = <&audio1_i2s>;
+			};
+		};
+		port@4 {			 /* AP2 = S/PDIF */
+			port-type = "spdif";
+			reg = <0x04>;
+			tda998x_spdif: endpoint {
+				remote-endpoint = <&audio1_spdif1>;
+			};
+		};
 	};
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 34e3874..6db1663 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -27,6 +27,7 @@ 
 #include <drm/drm_edid.h>
 #include <drm/drm_of.h>
 #include <drm/i2c/tda998x.h>
+#include <sound/tda998x.h>
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 
@@ -53,6 +54,8 @@  struct tda998x_priv {
 
 	struct drm_encoder encoder;
 	struct drm_connector connector;
+
+	struct tda998x_audio audio;
 };
 
 #define conn_to_tda998x_priv(x) \
@@ -821,6 +824,8 @@  static void tda998x_encoder_set_config(struct tda998x_priv *priv,
 			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
 
 	priv->params = *p;
+	priv->audio.port_types[0] = p->audio_format;
+	priv->audio.ports[0] = p->audio_cfg;
 }
 
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
@@ -1199,9 +1204,57 @@  static void tda998x_destroy(struct tda998x_priv *priv)
 
 /* I2C driver functions */
 
+static int tda998x_parse_ports(struct tda998x_priv *priv,
+				struct device_node *np)
+{
+	struct device_node *of_port;
+	const char *port_type;
+	int ret, audio_index, reg, afmt;
+
+	audio_index = 0;
+	for_each_child_of_node(np, of_port) {
+		if (!of_port->name
+		 || of_node_cmp(of_port->name, "port") != 0)
+			continue;
+		ret = of_property_read_string(of_port, "port-type",
+					&port_type);
+		if (ret < 0)
+			continue;
+		ret = of_property_read_u32(of_port, "reg", &reg);
+		if (strcmp(port_type, "rgb") == 0) {
+			if (!ret) {		/* video reg is optional */
+				priv->vip_cntrl_0 = reg >> 16;
+				priv->vip_cntrl_1 = reg >> 8;
+				priv->vip_cntrl_2 = reg;
+			}
+			continue;
+		}
+		if (strcmp(port_type, "i2s") == 0)
+			afmt = AFMT_I2S;
+		else if (strcmp(port_type, "spdif") == 0)
+			afmt = AFMT_SPDIF;
+		else
+			continue;
+		if (ret < 0) {
+			dev_err(&priv->hdmi->dev, "missing reg for %s\n",
+				port_type);
+			return ret;
+		}
+		if (audio_index >= ARRAY_SIZE(priv->audio.ports)) {
+			dev_err(&priv->hdmi->dev, "too many audio ports\n");
+			break;
+		}
+		priv->audio.ports[audio_index] = reg;
+		priv->audio.port_types[audio_index] = afmt;
+		audio_index++;
+	}
+	return 0;
+}
+
 static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 {
 	struct device_node *np = client->dev.of_node;
+	struct device_node *of_port;
 	u32 video;
 	int rev_lo, rev_hi, ret;
 	unsigned short cec_addr;
@@ -1309,15 +1362,34 @@  static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	/* enable EDID read irq: */
 	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
-	if (!np)
-		return 0;		/* non-DT */
-
-	/* get the optional video properties */
-	ret = of_property_read_u32(np, "video-ports", &video);
-	if (ret == 0) {
-		priv->vip_cntrl_0 = video >> 16;
-		priv->vip_cntrl_1 = video >> 8;
-		priv->vip_cntrl_2 = video;
+	/* get the device tree parameters */
+	if (np) {
+		of_port = of_get_child_by_name(np, "port");
+		if (of_port) {				/* graph of ports */
+			of_node_put(of_port);
+			ret = tda998x_parse_ports(priv, np);
+			if (ret < 0)
+				goto fail;
+
+			/* initialize the default audio configuration */
+			if (priv->audio.ports[0]) {
+				priv->params.audio_cfg = priv->audio.ports[0];
+				priv->params.audio_format =
+						priv->audio.port_types[0];
+				priv->params.audio_clk_cfg =
+					priv->params.audio_format ==
+							AFMT_SPDIF ? 0 : 1;
+			}
+		} else {
+
+			/* optional video properties */
+			ret = of_property_read_u32(np, "video-ports", &video);
+			if (ret == 0) {
+				priv->vip_cntrl_0 = video >> 16;
+				priv->vip_cntrl_1 = video >> 8;
+				priv->vip_cntrl_2 = video;
+			}
+		}
 	}
 
 	return 0;
diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h
new file mode 100644
index 0000000..bef1da7
--- /dev/null
+++ b/include/sound/tda998x.h
@@ -0,0 +1,8 @@ 
+#ifndef SND_TDA998X_H
+#define SND_TDA998X_H
+
+struct tda998x_audio {
+	u8 ports[2];			/* AP value */
+	u8 port_types[2];		/* AFMT_xxx */
+};
+#endif