[PATCH v5 00/13] TVP5150 new features

[PATCH v5 00/13] TVP5150 new features

[Marco Felsch]

Hi,

few months ago I send my v4 of this series [1] unfortunately I got no
feedback from Mauro but Jacopos feedback was quite helpfull =)

After my v4 I send another series which adds a generic way to parse
connector endpoints [2]. To make it easier for everyone I squashed both
series [1,2] into this one.

I recognized that patch ("media: v4l2-subdev: add stubs for
v4l2_subdev_get_try_*") was a possible blocker for this series so I
factored the patch out [3].

My main goal with the v5 was to simplify the link_setup() code a lot.
There were also some build-dep issues which I fixed too. Last
significant improvement is done on patch ("media: tvp5150: add
support to limit tv norms on connector").

I've tested it on a custom hardware and compile tested it too.

@Mauro
Please let me know how I can help you to speed up the review progress
since I wanted to get those changes merged in the near future =)

@Sakari, Hans
I've added you to the series since I made some core changes.

New patches (wasn't part of [1] or [2]):
  media: dt-bindings: tvp5150: cleanup bindings stlye
  media: dt-bindings: tvp5150: add optional tvnorms documentation
  media: tvp5150: make debug output more readable

New (squashed) patches (was part of [2]):
  dt-bindings: connector: analog: add tv norms property
  media: v4l2-fwnode: add v4l2_fwnode_connector
  media: v4l2-fwnode: add initial connector parsing support
  media: tvp5150: add support to limit tv norms on connector

Droped patches:
  media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*

[1] https://patchwork.ozlabs.org/cover/1032891/
[2] https://patchwork.kernel.org/cover/10794703/
[3] https://www.mail-archive.com/linux-media@vger.kernel.org/msg146065.html

Regards,
	Marco

Javier Martinez Canillas (1):
  partial revert of "[media] tvp5150: add HW input connectors support"

Marco Felsch (11):
  dt-bindings: connector: analog: add tv norms property
  media: v4l2-fwnode: add v4l2_fwnode_connector
  media: v4l2-fwnode: add initial connector parsing support
  media: tvp5150: add input source selection of_graph support
  media: dt-bindings: tvp5150: Add input port connectors DT bindings
  media: tvp5150: add FORMAT_TRY support for get/set selection handlers
  media: tvp5150: add s_power callback
  media: dt-bindings: tvp5150: cleanup bindings stlye
  media: dt-bindings: tvp5150: add optional tvnorms documentation
  media: tvp5150: add support to limit tv norms on connector
  media: tvp5150: make debug output more readable

Michael Tretter (1):
  media: tvp5150: initialize subdev before parsing device tree

 .../display/connector/analog-tv-connector.txt |   4 +
 .../devicetree/bindings/media/i2c/tvp5150.txt | 125 +++-
 drivers/media/i2c/tvp5150.c                   | 673 +++++++++++++-----
 drivers/media/v4l2-core/v4l2-fwnode.c         | 113 +++
 include/dt-bindings/media/tvnorms.h           |  42 ++
 include/dt-bindings/media/tvp5150.h           |   2 -
 include/media/v4l2-connector.h                |  34 +
 include/media/v4l2-fwnode.h                   |  49 ++
 8 files changed, 850 insertions(+), 192 deletions(-)
 create mode 100644 include/dt-bindings/media/tvnorms.h
 create mode 100644 include/media/v4l2-connector.h

-- 
2.20.1

[PATCH v5 01/13] dt-bindings: connector: analog: add tv norms property

[Marco Felsch]

Some connectors no matter if in- or output supports only a limited
range of tv norms. It doesn't matter if the hardware behind that
connector supports more than the listed formats since the users are
restriced by a label e.g. to plug only a camera into this connector
which uses the PAL format.

This patch adds the capability to describe such limitation within the
firmware. There are no format restrictions if the property isn't
present, so it's completely backward compatible.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../display/connector/analog-tv-connector.txt |  4 ++
 include/dt-bindings/media/tvnorms.h           | 42 +++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 include/dt-bindings/media/tvnorms.h

diff --git a/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt b/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt
index 0c0970c210ab..346f8937a0b7 100644
--- a/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt
+++ b/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt
@@ -6,6 +6,9 @@ Required properties:
 
 Optional properties:
 - label: a symbolic name for the connector
+- tvnorms: limit the supported tv norms on a connector to the given ones else
+           all tv norms are allowed. Possible video standards are defined in
+           include/dt-bindings/media/tvnorms.h.
 
 Required nodes:
 - Video port for TV input
@@ -16,6 +19,7 @@ Example
 tv: connector {
 	compatible = "composite-video-connector";
 	label = "tv";
+	tvnorms = <(TVNORM_PAL_M | TVNORM_NTSC_M)>;
 
 	port {
 		tv_connector_in: endpoint {
diff --git a/include/dt-bindings/media/tvnorms.h b/include/dt-bindings/media/tvnorms.h
new file mode 100644
index 000000000000..ec3b414a7a00
--- /dev/null
+++ b/include/dt-bindings/media/tvnorms.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0-only or X11 */
+/*
+ * Copyright 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de>
+ */
+
+#ifndef _DT_BINDINGS_MEDIA_TVNORMS_H
+#define _DT_BINDINGS_MEDIA_TVNORMS_H
+
+/* one bit for each */
+#define TVNORM_PAL_B          0x00000001
+#define TVNORM_PAL_B1         0x00000002
+#define TVNORM_PAL_G          0x00000004
+#define TVNORM_PAL_H          0x00000008
+#define TVNORM_PAL_I          0x00000010
+#define TVNORM_PAL_D          0x00000020
+#define TVNORM_PAL_D1         0x00000040
+#define TVNORM_PAL_K          0x00000080
+
+#define TVNORM_PAL_M          0x00000100
+#define TVNORM_PAL_N          0x00000200
+#define TVNORM_PAL_Nc         0x00000400
+#define TVNORM_PAL_60         0x00000800
+
+#define TVNORM_NTSC_M         0x00001000	/* BTSC */
+#define TVNORM_NTSC_M_JP      0x00002000	/* EIA-J */
+#define TVNORM_NTSC_443       0x00004000
+#define TVNORM_NTSC_M_KR      0x00008000	/* FM A2 */
+
+#define TVNORM_SECAM_B        0x00010000
+#define TVNORM_SECAM_D        0x00020000
+#define TVNORM_SECAM_G        0x00040000
+#define TVNORM_SECAM_H        0x00080000
+#define TVNORM_SECAM_K        0x00100000
+#define TVNORM_SECAM_K1       0x00200000
+#define TVNORM_SECAM_L        0x00400000
+#define TVNORM_SECAM_LC       0x00800000
+
+/* ATSC/HDTV */
+#define TVNORM_ATSC_8_VSB     0x01000000
+#define TVNORM_ATSC_16_VSB    0x02000000
+
+#endif /* _DT_BINDINGS_MEDIA_TVNORMS_H */
-- 
2.20.1

Re: [PATCH v5 01/13] dt-bindings: connector: analog: add tv norms property

[Hans Verkuil]

On 4/5/19 8:03 AM, Marco Felsch wrote:
> Some connectors no matter if in- or output supports only a limited
> range of tv norms. It doesn't matter if the hardware behind that
> connector supports more than the listed formats since the users are
> restriced by a label e.g. to plug only a camera into this connector
> which uses the PAL format.
> 
> This patch adds the capability to describe such limitation within the
> firmware. There are no format restrictions if the property isn't
> present, so it's completely backward compatible.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../display/connector/analog-tv-connector.txt |  4 ++
>  include/dt-bindings/media/tvnorms.h           | 42 +++++++++++++++++++
>  2 files changed, 46 insertions(+)
>  create mode 100644 include/dt-bindings/media/tvnorms.h
> 
> diff --git a/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt b/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt
> index 0c0970c210ab..346f8937a0b7 100644
> --- a/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt
> +++ b/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt
> @@ -6,6 +6,9 @@ Required properties:
>  
>  Optional properties:
>  - label: a symbolic name for the connector
> +- tvnorms: limit the supported tv norms on a connector to the given ones else
> +           all tv norms are allowed. Possible video standards are defined in
> +           include/dt-bindings/media/tvnorms.h.
>  
>  Required nodes:
>  - Video port for TV input
> @@ -16,6 +19,7 @@ Example
>  tv: connector {
>  	compatible = "composite-video-connector";
>  	label = "tv";
> +	tvnorms = <(TVNORM_PAL_M | TVNORM_NTSC_M)>;
>  
>  	port {
>  		tv_connector_in: endpoint {
> diff --git a/include/dt-bindings/media/tvnorms.h b/include/dt-bindings/media/tvnorms.h
> new file mode 100644
> index 000000000000..ec3b414a7a00
> --- /dev/null
> +++ b/include/dt-bindings/media/tvnorms.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-only or X11 */
> +/*
> + * Copyright 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de>
> + */
> +
> +#ifndef _DT_BINDINGS_MEDIA_TVNORMS_H
> +#define _DT_BINDINGS_MEDIA_TVNORMS_H
> +
> +/* one bit for each */
> +#define TVNORM_PAL_B          0x00000001
> +#define TVNORM_PAL_B1         0x00000002
> +#define TVNORM_PAL_G          0x00000004
> +#define TVNORM_PAL_H          0x00000008
> +#define TVNORM_PAL_I          0x00000010
> +#define TVNORM_PAL_D          0x00000020
> +#define TVNORM_PAL_D1         0x00000040
> +#define TVNORM_PAL_K          0x00000080

I would add a TVNORM_PAL here as well that is the OR of the previous defines.

> +
> +#define TVNORM_PAL_M          0x00000100
> +#define TVNORM_PAL_N          0x00000200
> +#define TVNORM_PAL_Nc         0x00000400
> +#define TVNORM_PAL_60         0x00000800
> +
> +#define TVNORM_NTSC_M         0x00001000	/* BTSC */
> +#define TVNORM_NTSC_M_JP      0x00002000	/* EIA-J */
> +#define TVNORM_NTSC_443       0x00004000
> +#define TVNORM_NTSC_M_KR      0x00008000	/* FM A2 */
> +
> +#define TVNORM_SECAM_B        0x00010000
> +#define TVNORM_SECAM_D        0x00020000
> +#define TVNORM_SECAM_G        0x00040000
> +#define TVNORM_SECAM_H        0x00080000
> +#define TVNORM_SECAM_K        0x00100000
> +#define TVNORM_SECAM_K1       0x00200000
> +#define TVNORM_SECAM_L        0x00400000
> +#define TVNORM_SECAM_LC       0x00800000

Same here for SECAM.

> +
> +/* ATSC/HDTV */
> +#define TVNORM_ATSC_8_VSB     0x01000000
> +#define TVNORM_ATSC_16_VSB    0x02000000

I would drop these. The only driver using this is pvrusb2, and I am not
convinced that's right. And in any case, that driver doesn't use the DT.

Regards,

	Hans

> +
> +#endif /* _DT_BINDINGS_MEDIA_TVNORMS_H */
> 

Re: [PATCH v5 01/13] dt-bindings: connector: analog: add tv norms property

[Marco Felsch]

Hi Hans,

On 19-04-12 14:14, Hans Verkuil wrote:
> On 4/5/19 8:03 AM, Marco Felsch wrote:
> > Some connectors no matter if in- or output supports only a limited
> > range of tv norms. It doesn't matter if the hardware behind that
> > connector supports more than the listed formats since the users are
> > restriced by a label e.g. to plug only a camera into this connector
> > which uses the PAL format.
> > 
> > This patch adds the capability to describe such limitation within the
> > firmware. There are no format restrictions if the property isn't
> > present, so it's completely backward compatible.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> >  .../display/connector/analog-tv-connector.txt |  4 ++
> >  include/dt-bindings/media/tvnorms.h           | 42 +++++++++++++++++++
> >  2 files changed, 46 insertions(+)
> >  create mode 100644 include/dt-bindings/media/tvnorms.h
> > 
> > diff --git a/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt b/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt
> > index 0c0970c210ab..346f8937a0b7 100644
> > --- a/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt
> > +++ b/Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt
> > @@ -6,6 +6,9 @@ Required properties:
> >  
> >  Optional properties:
> >  - label: a symbolic name for the connector
> > +- tvnorms: limit the supported tv norms on a connector to the given ones else
> > +           all tv norms are allowed. Possible video standards are defined in
> > +           include/dt-bindings/media/tvnorms.h.
> >  
> >  Required nodes:
> >  - Video port for TV input
> > @@ -16,6 +19,7 @@ Example
> >  tv: connector {
> >  	compatible = "composite-video-connector";
> >  	label = "tv";
> > +	tvnorms = <(TVNORM_PAL_M | TVNORM_NTSC_M)>;
> >  
> >  	port {
> >  		tv_connector_in: endpoint {
> > diff --git a/include/dt-bindings/media/tvnorms.h b/include/dt-bindings/media/tvnorms.h
> > new file mode 100644
> > index 000000000000..ec3b414a7a00
> > --- /dev/null
> > +++ b/include/dt-bindings/media/tvnorms.h
> > @@ -0,0 +1,42 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only or X11 */
> > +/*
> > + * Copyright 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de>
> > + */
> > +
> > +#ifndef _DT_BINDINGS_MEDIA_TVNORMS_H
> > +#define _DT_BINDINGS_MEDIA_TVNORMS_H
> > +
> > +/* one bit for each */
> > +#define TVNORM_PAL_B          0x00000001
> > +#define TVNORM_PAL_B1         0x00000002
> > +#define TVNORM_PAL_G          0x00000004
> > +#define TVNORM_PAL_H          0x00000008
> > +#define TVNORM_PAL_I          0x00000010
> > +#define TVNORM_PAL_D          0x00000020
> > +#define TVNORM_PAL_D1         0x00000040
> > +#define TVNORM_PAL_K          0x00000080
> 
> I would add a TVNORM_PAL here as well that is the OR of the previous defines.

Okay, I can do that.

> > +
> > +#define TVNORM_PAL_M          0x00000100
> > +#define TVNORM_PAL_N          0x00000200
> > +#define TVNORM_PAL_Nc         0x00000400
> > +#define TVNORM_PAL_60         0x00000800
> > +
> > +#define TVNORM_NTSC_M         0x00001000	/* BTSC */
> > +#define TVNORM_NTSC_M_JP      0x00002000	/* EIA-J */
> > +#define TVNORM_NTSC_443       0x00004000
> > +#define TVNORM_NTSC_M_KR      0x00008000	/* FM A2 */
> > +
> > +#define TVNORM_SECAM_B        0x00010000
> > +#define TVNORM_SECAM_D        0x00020000
> > +#define TVNORM_SECAM_G        0x00040000
> > +#define TVNORM_SECAM_H        0x00080000
> > +#define TVNORM_SECAM_K        0x00100000
> > +#define TVNORM_SECAM_K1       0x00200000
> > +#define TVNORM_SECAM_L        0x00400000
> > +#define TVNORM_SECAM_LC       0x00800000
> 
> Same here for SECAM.

Here also.

> > +
> > +/* ATSC/HDTV */
> > +#define TVNORM_ATSC_8_VSB     0x01000000
> > +#define TVNORM_ATSC_16_VSB    0x02000000
> 
> I would drop these. The only driver using this is pvrusb2, and I am not
> convinced that's right. And in any case, that driver doesn't use the DT.

Okay, if there is no user we can drop it. Just included it for
completeness.

Regards,

	Marco

> Regards,
> 
> 	Hans
> 
> > +
> > +#endif /* _DT_BINDINGS_MEDIA_TVNORMS_H */
> > 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v5 02/13] media: v4l2-fwnode: add v4l2_fwnode_connector

[Marco Felsch]

Currently every driver needs to parse the connector endpoints by it self.
This is the initial work to make this generic. The generic connector has
some common fields and some connector specific parts. The generic one
includes:
  - type
  - label
  - remote_port (the port where the connector is connected to)
  - remote_id   (the endpoint where the connector is connected to)

The specific fields are within a union, since only one of them can be
available at the time. Since this is the initial support the patch adds
only the analog-connector specific ones.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 include/media/v4l2-connector.h | 34 ++++++++++++++++++++++++++++++++++
 include/media/v4l2-fwnode.h    | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)
 create mode 100644 include/media/v4l2-connector.h

diff --git a/include/media/v4l2-connector.h b/include/media/v4l2-connector.h
new file mode 100644
index 000000000000..967336e38215
--- /dev/null
+++ b/include/media/v4l2-connector.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * v4l2-connector.h
+ *
+ * V4L2 connector types.
+ *
+ * Copyright 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de>
+ */
+
+#ifndef V4L2_CONNECTOR_H
+#define V4L2_CONNECTOR_H
+
+#define V4L2_CONNECTOR_MAX_LABEL 41
+
+/**
+ * enum v4l2_connector_type - connector type
+ * @V4L2_CON_UNKNOWN:   unknown connector type, no V4L2 connetor configuration
+ * @V4L2_CON_COMPOSITE: analog composite connector
+ * @V4L2_CON_SVIDEO:    analog svideo connector
+ * @V4L2_CON_VGA:       analog vga connector
+ * @V4L2_CON_DVI:	analog or digital dvi connector
+ * @V4L2_CON_HDMI:      digital hdmi connetor
+ */
+enum v4l2_connector_type {
+	V4L2_CON_UNKNOWN,
+	V4L2_CON_COMPOSITE,
+	V4L2_CON_SVIDEO,
+	V4L2_CON_VGA,
+	V4L2_CON_DVI,
+	V4L2_CON_HDMI,
+};
+
+#endif /* V4L2_CONNECTOR_H */
+
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 6c07825e18b9..04344b71656f 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -22,6 +22,7 @@
 #include <linux/list.h>
 #include <linux/types.h>
 
+#include <media/v4l2-connector.h>
 #include <media/v4l2-mediabus.h>
 #include <media/v4l2-subdev.h>
 
@@ -126,6 +127,38 @@ struct v4l2_fwnode_link {
 	unsigned int remote_port;
 };
 
+/**
+ * struct v4l2_fwnode_connector_analog - analog connector data structure
+ * @supported_tvnorms: tv norms this connector supports, set to V4L2_STD_ALL
+ *                     if no restrictions are specified.
+ */
+struct v4l2_fwnode_connector_analog {
+	v4l2_std_id supported_tvnorms;
+};
+
+/** struct v4l2_fwnode_connector - the connector data structure
+ * @remote_port: identifier of the port the remote endpoint belongs to
+ * @remote_id: identifier of the id the remote endpoint belongs to
+ * @label: connetor label
+ * @type: connector type
+ * @connector: union with connector configuration data struct
+ * @connector.analog: embedded &struct v4l2_fwnode_connector_analog.
+ *                    Used if connector is of type analog.
+ */
+struct v4l2_fwnode_connector {
+	/* common fields for all v4l2_fwnode_connectors */
+	unsigned int remote_port;
+	unsigned int remote_id;
+	char label[V4L2_CONNECTOR_MAX_LABEL];
+	enum v4l2_connector_type type;
+
+	/* connector specific fields */
+	union {
+		struct v4l2_fwnode_connector_analog analog;
+		/* future connectors */
+	} connector;
+};
+
 /**
  * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
  * @fwnode: pointer to the endpoint's fwnode handle
-- 
2.20.1

Re: [PATCH v5 02/13] media: v4l2-fwnode: add v4l2_fwnode_connector

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 4435 bytes --]

Hi Marco,

On Fri, Apr 05, 2019 at 08:03:06AM +0200, Marco Felsch wrote:
> Currently every driver needs to parse the connector endpoints by it self.
> This is the initial work to make this generic. The generic connector has
> some common fields and some connector specific parts. The generic one
> includes:
>   - type
>   - label
>   - remote_port (the port where the connector is connected to)
>   - remote_id   (the endpoint where the connector is connected to)
>
> The specific fields are within a union, since only one of them can be
> available at the time. Since this is the initial support the patch adds
> only the analog-connector specific ones.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  include/media/v4l2-connector.h | 34 ++++++++++++++++++++++++++++++++++
>  include/media/v4l2-fwnode.h    | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 67 insertions(+)
>  create mode 100644 include/media/v4l2-connector.h
>
> diff --git a/include/media/v4l2-connector.h b/include/media/v4l2-connector.h
> new file mode 100644
> index 000000000000..967336e38215
> --- /dev/null
> +++ b/include/media/v4l2-connector.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * v4l2-connector.h
> + *
> + * V4L2 connector types.
> + *
> + * Copyright 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de>
> + */
> +
> +#ifndef V4L2_CONNECTOR_H
> +#define V4L2_CONNECTOR_H
> +
> +#define V4L2_CONNECTOR_MAX_LABEL 41
> +
> +/**
> + * enum v4l2_connector_type - connector type
> + * @V4L2_CON_UNKNOWN:   unknown connector type, no V4L2 connetor configuration
> + * @V4L2_CON_COMPOSITE: analog composite connector
> + * @V4L2_CON_SVIDEO:    analog svideo connector
> + * @V4L2_CON_VGA:       analog vga connector
> + * @V4L2_CON_DVI:	analog or digital dvi connector
> + * @V4L2_CON_HDMI:      digital hdmi connetor
> + */
> +enum v4l2_connector_type {
> +	V4L2_CON_UNKNOWN,
> +	V4L2_CON_COMPOSITE,
> +	V4L2_CON_SVIDEO,
> +	V4L2_CON_VGA,
> +	V4L2_CON_DVI,
> +	V4L2_CON_HDMI,
> +};
> +
> +#endif /* V4L2_CONNECTOR_H */
> +
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 6c07825e18b9..04344b71656f 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -22,6 +22,7 @@
>  #include <linux/list.h>
>  #include <linux/types.h>
>
> +#include <media/v4l2-connector.h>
>  #include <media/v4l2-mediabus.h>
>  #include <media/v4l2-subdev.h>
>
> @@ -126,6 +127,38 @@ struct v4l2_fwnode_link {
>  	unsigned int remote_port;
>  };
>
> +/**
> + * struct v4l2_fwnode_connector_analog - analog connector data structure
> + * @supported_tvnorms: tv norms this connector supports, set to V4L2_STD_ALL
> + *                     if no restrictions are specified.
> + */
> +struct v4l2_fwnode_connector_analog {
> +	v4l2_std_id supported_tvnorms;
> +};
> +
> +/** struct v4l2_fwnode_connector - the connector data structure

Break the line please

> + * @remote_port: identifier of the port the remote endpoint belongs to

I liked best the description in the commmit message:
@remote_port: identifier of the remote endpoint port the connector connects to

but I see the exact same sentence you used already in an existing
comment, so feel free to chose the one you prefer.

> + * @remote_id: identifier of the id the remote endpoint belongs to

identifier of the remote endpoint the connector connect/belong to?

> + * @label: connetor label
> + * @type: connector type
> + * @connector: union with connector configuration data struct

just "connector configuration" ?

> + * @connector.analog: embedded &struct v4l2_fwnode_connector_analog.
> + *                    Used if connector is of type analog.

just "analog connector configuration" ?

> + */
> +struct v4l2_fwnode_connector {
> +	/* common fields for all v4l2_fwnode_connectors */

I would drop this

> +	unsigned int remote_port;
> +	unsigned int remote_id;
> +	char label[V4L2_CONNECTOR_MAX_LABEL];
> +	enum v4l2_connector_type type;
> +
> +	/* connector specific fields */

and this too :)

All minor stuff, feel free to pick what you like from the comments.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j
> +	union {
> +		struct v4l2_fwnode_connector_analog analog;
> +		/* future connectors */
> +	} connector;
> +};
> +
>  /**
>   * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
>   * @fwnode: pointer to the endpoint's fwnode handle
> --
> 2.20.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 02/13] media: v4l2-fwnode: add v4l2_fwnode_connector

[Marco Felsch]

Hi Jacopo,

On 19-04-05 14:43, Jacopo Mondi wrote:
> Hi Marco,
> 
> On Fri, Apr 05, 2019 at 08:03:06AM +0200, Marco Felsch wrote:
> > Currently every driver needs to parse the connector endpoints by it self.
> > This is the initial work to make this generic. The generic connector has
> > some common fields and some connector specific parts. The generic one
> > includes:
> >   - type
> >   - label
> >   - remote_port (the port where the connector is connected to)
> >   - remote_id   (the endpoint where the connector is connected to)
> >
> > The specific fields are within a union, since only one of them can be
> > available at the time. Since this is the initial support the patch adds
> > only the analog-connector specific ones.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  include/media/v4l2-connector.h | 34 ++++++++++++++++++++++++++++++++++
> >  include/media/v4l2-fwnode.h    | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 67 insertions(+)
> >  create mode 100644 include/media/v4l2-connector.h
> >
> > diff --git a/include/media/v4l2-connector.h b/include/media/v4l2-connector.h
> > new file mode 100644
> > index 000000000000..967336e38215
> > --- /dev/null
> > +++ b/include/media/v4l2-connector.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * v4l2-connector.h
> > + *
> > + * V4L2 connector types.
> > + *
> > + * Copyright 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de>
> > + */
> > +
> > +#ifndef V4L2_CONNECTOR_H
> > +#define V4L2_CONNECTOR_H
> > +
> > +#define V4L2_CONNECTOR_MAX_LABEL 41
> > +
> > +/**
> > + * enum v4l2_connector_type - connector type
> > + * @V4L2_CON_UNKNOWN:   unknown connector type, no V4L2 connetor configuration
> > + * @V4L2_CON_COMPOSITE: analog composite connector
> > + * @V4L2_CON_SVIDEO:    analog svideo connector
> > + * @V4L2_CON_VGA:       analog vga connector
> > + * @V4L2_CON_DVI:	analog or digital dvi connector
> > + * @V4L2_CON_HDMI:      digital hdmi connetor
> > + */
> > +enum v4l2_connector_type {
> > +	V4L2_CON_UNKNOWN,
> > +	V4L2_CON_COMPOSITE,
> > +	V4L2_CON_SVIDEO,
> > +	V4L2_CON_VGA,
> > +	V4L2_CON_DVI,
> > +	V4L2_CON_HDMI,
> > +};
> > +
> > +#endif /* V4L2_CONNECTOR_H */
> > +
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index 6c07825e18b9..04344b71656f 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -22,6 +22,7 @@
> >  #include <linux/list.h>
> >  #include <linux/types.h>
> >
> > +#include <media/v4l2-connector.h>
> >  #include <media/v4l2-mediabus.h>
> >  #include <media/v4l2-subdev.h>
> >
> > @@ -126,6 +127,38 @@ struct v4l2_fwnode_link {
> >  	unsigned int remote_port;
> >  };
> >
> > +/**
> > + * struct v4l2_fwnode_connector_analog - analog connector data structure
> > + * @supported_tvnorms: tv norms this connector supports, set to V4L2_STD_ALL
> > + *                     if no restrictions are specified.
> > + */
> > +struct v4l2_fwnode_connector_analog {
> > +	v4l2_std_id supported_tvnorms;
> > +};
> > +
> > +/** struct v4l2_fwnode_connector - the connector data structure
> 
> Break the line please

Yes, missed that.

> > + * @remote_port: identifier of the port the remote endpoint belongs to
> 
> I liked best the description in the commmit message:
> @remote_port: identifier of the remote endpoint port the connector connects to
> 
> but I see the exact same sentence you used already in an existing
> comment, so feel free to chose the one you prefer.

Let me change this. I was inspired by the v4l2_fwnode_link description.
I think the commit description is a bit intuitiver.

> > + * @remote_id: identifier of the id the remote endpoint belongs to
> 
> identifier of the remote endpoint the connector connect/belong to?

Here too.

> > + * @label: connetor label
> > + * @type: connector type
> > + * @connector: union with connector configuration data struct
> 
> just "connector configuration" ?

Yes that sounds better.

> > + * @connector.analog: embedded &struct v4l2_fwnode_connector_analog.
> > + *                    Used if connector is of type analog.
> 
> just "analog connector configuration" ?
> 

The connector and connetor.analog description is inspired by the
v4l2_fwnode_endpoint description. Your sounds a bit more natural so I
will pick them.

> > + */
> > +struct v4l2_fwnode_connector {
> > +	/* common fields for all v4l2_fwnode_connectors */
> 
> I would drop this

Okay, I will drop both commit messages.

> > +	unsigned int remote_port;
> > +	unsigned int remote_id;
> > +	char label[V4L2_CONNECTOR_MAX_LABEL];
> > +	enum v4l2_connector_type type;
> > +
> > +	/* connector specific fields */
> 
> and this too :)
> 
> All minor stuff, feel free to pick what you like from the comments.

Thanks a lot for the review =) I will add your Reviewed-by tag in the
next verion.

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Thanks
>   j

Regards,
  Marco

> > +	union {
> > +		struct v4l2_fwnode_connector_analog analog;
> > +		/* future connectors */
> > +	} connector;
> > +};
> > +
> >  /**
> >   * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
> >   * @fwnode: pointer to the endpoint's fwnode handle
> > --
> > 2.20.1
> >



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v5 02/13] media: v4l2-fwnode: add v4l2_fwnode_connector

[Hans Verkuil]

On 4/5/19 8:03 AM, Marco Felsch wrote:
> Currently every driver needs to parse the connector endpoints by it self.
> This is the initial work to make this generic. The generic connector has
> some common fields and some connector specific parts. The generic one
> includes:
>   - type
>   - label
>   - remote_port (the port where the connector is connected to)
>   - remote_id   (the endpoint where the connector is connected to)
> 
> The specific fields are within a union, since only one of them can be
> available at the time. Since this is the initial support the patch adds
> only the analog-connector specific ones.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  include/media/v4l2-connector.h | 34 ++++++++++++++++++++++++++++++++++
>  include/media/v4l2-fwnode.h    | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 67 insertions(+)
>  create mode 100644 include/media/v4l2-connector.h
> 
> diff --git a/include/media/v4l2-connector.h b/include/media/v4l2-connector.h
> new file mode 100644
> index 000000000000..967336e38215
> --- /dev/null
> +++ b/include/media/v4l2-connector.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * v4l2-connector.h
> + *
> + * V4L2 connector types.
> + *
> + * Copyright 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de>
> + */
> +
> +#ifndef V4L2_CONNECTOR_H
> +#define V4L2_CONNECTOR_H
> +
> +#define V4L2_CONNECTOR_MAX_LABEL 41
> +
> +/**
> + * enum v4l2_connector_type - connector type
> + * @V4L2_CON_UNKNOWN:   unknown connector type, no V4L2 connetor configuration
> + * @V4L2_CON_COMPOSITE: analog composite connector
> + * @V4L2_CON_SVIDEO:    analog svideo connector
> + * @V4L2_CON_VGA:       analog vga connector
> + * @V4L2_CON_DVI:	analog or digital dvi connector

I would drop VGA and DVI for now. Only add them if they are needed. These
connectors are rarely seen these days.

> + * @V4L2_CON_HDMI:      digital hdmi connetor

connetor -> connector


Regards,

	Hans

> + */
> +enum v4l2_connector_type {
> +	V4L2_CON_UNKNOWN,
> +	V4L2_CON_COMPOSITE,
> +	V4L2_CON_SVIDEO,
> +	V4L2_CON_VGA,
> +	V4L2_CON_DVI,
> +	V4L2_CON_HDMI,
> +};
> +
> +#endif /* V4L2_CONNECTOR_H */
> +
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 6c07825e18b9..04344b71656f 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -22,6 +22,7 @@
>  #include <linux/list.h>
>  #include <linux/types.h>
>  
> +#include <media/v4l2-connector.h>
>  #include <media/v4l2-mediabus.h>
>  #include <media/v4l2-subdev.h>
>  
> @@ -126,6 +127,38 @@ struct v4l2_fwnode_link {
>  	unsigned int remote_port;
>  };
>  
> +/**
> + * struct v4l2_fwnode_connector_analog - analog connector data structure
> + * @supported_tvnorms: tv norms this connector supports, set to V4L2_STD_ALL
> + *                     if no restrictions are specified.
> + */
> +struct v4l2_fwnode_connector_analog {
> +	v4l2_std_id supported_tvnorms;
> +};
> +
> +/** struct v4l2_fwnode_connector - the connector data structure
> + * @remote_port: identifier of the port the remote endpoint belongs to
> + * @remote_id: identifier of the id the remote endpoint belongs to
> + * @label: connetor label
> + * @type: connector type
> + * @connector: union with connector configuration data struct
> + * @connector.analog: embedded &struct v4l2_fwnode_connector_analog.
> + *                    Used if connector is of type analog.
> + */
> +struct v4l2_fwnode_connector {
> +	/* common fields for all v4l2_fwnode_connectors */
> +	unsigned int remote_port;
> +	unsigned int remote_id;
> +	char label[V4L2_CONNECTOR_MAX_LABEL];
> +	enum v4l2_connector_type type;
> +
> +	/* connector specific fields */
> +	union {
> +		struct v4l2_fwnode_connector_analog analog;
> +		/* future connectors */
> +	} connector;
> +};
> +
>  /**
>   * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
>   * @fwnode: pointer to the endpoint's fwnode handle
> 

Re: [PATCH v5 02/13] media: v4l2-fwnode: add v4l2_fwnode_connector

[Marco Felsch]

Hi Hans,

On 19-04-12 14:17, Hans Verkuil wrote:
> On 4/5/19 8:03 AM, Marco Felsch wrote:
> > Currently every driver needs to parse the connector endpoints by it self.
> > This is the initial work to make this generic. The generic connector has
> > some common fields and some connector specific parts. The generic one
> > includes:
> >   - type
> >   - label
> >   - remote_port (the port where the connector is connected to)
> >   - remote_id   (the endpoint where the connector is connected to)
> > 
> > The specific fields are within a union, since only one of them can be
> > available at the time. Since this is the initial support the patch adds
> > only the analog-connector specific ones.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  include/media/v4l2-connector.h | 34 ++++++++++++++++++++++++++++++++++
> >  include/media/v4l2-fwnode.h    | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 67 insertions(+)
> >  create mode 100644 include/media/v4l2-connector.h
> > 
> > diff --git a/include/media/v4l2-connector.h b/include/media/v4l2-connector.h
> > new file mode 100644
> > index 000000000000..967336e38215
> > --- /dev/null
> > +++ b/include/media/v4l2-connector.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * v4l2-connector.h
> > + *
> > + * V4L2 connector types.
> > + *
> > + * Copyright 2019 Pengutronix, Marco Felsch <kernel@pengutronix.de>
> > + */
> > +
> > +#ifndef V4L2_CONNECTOR_H
> > +#define V4L2_CONNECTOR_H
> > +
> > +#define V4L2_CONNECTOR_MAX_LABEL 41
> > +
> > +/**
> > + * enum v4l2_connector_type - connector type
> > + * @V4L2_CON_UNKNOWN:   unknown connector type, no V4L2 connetor configuration
> > + * @V4L2_CON_COMPOSITE: analog composite connector
> > + * @V4L2_CON_SVIDEO:    analog svideo connector
> > + * @V4L2_CON_VGA:       analog vga connector
> > + * @V4L2_CON_DVI:	analog or digital dvi connector
> 
> I would drop VGA and DVI for now. Only add them if they are needed. These
> connectors are rarely seen these days.

Okay, dropping shouldn't be a problem.

> > + * @V4L2_CON_HDMI:      digital hdmi connetor
> 
> connetor -> connector

Yes, thanks and thanks for the review.

Regards,

	Marco

> 
> Regards,
> 
> 	Hans
> 
> > + */
> > +enum v4l2_connector_type {
> > +	V4L2_CON_UNKNOWN,
> > +	V4L2_CON_COMPOSITE,
> > +	V4L2_CON_SVIDEO,
> > +	V4L2_CON_VGA,
> > +	V4L2_CON_DVI,
> > +	V4L2_CON_HDMI,
> > +};
> > +
> > +#endif /* V4L2_CONNECTOR_H */
> > +
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index 6c07825e18b9..04344b71656f 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -22,6 +22,7 @@
> >  #include <linux/list.h>
> >  #include <linux/types.h>
> >  
> > +#include <media/v4l2-connector.h>
> >  #include <media/v4l2-mediabus.h>
> >  #include <media/v4l2-subdev.h>
> >  
> > @@ -126,6 +127,38 @@ struct v4l2_fwnode_link {
> >  	unsigned int remote_port;
> >  };
> >  
> > +/**
> > + * struct v4l2_fwnode_connector_analog - analog connector data structure
> > + * @supported_tvnorms: tv norms this connector supports, set to V4L2_STD_ALL
> > + *                     if no restrictions are specified.
> > + */
> > +struct v4l2_fwnode_connector_analog {
> > +	v4l2_std_id supported_tvnorms;
> > +};
> > +
> > +/** struct v4l2_fwnode_connector - the connector data structure
> > + * @remote_port: identifier of the port the remote endpoint belongs to
> > + * @remote_id: identifier of the id the remote endpoint belongs to
> > + * @label: connetor label
> > + * @type: connector type
> > + * @connector: union with connector configuration data struct
> > + * @connector.analog: embedded &struct v4l2_fwnode_connector_analog.
> > + *                    Used if connector is of type analog.
> > + */
> > +struct v4l2_fwnode_connector {
> > +	/* common fields for all v4l2_fwnode_connectors */
> > +	unsigned int remote_port;
> > +	unsigned int remote_id;
> > +	char label[V4L2_CONNECTOR_MAX_LABEL];
> > +	enum v4l2_connector_type type;
> > +
> > +	/* connector specific fields */
> > +	union {
> > +		struct v4l2_fwnode_connector_analog analog;
> > +		/* future connectors */
> > +	} connector;
> > +};
> > +
> >  /**
> >   * v4l2_fwnode_endpoint_parse() - parse all fwnode node properties
> >   * @fwnode: pointer to the endpoint's fwnode handle
> > 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v5 03/13] media: v4l2-fwnode: add initial connector parsing support

[Marco Felsch]

The patch adds the initial connector parsing code, so we can move from a
driver specific parsing code to a generic one. Currently only the
generic fields and the analog-connector specific fields are parsed. Parsing
the other connector specific fields can be added by a simple callbacks.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---

[1] https://patchwork.kernel.org/cover/10794703/

v5:
- s/strlcpy/strscpy/

v2-v4:
- nothing since the patch was squashed from series [1] into this
  series.

 drivers/media/v4l2-core/v4l2-fwnode.c | 113 ++++++++++++++++++++++++++
 include/media/v4l2-fwnode.h           |  16 ++++
 2 files changed, 129 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 20571846e636..a6bbe42ca518 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -592,6 +592,119 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
 
+static const struct v4l2_fwnode_connector_conv {
+	enum v4l2_connector_type type;
+	const char *name;
+} connectors[] = {
+	{
+		.type = V4L2_CON_COMPOSITE,
+		.name = "composite-video-connector",
+	}, {
+		.type = V4L2_CON_SVIDEO,
+		.name = "svideo-connector",
+	}, {
+		.type = V4L2_CON_VGA,
+		.name = "vga-connector",
+	}, {
+		.type = V4L2_CON_DVI,
+		.name = "dvi-connector",
+	}, {
+		.type = V4L2_CON_HDMI,
+		.name = "hdmi-connector"
+	}
+};
+
+static enum v4l2_connector_type
+v4l2_fwnode_string_to_connector_type(const char *con_str)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(connectors); i++)
+		if (!strcmp(con_str, connectors[i].name))
+			return connectors[i].type;
+
+	/* no valid connector found */
+	return V4L2_CON_UNKNOWN;
+}
+
+static int
+v4l2_fwnode_connector_parse_analog(struct fwnode_handle *fwnode,
+				   struct v4l2_fwnode_connector *vc)
+{
+	u32 tvnorms;
+	int ret;
+
+	ret = fwnode_property_read_u32(fwnode, "tvnorms", &tvnorms);
+
+	/* set it to V4L2_STD_ALL in case of not specified tvnorms */
+	vc->connector.analog.supported_tvnorms = ret ? V4L2_STD_ALL : tvnorms;
+	return 0;
+}
+
+int v4l2_fwnode_parse_connector(struct fwnode_handle *__fwnode,
+				struct v4l2_fwnode_connector *connector)
+{
+	struct fwnode_handle *fwnode;
+	struct fwnode_endpoint __ep;
+	const char *c_type_str, *label;
+	int ret;
+
+	memset(connector, 0, sizeof(*connector));
+
+	fwnode = fwnode_graph_get_remote_port_parent(__fwnode);
+
+	/* 1st parse all common properties */
+	/* connector-type is stored within the compatible string */
+	ret = fwnode_property_read_string(fwnode, "compatible", &c_type_str);
+	if (ret)
+		return -EINVAL;
+
+	connector->type = v4l2_fwnode_string_to_connector_type(c_type_str);
+
+	fwnode_graph_parse_endpoint(__fwnode, &__ep);
+	connector->remote_port = __ep.port;
+	connector->remote_id = __ep.id;
+
+	ret = fwnode_property_read_string(fwnode, "label", &label);
+	if (!ret) {
+		/* ensure label doesn't exceed V4L2_CONNECTOR_MAX_LABEL size */
+		strscpy(connector->label, label, V4L2_CONNECTOR_MAX_LABEL);
+	} else {
+		/*
+		 * labels are optional, if no one is given create one:
+		 * <connector-type-string>@port<endpoint_port>/ep<endpoint_id>
+		 */
+		snprintf(connector->label, V4L2_CONNECTOR_MAX_LABEL,
+			 "%s@port%u/ep%u", c_type_str, connector->remote_port,
+			 connector->remote_id);
+	}
+
+	/* now parse the connector specific properties */
+	switch (connector->type) {
+		/* fall through */
+	case V4L2_CON_COMPOSITE:
+	case V4L2_CON_SVIDEO:
+		ret = v4l2_fwnode_connector_parse_analog(fwnode, connector);
+		break;
+		/* fall through */
+	case V4L2_CON_VGA:
+	case V4L2_CON_DVI:
+	case V4L2_CON_HDMI:
+		pr_warn("Connector specific parsing is currently not supported for %s\n",
+			c_type_str);
+		ret = 0;
+		break;
+		/* fall through */
+	case V4L2_CON_UNKNOWN:
+	default:
+		pr_err("Unknown connector type\n");
+		ret = -EINVAL;
+	};
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_parse_connector);
+
 static int
 v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
 					  struct v4l2_async_notifier *notifier,
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 04344b71656f..aa8bbef8589b 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -269,6 +269,22 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
  */
 void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
 
+/**
+ * v4l2_fwnode_parse_connector() - parse the connector on endpoint
+ * @fwnode: pointer to the endpoint's fwnode handle where the connector is
+ *          connected to.
+ * @connector: pointer to the V4L2 fwnode connector data structure
+ *
+ * Fill the connector data structure with the connector type, label and the
+ * endpoint id and port where the connector belongs to. If no label is present
+ * a unique one will be created. Labels with more than 40 characters are cut.
+ *
+ * Return: %0 on success or a negative error code on failure:
+ *	   %-EINVAL on parsing failure
+ */
+int v4l2_fwnode_parse_connector(struct fwnode_handle *fwnode,
+				struct v4l2_fwnode_connector *connector);
+
 /**
  * typedef parse_endpoint_func - Driver's callback function to be called on
  *	each V4L2 fwnode endpoint.
-- 
2.20.1

Re: [PATCH v5 03/13] media: v4l2-fwnode: add initial connector parsing support

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 6226 bytes --]

Hi Marco,
   thanks for the patch

On Fri, Apr 05, 2019 at 08:03:07AM +0200, Marco Felsch wrote:
> The patch adds the initial connector parsing code, so we can move from a
> driver specific parsing code to a generic one. Currently only the
> generic fields and the analog-connector specific fields are parsed. Parsing
> the other connector specific fields can be added by a simple callbacks.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>
> [1] https://patchwork.kernel.org/cover/10794703/
>
> v5:
> - s/strlcpy/strscpy/
>
> v2-v4:
> - nothing since the patch was squashed from series [1] into this
>   series.
>
>  drivers/media/v4l2-core/v4l2-fwnode.c | 113 ++++++++++++++++++++++++++
>  include/media/v4l2-fwnode.h           |  16 ++++
>  2 files changed, 129 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 20571846e636..a6bbe42ca518 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -592,6 +592,119 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
>
> +static const struct v4l2_fwnode_connector_conv {
> +	enum v4l2_connector_type type;
> +	const char *name;
> +} connectors[] = {
> +	{
> +		.type = V4L2_CON_COMPOSITE,
> +		.name = "composite-video-connector",
> +	}, {
> +		.type = V4L2_CON_SVIDEO,
> +		.name = "svideo-connector",
> +	}, {
> +		.type = V4L2_CON_VGA,
> +		.name = "vga-connector",
> +	}, {
> +		.type = V4L2_CON_DVI,
> +		.name = "dvi-connector",
> +	}, {
> +		.type = V4L2_CON_HDMI,
> +		.name = "hdmi-connector"
> +	}
> +};
> +
> +static enum v4l2_connector_type
> +v4l2_fwnode_string_to_connector_type(const char *con_str)
> +{
> +	int i;

unsigned int

> +
> +	for (i = 0; i < ARRAY_SIZE(connectors); i++)
> +		if (!strcmp(con_str, connectors[i].name))
> +			return connectors[i].type;
> +
> +	/* no valid connector found */
> +	return V4L2_CON_UNKNOWN;
> +}
> +
> +static int
> +v4l2_fwnode_connector_parse_analog(struct fwnode_handle *fwnode,
> +				   struct v4l2_fwnode_connector *vc)
> +{
> +	u32 tvnorms;
> +	int ret;
> +
> +	ret = fwnode_property_read_u32(fwnode, "tvnorms", &tvnorms);
> +
> +	/* set it to V4L2_STD_ALL in case of not specified tvnorms */

Is this comment needed?

> +	vc->connector.analog.supported_tvnorms = ret ? V4L2_STD_ALL : tvnorms;

empty line before return?

> +	return 0;
> +}
> +
> +int v4l2_fwnode_parse_connector(struct fwnode_handle *__fwnode,
> +				struct v4l2_fwnode_connector *connector)
> +{
> +	struct fwnode_handle *fwnode;
> +	struct fwnode_endpoint __ep;
> +	const char *c_type_str, *label;
> +	int ret;
> +
> +	memset(connector, 0, sizeof(*connector));
> +
> +	fwnode = fwnode_graph_get_remote_port_parent(__fwnode);

You should fwnode_handle_put(fwnode) when done and in the error path

> +
> +	/* 1st parse all common properties */
> +	/* connector-type is stored within the compatible string */
> +	ret = fwnode_property_read_string(fwnode, "compatible", &c_type_str);
> +	if (ret)
> +		return -EINVAL;
> +
> +	connector->type = v4l2_fwnode_string_to_connector_type(c_type_str);
> +
> +	fwnode_graph_parse_endpoint(__fwnode, &__ep);
> +	connector->remote_port = __ep.port;
> +	connector->remote_id = __ep.id;
> +
> +	ret = fwnode_property_read_string(fwnode, "label", &label);
> +	if (!ret) {
> +		/* ensure label doesn't exceed V4L2_CONNECTOR_MAX_LABEL size */
> +		strscpy(connector->label, label, V4L2_CONNECTOR_MAX_LABEL);
> +	} else {
> +		/*
> +		 * labels are optional, if no one is given create one:

s/no one/none.

> +		 * <connector-type-string>@port<endpoint_port>/ep<endpoint_id>
> +		 */
> +		snprintf(connector->label, V4L2_CONNECTOR_MAX_LABEL,
> +			 "%s@port%u/ep%u", c_type_str, connector->remote_port,
> +			 connector->remote_id);
> +	}
> +
> +	/* now parse the connector specific properties */
> +	switch (connector->type) {
> +		/* fall through */

Could you move the /* fall through */ comment below case?

> +	case V4L2_CON_COMPOSITE:
here
> +	case V4L2_CON_SVIDEO:
> +		ret = v4l2_fwnode_connector_parse_analog(fwnode, connector);
> +		break;
> +		/* fall through */
> +	case V4L2_CON_VGA:
here
> +	case V4L2_CON_DVI:
here
> +	case V4L2_CON_HDMI:
> +		pr_warn("Connector specific parsing is currently not supported for %s\n",
> +			c_type_str);
> +		ret = 0;
> +		break;
> +		/* fall through */
> +	case V4L2_CON_UNKNOWN:
and here
> +	default:
> +		pr_err("Unknown connector type\n");
> +		ret = -EINVAL;
> +	};
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_parse_connector);
> +
>  static int
>  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
>  					  struct v4l2_async_notifier *notifier,
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 04344b71656f..aa8bbef8589b 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -269,6 +269,22 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
>   */
>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
>
> +/**
> + * v4l2_fwnode_parse_connector() - parse the connector on endpoint
> + * @fwnode: pointer to the endpoint's fwnode handle where the connector is
> + *          connected to.

No full stop please (even if this is highly unconsistent in this
header, some lines have it, some don't, but it seems to me the most of
them do not)

> + * @connector: pointer to the V4L2 fwnode connector data structure
> + *
> + * Fill the connector data structure with the connector type, label and the
> + * endpoint id and port where the connector belongs to. If no label is present
> + * a unique one will be created. Labels with more than 40 characters are cut.
> + *
> + * Return: %0 on success or a negative error code on failure:
> + *	   %-EINVAL on parsing failure

This matches the other function's documentation style. Nice!

> + */
> +int v4l2_fwnode_parse_connector(struct fwnode_handle *fwnode,
> +				struct v4l2_fwnode_connector *connector);
> +

All minor comments:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  /**
>   * typedef parse_endpoint_func - Driver's callback function to be called on
>   *	each V4L2 fwnode endpoint.
> --
> 2.20.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 03/13] media: v4l2-fwnode: add initial connector parsing support

[Marco Felsch]

Hi Jacopo,

On 19-04-05 15:06, Jacopo Mondi wrote:
> Hi Marco,
>    thanks for the patch

thanks for the fast response =)

> On Fri, Apr 05, 2019 at 08:03:07AM +0200, Marco Felsch wrote:
> > The patch adds the initial connector parsing code, so we can move from a
> > driver specific parsing code to a generic one. Currently only the
> > generic fields and the analog-connector specific fields are parsed. Parsing
> > the other connector specific fields can be added by a simple callbacks.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >
> > [1] https://patchwork.kernel.org/cover/10794703/
> >
> > v5:
> > - s/strlcpy/strscpy/
> >
> > v2-v4:
> > - nothing since the patch was squashed from series [1] into this
> >   series.
> >
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 113 ++++++++++++++++++++++++++
> >  include/media/v4l2-fwnode.h           |  16 ++++
> >  2 files changed, 129 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 20571846e636..a6bbe42ca518 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -592,6 +592,119 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >
> > +static const struct v4l2_fwnode_connector_conv {
> > +	enum v4l2_connector_type type;
> > +	const char *name;
> > +} connectors[] = {
> > +	{
> > +		.type = V4L2_CON_COMPOSITE,
> > +		.name = "composite-video-connector",
> > +	}, {
> > +		.type = V4L2_CON_SVIDEO,
> > +		.name = "svideo-connector",
> > +	}, {
> > +		.type = V4L2_CON_VGA,
> > +		.name = "vga-connector",
> > +	}, {
> > +		.type = V4L2_CON_DVI,
> > +		.name = "dvi-connector",
> > +	}, {
> > +		.type = V4L2_CON_HDMI,
> > +		.name = "hdmi-connector"
> > +	}
> > +};
> > +
> > +static enum v4l2_connector_type
> > +v4l2_fwnode_string_to_connector_type(const char *con_str)
> > +{
> > +	int i;
> 
> unsigned int

Okay.

> > +
> > +	for (i = 0; i < ARRAY_SIZE(connectors); i++)
> > +		if (!strcmp(con_str, connectors[i].name))
> > +			return connectors[i].type;
> > +
> > +	/* no valid connector found */
> > +	return V4L2_CON_UNKNOWN;
> > +}
> > +
> > +static int
> > +v4l2_fwnode_connector_parse_analog(struct fwnode_handle *fwnode,
> > +				   struct v4l2_fwnode_connector *vc)
> > +{
> > +	u32 tvnorms;
> > +	int ret;
> > +
> > +	ret = fwnode_property_read_u32(fwnode, "tvnorms", &tvnorms);
> > +
> > +	/* set it to V4L2_STD_ALL in case of not specified tvnorms */
> 
> Is this comment needed?

Nope, let me reword it to: "tvnorms is optional"

> > +	vc->connector.analog.supported_tvnorms = ret ? V4L2_STD_ALL : tvnorms;
> 
> empty line before return?

Yes.

> > +	return 0;
> > +}
> > +
> > +int v4l2_fwnode_parse_connector(struct fwnode_handle *__fwnode,
> > +				struct v4l2_fwnode_connector *connector)
> > +{
> > +	struct fwnode_handle *fwnode;
> > +	struct fwnode_endpoint __ep;
> > +	const char *c_type_str, *label;
> > +	int ret;
> > +
> > +	memset(connector, 0, sizeof(*connector));
> > +
> > +	fwnode = fwnode_graph_get_remote_port_parent(__fwnode);
> 
> You should fwnode_handle_put(fwnode) when done and in the error path

Of course.

> > +
> > +	/* 1st parse all common properties */
> > +	/* connector-type is stored within the compatible string */
> > +	ret = fwnode_property_read_string(fwnode, "compatible", &c_type_str);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	connector->type = v4l2_fwnode_string_to_connector_type(c_type_str);
> > +
> > +	fwnode_graph_parse_endpoint(__fwnode, &__ep);
> > +	connector->remote_port = __ep.port;
> > +	connector->remote_id = __ep.id;
> > +
> > +	ret = fwnode_property_read_string(fwnode, "label", &label);
> > +	if (!ret) {
> > +		/* ensure label doesn't exceed V4L2_CONNECTOR_MAX_LABEL size */
> > +		strscpy(connector->label, label, V4L2_CONNECTOR_MAX_LABEL);
> > +	} else {
> > +		/*
> > +		 * labels are optional, if no one is given create one:
> 
> s/no one/none.

Okay.

> > +		 * <connector-type-string>@port<endpoint_port>/ep<endpoint_id>
> > +		 */
> > +		snprintf(connector->label, V4L2_CONNECTOR_MAX_LABEL,
> > +			 "%s@port%u/ep%u", c_type_str, connector->remote_port,
> > +			 connector->remote_id);
> > +	}
> > +
> > +	/* now parse the connector specific properties */
> > +	switch (connector->type) {
> > +		/* fall through */
> 
> Could you move the /* fall through */ comment below case?

Yes.

> > +	case V4L2_CON_COMPOSITE:
> here
> > +	case V4L2_CON_SVIDEO:
> > +		ret = v4l2_fwnode_connector_parse_analog(fwnode, connector);
> > +		break;
> > +		/* fall through */
> > +	case V4L2_CON_VGA:
> here
> > +	case V4L2_CON_DVI:
> here
> > +	case V4L2_CON_HDMI:
> > +		pr_warn("Connector specific parsing is currently not supported for %s\n",
> > +			c_type_str);
> > +		ret = 0;
> > +		break;
> > +		/* fall through */
> > +	case V4L2_CON_UNKNOWN:
> and here
> > +	default:
> > +		pr_err("Unknown connector type\n");
> > +		ret = -EINVAL;
> > +	};
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_parse_connector);
> > +
> >  static int
> >  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> >  					  struct v4l2_async_notifier *notifier,
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index 04344b71656f..aa8bbef8589b 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -269,6 +269,22 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> >   */
> >  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >
> > +/**
> > + * v4l2_fwnode_parse_connector() - parse the connector on endpoint
> > + * @fwnode: pointer to the endpoint's fwnode handle where the connector is
> > + *          connected to.
> 
> No full stop please (even if this is highly unconsistent in this
> header, some lines have it, some don't, but it seems to me the most of
> them do not)

Yes I see it.. damn.

> > + * @connector: pointer to the V4L2 fwnode connector data structure
> > + *
> > + * Fill the connector data structure with the connector type, label and the
> > + * endpoint id and port where the connector belongs to. If no label is present
> > + * a unique one will be created. Labels with more than 40 characters are cut.
> > + *
> > + * Return: %0 on success or a negative error code on failure:
> > + *	   %-EINVAL on parsing failure
> 
> This matches the other function's documentation style. Nice!
> 
> > + */
> > +int v4l2_fwnode_parse_connector(struct fwnode_handle *fwnode,
> > +				struct v4l2_fwnode_connector *connector);
> > +
> 
> All minor comments:
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks for the review, I will add it in the next version.

> Thanks
>   j
> 

Regards,
  Marco

> >  /**
> >   * typedef parse_endpoint_func - Driver's callback function to be called on
> >   *	each V4L2 fwnode endpoint.
> > --
> > 2.20.1
> >



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v5 04/13] partial revert of "[media] tvp5150: add HW input connectors support"

[Marco Felsch]

From: Javier Martinez Canillas <javierm@redhat.com>

Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
added input signals support for the tvp5150, but the approach was found
to be incorrect so the corresponding DT binding commit 82c2ffeb217a
("[media] tvp5150: document input connectors DT bindings") was reverted.

This left the driver with an undocumented (and wrong) DT parsing logic,
so lets get rid of this code as well until the input connectors support
is implemented properly.

It's a partial revert due other patches added on top of mentioned commit
not allowing the commit to be reverted cleanly anymore. But all the code
related to the DT parsing logic and input entities creation are removed.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[m.felsch@pengutronix.de: rm TVP5150_INPUT_NUM define]
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 drivers/media/i2c/tvp5150.c         | 141 ----------------------------
 include/dt-bindings/media/tvp5150.h |   2 -
 2 files changed, 143 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index eaddd977ba40..89da921c8886 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -53,8 +53,6 @@ struct tvp5150 {
 	struct v4l2_subdev sd;
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_pad pads[TVP5150_NUM_PADS];
-	struct media_entity input_ent[TVP5150_INPUT_NUM];
-	struct media_pad input_pad[TVP5150_INPUT_NUM];
 #endif
 	struct v4l2_ctrl_handler hdl;
 	struct v4l2_rect rect;
@@ -1169,40 +1167,6 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
-/****************************************************************************
-			Media entity ops
- ****************************************************************************/
-
-#ifdef CONFIG_MEDIA_CONTROLLER
-static int tvp5150_link_setup(struct media_entity *entity,
-			      const struct media_pad *local,
-			      const struct media_pad *remote, u32 flags)
-{
-	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
-	struct tvp5150 *decoder = to_tvp5150(sd);
-	int i;
-
-	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
-		if (remote->entity == &decoder->input_ent[i])
-			break;
-	}
-
-	/* Do nothing for entities that are not input connectors */
-	if (i == TVP5150_INPUT_NUM)
-		return 0;
-
-	decoder->input = i;
-
-	tvp5150_selmux(sd);
-
-	return 0;
-}
-
-static const struct media_entity_operations tvp5150_sd_media_ops = {
-	.link_setup = tvp5150_link_setup,
-};
-#endif
-
 /****************************************************************************
 			I2C Command
  ****************************************************************************/
@@ -1350,42 +1314,6 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 	return 0;
 }
 
-static int tvp5150_registered(struct v4l2_subdev *sd)
-{
-#ifdef CONFIG_MEDIA_CONTROLLER
-	struct tvp5150 *decoder = to_tvp5150(sd);
-	int ret = 0;
-	int i;
-
-	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
-		struct media_entity *input = &decoder->input_ent[i];
-		struct media_pad *pad = &decoder->input_pad[i];
-
-		if (!input->name)
-			continue;
-
-		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
-
-		ret = media_entity_pads_init(input, 1, pad);
-		if (ret < 0)
-			return ret;
-
-		ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
-		if (ret < 0)
-			return ret;
-
-		ret = media_create_pad_link(input, 0, &sd->entity,
-					    TVP5150_PAD_IF_INPUT, 0);
-		if (ret < 0) {
-			media_device_unregister_entity(input);
-			return ret;
-		}
-	}
-#endif
-
-	return 0;
-}
-
 /* ----------------------------------------------------------------------- */
 
 static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
