[PATCH v7 00/13] Add TVP5150 features

[PATCH v7 00/13] Add TVP5150 features

[Marco Felsch]

Hi,

many thanks to Mauro, Hans, Jacopo and Laurent for the
feedack :)
Hopefully I got all the comments and concerns and fixed those
with this v7.

In short:
 - v4l2-connector label isn't set anymore if not specified
 - tvp5150 connectors are now static
   -> reduces code and is easier to review and maintain

Each changed patch contains a more detailed changelog.

Hopefully we get this series done :)

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 | 145 +++-
 drivers/media/i2c/tvp5150.c                   | 644 +++++++++++++-----
 drivers/media/v4l2-core/v4l2-fwnode.c         | 137 ++++
 include/dt-bindings/media/tvnorms.h           |  72 ++
 include/dt-bindings/media/tvp5150.h           |   2 -
 include/media/v4l2-fwnode.h                   |  83 +++
 7 files changed, 895 insertions(+), 192 deletions(-)
 create mode 100644 include/dt-bindings/media/tvnorms.h

-- 
2.20.1

[PATCH v7 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>
---
[1] https://patchwork.kernel.org/cover/10794703/

v7:
I kept Robs r b tag because I only changed the example and extended
TVNORM_* macros.

- fix some style issues
- add TVNORM_NTSC, TVNORM_525_60 and TVNORM_625_50

v6:
- tvnorms.h: use tabs instead of spaces
- tvnorms.h: add TVNORM_PAL and TVNORM_SECAM
- tvnorms.h: drop rarely used TVNORM_ATSC_* norms

v2-v4:
- nothing since the patch was squashed from series [1] into this
  series.
---
 .../display/connector/analog-tv-connector.txt |  4 ++
 include/dt-bindings/media/tvnorms.h           | 72 +++++++++++++++++++
 2 files changed, 76 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..434e8aa0398b 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 | TVNORM_NTSC)>;
 
 	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..e1275673c4d6
--- /dev/null
+++ b/include/dt-bindings/media/tvnorms.h
@@ -0,0 +1,72 @@
+/* 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 standard */
+#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		(TVNORM_PAL_B		| \
+				 TVNORM_PAL_B1		| \
+				 TVNORM_PAL_G		| \
+				 TVNORM_PAL_H		| \
+				 TVNORM_PAL_I		| \
+				 TVNORM_PAL_D		| \
+				 TVNORM_PAL_D1		| \
+				 TVNORM_PAL_K)
+
+#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_NTSC		(TVNORM_NTSC_M		|\
+				 TVNORM_NTSC_M_JP	|\
+				 TVNORM_NTSC_M_KR)
+
+#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
+
+#define TVNORM_SECAM		(TVNORM_SECAM_B		| \
+				 TVNORM_SECAM_D		| \
+				 TVNORM_SECAM_G		| \
+				 TVNORM_SECAM_H		| \
+				 TVNORM_SECAM_K		| \
+				 TVNORM_SECAM_K1	| \
+				 TVNORM_SECAM_L		| \
+				 TVNORM_SECAM_LC)
+
+/* Standards for Countries with 60Hz Line frequency */
+#define TVNORM_525_60		(TVNORM_PAL_M		| \
+				 TVNORM_PAL_60		| \
+				 TVNORM_NTSC		| \
+				 TVNORM_NTSC_443)
+
+/* Standards for Countries with 50Hz Line frequency */
+#define TVNORM_625_50		(TVNORM_PAL		|\
+				 TVNORM_PAL_N		|\
+				 TVNORM_PAL_Nc		|\
+				 TVNORM_SECAM)
+
+#endif /* _DT_BINDINGS_MEDIA_TVNORMS_H */
-- 
2.20.1

[PATCH v7 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. A generic connector has
common members and connector specific members. The common members are:
  - type
  - label (optional)
  - links
  - nr_of_links

The specific members are stored 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>
---
[1] https://patchwork.kernel.org/cover/10794703/

v7:
- fix spelling issues
- constify label
- support variable label size
- replace single remote_port/id members by links member of variable
  size
- squash v4l2-connector into v4l2-fwnode

@Jacopo: I dropped your r b tag because I changed the port/id logic.

v6:
- fix some spelling and style issues
- rm unnecessary comments
- drop vga and dvi connector
- fix misspelt connector

v2-v4:
- nothing since the patch was squashed from series [1] into this
  series.
---
 include/media/v4l2-fwnode.h | 45 +++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index f6a7bcd13197..e39c198882fc 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -123,6 +123,51 @@ struct v4l2_fwnode_link {
 	unsigned int remote_port;
 };
 
+/**
+ * enum v4l2_connector_type - connector type
+ * @V4L2_CON_UNKNOWN:   unknown connector type, no V4L2 connector configuration
+ * @V4L2_CON_COMPOSITE: analog composite connector
+ * @V4L2_CON_SVIDEO:    analog svideo connector
+ * @V4L2_CON_HDMI:      digital hdmi connector
+ */
+enum v4l2_connector_type {
+	V4L2_CON_UNKNOWN,
+	V4L2_CON_COMPOSITE,
+	V4L2_CON_SVIDEO,
+	V4L2_CON_HDMI,
+};
+
+/**
+ * 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
+ * @label: optional connector label
+ * @type: connector type
+ * @links: list of &struct v4l2_fwnode_link links the connector is connected to
+ * @nr_of_links: total number of links
+ * @connector: connector configuration
+ * @connector.analog: analog connector configuration
+ *                    &struct v4l2_fwnode_connector_analog
+ */
+struct v4l2_fwnode_connector {
+	const char *label;
+	enum v4l2_connector_type type;
+	struct v4l2_fwnode_link *links;
+	unsigned int nr_of_links;
+
+	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 v7 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/

v7:
@Jacopo: I dropped your r b tag becuase of the amount of changes I
made..

- drop unnecessary comments
- fix commet style
- s/v4l2_fwnode_connector_conv.name/v4l2_fwnode_connector_conv.compatible/
- make label size variable and drop V4L2_CONNECTOR_MAX_LABEL usage
- do not assign a default label in case of no label was specified
- remove useless /* fall through */ comments
- add support for N connector links
- rename local variables to be more meaningful
- adjust kernedoc
- add v4l2_fwnode_connector_free()
- improve error handling (use different error values)
- make use of pr_warn_once()

v6:
- use unsigned count var
- fix comment and style issues
- place '/* fall through */' to correct places
- fix error handling and cleanup by releasing fwnode
- drop vga and dvi parsing support as those connectors are rarely used
  these days

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 | 137 ++++++++++++++++++++++++++
 include/media/v4l2-fwnode.h           |  38 +++++++
 2 files changed, 175 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 3bd1888787eb..c6eb8a8105cc 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -595,6 +595,143 @@ 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 *compatible;
+} connectors[] = {
+	{
+		.type = V4L2_CON_COMPOSITE,
+		.compatible = "composite-video-connector",
+	}, {
+		.type = V4L2_CON_SVIDEO,
+		.compatible = "svideo-connector",
+	}, {
+		.type = V4L2_CON_HDMI,
+		.compatible = "hdmi-connector",
+	},
+};
+
+static enum v4l2_connector_type
+v4l2_fwnode_string_to_connector_type(const char *con_str)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(connectors); i++)
+		if (!strcmp(con_str, connectors[i].compatible))
+			return connectors[i].type;
+
+	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);
+
+	/* The tvnorms property is optional. */
+	vc->connector.analog.supported_tvnorms = ret ? V4L2_STD_ALL : tvnorms;
+
+	return 0;
+}
+
+void v4l2_fwnode_connector_free(struct v4l2_fwnode_connector *connector)
+{
+	unsigned int i;
+
+	if (IS_ERR_OR_NULL(connector))
+		return;
+
+	for (i = 0; i < connector->nr_of_links; i++)
+		v4l2_fwnode_put_link(&connector->links[i]);
+	kfree(connector->links);
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_free);
+
+int v4l2_fwnode_connector_alloc_parse(struct fwnode_handle *fwnode,
+				      struct v4l2_fwnode_connector *connector)
+{
+	struct fwnode_handle *remote_pp, *remote_ep;
+	const char *type_name;
+	unsigned int i = 0, ep_num = 0;
+	int err;
+
+	memset(connector, 0, sizeof(*connector));
+
+	remote_pp = fwnode_graph_get_remote_port_parent(fwnode);
+	if (!remote_pp)
+		return -ENOLINK;
+
+	/* Parse all common properties first. */
+	fwnode_graph_for_each_endpoint(remote_pp, remote_ep)
+		ep_num++;
+
+	connector->nr_of_links = ep_num;
+	connector->links = kmalloc_array(ep_num, sizeof(*connector->links),
+					 GFP_KERNEL);
+	if (!connector->links) {
+		err = -ENOMEM;
+		goto err_put_fwnode;
+	}
+
+	fwnode_graph_for_each_endpoint(remote_pp, remote_ep) {
+		err = v4l2_fwnode_parse_link(remote_ep, &connector->links[i]);
+		if (err) {
+			fwnode_handle_put(remote_ep);
+			goto err_free_links;
+		}
+		i++;
+	}
+
+	/*
+	 * Links reference counting guarantees access -> no duplication needed
+	 */
+	fwnode_property_read_string(remote_pp, "label", &connector->label);
+
+	/* The connector-type is stored within the compatible string. */
+	err = fwnode_property_read_string(remote_pp, "compatible", &type_name);
+	if (err) {
+		err = -EINVAL;
+		goto err_free_links;
+	}
+	connector->type = v4l2_fwnode_string_to_connector_type(type_name);
+
+	/* Now parse the connector specific properties. */
+	switch (connector->type) {
+	case V4L2_CON_COMPOSITE:
+	case V4L2_CON_SVIDEO:
+		err = v4l2_fwnode_connector_parse_analog(remote_pp, connector);
+		break;
+	case V4L2_CON_HDMI:
+		pr_warn_once("Connector specific parsing is currently not supported for %s\n",
+			     type_name);
+		err = 0;
+		break;
+	case V4L2_CON_UNKNOWN:
+	default:
+		pr_err("Unknown connector type\n");
+		err = -EINVAL;
+		goto err_free_links;
+	};
+
+	fwnode_handle_put(remote_pp);
+
+	return err;
+
+err_free_links:
+	for (i = 0; i < ep_num; i++)
+		v4l2_fwnode_put_link(&connector->links[i]);
+	kfree(connector->links);
+err_put_fwnode:
+	fwnode_handle_put(remote_pp);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_alloc_parse);
+
 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 e39c198882fc..235c7214be3c 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -278,6 +278,44 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
  */
 void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
 
+/**
+ * v4l2_fwnode_connector_free() - free the V4L2 connector acquired by
+ * v4l2_fwnode_parse_connector()
+ * @connector: the V4L2 connector the resources of which are to be released
+ *
+ * Drop references to the connection link partners and free the memory allocated
+ * by v4l2_fwnode_parse_connector() call.
+ *
+ * It is safe to call this function with NULL argument or on a V4L2 connector
+ * the parsing of which failed.
+ */
+void v4l2_fwnode_connector_free(struct v4l2_fwnode_connector *connector);
+
+/**
+ * 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, all found
+ * links between the host and the connector as well as connector specific data.
+ * Since the label is optional it can be %NULL if no one was found.
+ *
+ * A reference is taken to both the connector and the connector destination
+ * where the connector belongs to. This must be dropped when no longer needed.
+ * Also the memory it has allocated to store the variable size data must be
+ * freed. Both dropping the references and freeing the memory is done by
+ * v4l2_fwnode_connector_free().
+ *
+ * Return:
+ * * %0 on success or a negative error code on failure:
+ * * %-ENOMEM if memory allocation failed
+ * * %-ENOLINK if the connector can't be found
+ * * %-EINVAL on parsing failure
+ */
+int v4l2_fwnode_connector_alloc_parse(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 v7 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         | 145 ----------------------------
 include/dt-bindings/media/tvp5150.h |   2 -
 2 files changed, 147 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index edad49cebcdf..11a5fd7e2f58 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,67 +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);
