[PATCH 0/5] TV norms limit and TVP5150 implementation

[PATCH 0/5] TV norms limit and TVP5150 implementation

[Marco Felsch]

Hi,

in short this series adds the support to limit the tv norms on an
analog-connector.

I recognized that all drivers dealing with connectors implemented
their own parsing routine due to the lack of a generic one. A generic
parsing routine needs a connector container which contain common
data and connector specific data. This series implements the connector
container struct and the generic parsing routine. At the moment only
analog-connectors are fully supported but adding the others should
be simple.

Finally the TVP5150 driver is converted to the generic connector and
make use of the new 'tv norms limiting' feature.

I'm not sure if the series applies cleanly without [1].

Regards,
Marco

[1] https://www.mail-archive.com/linux-media@vger.kernel.org/msg143925.html

Marco Felsch (5):
  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: make use of generic connector parsing
  media: tvp5150: add support to limit tv norms on connector

 .../display/connector/analog-tv-connector.txt |   4 +
 drivers/media/i2c/tvp5150.c                   | 116 +++++++++---------
 drivers/media/v4l2-core/v4l2-fwnode.c         | 113 +++++++++++++++++
 include/dt-bindings/media/tvnorms.h           |  42 +++++++
 include/media/v4l2-connector.h                |  34 +++++
 include/media/v4l2-fwnode.h                   |  49 ++++++++
 6 files changed, 302 insertions(+), 56 deletions(-)
 create mode 100644 include/dt-bindings/media/tvnorms.h
 create mode 100644 include/media/v4l2-connector.h

-- 
2.20.1

[PATCH 1/5] 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>
---
 .../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 1/5] dt-bindings: connector: analog: add tv norms property

[Rob Herring]

On Sat,  2 Feb 2019 13:10:00 +0100, 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>
> ---
>  .../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
> 

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

[PATCH 2/5] 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 6d9d9f1839ac..cf87e819800f 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

[PATCH 3/5] 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>
---
 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 9bfedd7596a1..56f581e00197 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 */
+		strlcpy(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 cf87e819800f..c00cb346b5eb 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

[PATCH 4/5] media: tvp5150: make use of generic connector parsing

[Marco Felsch]

Drop the driver specific connector parsing since we can use the generic
parsing provided by the v4l2-fwnode core.

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

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 1f0dd9d3655c..f3a2ad00a40d 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -59,10 +59,9 @@ enum tvp5150_pads_state {
 };
 
 struct tvp5150_connector {
+	struct v4l2_fwnode_connector base;
 	struct media_entity ent;
 	struct media_pad pad;
-	unsigned int port_num;
-	bool is_svideo;
 };
 #endif
 
@@ -1310,7 +1309,8 @@ static int tvp5150_link_setup(struct media_entity *entity,
 	/* 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].is_svideo;
+			is_svideo =
+			   decoder->connectors[i].base.type == V4L2_CON_SVIDEO;
 			break;
 		}
 	}
@@ -1555,8 +1555,9 @@ static int tvp5150_registered(struct v4l2_subdev *sd)
 	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].port_num;
-		bool is_svideo = decoder->connectors[i].is_svideo;
+		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;
@@ -1821,8 +1822,6 @@ static int tvp5150_init(struct i2c_client *c)
 static int tvp5150_add_of_connectors(struct tvp5150 *decoder)
 {
 	struct device *dev = decoder->sd.dev;
-	struct device_node *rp;
-	struct of_endpoint ep;
 	struct tvp5150_connector *connectors;
 	unsigned int connectors_num = decoder->connectors_num;
 	int i, ret;
@@ -1834,22 +1833,15 @@ static int tvp5150_add_of_connectors(struct tvp5150 *decoder)
 		return -ENOMEM;
 
 	for (i = 0; i < connectors_num; i++) {
-		rp = of_graph_get_remote_port_parent(decoder->endpoints[i]);
-		of_graph_parse_endpoint(decoder->endpoints[i], &ep);
-		connectors[i].port_num = ep.port;
-		connectors[i].is_svideo = !!of_device_is_compatible(rp,
-							    "svideo-connector");
-
-		if (connectors[i].is_svideo)
-			connectors[i].ent.function = MEDIA_ENT_F_CONN_SVIDEO;
-		else
-			connectors[i].ent.function = MEDIA_ENT_F_CONN_COMPOSITE;
+		struct v4l2_fwnode_connector *c = &connectors[i].base;
+
+		ret = v4l2_fwnode_parse_connector(
+				   of_fwnode_handle(decoder->endpoints[i]), c);
 
 		connectors[i].ent.flags = MEDIA_ENT_FL_CONNECTOR;
-		ret = of_property_read_string(rp, "label",
-					      &connectors[i].ent.name);
-		if (ret < 0)
-			return ret;
+		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;
@@ -1890,41 +1882,11 @@ static int tvp5150_mc_init(struct v4l2_subdev *sd)
 	return ret;
 }
 
-static bool tvp5150_of_valid_input(struct device_node *endpoint,
-				unsigned int port, unsigned int id)
-{
-	struct device_node *rp = of_graph_get_remote_port_parent(endpoint);
-	const char *input;
-	int ret;
-
-	/* perform some basic checks needed for later mc_init */
-	switch (port) {
-	case TVP5150_PAD_AIP1A:
-		/* svideo must be connected to endpoint@1  */
-		ret = id ? of_device_is_compatible(rp, "svideo-connector") :
-			   of_device_is_compatible(rp,
-						   "composite-video-connector");
-		if (!ret)
-			return false;
-		break;
-	case TVP5150_PAD_AIP1B:
-		ret = of_device_is_compatible(rp, "composite-video-connector");
-		if (!ret)
-			return false;
-		break;
-	}
-
-	ret = of_property_read_string(rp, "label", &input);
-	if (ret < 0)
-		return false;
-
-	return true;
-}
-
 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 = 0 };