@@ -1439,10 +1367,6 @@ static const struct v4l2_subdev_ops tvp5150_ops = {
 	.pad = &tvp5150_pad_ops,
 };
 
-static const struct v4l2_subdev_internal_ops tvp5150_internal_ops = {
-	.registered = tvp5150_registered,
-};
-
 /****************************************************************************
 			I2C Client & Driver
  ****************************************************************************/
@@ -1595,12 +1519,6 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 {
 	struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
 	struct device_node *ep;
-#ifdef CONFIG_MEDIA_CONTROLLER
-	struct device_node *connectors, *child;
-	struct media_entity *input;
-	const char *name;
-	u32 input_type;
-#endif
 	unsigned int flags;
 	int ret = 0;
 
@@ -1624,63 +1542,6 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 
 	decoder->mbus_type = bus_cfg.bus_type;
 
-#ifdef CONFIG_MEDIA_CONTROLLER
-	connectors = of_get_child_by_name(np, "connectors");
-
-	if (!connectors)
-		goto err;
-
-	for_each_available_child_of_node(connectors, child) {
-		ret = of_property_read_u32(child, "input", &input_type);
-		if (ret) {
-			dev_err(decoder->sd.dev,
-				 "missing type property in node %pOFn\n",
-				 child);
-			goto err_connector;
-		}
-
-		if (input_type >= TVP5150_INPUT_NUM) {
-			ret = -EINVAL;
-			goto err_connector;
-		}
-
-		input = &decoder->input_ent[input_type];
-
-		/* Each input connector can only be defined once */
-		if (input->name) {
-			dev_err(decoder->sd.dev,
-				 "input %s with same type already exists\n",
-				 input->name);
-			ret = -EINVAL;
-			goto err_connector;
-		}
-
-		switch (input_type) {
-		case TVP5150_COMPOSITE0:
-		case TVP5150_COMPOSITE1:
-			input->function = MEDIA_ENT_F_CONN_COMPOSITE;
-			break;
-		case TVP5150_SVIDEO:
-			input->function = MEDIA_ENT_F_CONN_SVIDEO;
-			break;
-		}
-
-		input->flags = MEDIA_ENT_FL_CONNECTOR;
-
-		ret = of_property_read_string(child, "label", &name);
-		if (ret < 0) {
-			dev_err(decoder->sd.dev,
-				 "missing label property in node %pOFn\n",
-				 child);
-			goto err_connector;
-		}
-
-		input->name = name;
-	}
-
-err_connector:
-	of_node_put(connectors);
-#endif
 err:
 	of_node_put(ep);
 	return ret;
@@ -1732,7 +1593,6 @@ static int tvp5150_probe(struct i2c_client *c,
 	}
 
 	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
-	sd->internal_ops = &tvp5150_internal_ops;
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
@@ -1747,7 +1607,6 @@ static int tvp5150_probe(struct i2c_client *c,
 	if (res < 0)
 		return res;
 
-	sd->entity.ops = &tvp5150_sd_media_ops;
 #endif
 
 	res = tvp5150_detect_version(core);
diff --git a/include/dt-bindings/media/tvp5150.h b/include/dt-bindings/media/tvp5150.h
index c852a35e916e..8637910aae76 100644
--- a/include/dt-bindings/media/tvp5150.h
+++ b/include/dt-bindings/media/tvp5150.h
@@ -26,8 +26,6 @@
 #define TVP5150_COMPOSITE1 1
 #define TVP5150_SVIDEO     2
 
-#define TVP5150_INPUT_NUM  3
-
 /* TVP5150 HW outputs */
 #define TVP5150_NORMAL       0
 #define TVP5150_BLACK_SCREEN 1
-- 
2.20.1

[PATCH v5 05/13] media: tvp5150: add input source selection of_graph support

[Marco Felsch]

This patch adds the of_graph support to describe the tvp connections.
Physical the TVP5150 has three ports: AIP1A, AIP1B and YOUT. As result
of discussion [1],[2] the device-tree maps these ports 1:1. The svideo
connector must be conneted to port@0/endpoint@1, look at the Documentation
for more information. Since the TVP5150 is a converter the device-tree
must contain at least 1-input and 1-output port. The mc-connectors and
mc-links are only created if the device-tree contains the corresponding
connector nodes. If more than one connector is available the
media_entity_operations.link_setup() callback ensures that only one
connector is active.

[1] https://www.spinics.net/lists/linux-media/msg138545.html
[2] https://www.spinics.net/lists/linux-media/msg138546.html

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:

[1] https://patchwork.kernel.org/cover/10794703/
[2] https://patchwork.kernel.org/cover/10786553/

v5:
- Fixing build deps:
  - tvp5150_mc_init: fix CONFIG_MEDIA_CONTROLLER deps
  - struct tvp5150: drop CONFIG_MEDIA_CONTROLLER conditional property
    includes. This leads into to complex deps for futher development.
  - tvp5150_dt_cleanup: enable function only if CONFIG_OF is enabled
  - tvp5150_parse_dt: enable function only if CONFIG_OF is enabled
  - tvp5150_probe: call tvp5150_dt_cleanup only if CONFIG_OF is enabled

- Simplify link_setup routine:
  - use generic connector parsing since both series [1,2] are squashed into
    one
  - struct tvp5150: drop pads_state and modify_second_link property
    due to link_setup() rework.
  - tvp5150_link_setup: add more comments
  - tvp5150_link_setup: simply the link setup routine a lot. Edit the 2nd
    link directly within the driver instead of a recursive media-framework
    call (__media_entity_setup_link). This improves the readability and
    shrinks the driver code.
  - tvp5150_link_setup: disable all active links in case user switches
    connectors without disable it first.
  - tvp5150_registered: simplify default link enable path due to link_setup()
    rework.

- General cleanups
  - tvp5150_parse_dt: drop unecessary test
  - tvp5150_parse_dt: add err message due to misconfiguration
  - tvp5150_parse_dt: make use of V4L2_MBUS_UNKNOWN definition
  - s/dev_dbg/dev_dbg_lvl

v4:
 - rebase on top of media_tree/master, fix merge conflict due to commit
   60359a28d592 ("media: v4l: fwnode: Initialise the V4L2 fwnode endpoints
   to zero")

v3:
- probe(): s/err/err_free_v4l2_ctrls
- drop MC dependency for tvp5150_pads

v2:
- adapt commit message
- unify ifdef switches
- rename tvp5150_valid_input -> tvp5150_of_valid_input, to be more precise
- mc: use 2-input and 1-output pad
- mc: link svideo connector to both input pads
- mc: enable/disable svideo links in one go
- mc: change link_setup() behaviour, switch the input src don't require a
      explicite disable before.
- mc: rename 'local' media_pad param to tvp5150_pad to avoid confusion
- mc: enable link to the first available connector and set the
      corresponding tvp5150 input src per default during registered()
- mc/of: factor out oftree connector allocation
- of: drop svideo dt port
- of: move svideo connector to port@0/endpoint@1
- of: require at least 1-in and 1-out endpoint

 drivers/media/i2c/tvp5150.c | 408 ++++++++++++++++++++++++++++++++----
 1 file changed, 368 insertions(+), 40 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 89da921c8886..91504fddb551 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -44,16 +44,29 @@ MODULE_PARM_DESC(debug, "Debug level (0-2)");
 #define dprintk0(__dev, __arg...) dev_dbg_lvl(__dev, 0, 0, __arg)
 
 enum tvp5150_pads {
-	TVP5150_PAD_IF_INPUT,
+	TVP5150_PAD_AIP1A = TVP5150_COMPOSITE0,
+	TVP5150_PAD_AIP1B,
 	TVP5150_PAD_VID_OUT,
 	TVP5150_NUM_PADS
 };
 
+struct tvp5150_connector {
+	struct v4l2_fwnode_connector base;
+	struct media_entity ent;
+	struct media_pad pad;
+};
+
 struct tvp5150 {
 	struct v4l2_subdev sd;
-#ifdef CONFIG_MEDIA_CONTROLLER
+	/* additional additional endpoint for the svideo connector */
+	struct device_node *endpoints[TVP5150_NUM_PADS + 1];
+	unsigned int endpoints_num;
+
+	/* media-ctl properties */
 	struct media_pad pads[TVP5150_NUM_PADS];
-#endif
+	struct tvp5150_connector *connectors;
+	int connectors_num;
+
 	struct v4l2_ctrl_handler hdl;
 	struct v4l2_rect rect;
 	struct regmap *regmap;
@@ -1167,6 +1180,129 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
+/****************************************************************************
+ *			Media entity ops
+ ****************************************************************************/
+#if defined(CONFIG_MEDIA_CONTROLLER)
+static int tvp5150_set_link(struct media_pad *connector_pad,
+			    struct media_pad *tvp5150_pad, u32 flags)
+{
+	struct media_link *link;
+
+	link = media_entity_find_link(connector_pad, tvp5150_pad);
+	if (!link)
+		return -EINVAL;
+
+	link->flags = flags;
+	link->reverse->flags = link->flags;
+
+	return 0;
+}
+
+static int tvp5150_disable_all_input_links(struct tvp5150 *decoder)
+{
+	struct media_pad *connector_pad;
+	int i, err;
+
+	for (i = 0; i < TVP5150_NUM_PADS - 1; i++) {
+		connector_pad = media_entity_remote_pad(&decoder->pads[i]);
+		if (!connector_pad)
+			continue;
+
+		err = tvp5150_set_link(connector_pad, &decoder->pads[i], 0);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int tvp5150_s_routing(struct v4l2_subdev *sd, u32 input, u32 output,
+			     u32 config);
+
+static int tvp5150_link_setup(struct media_entity *entity,
+			      const struct media_pad *tvp5150_pad,
+			      const struct media_pad *remote, u32 flags)
+{
+	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+	struct tvp5150 *decoder = to_tvp5150(sd);
+	struct media_pad *other_tvp5150_pad =
+		&decoder->pads[tvp5150_pad->index ^ 1];
+	bool is_svideo = false;
+	int i, err;
+
+	/*
+	 * The TVP5150 state is determined by the enabled sink pad link(s).
+	 * Enabling or disabling the source pad link has no effect.
+	 */
+	if (tvp5150_pad->flags & MEDIA_PAD_FL_SOURCE)
+		return 0;
+
+	/* Check if the svideo connector should be enabled */
+	for (i = 0; i < decoder->connectors_num; i++) {
+		if (remote->entity == &decoder->connectors[i].ent) {
+			is_svideo =
+			   decoder->connectors[i].base.type == V4L2_CON_SVIDEO;
+			break;
+		}
+	}
+
+	dev_dbg_lvl(sd->dev, 1, debug, "link setup '%s':%d->'%s':%d[%d]",
+		    remote->entity->name, remote->index,
+		    tvp5150_pad->entity->name, tvp5150_pad->index,
+		    flags & MEDIA_LNK_FL_ENABLED);
+	if (is_svideo)
+		dev_dbg_lvl(sd->dev, 1, debug,
+			    "link setup '%s':%d->'%s':%d[%d]",
+			    remote->entity->name, remote->index,
+			    other_tvp5150_pad->entity->name,
+			    other_tvp5150_pad->index,
+			    flags & MEDIA_LNK_FL_ENABLED);
+
+	/*
+	 * The TVP5150 has a internal mux which allows the following setup:
+	 *
+	 * comp-connector1  --\
+	 *		       |---> AIP1A
+	 *		      /
+	 * svideo-connector -|
+	 *		      \
+	 *		       |---> AIP1B
+	 * comp-connector2  --/
+	 *
+	 * We can't rely on user space that the current connector gets disabled
+	 * first before enabling the new connector. Disable all active
+	 * connector links to be on the safe side.
+	 */
+	err = tvp5150_disable_all_input_links(decoder);
+	if (err)
+		return err;
+
+	tvp5150_s_routing(sd, is_svideo ? TVP5150_SVIDEO : tvp5150_pad->index,
+			  flags & MEDIA_LNK_FL_ENABLED ? TVP5150_NORMAL :
+			  TVP5150_BLACK_SCREEN, 0);
+
+	if (flags & MEDIA_LNK_FL_ENABLED) {
+		/*
+		 * S-Video connector is conneted to both ports AIP1A and AIP1B.
+		 * Both links must be enabled in one-shot regardless which link
+		 * the user requests.
+		 */
+		if (is_svideo) {
+			err = tvp5150_set_link((struct media_pad *) remote,
+					       other_tvp5150_pad, flags);
+			if (err)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
+static const struct media_entity_operations tvp5150_sd_media_ops = {
+	.link_setup = tvp5150_link_setup,
+};
+#endif
 /****************************************************************************
 			I2C Command
  ****************************************************************************/
@@ -1314,6 +1450,65 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 	return 0;
 }
 
+static int tvp5150_registered(struct v4l2_subdev *sd)
+{
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	struct tvp5150 *decoder = to_tvp5150(sd);
+	unsigned int i;
+	int ret;
+
+	/*
+	 * Setup connector pads and links. Enable the link to the first
+	 * available connector per default.
+	 */
+	for (i = 0; i < decoder->connectors_num; i++) {
+		struct media_entity *con = &decoder->connectors[i].ent;
+		struct media_pad *pad = &decoder->connectors[i].pad;
+		unsigned int port = decoder->connectors[i].base.remote_port;
+		bool is_svideo =
+			decoder->connectors[i].base.type == V4L2_CON_SVIDEO;
+		int flags = i ? 0 : MEDIA_LNK_FL_ENABLED;
+
+		pad->flags = MEDIA_PAD_FL_SOURCE;
+		ret = media_entity_pads_init(con, 1, pad);
+		if (ret < 0)
+			return ret;
+
+		ret = media_device_register_entity(sd->v4l2_dev->mdev, con);
+		if (ret < 0)
+			return ret;
+
+		ret = media_create_pad_link(con, 0, &sd->entity, port, flags);
+		if (ret < 0) {
+			media_device_unregister_entity(con);
+			return ret;
+		}
+
+		if (is_svideo) {
+			/* svideo links to both aip1a and aip1b */
+			ret = media_create_pad_link(con, 0, &sd->entity,
+						    port + 1, flags);
+			if (ret < 0) {
+				media_device_unregister_entity(con);
+				return ret;
+			}
+		}
+
+		/* enable default input */
+		if (flags == MEDIA_LNK_FL_ENABLED) {
+			decoder->input =
+				is_svideo ? TVP5150_SVIDEO :
+				port == 0 ? TVP5150_COMPOSITE0 :
+				TVP5150_COMPOSITE1;
+
+			tvp5150_selmux(sd);
+		}
+	}
+#endif
+	return 0;
+}
+
+
 /* ----------------------------------------------------------------------- */
 
 static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
@@ -1367,6 +1562,10 @@ static const struct v4l2_subdev_ops tvp5150_ops = {
 	.pad = &tvp5150_pad_ops,
 };
 
+static const struct v4l2_subdev_internal_ops tvp5150_internal_ops = {
+	.registered = tvp5150_registered,
+};
+
 /****************************************************************************
 			I2C Client & Driver
  ****************************************************************************/
@@ -1515,38 +1714,171 @@ static int tvp5150_init(struct i2c_client *c)
 	return 0;
 }
 
-static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
+#if defined(CONFIG_MEDIA_CONTROLLER)
+static int tvp5150_add_of_connectors(struct tvp5150 *decoder)
 {
-	struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
-	struct device_node *ep;
-	unsigned int flags;
-	int ret = 0;
+	struct device *dev = decoder->sd.dev;
+	struct tvp5150_connector *connectors;
+	unsigned int connectors_num = decoder->connectors_num;
+	int i, ret;
+
+	/* Allocate and initialize all available input connectors */
+	connectors = devm_kcalloc(dev, connectors_num, sizeof(*connectors),
+				  GFP_KERNEL);
+	if (!connectors)
+		return -ENOMEM;
 
-	ep = of_graph_get_next_endpoint(np, NULL);
-	if (!ep)
-		return -EINVAL;
+	for (i = 0; i < connectors_num; i++) {
+		struct v4l2_fwnode_connector *c = &connectors[i].base;
 
-	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &bus_cfg);
-	if (ret)
-		goto err;
+		ret = v4l2_fwnode_parse_connector(
+				   of_fwnode_handle(decoder->endpoints[i]), c);
+
+		connectors[i].ent.flags = MEDIA_ENT_FL_CONNECTOR;
+		connectors[i].ent.function = c->type == V4L2_CON_SVIDEO ?
+			MEDIA_ENT_F_CONN_SVIDEO : MEDIA_ENT_F_CONN_COMPOSITE;
+		connectors[i].ent.name = c->label;
+	}
+
+	decoder->connectors = connectors;
 
-	flags = bus_cfg.bus.parallel.flags;
+	return 0;
+}
+#endif
 
-	if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
-	    !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
-	      flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
-	      flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
+static int tvp5150_mc_init(struct v4l2_subdev *sd)
+{
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	struct tvp5150 *decoder = to_tvp5150(sd);
+	unsigned int i;
+	int ret;
+
+	sd->entity.ops = &tvp5150_sd_media_ops;
+	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
+
+	/* Initialize all TVP5150 pads */
+	for (i = 0; i < TVP5150_NUM_PADS; i++) {
+		if (i < TVP5150_NUM_PADS - 1) {
+			decoder->pads[i].flags = MEDIA_PAD_FL_SINK;
+			decoder->pads[i].sig_type = PAD_SIGNAL_ANALOG;
+		} else {
+			decoder->pads[i].flags = MEDIA_PAD_FL_SOURCE;
+			decoder->pads[i].sig_type = PAD_SIGNAL_DV;
+		}
+	}
+	ret = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS,
+				     decoder->pads);
+	if (ret < 0)
+		goto out;
+
+	if (IS_ENABLED(CONFIG_OF))
+		ret = tvp5150_add_of_connectors(decoder);
+out:
+	return ret;
+#else
+	return 0;
+#endif
+}
+
+#if defined(CONFIG_OF)
+static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
+{
+	struct device *dev = decoder->sd.dev;
+	struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = V4L2_MBUS_UNKNOWN };
+	struct v4l2_fwnode_connector c;
+	struct device_node *ep_np;
+	unsigned int flags;
+	int ret, i = 0, in = 0;
+	bool found = false;
+
+	/* at least 1 output and 1 input */
+	decoder->endpoints_num = of_graph_get_endpoint_count(np);
+	if (decoder->endpoints_num < 2 || decoder->endpoints_num > 4) {
+		dev_err(dev, "At least 1 input and 1 output must be connected to the device.\n");
 		ret = -EINVAL;
 		goto err;
 	}
 
-	decoder->mbus_type = bus_cfg.bus_type;
+	for_each_endpoint_of_node(np, ep_np) {
+		struct of_endpoint ep;
+
+		of_graph_parse_endpoint(ep_np, &ep);
+		switch (ep.port) {
+			/* fall through */
+		case TVP5150_PAD_AIP1A:
+		case TVP5150_PAD_AIP1B:
+			ret = v4l2_fwnode_parse_connector(
+						   of_fwnode_handle(ep_np), &c);
+			if (c.type != V4L2_CON_COMPOSITE &&
+			    c.type != V4L2_CON_SVIDEO) {
+				dev_err(dev,
+					"Invalid endpoint %d on port %d\n",
+					c.remote_id, c.remote_port);
+				ret = -EINVAL;
+				goto err;
+			}
+			in++;
+			break;
+		case TVP5150_PAD_VID_OUT:
+			ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_np),
+							 &bus_cfg);
+			if (ret)
+				goto err;
+
+			flags = bus_cfg.bus.parallel.flags;
+
+			if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
+			    !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
+			      flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
+			      flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
+				ret = -EINVAL;
+				goto err;
+			}
+
+			decoder->mbus_type = bus_cfg.bus_type;
+			break;
+		default:
+			dev_err(dev, "Invalid port %d for endpoint %pOF\n",
+				ep.port, ep.local_node);
+			ret = -EINVAL;
+			goto err;
+		}
+
+		of_node_get(ep_np);
+		decoder->endpoints[i] = ep_np;
+		i++;
 
+		found = true;
+	}
+
+	decoder->connectors_num = in;
+	return found ? 0 : -ENODEV;
 err:
-	of_node_put(ep);
 	return ret;
 }
 
+static void tvp5150_dt_cleanup(struct tvp5150 *decoder)
+{
+	unsigned int i;
+
+	for (i = 0; i < TVP5150_NUM_PADS; i++)
+		of_node_put(decoder->endpoints[i]);
+}
+
+#else /* !defined(CONFIG_OF) */
+
+static inline int
+tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
+{
+	return 0;
+}
+
+static inline void tvp5150_dt_cleanup(struct tvp5150 *decoder)
+{
+
+}
+#endif
+
 static const char * const tvp5150_test_patterns[2] = {
 	"Disabled",
 	"Black screen"
@@ -1585,7 +1917,7 @@ static int tvp5150_probe(struct i2c_client *c,
 		res = tvp5150_parse_dt(core, np);
 		if (res) {
 			dev_err(sd->dev, "DT parsing error: %d\n", res);
-			return res;
+			goto err_cleanup_dt;
 		}
 	} else {
 		/* Default to BT.656 embedded sync */
@@ -1593,25 +1925,16 @@ static int tvp5150_probe(struct i2c_client *c,
 	}
 
 	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
+	sd->internal_ops = &tvp5150_internal_ops;
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	core->pads[TVP5150_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
-	core->pads[TVP5150_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
-	core->pads[TVP5150_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
-	core->pads[TVP5150_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
-
-	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
-
-	res = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS, core->pads);
-	if (res < 0)
-		return res;
-
-#endif
+	res = tvp5150_mc_init(sd);
+	if (res)
+		goto err_cleanup_dt;
 
 	res = tvp5150_detect_version(core);
 	if (res < 0)
-		return res;
+		goto err_cleanup_dt;
 
 	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
 	core->detected_norm = V4L2_STD_UNKNOWN;
@@ -1637,7 +1960,7 @@ static int tvp5150_probe(struct i2c_client *c,
 	sd->ctrl_handler = &core->hdl;
 	if (core->hdl.error) {
 		res = core->hdl.error;
-		goto err;
+		goto err_free_v4l2_ctrls;
 	}
 
 	tvp5150_set_default(tvp5150_read_std(sd), &core->rect);
@@ -1649,19 +1972,24 @@ static int tvp5150_probe(struct i2c_client *c,
 						tvp5150_isr, IRQF_TRIGGER_HIGH |
 						IRQF_ONESHOT, "tvp5150", core);
 		if (res)
-			goto err;
+			goto err_free_v4l2_ctrls;
 	}
 
 	res = v4l2_async_register_subdev(sd);
 	if (res < 0)
-		goto err;
+		goto err_free_v4l2_ctrls;
 
 	if (debug > 1)
 		tvp5150_log_status(sd);
+
 	return 0;
 
-err:
+err_free_v4l2_ctrls:
 	v4l2_ctrl_handler_free(&core->hdl);
+err_cleanup_dt:
+	if (IS_ENABLED(CONFIG_OF) && np)
+		tvp5150_dt_cleanup(core);
+
 	return res;
 }
 
-- 
2.20.1

Re: [PATCH v5 05/13] media: tvp5150: add input source selection of_graph support

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 20819 bytes --]

Hi Marco,

On Fri, Apr 05, 2019 at 08:03:09AM +0200, Marco Felsch wrote:
> This patch adds the of_graph support to describe the tvp connections.
> Physical the TVP5150 has three ports: AIP1A, AIP1B and YOUT. As result
> of discussion [1],[2] the device-tree maps these ports 1:1. The svideo
> connector must be conneted to port@0/endpoint@1, look at the Documentation
> for more information. Since the TVP5150 is a converter the device-tree
> must contain at least 1-input and 1-output port. The mc-connectors and
> mc-links are only created if the device-tree contains the corresponding
> connector nodes. If more than one connector is available the
> media_entity_operations.link_setup() callback ensures that only one
> connector is active.
>
> [1] https://www.spinics.net/lists/linux-media/msg138545.html
> [2] https://www.spinics.net/lists/linux-media/msg138546.html
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Changelog:
>
> [1] https://patchwork.kernel.org/cover/10794703/
> [2] https://patchwork.kernel.org/cover/10786553/
>
> v5:
> - Fixing build deps:
>   - tvp5150_mc_init: fix CONFIG_MEDIA_CONTROLLER deps
>   - struct tvp5150: drop CONFIG_MEDIA_CONTROLLER conditional property
>     includes. This leads into to complex deps for futher development.
>   - tvp5150_dt_cleanup: enable function only if CONFIG_OF is enabled
>   - tvp5150_parse_dt: enable function only if CONFIG_OF is enabled
>   - tvp5150_probe: call tvp5150_dt_cleanup only if CONFIG_OF is enabled
>
> - Simplify link_setup routine:
>   - use generic connector parsing since both series [1,2] are squashed into
>     one
>   - struct tvp5150: drop pads_state and modify_second_link property
>     due to link_setup() rework.
>   - tvp5150_link_setup: add more comments
>   - tvp5150_link_setup: simply the link setup routine a lot. Edit the 2nd
>     link directly within the driver instead of a recursive media-framework
>     call (__media_entity_setup_link). This improves the readability and
>     shrinks the driver code.
>   - tvp5150_link_setup: disable all active links in case user switches
>     connectors without disable it first.
>   - tvp5150_registered: simplify default link enable path due to link_setup()
>     rework.
>
> - General cleanups
>   - tvp5150_parse_dt: drop unecessary test
>   - tvp5150_parse_dt: add err message due to misconfiguration
>   - tvp5150_parse_dt: make use of V4L2_MBUS_UNKNOWN definition
>   - s/dev_dbg/dev_dbg_lvl
>

Great, this looks much nicer!

> v4:
>  - rebase on top of media_tree/master, fix merge conflict due to commit
>    60359a28d592 ("media: v4l: fwnode: Initialise the V4L2 fwnode endpoints
>    to zero")
>
> v3:
> - probe(): s/err/err_free_v4l2_ctrls
> - drop MC dependency for tvp5150_pads
>
> v2:
> - adapt commit message
> - unify ifdef switches
> - rename tvp5150_valid_input -> tvp5150_of_valid_input, to be more precise
> - mc: use 2-input and 1-output pad
> - mc: link svideo connector to both input pads
> - mc: enable/disable svideo links in one go
> - mc: change link_setup() behaviour, switch the input src don't require a
>       explicite disable before.
> - mc: rename 'local' media_pad param to tvp5150_pad to avoid confusion
> - mc: enable link to the first available connector and set the
>       corresponding tvp5150 input src per default during registered()
> - mc/of: factor out oftree connector allocation
> - of: drop svideo dt port
> - of: move svideo connector to port@0/endpoint@1
> - of: require at least 1-in and 1-out endpoint
>
>  drivers/media/i2c/tvp5150.c | 408 ++++++++++++++++++++++++++++++++----
>  1 file changed, 368 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index 89da921c8886..91504fddb551 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -44,16 +44,29 @@ MODULE_PARM_DESC(debug, "Debug level (0-2)");
>  #define dprintk0(__dev, __arg...) dev_dbg_lvl(__dev, 0, 0, __arg)
>
>  enum tvp5150_pads {
> -	TVP5150_PAD_IF_INPUT,
> +	TVP5150_PAD_AIP1A = TVP5150_COMPOSITE0,
> +	TVP5150_PAD_AIP1B,
>  	TVP5150_PAD_VID_OUT,
>  	TVP5150_NUM_PADS
>  };
>
> +struct tvp5150_connector {
> +	struct v4l2_fwnode_connector base;
> +	struct media_entity ent;
> +	struct media_pad pad;
> +};
> +
>  struct tvp5150 {
>  	struct v4l2_subdev sd;
> -#ifdef CONFIG_MEDIA_CONTROLLER
> +	/* additional additional endpoint for the svideo connector */

s/additional additional/additional/

> +	struct device_node *endpoints[TVP5150_NUM_PADS + 1];
> +	unsigned int endpoints_num;
> +
> +	/* media-ctl properties */
>  	struct media_pad pads[TVP5150_NUM_PADS];
> -#endif
> +	struct tvp5150_connector *connectors;
> +	int connectors_num;
> +
>  	struct v4l2_ctrl_handler hdl;
>  	struct v4l2_rect rect;
>  	struct regmap *regmap;
> @@ -1167,6 +1180,129 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
>  	return 0;
>  }
>
> +/****************************************************************************
> + *			Media entity ops
> + ****************************************************************************/
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +static int tvp5150_set_link(struct media_pad *connector_pad,
> +			    struct media_pad *tvp5150_pad, u32 flags)
> +{
> +	struct media_link *link;
> +
> +	link = media_entity_find_link(connector_pad, tvp5150_pad);
> +	if (!link)
> +		return -EINVAL;
> +
> +	link->flags = flags;
> +	link->reverse->flags = link->flags;
> +
> +	return 0;
> +}
> +
> +static int tvp5150_disable_all_input_links(struct tvp5150 *decoder)
> +{
> +	struct media_pad *connector_pad;
> +	int i, err;

i can be unsigned here

> +
> +	for (i = 0; i < TVP5150_NUM_PADS - 1; i++) {
> +		connector_pad = media_entity_remote_pad(&decoder->pads[i]);
> +		if (!connector_pad)
> +			continue;
> +
> +		err = tvp5150_set_link(connector_pad, &decoder->pads[i], 0);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tvp5150_s_routing(struct v4l2_subdev *sd, u32 input, u32 output,
> +			     u32 config);
> +
> +static int tvp5150_link_setup(struct media_entity *entity,
> +			      const struct media_pad *tvp5150_pad,
> +			      const struct media_pad *remote, u32 flags)
> +{
> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> +	struct tvp5150 *decoder = to_tvp5150(sd);
> +	struct media_pad *other_tvp5150_pad =
> +		&decoder->pads[tvp5150_pad->index ^ 1];
> +	bool is_svideo = false;
> +	int i, err;

i can be unsigned

> +
> +	/*
> +	 * The TVP5150 state is determined by the enabled sink pad link(s).
> +	 * Enabling or disabling the source pad link has no effect.
> +	 */
> +	if (tvp5150_pad->flags & MEDIA_PAD_FL_SOURCE)
> +		return 0;
> +
> +	/* Check if the svideo connector should be enabled */
> +	for (i = 0; i < decoder->connectors_num; i++) {
> +		if (remote->entity == &decoder->connectors[i].ent) {
> +			is_svideo =
> +			   decoder->connectors[i].base.type == V4L2_CON_SVIDEO;
> +			break;
> +		}
> +	}
> +
> +	dev_dbg_lvl(sd->dev, 1, debug, "link setup '%s':%d->'%s':%d[%d]",
> +		    remote->entity->name, remote->index,
> +		    tvp5150_pad->entity->name, tvp5150_pad->index,
> +		    flags & MEDIA_LNK_FL_ENABLED);
> +	if (is_svideo)
> +		dev_dbg_lvl(sd->dev, 1, debug,
> +			    "link setup '%s':%d->'%s':%d[%d]",
> +			    remote->entity->name, remote->index,
> +			    other_tvp5150_pad->entity->name,
> +			    other_tvp5150_pad->index,
> +			    flags & MEDIA_LNK_FL_ENABLED);
> +
> +	/*
> +	 * The TVP5150 has a internal mux which allows the following setup:

an internal

> +	 *
> +	 * comp-connector1  --\
> +	 *		       |---> AIP1A
> +	 *		      /
> +	 * svideo-connector -|
> +	 *		      \
> +	 *		       |---> AIP1B
> +	 * comp-connector2  --/
> +	 *
> +	 * We can't rely on user space that the current connector gets disabled
> +	 * first before enabling the new connector. Disable all active
> +	 * connector links to be on the safe side.
> +	 */
> +	err = tvp5150_disable_all_input_links(decoder);
> +	if (err)
> +		return err;

Not sure here... I think you should refuse, say, to enable
comp-connector2->AIP1B if S_VIDEO is enabled on AIP1A, or maybe that's not
possible, as you have a single linke to AIP1B... Another example: you
should refuse to enable comp-connector1->AIP1A if
svideo-connector->AIP1A is enabled. These are two distinct links, so
this should be possible. Am I wrong?

> +
> +	tvp5150_s_routing(sd, is_svideo ? TVP5150_SVIDEO : tvp5150_pad->index,
> +			  flags & MEDIA_LNK_FL_ENABLED ? TVP5150_NORMAL :
> +			  TVP5150_BLACK_SCREEN, 0);
> +
> +	if (flags & MEDIA_LNK_FL_ENABLED) {
> +		/*
> +		 * S-Video connector is conneted to both ports AIP1A and AIP1B.
> +		 * Both links must be enabled in one-shot regardless which link
> +		 * the user requests.
> +		 */
> +		if (is_svideo) {
> +			err = tvp5150_set_link((struct media_pad *) remote,
> +					       other_tvp5150_pad, flags);
> +			if (err)
> +				return err;
> +		}
> +	}

This is much much nicer than v4! Great job!

> +
> +	return 0;
> +}
> +
> +static const struct media_entity_operations tvp5150_sd_media_ops = {
> +	.link_setup = tvp5150_link_setup,
> +};
> +#endif
>  /****************************************************************************
>  			I2C Command
>   ****************************************************************************/
> @@ -1314,6 +1450,65 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
>  	return 0;
>  }
>
> +static int tvp5150_registered(struct v4l2_subdev *sd)
> +{
> +#if defined(CONFIG_MEDIA_CONTROLLER)

I wonder if it wouldn't make more sense to select MEDIA_CONTROLLER...
How do you configure the device without MC, since I haven't seen support for
platform data?

> +	struct tvp5150 *decoder = to_tvp5150(sd);
> +	unsigned int i;
> +	int ret;
> +
> +	/*
> +	 * Setup connector pads and links. Enable the link to the first
> +	 * available connector per default.
> +	 */
> +	for (i = 0; i < decoder->connectors_num; i++) {
> +		struct media_entity *con = &decoder->connectors[i].ent;
> +		struct media_pad *pad = &decoder->connectors[i].pad;
> +		unsigned int port = decoder->connectors[i].base.remote_port;
> +		bool is_svideo =
> +			decoder->connectors[i].base.type == V4L2_CON_SVIDEO;
> +		int flags = i ? 0 : MEDIA_LNK_FL_ENABLED;
> +
> +		pad->flags = MEDIA_PAD_FL_SOURCE;
> +		ret = media_entity_pads_init(con, 1, pad);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = media_device_register_entity(sd->v4l2_dev->mdev, con);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = media_create_pad_link(con, 0, &sd->entity, port, flags);
> +		if (ret < 0) {
> +			media_device_unregister_entity(con);
> +			return ret;
> +		}
> +
> +		if (is_svideo) {
> +			/* svideo links to both aip1a and aip1b */
> +			ret = media_create_pad_link(con, 0, &sd->entity,
> +						    port + 1, flags);
> +			if (ret < 0) {
> +				media_device_unregister_entity(con);
> +				return ret;
> +			}
> +		}
> +
> +		/* enable default input */
> +		if (flags == MEDIA_LNK_FL_ENABLED) {
> +			decoder->input =
> +				is_svideo ? TVP5150_SVIDEO :
> +				port == 0 ? TVP5150_COMPOSITE0 :
> +				TVP5150_COMPOSITE1;
> +
> +			tvp5150_selmux(sd);
> +		}
> +	}
> +#endif
> +	return 0;
> +}
> +
> +
>  /* ----------------------------------------------------------------------- */
>
>  static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
> @@ -1367,6 +1562,10 @@ static const struct v4l2_subdev_ops tvp5150_ops = {
>  	.pad = &tvp5150_pad_ops,
>  };
>
> +static const struct v4l2_subdev_internal_ops tvp5150_internal_ops = {
> +	.registered = tvp5150_registered,
> +};
> +
>  /****************************************************************************
>  			I2C Client & Driver
>   ****************************************************************************/
> @@ -1515,38 +1714,171 @@ static int tvp5150_init(struct i2c_client *c)
>  	return 0;
>  }
>
> -static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +static int tvp5150_add_of_connectors(struct tvp5150 *decoder)
>  {
> -	struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> -	struct device_node *ep;
> -	unsigned int flags;
> -	int ret = 0;
> +	struct device *dev = decoder->sd.dev;
> +	struct tvp5150_connector *connectors;
> +	unsigned int connectors_num = decoder->connectors_num;
> +	int i, ret;
> +
> +	/* Allocate and initialize all available input connectors */
> +	connectors = devm_kcalloc(dev, connectors_num, sizeof(*connectors),
> +				  GFP_KERNEL);
> +	if (!connectors)
> +		return -ENOMEM;
>
> -	ep = of_graph_get_next_endpoint(np, NULL);
> -	if (!ep)
> -		return -EINVAL;
> +	for (i = 0; i < connectors_num; i++) {
> +		struct v4l2_fwnode_connector *c = &connectors[i].base;
>
> -	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &bus_cfg);
> -	if (ret)
> -		goto err;
> +		ret = v4l2_fwnode_parse_connector(
> +				   of_fwnode_handle(decoder->endpoints[i]), c);
> +
> +		connectors[i].ent.flags = MEDIA_ENT_FL_CONNECTOR;
> +		connectors[i].ent.function = c->type == V4L2_CON_SVIDEO ?
> +			MEDIA_ENT_F_CONN_SVIDEO : MEDIA_ENT_F_CONN_COMPOSITE;
> +		connectors[i].ent.name = c->label;

This is a bit of a duplication of what you're doing at parse_dt time,
where you parse_connector() already. I guess you do this in two steps
because you need to count connectors first, as you allocate their
descriptor structure dynamically.

Have you considered allocating them statically (it's up to 4
connectors, not a big waste) and populate them at parse_dt time? (you
could re-alloc too, but let's not consider that for now).

Bonus point, if you parse_dt() then CONFIG_OF is enabled, and you can
save the
        if (IS_ENABLED(CONFIG_OF))
		ret = tvp5150_add_of_connectors(decoder);

you now have in tvp5150_mc_init()


> +	}
> +
> +	decoder->connectors = connectors;
>
> -	flags = bus_cfg.bus.parallel.flags;
> +	return 0;
> +}
> +#endif
>
> -	if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
> -	    !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
> -	      flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
> -	      flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
> +static int tvp5150_mc_init(struct v4l2_subdev *sd)
> +{
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	struct tvp5150 *decoder = to_tvp5150(sd);
> +	unsigned int i;
> +	int ret;
> +
> +	sd->entity.ops = &tvp5150_sd_media_ops;
> +	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> +
> +	/* Initialize all TVP5150 pads */
> +	for (i = 0; i < TVP5150_NUM_PADS; i++) {
> +		if (i < TVP5150_NUM_PADS - 1) {
> +			decoder->pads[i].flags = MEDIA_PAD_FL_SINK;
> +			decoder->pads[i].sig_type = PAD_SIGNAL_ANALOG;
> +		} else {
> +			decoder->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +			decoder->pads[i].sig_type = PAD_SIGNAL_DV;
> +		}
> +	}
> +	ret = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS,
> +				     decoder->pads);
> +	if (ret < 0)
> +		goto out;
> +
> +	if (IS_ENABLED(CONFIG_OF))
> +		ret = tvp5150_add_of_connectors(decoder);
> +out:
> +	return ret;
> +#else
> +	return 0;
> +#endif

This would really read better as:

#if defined(CONFIG_MEDIA_CONTROLLER)
static int tvp5150_mc_init(struct v4l2_subdev *sd)
{
        ...

}
static int tvp5150_mc_init(struct v4l2_subdev *sd)
{
        ....
}
#else /* defined(CONFIG_MEDIA_CONTROLLER) */
static int tvp5150_mc_init(struct v4l2_subdev *sd)
{
        return 0;
}
static int tvp5150_mc_init(struct v4l2_subdev *sd)
{
        return 0;
}
#endif /* defined(CONFIG_MEDIA_CONTROLLER) */

> +}
> +
> +#if defined(CONFIG_OF)

Do you need this? I tried compiling with OF support disabled and this
#if guard removed and all went fine. After all, all calls to parse_dt
and dt_cleanup are already guarded by an IS_ENABLED(CONFIG_OF)

> +static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> +{
> +	struct device *dev = decoder->sd.dev;
> +	struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = V4L2_MBUS_UNKNOWN };
> +	struct v4l2_fwnode_connector c;

Could you declare this inside the for loop?

> +	struct device_node *ep_np;
> +	unsigned int flags;
> +	int ret, i = 0, in = 0;

i and in could be unsigned

> +	bool found = false;
> +
> +	/* at least 1 output and 1 input */
> +	decoder->endpoints_num = of_graph_get_endpoint_count(np);
> +	if (decoder->endpoints_num < 2 || decoder->endpoints_num > 4) {
> +		dev_err(dev, "At least 1 input and 1 output must be connected to the device.\n");
>  		ret = -EINVAL;
>  		goto err;
>  	}
>
> -	decoder->mbus_type = bus_cfg.bus_type;
> +	for_each_endpoint_of_node(np, ep_np) {
> +		struct of_endpoint ep;
> +
> +		of_graph_parse_endpoint(ep_np, &ep);
> +		switch (ep.port) {
> +			/* fall through */

Move this below the case please

> +		case TVP5150_PAD_AIP1A:
> +		case TVP5150_PAD_AIP1B:
> +			ret = v4l2_fwnode_parse_connector(
> +						   of_fwnode_handle(ep_np), &c);
> +			if (c.type != V4L2_CON_COMPOSITE &&
> +			    c.type != V4L2_CON_SVIDEO) {
> +				dev_err(dev,
> +					"Invalid endpoint %d on port %d\n",
> +					c.remote_id, c.remote_port);
> +				ret = -EINVAL;
> +				goto err;
> +			}
> +			in++;
> +			break;
> +		case TVP5150_PAD_VID_OUT:
> +			ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_np),
> +							 &bus_cfg);
> +			if (ret)
> +				goto err;
> +
> +			flags = bus_cfg.bus.parallel.flags;
> +
> +			if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
> +			    !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
> +			      flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
> +			      flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
> +				ret = -EINVAL;
> +				goto err;
> +			}
> +
> +			decoder->mbus_type = bus_cfg.bus_type;
> +			break;
> +		default:
> +			dev_err(dev, "Invalid port %d for endpoint %pOF\n",
> +				ep.port, ep.local_node);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +
> +		of_node_get(ep_np);
> +		decoder->endpoints[i] = ep_np;
> +		i++;
>
> +		found = true;
> +	}
> +
> +	decoder->connectors_num = in;
> +	return found ? 0 : -ENODEV;
>  err:
> -	of_node_put(ep);
>  	return ret;
>  }
>
> +static void tvp5150_dt_cleanup(struct tvp5150 *decoder)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < TVP5150_NUM_PADS; i++)
> +		of_node_put(decoder->endpoints[i]);
> +}
> +
> +#else /* !defined(CONFIG_OF) */
> +
> +static inline int
> +tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> +{
> +	return 0;
> +}
> +
> +static inline void tvp5150_dt_cleanup(struct tvp5150 *decoder)
> +{
> +
> +}
> +#endif
> +
>  static const char * const tvp5150_test_patterns[2] = {
>  	"Disabled",
>  	"Black screen"
> @@ -1585,7 +1917,7 @@ static int tvp5150_probe(struct i2c_client *c,
>  		res = tvp5150_parse_dt(core, np);
>  		if (res) {
>  			dev_err(sd->dev, "DT parsing error: %d\n", res);
> -			return res;
> +			goto err_cleanup_dt;
>  		}
>  	} else {
>  		/* Default to BT.656 embedded sync */
> @@ -1593,25 +1925,16 @@ static int tvp5150_probe(struct i2c_client *c,
>  	}
>
>  	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
> +	sd->internal_ops = &tvp5150_internal_ops;
>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	core->pads[TVP5150_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
> -	core->pads[TVP5150_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
> -	core->pads[TVP5150_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
> -	core->pads[TVP5150_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
> -
> -	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> -
> -	res = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS, core->pads);
> -	if (res < 0)
> -		return res;
> -
> -#endif
> +	res = tvp5150_mc_init(sd);
> +	if (res)
> +		goto err_cleanup_dt;
>
>  	res = tvp5150_detect_version(core);
>  	if (res < 0)
> -		return res;
> +		goto err_cleanup_dt;
>
>  	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
>  	core->detected_norm = V4L2_STD_UNKNOWN;
> @@ -1637,7 +1960,7 @@ static int tvp5150_probe(struct i2c_client *c,
>  	sd->ctrl_handler = &core->hdl;
>  	if (core->hdl.error) {
>  		res = core->hdl.error;
> -		goto err;
> +		goto err_free_v4l2_ctrls;
>  	}
>
>  	tvp5150_set_default(tvp5150_read_std(sd), &core->rect);
> @@ -1649,19 +1972,24 @@ static int tvp5150_probe(struct i2c_client *c,
>  						tvp5150_isr, IRQF_TRIGGER_HIGH |
>  						IRQF_ONESHOT, "tvp5150", core);
>  		if (res)
> -			goto err;
> +			goto err_free_v4l2_ctrls;
>  	}
>
>  	res = v4l2_async_register_subdev(sd);
>  	if (res < 0)
> -		goto err;
> +		goto err_free_v4l2_ctrls;
>
>  	if (debug > 1)
>  		tvp5150_log_status(sd);
> +
>  	return 0;
>
> -err:
> +err_free_v4l2_ctrls:
>  	v4l2_ctrl_handler_free(&core->hdl);
> +err_cleanup_dt:
> +	if (IS_ENABLED(CONFIG_OF) && np)

is there any case where CONFIG_OF is enabled and dev->of_node might be
NULL ? How did the driver probed in first place if that was the case.

I made to the end of this big patch :p quite a nice one!
Thanks
  j

> +		tvp5150_dt_cleanup(core);
> +
>  	return res;
>  }
>
> --
> 2.20.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 05/13] media: tvp5150: add input source selection of_graph support

[Marco Felsch]

Hi Jacopo,

On 19-04-05 16:45, Jacopo Mondi wrote:
> Hi Marco,
> 
> On Fri, Apr 05, 2019 at 08:03:09AM +0200, Marco Felsch wrote:
> > This patch adds the of_graph support to describe the tvp connections.
> > Physical the TVP5150 has three ports: AIP1A, AIP1B and YOUT. As result
> > of discussion [1],[2] the device-tree maps these ports 1:1. The svideo
> > connector must be conneted to port@0/endpoint@1, look at the Documentation
> > for more information. Since the TVP5150 is a converter the device-tree
> > must contain at least 1-input and 1-output port. The mc-connectors and
> > mc-links are only created if the device-tree contains the corresponding
> > connector nodes. If more than one connector is available the
> > media_entity_operations.link_setup() callback ensures that only one
> > connector is active.
> >
> > [1] https://www.spinics.net/lists/linux-media/msg138545.html
> > [2] https://www.spinics.net/lists/linux-media/msg138546.html
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > Changelog:
> >
> > [1] https://patchwork.kernel.org/cover/10794703/
> > [2] https://patchwork.kernel.org/cover/10786553/
> >
> > v5:
> > - Fixing build deps:
> >   - tvp5150_mc_init: fix CONFIG_MEDIA_CONTROLLER deps
> >   - struct tvp5150: drop CONFIG_MEDIA_CONTROLLER conditional property
> >     includes. This leads into to complex deps for futher development.
> >   - tvp5150_dt_cleanup: enable function only if CONFIG_OF is enabled
> >   - tvp5150_parse_dt: enable function only if CONFIG_OF is enabled
> >   - tvp5150_probe: call tvp5150_dt_cleanup only if CONFIG_OF is enabled
> >
> > - Simplify link_setup routine:
> >   - use generic connector parsing since both series [1,2] are squashed into
> >     one
> >   - struct tvp5150: drop pads_state and modify_second_link property
> >     due to link_setup() rework.
> >   - tvp5150_link_setup: add more comments
> >   - tvp5150_link_setup: simply the link setup routine a lot. Edit the 2nd
> >     link directly within the driver instead of a recursive media-framework
> >     call (__media_entity_setup_link). This improves the readability and
> >     shrinks the driver code.
> >   - tvp5150_link_setup: disable all active links in case user switches
> >     connectors without disable it first.
> >   - tvp5150_registered: simplify default link enable path due to link_setup()
> >     rework.
> >
> > - General cleanups
> >   - tvp5150_parse_dt: drop unecessary test
> >   - tvp5150_parse_dt: add err message due to misconfiguration
> >   - tvp5150_parse_dt: make use of V4L2_MBUS_UNKNOWN definition
> >   - s/dev_dbg/dev_dbg_lvl
> >
> 
> Great, this looks much nicer!
> 
> > v4:
> >  - rebase on top of media_tree/master, fix merge conflict due to commit
> >    60359a28d592 ("media: v4l: fwnode: Initialise the V4L2 fwnode endpoints
> >    to zero")
> >
> > v3:
> > - probe(): s/err/err_free_v4l2_ctrls
> > - drop MC dependency for tvp5150_pads
> >
> > v2:
> > - adapt commit message
> > - unify ifdef switches
> > - rename tvp5150_valid_input -> tvp5150_of_valid_input, to be more precise
> > - mc: use 2-input and 1-output pad
> > - mc: link svideo connector to both input pads
> > - mc: enable/disable svideo links in one go
> > - mc: change link_setup() behaviour, switch the input src don't require a
> >       explicite disable before.
> > - mc: rename 'local' media_pad param to tvp5150_pad to avoid confusion
> > - mc: enable link to the first available connector and set the
> >       corresponding tvp5150 input src per default during registered()
> > - mc/of: factor out oftree connector allocation
> > - of: drop svideo dt port
> > - of: move svideo connector to port@0/endpoint@1
> > - of: require at least 1-in and 1-out endpoint
> >
> >  drivers/media/i2c/tvp5150.c | 408 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 368 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > index 89da921c8886..91504fddb551 100644
> > --- a/drivers/media/i2c/tvp5150.c
> > +++ b/drivers/media/i2c/tvp5150.c
> > @@ -44,16 +44,29 @@ MODULE_PARM_DESC(debug, "Debug level (0-2)");
> >  #define dprintk0(__dev, __arg...) dev_dbg_lvl(__dev, 0, 0, __arg)
> >
> >  enum tvp5150_pads {
> > -	TVP5150_PAD_IF_INPUT,
> > +	TVP5150_PAD_AIP1A = TVP5150_COMPOSITE0,
> > +	TVP5150_PAD_AIP1B,
> >  	TVP5150_PAD_VID_OUT,
> >  	TVP5150_NUM_PADS
> >  };
> >
> > +struct tvp5150_connector {
> > +	struct v4l2_fwnode_connector base;
> > +	struct media_entity ent;
> > +	struct media_pad pad;
> > +};
> > +
> >  struct tvp5150 {
> >  	struct v4l2_subdev sd;
> > -#ifdef CONFIG_MEDIA_CONTROLLER
> > +	/* additional additional endpoint for the svideo connector */
> 
> s/additional additional/additional/

Damn, fixed it.

> > +	struct device_node *endpoints[TVP5150_NUM_PADS + 1];
> > +	unsigned int endpoints_num;
> > +
> > +	/* media-ctl properties */
> >  	struct media_pad pads[TVP5150_NUM_PADS];
> > -#endif
> > +	struct tvp5150_connector *connectors;
> > +	int connectors_num;
> > +
> >  	struct v4l2_ctrl_handler hdl;
> >  	struct v4l2_rect rect;
> >  	struct regmap *regmap;
> > @@ -1167,6 +1180,129 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >
> > +/****************************************************************************
> > + *			Media entity ops
> > + ****************************************************************************/
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +static int tvp5150_set_link(struct media_pad *connector_pad,
> > +			    struct media_pad *tvp5150_pad, u32 flags)
> > +{
> > +	struct media_link *link;
> > +
> > +	link = media_entity_find_link(connector_pad, tvp5150_pad);
> > +	if (!link)
> > +		return -EINVAL;
> > +
> > +	link->flags = flags;
> > +	link->reverse->flags = link->flags;
> > +
> > +	return 0;
> > +}
> > +
> > +static int tvp5150_disable_all_input_links(struct tvp5150 *decoder)
> > +{
> > +	struct media_pad *connector_pad;
> > +	int i, err;
> 
> i can be unsigned here

Okay

> > +
> > +	for (i = 0; i < TVP5150_NUM_PADS - 1; i++) {
> > +		connector_pad = media_entity_remote_pad(&decoder->pads[i]);
> > +		if (!connector_pad)
> > +			continue;
> > +
> > +		err = tvp5150_set_link(connector_pad, &decoder->pads[i], 0);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int tvp5150_s_routing(struct v4l2_subdev *sd, u32 input, u32 output,
> > +			     u32 config);
> > +
> > +static int tvp5150_link_setup(struct media_entity *entity,
> > +			      const struct media_pad *tvp5150_pad,
> > +			      const struct media_pad *remote, u32 flags)
> > +{
> > +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > +	struct tvp5150 *decoder = to_tvp5150(sd);
> > +	struct media_pad *other_tvp5150_pad =
> > +		&decoder->pads[tvp5150_pad->index ^ 1];
> > +	bool is_svideo = false;
> > +	int i, err;
> 
> i can be unsigned

Okay

> > +
> > +	/*
> > +	 * The TVP5150 state is determined by the enabled sink pad link(s).
> > +	 * Enabling or disabling the source pad link has no effect.
> > +	 */
> > +	if (tvp5150_pad->flags & MEDIA_PAD_FL_SOURCE)
> > +		return 0;
> > +
> > +	/* Check if the svideo connector should be enabled */
> > +	for (i = 0; i < decoder->connectors_num; i++) {
> > +		if (remote->entity == &decoder->connectors[i].ent) {
> > +			is_svideo =
> > +			   decoder->connectors[i].base.type == V4L2_CON_SVIDEO;
> > +			break;
> > +		}
> > +	}
> > +
> > +	dev_dbg_lvl(sd->dev, 1, debug, "link setup '%s':%d->'%s':%d[%d]",
> > +		    remote->entity->name, remote->index,
> > +		    tvp5150_pad->entity->name, tvp5150_pad->index,
> > +		    flags & MEDIA_LNK_FL_ENABLED);
> > +	if (is_svideo)
> > +		dev_dbg_lvl(sd->dev, 1, debug,
> > +			    "link setup '%s':%d->'%s':%d[%d]",
> > +			    remote->entity->name, remote->index,
> > +			    other_tvp5150_pad->entity->name,
> > +			    other_tvp5150_pad->index,
> > +			    flags & MEDIA_LNK_FL_ENABLED);
> > +
> > +	/*
> > +	 * The TVP5150 has a internal mux which allows the following setup:
> 
> an internal

Yes

> > +	 *
> > +	 * comp-connector1  --\
> > +	 *		       |---> AIP1A
> > +	 *		      /
> > +	 * svideo-connector -|
> > +	 *		      \
> > +	 *		       |---> AIP1B
> > +	 * comp-connector2  --/
> > +	 *
> > +	 * We can't rely on user space that the current connector gets disabled
> > +	 * first before enabling the new connector. Disable all active
> > +	 * connector links to be on the safe side.
> > +	 */
> > +	err = tvp5150_disable_all_input_links(decoder);
> > +	if (err)
> > +		return err;
> 
> Not sure here... I think you should refuse, say, to enable
> comp-connector2->AIP1B if S_VIDEO is enabled on AIP1A, or maybe that's not
> possible, as you have a single linke to AIP1B... Another example: you
> should refuse to enable comp-connector1->AIP1A if
> svideo-connector->AIP1A is enabled. These are two distinct links, so
> this should be possible. Am I wrong?

I tought the diagram and prose makes it cleaner, damn.. Thanks for
asking maybe I should think about rewording it..

In short, the svideo needs both links (AIP1A, AIP1B) enabled so there is
no AIP1A or AIP1B case since both are on. In a previouse version of this
series I added the refusing mechanism but Mauro had some concerns about
that. Since the media-api don't cover that he said the dynamic switching
should be the way to go since it is easier for the user space.

So the current solution is that there can be one link enabled (comp1 or
comp2) or two links (svideo). To handle these cases and avoid much
complexity I disable all active links first since it isn't forbidden by
the media-api.

> > +
> > +	tvp5150_s_routing(sd, is_svideo ? TVP5150_SVIDEO : tvp5150_pad->index,
> > +			  flags & MEDIA_LNK_FL_ENABLED ? TVP5150_NORMAL :
> > +			  TVP5150_BLACK_SCREEN, 0);
> > +
> > +	if (flags & MEDIA_LNK_FL_ENABLED) {
> > +		/*
> > +		 * S-Video connector is conneted to both ports AIP1A and AIP1B.
> > +		 * Both links must be enabled in one-shot regardless which link
> > +		 * the user requests.
> > +		 */
> > +		if (is_svideo) {
> > +			err = tvp5150_set_link((struct media_pad *) remote,
> > +					       other_tvp5150_pad, flags);
> > +			if (err)
> > +				return err;
> > +		}
> > +	}
> 
> This is much much nicer than v4! Great job!
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct media_entity_operations tvp5150_sd_media_ops = {
> > +	.link_setup = tvp5150_link_setup,
> > +};
> > +#endif
> >  /****************************************************************************
> >  			I2C Command
> >   ****************************************************************************/
> > @@ -1314,6 +1450,65 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
> >  	return 0;
> >  }
> >
> > +static int tvp5150_registered(struct v4l2_subdev *sd)
> > +{
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> 
> I wonder if it wouldn't make more sense to select MEDIA_CONTROLLER...
> How do you configure the device without MC, since I haven't seen support for
> platform data?

@Mauro
Can you say something about the non MC case?

I know that the em28xx device is using this driver and if I get it right
the em28xx calls the s_routing callback to set the specific input
source.

So in that case the MC mustn't available/enabled.

Luckily the em28xx can use most of the tvp5150 default config values.

> 
> > +	struct tvp5150 *decoder = to_tvp5150(sd);
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	/*
> > +	 * Setup connector pads and links. Enable the link to the first
> > +	 * available connector per default.
> > +	 */
> > +	for (i = 0; i < decoder->connectors_num; i++) {
> > +		struct media_entity *con = &decoder->connectors[i].ent;
> > +		struct media_pad *pad = &decoder->connectors[i].pad;
> > +		unsigned int port = decoder->connectors[i].base.remote_port;
> > +		bool is_svideo =
> > +			decoder->connectors[i].base.type == V4L2_CON_SVIDEO;
> > +		int flags = i ? 0 : MEDIA_LNK_FL_ENABLED;
> > +
> > +		pad->flags = MEDIA_PAD_FL_SOURCE;
> > +		ret = media_entity_pads_init(con, 1, pad);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = media_device_register_entity(sd->v4l2_dev->mdev, con);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = media_create_pad_link(con, 0, &sd->entity, port, flags);
> > +		if (ret < 0) {
> > +			media_device_unregister_entity(con);
> > +			return ret;
> > +		}
> > +
> > +		if (is_svideo) {
> > +			/* svideo links to both aip1a and aip1b */
> > +			ret = media_create_pad_link(con, 0, &sd->entity,
> > +						    port + 1, flags);
> > +			if (ret < 0) {
> > +				media_device_unregister_entity(con);
> > +				return ret;
> > +			}
> > +		}
> > +
> > +		/* enable default input */
> > +		if (flags == MEDIA_LNK_FL_ENABLED) {
> > +			decoder->input =
> > +				is_svideo ? TVP5150_SVIDEO :
> > +				port == 0 ? TVP5150_COMPOSITE0 :
> > +				TVP5150_COMPOSITE1;
> > +
> > +			tvp5150_selmux(sd);
> > +		}
> > +	}
> > +#endif
> > +	return 0;
> > +}
> > +
> > +
> >  /* ----------------------------------------------------------------------- */
> >
> >  static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
> > @@ -1367,6 +1562,10 @@ static const struct v4l2_subdev_ops tvp5150_ops = {
> >  	.pad = &tvp5150_pad_ops,
> >  };
> >
> > +static const struct v4l2_subdev_internal_ops tvp5150_internal_ops = {
> > +	.registered = tvp5150_registered,
> > +};
> > +
> >  /****************************************************************************
> >  			I2C Client & Driver
> >   ****************************************************************************/
> > @@ -1515,38 +1714,171 @@ static int tvp5150_init(struct i2c_client *c)
> >  	return 0;
> >  }
> >
> > -static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +static int tvp5150_add_of_connectors(struct tvp5150 *decoder)
> >  {
> > -	struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> > -	struct device_node *ep;
> > -	unsigned int flags;
> > -	int ret = 0;
> > +	struct device *dev = decoder->sd.dev;
> > +	struct tvp5150_connector *connectors;
> > +	unsigned int connectors_num = decoder->connectors_num;
> > +	int i, ret;
> > +
> > +	/* Allocate and initialize all available input connectors */
> > +	connectors = devm_kcalloc(dev, connectors_num, sizeof(*connectors),
> > +				  GFP_KERNEL);
> > +	if (!connectors)
> > +		return -ENOMEM;
> >
> > -	ep = of_graph_get_next_endpoint(np, NULL);
> > -	if (!ep)
> > -		return -EINVAL;
> > +	for (i = 0; i < connectors_num; i++) {
> > +		struct v4l2_fwnode_connector *c = &connectors[i].base;
> >
> > -	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &bus_cfg);
> > -	if (ret)
> > -		goto err;
> > +		ret = v4l2_fwnode_parse_connector(
> > +				   of_fwnode_handle(decoder->endpoints[i]), c);
> > +
> > +		connectors[i].ent.flags = MEDIA_ENT_FL_CONNECTOR;
> > +		connectors[i].ent.function = c->type == V4L2_CON_SVIDEO ?
> > +			MEDIA_ENT_F_CONN_SVIDEO : MEDIA_ENT_F_CONN_COMPOSITE;
> > +		connectors[i].ent.name = c->label;
> 
> This is a bit of a duplication of what you're doing at parse_dt time,
> where you parse_connector() already. I guess you do this in two steps
> because you need to count connectors first, as you allocate their
> descriptor structure dynamically.
> 
> Have you considered allocating them statically (it's up to 4
> connectors, not a big waste) and populate them at parse_dt time? (you
> could re-alloc too, but let's not consider that for now).
> 
> Bonus point, if you parse_dt() then CONFIG_OF is enabled, and you can
> save the
>         if (IS_ENABLED(CONFIG_OF))
> 		ret = tvp5150_add_of_connectors(decoder);
> 
> you now have in tvp5150_mc_init()

Good point and your're right I parse it twice a time. I tought about
your suggestion but then I also tought about the case where it can be
get more complicated. Imagine you have 5 composite connectors: two
connected to AIP1A and 3 connected to AIP1B. Still the same behaviour,
only one link at the time can be enbaled. In that case you can't use
the static allocation. Anyway it isn't supported by the driver right now
but it is easier to add the support if we alloc them dynamic.

My idea between splitting was to have a correct abstraction.
Parsing/validation is done by parse_dt, the tvp5150_add_of_connectors()
relies on that result since it didn't check it again. Of course
v4l2_fwnode_parse_connector() gets parsed twice but this is done during
probe.

But your're right with that

>         if (IS_ENABLED(CONFIG_OF))
> 		ret = tvp5150_add_of_connectors(decoder);

and your last review note. That check isn't correct since it can be the
case where CONFIG_OF and CONFIG_MEDIA_CONTROLLER is enabled but the
device using the driver isn't OF-capable (e.g. em28xx usb-device). So
the check must be:

	if (decoder->connectors_num)
			ret = tvp5150_add_of_connectors(decoder);

This ensures that tvp5150_add_of_connectors() gets called only for
OF-capable devices.

> 
> > +	}
> > +
> > +	decoder->connectors = connectors;
> >
> > -	flags = bus_cfg.bus.parallel.flags;
> > +	return 0;
> > +}
> > +#endif
> >
> > -	if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
> > -	    !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
> > -	      flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
> > -	      flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
> > +static int tvp5150_mc_init(struct v4l2_subdev *sd)
> > +{
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +	struct tvp5150 *decoder = to_tvp5150(sd);
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	sd->entity.ops = &tvp5150_sd_media_ops;
> > +	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> > +
> > +	/* Initialize all TVP5150 pads */
> > +	for (i = 0; i < TVP5150_NUM_PADS; i++) {
> > +		if (i < TVP5150_NUM_PADS - 1) {
> > +			decoder->pads[i].flags = MEDIA_PAD_FL_SINK;
> > +			decoder->pads[i].sig_type = PAD_SIGNAL_ANALOG;
> > +		} else {
> > +			decoder->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > +			decoder->pads[i].sig_type = PAD_SIGNAL_DV;
> > +		}
> > +	}
> > +	ret = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS,
> > +				     decoder->pads);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	if (IS_ENABLED(CONFIG_OF))
> > +		ret = tvp5150_add_of_connectors(decoder);
> > +out:
> > +	return ret;
> > +#else
> > +	return 0;
> > +#endif
> 
> This would really read better as:

Good point, I will change that or should I rather change the
tvp5150_mc_init() call during the probe to

tvp5150_probe()
{
	...

	if(IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
		tvp5150_mc_init()

	...
}

> #if defined(CONFIG_MEDIA_CONTROLLER)
> static int tvp5150_mc_init(struct v4l2_subdev *sd)
> {
>         ...
> 
> }
> static int tvp5150_mc_init(struct v4l2_subdev *sd)
> {
>         ....
> }
> #else /* defined(CONFIG_MEDIA_CONTROLLER) */
> static int tvp5150_mc_init(struct v4l2_subdev *sd)
> {
>         return 0;
> }
> static int tvp5150_mc_init(struct v4l2_subdev *sd)
> {
>         return 0;
> }
> #endif /* defined(CONFIG_MEDIA_CONTROLLER) */
> 
> > +}
> > +
> > +#if defined(CONFIG_OF)
> 
> Do you need this? I tried compiling with OF support disabled and this
> #if guard removed and all went fine. After all, all calls to parse_dt
> and dt_cleanup are already guarded by an IS_ENABLED(CONFIG_OF)

My goal was to reduce the code size in case OF isn't enabled since the
call don't depend only on the CONFIG_OF this function won't be threw
away in case CONFIG_OF is disabled if I got it right.

	if (IS_ENABLED(CONFIG_OF) && np)

But you're right I can remove it to reducing the ifdef switches.

> > +static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> > +{
> > +	struct device *dev = decoder->sd.dev;
> > +	struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = V4L2_MBUS_UNKNOWN };
> > +	struct v4l2_fwnode_connector c;
> 
> Could you declare this inside the for loop?

Of course.

> > +	struct device_node *ep_np;
> > +	unsigned int flags;

Moved flags inside the for loop too.

> > +	int ret, i = 0, in = 0;
> 
> i and in could be unsigned

okay

> > +	bool found = false;
> > +
> > +	/* at least 1 output and 1 input */
> > +	decoder->endpoints_num = of_graph_get_endpoint_count(np);
> > +	if (decoder->endpoints_num < 2 || decoder->endpoints_num > 4) {
> > +		dev_err(dev, "At least 1 input and 1 output must be connected to the device.\n");
> >  		ret = -EINVAL;
> >  		goto err;
> >  	}
> >
> > -	decoder->mbus_type = bus_cfg.bus_type;
> > +	for_each_endpoint_of_node(np, ep_np) {
> > +		struct of_endpoint ep;
> > +
> > +		of_graph_parse_endpoint(ep_np, &ep);
> > +		switch (ep.port) {
> > +			/* fall through */
> 
> Move this below the case please

Yes.

> > +		case TVP5150_PAD_AIP1A:
> > +		case TVP5150_PAD_AIP1B:
> > +			ret = v4l2_fwnode_parse_connector(
> > +						   of_fwnode_handle(ep_np), &c);
> > +			if (c.type != V4L2_CON_COMPOSITE &&
> > +			    c.type != V4L2_CON_SVIDEO) {
> > +				dev_err(dev,
> > +					"Invalid endpoint %d on port %d\n",
> > +					c.remote_id, c.remote_port);
> > +				ret = -EINVAL;
> > +				goto err;
> > +			}
> > +			in++;
> > +			break;
> > +		case TVP5150_PAD_VID_OUT:
> > +			ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_np),
> > +							 &bus_cfg);
> > +			if (ret)
> > +				goto err;
> > +
> > +			flags = bus_cfg.bus.parallel.flags;
> > +
> > +			if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
> > +			    !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
> > +			      flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
> > +			      flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
> > +				ret = -EINVAL;
> > +				goto err;
> > +			}
> > +
> > +			decoder->mbus_type = bus_cfg.bus_type;
> > +			break;
> > +		default:
> > +			dev_err(dev, "Invalid port %d for endpoint %pOF\n",
> > +				ep.port, ep.local_node);
> > +			ret = -EINVAL;
> > +			goto err;
> > +		}
> > +
> > +		of_node_get(ep_np);
> > +		decoder->endpoints[i] = ep_np;
> > +		i++;
> >
> > +		found = true;
> > +	}
> > +
> > +	decoder->connectors_num = in;
> > +	return found ? 0 : -ENODEV;
> >  err:
> > -	of_node_put(ep);
> >  	return ret;
> >  }
> >
> > +static void tvp5150_dt_cleanup(struct tvp5150 *decoder)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < TVP5150_NUM_PADS; i++)
> > +		of_node_put(decoder->endpoints[i]);
> > +}
> > +
> > +#else /* !defined(CONFIG_OF) */
> > +
> > +static inline int
> > +tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline void tvp5150_dt_cleanup(struct tvp5150 *decoder)
> > +{
> > +
> > +}
> > +#endif
> > +
> >  static const char * const tvp5150_test_patterns[2] = {
> >  	"Disabled",
> >  	"Black screen"
> > @@ -1585,7 +1917,7 @@ static int tvp5150_probe(struct i2c_client *c,
> >  		res = tvp5150_parse_dt(core, np);
> >  		if (res) {
> >  			dev_err(sd->dev, "DT parsing error: %d\n", res);
> > -			return res;
> > +			goto err_cleanup_dt;
> >  		}
> >  	} else {
> >  		/* Default to BT.656 embedded sync */
> > @@ -1593,25 +1925,16 @@ static int tvp5150_probe(struct i2c_client *c,
> >  	}
> >
> >  	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
> > +	sd->internal_ops = &tvp5150_internal_ops;
> >  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >
> > -#if defined(CONFIG_MEDIA_CONTROLLER)
> > -	core->pads[TVP5150_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
> > -	core->pads[TVP5150_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
> > -	core->pads[TVP5150_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
> > -	core->pads[TVP5150_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
> > -
> > -	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> > -
> > -	res = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS, core->pads);
> > -	if (res < 0)
> > -		return res;
> > -
> > -#endif
> > +	res = tvp5150_mc_init(sd);
> > +	if (res)
> > +		goto err_cleanup_dt;
> >
> >  	res = tvp5150_detect_version(core);
> >  	if (res < 0)
> > -		return res;
> > +		goto err_cleanup_dt;
> >
> >  	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
> >  	core->detected_norm = V4L2_STD_UNKNOWN;
> > @@ -1637,7 +1960,7 @@ static int tvp5150_probe(struct i2c_client *c,
> >  	sd->ctrl_handler = &core->hdl;
> >  	if (core->hdl.error) {
> >  		res = core->hdl.error;
> > -		goto err;
> > +		goto err_free_v4l2_ctrls;
> >  	}
> >
> >  	tvp5150_set_default(tvp5150_read_std(sd), &core->rect);
> > @@ -1649,19 +1972,24 @@ static int tvp5150_probe(struct i2c_client *c,
> >  						tvp5150_isr, IRQF_TRIGGER_HIGH |
> >  						IRQF_ONESHOT, "tvp5150", core);
> >  		if (res)
> > -			goto err;
> > +			goto err_free_v4l2_ctrls;
> >  	}
> >
> >  	res = v4l2_async_register_subdev(sd);
> >  	if (res < 0)
> > -		goto err;
> > +		goto err_free_v4l2_ctrls;
> >
> >  	if (debug > 1)
> >  		tvp5150_log_status(sd);
> > +
> >  	return 0;
> >
> > -err:
> > +err_free_v4l2_ctrls:
> >  	v4l2_ctrl_handler_free(&core->hdl);
> > +err_cleanup_dt:
> > +	if (IS_ENABLED(CONFIG_OF) && np)
> 
> is there any case where CONFIG_OF is enabled and dev->of_node might be
> NULL ? How did the driver probed in first place if that was the case.

I think that can be the case if you have an embedded device (of-support)
where a tvp5150 is connected directly and an attached em28xx usb device.

> 
> I made to the end of this big patch :p quite a nice one!

Thanks for your feedback =)

> Thanks
>   j
> 

Regards,
  Marco

> > +		tvp5150_dt_cleanup(core);
> > +
> >  	return res;
> >  }
> >
> > --
> > 2.20.1
> >



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v5 06/13] media: dt-bindings: tvp5150: Add input port connectors DT bindings

[Marco Felsch]

The TVP5150/1 decoders support different video input sources to their
AIP1A/B pins.

Possible configurations are as follows:
  - Analog Composite signal connected to AIP1A.
  - Analog Composite signal connected to AIP1B.
  - Analog S-Video Y (luminance) and C (chrominance)
    signals connected to AIP1A and AIP1B respectively.

This patch extends the device tree bindings documentation to describe
how the input connectors for these devices should be defined in a DT.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changelog:

v3:
- remove examples for one and two inputs
- replace space by tabs

v2:
- adapt port layout in accordance with
  https://www.spinics.net/lists/linux-media/msg138546.html with the
  svideo-connector deviation (use only one endpoint)

 .../devicetree/bindings/media/i2c/tvp5150.txt | 92 +++++++++++++++++--
 1 file changed, 85 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
index 8c0fc1a26bf0..bdd273d8b44d 100644
--- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
+++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
@@ -12,11 +12,31 @@ Optional Properties:
 - pdn-gpios: phandle for the GPIO connected to the PDN pin, if any.
 - reset-gpios: phandle for the GPIO connected to the RESETB pin, if any.
 
-The device node must contain one 'port' child node for its digital output
-video port, in accordance with the video interface bindings defined in
-Documentation/devicetree/bindings/media/video-interfaces.txt.
+The device node must contain one 'port' child node per device physical input
+and output port, in accordance with the video interface bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt. The port nodes
+are numbered as follows
 
-Required Endpoint Properties for parallel synchronization:
+	  Name		Type		Port
+	--------------------------------------
+	  AIP1A		sink		0
+	  AIP1B		sink		1
+	  Y-OUT		src		2
+
+The device node must contain at least one sink port and the src port. Each input
+port must be linked to an endpoint defined in
+Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt. The
+port/connector layout is as follows
+
+tvp-5150 port@0 (AIP1A)
+	endpoint@0 -----------> Comp0-Con  port
+	endpoint@1 -----------> Svideo-Con port
+tvp-5150 port@1 (AIP1B)
+	endpoint   -----------> Comp1-Con  port
+tvp-5150 port@2
+	endpoint (video bitstream output at YOUT[0-7] parallel bus)
+
+Required Endpoint Properties for parallel synchronization on output port:
 
 - hsync-active: active state of the HSYNC signal. Must be <1> (HIGH).
 - vsync-active: active state of the VSYNC signal. Must be <1> (HIGH).
@@ -26,17 +46,75 @@ Required Endpoint Properties for parallel synchronization:
 If none of hsync-active, vsync-active and field-even-active is specified,
 the endpoint is assumed to use embedded BT.656 synchronization.
 
-Example:
+Example - three input sources:
+
+comp_connector_0 {
+	compatible = "composite-video-connector";
+	label = "Composite0";
+
+	port {
+		composite0_to_tvp5150: endpoint {
+			remote-endpoint = <&tvp5150_to_composite0>;
+		};
+	};
+};
+
+comp_connector_1 {
+	compatible = "composite-video-connector";
+	label = "Composite1";
+
+	port {
+		composite1_to_tvp5150: endpoint {
+			remote-endpoint = <&tvp5150_to_composite1>;
+		};
+	};
+};
+
+svid_connector {
+	compatible = "svideo-connector";
+	label = "S-Video";
+
+	port {
+		svideo_to_tvp5150: endpoint {
+			remote-endpoint = <&tvp5150_to_svideo>;
+		};
+	};
+};
 
 &i2c2 {
-	...
 	tvp5150@5c {
 		compatible = "ti,tvp5150";
 		reg = <0x5c>;
 		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
 		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
 
-		port {
+		port@0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+
+			tvp5150_to_composite0: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&composite0_to_tvp5150>;
+			};
+
+			tvp5150_to_svideo: endpoint@1 {
+				reg = <1>;
+				remote-endpoint = <&svideo_to_tvp5150>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			tvp5150_to_composite1: endpoint {
+                                remote-endpoint = <&composite1_to_tvp5150>;
+			};
+		};
+
+		port@2 {
+			reg = <2>;
+
 			tvp5150_1: endpoint {
 				remote-endpoint = <&ccdc_ep>;
 			};
-- 
2.20.1

[PATCH v5 07/13] media: tvp5150: add FORMAT_TRY support for get/set selection handlers

[Marco Felsch]

Since commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops")
the 'which' field for set/get_selection must be FORMAT_ACTIVE. There is
no way to try different selections. The patch adds a helper function to
select the correct selection memory space (sub-device file handle or
driver state) which will be set/returned.

The TVP5150 AVID will be updated if the 'which' field is FORMAT_ACTIVE
and the requested selection rectangle differs from the already set one.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:

v5:
 - handle stub for v4l2_subdev_get_try_crop() internal since commit
   ("media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*")
   isn't anymore part of this series.
 - add error handling of __tvp5150_get_pad_crop()
v4:
 - fix merge conflict due to rebase on top of media-tree/master
 - __tvp5150_get_pad_crop(): cosmetic alignment fixes

 drivers/media/i2c/tvp5150.c | 130 ++++++++++++++++++++++++++----------
 1 file changed, 96 insertions(+), 34 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 91504fddb551..d126127628c5 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -19,6 +19,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-mc.h>
+#include <media/v4l2-rect.h>
 
 #include "tvp5150_reg.h"
 
@@ -997,20 +998,48 @@ static void tvp5150_set_default(v4l2_std_id std, struct v4l2_rect *crop)
 		crop->height = TVP5150_V_MAX_OTHERS;
 }
 
+static struct v4l2_rect *
+__tvp5150_get_pad_crop(struct tvp5150 *decoder,
+		       struct v4l2_subdev_pad_config *cfg, unsigned int pad,
+		       enum v4l2_subdev_format_whence which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
+		return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad);
+#else
+		return ERR_PTR(-ENOTTY);
+#endif
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &decoder->rect;
+	default:
+		return NULL;
+	}
+}
+
 static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
 			    struct v4l2_subdev_pad_config *cfg,
 			    struct v4l2_subdev_format *format)
 {
 	struct v4l2_mbus_framefmt *f;
+	struct v4l2_rect *__crop;
 	struct tvp5150 *decoder = to_tvp5150(sd);
 
 	if (!format || (format->pad != TVP5150_PAD_VID_OUT))
 		return -EINVAL;
 
 	f = &format->format;
+	__crop = __tvp5150_get_pad_crop(decoder, cfg, format->pad,
+					format->which);
+	if (IS_ERR_OR_NULL(__crop)) {
+		if (!__crop)
+			return -EINVAL;
+		else
+			return PTR_ERR(__crop);
+	}
 
-	f->width = decoder->rect.width;
-	f->height = decoder->rect.height / 2;
+	f->width = __crop->width;
+	f->height = __crop->height / 2;
 
 	f->code = TVP5150_MBUS_FMT;
 	f->field = TVP5150_FIELD;
@@ -1021,17 +1050,51 @@ static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+unsigned int tvp5150_get_hmax(struct v4l2_subdev *sd)
+{
+	struct tvp5150 *decoder = to_tvp5150(sd);
+	v4l2_std_id std;
+
+	/* Calculate height based on current standard */
+	if (decoder->norm == V4L2_STD_ALL)
+		std = tvp5150_read_std(sd);
+	else
+		std = decoder->norm;
+
+	return (std & V4L2_STD_525_60) ?
+		TVP5150_V_MAX_525_60 : TVP5150_V_MAX_OTHERS;
+}
+
+static inline void
+__tvp5150_set_selection(struct v4l2_subdev *sd, struct v4l2_rect rect)
+{
+	struct tvp5150 *decoder = to_tvp5150(sd);
+	unsigned int hmax = tvp5150_get_hmax(sd);
+
+	regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top);
+	regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP,
+		     rect.top + rect.height - hmax);
+	regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB,
+		     rect.left >> TVP5150_CROP_SHIFT);
+	regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB,
+		     rect.left | (1 << TVP5150_CROP_SHIFT));
+	regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB,
+		     (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >>
+		     TVP5150_CROP_SHIFT);
+	regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB,
+		     rect.left + rect.width - TVP5150_MAX_CROP_LEFT);
+}
+
 static int tvp5150_set_selection(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_pad_config *cfg,
 				 struct v4l2_subdev_selection *sel)
 {
 	struct tvp5150 *decoder = to_tvp5150(sd);
 	struct v4l2_rect rect = sel->r;
-	v4l2_std_id std;
-	int hmax;
+	struct v4l2_rect *__crop;
+	unsigned int hmax;
 
-	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
-	    sel->target != V4L2_SEL_TGT_CROP)
+	if (sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
 
 	dev_dbg_lvl(sd->dev, 1, debug, "%s left=%d, top=%d, width=%d, height=%d\n",
@@ -1040,17 +1103,7 @@ static int tvp5150_set_selection(struct v4l2_subdev *sd,
 	/* tvp5150 has some special limits */
 	rect.left = clamp(rect.left, 0, TVP5150_MAX_CROP_LEFT);
 	rect.top = clamp(rect.top, 0, TVP5150_MAX_CROP_TOP);
-
-	/* Calculate height based on current standard */
-	if (decoder->norm == V4L2_STD_ALL)
-		std = tvp5150_read_std(sd);
-	else
-		std = decoder->norm;
-
-	if (std & V4L2_STD_525_60)
-		hmax = TVP5150_V_MAX_525_60;
-	else
-		hmax = TVP5150_V_MAX_OTHERS;
+	hmax = tvp5150_get_hmax(sd);
 
 	/*
 	 * alignments:
@@ -1063,20 +1116,23 @@ static int tvp5150_set_selection(struct v4l2_subdev *sd,
 			      hmax - TVP5150_MAX_CROP_TOP - rect.top,
 			      hmax - rect.top, 0, 0);
 
-	regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_START, rect.top);
-	regmap_write(decoder->regmap, TVP5150_VERT_BLANKING_STOP,
-		     rect.top + rect.height - hmax);
-	regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_MSB,
-		     rect.left >> TVP5150_CROP_SHIFT);
-	regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_ST_LSB,
-		     rect.left | (1 << TVP5150_CROP_SHIFT));
-	regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_MSB,
-		     (rect.left + rect.width - TVP5150_MAX_CROP_LEFT) >>
-		     TVP5150_CROP_SHIFT);
-	regmap_write(decoder->regmap, TVP5150_ACT_VD_CROP_STP_LSB,
-		     rect.left + rect.width - TVP5150_MAX_CROP_LEFT);
+	__crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad, sel->which);
+	if (IS_ERR_OR_NULL(__crop)) {
+		if (!__crop)
+			return -EINVAL;
+		else
+			return PTR_ERR(__crop);
+	}
+
+	/*
+	 * Update output image size if the selection (crop) rectangle size or
+	 * position has been modified.
+	 */
+	if (!v4l2_rect_equal(&rect, __crop))
+		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+			__tvp5150_set_selection(sd, rect);
 
-	decoder->rect = rect;
+	*__crop = rect;
 
 	return 0;
 }
@@ -1086,11 +1142,9 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_selection *sel)
 {
 	struct tvp5150 *decoder = container_of(sd, struct tvp5150, sd);
+	struct v4l2_rect *__crop;
 	v4l2_std_id std;
 
-	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-		return -EINVAL;
-
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP_BOUNDS:
 		sel->r.left = 0;
@@ -1108,7 +1162,15 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd,
 			sel->r.height = TVP5150_V_MAX_OTHERS;
 		return 0;
 	case V4L2_SEL_TGT_CROP:
-		sel->r = decoder->rect;
+		__crop = __tvp5150_get_pad_crop(decoder, cfg, sel->pad,
+						sel->which);
+		if (IS_ERR_OR_NULL(__crop)) {
+			if (!__crop)
+				return -EINVAL;
+			else
+				return PTR_ERR(__crop);
+		}
+		sel->r = *__crop;
 		return 0;
 	default:
 		return -EINVAL;
-- 
2.20.1

[PATCH v5 08/13] media: tvp5150: initialize subdev before parsing device tree

[Marco Felsch]

From: Michael Tretter <m.tretter@pengutronix.de>

There are several debug prints in the tvp5150_parse_dt() function, which
do not print the prefix, because the v4l2_subdev is not initialized, yet.

Initialize the v4l2_subdev before parsing the device tree to fix the
debug messages.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/tvp5150.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index d126127628c5..82a9cb9bf736 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1974,6 +1974,9 @@ static int tvp5150_probe(struct i2c_client *c,
 
 	core->regmap = map;
 	sd = &core->sd;
+	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
+	sd->internal_ops = &tvp5150_internal_ops;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 	if (IS_ENABLED(CONFIG_OF) && np) {
 		res = tvp5150_parse_dt(core, np);
@@ -1986,10 +1989,6 @@ static int tvp5150_probe(struct i2c_client *c,
 		core->mbus_type = V4L2_MBUS_BT656;
 	}
 
-	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
-	sd->internal_ops = &tvp5150_internal_ops;
-	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-
 	res = tvp5150_mc_init(sd);
 	if (res)
 		goto err_cleanup_dt;
-- 
2.20.1

[PATCH v5 09/13] media: tvp5150: add s_power callback

[Marco Felsch]

Don't en-/disable the interrupts during s_stream because someone can
disable the stream but wants to get informed if the stream is locked
again. So keep the interrupts enabled the whole time the pipeline is
opened.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/tvp5150.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 82a9cb9bf736..2cae1b590ca4 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1368,11 +1368,26 @@ static const struct media_entity_operations tvp5150_sd_media_ops = {
 /****************************************************************************
 			I2C Command
  ****************************************************************************/
+static int tvp5150_s_power(struct  v4l2_subdev *sd, int on)
+{
+	struct tvp5150 *decoder = to_tvp5150(sd);
+	unsigned int val = 0;
+
+	if (on)
+		val = TVP5150_INT_A_LOCK;
+
+	if (decoder->irq)
+		/* Enable / Disable lock interrupt */
+		regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
+				   TVP5150_INT_A_LOCK, val);
+
+	return 0;
+}
 
 static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct tvp5150 *decoder = to_tvp5150(sd);
-	unsigned int mask, val = 0, int_val = 0;
+	unsigned int mask, val = 0;
 
 	mask = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE |
 	       TVP5150_MISC_CTL_CLOCK_OE;
@@ -1385,15 +1400,10 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable)
 			val = decoder->lock ? decoder->oe : 0;
 		else
 			val = decoder->oe;
-		int_val = TVP5150_INT_A_LOCK;
 		v4l2_subdev_notify_event(&decoder->sd, &tvp5150_ev_fmt);
 	}
 
 	regmap_update_bits(decoder->regmap, TVP5150_MISC_CTL, mask, val);
-	if (decoder->irq)
-		/* Enable / Disable lock interrupt */
-		regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
-				   TVP5150_INT_A_LOCK, int_val);
 
 	return 0;
 }