-			of_node_put(child);
-			goto err_connector;
-		}
-
-		if (input_type >= TVP5150_INPUT_NUM) {
-			ret = -EINVAL;
-			of_node_put(child);
-			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);
-			of_node_put(child);
-			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);
-			of_node_put(child);
-			goto err_connector;
-		}
-
-		input->name = name;
-	}
-
-err_connector:
-	of_node_put(connectors);
-#endif
 err:
 	of_node_put(ep);
 	return ret;
@@ -1735,7 +1592,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)
@@ -1750,7 +1606,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 01eedf4985b8..dda00c038530 100644
--- a/include/dt-bindings/media/tvp5150.h
+++ b/include/dt-bindings/media/tvp5150.h
@@ -14,8 +14,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 v7 05/13] media: tvp5150: add input source selection of_graph support

[Marco Felsch]

This patch adds the of_graph support to describe the tvp input 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. Look at the
Documentation for more information. Since the TVP5150 is a converter/bridge
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/

v7:
- don't init enum tvp5150_pads with TVP5150_COMPOSITE0 functionality
- break some 80 character limitation to improve readability
- fix comment style -> always start with capital letters and end with dot
- fix some minor style issues
- fix tvp5150_registered error handling
- simplify tvp5150_mc_init
- make connectors static
  -> since now only three connectors are possible (as described in DT)
  -> drop tvp5150.endpoints storage
  -> squash tvp5150_parse_dt and tvp5150_add_of_connectors
- improve tvp5150_parse_dt:
  -> make parsing stricter and fix not detected missconfigured dt-data
  -> svideo must be connected now to port@[0,1]/endpoint@1

v6:
- fix misspelled comments
- use 'unsigned int' where it's possible
- cleanup ifdef part-2:
  * tvp5150_mc_init, tvp5150_add_of_connectors: add surrounding
    CONFIG_MEDIA_CONTROLLER if def and stubs to improve quality
- tvp5150_mc_init: uniform interface, use 'struct tvp5150' since all
  internal function do this.
- tvp5150_add_of_connectors: call within probe() to make it cleaner
- tvp5150_parse_dt: move local loop vars within the loop.

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 | 379 +++++++++++++++++++++++++++++++++---
 1 file changed, 350 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 11a5fd7e2f58..dfbf5bbc307c 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -32,11 +32,12 @@
 #define TVP5150_FIELD		V4L2_FIELD_ALTERNATE
 #define TVP5150_COLORSPACE	V4L2_COLORSPACE_SMPTE170M
 
+#define TVP5150_MAX_CONNECTORS	3 /* Check dt-bindings for more informations. */
+
 MODULE_DESCRIPTION("Texas Instruments TVP5150A/TVP5150AM1/TVP5151 video decoder driver");
 MODULE_AUTHOR("Mauro Carvalho Chehab");
 MODULE_LICENSE("GPL v2");
 
-
 static int debug;
 module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Debug level (0-2)");
@@ -44,16 +45,25 @@ 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_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
+
 	struct media_pad pads[TVP5150_NUM_PADS];
-#endif
+	struct tvp5150_connector connectors[TVP5150_MAX_CONNECTORS];
+	unsigned int connectors_num;
+
 	struct v4l2_ctrl_handler hdl;
 	struct v4l2_rect rect;
 	struct regmap *regmap;
@@ -1167,6 +1177,130 @@ 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;
+	unsigned int i;
+	int 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;
+	unsigned int i;
+	int 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 an 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 +1448,70 @@ 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.links[0].remote_port;
+		unsigned int flags = i ? 0 : MEDIA_LNK_FL_ENABLED;
+		bool is_svideo = decoder->connectors[i].base.type == V4L2_CON_SVIDEO;
+
+		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)
+			goto err;
+
+		if (is_svideo) {
+			/*
+			 * Check tvp5150_link_setup() comments for more
+			 * information.
+			 */
+			port = decoder->connectors[i].base.links[1].remote_port;
+			ret = media_create_pad_link(con, 0, &sd->entity, port,
+						    flags);
+			if (ret < 0)
+				goto err;
+		}
+
+		/* 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;
+
+err:
+	for (i = 0; i < decoder->connectors_num; i++)
+		media_device_unregister_entity(&decoder->connectors[i].ent);
+
+	return ret;
+}
+
 /* ----------------------------------------------------------------------- */
 
 static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
@@ -1367,6 +1565,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,36 +1717,164 @@ static int tvp5150_init(struct i2c_client *c)
 	return 0;
 }
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
+static int tvp5150_mc_init(struct tvp5150 *decoder)
+{
+	struct v4l2_subdev *sd = &decoder->sd;
+	unsigned int i;
+
+	sd->entity.ops = &tvp5150_sd_media_ops;
+	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
+
+	for (i = 0; i < TVP5150_NUM_PADS - 1; i++) {
+		decoder->pads[i].flags = MEDIA_PAD_FL_SINK;
+		decoder->pads[i].sig_type = PAD_SIGNAL_ANALOG;
+	}
+
+	decoder->pads[i].flags = MEDIA_PAD_FL_SOURCE;
+	decoder->pads[i].sig_type = PAD_SIGNAL_DV;
+
+	return media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS,
+				      decoder->pads);
+}
+
+#else /* !defined(CONFIG_MEDIA_CONTROLLER) */
+
+static inline int tvp5150_mc_init(struct tvp5150 *decoder)
+{
+	return 0;
+}
+#endif /* defined(CONFIG_MEDIA_CONTROLLER) */
+
+static int
+tvp5150_add_connector(struct tvp5150 *decoder, struct device_node *np,
+		      unsigned int portn, unsigned int epn, unsigned int svideo)
+{
+	struct device *dev = decoder->sd.dev;
+	struct device_node *ep_np, *remote_np;
+	struct v4l2_fwnode_connector *vc;
+	struct tvp5150_connector *c;
+	unsigned int next_connector = decoder->connectors_num;
+	int ret;
+
+	/* Get free connector entry. */
+	if (next_connector >= TVP5150_MAX_CONNECTORS)
+		return 0;
+
+	c = &decoder->connectors[next_connector];
+	vc = &c->base;
+	ep_np = of_graph_get_endpoint_by_regs(np, portn, epn);
+	remote_np = of_graph_get_remote_port_parent(ep_np);
+
+	ret = v4l2_fwnode_connector_alloc_parse(of_fwnode_handle(ep_np), vc);
+	if (ret)
+		goto err;
+
+	if ((!svideo && vc->type != V4L2_CON_COMPOSITE) ||
+	    (svideo && vc->type != V4L2_CON_SVIDEO)) {
+		dev_err(dev, "Invalid connector type for port@%u/endpoint@%u\n",
+			portn, epn);
+		ret = -EINVAL;
+		goto err;
+	}
+
+	if (svideo &&
+	    (vc->nr_of_links != 2 ||
+	     vc->links[0].remote_port == vc->links[1].remote_port)) {
+		dev_err(dev, "Invalid Svideo connector configuration\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
+	if (!(vc->connector.analog.supported_tvnorms & TVP5150_STD_MASK)) {
+		dev_err(dev, "Unsupported tv-norm on connector %s\n",
+			remote_np->name);
+		ret = -EINVAL;
+		goto err;
+	}
+
+	c->ent.flags = MEDIA_ENT_FL_CONNECTOR;
+	c->ent.name = vc->label;
+	c->ent.name = kasprintf(GFP_KERNEL, "%s %s", remote_np->name,
+				vc->label ? vc->label : "");
+	c->ent.function = vc->type == V4L2_CON_SVIDEO ?
+		MEDIA_ENT_F_CONN_SVIDEO : MEDIA_ENT_F_CONN_COMPOSITE;
+
+	decoder->connectors_num++;
+
+	of_node_put(remote_np);
+	of_node_put(ep_np);
+	return 0;
+
+err:
+	of_node_put(remote_np);
+	of_node_put(ep_np);
+	v4l2_fwnode_connector_free(vc);
+	return ret;
+}
+
 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;
+	struct device *dev = decoder->sd.dev;
+	struct device_node *ep_np;
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_UNKNOWN
+	};
 	unsigned int flags;
-	int ret = 0;
+	int ret, ep_num;
 
-	ep = of_graph_get_next_endpoint(np, NULL);
-	if (!ep)
+	/* At least 1 output and 1 input. */
+	ep_num = of_graph_get_endpoint_count(np);
+	if (ep_num < 2 || ep_num > 5) {
+		dev_err(dev, "At least 1 input and 1 output must be connected to the device.\n");
 		return -EINVAL;
+	}
+
+	/* Layout if all connectors are used
+	 *
+	 * tvp-5150 port@0 (AIP1A)
+	 *	endpoint@0 -----------> Comp0-Con  port
+	 *	endpoint@1 --------+--> Svideo-Con port
+	 * tvp-5150 port@1 (AIP1B) |
+	 *	endpoint@1 --------+
+	 *	endpoint@0 -----------> Comp1-Con  port
+	 * tvp-5150 port@2
+	 *	endpoint (video bitstream output at YOUT[0-7] parallel bus)
+	 */
+	for_each_endpoint_of_node(np, ep_np) {
+		struct of_endpoint ep;
+
+		of_graph_parse_endpoint(ep_np, &ep);
+		if (ep.port > 1)
+			continue;
+
+		ret = tvp5150_add_connector(decoder, np, ep.port, ep.id,
+					    ep.id ? 1 : 0);
+		if (ret)
+			return ret;
+	}
 
-	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &bus_cfg);
+	ep_np = of_graph_get_endpoint_by_regs(np, TVP5150_PAD_VID_OUT, 0);
+	if (!ep_np) {
+		dev_err(dev, "Error no output endpoint available\n");
+		return -EINVAL;
+	}
+	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_np), &bus_cfg);
+	of_node_put(ep_np);
 	if (ret)
-		goto err;
+		return ret;
 
 	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;
+		return -EINVAL;
 	}
 
 	decoder->mbus_type = bus_cfg.bus_type;
 
-err:
-	of_node_put(ep);
-	return ret;
+	return decoder->connectors_num ? 0 : -ENODEV;
 }
 
 static const char * const tvp5150_test_patterns[2] = {
@@ -1592,22 +1922,13 @@ 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)
+	res = tvp5150_mc_init(core);
+	if (res)
 		return res;
 
-#endif
-
 	res = tvp5150_detect_version(core);
 	if (res < 0)
 		return res;
-- 
2.20.1