+	struct v4l2_fwnode_connector c;
 	struct device_node *ep_np;
 	unsigned int flags;
 	int ret, i = 0, in = 0;
@@ -1953,10 +1915,13 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 			/* fall through */
 		case TVP5150_PAD_AIP1A:
 		case TVP5150_PAD_AIP1B:
-			if (!tvp5150_of_valid_input(ep_np, ep.port, ep.id)) {
+			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 %pOF on port %d\n",
-					ep.local_node, ep.port);
+					"Invalid endpoint %d on port %d\n",
+					c.remote_id, c.remote_port);
 				ret = -EINVAL;
 				goto err;
 			}
-- 
2.20.1

[PATCH 5/5] 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>
---
 drivers/media/i2c/tvp5150.c | 42 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index f3a2ad00a40d..7619793dee67 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");
@@ -74,6 +81,7 @@ struct tvp5150 {
 	struct media_pad pads[TVP5150_NUM_PADS];
 	int pads_state[TVP5150_NUM_PADS];
 	struct tvp5150_connector *connectors;
+	struct tvp5150_connector *cur_connector;
 	int connectors_num;
 	bool modify_second_link;
 #endif
@@ -794,17 +802,27 @@ 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);
+	v4l2_std_id supported_norms =
+		decoder->cur_connector->base.connector.analog.supported_tvnorms;
 
 	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);
 }
@@ -1298,6 +1316,7 @@ static int tvp5150_link_setup(struct media_entity *entity,
 	int *pad_state = &decoder->pads_state[0];
 	int i, active_pad, ret = 0;
 	bool is_svideo = false;
+	bool update_cur_connector = false;
 
 	/*
 	 * The tvp state is determined by the enabled sink pad link.
@@ -1344,10 +1363,12 @@ static int tvp5150_link_setup(struct media_entity *entity,
 				decoder->modify_second_link = false;
 				tvp5150_s_routing(sd, TVP5150_SVIDEO,
 						  TVP5150_NORMAL, 0);
+				update_cur_connector = true;
 			}
 		} else {
 			tvp5150_s_routing(sd, tvp5150_pad->index,
 					  TVP5150_NORMAL, 0);
+			update_cur_connector = true;
 		}
 	} else {
 		/*
@@ -1376,6 +1397,14 @@ static int tvp5150_link_setup(struct media_entity *entity,
 					  active_pad, TVP5150_BLACK_SCREEN, 0);
 		decoder->modify_second_link = false;
 	}
+
+	if (update_cur_connector) {
+		/* Update tvnorm according to connector */
+		decoder->cur_connector =
+			container_of(remote, struct tvp5150_connector, pad);
+		tvp5150_s_std(sd,
+			decoder->cur_connector->base.connector.analog.supported_tvnorms);
+	}
 out:
 	return ret;
 }