@@ -1584,6 +1594,7 @@ static const struct v4l2_subdev_core_ops tvp5150_core_ops = {
 	.g_register = tvp5150_g_register,
 	.s_register = tvp5150_s_register,
 #endif
+	.s_power = tvp5150_s_power,
 };
 
 static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
-- 
2.20.1

[PATCH v5 10/13] media: dt-bindings: tvp5150: cleanup bindings stlye

[Marco Felsch]

Use underlines to highlight optional and required properties. This is
quite common for all bindings. Align descriptions and start sentence
with uppercase letter. Also reword the usage of the required
endpoint properties for the output port in case BT.656 should be used.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 .../devicetree/bindings/media/i2c/tvp5150.txt | 30 +++++++++++--------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
index bdd273d8b44d..82fff80fa63a 100644
--- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
+++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
@@ -5,12 +5,14 @@ The TVP5150 and TVP5151 are video decoders that convert baseband NTSC and PAL
 with discrete syncs or 8-bit ITU-R BT.656 with embedded syncs output formats.
 
 Required Properties:
-- compatible: value must be "ti,tvp5150"
-- reg: I2C slave address
+====================
+- compatible:	Value must be "ti,tvp5150".
+- reg:		I2C slave address.
 
 Optional Properties:
-- pdn-gpios: phandle for the GPIO connected to the PDN pin, if any.
-- reset-gpios: phandle for the GPIO connected to the RESETB pin, if any.
+====================
+- pdn-gpios:	Phandle for the GPIO connected to the PDN pin, if any.
+- reset-gpios:	Phandle for the GPIO connected to the RESETB pin, if any.
 
 The device node must contain one 'port' child node per device physical input
 and output port, in accordance with the video interface bindings defined in