[PATCH v7 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>
---
Changelog:

v7:
Hi Rob,
I droped your r b tag because I changed the bindings in this
patch version. Please can you have a look on it again?

- fix missing AIP1B svideo connection (description and examples)

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 | 112 ++++++++++++++++--
 1 file changed, 105 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
index 8c0fc1a26bf0..28b64ad149ef 100644
--- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
+++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
@@ -12,11 +12,32 @@ 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@1 ------+
+	endpoint@0 -----------> 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 +47,94 @@ 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>;
+		};
+	};
+};
+
+svideo_connector {
+	compatible = "svideo-connector";
+	label = "S-Video";
+
+	port {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		svideo_luma_to_tvp5150: endpoint@0 {
+			reg = <0>;
+			remote-endpoint = <&tvp5150_to_svideo_luma>;
+		};
+
+		svideo_chroma_to_tvp5150: endpoint@1 {
+			reg = <1>;
+			remote-endpoint = <&tvp5150_to_svideo_chroma>;
+		};
+	};
+};
 
 &i2c2 {
-	...
 	tvp5150@5c {
 		compatible = "ti,tvp5150";
 		reg = <0x5c>;
 		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
 		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		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_luma: endpoint@1 {
+				reg = <1>;
+				remote-endpoint = <&svideo_luma_to_tvp5150>;
+			};
+		};
+
+		port@1 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <1>;
+
+			tvp5150_to_composite1: endpoint@0 {
+				reg = <0>;
+                                remote-endpoint = <&composite1_to_tvp5150>;
+			};
+
+			tvp5150_to_svideo_chroma: endpoint@1 {
+				reg = <1>;
+				remote-endpoint = <&svideo_chroma_to_tvp5150>;
+			};
+		};
+
+		port@2 {
+			reg = <2>;
 
-		port {
 			tvp5150_1: endpoint {
 				remote-endpoint = <&ccdc_ep>;
 			};
-- 
2.20.1

[PATCH v7 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:

v7:
- __tvp5150_get_pad_crop(): return error on default case
- simplify __tvp5150_get_pad_crop() error handling
- tvp5150_set_selection() squash __tvp5150_set_selection() execution
  conditions
v6:
nothing
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 | 118 +++++++++++++++++++++++++-----------
 1 file changed, 84 insertions(+), 34 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index dfbf5bbc307c..ad59e65e6771 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"
 
@@ -994,20 +995,44 @@ 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 ERR_PTR(-EINVAL);
+	}
+}
+
 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(__crop))
+		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;
@@ -1018,17 +1043,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",
@@ -1037,17 +1096,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:
@@ -1060,20 +1109,19 @@ 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(__crop))
+		return PTR_ERR(__crop);
 
-	decoder->rect = rect;
+	/*
+	 * Update output image size if the selection (crop) rectangle size or
+	 * position has been modified.
+	 */
+	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
+	    !v4l2_rect_equal(&rect, __crop))
+		__tvp5150_set_selection(sd, rect);
+
+	*__crop = rect;
 
 	return 0;
 }
@@ -1083,11 +1131,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;
@@ -1105,7 +1151,11 @@ 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(__crop))
+			return PTR_ERR(__crop);
+		sel->r = *__crop;
 		return 0;
 	default:
 		return -EINVAL;
-- 
2.20.1

[PATCH v7 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 ad59e65e6771..26a156da3ab4 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1959,6 +1959,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);
@@ -1971,10 +1974,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(core);
 	if (res)
 		return res;
-- 
2.20.1

[PATCH v7 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 26a156da3ab4..9a812d5f45d7 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1354,11 +1354,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;
@@ -1371,15 +1386,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;
 }
@@ -1575,6 +1585,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 v7 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../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 28b64ad149ef..cc98b38c7e73 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
@@ -38,14 +39,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

[PATCH v7 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 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 cc98b38c7e73..93731638b509 100644
--- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
+++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
@@ -49,6 +49,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:
@@ -56,6 +63,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 {
@@ -67,6 +75,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

[PATCH v7 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 | 63 +++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 9a812d5f45d7..8f0daa2f0d58 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)
 
 #define TVP5150_MAX_CONNECTORS	3 /* Check dt-bindings for more informations. */
 
@@ -63,6 +70,7 @@ struct tvp5150 {
 
 	struct media_pad pads[TVP5150_NUM_PADS];
 	struct tvp5150_connector connectors[TVP5150_MAX_CONNECTORS];
+	struct tvp5150_connector *cur_connector;
 	unsigned int connectors_num;
 
 	struct v4l2_ctrl_handler hdl;
@@ -782,17 +790,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);
 }
@@ -1331,6 +1350,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
@@ -1342,6 +1363,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;
@@ -1559,6 +1600,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
@@ -1949,6 +1993,7 @@ static int tvp5150_probe(struct i2c_client *c)
 	struct v4l2_subdev *sd;
 	struct device_node *np = c->dev.of_node;
 	struct regmap *map;
+	unsigned int i;
 	int res;
 
 	/* Check if the adapter supports the needed features */
@@ -1993,7 +2038,21 @@ static int tvp5150_probe(struct i2c_client *c)
 	if (res < 0)
 		return res;
 
-	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.
+	 */
+	for (i = 0; i < core->connectors_num; i++) {
+		struct v4l2_fwnode_connector *vc;
+
+		vc = &core->connectors[i].base;
+		core->norm |= vc->connector.analog.supported_tvnorms;
+	}
+
+	if (!core->connectors_num)
+		core->norm = V4L2_STD_ALL;
+
 	core->detected_norm = V4L2_STD_UNKNOWN;
 	core->input = TVP5150_COMPOSITE1;
 	core->enable = true;
-- 
2.20.1

[PATCH v7 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>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 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 8f0daa2f0d58..0103eae9e459 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -299,9 +299,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 v7 02/13] media: v4l2-fwnode: add v4l2_fwnode_connector

[Hans Verkuil]

On 8/15/19 1:57 PM, Marco Felsch wrote:
> Currently every driver needs to parse the connector endpoints by it self.
> This is the initial work to make this generic. A generic connector has
> common members and connector specific members. The common members are:
>   - type
>   - label (optional)
>   - links
>   - nr_of_links
> 
> The specific members are stored 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>
> ---
> [1] https://patchwork.kernel.org/cover/10794703/
> 
> v7:
> - fix spelling issues
> - constify label
> - support variable label size
> - replace single remote_port/id members by links member of variable
>   size
> - squash v4l2-connector into v4l2-fwnode
> 
> @Jacopo: I dropped your r b tag because I changed the port/id logic.
> 
> v6:
> - fix some spelling and style issues
> - rm unnecessary comments
> - drop vga and dvi connector
> - fix misspelt connector
> 
> v2-v4:
> - nothing since the patch was squashed from series [1] into this
>   series.
> ---
>  include/media/v4l2-fwnode.h | 45 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index f6a7bcd13197..e39c198882fc 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -123,6 +123,51 @@ struct v4l2_fwnode_link {
>  	unsigned int remote_port;
>  };
>  
> +/**
> + * enum v4l2_connector_type - connector type
> + * @V4L2_CON_UNKNOWN:   unknown connector type, no V4L2 connector configuration
> + * @V4L2_CON_COMPOSITE: analog composite connector
> + * @V4L2_CON_SVIDEO:    analog svideo connector
> + * @V4L2_CON_HDMI:      digital hdmi connector
> + */
> +enum v4l2_connector_type {
> +	V4L2_CON_UNKNOWN,
> +	V4L2_CON_COMPOSITE,
> +	V4L2_CON_SVIDEO,
> +	V4L2_CON_HDMI,

Please use CONN instead of CON. CONN is the traditional abbreviation
used for connectors. 'CON' is too generic (there are many words that
start with 'con').

Regards,

	Hans

> +};
> +
> +/**
> + * 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
> + * @label: optional connector label
> + * @type: connector type
> + * @links: list of &struct v4l2_fwnode_link links the connector is connected to
> + * @nr_of_links: total number of links
> + * @connector: connector configuration
> + * @connector.analog: analog connector configuration
> + *                    &struct v4l2_fwnode_connector_analog
> + */
> +struct v4l2_fwnode_connector {
> +	const char *label;
> +	enum v4l2_connector_type type;
> +	struct v4l2_fwnode_link *links;
> +	unsigned int nr_of_links;
> +
> +	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 v7 01/13] dt-bindings: connector: analog: add tv norms property

[Hans Verkuil]

On 8/15/19 1:57 PM, 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>
> ---
> [1] https://patchwork.kernel.org/cover/10794703/
> 
> v7:
> I kept Robs r b tag because I only changed the example and extended
> TVNORM_* macros.
> 
> - fix some style issues
> - add TVNORM_NTSC, TVNORM_525_60 and TVNORM_625_50
> 
> v6:
> - tvnorms.h: use tabs instead of spaces
> - tvnorms.h: add TVNORM_PAL and TVNORM_SECAM
> - tvnorms.h: drop rarely used TVNORM_ATSC_* norms
> 
> v2-v4:
> - nothing since the patch was squashed from series [1] into this
>   series.
> ---
>  .../display/connector/analog-tv-connector.txt |  4 ++
>  include/dt-bindings/media/tvnorms.h           | 72 +++++++++++++++++++
>  2 files changed, 76 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..434e8aa0398b 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

tv -> TV

> +           all tv norms are allowed. Possible video standards are defined in

tv -> TV

And a more high-level question: I think tvnorm is a very vague name. It's a term
used in media, but what does 'norm' mean anyway? 'tv_standards' or 'video_standards'
would be a lot more descriptive.

'tvnorm' is, I think, a term used only internally in the media subsystem for no
clear reason. In the V4L2 spec it talks about 'video standard'.

Sorry for being so late with raising this issue.

> +           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 | TVNORM_NTSC)>;
>  
>  	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..e1275673c4d6
> --- /dev/null
> +++ b/include/dt-bindings/media/tvnorms.h
> @@ -0,0 +1,72 @@
> +/* 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 standard */

I would add a comment here and in videodev2.h where you mention that
the two headers should remain in sync.

> +#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		(TVNORM_PAL_B		| \
> +				 TVNORM_PAL_B1		| \
> +				 TVNORM_PAL_G		| \
> +				 TVNORM_PAL_H		| \
> +				 TVNORM_PAL_I		| \
> +				 TVNORM_PAL_D		| \
> +				 TVNORM_PAL_D1		| \
> +				 TVNORM_PAL_K)
> +
> +#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_NTSC		(TVNORM_NTSC_M		|\
> +				 TVNORM_NTSC_M_JP	|\

Add space before \

> +				 TVNORM_NTSC_M_KR)
> +
> +#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
> +
> +#define TVNORM_SECAM		(TVNORM_SECAM_B		| \
> +				 TVNORM_SECAM_D		| \
> +				 TVNORM_SECAM_G		| \
> +				 TVNORM_SECAM_H		| \
> +				 TVNORM_SECAM_K		| \
> +				 TVNORM_SECAM_K1	| \
> +				 TVNORM_SECAM_L		| \
> +				 TVNORM_SECAM_LC)
> +
> +/* Standards for Countries with 60Hz Line frequency */
> +#define TVNORM_525_60		(TVNORM_PAL_M		| \
> +				 TVNORM_PAL_60		| \
> +				 TVNORM_NTSC		| \
> +				 TVNORM_NTSC_443)
> +
> +/* Standards for Countries with 50Hz Line frequency */
> +#define TVNORM_625_50		(TVNORM_PAL		|\
> +				 TVNORM_PAL_N		|\
> +				 TVNORM_PAL_Nc		|\

Add space before \

> +				 TVNORM_SECAM)
> +
> +#endif /* _DT_BINDINGS_MEDIA_TVNORMS_H */
> 

I was also wondering if this header shouldn't be in include/dt-bindings/display/
since the bindings are also described in 'display'.

Regards,

	Hans

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

[Hans Verkuil]

On 8/15/19 1:57 PM, Marco Felsch wrote:
> 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:
> 
> v7:
> - __tvp5150_get_pad_crop(): return error on default case
> - simplify __tvp5150_get_pad_crop() error handling
> - tvp5150_set_selection() squash __tvp5150_set_selection() execution
>   conditions
> v6:
> nothing
> 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 | 118 +++++++++++++++++++++++++-----------
>  1 file changed, 84 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index dfbf5bbc307c..ad59e65e6771 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"
>  
> @@ -994,20 +995,44 @@ 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);

That should be ERR_PTR(-EINVAL). Just because V4L2_SUBDEV_API is undefined, that
doesn't mean that the whole functionality is not implemented! Just the TRY
is not available.