@@ -1605,6 +1634,9 @@ static int tvp5150_registered(struct v4l2_subdev *sd)
 			}
 			tvp5150_selmux(sd);
 			decoder->modify_second_link = false;
+			decoder->cur_connector = &decoder->connectors[i];
+			tvp5150_s_std(sd,
+				decoder->connectors[i].base.connector.analog.supported_tvnorms);
 		}
 	}
 #endif
@@ -1925,6 +1957,14 @@ 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_err(dev,
+					"Invalid tv norm(s) on connector %s.\n",
+					c.label);
+				ret = -EINVAL;
+				goto err;
+			}
 			in++;
 			break;
 		case TVP5150_PAD_VID_OUT:
-- 
2.20.1

Re: [PATCH 5/5] media: tvp5150: add support to limit tv norms on connector

[Mauro Carvalho Chehab]

Em Sat,  2 Feb 2019 13:10:04 +0100
Marco Felsch <m.felsch@pengutronix.de> escreveu:

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

Hmm... I'm afraid of this change. As far as I remember, I tested some
weird format variants like V4L2_STD_PAL_60 a long time ago, but there's
no way to force video to use those. The format selection logic simply
places the device on auto-detect mode for those weirdos, and that
works fine at the devices I know.

A change like that may break things. So I would actually have a quirk
to optionally disable auto-detection on devices that this is not know
to work.

> 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>
> ---
>  drivers/media/i2c/tvp5150.c | 42 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index f3a2ad00a40d..7619793dee67 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");
> @@ -74,6 +81,7 @@ struct tvp5150 {
>  	struct media_pad pads[TVP5150_NUM_PADS];
>  	int pads_state[TVP5150_NUM_PADS];
>  	struct tvp5150_connector *connectors;
> +	struct tvp5150_connector *cur_connector;
>  	int connectors_num;
>  	bool modify_second_link;
>  #endif
> @@ -794,17 +802,27 @@ 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);
> +	v4l2_std_id supported_norms =
> +		decoder->cur_connector->base.connector.analog.supported_tvnorms;
>  
>  	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);
>  }
> @@ -1298,6 +1316,7 @@ static int tvp5150_link_setup(struct media_entity *entity,
>  	int *pad_state = &decoder->pads_state[0];
>  	int i, active_pad, ret = 0;
>  	bool is_svideo = false;
> +	bool update_cur_connector = false;
>  
>  	/*
>  	 * The tvp state is determined by the enabled sink pad link.
> @@ -1344,10 +1363,12 @@ static int tvp5150_link_setup(struct media_entity *entity,
>  				decoder->modify_second_link = false;
>  				tvp5150_s_routing(sd, TVP5150_SVIDEO,
>  						  TVP5150_NORMAL, 0);
> +				update_cur_connector = true;
>  			}
>  		} else {
>  			tvp5150_s_routing(sd, tvp5150_pad->index,
>  					  TVP5150_NORMAL, 0);
> +			update_cur_connector = true;
>  		}
>  	} else {
>  		/*
> @@ -1376,6 +1397,14 @@ static int tvp5150_link_setup(struct media_entity *entity,
>  					  active_pad, TVP5150_BLACK_SCREEN, 0);
>  		decoder->modify_second_link = false;
>  	}
> +
> +	if (update_cur_connector) {
> +		/* Update tvnorm according to connector */
> +		decoder->cur_connector =
> +			container_of(remote, struct tvp5150_connector, pad);
> +		tvp5150_s_std(sd,
> +			decoder->cur_connector->base.connector.analog.supported_tvnorms);
> +	}
>  out:
>  	return ret;
>  }
> @@ -1605,6 +1634,9 @@ static int tvp5150_registered(struct v4l2_subdev *sd)
>  			}
>  			tvp5150_selmux(sd);
>  			decoder->modify_second_link = false;
> +			decoder->cur_connector = &decoder->connectors[i];
> +			tvp5150_s_std(sd,
> +				decoder->connectors[i].base.connector.analog.supported_tvnorms);
>  		}
>  	}
>  #endif
> @@ -1925,6 +1957,14 @@ 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_err(dev,
> +					"Invalid tv norm(s) on connector %s.\n",
> +					c.label);
> +				ret = -EINVAL;
> +				goto err;
> +			}
>  			in++;
>  			break;
>  		case TVP5150_PAD_VID_OUT:



Thanks,
Mauro

Re: [PATCH 5/5] media: tvp5150: add support to limit tv norms on connector

[Marco Felsch]

Hi Mauro,

On 19-03-20 11:18, Mauro Carvalho Chehab wrote:
> Em Sat,  2 Feb 2019 13:10:04 +0100
> Marco Felsch <m.felsch@pengutronix.de> escreveu:
> 
> > 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. 
> 
> Hmm... I'm afraid of this change. As far as I remember, I tested some
> weird format variants like V4L2_STD_PAL_60 a long time ago, but there's
> no way to force video to use those. The format selection logic simply
> places the device on auto-detect mode for those weirdos, and that
> works fine at the devices I know.

Sorry I didn't get this. The format is set to autodetect during probe().
If there is no format limitation this won't be changed during
media.link_setup(). You're right I forgot to check if the cur_connector
is available during tvp5150_s_std(), in case of pdata related devices.
In such a case we should set supported_norms to V4L2_STD_ALL as it is
done by v4l2_fwnode_parse_connector() if no limitations are given.

Btw, how does it look with the other patchset?

Regards,
Marco

> 
> A change like that may break things. So I would actually have a quirk
> to optionally disable auto-detection on devices that this is not know
> to work.
> 
> > 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>
> > ---
> >  drivers/media/i2c/tvp5150.c | 42 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > index f3a2ad00a40d..7619793dee67 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");
> > @@ -74,6 +81,7 @@ struct tvp5150 {
> >  	struct media_pad pads[TVP5150_NUM_PADS];
> >  	int pads_state[TVP5150_NUM_PADS];
> >  	struct tvp5150_connector *connectors;
> > +	struct tvp5150_connector *cur_connector;
> >  	int connectors_num;
> >  	bool modify_second_link;
> >  #endif
> > @@ -794,17 +802,27 @@ 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);
> > +	v4l2_std_id supported_norms =
> > +		decoder->cur_connector->base.connector.analog.supported_tvnorms;
> >  
> >  	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);
> >  }
> > @@ -1298,6 +1316,7 @@ static int tvp5150_link_setup(struct media_entity *entity,
> >  	int *pad_state = &decoder->pads_state[0];
> >  	int i, active_pad, ret = 0;
> >  	bool is_svideo = false;
> > +	bool update_cur_connector = false;
> >  
> >  	/*
> >  	 * The tvp state is determined by the enabled sink pad link.
> > @@ -1344,10 +1363,12 @@ static int tvp5150_link_setup(struct media_entity *entity,
> >  				decoder->modify_second_link = false;
> >  				tvp5150_s_routing(sd, TVP5150_SVIDEO,
> >  						  TVP5150_NORMAL, 0);
> > +				update_cur_connector = true;
> >  			}
> >  		} else {
> >  			tvp5150_s_routing(sd, tvp5150_pad->index,
> >  					  TVP5150_NORMAL, 0);
> > +			update_cur_connector = true;
> >  		}
> >  	} else {
> >  		/*
> > @@ -1376,6 +1397,14 @@ static int tvp5150_link_setup(struct media_entity *entity,
> >  					  active_pad, TVP5150_BLACK_SCREEN, 0);
> >  		decoder->modify_second_link = false;
> >  	}
> > +
> > +	if (update_cur_connector) {
> > +		/* Update tvnorm according to connector */
> > +		decoder->cur_connector =
> > +			container_of(remote, struct tvp5150_connector, pad);
> > +		tvp5150_s_std(sd,
> > +			decoder->cur_connector->base.connector.analog.supported_tvnorms);
> > +	}
> >  out:
> >  	return ret;
> >  }
> > @@ -1605,6 +1634,9 @@ static int tvp5150_registered(struct v4l2_subdev *sd)
> >  			}
> >  			tvp5150_selmux(sd);
> >  			decoder->modify_second_link = false;
> > +			decoder->cur_connector = &decoder->connectors[i];
> > +			tvp5150_s_std(sd,
> > +				decoder->connectors[i].base.connector.analog.supported_tvnorms);
> >  		}
> >  	}
> >  #endif
> > @@ -1925,6 +1957,14 @@ 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_err(dev,
> > +					"Invalid tv norm(s) on connector %s.\n",
> > +					c.label);
> > +				ret = -EINVAL;
> > +				goto err;
> > +			}
> >  			in++;
> >  			break;
> >  		case TVP5150_PAD_VID_OUT:
> 
> 
> 
> 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 |

