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

[Jean-Francois Moine]

On Mon, 21 Sep 2015 10:19:18 -0500
Rob Herring  wrote:

> On 09/18/2015 06:06 AM, Jyri Sarha wrote:
> > From: Jean-Francois Moine 
> > 
> > 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 
> > Signed-off-by: Jyri Sarha 
> > ---
[snip]
> > diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt 
> > b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> > index e9e4bce..35f6a80 100644
> > --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> > +++ b/Documentation/devicetree/bindings/drm/i2c/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"  
> 
> Why do you need this if there is no other choice? The port number should
> define which one is video.

There is no specific port number.
One of the ports is video and two other ones are audio.

> > +
> > +   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>.  
> 
> I'm failing to decode how this value works. In any case, I would keep
> this as a vendor specific property rather than abusing reg.

This has been explained in
https://lkml.org/lkml/2014/1/20/86

> > +
> > +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)  
> 
> reg is 32-bits.

This port has only 8 significant bits as explained in
https://lkml.org/lkml/2015/1/7/362

> > +
> > +[1] Documentation/devicetree/bindings/graph.txt
> >  
> >  Example:
> >  
> > @@ -26,4 +55,26 @@ Example:
> > interrupts = <27 2>;/* falling edge */
> > pinctrl-0 = <_camera>;
> > pinctrl-names = "default";
> > +
> > +   port at 230145 {
> > +   port-type = "rgb";
> > +   reg = <0x230145>;
> > +   hdmi_0: endpoint {
> > +   remote-endpoint = <_0>;
> > +   };
> > +   };
> > +   port at 3 { /* AP1 = I2S */  
> 
> Is 3 significant? What happened to 0-2?

The value after @ is the 'reg' value (= value of the port register).

> > +   port-type = "i2s";
> > +   reg = <0x03>;
> > +   tda998x_i2s: endpoint {
> > +   remote-endpoint = <_i2s>;
> > +   };
> > +   };

-- 
Ken ar c'hentañ| ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

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

[Rob Herring]

On Thu, Sep 24, 2015 at 5:36 AM, Jean-Francois Moine  wrote:
> On Mon, 21 Sep 2015 10:19:18 -0500
> Rob Herring  wrote:
>
>> On 09/18/2015 06:06 AM, Jyri Sarha wrote:
>> > From: Jean-Francois Moine 
>> >
>> > 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 
>> > Signed-off-by: Jyri Sarha 
>> > ---
> [snip]
>> > diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt 
>> > b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
>> > index e9e4bce..35f6a80 100644
>> > --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
>> > +++ b/Documentation/devicetree/bindings/drm/i2c/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"
>>
>> Why do you need this if there is no other choice? The port number should
>> define which one is video.
>
> There is no specific port number.
> One of the ports is video and two other ones are audio.

I saying you should define the port numbering in the binding. Port 0
is video, Port 1 is i2s, etc.

>
>> > +
>> > +   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>.
>>
>> I'm failing to decode how this value works. In any case, I would keep
>> this as a vendor specific property rather than abusing reg.
>
> This has been explained in
> https://lkml.org/lkml/2014/1/20/86

Okay, so that explains how it works, but still it should not be in reg.

>
>> > +
>> > +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)
>>
>> reg is 32-bits.
>
> This port has only 8 significant bits as explained in
> https://lkml.org/lkml/2015/1/7/362

Maybe only 8-bits are used, but the size of the data in the DTB is
32-bits unless you add 8-bit annotation. Again, reg is not appropriate
to use here.

Rob

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

[Rob Herring]

On 09/18/2015 06:06 AM, Jyri Sarha wrote:
> From: Jean-Francois Moine 
> 
> 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 
> Signed-off-by: Jyri Sarha 
> ---
>  .../devicetree/bindings/drm/i2c/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/drm/i2c/tda998x.txt 
> b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> index e9e4bce..35f6a80 100644
> --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> +++ b/Documentation/devicetree/bindings/drm/i2c/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"

Why do you need this if there is no other choice? The port number should
define which one is video.

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

I'm failing to decode how this value works. In any case, I would keep
this as a vendor specific property rather than abusing reg.

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

reg is 32-bits.

> +
> +[1] Documentation/devicetree/bindings/graph.txt
>  
>  Example:
>  
> @@ -26,4 +55,26 @@ Example:
>   interrupts = <27 2>;/* falling edge */
>   pinctrl-0 = <_camera>;
>   pinctrl-names = "default";
> +
> + port at 230145 {
> + port-type = "rgb";
> + reg = <0x230145>;
> + hdmi_0: endpoint {
> + remote-endpoint = <_0>;
> + };
> + };
> + port at 3 { /* AP1 = I2S */

Is 3 significant? What happened to 0-2?

> + port-type = "i2s";
> + reg = <0x03>;
> + tda998x_i2s: endpoint {
> + remote-endpoint = <_i2s>;
> + };
> + };
> + port at 4 {  /* AP2 = S/PDIF */
> + port-type = "spdif";
> + reg = <0x04>;
> + tda998x_spdif: endpoint {
> + remote-endpoint = <_spdif1>;
> + };
> + };
>   };
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 424228b..0952eac 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
>  
> @@ -47,6 +48,8 @@ struct tda998x_priv {
>   wait_queue_head_t wq_edid;
>   volatile int wq_edid_wait;
>   struct drm_encoder *encoder;
> +
> + struct tda998x_audio audio;
>  };
>  
>  #define to_tda998x_priv(x)  ((struct tda998x_priv 
> *)to_encoder_slave(x)->slave_priv)
> @@ -774,6 +777,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 tda998x_priv *priv, int mode)
> @@ -1230,9 +1235,57 @@ static struct drm_encoder_slave_funcs 
> tda998x_encoder_slave_funcs = {
>  
>  /* 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;
> +
> +  

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

[Jyri Sarha]

From: Jean-Francois Moine 

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 
Signed-off-by: Jyri Sarha 
---
 .../devicetree/bindings/drm/i2c/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/drm/i2c/tda998x.txt 
b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
index e9e4bce..35f6a80 100644
--- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
+++ b/Documentation/devicetree/bindings/drm/i2c/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 = <_camera>;
pinctrl-names = "default";
+
+   port at 230145 {
+   port-type = "rgb";
+   reg = <0x230145>;
+   hdmi_0: endpoint {
+   remote-endpoint = <_0>;
+   };
+   };
+   port at 3 { /* AP1 = I2S */
+   port-type = "i2s";
+   reg = <0x03>;
+   tda998x_i2s: endpoint {
+   remote-endpoint = <_i2s>;
+   };
+   };
+   port at 4 {  /* AP2 = S/PDIF */
+   port-type = "spdif";
+   reg = <0x04>;
+   tda998x_spdif: endpoint {
+   remote-endpoint = <_spdif1>;
+   };
+   };
};
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 424228b..0952eac 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 

 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)

@@ -47,6 +48,8 @@ struct tda998x_priv {
wait_queue_head_t wq_edid;
volatile int wq_edid_wait;
struct drm_encoder *encoder;
+
+   struct tda998x_audio audio;
 };

 #define to_tda998x_priv(x)  ((struct tda998x_priv 
*)to_encoder_slave(x)->slave_priv)
@@ -774,6 +777,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 tda998x_priv *priv, int mode)
@@ -1230,9 +1235,57 @@ static struct drm_encoder_slave_funcs 
tda998x_encoder_slave_funcs = {

 /* 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",
+   _type);
+   if (ret < 0)
+   continue;
+   ret = of_property_read_u32(of_port, "reg", );
+   if (strcmp(port_type, "rgb") == 0) {
+