> +#endif
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		return &decoder->rect;
> +	default:
> +		return ERR_PTR(-EINVAL);
> +	}
> +}
> +
>  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(__crop))
> +		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;
> @@ -1018,17 +1043,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;

No need for __ prefix.

> +	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",
> @@ -1037,17 +1096,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:
> @@ -1060,20 +1109,19 @@ 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 SUBDEV_API is not defined, then just set *__crop = rect and return 0;

See also the commit log of commit fa564e90257e.

> +	if (IS_ERR(__crop))
> +		return PTR_ERR(__crop);
>  
> -	decoder->rect = rect;
> +	/*
> +	 * Update output image size if the selection (crop) rectangle size or
> +	 * position has been modified.
> +	 */
> +	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
> +	    !v4l2_rect_equal(&rect, __crop))
> +		__tvp5150_set_selection(sd, rect);
> +
> +	*__crop = rect;
>  
>  	return 0;
>  }
> @@ -1083,11 +1131,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;
> @@ -1105,7 +1151,11 @@ 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(__crop))
> +			return PTR_ERR(__crop);
> +		sel->r = *__crop;
>  		return 0;
>  	default:
>  		return -EINVAL;
> 

Regards,

	Hans

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

[Hans Verkuil]

Typo in subject: stlye -> style.

Regards,

	Hans

On 8/15/19 1:57 PM, 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>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../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 28b64ad149ef..cc98b38c7e73 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
> @@ -38,14 +39,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:
>  
> 

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

[Rob Herring]

On Thu, Aug 15, 2019 at 01:57:40PM +0200, Marco Felsch wrote:
> 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>
> ---
> Changelog:
> 
> v7:
> Hi Rob,
> I droped your r b tag because I changed the bindings in this
> patch version. Please can you have a look on it again?
> 
> - fix missing AIP1B svideo connection (description and examples)
> 
> 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 | 112 ++++++++++++++++--
>  1 file changed, 105 insertions(+), 7 deletions(-)

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

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

[Marco Felsch]

Hi,

sorry for the noise but this patch has two bugs. First is a rebasing
issue and the second a wrong #ifdef handling. I will prepare a v8
addressing Hans comments and fixing those issues. Please let me know if
you have more comments for this patch.

Regards,
  Marco

On 19-08-15 13:57, Marco Felsch wrote:
> This patch adds the of_graph support to describe the tvp input 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. Look at the
> Documentation for more information. Since the TVP5150 is a converter/bridge
> 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/
> 
> v7:
> - don't init enum tvp5150_pads with TVP5150_COMPOSITE0 functionality
> - break some 80 character limitation to improve readability
> - fix comment style -> always start with capital letters and end with dot
> - fix some minor style issues
> - fix tvp5150_registered error handling
> - simplify tvp5150_mc_init
> - make connectors static
>   -> since now only three connectors are possible (as described in DT)
>   -> drop tvp5150.endpoints storage
>   -> squash tvp5150_parse_dt and tvp5150_add_of_connectors
> - improve tvp5150_parse_dt:
>   -> make parsing stricter and fix not detected missconfigured dt-data
>   -> svideo must be connected now to port@[0,1]/endpoint@1
> 
> v6:
> - fix misspelled comments
> - use 'unsigned int' where it's possible
> - cleanup ifdef part-2:
>   * tvp5150_mc_init, tvp5150_add_of_connectors: add surrounding
>     CONFIG_MEDIA_CONTROLLER if def and stubs to improve quality
> - tvp5150_mc_init: uniform interface, use 'struct tvp5150' since all
>   internal function do this.
> - tvp5150_add_of_connectors: call within probe() to make it cleaner
> - tvp5150_parse_dt: move local loop vars within the loop.
> 
> 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 | 379 +++++++++++++++++++++++++++++++++---
>  1 file changed, 350 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index 11a5fd7e2f58..dfbf5bbc307c 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -32,11 +32,12 @@
>  #define TVP5150_FIELD		V4L2_FIELD_ALTERNATE
>  #define TVP5150_COLORSPACE	V4L2_COLORSPACE_SMPTE170M
>  
> +#define TVP5150_MAX_CONNECTORS	3 /* Check dt-bindings for more informations. */
> +
>  MODULE_DESCRIPTION("Texas Instruments TVP5150A/TVP5150AM1/TVP5151 video decoder driver");
>  MODULE_AUTHOR("Mauro Carvalho Chehab");
>  MODULE_LICENSE("GPL v2");
>  
> -
>  static int debug;
>  module_param(debug, int, 0644);
>  MODULE_PARM_DESC(debug, "Debug level (0-2)");
> @@ -44,16 +45,25 @@ 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_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
> +
>  	struct media_pad pads[TVP5150_NUM_PADS];
> -#endif
> +	struct tvp5150_connector connectors[TVP5150_MAX_CONNECTORS];
> +	unsigned int connectors_num;
> +
>  	struct v4l2_ctrl_handler hdl;
>  	struct v4l2_rect rect;
>  	struct regmap *regmap;
> @@ -1167,6 +1177,130 @@ 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;
> +	unsigned int i;
> +	int 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;
> +	unsigned int i;
> +	int 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 an 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 +1448,70 @@ 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.links[0].remote_port;
> +		unsigned int flags = i ? 0 : MEDIA_LNK_FL_ENABLED;
> +		bool is_svideo = decoder->connectors[i].base.type == V4L2_CON_SVIDEO;
> +
> +		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)
> +			goto err;
> +
> +		if (is_svideo) {
> +			/*
> +			 * Check tvp5150_link_setup() comments for more
> +			 * information.
> +			 */
> +			port = decoder->connectors[i].base.links[1].remote_port;
> +			ret = media_create_pad_link(con, 0, &sd->entity, port,
> +						    flags);
> +			if (ret < 0)
> +				goto err;
> +		}
> +
> +		/* 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;
> +
> +err:
> +	for (i = 0; i < decoder->connectors_num; i++)
> +		media_device_unregister_entity(&decoder->connectors[i].ent);
> +
> +	return ret;
> +}
> +
>  /* ----------------------------------------------------------------------- */
>  
>  static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
> @@ -1367,6 +1565,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,36 +1717,164 @@ static int tvp5150_init(struct i2c_client *c)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +static int tvp5150_mc_init(struct tvp5150 *decoder)
> +{
> +	struct v4l2_subdev *sd = &decoder->sd;
> +	unsigned int i;
> +
> +	sd->entity.ops = &tvp5150_sd_media_ops;
> +	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> +
> +	for (i = 0; i < TVP5150_NUM_PADS - 1; i++) {
> +		decoder->pads[i].flags = MEDIA_PAD_FL_SINK;
> +		decoder->pads[i].sig_type = PAD_SIGNAL_ANALOG;
> +	}
> +
> +	decoder->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +	decoder->pads[i].sig_type = PAD_SIGNAL_DV;
> +
> +	return media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS,
> +				      decoder->pads);
> +}
> +
> +#else /* !defined(CONFIG_MEDIA_CONTROLLER) */
> +
> +static inline int tvp5150_mc_init(struct tvp5150 *decoder)
> +{
> +	return 0;
> +}
> +#endif /* defined(CONFIG_MEDIA_CONTROLLER) */
> +
> +static int
> +tvp5150_add_connector(struct tvp5150 *decoder, struct device_node *np,
> +		      unsigned int portn, unsigned int epn, unsigned int svideo)
> +{
> +	struct device *dev = decoder->sd.dev;
> +	struct device_node *ep_np, *remote_np;
> +	struct v4l2_fwnode_connector *vc;
> +	struct tvp5150_connector *c;
> +	unsigned int next_connector = decoder->connectors_num;
> +	int ret;
> +
> +	/* Get free connector entry. */
> +	if (next_connector >= TVP5150_MAX_CONNECTORS)
> +		return 0;
> +
> +	c = &decoder->connectors[next_connector];
> +	vc = &c->base;
> +	ep_np = of_graph_get_endpoint_by_regs(np, portn, epn);
> +	remote_np = of_graph_get_remote_port_parent(ep_np);
> +
> +	ret = v4l2_fwnode_connector_alloc_parse(of_fwnode_handle(ep_np), vc);
> +	if (ret)
> +		goto err;
> +
> +	if ((!svideo && vc->type != V4L2_CON_COMPOSITE) ||
> +	    (svideo && vc->type != V4L2_CON_SVIDEO)) {
> +		dev_err(dev, "Invalid connector type for port@%u/endpoint@%u\n",
> +			portn, epn);
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	if (svideo &&
> +	    (vc->nr_of_links != 2 ||
> +	     vc->links[0].remote_port == vc->links[1].remote_port)) {
> +		dev_err(dev, "Invalid Svideo connector configuration\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	if (!(vc->connector.analog.supported_tvnorms & TVP5150_STD_MASK)) {
> +		dev_err(dev, "Unsupported tv-norm on connector %s\n",
> +			remote_np->name);
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	c->ent.flags = MEDIA_ENT_FL_CONNECTOR;
> +	c->ent.name = vc->label;
> +	c->ent.name = kasprintf(GFP_KERNEL, "%s %s", remote_np->name,
> +				vc->label ? vc->label : "");
> +	c->ent.function = vc->type == V4L2_CON_SVIDEO ?
> +		MEDIA_ENT_F_CONN_SVIDEO : MEDIA_ENT_F_CONN_COMPOSITE;
> +
> +	decoder->connectors_num++;
> +
> +	of_node_put(remote_np);
> +	of_node_put(ep_np);
> +	return 0;
> +
> +err:
> +	of_node_put(remote_np);
> +	of_node_put(ep_np);
> +	v4l2_fwnode_connector_free(vc);
> +	return ret;
> +}
> +
>  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;
> +	struct device *dev = decoder->sd.dev;
> +	struct device_node *ep_np;
> +	struct v4l2_fwnode_endpoint bus_cfg = {
> +		.bus_type = V4L2_MBUS_UNKNOWN
> +	};
>  	unsigned int flags;
> -	int ret = 0;
> +	int ret, ep_num;
>  
> -	ep = of_graph_get_next_endpoint(np, NULL);
> -	if (!ep)
> +	/* At least 1 output and 1 input. */
> +	ep_num = of_graph_get_endpoint_count(np);
> +	if (ep_num < 2 || ep_num > 5) {
> +		dev_err(dev, "At least 1 input and 1 output must be connected to the device.\n");
>  		return -EINVAL;
> +	}
> +
> +	/* Layout if all connectors are used
> +	 *
> +	 * tvp-5150 port@0 (AIP1A)
> +	 *	endpoint@0 -----------> Comp0-Con  port
> +	 *	endpoint@1 --------+--> Svideo-Con port
> +	 * tvp-5150 port@1 (AIP1B) |
> +	 *	endpoint@1 --------+
> +	 *	endpoint@0 -----------> Comp1-Con  port
> +	 * tvp-5150 port@2
> +	 *	endpoint (video bitstream output at YOUT[0-7] parallel bus)
> +	 */
> +	for_each_endpoint_of_node(np, ep_np) {
> +		struct of_endpoint ep;
> +
> +		of_graph_parse_endpoint(ep_np, &ep);
> +		if (ep.port > 1)
> +			continue;
> +
> +		ret = tvp5150_add_connector(decoder, np, ep.port, ep.id,
> +					    ep.id ? 1 : 0);
> +		if (ret)
> +			return ret;
> +	}
>  
> -	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &bus_cfg);
> +	ep_np = of_graph_get_endpoint_by_regs(np, TVP5150_PAD_VID_OUT, 0);
> +	if (!ep_np) {
> +		dev_err(dev, "Error no output endpoint available\n");
> +		return -EINVAL;
> +	}
> +	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_np), &bus_cfg);
> +	of_node_put(ep_np);
>  	if (ret)
> -		goto err;
> +		return ret;
>  
>  	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;
> +		return -EINVAL;
>  	}
>  
>  	decoder->mbus_type = bus_cfg.bus_type;
>  
> -err:
> -	of_node_put(ep);
> -	return ret;
> +	return decoder->connectors_num ? 0 : -ENODEV;
>  }
>  
>  static const char * const tvp5150_test_patterns[2] = {
> @@ -1592,22 +1922,13 @@ 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)
> +	res = tvp5150_mc_init(core);
> +	if (res)
>  		return res;
>  
> -#endif
> -
>  	res = tvp5150_detect_version(core);
>  	if (res < 0)
>  		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 |

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

