| Message ID | 842e221030a0b14bc862790eb2f5bc97bb29c012.1455720381.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote: > From: Jean-Francois Moine <moinejf@free.fr> > > 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 <moinejf@free.fr> > Signed-off-by: Jyri Sarha <jsarha@ti.com> > --- > .../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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 18 Feb 2016 08:35:30 -0600 Rob Herring <robh@kernel.org> wrote: > On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote: > > From: Jean-Francois Moine <moinejf@free.fr> > > > > 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 <moinejf@free.fr> > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > > --- > > .../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).
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 <robh@kernel.org> wrote: > > > On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote: > > > From: Jean-Francois Moine <moinejf@free.fr> > > > > > > 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 <moinejf@free.fr> > > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > > > --- > > > .../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."
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 <moinejf@free.fr> >> >> 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 <moinejf@free.fr> >> Signed-off-by: Jyri Sarha <jsarha@ti.com> >> --- >> .../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 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 1 Mar 2016 16:26:50 +0200 Jyri Sarha <jsarha@ti.com> 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
On 03/01/16 17:35, Jean-Francois Moine wrote: > On Tue, 1 Mar 2016 16:26:50 +0200 > Jyri Sarha <jsarha@ti.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 1 Mar 2016 17:51:09 +0200 Jyri Sarha <jsarha@ti.com> 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.
On 03/01/16 18:16, Jean-Francois Moine wrote: > On Tue, 1 Mar 2016 17:51:09 +0200 > Jyri Sarha <jsarha@ti.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 1 Mar 2016 20:29:17 +0200 Jyri Sarha <jsarha@ti.com> 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.
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.
On 03/01/16 21:26, Jean-Francois Moine wrote: > On Tue, 1 Mar 2016 20:29:17 +0200 > Jyri Sarha <jsarha@ti.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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", ®); + 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