@@ -24,9 +26,8 @@ are numbered as follows
 	  Y-OUT		src		2
 
 The device node must contain at least one sink port and the src port. Each input
-port must be linked to an endpoint defined in
-Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt. The
-port/connector layout is as follows
+port must be linked to an endpoint defined in [1]. The port/connector layout is
+as follows
 
 tvp-5150 port@0 (AIP1A)
 	endpoint@0 -----------> Comp0-Con  port
@@ -37,14 +38,17 @@ tvp-5150 port@2
 	endpoint (video bitstream output at YOUT[0-7] parallel bus)
 
 Required Endpoint Properties for parallel synchronization on output port:
+=========================================================================
 
-- hsync-active: active state of the HSYNC signal. Must be <1> (HIGH).
-- vsync-active: active state of the VSYNC signal. Must be <1> (HIGH).
-- field-even-active: field signal level during the even field data
-  transmission. Must be <0>.
+- hsync-active:		Active state of the HSYNC signal. Must be <1> (HIGH).
+- vsync-active:		Active state of the VSYNC signal. Must be <1> (HIGH).
+- field-even-active:	Field signal level during the even field data
+			transmission. Must be <0>.
 
-If none of hsync-active, vsync-active and field-even-active is specified,
-the endpoint is assumed to use embedded BT.656 synchronization.
+Note: Do not specify any of these properties if you want to use the embedded
+      BT.656 synchronization.
+
+[1] Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt.
 
 Example - three input sources:
 