[Marco Felsch]

Hi Hans,

On 19-08-16 13:11, Hans Verkuil wrote:
> On 8/15/19 1:57 PM, 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>
> > ---
> > [1] https://patchwork.kernel.org/cover/10794703/
> > 
> > v7:
> > I kept Robs r b tag because I only changed the example and extended
> > TVNORM_* macros.
> > 
> > - fix some style issues
> > - add TVNORM_NTSC, TVNORM_525_60 and TVNORM_625_50
> > 
> > v6:
> > - tvnorms.h: use tabs instead of spaces
> > - tvnorms.h: add TVNORM_PAL and TVNORM_SECAM
> > - tvnorms.h: drop rarely used TVNORM_ATSC_* norms
> > 
> > v2-v4:
> > - nothing since the patch was squashed from series [1] into this
> >   series.
> > ---
> >  .../display/connector/analog-tv-connector.txt |  4 ++
> >  include/dt-bindings/media/tvnorms.h           | 72 +++++++++++++++++++
> >  2 files changed, 76 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..434e8aa0398b 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
> 
> tv -> TV
> 
> > +           all tv norms are allowed. Possible video standards are defined in
> 
> tv -> TV

Changed both thanks.

> And a more high-level question: I think tvnorm is a very vague name. It's a term
> used in media, but what does 'norm' mean anyway? 'tv_standards' or 'video_standards'
> would be a lot more descriptive.

I'm with you *_standards would be more descriptive. Now the question is:
is it a video or a tv standard? Wikipedia says that SECAM, PAL, NTSC is
a television standard. So I prefer the 'tv_standards' or better the
'tv-standards' approach.

> 'tvnorm' is, I think, a term used only internally in the media subsystem for no
> clear reason. In the V4L2 spec it talks about 'video standard'.
> 
> Sorry for being so late with raising this issue.
> 
> > +           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 | TVNORM_NTSC)>;
> >  
> >  	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..e1275673c4d6
> > --- /dev/null
> > +++ b/include/dt-bindings/media/tvnorms.h
> > @@ -0,0 +1,72 @@
> > +/* 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 standard */
> 
> I would add a comment here and in videodev2.h where you mention that
> the two headers should remain in sync.

Yes, good point.

> > +#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		(TVNORM_PAL_B		| \
> > +				 TVNORM_PAL_B1		| \
> > +				 TVNORM_PAL_G		| \
> > +				 TVNORM_PAL_H		| \
> > +				 TVNORM_PAL_I		| \
> > +				 TVNORM_PAL_D		| \
> > +				 TVNORM_PAL_D1		| \
> > +				 TVNORM_PAL_K)
> > +
> > +#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_NTSC		(TVNORM_NTSC_M		|\
> > +				 TVNORM_NTSC_M_JP	|\
> 
> Add space before \
> 
> > +				 TVNORM_NTSC_M_KR)
> > +
> > +#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
> > +
> > +#define TVNORM_SECAM		(TVNORM_SECAM_B		| \
> > +				 TVNORM_SECAM_D		| \
> > +				 TVNORM_SECAM_G		| \
> > +				 TVNORM_SECAM_H		| \
> > +				 TVNORM_SECAM_K		| \
> > +				 TVNORM_SECAM_K1	| \
> > +				 TVNORM_SECAM_L		| \
> > +				 TVNORM_SECAM_LC)
> > +
> > +/* Standards for Countries with 60Hz Line frequency */
> > +#define TVNORM_525_60		(TVNORM_PAL_M		| \
> > +				 TVNORM_PAL_60		| \
> > +				 TVNORM_NTSC		| \
> > +				 TVNORM_NTSC_443)
> > +
> > +/* Standards for Countries with 50Hz Line frequency */
> > +#define TVNORM_625_50		(TVNORM_PAL		|\
> > +				 TVNORM_PAL_N		|\
> > +				 TVNORM_PAL_Nc		|\
> 
> Add space before \

Both space errors fixed.

> > +				 TVNORM_SECAM)
> > +
> > +#endif /* _DT_BINDINGS_MEDIA_TVNORMS_H */
> > 
> 
> I was also wondering if this header shouldn't be in include/dt-bindings/display/
> since the bindings are also described in 'display'.

That's a good one and I really don't know which is the 'right' place for
it. Since the standard has nothing to do with a display I would keep it
within media. But the radeon [1] gpu uses the stds too. So I really
don't know which place is better.

[1] drivers/gpu/drm/radeon/radeon_mode.h. 

Regards,
  Marco

> Regards,
> 
> 	Hans
> 

-- 
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 v7 01/13] dt-bindings: connector: analog: add tv norms property

[Marco Felsch]

Hi,

On 19-08-19 10:09, Marco Felsch wrote:
> Hi Hans,
> 
> On 19-08-16 13:11, Hans Verkuil wrote:
> > On 8/15/19 1:57 PM, 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>
> > > ---
> > > [1] https://patchwork.kernel.org/cover/10794703/
> > > 
> > > v7:
> > > I kept Robs r b tag because I only changed the example and extended
> > > TVNORM_* macros.
> > > 
> > > - fix some style issues
> > > - add TVNORM_NTSC, TVNORM_525_60 and TVNORM_625_50
> > > 
> > > v6:
> > > - tvnorms.h: use tabs instead of spaces
> > > - tvnorms.h: add TVNORM_PAL and TVNORM_SECAM
> > > - tvnorms.h: drop rarely used TVNORM_ATSC_* norms
> > > 
> > > v2-v4:
> > > - nothing since the patch was squashed from series [1] into this
> > >   series.
> > > ---
> > >  .../display/connector/analog-tv-connector.txt |  4 ++
> > >  include/dt-bindings/media/tvnorms.h           | 72 +++++++++++++++++++
> > >  2 files changed, 76 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..434e8aa0398b 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
> > 
> > tv -> TV
> > 
> > > +           all tv norms are allowed. Possible video standards are defined in
> > 
> > tv -> TV
> 
> Changed both thanks.
> 
> > And a more high-level question: I think tvnorm is a very vague name. It's a term
> > used in media, but what does 'norm' mean anyway? 'tv_standards' or 'video_standards'
> > would be a lot more descriptive.
> 
> I'm with you *_standards would be more descriptive. Now the question is:
> is it a video or a tv standard? Wikipedia says that SECAM, PAL, NTSC is
> a television standard. So I prefer the 'tv_standards' or better the
> 'tv-standards' approach.

While on it I noticed that the v4l_std_id is u64. Should we add the
support for u64 here too?

Regards,
  Marco

> > 'tvnorm' is, I think, a term used only internally in the media subsystem for no
> > clear reason. In the V4L2 spec it talks about 'video standard'.
> > 
> > Sorry for being so late with raising this issue.
> > 
> > > +           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 | TVNORM_NTSC)>;
> > >  
> > >  	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..e1275673c4d6
> > > --- /dev/null
> > > +++ b/include/dt-bindings/media/tvnorms.h
> > > @@ -0,0 +1,72 @@
> > > +/* 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 standard */
> > 
> > I would add a comment here and in videodev2.h where you mention that
> > the two headers should remain in sync.
> 
> Yes, good point.
> 
> > > +#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		(TVNORM_PAL_B		| \
> > > +				 TVNORM_PAL_B1		| \
> > > +				 TVNORM_PAL_G		| \
> > > +				 TVNORM_PAL_H		| \
> > > +				 TVNORM_PAL_I		| \
> > > +				 TVNORM_PAL_D		| \
> > > +				 TVNORM_PAL_D1		| \
> > > +				 TVNORM_PAL_K)
> > > +
> > > +#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_NTSC		(TVNORM_NTSC_M		|\
> > > +				 TVNORM_NTSC_M_JP	|\
> > 
> > Add space before \
> > 
> > > +				 TVNORM_NTSC_M_KR)
> > > +
> > > +#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
> > > +
> > > +#define TVNORM_SECAM		(TVNORM_SECAM_B		| \
> > > +				 TVNORM_SECAM_D		| \
> > > +				 TVNORM_SECAM_G		| \
> > > +				 TVNORM_SECAM_H		| \
> > > +				 TVNORM_SECAM_K		| \
> > > +				 TVNORM_SECAM_K1	| \
> > > +				 TVNORM_SECAM_L		| \
> > > +				 TVNORM_SECAM_LC)
> > > +
> > > +/* Standards for Countries with 60Hz Line frequency */
> > > +#define TVNORM_525_60		(TVNORM_PAL_M		| \
> > > +				 TVNORM_PAL_60		| \
> > > +				 TVNORM_NTSC		| \
> > > +				 TVNORM_NTSC_443)
> > > +
> > > +/* Standards for Countries with 50Hz Line frequency */
> > > +#define TVNORM_625_50		(TVNORM_PAL		|\
> > > +				 TVNORM_PAL_N		|\
> > > +				 TVNORM_PAL_Nc		|\
> > 
> > Add space before \
> 
> Both space errors fixed.
> 
> > > +				 TVNORM_SECAM)
> > > +
> > > +#endif /* _DT_BINDINGS_MEDIA_TVNORMS_H */
> > > 
> > 
> > I was also wondering if this header shouldn't be in include/dt-bindings/display/
> > since the bindings are also described in 'display'.
> 
> That's a good one and I really don't know which is the 'right' place for
> it. Since the standard has nothing to do with a display I would keep it
> within media. But the radeon [1] gpu uses the stds too. So I really
> don't know which place is better.
> 
> [1] drivers/gpu/drm/radeon/radeon_mode.h. 
> 
> Regards,
>   Marco
> 
> > Regards,
> > 
> > 	Hans
> > 
> 
> -- 
> 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 |
> 
> 

-- 
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 v7 01/13] dt-bindings: connector: analog: add tv norms property

[Hans Verkuil]

On 8/19/19 10:09 AM, Marco Felsch wrote:
> Hi Hans,
> 
> On 19-08-16 13:11, Hans Verkuil wrote:
>> On 8/15/19 1:57 PM, 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>
>>> ---
>>> [1] https://patchwork.kernel.org/cover/10794703/
>>>
>>> v7:
>>> I kept Robs r b tag because I only changed the example and extended
>>> TVNORM_* macros.
>>>
>>> - fix some style issues
>>> - add TVNORM_NTSC, TVNORM_525_60 and TVNORM_625_50
>>>
>>> v6:
>>> - tvnorms.h: use tabs instead of spaces
>>> - tvnorms.h: add TVNORM_PAL and TVNORM_SECAM
>>> - tvnorms.h: drop rarely used TVNORM_ATSC_* norms
>>>
>>> v2-v4:
>>> - nothing since the patch was squashed from series [1] into this
>>>   series.
>>> ---
>>>  .../display/connector/analog-tv-connector.txt |  4 ++
>>>  include/dt-bindings/media/tvnorms.h           | 72 +++++++++++++++++++
>>>  2 files changed, 76 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..434e8aa0398b 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
>>
>> tv -> TV
>>
>>> +           all tv norms are allowed. Possible video standards are defined in
>>
>> tv -> TV
> 
> Changed both thanks.
> 
>> And a more high-level question: I think tvnorm is a very vague name. It's a term
>> used in media, but what does 'norm' mean anyway? 'tv_standards' or 'video_standards'
>> would be a lot more descriptive.
> 
> I'm with you *_standards would be more descriptive. Now the question is:
> is it a video or a tv standard? Wikipedia says that SECAM, PAL, NTSC is
> a television standard. So I prefer the 'tv_standards' or better the
> 'tv-standards' approach.