Re: [PATCH 5/5] media: tvp5150: add support to limit tv norms on connector

[Mauro Carvalho Chehab]

Em Wed, 20 Mar 2019 17:36:50 +0100
Marco Felsch <m.felsch@pengutronix.de> escreveu:

> Hi Mauro,
> 
> On 19-03-20 11:18, Mauro Carvalho Chehab wrote:
> > Em Sat,  2 Feb 2019 13:10:04 +0100
> > Marco Felsch <m.felsch@pengutronix.de> escreveu:
> >   
> > > 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.   
> > 
> > Hmm... I'm afraid of this change. As far as I remember, I tested some
> > weird format variants like V4L2_STD_PAL_60 a long time ago, but there's
> > no way to force video to use those. The format selection logic simply
> > places the device on auto-detect mode for those weirdos, and that
> > works fine at the devices I know.  
> 
> Sorry I didn't get this. The format is set to autodetect during probe().

Yes, but apps will change based on G_FMT, TRY_FMT and S_FMT.

See, my main concern here is with existing tvp5150 non-platform
drivers, as a change there would be a regression.

> If there is no format limitation this won't be changed during
> media.link_setup(). You're right I forgot to check if the cur_connector
> is available during tvp5150_s_std(), in case of pdata related devices.

Yeah, that's what I'm talking about.

> In such a case we should set supported_norms to V4L2_STD_ALL as it is
> done by v4l2_fwnode_parse_connector() if no limitations are given.
> 
> Btw, how does it look with the other patchset?

I asked Hans to take a look at the patch series, as he's sub-maintaining
the v4l2 stuff. 

I'm intending to take
a deeper look at patch 2/7 v4 from the past series.

