| Message ID | 0084acea5a3475a77531d6a77483f36d3469111a.1420628786.git.moinejf@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/07/15 09:10, Jean-Francois Moine wrote: > This patch permits the definition of the audio ports from the device tree. > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > --- > .../devicetree/bindings/drm/i2c/tda998x.txt | 18 +++++++ > drivers/gpu/drm/i2c/tda998x_drv.c | 60 ++++++++++++++++++---- > 2 files changed, 69 insertions(+), 9 deletions(-) > > diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt > index e9e4bce..e50e7cd 100644 > --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt > +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt > @@ -17,6 +17,20 @@ Optional properties: > - video-ports: 24 bits value which defines how the video controller > output is wired to the TDA998x input - default: <0x230145> > > + - 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. Andrew > + > + - audio-port-names: must contain entries matching the entries in > + the audio-ports property. > + Each value may be "i2s" or "spdif", giving the type of > + the audio source. > + > + - #sound-dai-cells: must be set to <1> for use with the simple-card. > + The TDA998x audio CODEC always defines two DAIs. > + The DAI 0 is the S/PDIF input and the DAI 1 is the I2S input. > + > Example: > > tda998x: hdmi-encoder { > @@ -26,4 +40,8 @@ Example: > interrupts = <27 2>; /* falling edge */ > pinctrl-0 = <&pmx_camera>; > pinctrl-names = "default"; > + > + audio-ports = <0x04>, <0x03>; > + audio-port-names = "spdif", "i2s"; > + #sound-dai-cells = <1>; > }; > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index 70658af..9d9b072 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -20,6 +20,7 @@ > #include <linux/module.h> > #include <linux/irq.h> > #include <sound/asoundef.h> > +#include <linux/platform_device.h> > > #include <drm/drmP.h> > #include <drm/drm_crtc_helper.h> > @@ -44,6 +45,8 @@ struct tda998x_priv { > wait_queue_head_t wq_edid; > volatile int wq_edid_wait; > struct drm_encoder *encoder; > + > + u8 audio_ports[2]; > }; > > #define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) > @@ -1254,12 +1257,16 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) > { > struct device_node *np = client->dev.of_node; > u32 video; > - int rev_lo, rev_hi, ret; > + int i, rev_lo, rev_hi, ret; > + const char *p; > > priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3); > priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1); > priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5); > > + priv->params.audio_frame[1] = 1; /* channels - 1 */ > + priv->params.audio_sample_rate = 48000; /* 48kHz */ > + > priv->current_page = 0xff; > priv->hdmi = client; > priv->cec = i2c_new_dummy(client->adapter, 0x34); > @@ -1351,15 +1358,50 @@ 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 device tree parameters */ > + if (np) { > + > + /* 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; > + } > + > + /* optional audio properties */ > + for (i = 0; i < 2; i++) { > + u32 port; > > - /* 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; > + ret = of_property_read_u32_index(np, "audio-ports", i, &port); > + if (ret) > + break; > + ret = of_property_read_string_index(np, "audio-port-names", > + i, &p); > + if (ret) { > + dev_err(&client->dev, > + "missing audio-port-names[%d]\n", i); > + break; > + } > + if (strcmp(p, "spdif") == 0) { > + priv->audio_ports[0] = port; > + } else if (strcmp(p, "i2s") == 0) { > + priv->audio_ports[1] = port; > + } else { > + dev_err(&client->dev, > + "bad audio-port-names '%s'\n", p); > + break; > + } > + } > + if (priv->audio_ports[0]) { > + priv->params.audio_cfg = priv->audio_ports[0]; > + priv->params.audio_format = AFMT_SPDIF; > + priv->params.audio_clk_cfg = 0; > + } else { > + priv->params.audio_cfg = priv->audio_ports[1]; > + priv->params.audio_format = AFMT_I2S; > + priv->params.audio_clk_cfg = 1; > + } > } > > return 0; >
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?
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 Wed, Jan 07, 2015 at 05:18:20PM +0000, Andrew Jackson wrote: > 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.". This is commonly done by just saying that the value will be written into a given bitfield for such and such a purpose and then relying on the chip documentation for that; it's a more direct way of saying the above.
On 01/07/2015 11:10 AM, Jean-Francois Moine wrote: > This patch permits the definition of the audio ports from the device tree. > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > --- > .../devicetree/bindings/drm/i2c/tda998x.txt | 18 +++++++ > drivers/gpu/drm/i2c/tda998x_drv.c | 60 ++++++++++++++++++---- > 2 files changed, 69 insertions(+), 9 deletions(-) > > diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt > index e9e4bce..e50e7cd 100644 > --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt > +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt > @@ -17,6 +17,20 @@ Optional properties: > - video-ports: 24 bits value which defines how the video controller > output is wired to the TDA998x input - default: <0x230145> > > + - 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. > + - audio-port-names: must contain entries matching the entries in > + the audio-ports property. > + Each value may be "i2s" or "spdif", giving the type of > + the audio source. > + > + - #sound-dai-cells: must be set to <1> for use with the simple-card. > + The TDA998x audio CODEC always defines two DAIs. > + The DAI 0 is the S/PDIF input and the DAI 1 is the I2S input. > + > Example: > > tda998x: hdmi-encoder { > @@ -26,4 +40,8 @@ Example: > interrupts = <27 2>; /* falling edge */ > pinctrl-0 = <&pmx_camera>; > pinctrl-names = "default"; > + > + audio-ports = <0x04>, <0x03>; > + audio-port-names = "spdif", "i2s"; > + #sound-dai-cells = <1>; > }; > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index 70658af..9d9b072 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -20,6 +20,7 @@ > #include <linux/module.h> > #include <linux/irq.h> > #include <sound/asoundef.h> > +#include <linux/platform_device.h> > > #include <drm/drmP.h> > #include <drm/drm_crtc_helper.h> > @@ -44,6 +45,8 @@ struct tda998x_priv { > wait_queue_head_t wq_edid; > volatile int wq_edid_wait; > struct drm_encoder *encoder; > + > + u8 audio_ports[2]; > }; > > #define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) > @@ -1254,12 +1257,16 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) > { > struct device_node *np = client->dev.of_node; > u32 video; > - int rev_lo, rev_hi, ret; > + int i, rev_lo, rev_hi, ret; > + const char *p; > > priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3); > priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1); > priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5); > > + priv->params.audio_frame[1] = 1; /* channels - 1 */ > + priv->params.audio_sample_rate = 48000; /* 48kHz */ > + > priv->current_page = 0xff; > priv->hdmi = client; > priv->cec = i2c_new_dummy(client->adapter, 0x34); > @@ -1351,15 +1358,50 @@ 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 device tree parameters */ > + if (np) { > + > + /* 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; > + } > + > + /* optional audio properties */ > + for (i = 0; i < 2; i++) { > + u32 port; > > - /* 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; > + ret = of_property_read_u32_index(np, "audio-ports", i, &port); > + if (ret) > + break; > + ret = of_property_read_string_index(np, "audio-port-names", > + i, &p); > + if (ret) { > + dev_err(&client->dev, > + "missing audio-port-names[%d]\n", i); > + break; > + } > + if (strcmp(p, "spdif") == 0) { > + priv->audio_ports[0] = port; > + } else if (strcmp(p, "i2s") == 0) { > + priv->audio_ports[1] = port; > + } else { > + dev_err(&client->dev, > + "bad audio-port-names '%s'\n", p); > + break; > + } > + } > + if (priv->audio_ports[0]) { > + priv->params.audio_cfg = priv->audio_ports[0]; > + priv->params.audio_format = AFMT_SPDIF; > + priv->params.audio_clk_cfg = 0; > + } else { > + priv->params.audio_cfg = priv->audio_ports[1]; > + priv->params.audio_format = AFMT_I2S; > + priv->params.audio_clk_cfg = 1; > + } > } > > return 0; >
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. 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";
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/15 20:04, Mark Brown wrote: > 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. > If it helps, I've collated the pin assignments given in the various TDA datasheets that I can find: Chip> 9983B 9989 19988 19989 Mode> - S/PDIF I2S S/PDIF I2S S/PDIF I2S Pin AP0 WS - WS - WS - WS AP1 I2S#0 S/PDIF I2S#0 S/PDIF I2S#0 S/PDIF I2S#0 AP2 I2S#1 - - S/PDIF I2S#1 S/PDIF I2S#1 AP3 I2S#2 - - - I2S#2* MCLK - AP4 I2S#3 - - - I2S#3* - - AP5 MCLK - - - - - - AP6 S/PDIF - - - - - - AP7 AUX - - - - - - WS = I2S Word Select * Depends on package The 9983B differs from the other devices in that the I2S and S/PDIF functionality is not multiplexed onto various pins. Andrew
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, 9 Jan 2015 12:13:04 +0200 Jyri Sarha <jsarha@ti.com> wrote: > On 01/08/2015 06:42 PM, Jean-Francois Moine wrote: > > 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. There are 2 possible S/PDIF pins in the tda998x, and there may be many audio chips with S/PDIF outputs. > > 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. 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";
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, 9 Jan 2015 11:45:29 +0000 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > 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. The DT describes the hardware configuration. 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. The relation between an audio device and the associated pin is done by the definition of the sound card. With S/PDIF, only one stereo channel may be sent, but with I2S, up to 4 stereo channels may be sent. These channels are extracted by the devices connected on the HDMI bus. There is no mixing. 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. 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.
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 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. * 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). * What the driver does need to take account of is the number of inputs that are active so that the sound CODEC can be properly configured. Andrew
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, 9 Jan 2015 14:57:41 +0000 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > 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. I had a look at the TDA19988 (small) datasheet I have and at the HDMI specs, and, yes, you are right: multi (stereo) channels implies speaker mapping, and, then, that the I2S streams come from a same media source. While, as said in the TDA19988 datasheet, "The I2S-bus input interface receives an I2S-bus signal including serial data, word select and serial clock", it would be strange that these I2S buses come from different audio devices. > 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) ... > 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. > 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>;
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.
On Fri, 9 Jan 2015 20:01:27 +0000 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > 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. Very interesting idea! That's a detail, but I don't fully agree your endpoint description: - pin description AP1 is the first S/PDIF input - ok AP2 is the second S/PDIF input - ok then, the I2S input may go only to AP3 or AP4. - source input nodes You declare the same external S/PDIF source on AP1 and AP2. I would have better seen a kirkwood-like audio device as one of the S/PDIF input, say the second one. To complexify a bit, I also connect the spdif wire of the kirkwood-like device to a S/PDIF optical output ;) spdif_in -------------+ v + i2s -----> tda998x ---> HDMI audio1 | ^ + spdif -------+--------> spdif_out Here are the modified ports of - the HDMI transmitter (I removed the #xxx_cells): port@0 { // AP 1 tda998x_spdif0: endpoint@02 { reg = <0x02>; remote-endpoint = <&spdif_in_port>; }; }; port@1 { //AP 2 tda998x_spdif1: endpoint@04 { reg = <0x04>; remote-endpoint = <&audio1_spdif0>; }; }; port@2 { //AP 3 tda998x_i2s: endpoint@08 { reg = <0x09>; remote-endpoint = <&audio1_i2s>; }; }; - the (internal) audio device (audio1): port@0 { audio1_spdif0: endpoint@0 { reg = 0; remote-endpoint = <&tda998x_spdif1>; }; audio1_spdif1: endpoint@1 { reg = 0; remote-endpoint = <&spdif_out_port>; }; }; port@1 { audio1_i2s: endpoint@1 { reg = 1; remote-endpoint = <&tda998x_i2s>; }; }; - and the S/PDIF external input (spdif_in): port { spdif_in_port: endpoint@0 { reg = 0; remote-endpoint = <&tda998x_spdif0>; }; }; - and the S/PDIF external output (spdif_out): port { spdif_out_port: endpoint@0 { reg = 0; remote-endpoint = <&audio1_spdif1>; }; }; All the hardware is described. There is nothing more to add in the DT. Especially, there is no 'simple-card' which is pure software and rather Linux specific. And, now, from this DT description, ASoC expects a sound card to be built. It seems that this creation should be done in the same way the video cards are built, i.e. from the source devices, i.e. the kirkwood-like device, and also from the spdif_in (which could be some other internal audio device outputting s/pdif instead of a simple S/PDIF input connector)! Is there one or two cards, and if 2 cards, how do they share the tda998x? Well, I will have a look at how to get audio out of my machine with these new DT definitions (hopefully, there is only one audio source!). Mark, you may forget about my other patch adding multi-codecs in the simple-card... Thanks.
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
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.
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, 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. 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.
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, Jan 12, 2015 at 06:13:41PM +0100, Jean-Francois Moine wrote: > 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: > > > 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. Hmm... as the DT binding doc doesn't specify this restriction, and we have a DT file which violates what Philipp has said, I think we ought to document that reg vs unit node name does not need to match each other, thereby making that official. > > 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. 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. 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.
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).
Am Montag, den 12.01.2015, 17:57 +0000 schrieb Russell King - ARM Linux: > On Mon, Jan 12, 2015 at 06:13:41PM +0100, Jean-Francois Moine wrote: > > 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: > > > > 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. > > Hmm... as the DT binding doc doesn't specify this restriction, and we > have a DT file which violates what Philipp has said, I think we ought > to document that reg vs unit node name does not need to match each > other, thereby making that official. The (unit address part == first reg property value) is from the ePAPR and still documented in http://www.devicetree.org/Device_Tree_Usage. It isn't explicitly stated as a hard requirement, but it is worded in such a way that I'd expect it to hold true most of the time :/ > > > 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>; > > }; > > }; > > }; Please don't add the complexity of multiple 'ports' nodes to the OF graph bindings. I'd rather have the driver determine the type of the port. Ideally it could know that port 0 always is video and all other ports are audio, otherwise checking the existence of a custom property in the 'port' node should work, for example 'nxp,audio-port' or 'nxp,video-port'. Why are those located in the 'endpoint' nodes in your example? Are you expecting to dynamically reconfigure the port type of a given AP from i2s to spdif depending on the activated endpoint? > > The port type is identified by the bit AP0. > > 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. 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>; }; }; regards Philipp
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, 13 Jan 2015 12:27:15 +0000 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > 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? 4 streams on 4 different APs (sources) should work, but 4 streams from a same source should be detailed. In an other way, the unit address (== first reg) does not need to be a sequence number. It is the I/O base in most DTs. So, it could be the port value: 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>; }; };
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, 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.
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 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.
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 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). Well, I am a bit lost between (only one source / many channels - I2S busses) and (many sources / one I2s bus!). Anyway, I don't understand your example. I'd expect that a port would be a DAI. 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. Then, it would have been simpler to define the DAIs from the ports: #address-cells = <1>; #size-cells = <0>; ... port@3 { /* AP1 = I2S */ port-type = "i2s"; reg = <0x03>; /* WS + AP1 */ tda998x_i2s1: endpoint { remote-endpoint = <&audio1_i2s>; }; }; port@5 { /* AP2 = I2S */ port-type = "i2s"; reg = <0x05>; /* WS + AP2 */ tda998x_i2s1: endpoint { remote-endpoint = <&audio1_i2s>; }; };
Hi Russell, thanks for the clarification. Am Dienstag, den 13.01.2015, 19:54 +0000 schrieb Russell King - ARM Linux: [...] > 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.) 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. How audio2_i2s is forced to synchronize its clock output to audio1_i2s is a problem their bindings will have to handle. regards Philipp
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 Wed, Jan 14, 2015 at 12:50:56PM +0000, Mark Brown wrote: > 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. Trying to get the synchronisation correct for proper surround audio reproduction would also be interesting. I suspect that userspace would have to be taught specifically about a hardware setup like this - in order to ensure that the slave I2S blocks are ready before the master starts outputting the I2S clocks. I'm /not/ that thrilled with the whole idea; I'd much rather suggest that we should concentrate on sane setups, but we know that hardware engineers do create some silly setups "because they can". > 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. This seems to favour the "one DT port for I2S" model - possibly with multiple endpoints for each I2S channel (if desired). There's nothing in that model which would prevent having one end point for all four channels. To put that another way, we can remain flexible. Maybe we should document the binding indicating that for I2S, one port and one endpoint connected to one I2S block which supplies all I2S channels is the preferred model, but others are permitted but not necessarily guaranteed to work correctly.
diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt index e9e4bce..e50e7cd 100644 --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt @@ -17,6 +17,20 @@ Optional properties: - video-ports: 24 bits value which defines how the video controller output is wired to the TDA998x input - default: <0x230145> + - 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. + + - audio-port-names: must contain entries matching the entries in + the audio-ports property. + Each value may be "i2s" or "spdif", giving the type of + the audio source. + + - #sound-dai-cells: must be set to <1> for use with the simple-card. + The TDA998x audio CODEC always defines two DAIs. + The DAI 0 is the S/PDIF input and the DAI 1 is the I2S input. + Example: tda998x: hdmi-encoder { @@ -26,4 +40,8 @@ Example: interrupts = <27 2>; /* falling edge */ pinctrl-0 = <&pmx_camera>; pinctrl-names = "default"; + + audio-ports = <0x04>, <0x03>; + audio-port-names = "spdif", "i2s"; + #sound-dai-cells = <1>; }; diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 70658af..9d9b072 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -20,6 +20,7 @@ #include <linux/module.h> #include <linux/irq.h> #include <sound/asoundef.h> +#include <linux/platform_device.h> #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> @@ -44,6 +45,8 @@ struct tda998x_priv { wait_queue_head_t wq_edid; volatile int wq_edid_wait; struct drm_encoder *encoder; + + u8 audio_ports[2]; }; #define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) @@ -1254,12 +1257,16 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) { struct device_node *np = client->dev.of_node; u32 video; - int rev_lo, rev_hi, ret; + int i, rev_lo, rev_hi, ret; + const char *p; priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3); priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1); priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5); + priv->params.audio_frame[1] = 1; /* channels - 1 */ + priv->params.audio_sample_rate = 48000; /* 48kHz */ + priv->current_page = 0xff; priv->hdmi = client; priv->cec = i2c_new_dummy(client->adapter, 0x34); @@ -1351,15 +1358,50 @@ 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 device tree parameters */ + if (np) { + + /* 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; + } + + /* optional audio properties */ + for (i = 0; i < 2; i++) { + u32 port; - /* 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; + ret = of_property_read_u32_index(np, "audio-ports", i, &port); + if (ret) + break; + ret = of_property_read_string_index(np, "audio-port-names", + i, &p); + if (ret) { + dev_err(&client->dev, + "missing audio-port-names[%d]\n", i); + break; + } + if (strcmp(p, "spdif") == 0) { + priv->audio_ports[0] = port; + } else if (strcmp(p, "i2s") == 0) { + priv->audio_ports[1] = port; + } else { + dev_err(&client->dev, + "bad audio-port-names '%s'\n", p); + break; + } + } + if (priv->audio_ports[0]) { + priv->params.audio_cfg = priv->audio_ports[0]; + priv->params.audio_format = AFMT_SPDIF; + priv->params.audio_clk_cfg = 0; + } else { + priv->params.audio_cfg = priv->audio_ports[1]; + priv->params.audio_format = AFMT_I2S; + priv->params.audio_clk_cfg = 1; + } } return 0;
This patch permits the definition of the audio ports from the device tree. Signed-off-by: Jean-Francois Moine <moinejf@free.fr> --- .../devicetree/bindings/drm/i2c/tda998x.txt | 18 +++++++ drivers/gpu/drm/i2c/tda998x_drv.c | 60 ++++++++++++++++++---- 2 files changed, 69 insertions(+), 9 deletions(-)