It's a bit of both, actually. But in the context of S-Video and Composite connectors
it is a video standard. In the context of a tuner/modulator it would be a TV standard.

I.e. there is no difference between PAL_B and PAL_[GHIDJ] when it comes to S-Video and
Composite connectors, but in the context of a tuner/modulator these defines give
additional information about transmission bands, etc.

That said, I think video-standards is too vague, but so is tv-standards. How about
sdtv-standards? I think that makes a lot of sense since this is really just for SDTV.

So tvnorms.h becomes sdtv-standards.h.

> 
>> 'tvnorm' is, I think, a term used only internally in the media subsystem for no
>> clear reason. In the V4L2 spec it talks about 'video standard'.
>>
>> Sorry for being so late with raising this issue.
>>
>>> +           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 | TVNORM_NTSC)>;
>>>  
>>>  	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..e1275673c4d6
>>> --- /dev/null
>>> +++ b/include/dt-bindings/media/tvnorms.h
>>> @@ -0,0 +1,72 @@
>>> +/* 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 standard */
>>
>> I would add a comment here and in videodev2.h where you mention that
>> the two headers should remain in sync.
> 
> Yes, good point.
> 
>>> +#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		(TVNORM_PAL_B		| \
>>> +				 TVNORM_PAL_B1		| \
>>> +				 TVNORM_PAL_G		| \
>>> +				 TVNORM_PAL_H		| \
>>> +				 TVNORM_PAL_I		| \
>>> +				 TVNORM_PAL_D		| \
>>> +				 TVNORM_PAL_D1		| \
>>> +				 TVNORM_PAL_K)
>>> +
>>> +#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_NTSC		(TVNORM_NTSC_M		|\
>>> +				 TVNORM_NTSC_M_JP	|\
>>
>> Add space before \
>>
>>> +				 TVNORM_NTSC_M_KR)
>>> +
>>> +#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
>>> +
>>> +#define TVNORM_SECAM		(TVNORM_SECAM_B		| \
>>> +				 TVNORM_SECAM_D		| \
>>> +				 TVNORM_SECAM_G		| \
>>> +				 TVNORM_SECAM_H		| \
>>> +				 TVNORM_SECAM_K		| \
>>> +				 TVNORM_SECAM_K1	| \
>>> +				 TVNORM_SECAM_L		| \
>>> +				 TVNORM_SECAM_LC)
>>> +
>>> +/* Standards for Countries with 60Hz Line frequency */
>>> +#define TVNORM_525_60		(TVNORM_PAL_M		| \
>>> +				 TVNORM_PAL_60		| \
>>> +				 TVNORM_NTSC		| \
>>> +				 TVNORM_NTSC_443)
>>> +
>>> +/* Standards for Countries with 50Hz Line frequency */
>>> +#define TVNORM_625_50		(TVNORM_PAL		|\
>>> +				 TVNORM_PAL_N		|\
>>> +				 TVNORM_PAL_Nc		|\
>>
>> Add space before \
> 
> Both space errors fixed.
> 
>>> +				 TVNORM_SECAM)
>>> +
>>> +#endif /* _DT_BINDINGS_MEDIA_TVNORMS_H */
>>>
>>
>> I was also wondering if this header shouldn't be in include/dt-bindings/display/
>> since the bindings are also described in 'display'.
> 
> That's a good one and I really don't know which is the 'right' place for
> it. Since the standard has nothing to do with a display I would keep it
> within media. But the radeon [1] gpu uses the stds too. So I really
> don't know which place is better.

Move it to display: that matches where the documentation is.

Note that this means that _DT_BINDINGS_MEDIA_TVNORMS_H also becomes
_DT_BINDINGS_DISPLAY_TVNORMS_H (or DISPLAY_SDTV_STANDARDS_H).

Regards,

	Hans

> 
> [1] drivers/gpu/drm/radeon/radeon_mode.h. 
> 
> Regards,
>   Marco
> 
>> Regards,
>>
>> 	Hans
>>
> 

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

[Hans Verkuil]

On 8/19/19 10:18 AM, Marco Felsch wrote:
> Hi,
> 
> On 19-08-19 10:09, Marco Felsch wrote:
>> Hi Hans,
>>
>> On 19-08-16 13:11, Hans Verkuil wrote:
>>> On 8/15/19 1:57 PM, 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>
>>>> ---
>>>> [1] https://patchwork.kernel.org/cover/10794703/
>>>>
>>>> v7:
>>>> I kept Robs r b tag because I only changed the example and extended
>>>> TVNORM_* macros.
>>>>
>>>> - fix some style issues
>>>> - add TVNORM_NTSC, TVNORM_525_60 and TVNORM_625_50
>>>>
>>>> v6:
>>>> - tvnorms.h: use tabs instead of spaces
>>>> - tvnorms.h: add TVNORM_PAL and TVNORM_SECAM
>>>> - tvnorms.h: drop rarely used TVNORM_ATSC_* norms
>>>>
>>>> v2-v4:
>>>> - nothing since the patch was squashed from series [1] into this
>>>>   series.
>>>> ---
>>>>  .../display/connector/analog-tv-connector.txt |  4 ++
>>>>  include/dt-bindings/media/tvnorms.h           | 72 +++++++++++++++++++
>>>>  2 files changed, 76 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..434e8aa0398b 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
>>>
>>> tv -> TV
>>>
>>>> +           all tv norms are allowed. Possible video standards are defined in
>>>
>>> tv -> TV
>>
>> Changed both thanks.
>>
>>> And a more high-level question: I think tvnorm is a very vague name. It's a term
>>> used in media, but what does 'norm' mean anyway? 'tv_standards' or 'video_standards'
>>> would be a lot more descriptive.
>>
>> I'm with you *_standards would be more descriptive. Now the question is:
>> is it a video or a tv standard? Wikipedia says that SECAM, PAL, NTSC is
>> a television standard. So I prefer the 'tv_standards' or better the
>> 'tv-standards' approach.
> 
> While on it I noticed that the v4l_std_id is u64. Should we add the
> support for u64 here too?

No need. SDTV standards are frozen (i.e. no new standards will ever be added),
so everything fits nicely in 32 bits.

Regards,

	Hans

> 
> Regards,
>   Marco
> 
>>> 'tvnorm' is, I think, a term used only internally in the media subsystem for no
>>> clear reason. In the V4L2 spec it talks about 'video standard'.
>>>
>>> Sorry for being so late with raising this issue.
>>>
>>>> +           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 | TVNORM_NTSC)>;
>>>>  
>>>>  	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..e1275673c4d6
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/media/tvnorms.h
>>>> @@ -0,0 +1,72 @@
>>>> +/* 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 standard */
>>>
>>> I would add a comment here and in videodev2.h where you mention that
>>> the two headers should remain in sync.
>>
>> Yes, good point.
>>
>>>> +#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		(TVNORM_PAL_B		| \
>>>> +				 TVNORM_PAL_B1		| \
>>>> +				 TVNORM_PAL_G		| \
>>>> +				 TVNORM_PAL_H		| \
>>>> +				 TVNORM_PAL_I		| \
>>>> +				 TVNORM_PAL_D		| \
>>>> +				 TVNORM_PAL_D1		| \
>>>> +				 TVNORM_PAL_K)
>>>> +
>>>> +#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_NTSC		(TVNORM_NTSC_M		|\
>>>> +				 TVNORM_NTSC_M_JP	|\
>>>
>>> Add space before \
>>>
>>>> +				 TVNORM_NTSC_M_KR)
>>>> +
>>>> +#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
>>>> +
>>>> +#define TVNORM_SECAM		(TVNORM_SECAM_B		| \
>>>> +				 TVNORM_SECAM_D		| \
>>>> +				 TVNORM_SECAM_G		| \
>>>> +				 TVNORM_SECAM_H		| \
>>>> +				 TVNORM_SECAM_K		| \
>>>> +				 TVNORM_SECAM_K1	| \
>>>> +				 TVNORM_SECAM_L		| \
>>>> +				 TVNORM_SECAM_LC)
>>>> +
>>>> +/* Standards for Countries with 60Hz Line frequency */
>>>> +#define TVNORM_525_60		(TVNORM_PAL_M		| \
>>>> +				 TVNORM_PAL_60		| \
>>>> +				 TVNORM_NTSC		| \
>>>> +				 TVNORM_NTSC_443)
>>>> +
>>>> +/* Standards for Countries with 50Hz Line frequency */
>>>> +#define TVNORM_625_50		(TVNORM_PAL		|\
>>>> +				 TVNORM_PAL_N		|\
>>>> +				 TVNORM_PAL_Nc		|\
>>>
>>> Add space before \
>>
>> Both space errors fixed.
>>
>>>> +				 TVNORM_SECAM)
>>>> +
>>>> +#endif /* _DT_BINDINGS_MEDIA_TVNORMS_H */
>>>>
>>>
>>> I was also wondering if this header shouldn't be in include/dt-bindings/display/
>>> since the bindings are also described in 'display'.
>>
>> That's a good one and I really don't know which is the 'right' place for
>> it. Since the standard has nothing to do with a display I would keep it
>> within media. But the radeon [1] gpu uses the stds too. So I really
>> don't know which place is better.
>>
>> [1] drivers/gpu/drm/radeon/radeon_mode.h. 
>>
>> Regards,
>>   Marco
>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>
>> -- 
>> 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 v7 01/13] dt-bindings: connector: analog: add tv norms property

[Marco Felsch]

On 19-08-19 10:39, Hans Verkuil wrote:
> On 8/19/19 10:09 AM, Marco Felsch wrote:
> > Hi Hans,
> > 
> > On 19-08-16 13:11, Hans Verkuil wrote:
> >> On 8/15/19 1:57 PM, 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>
> >>> ---
> >>> [1] https://patchwork.kernel.org/cover/10794703/
> >>>
> >>> v7:
> >>> I kept Robs r b tag because I only changed the example and extended
> >>> TVNORM_* macros.
> >>>
> >>> - fix some style issues
> >>> - add TVNORM_NTSC, TVNORM_525_60 and TVNORM_625_50
> >>>
> >>> v6:
> >>> - tvnorms.h: use tabs instead of spaces
> >>> - tvnorms.h: add TVNORM_PAL and TVNORM_SECAM
> >>> - tvnorms.h: drop rarely used TVNORM_ATSC_* norms
> >>>
> >>> v2-v4:
> >>> - nothing since the patch was squashed from series [1] into this
> >>>   series.
> >>> ---
> >>>  .../display/connector/analog-tv-connector.txt |  4 ++
> >>>  include/dt-bindings/media/tvnorms.h           | 72 +++++++++++++++++++
> >>>  2 files changed, 76 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..434e8aa0398b 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
> >>
> >> tv -> TV
> >>
> >>> +           all tv norms are allowed. Possible video standards are defined in
> >>
> >> tv -> TV
> > 
> > Changed both thanks.
> > 
> >> And a more high-level question: I think tvnorm is a very vague name. It's a term
> >> used in media, but what does 'norm' mean anyway? 'tv_standards' or 'video_standards'
> >> would be a lot more descriptive.
> > 
> > I'm with you *_standards would be more descriptive. Now the question is:
> > is it a video or a tv standard? Wikipedia says that SECAM, PAL, NTSC is
> > a television standard. So I prefer the 'tv_standards' or better the
> > 'tv-standards' approach.
> 
> It's a bit of both, actually. But in the context of S-Video and Composite connectors
> it is a video standard. In the context of a tuner/modulator it would be a TV standard.
> 
> I.e. there is no difference between PAL_B and PAL_[GHIDJ] when it comes to S-Video and
> Composite connectors, but in the context of a tuner/modulator these defines give
> additional information about transmission bands, etc.
> 
> That said, I think video-standards is too vague, but so is tv-standards. How about
> sdtv-standards? I think that makes a lot of sense since this is really just for SDTV.

Yes, thats a good name.

> So tvnorms.h becomes sdtv-standards.h.

Of course.

> > 
> >> 'tvnorm' is, I think, a term used only internally in the media subsystem for no
> >> clear reason. In the V4L2 spec it talks about 'video standard'.
> >>
> >> Sorry for being so late with raising this issue.
> >>
> >>> +           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 | TVNORM_NTSC)>;
> >>>  
> >>>  	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..e1275673c4d6
> >>> --- /dev/null
> >>> +++ b/include/dt-bindings/media/tvnorms.h
> >>> @@ -0,0 +1,72 @@
> >>> +/* 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 standard */
> >>
> >> I would add a comment here and in videodev2.h where you mention that
> >> the two headers should remain in sync.
> > 
> > Yes, good point.
> > 
> >>> +#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		(TVNORM_PAL_B		| \
> >>> +				 TVNORM_PAL_B1		| \
> >>> +				 TVNORM_PAL_G		| \
> >>> +				 TVNORM_PAL_H		| \
> >>> +				 TVNORM_PAL_I		| \
> >>> +				 TVNORM_PAL_D		| \
> >>> +				 TVNORM_PAL_D1		| \
> >>> +				 TVNORM_PAL_K)
> >>> +
> >>> +#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_NTSC		(TVNORM_NTSC_M		|\
> >>> +				 TVNORM_NTSC_M_JP	|\
> >>
> >> Add space before \
> >>
> >>> +				 TVNORM_NTSC_M_KR)
> >>> +
> >>> +#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
> >>> +
> >>> +#define TVNORM_SECAM		(TVNORM_SECAM_B		| \
> >>> +				 TVNORM_SECAM_D		| \
> >>> +				 TVNORM_SECAM_G		| \
> >>> +				 TVNORM_SECAM_H		| \
> >>> +				 TVNORM_SECAM_K		| \
> >>> +				 TVNORM_SECAM_K1	| \
> >>> +				 TVNORM_SECAM_L		| \
> >>> +				 TVNORM_SECAM_LC)
> >>> +
> >>> +/* Standards for Countries with 60Hz Line frequency */
> >>> +#define TVNORM_525_60		(TVNORM_PAL_M		| \
> >>> +				 TVNORM_PAL_60		| \
> >>> +				 TVNORM_NTSC		| \
> >>> +				 TVNORM_NTSC_443)
> >>> +
> >>> +/* Standards for Countries with 50Hz Line frequency */
> >>> +#define TVNORM_625_50		(TVNORM_PAL		|\
> >>> +				 TVNORM_PAL_N		|\
> >>> +				 TVNORM_PAL_Nc		|\
> >>
> >> Add space before \
> > 
> > Both space errors fixed.
> > 
> >>> +				 TVNORM_SECAM)
> >>> +
> >>> +#endif /* _DT_BINDINGS_MEDIA_TVNORMS_H */
> >>>
> >>
> >> I was also wondering if this header shouldn't be in include/dt-bindings/display/
> >> since the bindings are also described in 'display'.
> > 
> > That's a good one and I really don't know which is the 'right' place for
> > it. Since the standard has nothing to do with a display I would keep it
> > within media. But the radeon [1] gpu uses the stds too. So I really
> > don't know which place is better.
> 
> Move it to display: that matches where the documentation is.

Okay.

> 
> Note that this means that _DT_BINDINGS_MEDIA_TVNORMS_H also becomes
> _DT_BINDINGS_DISPLAY_TVNORMS_H (or DISPLAY_SDTV_STANDARDS_H).

Yes.

Last question. I replaced TVNORM_* by TV_STD_*. Should I move to
SDTV_STD_* too?

Regards,
  Marco

> Regards,
> 
> 	Hans
> 
> > 
> > [1] drivers/gpu/drm/radeon/radeon_mode.h. 
> > 
> > Regards,
> >   Marco
> > 
> >> Regards,
> >>
> >> 	Hans
> >>
> > 
> 
> 

-- 
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 v7 01/13] dt-bindings: connector: analog: add tv norms property

[Hans Verkuil]

On 8/19/19 11:01 AM, Marco Felsch wrote:
> On 19-08-19 10:39, Hans Verkuil wrote:
>> On 8/19/19 10:09 AM, Marco Felsch wrote:
>>> Hi Hans,
>>>
>>> On 19-08-16 13:11, Hans Verkuil wrote:
>>>> On 8/15/19 1:57 PM, 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>
>>>>> ---
>>>>> [1] https://patchwork.kernel.org/cover/10794703/
>>>>>
>>>>> v7:
>>>>> I kept Robs r b tag because I only changed the example and extended
>>>>> TVNORM_* macros.
>>>>>
>>>>> - fix some style issues
>>>>> - add TVNORM_NTSC, TVNORM_525_60 and TVNORM_625_50
>>>>>
>>>>> v6:
>>>>> - tvnorms.h: use tabs instead of spaces
>>>>> - tvnorms.h: add TVNORM_PAL and TVNORM_SECAM
>>>>> - tvnorms.h: drop rarely used TVNORM_ATSC_* norms
>>>>>
>>>>> v2-v4:
>>>>> - nothing since the patch was squashed from series [1] into this
>>>>>   series.
>>>>> ---
>>>>>  .../display/connector/analog-tv-connector.txt |  4 ++
>>>>>  include/dt-bindings/media/tvnorms.h           | 72 +++++++++++++++++++
>>>>>  2 files changed, 76 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..434e8aa0398b 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
>>>>
>>>> tv -> TV
>>>>
>>>>> +           all tv norms are allowed. Possible video standards are defined in
>>>>
>>>> tv -> TV
>>>
>>> Changed both thanks.
>>>
>>>> And a more high-level question: I think tvnorm is a very vague name. It's a term
>>>> used in media, but what does 'norm' mean anyway? 'tv_standards' or 'video_standards'
>>>> would be a lot more descriptive.
>>>
>>> I'm with you *_standards would be more descriptive. Now the question is:
>>> is it a video or a tv standard? Wikipedia says that SECAM, PAL, NTSC is
>>> a television standard. So I prefer the 'tv_standards' or better the
>>> 'tv-standards' approach.
>>
>> It's a bit of both, actually. But in the context of S-Video and Composite connectors
>> it is a video standard. In the context of a tuner/modulator it would be a TV standard.
>>
>> I.e. there is no difference between PAL_B and PAL_[GHIDJ] when it comes to S-Video and
>> Composite connectors, but in the context of a tuner/modulator these defines give
>> additional information about transmission bands, etc.
>>
>> That said, I think video-standards is too vague, but so is tv-standards. How about
>> sdtv-standards? I think that makes a lot of sense since this is really just for SDTV.
> 
> Yes, thats a good name.
> 
>> So tvnorms.h becomes sdtv-standards.h.
> 
> Of course.
> 
>>>
>>>> 'tvnorm' is, I think, a term used only internally in the media subsystem for no
>>>> clear reason. In the V4L2 spec it talks about 'video standard'.
>>>>
>>>> Sorry for being so late with raising this issue.
>>>>
>>>>> +           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 | TVNORM_NTSC)>;
>>>>>  
>>>>>  	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..e1275673c4d6
>>>>> --- /dev/null
>>>>> +++ b/include/dt-bindings/media/tvnorms.h
>>>>> @@ -0,0 +1,72 @@
>>>>> +/* 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 standard */
>>>>
>>>> I would add a comment here and in videodev2.h where you mention that
>>>> the two headers should remain in sync.
>>>
>>> Yes, good point.
>>>
>>>>> +#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		(TVNORM_PAL_B		| \
>>>>> +				 TVNORM_PAL_B1		| \
>>>>> +				 TVNORM_PAL_G		| \
>>>>> +				 TVNORM_PAL_H		| \
>>>>> +				 TVNORM_PAL_I		| \
>>>>> +				 TVNORM_PAL_D		| \
>>>>> +				 TVNORM_PAL_D1		| \
>>>>> +				 TVNORM_PAL_K)
>>>>> +
>>>>> +#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_NTSC		(TVNORM_NTSC_M		|\
>>>>> +				 TVNORM_NTSC_M_JP	|\
>>>>
>>>> Add space before \
>>>>
>>>>> +				 TVNORM_NTSC_M_KR)
>>>>> +
>>>>> +#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
>>>>> +
>>>>> +#define TVNORM_SECAM		(TVNORM_SECAM_B		| \
>>>>> +				 TVNORM_SECAM_D		| \
>>>>> +				 TVNORM_SECAM_G		| \
>>>>> +				 TVNORM_SECAM_H		| \
>>>>> +				 TVNORM_SECAM_K		| \
>>>>> +				 TVNORM_SECAM_K1	| \
>>>>> +				 TVNORM_SECAM_L		| \
>>>>> +				 TVNORM_SECAM_LC)
>>>>> +
>>>>> +/* Standards for Countries with 60Hz Line frequency */
>>>>> +#define TVNORM_525_60		(TVNORM_PAL_M		| \
>>>>> +				 TVNORM_PAL_60		| \
>>>>> +				 TVNORM_NTSC		| \
>>>>> +				 TVNORM_NTSC_443)
>>>>> +
>>>>> +/* Standards for Countries with 50Hz Line frequency */
>>>>> +#define TVNORM_625_50		(TVNORM_PAL		|\
>>>>> +				 TVNORM_PAL_N		|\
>>>>> +				 TVNORM_PAL_Nc		|\
>>>>
>>>> Add space before \
>>>
>>> Both space errors fixed.
>>>
>>>>> +				 TVNORM_SECAM)
>>>>> +
>>>>> +#endif /* _DT_BINDINGS_MEDIA_TVNORMS_H */
>>>>>
>>>>
>>>> I was also wondering if this header shouldn't be in include/dt-bindings/display/
>>>> since the bindings are also described in 'display'.
>>>
>>> That's a good one and I really don't know which is the 'right' place for
>>> it. Since the standard has nothing to do with a display I would keep it
>>> within media. But the radeon [1] gpu uses the stds too. So I really
>>> don't know which place is better.
>>
>> Move it to display: that matches where the documentation is.
> 
> Okay.
> 
>>
>> Note that this means that _DT_BINDINGS_MEDIA_TVNORMS_H also becomes
>> _DT_BINDINGS_DISPLAY_TVNORMS_H (or DISPLAY_SDTV_STANDARDS_H).
> 
> Yes.
> 
> Last question. I replaced TVNORM_* by TV_STD_*. Should I move to
> SDTV_STD_* too?

I think so, yes.

Regards,

	Hans

> 
> Regards,
>   Marco
> 
>> Regards,
>>
>> 	Hans
>>
>>>
>>> [1] drivers/gpu/drm/radeon/radeon_mode.h. 
>>>
>>> Regards,
>>>   Marco
>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>
>>
>>
> 

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

[Marco Felsch]