> 
> Regards,
> Marco
> 
> > 
> > A change like that may break things. So I would actually have a quirk
> > to optionally disable auto-detection on devices that this is not know
> > to work.
> >   
> > > 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>
> > > ---
> > >  drivers/media/i2c/tvp5150.c | 42 ++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > > index f3a2ad00a40d..7619793dee67 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");
> > > @@ -74,6 +81,7 @@ struct tvp5150 {
> > >  	struct media_pad pads[TVP5150_NUM_PADS];
> > >  	int pads_state[TVP5150_NUM_PADS];
> > >  	struct tvp5150_connector *connectors;
> > > +	struct tvp5150_connector *cur_connector;
> > >  	int connectors_num;
> > >  	bool modify_second_link;
> > >  #endif
> > > @@ -794,17 +802,27 @@ 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);
> > > +	v4l2_std_id supported_norms =
> > > +		decoder->cur_connector->base.connector.analog.supported_tvnorms;
> > >  
> > >  	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);
> > >  }
> > > @@ -1298,6 +1316,7 @@ static int tvp5150_link_setup(struct media_entity *entity,
> > >  	int *pad_state = &decoder->pads_state[0];
> > >  	int i, active_pad, ret = 0;
> > >  	bool is_svideo = false;
> > > +	bool update_cur_connector = false;
> > >  
> > >  	/*
> > >  	 * The tvp state is determined by the enabled sink pad link.
> > > @@ -1344,10 +1363,12 @@ static int tvp5150_link_setup(struct media_entity *entity,
> > >  				decoder->modify_second_link = false;
> > >  				tvp5150_s_routing(sd, TVP5150_SVIDEO,
> > >  						  TVP5150_NORMAL, 0);
> > > +				update_cur_connector = true;
> > >  			}
> > >  		} else {
> > >  			tvp5150_s_routing(sd, tvp5150_pad->index,
> > >  					  TVP5150_NORMAL, 0);
> > > +			update_cur_connector = true;
> > >  		}
> > >  	} else {
> > >  		/*
> > > @@ -1376,6 +1397,14 @@ static int tvp5150_link_setup(struct media_entity *entity,
> > >  					  active_pad, TVP5150_BLACK_SCREEN, 0);
> > >  		decoder->modify_second_link = false;
> > >  	}
> > > +
> > > +	if (update_cur_connector) {
> > > +		/* Update tvnorm according to connector */
> > > +		decoder->cur_connector =
> > > +			container_of(remote, struct tvp5150_connector, pad);
> > > +		tvp5150_s_std(sd,
> > > +			decoder->cur_connector->base.connector.analog.supported_tvnorms);
> > > +	}
> > >  out:
> > >  	return ret;
> > >  }
> > > @@ -1605,6 +1634,9 @@ static int tvp5150_registered(struct v4l2_subdev *sd)
> > >  			}
> > >  			tvp5150_selmux(sd);
> > >  			decoder->modify_second_link = false;
> > > +			decoder->cur_connector = &decoder->connectors[i];
> > > +			tvp5150_s_std(sd,
> > > +				decoder->connectors[i].base.connector.analog.supported_tvnorms);
> > >  		}
> > >  	}
> > >  #endif
> > > @@ -1925,6 +1957,14 @@ 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_err(dev,
> > > +					"Invalid tv norm(s) on connector %s.\n",
> > > +					c.label);
> > > +				ret = -EINVAL;
> > > +				goto err;
> > > +			}
> > >  			in++;
> > >  			break;
> > >  		case TVP5150_PAD_VID_OUT:  
> > 
> > 
> > 
> > Thanks,
> > Mauro
> >   
> 



Thanks,
Mauro

Re: [PATCH 5/5] media: tvp5150: add support to limit tv norms on connector

[Marco Felsch]

Hi Mauro,

On 19-03-20 14:29, Mauro Carvalho Chehab wrote:
> Em Wed, 20 Mar 2019 17:36:50 +0100
> Marco Felsch <m.felsch@pengutronix.de> escreveu:
> 
> > Hi Mauro,
> > 
> > On 19-03-20 11:18, Mauro Carvalho Chehab wrote:
> > > Em Sat,  2 Feb 2019 13:10:04 +0100
> > > Marco Felsch <m.felsch@pengutronix.de> escreveu:
> > >   
> > > > 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.   
> > > 
> > > Hmm... I'm afraid of this change. As far as I remember, I tested some
> > > weird format variants like V4L2_STD_PAL_60 a long time ago, but there's
> > > no way to force video to use those. The format selection logic simply
> > > places the device on auto-detect mode for those weirdos, and that
> > > works fine at the devices I know.  
> > 
> > Sorry I didn't get this. The format is set to autodetect during probe().
> 
> Yes, but apps will change based on G_FMT, TRY_FMT and S_FMT.
> 
> See, my main concern here is with existing tvp5150 non-platform
> drivers, as a change there would be a regression.

Of course I won't break existing stuff too :)

> > If there is no format limitation this won't be changed during
> > media.link_setup(). You're right I forgot to check if the cur_connector
> > is available during tvp5150_s_std(), in case of pdata related devices.
> 
> Yeah, that's what I'm talking about.

So are you with me with the following lines? This should avoid breaking
exisiting drivers.

 static int tvp5150_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
 {
 	struct tvp5150 *decoder = to_tvp5150(sd);
+	v4l2_std_id supported_norms = decoder->cur_connector ?
+		decoder->cur_connector->base.connector.analog.supported_tvnorms :
+		V4L2_STD_ALL;

 ...

 }

> > In such a case we should set supported_norms to V4L2_STD_ALL as it is
> > done by v4l2_fwnode_parse_connector() if no limitations are given.
> > 
> > Btw, how does it look with the other patchset?
> 
> I asked Hans to take a look at the patch series, as he's sub-maintaining
> the v4l2 stuff.

