[PATCH v2 0/7] TVP5150 fixes and new features

[PATCH v2 0/7] TVP5150 fixes and new features

[Marco Felsch]

Hi,

this is my v2 with the integrated reviews from my v1 [1]. Since Mauro
applied the most patches from my v1 to his experimental/tvp5150-3
branch [2], this series only contains those which aren't applied.

Patches I changed contain a changelog, so I will omit these here.

Patch ('[media] tvp5150: add FORMAT_TRY support for get/set selection
handlers') throws a compile error. Therefore I added two v4l2-subdev.h
patches which should fix the error in a common way.

Patch ('[media] tvp5150: add s_power callback') is new too. I forget
them in my v1. This patch address the interrupt enable/disable handling.
Now it is possible to pause streaming and keep the interrupts on.

The changes I made in the ('[media] tvp5150: add input source selection
of_graph support') patch are based on the the RFC [3] and discussion [4].
I dropped patch ('[media] tvp5150: Change default input source selection
behaviour') since the default input source selectopn is setup during the
.registered() callback now.

I've tested this series on a customer dt-based board. Unfortunately I
haven't a device which use the em28xx driver. So other tester a welcome :)

[1] https://www.spinics.net/lists/devicetree/msg236650.html
[2] https://git.linuxtv.org/mchehab/experimental.git/log/?h=tvp5150-3
[3] https://www.spinics.net/lists/devicetree/msg243181.html
[4] https://www.spinics.net/lists/devicetree/msg243840.html

Regards,
Marco

Marco Felsch (6):
  [media] tvp5150: add input source selection of_graph support
  [media] dt-bindings: tvp5150: Add input port connectors DT bindings
  [media] v4l2-subdev: add stubs for v4l2_subdev_get_try_*
  [media] v4l2-subdev: fix v4l2_subdev_get_try_* dependency
  [media] tvp5150: add FORMAT_TRY support for get/set selection handlers
  [media] tvp5150: add s_power callback

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

 .../devicetree/bindings/media/i2c/tvp5150.txt | 191 +++++-
 drivers/media/i2c/tvp5150.c                   | 611 +++++++++++++++---
 include/media/v4l2-subdev.h                   | 111 ++--
 3 files changed, 776 insertions(+), 137 deletions(-)

-- 
2.18.0

[PATCH v2 1/7] [media] tvp5150: add input source selection of_graph support

[Marco Felsch]

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

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

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

---
Changelog:

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 | 478 +++++++++++++++++++++++++++++++++---
 1 file changed, 441 insertions(+), 37 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index b5d44c25d1da..a9cd79cbd7b0 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -43,16 +43,39 @@ MODULE_PARM_DESC(debug, "Debug level (0-2)");
 
 #define dprintk0(__dev, __arg...) dev_dbg_lvl(__dev, 0, 0, __arg)
 
+#if defined(CONFIG_MEDIA_CONTROLLER)
 enum tvp5150_pads {
-       TVP5150_PAD_IF_INPUT,
-       TVP5150_PAD_VID_OUT,
-       TVP5150_NUM_PADS
+	TVP5150_PAD_AIP1A = TVP5150_COMPOSITE0,
+	TVP5150_PAD_AIP1B,
+	TVP5150_PAD_VID_OUT,
+	TVP5150_NUM_PADS
+};
+
+enum tvp5150_pads_state {
+	TVP5150_PAD_INACTIVE,
+	TVP5150_PAD_ACTIVE_COMPOSITE,
+	TVP5150_PAD_ACTIVE_SVIDEO,
+};
+
+struct tvp5150_connector {
+	struct media_entity ent;
+	struct media_pad pad;
+	unsigned int port_num;
+	bool is_svideo;
 };
+#endif
 
 struct tvp5150 {
 	struct v4l2_subdev sd;
-#ifdef CONFIG_MEDIA_CONTROLLER
+	/* additional additional endpoint for the svideo connector */
+	struct device_node *endpoints[TVP5150_NUM_PADS + 1];
+	unsigned int endpoints_num;
+#if defined(CONFIG_MEDIA_CONTROLLER)
 	struct media_pad pads[TVP5150_NUM_PADS];
+	int pads_state[TVP5150_NUM_PADS];
+	struct tvp5150_connector *connectors;
+	int connectors_num;
+	bool modify_second_link;
 #endif
 	struct v4l2_ctrl_handler hdl;
 	struct v4l2_rect rect;
@@ -1168,6 +1191,160 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
+/****************************************************************************
+ *			Media entity ops
+ ****************************************************************************/
+#if defined(CONFIG_MEDIA_CONTROLLER)
+static int tvp5150_active_pad_idx(struct tvp5150 *decoder)
+{
+	int *pad_state = &decoder->pads_state[0];
+	int i, idx = -1;
+
+	for (i = 0; i < TVP5150_NUM_PADS - 1; i++) {
+		if ((pad_state[i] == TVP5150_PAD_ACTIVE_COMPOSITE) ||
+		    (pad_state[i] == TVP5150_PAD_ACTIVE_SVIDEO)) {
+			idx = i;
+			break;
+		}
+	}
+
+	return idx;
+}
+
+static int tvp5150_active_svideo_links(struct tvp5150 *decoder)
+{
+	int *pad_state = &decoder->pads_state[0];
+	int i, links = 0;
+
+	for (i = 0; i < TVP5150_NUM_PADS - 1; i++)
+		if (pad_state[i] == TVP5150_PAD_ACTIVE_SVIDEO)
+			links++;
+
+	return links;
+}
+
+static int tvp5150_modify_link(struct tvp5150 *decoder, unsigned int pad_idx,
+			       struct media_pad *pad, int flags)
+{
+	struct media_pad *src_pad;
+	struct media_link *link;
+
+	if (pad)
+		src_pad = pad;
+	else
+		src_pad = media_entity_remote_pad(&decoder->pads[pad_idx]);
+
+	if (!src_pad)
+		return -1;
+
+	link = media_entity_find_link(src_pad,
+				      &decoder->pads[pad_idx]);
+
+	/*
+	 * Don't use locked version, since we are running already within the
+	 * media_entity_setup_link() context.
+	 */
+	return __media_entity_setup_link(link, flags);
+
+}
+
+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);
+	int *pad_state = &decoder->pads_state[0];
+	int i, active_pad, ret = 0;
+	bool is_svideo = false;
+
+	/*
+	 * The tvp state is determined by the enabled sink pad link.
+	 * 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].is_svideo;
+			break;
+		}
+	}
+
+	/* check if there is already a enabled svideo link and determine pad */
+	active_pad = tvp5150_active_pad_idx(decoder);
+
+	dev_dbg(sd->dev, "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 (flags & MEDIA_LNK_FL_ENABLED) {
+		if (active_pad >= 0 && !decoder->modify_second_link)
+			tvp5150_modify_link(decoder, active_pad, NULL, 0);
+
+		dev_dbg(sd->dev, "Setting %d active [%s]\n", tvp5150_pad->index,
+			is_svideo ? "svideo" : "composite");
+		pad_state[tvp5150_pad->index] =
+			is_svideo ? TVP5150_PAD_ACTIVE_SVIDEO :
+				    TVP5150_PAD_ACTIVE_COMPOSITE;
+
+		if (is_svideo) {
+			if (tvp5150_active_svideo_links(decoder) < 2) {
+				unsigned int idx = tvp5150_pad->index ^ 1;
+
+				decoder->modify_second_link = true;
+				tvp5150_modify_link(decoder, idx,
+						    (struct media_pad *)remote,
+						    MEDIA_LNK_FL_ENABLED);
+			} else {
+				decoder->modify_second_link = false;
+				tvp5150_s_routing(sd, TVP5150_SVIDEO,
+						  TVP5150_NORMAL, 0);
+			}
+		} else {
+			tvp5150_s_routing(sd, tvp5150_pad->index,
+					  TVP5150_NORMAL, 0);
+		}
+	} else {
+		/*
+		 * Svideo streams on two pads and user can request to AIP1A or
+		 * AIP1B pad. In either case both pads gets disabled in in go.
+		 * So check only if user wants to disable a not enabled
+		 * composite pad.
+		 */
+		if (!is_svideo && tvp5150_pad->index != active_pad)
+			goto out;
+
+		dev_dbg(sd->dev, "going inactive\n");
+		pad_state[tvp5150_pad->index] = TVP5150_PAD_INACTIVE;
+
+		/* in case of svideo we need to disable the second pad too */
+		if (tvp5150_active_svideo_links(decoder) > 0) {
+			unsigned int idx = tvp5150_pad->index ^ 1;
+
+			decoder->modify_second_link = true;
+			tvp5150_modify_link(decoder, idx,
+					    (struct media_pad *)remote, 0);
+		}
+
+		if (!decoder->modify_second_link)
+			tvp5150_s_routing(sd, is_svideo ? TVP5150_SVIDEO :
+					  active_pad, TVP5150_BLACK_SCREEN, 0);
+		decoder->modify_second_link = false;
+	}
+out:
+	return ret;
+}
+
+static const struct media_entity_operations tvp5150_sd_media_ops = {
+	.link_setup = tvp5150_link_setup,
+};
+#endif
 /****************************************************************************
 			I2C Command
  ****************************************************************************/
@@ -1315,6 +1492,76 @@ 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].port_num;
+		bool is_svideo = decoder->connectors[i].is_svideo;
+		int flags = i ? 0 : MEDIA_LNK_FL_ENABLED;
+
+		pad->flags = MEDIA_PAD_FL_SOURCE;
+		ret = media_entity_pads_init(con, 1, pad);
+		if (ret < 0)
+			return ret;
+
+		ret = media_device_register_entity(sd->v4l2_dev->mdev, con);
+		if (ret < 0)
+			return ret;
+
+		ret = media_create_pad_link(con, 0, &sd->entity, port, flags);
+		if (ret < 0) {
+			media_device_unregister_entity(con);
+			return ret;
+		}
+
+		if (is_svideo) {
+			/* svideo links to both aip1a and aip1b */
+			ret = media_create_pad_link(con, 0, &sd->entity,
+						    port + 1, flags);
+			if (ret < 0) {
+				media_device_unregister_entity(con);
+				return ret;
+			}
+		}
+
+		/* enable default input */
+		if (flags == MEDIA_LNK_FL_ENABLED) {
+			if (is_svideo) {
+				decoder->pads_state[TVP5150_PAD_AIP1A] =
+				decoder->pads_state[TVP5150_PAD_AIP1B] =
+						TVP5150_PAD_ACTIVE_SVIDEO;
+				decoder->input = TVP5150_SVIDEO;
+			} else {
+				if (port == 0) {
+					decoder->pads_state[TVP5150_PAD_AIP1A] =
+						TVP5150_PAD_ACTIVE_COMPOSITE;
+					decoder->input = TVP5150_COMPOSITE0;
+				} else {
+					decoder->pads_state[TVP5150_PAD_AIP1B] =
+						TVP5150_PAD_ACTIVE_COMPOSITE;
+					decoder->input = TVP5150_COMPOSITE1;
+				}
+			}
+			tvp5150_selmux(sd);
+			decoder->modify_second_link = false;
+		}
+	}
+#endif
+	return 0;
+}
+
+
 /* ----------------------------------------------------------------------- */
 
 static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
@@ -1368,6 +1615,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
  ****************************************************************************/
@@ -1516,38 +1767,197 @@ static int tvp5150_init(struct i2c_client *c)
 	return 0;
 }
 
-static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
+#if defined(CONFIG_MEDIA_CONTROLLER)
+static int tvp5150_add_of_connectors(struct tvp5150 *decoder)
 {
-	struct v4l2_fwnode_endpoint bus_cfg;
-	struct device_node *ep;
-	unsigned int flags;
-	int ret = 0;
+	struct device *dev = decoder->sd.dev;
+	struct device_node *rp;
+	struct of_endpoint ep;
+	struct tvp5150_connector *connectors;
+	unsigned int connectors_num = decoder->connectors_num;
+	int i, ret;
+
+	/* Allocate and initialize all available input connectors */
+	connectors = devm_kcalloc(dev, connectors_num, sizeof(*connectors),
+				  GFP_KERNEL);
+	if (!connectors)
+		return -ENOMEM;
 
-	ep = of_graph_get_next_endpoint(np, NULL);
-	if (!ep)
-		return -EINVAL;
+	for (i = 0; i < connectors_num; i++) {
+		rp = of_graph_get_remote_port_parent(decoder->endpoints[i]);
+		of_graph_parse_endpoint(decoder->endpoints[i], &ep);
+		connectors[i].port_num = ep.port;
+		connectors[i].is_svideo = !!of_device_is_compatible(rp,
+							    "svideo-connector");
 
-	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &bus_cfg);
-	if (ret)
-		goto err;
+		if (connectors[i].is_svideo)
+			connectors[i].ent.function = MEDIA_ENT_F_CONN_SVIDEO;
+		else
+			connectors[i].ent.function = MEDIA_ENT_F_CONN_COMPOSITE;
+
+		connectors[i].ent.flags = MEDIA_ENT_FL_CONNECTOR;
+		ret = of_property_read_string(rp, "label",
+					      &connectors[i].ent.name);
+		if (ret < 0)
+			return ret;
+	}
 
-	flags = bus_cfg.bus.parallel.flags;
+	decoder->connectors = connectors;
+
+	return 0;
+}
+#endif
+
+static int tvp5150_mc_init(struct v4l2_subdev *sd)
+{
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	struct tvp5150 *decoder = to_tvp5150(sd);
+	unsigned int i;
+	int ret;
+
+	sd->entity.ops = &tvp5150_sd_media_ops;
+	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
+
+	/* Initialize all TVP5150 pads */
+	for (i = 0; i < TVP5150_NUM_PADS; i++) {
+		if (i < TVP5150_NUM_PADS - 1) {
+			decoder->pads[i].flags = MEDIA_PAD_FL_SINK;
+			decoder->pads[i].sig_type = PAD_SIGNAL_ANALOG;
+		} else {
+			decoder->pads[i].flags = MEDIA_PAD_FL_SOURCE;
+			decoder->pads[i].sig_type = PAD_SIGNAL_DV;
+		}
+	}
+	ret = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS,
+				     decoder->pads);
+	if (ret < 0)
+		goto out;
+
+	if (IS_ENABLED(CONFIG_OF))
+		ret = tvp5150_add_of_connectors(decoder);
+#endif
+out:
+	return ret;
+}
+
+static bool tvp5150_of_valid_input(struct device_node *endpoint,
+				unsigned int port, unsigned int id)
+{
+	struct device_node *rp = of_graph_get_remote_port_parent(endpoint);
+	const char *input;
+	int ret;
+
+	/* perform some basic checks needed for later mc_init */
+	switch (port) {
+	case TVP5150_PAD_AIP1A:
+		/* svideo must be connected to endpoint@1  */
+		ret = id ? of_device_is_compatible(rp, "svideo-connector") :
+			   of_device_is_compatible(rp,
+						   "composite-video-connector");
+		if (!ret)
+			return false;
+		break;
+	case TVP5150_PAD_AIP1B:
+		ret = of_device_is_compatible(rp, "composite-video-connector");
+		if (!ret)
+			return false;
+		break;
+	}
 
-	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 = of_property_read_string(rp, "label", &input);
+	if (ret < 0)
+		return false;
+
+	return true;
+}
+
+static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
+{
+	struct device *dev = decoder->sd.dev;
+	struct v4l2_fwnode_endpoint bus_cfg;
+	struct device_node *ep_np;
+	unsigned int flags;
+	int ret, i = 0, in = 0;
+	bool found = false;
+
+	/* at least 1 output and 1 input */
+	decoder->endpoints_num = of_graph_get_endpoint_count(np);
+	if (decoder->endpoints_num < 2 || decoder->endpoints_num > 4) {
 		ret = -EINVAL;
 		goto err;
 	}
 
-	decoder->mbus_type = bus_cfg.bus_type;
+	for_each_endpoint_of_node(np, ep_np) {
+		struct of_endpoint ep;
+
+		of_graph_parse_endpoint(ep_np, &ep);
+		if (decoder->endpoints[i]) {
+			/* this should never happen */
+			dev_err(dev, "Invalid endpoint %pOF on port %d\n",
+				ep.local_node, ep.port);
+				ret = -EINVAL;
+				goto err;
+		}
+
+		switch (ep.port) {
+			/* fall through */
+		case TVP5150_PAD_AIP1A:
+		case TVP5150_PAD_AIP1B:
+			if (!tvp5150_of_valid_input(ep_np, ep.port, ep.id)) {
+				dev_err(dev,
+					"Invalid endpoint %pOF on port %d\n",
+					ep.local_node, ep.port);
+				ret = -EINVAL;
+				goto err;
+			}
+			in++;
+			break;
+		case TVP5150_PAD_VID_OUT:
+			ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_np),
+							 &bus_cfg);
+			if (ret)
+				goto err;
+
+			flags = bus_cfg.bus.parallel.flags;
+
+			if (bus_cfg.bus_type == V4L2_MBUS_PARALLEL &&
+			    !(flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH &&
+			      flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH &&
+			      flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
+				ret = -EINVAL;
+				goto err;
+			}
+
+			decoder->mbus_type = bus_cfg.bus_type;
+			break;
+		default:
+			dev_err(dev, "Invalid port %d for endpoint %pOF\n",
+				ep.port, ep.local_node);
+			ret = -EINVAL;
+			goto err;
+		}
+
+		of_node_get(ep_np);
+		decoder->endpoints[i] = ep_np;
+		i++;
+
+		found = true;
+	}
 
+	decoder->connectors_num = in;
+	return found ? 0 : -ENODEV;
 err:
-	of_node_put(ep);
 	return ret;
 }
 
+static void tvp5150_dt_cleanup(struct tvp5150 *decoder)
+{
+	unsigned int i;
+
+	for (i = 0; i < TVP5150_NUM_PADS; i++)
+		of_node_put(decoder->endpoints[i]);
+}
+
 static const char * const tvp5150_test_patterns[2] = {
 	"Disabled",
 	"Black screen"
@@ -1586,7 +1996,7 @@ static int tvp5150_probe(struct i2c_client *c,
 		res = tvp5150_parse_dt(core, np);
 		if (res) {
 			dev_err(sd->dev, "DT parsing error: %d\n", res);
-			return res;
+			goto err_cleanup_dt;
 		}
 	} else {
 		/* Default to BT.656 embedded sync */
@@ -1594,25 +2004,16 @@ static int tvp5150_probe(struct i2c_client *c,
 	}
 
 	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
+	sd->internal_ops = &tvp5150_internal_ops;
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	core->pads[TVP5150_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
-	core->pads[TVP5150_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
-	core->pads[TVP5150_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
-	core->pads[TVP5150_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
-
-	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
-
-	res = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS, core->pads);
-	if (res < 0)
-		return res;
-
-#endif
+	res = tvp5150_mc_init(sd);
+	if (res)
+		goto err_cleanup_dt;
 
 	res = tvp5150_detect_version(core);
 	if (res < 0)
-		return res;
+		goto err_cleanup_dt;
 
 	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
 	core->detected_norm = V4L2_STD_UNKNOWN;
@@ -1664,6 +2065,9 @@ static int tvp5150_probe(struct i2c_client *c,
 err:
 	v4l2_ctrl_handler_free(&core->hdl);
 	return res;
+err_cleanup_dt:
+	tvp5150_dt_cleanup(core);
+	return res;
 }
 
 static int tvp5150_remove(struct i2c_client *c)
-- 
2.18.0

Re: [PATCH v2 1/7] [media] tvp5150: add input source selection of_graph support

[Sakari Ailus]

Hi Marco,

On Mon, Aug 13, 2018 at 11:25:02AM +0200, Marco Felsch wrote:
...
> +static void tvp5150_dt_cleanup(struct tvp5150 *decoder)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < TVP5150_NUM_PADS; i++)
> +		of_node_put(decoder->endpoints[i]);
> +}
> +
>  static const char * const tvp5150_test_patterns[2] = {
>  	"Disabled",
>  	"Black screen"
> @@ -1586,7 +1996,7 @@ static int tvp5150_probe(struct i2c_client *c,
>  		res = tvp5150_parse_dt(core, np);
>  		if (res) {
>  			dev_err(sd->dev, "DT parsing error: %d\n", res);
> -			return res;
> +			goto err_cleanup_dt;
>  		}
>  	} else {
>  		/* Default to BT.656 embedded sync */
> @@ -1594,25 +2004,16 @@ static int tvp5150_probe(struct i2c_client *c,
>  	}
>  
>  	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
> +	sd->internal_ops = &tvp5150_internal_ops;
>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	core->pads[TVP5150_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
> -	core->pads[TVP5150_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
> -	core->pads[TVP5150_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
> -	core->pads[TVP5150_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
> -
> -	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> -
> -	res = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS, core->pads);
> -	if (res < 0)
> -		return res;
> -
> -#endif
> +	res = tvp5150_mc_init(sd);
> +	if (res)
> +		goto err_cleanup_dt;
>  
>  	res = tvp5150_detect_version(core);
>  	if (res < 0)
> -		return res;
> +		goto err_cleanup_dt;
>  
>  	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
>  	core->detected_norm = V4L2_STD_UNKNOWN;
> @@ -1664,6 +2065,9 @@ static int tvp5150_probe(struct i2c_client *c,
>  err:

Now that you have more error labels, you could rename this one.

>  	v4l2_ctrl_handler_free(&core->hdl);
>  	return res;

Is the above line intended to be kept?

> +err_cleanup_dt:
> +	tvp5150_dt_cleanup(core);
> +	return res;
>  }
>  
>  static int tvp5150_remove(struct i2c_client *c)

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 1/7] [media] tvp5150: add input source selection of_graph support

[Marco Felsch]

Hi Sakari,

thanks for the review.

On 18-09-14 16:31, Sakari Ailus wrote:
> Hi Marco,
> 
> On Mon, Aug 13, 2018 at 11:25:02AM +0200, Marco Felsch wrote:
> ...
> > +static void tvp5150_dt_cleanup(struct tvp5150 *decoder)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < TVP5150_NUM_PADS; i++)
> > +		of_node_put(decoder->endpoints[i]);
> > +}
> > +
> >  static const char * const tvp5150_test_patterns[2] = {
> >  	"Disabled",
> >  	"Black screen"
> > @@ -1586,7 +1996,7 @@ static int tvp5150_probe(struct i2c_client *c,
> >  		res = tvp5150_parse_dt(core, np);
> >  		if (res) {
> >  			dev_err(sd->dev, "DT parsing error: %d\n", res);
> > -			return res;
> > +			goto err_cleanup_dt;
> >  		}
> >  	} else {
> >  		/* Default to BT.656 embedded sync */
> > @@ -1594,25 +2004,16 @@ static int tvp5150_probe(struct i2c_client *c,
> >  	}
> >  
> >  	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
> > +	sd->internal_ops = &tvp5150_internal_ops;
> >  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >  
> > -#if defined(CONFIG_MEDIA_CONTROLLER)
> > -	core->pads[TVP5150_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
> > -	core->pads[TVP5150_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
> > -	core->pads[TVP5150_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
> > -	core->pads[TVP5150_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
> > -
> > -	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
> > -
> > -	res = media_entity_pads_init(&sd->entity, TVP5150_NUM_PADS, core->pads);
> > -	if (res < 0)
> > -		return res;
> > -
> > -#endif
> > +	res = tvp5150_mc_init(sd);
> > +	if (res)
> > +		goto err_cleanup_dt;
> >  
> >  	res = tvp5150_detect_version(core);
> >  	if (res < 0)
> > -		return res;
> > +		goto err_cleanup_dt;
> >  
> >  	core->norm = V4L2_STD_ALL;	/* Default is autodetect */
> >  	core->detected_norm = V4L2_STD_UNKNOWN;
> > @@ -1664,6 +2065,9 @@ static int tvp5150_probe(struct i2c_client *c,
> >  err:
> 
> Now that you have more error labels, you could rename this one.

Hm.. okay make sense, I will change this.

> 
> >  	v4l2_ctrl_handler_free(&core->hdl);
> >  	return res;
> 
> Is the above line intended to be kept?

Nope, sorry I will fix this.

Regards,
Marco

> 
> > +err_cleanup_dt:
> > +	tvp5150_dt_cleanup(core);
> > +	return res;
> >  }
> >  
> >  static int tvp5150_remove(struct i2c_client *c)
> 
> -- 
> Sakari Ailus
> sakari.ailus@linux.intel.com
> 

[PATCH v2 2/7] [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:
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 | 191 +++++++++++++++++-
 1 file changed, 185 insertions(+), 6 deletions(-)

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

Re: [PATCH v2 2/7] [media] dt-bindings: tvp5150: Add input port connectors DT bindings

[Rob Herring]

On Mon, Aug 13, 2018 at 11:25:03AM +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:
> 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 | 191 +++++++++++++++++-
>  1 file changed, 185 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> index 8c0fc1a26bf0..d647d671f14a 100644
> --- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> @@ -12,11 +12,31 @@ Optional Properties:
>  - pdn-gpios: phandle for the GPIO connected to the PDN pin, if any.
>  - reset-gpios: phandle for the GPIO connected to the RESETB pin, if any.
>  
> -The device node must contain one 'port' child node for its digital output
> -video port, in accordance with the video interface bindings defined in
> -Documentation/devicetree/bindings/media/video-interfaces.txt.
> +The device node must contain one 'port' child node per device physical input
> +and output port, in accordance with the video interface bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt. The port nodes
> +are numbered as follows
>  
> -Required Endpoint Properties for parallel synchronization:
> +	  Name		Type		Port
> +	--------------------------------------
> +	  AIP1A		sink		0
> +	  AIP1B		sink		1
> +	  Y-OUT		src		2
> +
> +The device node must contain at least one sink port and the src port. Each input
> +port must be linked to an endpoint defined in
> +Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt. The
> +port/connector layout is as follows
> +
> +tvp-5150 port@0 (AIP1A)
> +	endpoint@0 -----------> Comp0-Con  port
> +	endpoint@1 -----------> Svideo-Con port
> +tvp-5150 port@1 (AIP1B)
> +	endpoint   -----------> Comp1-Con  port
> +tvp-5150 port@2
> +	endpoint (video bitstream output at YOUT[0-7] parallel bus)
> +
> +Required Endpoint Properties for parallel synchronization on output port:
>  
>  - hsync-active: active state of the HSYNC signal. Must be <1> (HIGH).
>  - vsync-active: active state of the VSYNC signal. Must be <1> (HIGH).
> @@ -26,7 +46,140 @@ 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:
> +Examples:

Is it really necessary to enumerate every possibility? Just show the 
most complicated case which is a superset of the rest.

> +
> +One Input:
> +
> +connector {
> +	compatible = "composite-video-connector";
> +	label = "Composite0";
> +
> +	port {
> +		composite0_to_tvp5150: endpoint {
> +			remote-endpoint = <&tvp5150_to_composite0>;
> +		};
> +	};
> +};
> +
> +&i2c2 {
> +	...
> +	tvp5150@5c {
> +		compatible = "ti,tvp5150";
> +		reg = <0x5c>;
> +		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
> +		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
> +
> +		port@0 {
> +			reg = <0>;
> +
> +			tvp5150_to_composite0: endpoint {
> +				remote-endpoint = <&composite0_to_tvp5150>;
> +			};
> +		};
> +
> +		port@2 {
> +			reg = <2>;
> +
> +			tvp5150_1: endpoint {
> +				remote-endpoint = <&ccdc_ep>;
> +			};
> +		};
> +	};
> +};
> +
> +Two Inputs:
> +
> +comp_connector_1 {
> +	compatible = "composite-video-connector";
> +	label = "Composite1";
> +
> +	port {
> +		composite1_to_tvp5150: endpoint {
> +			remote-endpoint = <&tvp5150_to_composite1>;
> +		};
> +	};
> +};
> +
> +svid_connector {
> +	compatible = "svideo-connector";
> +	label = "S-Video";
> +
> +	port {
> +		svideo_to_tvp5150: endpoint {
> +			remote-endpoint = <&tvp5150_to_svideo>;
> +		};
> +	};
> +};
> +
> +&i2c2 {
> +	...
> +	tvp5150@5c {
> +		compatible = "ti,tvp5150";
> +		reg = <0x5c>;
> +		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
> +		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
> +
> +                port@0 {
> +                        reg = <0>;
> +
> +                        tvp5150_to_svideo: endpoint@1 {
> +                                reg = <1>;
> +                                remote-endpoint = <&svideo_to_tvp5150>;
> +                        };
> +                };
> +
> +                port@1 {
> +                        reg = <1>;
> +
> +                        tvp5150_to_composite1: endpoint {
> +                                remote-endpoint = <&composite1_to_tvp5150>;
> +                        };
> +                };

Spaces used instead of tabs.

> +
> +		port@2 {
> +			reg = <2>;
> +
> +			tvp5150_1: endpoint {
> +				remote-endpoint = <&ccdc_ep>;
> +			};
> +		};
> +	};
> +};
> +
> +Three Inputs:
> +
> +comp_connector_0 {
> +	compatible = "composite-video-connector";
> +	label = "Composite0";
> +
> +	port {
> +		composite0_to_tvp5150: endpoint {
> +			remote-endpoint = <&tvp5150_to_composite0>;
> +		};
> +	};
> +};
> +
> +comp_connector_1 {
> +	compatible = "composite-video-connector";
> +	label = "Composite1";
> +
> +	port {
> +		composite1_to_tvp5150: endpoint {
> +			remote-endpoint = <&tvp5150_to_composite1>;
> +		};
> +	};
> +};
> +
> +svid_connector {
> +	compatible = "svideo-connector";
> +	label = "S-Video";
> +
> +	port {
> +		svideo_to_tvp5150: endpoint {
> +			remote-endpoint = <&tvp5150_to_svideo>;
> +		};
> +	};
> +};
>  
>  &i2c2 {
>  	...
> @@ -36,7 +189,33 @@ Example:
>  		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
>  		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
>  
> -		port {
> +                port@0 {
> +                        #address-cells = <1>;
> +                        #size-cells = <0>;
> +                        reg = <0>;
> +
> +                        tvp5150_to_composite0: endpoint@0 {
> +                                reg = <0>;
> +                                remote-endpoint = <&composite0_to_tvp5150>;
> +                        };
> +
> +                        tvp5150_to_svideo: endpoint@1 {
> +                                reg = <1>;
> +                                remote-endpoint = <&svideo_to_tvp5150>;
> +                        };
> +                };
> +
> +                port@1 {
> +                        reg = <1>;
> +
> +                        tvp5150_to_composite1: endpoint {
> +                                remote-endpoint = <&composite1_to_tvp5150>;
> +                        };
> +                };

Here too.

> +
> +		port@2 {
> +			reg = <2>;
> +
>  			tvp5150_1: endpoint {
>  				remote-endpoint = <&ccdc_ep>;
>  			};
> -- 
> 2.18.0
> 

Re: [PATCH v2 2/7] [media] dt-bindings: tvp5150: Add input port connectors DT bindings

[Marco Felsch]

On 18-08-13 15:41, Rob Herring wrote:
> On Mon, Aug 13, 2018 at 11:25:03AM +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:
> > 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 | 191 +++++++++++++++++-
> >  1 file changed, 185 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> > index 8c0fc1a26bf0..d647d671f14a 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> > @@ -12,11 +12,31 @@ Optional Properties:
> >  - pdn-gpios: phandle for the GPIO connected to the PDN pin, if any.
> >  - reset-gpios: phandle for the GPIO connected to the RESETB pin, if any.
> >  
> > -The device node must contain one 'port' child node for its digital output
> > -video port, in accordance with the video interface bindings defined in
> > -Documentation/devicetree/bindings/media/video-interfaces.txt.
> > +The device node must contain one 'port' child node per device physical input
> > +and output port, in accordance with the video interface bindings defined in
> > +Documentation/devicetree/bindings/media/video-interfaces.txt. The port nodes
> > +are numbered as follows
> >  
> > -Required Endpoint Properties for parallel synchronization:
> > +	  Name		Type		Port
> > +	--------------------------------------
> > +	  AIP1A		sink		0
> > +	  AIP1B		sink		1
> > +	  Y-OUT		src		2
> > +
> > +The device node must contain at least one sink port and the src port. Each input
> > +port must be linked to an endpoint defined in
> > +Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt. The
> > +port/connector layout is as follows
> > +
> > +tvp-5150 port@0 (AIP1A)
> > +	endpoint@0 -----------> Comp0-Con  port
> > +	endpoint@1 -----------> Svideo-Con port
> > +tvp-5150 port@1 (AIP1B)
> > +	endpoint   -----------> Comp1-Con  port
> > +tvp-5150 port@2
> > +	endpoint (video bitstream output at YOUT[0-7] parallel bus)
> > +
> > +Required Endpoint Properties for parallel synchronization on output port:
> >  
> >  - hsync-active: active state of the HSYNC signal. Must be <1> (HIGH).
> >  - vsync-active: active state of the VSYNC signal. Must be <1> (HIGH).
> > @@ -26,7 +46,140 @@ 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:
> > +Examples:
> 
> Is it really necessary to enumerate every possibility? Just show the 
> most complicated case which is a superset of the rest.

I just wanted to be a bit more verbose, since not all users (e.g.
beginners) know how the of_graph works. Anyway, I can drop the 1st and
2nd example.

> 
> > +
> > +One Input:
> > +
> > +connector {
> > +	compatible = "composite-video-connector";
> > +	label = "Composite0";
> > +
> > +	port {
> > +		composite0_to_tvp5150: endpoint {
> > +			remote-endpoint = <&tvp5150_to_composite0>;
> > +		};
> > +	};
> > +};
> > +
> > +&i2c2 {
> > +	...
> > +	tvp5150@5c {
> > +		compatible = "ti,tvp5150";
> > +		reg = <0x5c>;
> > +		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
> > +		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
> > +
> > +		port@0 {
> > +			reg = <0>;
> > +
> > +			tvp5150_to_composite0: endpoint {
> > +				remote-endpoint = <&composite0_to_tvp5150>;
> > +			};
> > +		};
> > +
> > +		port@2 {
> > +			reg = <2>;
> > +
> > +			tvp5150_1: endpoint {
> > +				remote-endpoint = <&ccdc_ep>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +Two Inputs:
> > +
> > +comp_connector_1 {
> > +	compatible = "composite-video-connector";
> > +	label = "Composite1";
> > +
> > +	port {
> > +		composite1_to_tvp5150: endpoint {
> > +			remote-endpoint = <&tvp5150_to_composite1>;
> > +		};
> > +	};
> > +};
> > +
> > +svid_connector {
> > +	compatible = "svideo-connector";
> > +	label = "S-Video";
> > +
> > +	port {
> > +		svideo_to_tvp5150: endpoint {
> > +			remote-endpoint = <&tvp5150_to_svideo>;
> > +		};
> > +	};
> > +};
> > +
> > +&i2c2 {
> > +	...
> > +	tvp5150@5c {
> > +		compatible = "ti,tvp5150";
> > +		reg = <0x5c>;
> > +		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
> > +		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
> > +
> > +                port@0 {
> > +                        reg = <0>;
> > +
> > +                        tvp5150_to_svideo: endpoint@1 {
> > +                                reg = <1>;
> > +                                remote-endpoint = <&svideo_to_tvp5150>;
> > +                        };
> > +                };
> > +
> > +                port@1 {
> > +                        reg = <1>;
> > +
> > +                        tvp5150_to_composite1: endpoint {
> > +                                remote-endpoint = <&composite1_to_tvp5150>;
> > +                        };
> > +                };
> 
> Spaces used instead of tabs.

Sorry for that. I checked it by checkpatch with no errors.
I will convert it.

> 
> > +
> > +		port@2 {
> > +			reg = <2>;
> > +
> > +			tvp5150_1: endpoint {
> > +				remote-endpoint = <&ccdc_ep>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +Three Inputs:
> > +
> > +comp_connector_0 {
> > +	compatible = "composite-video-connector";
> > +	label = "Composite0";
> > +
> > +	port {
> > +		composite0_to_tvp5150: endpoint {
> > +			remote-endpoint = <&tvp5150_to_composite0>;
> > +		};
> > +	};
> > +};
> > +
> > +comp_connector_1 {
> > +	compatible = "composite-video-connector";
> > +	label = "Composite1";
> > +
> > +	port {
> > +		composite1_to_tvp5150: endpoint {
> > +			remote-endpoint = <&tvp5150_to_composite1>;
> > +		};
> > +	};
> > +};
> > +
> > +svid_connector {
> > +	compatible = "svideo-connector";
> > +	label = "S-Video";
> > +
> > +	port {
> > +		svideo_to_tvp5150: endpoint {
> > +			remote-endpoint = <&tvp5150_to_svideo>;
> > +		};
> > +	};
> > +};
> >  
> >  &i2c2 {
> >  	...
> > @@ -36,7 +189,33 @@ Example:
> >  		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
> >  		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
> >  
> > -		port {
> > +                port@0 {
> > +                        #address-cells = <1>;
> > +                        #size-cells = <0>;
> > +                        reg = <0>;
> > +
> > +                        tvp5150_to_composite0: endpoint@0 {
> > +                                reg = <0>;
> > +                                remote-endpoint = <&composite0_to_tvp5150>;
> > +                        };
> > +
> > +                        tvp5150_to_svideo: endpoint@1 {
> > +                                reg = <1>;
> > +                                remote-endpoint = <&svideo_to_tvp5150>;
> > +                        };
> > +                };
> > +
> > +                port@1 {
> > +                        reg = <1>;
> > +
> > +                        tvp5150_to_composite1: endpoint {
> > +                                remote-endpoint = <&composite1_to_tvp5150>;
> > +                        };
> > +                };
> 
> Here too.
> 
> > +
> > +		port@2 {
> > +			reg = <2>;
> > +
> >  			tvp5150_1: endpoint {
> >  				remote-endpoint = <&ccdc_ep>;
> >  			};
> > -- 
> > 2.18.0
> > 
> 

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

[PATCH v2 3/7] [media] v4l2-subdev: add stubs for v4l2_subdev_get_try_*

[Marco Felsch]

In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't
available. So each driver have to add ifdefs around those helpers or
add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy.

Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API
isn't set to avoid ifdefs. This approach is less error prone too.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 include/media/v4l2-subdev.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 9102d6ca566e..ce48f1fcf295 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -912,8 +912,6 @@ struct v4l2_subdev_fh {
 #define to_v4l2_subdev_fh(fh)	\
 	container_of(fh, struct v4l2_subdev_fh, vfh)
 
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-
 /**
  * v4l2_subdev_get_try_format - ancillary routine to call
  *	&struct v4l2_subdev_pad_config->try_fmt
@@ -927,9 +925,13 @@ static inline struct v4l2_mbus_framefmt
 			    struct v4l2_subdev_pad_config *cfg,
 			    unsigned int pad)
 {
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	if (WARN_ON(pad >= sd->entity.num_pads))
 		pad = 0;
 	return &cfg[pad].try_fmt;
+#else
+	return NULL;
+#endif
 }
 
 /**
@@ -945,9 +947,13 @@ static inline struct v4l2_rect
 			  struct v4l2_subdev_pad_config *cfg,
 			  unsigned int pad)
 {
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	if (WARN_ON(pad >= sd->entity.num_pads))
 		pad = 0;
 	return &cfg[pad].try_crop;
+#else
+	return NULL;
+#endif
 }
 
 /**
@@ -963,11 +969,14 @@ static inline struct v4l2_rect
 			     struct v4l2_subdev_pad_config *cfg,
 			     unsigned int pad)
 {
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	if (WARN_ON(pad >= sd->entity.num_pads))
 		pad = 0;
 	return &cfg[pad].try_compose;
-}
+#else
+	return NULL;
 #endif
+}
 
 extern const struct v4l2_file_operations v4l2_subdev_fops;
 
-- 
2.18.0

[PATCH v2 4/7] [media] v4l2-subdev: fix v4l2_subdev_get_try_* dependency

[Marco Felsch]

These helpers make us of the media-controller entity which is only
available if the CONFIG_MEDIA_CONTROLLER is enabled.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 include/media/v4l2-subdev.h | 100 ++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 50 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index ce48f1fcf295..79c066934ad2 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -912,6 +912,56 @@ struct v4l2_subdev_fh {
 #define to_v4l2_subdev_fh(fh)	\
 	container_of(fh, struct v4l2_subdev_fh, vfh)
 
+extern const struct v4l2_file_operations v4l2_subdev_fops;
+
+/**
+ * v4l2_set_subdevdata - Sets V4L2 dev private device data
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ * @p: pointer to the private device data to be stored.
+ */
+static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
+{
+	sd->dev_priv = p;
+}
+
+/**
+ * v4l2_get_subdevdata - Gets V4L2 dev private device data
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ *
+ * Returns the pointer to the private device data to be stored.
+ */
+static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
+{
+	return sd->dev_priv;
+}
+
+/**
+ * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ * @p: pointer to the private data to be stored.
+ */
+static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void *p)
+{
+	sd->host_priv = p;
+}
+
+/**
+ * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ *
+ * Returns the pointer to the private host data to be stored.
+ */
+static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
+{
+	return sd->host_priv;
+}
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+
 /**
  * v4l2_subdev_get_try_format - ancillary routine to call
  *	&struct v4l2_subdev_pad_config->try_fmt
@@ -978,56 +1028,6 @@ static inline struct v4l2_rect
 #endif
 }
 
-extern const struct v4l2_file_operations v4l2_subdev_fops;
-
-/**
- * v4l2_set_subdevdata - Sets V4L2 dev private device data
- *
- * @sd: pointer to &struct v4l2_subdev
- * @p: pointer to the private device data to be stored.
- */
-static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
-{
-	sd->dev_priv = p;
-}
-
-/**
- * v4l2_get_subdevdata - Gets V4L2 dev private device data
- *
- * @sd: pointer to &struct v4l2_subdev
- *
- * Returns the pointer to the private device data to be stored.
- */
-static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
-{
-	return sd->dev_priv;
-}
-
-/**
- * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
- *
- * @sd: pointer to &struct v4l2_subdev
- * @p: pointer to the private data to be stored.
- */
-static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void *p)
-{
-	sd->host_priv = p;
-}
-
-/**
- * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
- *
- * @sd: pointer to &struct v4l2_subdev
- *
- * Returns the pointer to the private host data to be stored.
- */
-static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
-{
-	return sd->host_priv;
-}
-
-#ifdef CONFIG_MEDIA_CONTROLLER
-
 /**
  * v4l2_subdev_link_validate_default - validates a media link
  *
-- 
2.18.0

Re: [PATCH v2 4/7] [media] v4l2-subdev: fix v4l2_subdev_get_try_* dependency

[Sakari Ailus]

Hi Marco,

On Mon, Aug 13, 2018 at 11:25:05AM +0200, Marco Felsch wrote:
> These helpers make us of the media-controller entity which is only
> available if the CONFIG_MEDIA_CONTROLLER is enabled.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  include/media/v4l2-subdev.h | 100 ++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 50 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index ce48f1fcf295..79c066934ad2 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -912,6 +912,56 @@ struct v4l2_subdev_fh {
>  #define to_v4l2_subdev_fh(fh)	\
>  	container_of(fh, struct v4l2_subdev_fh, vfh)
>  
> +extern const struct v4l2_file_operations v4l2_subdev_fops;
> +
> +/**
> + * v4l2_set_subdevdata - Sets V4L2 dev private device data
> + *
> + * @sd: pointer to &struct v4l2_subdev
> + * @p: pointer to the private device data to be stored.
> + */
> +static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
> +{
> +	sd->dev_priv = p;
> +}
> +
> +/**
> + * v4l2_get_subdevdata - Gets V4L2 dev private device data
> + *
> + * @sd: pointer to &struct v4l2_subdev
> + *
> + * Returns the pointer to the private device data to be stored.
> + */
> +static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
> +{
> +	return sd->dev_priv;
> +}
> +
> +/**
> + * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
> + *
> + * @sd: pointer to &struct v4l2_subdev
> + * @p: pointer to the private data to be stored.
> + */
> +static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void *p)
> +{
> +	sd->host_priv = p;
> +}
> +
> +/**
> + * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
> + *
> + * @sd: pointer to &struct v4l2_subdev
> + *
> + * Returns the pointer to the private host data to be stored.
> + */
> +static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
> +{
> +	return sd->host_priv;
> +}

Could you leave the functions dealing with host_priv where they are? I'd
like to avoid expanding their use; rather reduce it. The field is
problematic in some cases and generally not really needed either.

> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +
>  /**
>   * v4l2_subdev_get_try_format - ancillary routine to call
>   *	&struct v4l2_subdev_pad_config->try_fmt
> @@ -978,56 +1028,6 @@ static inline struct v4l2_rect
>  #endif
>  }
>  
> -extern const struct v4l2_file_operations v4l2_subdev_fops;
> -
> -/**
> - * v4l2_set_subdevdata - Sets V4L2 dev private device data
> - *
> - * @sd: pointer to &struct v4l2_subdev
> - * @p: pointer to the private device data to be stored.
> - */
> -static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
> -{
> -	sd->dev_priv = p;
> -}
> -
> -/**
> - * v4l2_get_subdevdata - Gets V4L2 dev private device data
> - *
> - * @sd: pointer to &struct v4l2_subdev
> - *
> - * Returns the pointer to the private device data to be stored.
> - */
> -static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
> -{
> -	return sd->dev_priv;
> -}
> -
> -/**
> - * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
> - *
> - * @sd: pointer to &struct v4l2_subdev
> - * @p: pointer to the private data to be stored.
> - */
> -static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void *p)
> -{
> -	sd->host_priv = p;
> -}
> -
> -/**
> - * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
> - *
> - * @sd: pointer to &struct v4l2_subdev
> - *
> - * Returns the pointer to the private host data to be stored.
> - */
> -static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
> -{
> -	return sd->host_priv;
> -}
> -
> -#ifdef CONFIG_MEDIA_CONTROLLER
> -
>  /**
>   * v4l2_subdev_link_validate_default - validates a media link
>   *

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 4/7] [media] v4l2-subdev: fix v4l2_subdev_get_try_* dependency

[Marco Felsch]

Hi Sakari,

On 18-09-14 16:25, Sakari Ailus wrote:
> Hi Marco,
> 
> On Mon, Aug 13, 2018 at 11:25:05AM +0200, Marco Felsch wrote:
> > These helpers make us of the media-controller entity which is only
> > available if the CONFIG_MEDIA_CONTROLLER is enabled.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  include/media/v4l2-subdev.h | 100 ++++++++++++++++++------------------
> >  1 file changed, 50 insertions(+), 50 deletions(-)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index ce48f1fcf295..79c066934ad2 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -912,6 +912,56 @@ struct v4l2_subdev_fh {
> >  #define to_v4l2_subdev_fh(fh)	\
> >  	container_of(fh, struct v4l2_subdev_fh, vfh)
> >  
> > +extern const struct v4l2_file_operations v4l2_subdev_fops;
> > +
> > +/**
> > + * v4l2_set_subdevdata - Sets V4L2 dev private device data
> > + *
> > + * @sd: pointer to &struct v4l2_subdev
> > + * @p: pointer to the private device data to be stored.
> > + */
> > +static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
> > +{
> > +	sd->dev_priv = p;
> > +}
> > +
> > +/**
> > + * v4l2_get_subdevdata - Gets V4L2 dev private device data
> > + *
> > + * @sd: pointer to &struct v4l2_subdev
> > + *
> > + * Returns the pointer to the private device data to be stored.
> > + */
> > +static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
> > +{
> > +	return sd->dev_priv;
> > +}
> > +
> > +/**
> > + * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
> > + *
> > + * @sd: pointer to &struct v4l2_subdev
> > + * @p: pointer to the private data to be stored.
> > + */
> > +static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void *p)
> > +{
> > +	sd->host_priv = p;
> > +}
> > +
> > +/**
> > + * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
> > + *
> > + * @sd: pointer to &struct v4l2_subdev
> > + *
> > + * Returns the pointer to the private host data to be stored.
> > + */
> > +static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
> > +{
> > +	return sd->host_priv;
> > +}
> 
> Could you leave the functions dealing with host_priv where they are? I'd
> like to avoid expanding their use; rather reduce it. The field is
> problematic in some cases and generally not really needed either.

Sure, I just moved the v4l2_subdev_get_try_* formats to the
CONFIG_MEDIA_CONTROLLER ifdef block to avoid a second ifdef block (see
below). Git made it that way.. and I'm with you, the patch looks not good.
Should I open a 2nd ifdef CONFIG_MEDIA_CONTROLLER block instead?

Regards,
Marco

> 
> > +
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +
> >  /**
> >   * v4l2_subdev_get_try_format - ancillary routine to call
> >   *	&struct v4l2_subdev_pad_config->try_fmt
> > @@ -978,56 +1028,6 @@ static inline struct v4l2_rect
> >  #endif
> >  }
> >  
> > -extern const struct v4l2_file_operations v4l2_subdev_fops;
> > -
> > -/**
> > - * v4l2_set_subdevdata - Sets V4L2 dev private device data
> > - *
> > - * @sd: pointer to &struct v4l2_subdev
> > - * @p: pointer to the private device data to be stored.
> > - */
> > -static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
> > -{
> > -	sd->dev_priv = p;
> > -}
> > -
> > -/**
> > - * v4l2_get_subdevdata - Gets V4L2 dev private device data
> > - *
> > - * @sd: pointer to &struct v4l2_subdev
> > - *
> > - * Returns the pointer to the private device data to be stored.
> > - */
> > -static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
> > -{
> > -	return sd->dev_priv;
> > -}
> > -
> > -/**
> > - * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
> > - *
> > - * @sd: pointer to &struct v4l2_subdev
> > - * @p: pointer to the private data to be stored.
> > - */
> > -static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void *p)
> > -{
> > -	sd->host_priv = p;
> > -}
> > -
> > -/**
> > - * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
> > - *
> > - * @sd: pointer to &struct v4l2_subdev
> > - *
> > - * Returns the pointer to the private host data to be stored.
> > - */
> > -static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
> > -{
> > -	return sd->host_priv;
> > -}
> > -
> > -#ifdef CONFIG_MEDIA_CONTROLLER
> > -
> >  /**
> >   * v4l2_subdev_link_validate_default - validates a media link
> >   *
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> sakari.ailus@linux.intel.com
> 

[PATCH v2 5/7] [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>
---
 drivers/media/i2c/tvp5150.c | 109 +++++++++++++++++++++++++-----------
 1 file changed, 75 insertions(+), 34 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index a9cd79cbd7b0..825dcf3a9d83 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"
 
@@ -1007,20 +1008,40 @@ 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:
+		return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &decoder->rect;
+	default:
+		return NULL;
+	}
+}
+
 static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
 			    struct v4l2_subdev_pad_config *cfg,
 			    struct v4l2_subdev_format *format)
 {
 	struct v4l2_mbus_framefmt *f;
+	struct v4l2_rect *__crop;
 	struct tvp5150 *decoder = to_tvp5150(sd);
 
 	if (!format || (format->pad != TVP5150_PAD_VID_OUT))
 		return -EINVAL;
 
 	f = &format->format;
+	__crop = __tvp5150_get_pad_crop(decoder, cfg, format->pad,
+					format->which);
+	if (!__crop)
+		return -EINVAL;
 
-	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;
@@ -1031,17 +1052,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",
@@ -1050,17 +1105,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:
@@ -1073,20 +1118,18 @@ 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);
+
+	/*
+	 * Update output image size if the selection (crop) rectangle size or
+	 * position has been modified.
+	 */
+	if (!v4l2_rect_equal(&rect, __crop))
+		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+			__tvp5150_set_selection(sd, rect);
 
-	decoder->rect = rect;
+	*__crop = rect;
 
 	return 0;
 }
@@ -1098,9 +1141,6 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd,
 	struct tvp5150 *decoder = container_of(sd, struct tvp5150, sd);
 	v4l2_std_id std;
 
-	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-		return -EINVAL;
-
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP_BOUNDS:
 	case V4L2_SEL_TGT_CROP_DEFAULT:
@@ -1119,7 +1159,8 @@ 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;
+		sel->r = *__tvp5150_get_pad_crop(decoder, cfg, sel->pad,
+						      sel->which);
 		return 0;
 	default:
 		return -EINVAL;
-- 
2.18.0

[PATCH v2 6/7] [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 825dcf3a9d83..e736f609fecd 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -2032,6 +2032,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);
@@ -2044,10 +2047,6 @@ static int tvp5150_probe(struct i2c_client *c,
 		core->mbus_type = V4L2_MBUS_BT656;
 	}
 
-	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
-	sd->internal_ops = &tvp5150_internal_ops;
-	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-
 	res = tvp5150_mc_init(sd);
 	if (res)
 		goto err_cleanup_dt;
-- 
2.18.0

[PATCH v2 7/7] [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 e736f609fecd..e296f5bfae21 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1389,11 +1389,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;
@@ -1406,15 +1421,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;
 }
@@ -1616,6 +1626,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.18.0

Re: [PATCH v2 7/7] [media] tvp5150: add s_power callback

[Sakari Ailus]

Hi Marco,

On Mon, Aug 13, 2018 at 11:25:08AM +0200, Marco Felsch wrote:
> 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 e736f609fecd..e296f5bfae21 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -1389,11 +1389,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);

Could you use runtime PM instead?

For an example, the dw9714 driver does this: drivers/media/i2c/dw9714.c .

> +
> +	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;
> @@ -1406,15 +1421,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;
>  }
> @@ -1616,6 +1626,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 = {

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 7/7] [media] tvp5150: add s_power callback

[Marco Felsch]

Hi Sakari,

On 18-09-14 16:23, Sakari Ailus wrote:
> Hi Marco,
> 
> On Mon, Aug 13, 2018 at 11:25:08AM +0200, Marco Felsch wrote:
> > 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 e736f609fecd..e296f5bfae21 100644
> > --- a/drivers/media/i2c/tvp5150.c
> > +++ b/drivers/media/i2c/tvp5150.c
> > @@ -1389,11 +1389,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);
> 
> Could you use runtime PM instead?

I will test it next monday. What's the different between s_power and
runtime PM?

> 
> For an example, the dw9714 driver does this: drivers/media/i2c/dw9714.c .

Hopefully I got you right, should I use the
v4l2_subdev_internal_ops.open/close and call the pm_runtime_put/get
there or did you mean the driver.pm callbacks? I'm not that familiar
with the pm ops at the moment, sorry.

Regards,
Marco

> > +
> > +	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;
> > @@ -1406,15 +1421,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;
> >  }
> > @@ -1616,6 +1626,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 = {
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> sakari.ailus@linux.intel.com
> 

-- 
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 v2 7/7] [media] tvp5150: add s_power callback

[Mauro Carvalho Chehab]

Em Fri, 14 Sep 2018 20:20:46 +0200
Marco Felsch <m.felsch@pengutronix.de> escreveu:

> Hi Sakari,
> 
> On 18-09-14 16:23, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Mon, Aug 13, 2018 at 11:25:08AM +0200, Marco Felsch wrote:  
> > > 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 e736f609fecd..e296f5bfae21 100644
> > > --- a/drivers/media/i2c/tvp5150.c
> > > +++ b/drivers/media/i2c/tvp5150.c
> > > @@ -1389,11 +1389,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);  
> > 
> > Could you use runtime PM instead?  
> 
> I will test it next monday. What's the different between s_power and
> runtime PM?
> 
> > 
> > For an example, the dw9714 driver does this: drivers/media/i2c/dw9714.c .  
> 
> Hopefully I got you right, should I use the
> v4l2_subdev_internal_ops.open/close and call the pm_runtime_put/get
> there or did you mean the driver.pm callbacks? I'm not that familiar
> with the pm ops at the moment, sorry.

I guess the main issue here is: will this work if the bridge
driver is em28xx?

Whatever change we do, tvp5150 should still fully work with em28xx,
as several devices use this demod there.

Changing em28xx to cope with runtime PM would be *very* complex,
as there are lots of other drivers that can work with it, and
touching those will affect lots of other drivers. At the end, it
will very likely affect all PCI/PCIe V4L2 drivers, and several
USB ones.

If it can be done without affecting PM with em28xx, let's do it.
Otherwise, let's stick with s_power on this series, and let 
the mass PM rework on non-platform drivers to happen on some
separate patchset.

Thanks,
Mauro

Re: [PATCH v2 7/7] [media] tvp5150: add s_power callback

[Marco Felsch]

Hi,

On 18-09-14 15:57, Mauro Carvalho Chehab wrote:
> Em Fri, 14 Sep 2018 20:20:46 +0200
> Marco Felsch <m.felsch@pengutronix.de> escreveu:
> 
> > Hi Sakari,
> > 
> > On 18-09-14 16:23, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > On Mon, Aug 13, 2018 at 11:25:08AM +0200, Marco Felsch wrote:  
> > > > 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 e736f609fecd..e296f5bfae21 100644
> > > > --- a/drivers/media/i2c/tvp5150.c
> > > > +++ b/drivers/media/i2c/tvp5150.c
> > > > @@ -1389,11 +1389,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);  
> > > 
> > > Could you use runtime PM instead?  
> > 
> > I will test it next monday. What's the different between s_power and
> > runtime PM?
> > 
> > > 
> > > For an example, the dw9714 driver does this: drivers/media/i2c/dw9714.c .  
> > 
> > Hopefully I got you right, should I use the
> > v4l2_subdev_internal_ops.open/close and call the pm_runtime_put/get
> > there or did you mean the driver.pm callbacks? I'm not that familiar
> > with the pm ops at the moment, sorry.

I did some tests with the PM runtime ops. I used the suspend() callback
to disbale the IRQ and the resume() callback to enable it. But this
won't work with my IRQ-driven setup because if the signal is not locked
(maybe lost due to no input stream) the tvp went into suspend. In this
mode the IRQ is disabled and I have no chance to enable the chip again.

As Mauro pointed out below, there maybe some other issues I can't test
at the moment. I'm with Mauro to keep the .s_power() callback for this
series. There should be another patchset which covers the PM rework for
all devices using the TVP5150.

> 
> I guess the main issue here is: will this work if the bridge
> driver is em28xx?
> 
> Whatever change we do, tvp5150 should still fully work with em28xx,
> as several devices use this demod there.
> 
> Changing em28xx to cope with runtime PM would be *very* complex,
> as there are lots of other drivers that can work with it, and
> touching those will affect lots of other drivers. At the end, it
> will very likely affect all PCI/PCIe V4L2 drivers, and several
> USB ones.
> 
> If it can be done without affecting PM with em28xx, let's do it.
> Otherwise, let's stick with s_power on this series, and let 
> the mass PM rework on non-platform drivers to happen on some
> separate patchset.
> 
> Thanks,
> Mauro
> 

Kind regards,
Marco

Re: [PATCH v2 0/7] TVP5150 fixes and new features

[Marco Felsch]

Hi,

since I sent this series I only got feedback from Rob.

Regards,
Marco

On 18-08-13 11:25, Marco Felsch wrote:
> Hi,
> 
> this is my v2 with the integrated reviews from my v1 [1]. Since Mauro
> applied the most patches from my v1 to his experimental/tvp5150-3
> branch [2], this series only contains those which aren't applied.
> 
> Patches I changed contain a changelog, so I will omit these here.
> 
> Patch ('[media] tvp5150: add FORMAT_TRY support for get/set selection
> handlers') throws a compile error. Therefore I added two v4l2-subdev.h
> patches which should fix the error in a common way.
> 
> Patch ('[media] tvp5150: add s_power callback') is new too. I forget
> them in my v1. This patch address the interrupt enable/disable handling.
> Now it is possible to pause streaming and keep the interrupts on.
> 
> The changes I made in the ('[media] tvp5150: add input source selection
> of_graph support') patch are based on the the RFC [3] and discussion [4].
> I dropped patch ('[media] tvp5150: Change default input source selection
> behaviour') since the default input source selectopn is setup during the
> .registered() callback now.
> 
> I've tested this series on a customer dt-based board. Unfortunately I
> haven't a device which use the em28xx driver. So other tester a welcome :)
> 
> [1] https://www.spinics.net/lists/devicetree/msg236650.html
> [2] https://git.linuxtv.org/mchehab/experimental.git/log/?h=tvp5150-3
> [3] https://www.spinics.net/lists/devicetree/msg243181.html
> [4] https://www.spinics.net/lists/devicetree/msg243840.html
> 
> Regards,
> Marco
> 
> Marco Felsch (6):
>   [media] tvp5150: add input source selection of_graph support
>   [media] dt-bindings: tvp5150: Add input port connectors DT bindings
>   [media] v4l2-subdev: add stubs for v4l2_subdev_get_try_*
>   [media] v4l2-subdev: fix v4l2_subdev_get_try_* dependency
>   [media] tvp5150: add FORMAT_TRY support for get/set selection handlers
>   [media] tvp5150: add s_power callback
> 
> Michael Tretter (1):
>   [media] tvp5150: initialize subdev before parsing device tree
> 
>  .../devicetree/bindings/media/i2c/tvp5150.txt | 191 +++++-
>  drivers/media/i2c/tvp5150.c                   | 611 +++++++++++++++---
>  include/media/v4l2-subdev.h                   | 111 ++--
>  3 files changed, 776 insertions(+), 137 deletions(-)
> 
> -- 
> 2.18.0
> 
> 

Re: [PATCH v2 0/7] TVP5150 fixes and new features

[Mauro Carvalho Chehab]

Em Fri, 14 Sep 2018 10:43:03 +0200
Marco Felsch <m.felsch@pengutronix.de> escreveu:

> Hi,
> 
> since I sent this series I only got feedback from Rob.

I'm doing some tests on it. If everything gets ok, I'll likely merge
it today.

> 
> Regards,
> Marco
> 
> On 18-08-13 11:25, Marco Felsch wrote:
> > Hi,
> > 
> > this is my v2 with the integrated reviews from my v1 [1]. Since Mauro
> > applied the most patches from my v1 to his experimental/tvp5150-3
> > branch [2], this series only contains those which aren't applied.
> > 
> > Patches I changed contain a changelog, so I will omit these here.
> > 
> > Patch ('[media] tvp5150: add FORMAT_TRY support for get/set selection
> > handlers') throws a compile error. Therefore I added two v4l2-subdev.h
> > patches which should fix the error in a common way.
> > 
> > Patch ('[media] tvp5150: add s_power callback') is new too. I forget
> > them in my v1. This patch address the interrupt enable/disable handling.
> > Now it is possible to pause streaming and keep the interrupts on.
> > 
> > The changes I made in the ('[media] tvp5150: add input source selection
> > of_graph support') patch are based on the the RFC [3] and discussion [4].
> > I dropped patch ('[media] tvp5150: Change default input source selection
> > behaviour') since the default input source selectopn is setup during the
> > .registered() callback now.
> > 
> > I've tested this series on a customer dt-based board. Unfortunately I
> > haven't a device which use the em28xx driver. So other tester a welcome :)
> > 
> > [1] https://www.spinics.net/lists/devicetree/msg236650.html
> > [2] https://git.linuxtv.org/mchehab/experimental.git/log/?h=tvp5150-3
> > [3] https://www.spinics.net/lists/devicetree/msg243181.html
> > [4] https://www.spinics.net/lists/devicetree/msg243840.html
> > 
> > Regards,
> > Marco
> > 
> > Marco Felsch (6):
> >   [media] tvp5150: add input source selection of_graph support
> >   [media] dt-bindings: tvp5150: Add input port connectors DT bindings
> >   [media] v4l2-subdev: add stubs for v4l2_subdev_get_try_*
> >   [media] v4l2-subdev: fix v4l2_subdev_get_try_* dependency
> >   [media] tvp5150: add FORMAT_TRY support for get/set selection handlers
> >   [media] tvp5150: add s_power callback
> > 
> > Michael Tretter (1):
> >   [media] tvp5150: initialize subdev before parsing device tree
> > 
> >  .../devicetree/bindings/media/i2c/tvp5150.txt | 191 +++++-
> >  drivers/media/i2c/tvp5150.c                   | 611 +++++++++++++++---
> >  include/media/v4l2-subdev.h                   | 111 ++--
> >  3 files changed, 776 insertions(+), 137 deletions(-)
> > 
> > -- 
> > 2.18.0
> > 
> >   



Thanks,
Mauro