On 19-08-16 12:59, Hans Verkuil wrote:
> On 8/15/19 1:57 PM, Marco Felsch wrote:
> > Currently every driver needs to parse the connector endpoints by it self.
> > This is the initial work to make this generic. A generic connector has
> > common members and connector specific members. The common members are:
> >   - type
> >   - label (optional)
> >   - links
> >   - nr_of_links
> > 
> > The specific members are stored 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>
> > ---
> > [1] https://patchwork.kernel.org/cover/10794703/
> > 
> > v7:
> > - fix spelling issues
> > - constify label
> > - support variable label size
> > - replace single remote_port/id members by links member of variable
> >   size
> > - squash v4l2-connector into v4l2-fwnode
> > 
> > @Jacopo: I dropped your r b tag because I changed the port/id logic.
> > 
> > v6:
> > - fix some spelling and style issues
> > - rm unnecessary comments
> > - drop vga and dvi connector
> > - fix misspelt connector
> > 
> > v2-v4:
> > - nothing since the patch was squashed from series [1] into this
> >   series.
> > ---
> >  include/media/v4l2-fwnode.h | 45 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> > 
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index f6a7bcd13197..e39c198882fc 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -123,6 +123,51 @@ struct v4l2_fwnode_link {
> >  	unsigned int remote_port;
> >  };
> >  
> > +/**
> > + * enum v4l2_connector_type - connector type
> > + * @V4L2_CON_UNKNOWN:   unknown connector type, no V4L2 connector configuration
> > + * @V4L2_CON_COMPOSITE: analog composite connector
> > + * @V4L2_CON_SVIDEO:    analog svideo connector
> > + * @V4L2_CON_HDMI:      digital hdmi connector
> > + */
> > +enum v4l2_connector_type {
> > +	V4L2_CON_UNKNOWN,
> > +	V4L2_CON_COMPOSITE,
> > +	V4L2_CON_SVIDEO,
> > +	V4L2_CON_HDMI,
> 
> Please use CONN instead of CON. CONN is the traditional abbreviation
> used for connectors. 'CON' is too generic (there are many words that
> start with 'con').

Okay.

> Regards,
> 
> 	Hans
> 
> > +};
> > +
> > +/**
> > + * 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;

After we found the correct dt-binding and naming convention I would
change that property to 'sdtv_stds'. Is that okay?

Regards,
  Marco

> > +};
> > +
> > +/**
> > + * struct v4l2_fwnode_connector - the connector data structure
> > + * @label: optional connector label
> > + * @type: connector type
> > + * @links: list of &struct v4l2_fwnode_link links the connector is connected to
> > + * @nr_of_links: total number of links
> > + * @connector: connector configuration
> > + * @connector.analog: analog connector configuration
> > + *                    &struct v4l2_fwnode_connector_analog
> > + */
> > +struct v4l2_fwnode_connector {
> > +	const char *label;
> > +	enum v4l2_connector_type type;
> > +	struct v4l2_fwnode_link *links;
> > +	unsigned int nr_of_links;
> > +
> > +	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 |

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

[Hans Verkuil]

On 8/19/19 11:17 AM, Marco Felsch wrote:
> On 19-08-16 12:59, Hans Verkuil wrote:
>> On 8/15/19 1:57 PM, Marco Felsch wrote:
>>> Currently every driver needs to parse the connector endpoints by it self.
>>> This is the initial work to make this generic. A generic connector has
>>> common members and connector specific members. The common members are:
>>>   - type
>>>   - label (optional)
>>>   - links
>>>   - nr_of_links
>>>
>>> The specific members are stored 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>
>>> ---
>>> [1] https://patchwork.kernel.org/cover/10794703/
>>>
>>> v7:
>>> - fix spelling issues
>>> - constify label
>>> - support variable label size
>>> - replace single remote_port/id members by links member of variable
>>>   size
>>> - squash v4l2-connector into v4l2-fwnode
>>>
>>> @Jacopo: I dropped your r b tag because I changed the port/id logic.
>>>
>>> v6:
>>> - fix some spelling and style issues
>>> - rm unnecessary comments
>>> - drop vga and dvi connector
>>> - fix misspelt connector
>>>
>>> v2-v4:
>>> - nothing since the patch was squashed from series [1] into this
>>>   series.
>>> ---
>>>  include/media/v4l2-fwnode.h | 45 +++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 45 insertions(+)
>>>
>>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
>>> index f6a7bcd13197..e39c198882fc 100644
>>> --- a/include/media/v4l2-fwnode.h
>>> +++ b/include/media/v4l2-fwnode.h
>>> @@ -123,6 +123,51 @@ struct v4l2_fwnode_link {
>>>  	unsigned int remote_port;
>>>  };
>>>  
>>> +/**
>>> + * enum v4l2_connector_type - connector type
>>> + * @V4L2_CON_UNKNOWN:   unknown connector type, no V4L2 connector configuration
>>> + * @V4L2_CON_COMPOSITE: analog composite connector
>>> + * @V4L2_CON_SVIDEO:    analog svideo connector
>>> + * @V4L2_CON_HDMI:      digital hdmi connector
>>> + */
>>> +enum v4l2_connector_type {
>>> +	V4L2_CON_UNKNOWN,
>>> +	V4L2_CON_COMPOSITE,
>>> +	V4L2_CON_SVIDEO,
>>> +	V4L2_CON_HDMI,
>>
>> Please use CONN instead of CON. CONN is the traditional abbreviation
>> used for connectors. 'CON' is too generic (there are many words that
>> start with 'con').
> 
> Okay.
> 
>> Regards,
>>
>> 	Hans
>>
>>> +};
>>> +
>>> +/**
>>> + * 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;
> 
> After we found the correct dt-binding and naming convention I would
> change that property to 'sdtv_stds'. Is that okay?

Yes.

	Hans

> 
> Regards,
>   Marco
> 
>>> +};
>>> +
>>> +/**
>>> + * struct v4l2_fwnode_connector - the connector data structure
>>> + * @label: optional connector label
>>> + * @type: connector type
>>> + * @links: list of &struct v4l2_fwnode_link links the connector is connected to
>>> + * @nr_of_links: total number of links
>>> + * @connector: connector configuration
>>> + * @connector.analog: analog connector configuration
>>> + *                    &struct v4l2_fwnode_connector_analog
>>> + */
>>> +struct v4l2_fwnode_connector {
>>> +	const char *label;
>>> +	enum v4l2_connector_type type;
>>> +	struct v4l2_fwnode_link *links;
>>> +	unsigned int nr_of_links;
>>> +
>>> +	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 v7 07/13] media: tvp5150: add FORMAT_TRY support for get/set selection handlers

[Marco Felsch]

On 19-08-16 13:27, Hans Verkuil wrote:
> On 8/15/19 1:57 PM, Marco Felsch wrote:
> > 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:
> > 
> > v7:
> > - __tvp5150_get_pad_crop(): return error on default case
> > - simplify __tvp5150_get_pad_crop() error handling
> > - tvp5150_set_selection() squash __tvp5150_set_selection() execution
> >   conditions
> > v6:
> > nothing
> > 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 | 118 +++++++++++++++++++++++++-----------
> >  1 file changed, 84 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > index dfbf5bbc307c..ad59e65e6771 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"
> >  
> > @@ -994,20 +995,44 @@ 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);
> 
> That should be ERR_PTR(-EINVAL). Just because V4L2_SUBDEV_API is undefined, that
> doesn't mean that the whole functionality is not implemented! Just the TRY
> is not available.

Okay.. The patch was made before you changed that for the other i2c
devices. I will change that.

> 
> > +#endif
> > +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > +		return &decoder->rect;
> > +	default:
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +}
> > +
> >  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(__crop))
> > +		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;
> > @@ -1018,17 +1043,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;
> 
> No need for __ prefix.

I dropped the __ prefix for all local crop vars.

> > +	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",
> > @@ -1037,17 +1096,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:
> > @@ -1060,20 +1109,19 @@ 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 SUBDEV_API is not defined, then just set *__crop = rect and return 0;
> 
> See also the commit log of commit fa564e90257e.

Applies this to the set/get_selection() too?

Regards,
  Marco


> > +	if (IS_ERR(__crop))
> > +		return PTR_ERR(__crop);
> >  
> > -	decoder->rect = rect;
> > +	/*
> > +	 * Update output image size if the selection (crop) rectangle size or
> > +	 * position has been modified.
> > +	 */
> > +	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
> > +	    !v4l2_rect_equal(&rect, __crop))
> > +		__tvp5150_set_selection(sd, rect);
> > +
> > +	*__crop = rect;
> >  
> >  	return 0;
> >  }
> > @@ -1083,11 +1131,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;
> > @@ -1105,7 +1151,11 @@ 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(__crop))
> > +			return PTR_ERR(__crop);
> > +		sel->r = *__crop;
> >  		return 0;
> >  	default:
> >  		return -EINVAL;
> > 
> 
> Regards,
> 
> 	Hans
> 

-- 
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 v7 07/13] media: tvp5150: add FORMAT_TRY support for get/set selection handlers

[Hans Verkuil]

On 8/19/19 12:40 PM, Marco Felsch wrote:
> On 19-08-16 13:27, Hans Verkuil wrote:
>> On 8/15/19 1:57 PM, Marco Felsch wrote:
>>> 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:
>>>
>>> v7:
>>> - __tvp5150_get_pad_crop(): return error on default case
>>> - simplify __tvp5150_get_pad_crop() error handling
>>> - tvp5150_set_selection() squash __tvp5150_set_selection() execution
>>>   conditions
>>> v6:
>>> nothing
>>> 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 | 118 +++++++++++++++++++++++++-----------
>>>  1 file changed, 84 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
>>> index dfbf5bbc307c..ad59e65e6771 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"
>>>  
>>> @@ -994,20 +995,44 @@ 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);
>>
>> That should be ERR_PTR(-EINVAL). Just because V4L2_SUBDEV_API is undefined, that
>> doesn't mean that the whole functionality is not implemented! Just the TRY
>> is not available.
> 
> Okay.. The patch was made before you changed that for the other i2c
> devices. I will change that.
> 
>>
>>> +#endif
>>> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
>>> +		return &decoder->rect;
>>> +	default:
>>> +		return ERR_PTR(-EINVAL);
>>> +	}
>>> +}
>>> +
>>>  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(__crop))
>>> +		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;
>>> @@ -1018,17 +1043,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;
>>
>> No need for __ prefix.
> 
> I dropped the __ prefix for all local crop vars.
> 
>>> +	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",
>>> @@ -1037,17 +1096,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:
>>> @@ -1060,20 +1109,19 @@ 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 SUBDEV_API is not defined, then just set *__crop = rect and return 0;
>>
>> See also the commit log of commit fa564e90257e.
> 
> Applies this to the set/get_selection() too?

Yes.

Regards,

	Hans

> 
> Regards,
>   Marco
> 
> 
>>> +	if (IS_ERR(__crop))
>>> +		return PTR_ERR(__crop);
>>>  
>>> -	decoder->rect = rect;
>>> +	/*
>>> +	 * Update output image size if the selection (crop) rectangle size or
>>> +	 * position has been modified.
>>> +	 */
>>> +	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
>>> +	    !v4l2_rect_equal(&rect, __crop))
>>> +		__tvp5150_set_selection(sd, rect);
>>> +
>>> +	*__crop = rect;
>>>  
>>>  	return 0;
>>>  }
>>> @@ -1083,11 +1131,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;
>>> @@ -1105,7 +1151,11 @@ 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(__crop))
>>> +			return PTR_ERR(__crop);
>>> +		sel->r = *__crop;
>>>  		return 0;
>>>  	default:
>>>  		return -EINVAL;
>>>
>>
>> Regards,
>>
>> 	Hans
>>
>