Cheers.

> I'm intending to take
> a deeper look at patch 2/7 v4 from the past series.

That would be great. I'm looking forward for your feedback :)

Regards,
Marco

> > 
> > Regards,
> > Marco
> > 
> > > 
> > > A change like that may break things. So I would actually have a quirk
> > > to optionally disable auto-detection on devices that this is not know
> > > to work.
> > >   
> > > > 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>
> > > > ---
> > > >  drivers/media/i2c/tvp5150.c | 42 ++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > > > index f3a2ad00a40d..7619793dee67 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");
> > > > @@ -74,6 +81,7 @@ struct tvp5150 {
> > > >  	struct media_pad pads[TVP5150_NUM_PADS];
> > > >  	int pads_state[TVP5150_NUM_PADS];
> > > >  	struct tvp5150_connector *connectors;
> > > > +	struct tvp5150_connector *cur_connector;
> > > >  	int connectors_num;
> > > >  	bool modify_second_link;
> > > >  #endif
> > > > @@ -794,17 +802,27 @@ 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);
> > > > +	v4l2_std_id supported_norms =
> > > > +		decoder->cur_connector->base.connector.analog.supported_tvnorms;
> > > >  
> > > >  	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);
> > > >  }
> > > > @@ -1298,6 +1316,7 @@ static int tvp5150_link_setup(struct media_entity *entity,
> > > >  	int *pad_state = &decoder->pads_state[0];
> > > >  	int i, active_pad, ret = 0;
> > > >  	bool is_svideo = false;
> > > > +	bool update_cur_connector = false;
> > > >  
> > > >  	/*
> > > >  	 * The tvp state is determined by the enabled sink pad link.
> > > > @@ -1344,10 +1363,12 @@ static int tvp5150_link_setup(struct media_entity *entity,
> > > >  				decoder->modify_second_link = false;
> > > >  				tvp5150_s_routing(sd, TVP5150_SVIDEO,
> > > >  						  TVP5150_NORMAL, 0);
> > > > +				update_cur_connector = true;
> > > >  			}
> > > >  		} else {
> > > >  			tvp5150_s_routing(sd, tvp5150_pad->index,
> > > >  					  TVP5150_NORMAL, 0);
> > > > +			update_cur_connector = true;
> > > >  		}
> > > >  	} else {
> > > >  		/*
> > > > @@ -1376,6 +1397,14 @@ static int tvp5150_link_setup(struct media_entity *entity,
> > > >  					  active_pad, TVP5150_BLACK_SCREEN, 0);
> > > >  		decoder->modify_second_link = false;
> > > >  	}
> > > > +
> > > > +	if (update_cur_connector) {
> > > > +		/* Update tvnorm according to connector */
> > > > +		decoder->cur_connector =
> > > > +			container_of(remote, struct tvp5150_connector, pad);
> > > > +		tvp5150_s_std(sd,
> > > > +			decoder->cur_connector->base.connector.analog.supported_tvnorms);
> > > > +	}
> > > >  out:
> > > >  	return ret;
> > > >  }
> > > > @@ -1605,6 +1634,9 @@ static int tvp5150_registered(struct v4l2_subdev *sd)
> > > >  			}
> > > >  			tvp5150_selmux(sd);
> > > >  			decoder->modify_second_link = false;
> > > > +			decoder->cur_connector = &decoder->connectors[i];
> > > > +			tvp5150_s_std(sd,
> > > > +				decoder->connectors[i].base.connector.analog.supported_tvnorms);
> > > >  		}
> > > >  	}
> > > >  #endif
> > > > @@ -1925,6 +1957,14 @@ 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_err(dev,
> > > > +					"Invalid tv norm(s) on connector %s.\n",
> > > > +					c.label);
> > > > +				ret = -EINVAL;
> > > > +				goto err;
> > > > +			}
> > > >  			in++;
> > > >  			break;
> > > >  		case TVP5150_PAD_VID_OUT:  
> > > 
> > > 
> > > 
> > > Thanks,
> > > Mauro
> > >   
> > 
> 
> 
> 
> 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 |