-- 
2.20.1

Re: [PATCH v5 10/13] media: dt-bindings: tvp5150: cleanup bindings stlye

[Rob Herring]

On Fri,  5 Apr 2019 08:03:14 +0200, Marco Felsch wrote:
> Use underlines to highlight optional and required properties. This is
> quite common for all bindings. Align descriptions and start sentence
> with uppercase letter. Also reword the usage of the required
> endpoint properties for the output port in case BT.656 should be used.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  .../devicetree/bindings/media/i2c/tvp5150.txt | 30 +++++++++++--------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v5 11/13] media: dt-bindings: tvp5150: add optional tvnorms documentation

[Marco Felsch]

Document the optional binding to limit the possible tv-norms on the
input connectors.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 Documentation/devicetree/bindings/media/i2c/tvp5150.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
index 82fff80fa63a..970be21fb191 100644
--- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
+++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
@@ -48,6 +48,13 @@ Required Endpoint Properties for parallel synchronization on output port:
 Note: Do not specify any of these properties if you want to use the embedded
       BT.656 synchronization.
 
+Optional Connector Properties:
+==============================
+
+- tvnorms: Set the possible signals to which the hardware tries to lock instead
+	   of using the autodetection mechnism. Please look at [1] for more
+	   information.
+
 [1] Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt.
 
 Example - three input sources:
@@ -55,6 +62,7 @@ Example - three input sources:
 comp_connector_0 {
 	compatible = "composite-video-connector";
 	label = "Composite0";
+	tvnorms = <TVNORM_PAL_M>; /* limit to pal-m signals */
 
 	port {
 		composite0_to_tvp5150: endpoint {
@@ -66,6 +74,7 @@ comp_connector_0 {
 comp_connector_1 {
 	compatible = "composite-video-connector";
 	label = "Composite1";
+	tvnorms = <TVNORM_NTSC_M>; /* limit to ntsc-m signals */
 
 	port {
 		composite1_to_tvp5150: endpoint {
-- 
2.20.1

Re: [PATCH v5 11/13] media: dt-bindings: tvp5150: add optional tvnorms documentation

[Rob Herring]

On Fri,  5 Apr 2019 08:03:15 +0200, Marco Felsch wrote:
> Document the optional binding to limit the possible tv-norms on the
> input connectors.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/media/i2c/tvp5150.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v5 12/13] media: tvp5150: add support to limit tv norms on connector

[Marco Felsch]

The tvp5150 accepts NTSC(M,J,4.43), PAL (B,D,G,H,I,M,N) and SECAM video
data and is able to auto-detect the input signal. The auto-detection
does not work if the connector does not receive an input signal and the
tvp5150 might not be configured correctly. This misconfiguration leads
into wrong decoded video streams if the tvp5150 gets powered on before
the video signal is present.

Limit the supported tv norms according to the actual selected connector
to avoid a misconfiguration.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
[1] https://patchwork.kernel.org/cover/10794703/

v5:
- probe() initialize supported tv-norms according the given connectors
  if they are available.
- check if media-controller is used. Don't limit the norm if it isn't
  used.
- add more logic to be smarter during connector changing so it is
  intuitiver for the user space.

v2-v4:
- nothing since the patch was squashed from series [1] into this
  series.

 drivers/media/i2c/tvp5150.c | 69 +++++++++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 2cae1b590ca4..6af04db45b7e 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -32,6 +32,13 @@
 #define TVP5150_MBUS_FMT	MEDIA_BUS_FMT_UYVY8_2X8
 #define TVP5150_FIELD		V4L2_FIELD_ALTERNATE
 #define TVP5150_COLORSPACE	V4L2_COLORSPACE_SMPTE170M
+#define TVP5150_STD_MASK	(V4L2_STD_NTSC     | \
+				 V4L2_STD_NTSC_443 | \
+				 V4L2_STD_PAL      | \
+				 V4L2_STD_PAL_M    | \
+				 V4L2_STD_PAL_N    | \
+				 V4L2_STD_PAL_Nc   | \
+				 V4L2_STD_SECAM)
 
 MODULE_DESCRIPTION("Texas Instruments TVP5150A/TVP5150AM1/TVP5151 video decoder driver");
 MODULE_AUTHOR("Mauro Carvalho Chehab");
@@ -66,6 +73,7 @@ struct tvp5150 {
 	/* media-ctl properties */
 	struct media_pad pads[TVP5150_NUM_PADS];
 	struct tvp5150_connector *connectors;
+	struct tvp5150_connector *cur_connector;
 	int connectors_num;
 
 	struct v4l2_ctrl_handler hdl;
@@ -785,17 +793,28 @@ static int tvp5150_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
 static int tvp5150_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 {
 	struct tvp5150 *decoder = to_tvp5150(sd);
+	struct tvp5150_connector *cur_con = decoder->cur_connector;
+	v4l2_std_id supported_norms = cur_con ?
+		cur_con->base.connector.analog.supported_tvnorms : V4L2_STD_ALL;
 
 	if (decoder->norm == std)
 		return 0;
 
+	/*
+	 * check if requested std or group of std's is/are supported by the
+	 * connector
+	 */
+	if ((supported_norms & std) == 0)
+		return -EINVAL;
+
 	/* Change cropping height limits */
 	if (std & V4L2_STD_525_60)
 		decoder->rect.height = TVP5150_V_MAX_525_60;
 	else
 		decoder->rect.height = TVP5150_V_MAX_OTHERS;
 
-	decoder->norm = std;
+	/* set only the specific supported std in case of group of std's */
+	decoder->norm = supported_norms & std;
 
 	return tvp5150_set_std(sd, std);
 }
@@ -1345,6 +1364,8 @@ static int tvp5150_link_setup(struct media_entity *entity,
 			  TVP5150_BLACK_SCREEN, 0);
 
 	if (flags & MEDIA_LNK_FL_ENABLED) {
+		u32 new_norm;
+
 		/*
 		 * S-Video connector is conneted to both ports AIP1A and AIP1B.
 		 * Both links must be enabled in one-shot regardless which link
@@ -1356,6 +1377,26 @@ static int tvp5150_link_setup(struct media_entity *entity,
 			if (err)
 				return err;
 		}
+
+		/* Update the current connector */
+		decoder->cur_connector =
+			container_of(remote, struct tvp5150_connector, pad);
+
+		/*
+		 * Do nothing if the new connector supports the same tv-norms as
+		 * the old one.
+		 */
+		new_norm = decoder->norm &
+			decoder->cur_connector->base.connector.analog.supported_tvnorms;
+		if (decoder->norm == new_norm)
+			return 0;
+
+		/*
+		 * Fallback to the new connector tv-norms if we can't find any
+		 * common between the current tv-norm and the new one.
+		 */
+		tvp5150_s_std(sd, new_norm ? new_norm :
+			decoder->cur_connector->base.connector.analog.supported_tvnorms);
 	}
 
 	return 0;
@@ -1574,6 +1615,9 @@ static int tvp5150_registered(struct v4l2_subdev *sd)
 				TVP5150_COMPOSITE1;
 
 			tvp5150_selmux(sd);
+			decoder->cur_connector = &decoder->connectors[i];
+			tvp5150_s_std(sd,
+				decoder->connectors[i].base.connector.analog.supported_tvnorms);
 		}
 	}
 #endif
@@ -1890,6 +1934,11 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 				ret = -EINVAL;
 				goto err;
 			}
+			if (!(c.connector.analog.supported_tvnorms &
+			    TVP5150_STD_MASK))
+				dev_warn(dev,
+					"Unsupported tv-norm on connector %s.\n",
+					c.label);
 			in++;
 			break;
 		case TVP5150_PAD_VID_OUT:
@@ -2008,7 +2057,23 @@ static int tvp5150_probe(struct i2c_client *c,
 	if (res < 0)
 		goto err_cleanup_dt;
 
-	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
+	/*
+	 * Iterate over all available connectors in case they are supported and
+	 * successfully parsed. Fallback to default autodetect in case they
+	 * aren't supported.
+	 */
+	if (core->connectors) {
+		struct v4l2_fwnode_connector *con;
+		int i;
+
+		for (i = 0; i < core->connectors_num; i++) {
+			con = &core->connectors[i].base;
+			core->norm |= con->connector.analog.supported_tvnorms;
+		}
+	} else {
+		core->norm = V4L2_STD_ALL;
+	}
+
 	core->detected_norm = V4L2_STD_UNKNOWN;
 	core->input = TVP5150_COMPOSITE1;
 	core->enable = true;
-- 
2.20.1

[PATCH v5 13/13] media: tvp5150: make debug output more readable

[Marco Felsch]

The debug output for tvp5150_selmux() isn't really intuitive. Register
values are printed decimal formatted and the input/output driver states
are printed as enum. Even more the "normal" output enum mapps to zero so
a active output will printing output=0 and a inactive output=1.

Change this by brinting the register values hex formatted and the states
as more readable string.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/tvp5150.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 6af04db45b7e..469dc7e24ae9 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -302,9 +302,12 @@ static void tvp5150_selmux(struct v4l2_subdev *sd)
 		break;
 	}
 
-	dev_dbg_lvl(sd->dev, 1, debug, "Selecting video route: route input=%i, output=%i => tvp5150 input=%i, opmode=%i\n",
-			decoder->input, decoder->output,
-			input, opmode);
+	dev_dbg_lvl(sd->dev, 1, debug,
+		    "Selecting video route: route input=%s, output=%s => tvp5150 input=0x%02x, opmode=0x%02x\n",
+		    decoder->input == 0 ? "aip1a" :
+		    decoder->input == 2 ? "aip1b" : "svideo",
+		    decoder->output == 0 ? "normal" : "black-frame-gen",
+		    input, opmode);
 
 	regmap_write(decoder->regmap, TVP5150_OP_MODE_CTL, opmode);
 	regmap_write(decoder->regmap, TVP5150_VD_IN_SRC_SEL_1, input);
-- 
2.20.1

Re: [PATCH v5 00/13] TVP5150 new features

[Mauro Carvalho Chehab]

Hi Marco,

Em Fri,  5 Apr 2019 08:03:04 +0200
Marco Felsch <m.felsch@pengutronix.de> escreveu:

> Hi,
> 
> few months ago I send my v4 of this series [1] unfortunately I got no
> feedback from Mauro but Jacopos feedback was quite helpfull =)
> 
> After my v4 I send another series which adds a generic way to parse
> connector endpoints [2]. To make it easier for everyone I squashed both
> series [1,2] into this one.
> 
> I recognized that patch ("media: v4l2-subdev: add stubs for
> v4l2_subdev_get_try_*") was a possible blocker for this series so I
> factored the patch out [3].
> 
> My main goal with the v5 was to simplify the link_setup() code a lot.
> There were also some build-dep issues which I fixed too. Last
> significant improvement is done on patch ("media: tvp5150: add
> support to limit tv norms on connector").
> 
> I've tested it on a custom hardware and compile tested it too.
> 
> @Mauro
> Please let me know how I can help you to speed up the review progress
> since I wanted to get those changes merged in the near future =)

I asked Hans to do the review. I prefer that he would do reviews
on patches from V4L2 drivers. Also, I'm currently OOT.


> 
> @Sakari, Hans
> I've added you to the series since I made some core changes.
> 
> New patches (wasn't part of [1] or [2]):
>   media: dt-bindings: tvp5150: cleanup bindings stlye
>   media: dt-bindings: tvp5150: add optional tvnorms documentation
>   media: tvp5150: make debug output more readable
> 
> New (squashed) patches (was part of [2]):
>   dt-bindings: connector: analog: add tv norms property
>   media: v4l2-fwnode: add v4l2_fwnode_connector
>   media: v4l2-fwnode: add initial connector parsing support
>   media: tvp5150: add support to limit tv norms on connector
> 
> Droped patches:
>   media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*
> 
> [1] https://patchwork.ozlabs.org/cover/1032891/
> [2] https://patchwork.kernel.org/cover/10794703/
> [3] https://www.mail-archive.com/linux-media@vger.kernel.org/msg146065.html
> 
> Regards,
> 	Marco
> 
> Javier Martinez Canillas (1):
>   partial revert of "[media] tvp5150: add HW input connectors support"
> 
> Marco Felsch (11):
>   dt-bindings: connector: analog: add tv norms property
>   media: v4l2-fwnode: add v4l2_fwnode_connector
>   media: v4l2-fwnode: add initial connector parsing support
>   media: tvp5150: add input source selection of_graph support
>   media: dt-bindings: tvp5150: Add input port connectors DT bindings
>   media: tvp5150: add FORMAT_TRY support for get/set selection handlers
>   media: tvp5150: add s_power callback
>   media: dt-bindings: tvp5150: cleanup bindings stlye
>   media: dt-bindings: tvp5150: add optional tvnorms documentation
>   media: tvp5150: add support to limit tv norms on connector
>   media: tvp5150: make debug output more readable
> 
> Michael Tretter (1):
>   media: tvp5150: initialize subdev before parsing device tree
> 
>  .../display/connector/analog-tv-connector.txt |   4 +
>  .../devicetree/bindings/media/i2c/tvp5150.txt | 125 +++-
>  drivers/media/i2c/tvp5150.c                   | 673 +++++++++++++-----
>  drivers/media/v4l2-core/v4l2-fwnode.c         | 113 +++
>  include/dt-bindings/media/tvnorms.h           |  42 ++
>  include/dt-bindings/media/tvp5150.h           |   2 -
>  include/media/v4l2-connector.h                |  34 +
>  include/media/v4l2-fwnode.h                   |  49 ++
>  8 files changed, 850 insertions(+), 192 deletions(-)
>  create mode 100644 include/dt-bindings/media/tvnorms.h
>  create mode 100644 include/media/v4l2-connector.h
> 



Thanks,
Mauro

Re: [PATCH v5 00/13] TVP5150 new features

[Marco Felsch]

Hi Mauro,

On 19-04-06 08:19, Mauro Carvalho Chehab wrote:
> Hi Marco,
> 
> Em Fri,  5 Apr 2019 08:03:04 +0200
> Marco Felsch <m.felsch@pengutronix.de> escreveu:
> 
> > Hi,
> > 
> > few months ago I send my v4 of this series [1] unfortunately I got no
> > feedback from Mauro but Jacopos feedback was quite helpfull =)
> > 
> > After my v4 I send another series which adds a generic way to parse
> > connector endpoints [2]. To make it easier for everyone I squashed both
> > series [1,2] into this one.
> > 
> > I recognized that patch ("media: v4l2-subdev: add stubs for
> > v4l2_subdev_get_try_*") was a possible blocker for this series so I
> > factored the patch out [3].
> > 
> > My main goal with the v5 was to simplify the link_setup() code a lot.
> > There were also some build-dep issues which I fixed too. Last
> > significant improvement is done on patch ("media: tvp5150: add
> > support to limit tv norms on connector").
> > 
> > I've tested it on a custom hardware and compile tested it too.
> > 
> > @Mauro
> > Please let me know how I can help you to speed up the review progress
> > since I wanted to get those changes merged in the near future =)
> 
> I asked Hans to do the review. I prefer that he would do reviews
> on patches from V4L2 drivers. Also, I'm currently OOT.

Thanks for your reply. Sorry for asking but what does that mean for the
tvp5150 related patches?

@Hans or Sakari
Do you have some feedback or concerns about patch 1-3? I integrated
Jacopo's feedback and wanna send a v6 tomorrow or on friday. Depending on
your feedback.

Regards,
  Marco

> > 
> > @Sakari, Hans
> > I've added you to the series since I made some core changes.
> > 
> > New patches (wasn't part of [1] or [2]):
> >   media: dt-bindings: tvp5150: cleanup bindings stlye
> >   media: dt-bindings: tvp5150: add optional tvnorms documentation
> >   media: tvp5150: make debug output more readable
> > 
> > New (squashed) patches (was part of [2]):
> >   dt-bindings: connector: analog: add tv norms property
> >   media: v4l2-fwnode: add v4l2_fwnode_connector
> >   media: v4l2-fwnode: add initial connector parsing support
> >   media: tvp5150: add support to limit tv norms on connector
> > 
> > Droped patches:
> >   media: v4l2-subdev: add stubs for v4l2_subdev_get_try_*
> > 
> > [1] https://patchwork.ozlabs.org/cover/1032891/
> > [2] https://patchwork.kernel.org/cover/10794703/
> > [3] https://www.mail-archive.com/linux-media@vger.kernel.org/msg146065.html
> > 
> > Regards,
> > 	Marco
> > 
> > Javier Martinez Canillas (1):
> >   partial revert of "[media] tvp5150: add HW input connectors support"
> > 
> > Marco Felsch (11):
> >   dt-bindings: connector: analog: add tv norms property
> >   media: v4l2-fwnode: add v4l2_fwnode_connector
> >   media: v4l2-fwnode: add initial connector parsing support
> >   media: tvp5150: add input source selection of_graph support
> >   media: dt-bindings: tvp5150: Add input port connectors DT bindings
> >   media: tvp5150: add FORMAT_TRY support for get/set selection handlers
> >   media: tvp5150: add s_power callback
> >   media: dt-bindings: tvp5150: cleanup bindings stlye
> >   media: dt-bindings: tvp5150: add optional tvnorms documentation
> >   media: tvp5150: add support to limit tv norms on connector
> >   media: tvp5150: make debug output more readable
> > 
> > Michael Tretter (1):
> >   media: tvp5150: initialize subdev before parsing device tree
> > 
> >  .../display/connector/analog-tv-connector.txt |   4 +
> >  .../devicetree/bindings/media/i2c/tvp5150.txt | 125 +++-
> >  drivers/media/i2c/tvp5150.c                   | 673 +++++++++++++-----
> >  drivers/media/v4l2-core/v4l2-fwnode.c         | 113 +++
> >  include/dt-bindings/media/tvnorms.h           |  42 ++
> >  include/dt-bindings/media/tvp5150.h           |   2 -
> >  include/media/v4l2-connector.h                |  34 +
> >  include/media/v4l2-fwnode.h                   |  49 ++
> >  8 files changed, 850 insertions(+), 192 deletions(-)
> >  create mode 100644 include/dt-bindings/media/tvnorms.h
> >  create mode 100644 include/media/v4l2-connector.h
> > 
> 
> 
> 
> Thanks,
> Mauro
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |