[PATCH 0/8] [media] tvp5150: add HW input connectors support

[PATCH 0/8] [media] tvp5150: add HW input connectors support

[Javier Martinez Canillas]

Hello,

One of my test machines for MC is an IGEPv2 board that has a tvp5151
decoder attached to the OMAP3 ISP bridge.

The board has 2 composite RCA input connectors on it but I've no way
to switch those using the MC framework.

The driver currently uses the s_routing callback to change the input
but the documentation is clear that user level input IDs should never
be used (e.g. Composite, S-Video, etc). See: include/media/v4l2-subdev.h.

Also, these are HW blocks and other interface-centric drivers use
media entities to represent the input connectors (i.e: au0828 and
cx231xx) so this patch series do the same for the tvp5150 driver.

By having media entities for the input connectors, switching the
current input can be easily done with the MEDIA_IOC_SETUP_LINK ioctl:

$ media-ctl -r -l '"Composite0":0->"tvp5150 1-005c":0[1]'

Since the driver is responsible for registering the media entities
and creating the pad links, the associated v4l2 media device is needed.

But the driver registers the sub-dev using v4l2_async_register_subdev()
so a subdev core operation operating has been added so the v4l2 async
core can invoke the driver's initialization routing after the sub-dev
has been registered.

Please let me know if you think there's a better way to solve this.

Best regards,
Javier


Javier Martinez Canillas (8):
  [media] v4l2-subdev: add registered_async subdev core operation
  [media] v4l2-async: call registered_async after subdev registration
  [media] tvp5150: put endpoint node on error
  [media] tvp5150: store dev id and rom version
  [media] tvp5150: add internal signal generator to HW input list
  [media] tvp5150: move input definition header to dt-bindings
  [media] tvp5150: document input connectors DT bindings
  [media] tvp5150: add HW input connectors support

 .../devicetree/bindings/media/i2c/tvp5150.txt      |  43 +++++
 drivers/media/i2c/tvp5150.c                        | 179 +++++++++++++++++++--
 drivers/media/v4l2-core/v4l2-async.c               |   7 +
 include/{media/i2c => dt-bindings/media}/tvp5150.h |   9 +-
 include/media/v4l2-subdev.h                        |   3 +
 5 files changed, 226 insertions(+), 15 deletions(-)
 rename include/{media/i2c => dt-bindings/media}/tvp5150.h (85%)

-- 
2.5.0

[PATCH 1/8] [media] v4l2-subdev: add registered_async subdev core operation

[Javier Martinez Canillas]

V4L2 sub-devices that are registered asynchronously have no way to know
when they have been registration with a V4L2 device but they might need
to take some action after this.

So let's add a callback that can be executed by the V4L2 async core to
allow sub-devices drivers to do any needed initialization that depends
on the registration.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 include/media/v4l2-subdev.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index b273cf9ac047..11e2dfec0198 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -179,6 +179,8 @@ struct v4l2_subdev_io_pin_config {
  *		     for it to be warned when the value of a control changes.
  *
  * @unsubscribe_event: remove event subscription from the control framework.
+ *
+ * @registered_async: the subdevice has been registered async.
  */
 struct v4l2_subdev_core_ops {
 	int (*log_status)(struct v4l2_subdev *sd);
@@ -211,6 +213,7 @@ struct v4l2_subdev_core_ops {
 			       struct v4l2_event_subscription *sub);
 	int (*unsubscribe_event)(struct v4l2_subdev *sd, struct v4l2_fh *fh,
 				 struct v4l2_event_subscription *sub);
+	int (*registered_async)(struct v4l2_subdev *sd);
 };
 
 /**
-- 
2.5.0

[PATCH 2/8] [media] v4l2-async: call registered_async after subdev registration

[Javier Martinez Canillas]

V4L2 sub-devices might need to do initialization that depends on being
registered with a V4L2 device. As an example, sub-devices with Media
Controller support may need to register entities and create pad links.

Execute the registered_async callback after the sub-device has been
registered with the V4L2 device so the driver can do any needed init.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/v4l2-core/v4l2-async.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 5bada202b2d3..716bfd47daab 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -119,6 +119,13 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
 		return ret;
 	}
 
+	ret = v4l2_subdev_call(sd, core, registered_async);
+	if (ret < 0) {
+		if (notifier->unbind)
+			notifier->unbind(notifier, sd, asd);
+		return ret;
+	}
+
 	if (list_empty(&notifier->waiting) && notifier->complete)
 		return notifier->complete(notifier);
 
-- 
2.5.0

[PATCH 3/8] [media] tvp5150: put endpoint node on error

[Javier Martinez Canillas]

If the parallel mbus configuration is not correct, the endpoint
device node isn't currently put again in the error path. Fix it.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

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

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 19b52736b24e..c7eeb59a999b 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1268,8 +1268,10 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 	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))
-		return -EINVAL;
+	      flags & V4L2_MBUS_FIELD_EVEN_LOW)) {
+		ret = -EINVAL;
+		goto err;
+	}
 
 	decoder->mbus_type = bus_cfg.bus_type;
 
-- 
2.5.0

[PATCH 4/8] [media] tvp5150: store dev id and rom version

[Javier Martinez Canillas]

Not all tvp5150 variants support the same, for example some have an
internal signal generator that can output a black screen.

So the device id and rom version have to be stored in the driver's
state to know what variant is a given device.

While being there, remove some redundant comments about the device
version since there is already calls to v4l2_info() with that info.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/i2c/tvp5150.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index c7eeb59a999b..093ff80f944c 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -49,6 +49,9 @@ struct tvp5150 {
 	u32 output;
 	int enable;
 
+	u16 dev_id;
+	u16 rom_ver;
+
 	enum v4l2_mbus_type mbus_type;
 };
 
@@ -1180,8 +1183,6 @@ static int tvp5150_detect_version(struct tvp5150 *core)
 	struct v4l2_subdev *sd = &core->sd;
 	struct i2c_client *c = v4l2_get_subdevdata(sd);
 	unsigned int i;
-	u16 dev_id;
-	u16 rom_ver;
 	u8 regs[4];
 	int res;
 
@@ -1196,23 +1197,25 @@ static int tvp5150_detect_version(struct tvp5150 *core)
 		regs[i] = res;
 	}
 
-	dev_id = (regs[0] << 8) | regs[1];
-	rom_ver = (regs[2] << 8) | regs[3];
+	core->dev_id = (regs[0] << 8) | regs[1];
+	core->rom_ver = (regs[2] << 8) | regs[3];
 
 	v4l2_info(sd, "tvp%04x (%u.%u) chip found @ 0x%02x (%s)\n",
-		  dev_id, regs[2], regs[3], c->addr << 1, c->adapter->name);
+		  core->dev_id, regs[2], regs[3], c->addr << 1,
+		  c->adapter->name);
 
-	if (dev_id == 0x5150 && rom_ver == 0x0321) { /* TVP51510A */
+	if (core->dev_id == 0x5150 && core->rom_ver == 0x0321) {
 		v4l2_info(sd, "tvp5150a detected.\n");
-	} else if (dev_id == 0x5150 && rom_ver == 0x0400) { /* TVP5150AM1 */
+	} else if (core->dev_id == 0x5150 && core->rom_ver == 0x0400) {
 		v4l2_info(sd, "tvp5150am1 detected.\n");
 
 		/* ITU-T BT.656.4 timing */
 		tvp5150_write(sd, TVP5150_REV_SELECT, 0);
-	} else if (dev_id == 0x5151 && rom_ver == 0x0100) { /* TVP5151 */
+	} else if (core->dev_id == 0x5151 && core->rom_ver == 0x0100) {
 		v4l2_info(sd, "tvp5151 detected.\n");
 	} else {
-		v4l2_info(sd, "*** unknown tvp%04x chip detected.\n", dev_id);
+		v4l2_info(sd, "*** unknown tvp%04x chip detected.\n",
+			  core->dev_id);
 	}
 
 	return 0;
-- 
2.5.0

[PATCH 5/8] [media] tvp5150: add internal signal generator to HW input list

[Javier Martinez Canillas]

Some tvp5150 variants, have an internal generator that can generate a
black screen output. Since this is a HW block, it should be in the HW
inputs list.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 include/media/i2c/tvp5150.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/media/i2c/tvp5150.h b/include/media/i2c/tvp5150.h
index 649908a25605..685a1e718531 100644
--- a/include/media/i2c/tvp5150.h
+++ b/include/media/i2c/tvp5150.h
@@ -25,6 +25,7 @@
 #define TVP5150_COMPOSITE0 0
 #define TVP5150_COMPOSITE1 1
 #define TVP5150_SVIDEO     2
+#define TVP5150_GENERATOR  3
 
 /* TVP5150 HW outputs */
 #define TVP5150_NORMAL       0
-- 
2.5.0

[PATCH 6/8] [media] tvp5150: move input definition header to dt-bindings

[Javier Martinez Canillas]

Add a header file for the tvp5150 input connectors constants that
can be shared between the driver and Device Tree source files.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/media/i2c/tvp5150.c                        | 2 +-
 include/{media/i2c => dt-bindings/media}/tvp5150.h | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)
 rename include/{media/i2c => dt-bindings/media}/tvp5150.h (90%)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 093ff80f944c..3a32fd1df805 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -5,6 +5,7 @@
  * This code is placed under the terms of the GNU General Public License v2
  */
 
+#include <dt-bindings/media/tvp5150.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
@@ -13,7 +14,6 @@
 #include <linux/module.h>
 #include <media/v4l2-async.h>
 #include <media/v4l2-device.h>
-#include <media/i2c/tvp5150.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-of.h>
 #include <media/v4l2-mc.h>
diff --git a/include/media/i2c/tvp5150.h b/include/dt-bindings/media/tvp5150.h
similarity index 90%
rename from include/media/i2c/tvp5150.h
rename to include/dt-bindings/media/tvp5150.h
index 685a1e718531..dc347569854f 100644
--- a/include/media/i2c/tvp5150.h
+++ b/include/dt-bindings/media/tvp5150.h
@@ -18,8 +18,8 @@
     Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 */
 
-#ifndef _TVP5150_H_
-#define _TVP5150_H_
+#ifndef _DT_BINDINGS_MEDIA_TVP5150_H
+#define _DT_BINDINGS_MEDIA_TVP5150_H
 
 /* TVP5150 HW inputs */
 #define TVP5150_COMPOSITE0 0
@@ -31,4 +31,4 @@
 #define TVP5150_NORMAL       0
 #define TVP5150_BLACK_SCREEN 1
 
-#endif
+#endif /* _DT_BINDINGS_MEDIA_TVP5150_H */
-- 
2.5.0

[PATCH 7/8] [media] tvp5150: document input connectors DT bindings

[Javier Martinez Canillas]

The tvp5150 decoder has different input connectors so extend the device
tree binding to allow device tree source files to define the connectors
that are available on a given board.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 .../devicetree/bindings/media/i2c/tvp5150.txt      | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
index 8c0fc1a26bf0..daa20e43a8e3 100644
--- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
+++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
@@ -12,6 +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.
 
+Optional nodes:
+- connectors: The input connectors of tvp5150 have to be defined under
+  a subnode name "connectors" using the following format:
+
+	input-connector-name {
+		input connector properties
+	};
+
+Each input connector must contain the following properties:
+
+	- label: a name for the connector.
+	- input: the input connector.
+
+The possible values for the "input" property are:
+	0: Composite0
+	1: Composite1
+	2: S-Video
+
+and on a tvp5150am1 and tvp5151 there is another:
+	4: Signal generator
+
+The list of valid input connectors are defined in dt-bindings/media/tvp5150.h
+header file and can be included by device tree source files.
+
+Each input connector can be defined only once.
+
 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.
@@ -36,6 +62,23 @@ Example:
 		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
 		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
 
+		connectors {
+			composite0 {
+				label = "Composite0";
+				input = <TVP5150_COMPOSITE0>;
+			};
+
+			composite1 {
+				label = "Composite1";
+				input = <TVP5150_COMPOSITE1>;
+			};
+
+			s-video {
+				label = "S-Video";
+				input = <TVP5150_SVIDEO>;
+			};
+		};
+
 		port {
 			tvp5150_1: endpoint {
 				remote-endpoint = <&ccdc_ep>;
-- 
2.5.0

[PATCH 8/8] [media] tvp5150: add HW input connectors support

[Javier Martinez Canillas]

The tvp5150 decoder has different input connectors. The actual list of
HW inputs depends on the device version but all have at least these 3:

1) Composite0
2) Composite1
3) S-Video

and some variants have a 4th possible input connector:

4) Signal generator

The driver currently uses the .s_routing callback to switch the input
connector but since these are separate HW blocks, it's better to use
media entities to represent the input connectors and their source pads
linked with the decoder's sink pad.

This allows user-space to use the MEDIA_IOC_SETUP_LINK ioctl to choose
the input connector. For example using the media-ctl user-space tool:

$ media-ctl -r -l '"Composite0":0->"tvp5150 1-005c":0[1]'

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

 drivers/media/i2c/tvp5150.c         | 150 ++++++++++++++++++++++++++++++++++++
 include/dt-bindings/media/tvp5150.h |   2 +
 2 files changed, 152 insertions(+)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 3a32fd1df805..b8976028fc82 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -40,6 +40,8 @@ struct tvp5150 {
 	struct v4l2_subdev sd;
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_pad pads[DEMOD_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;
@@ -997,6 +999,49 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
 }
 
 /****************************************************************************
+			Media entity ops
+ ****************************************************************************/
+
+static int tvp5150_link_setup(struct media_entity *entity,
+			      const struct media_pad *local,
+			      const struct media_pad *remote, u32 flags)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+	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;
+
+	/* Only tvp5150am1 and tvp5151 have signal generator support */
+	if ((decoder->dev_id == 0x5150 && decoder->rom_ver == 0x0400) ||
+	    (decoder->dev_id == 0x5151 && decoder->rom_ver == 0x0100)) {
+		decoder->output = (i == TVP5150_GENERATOR ?
+				   TVP5150_BLACK_SCREEN : TVP5150_NORMAL);
+	} else {
+		decoder->output = TVP5150_NORMAL;
+	}
+
+	tvp5150_selmux(sd);
+#endif
+
+	return 0;
+}
+
+static const struct media_entity_operations tvp5150_sd_media_ops = {
+	.link_setup = tvp5150_link_setup,
+};
+
+/****************************************************************************
 			I2C Command
  ****************************************************************************/
 
@@ -1122,6 +1167,42 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
 	return 0;
 }
 
+static int tvp5150_registered_async(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,
+					    DEMOD_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 = {
@@ -1135,6 +1216,7 @@ static const struct v4l2_subdev_core_ops tvp5150_core_ops = {
 	.g_register = tvp5150_g_register,
 	.s_register = tvp5150_s_register,
 #endif
+	.registered_async = tvp5150_registered_async,
 };
 
 static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
@@ -1255,6 +1337,12 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 {
 	struct v4l2_of_endpoint bus_cfg;
 	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;
 
@@ -1278,6 +1366,66 @@ 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) {
+			v4l2_err(&decoder->sd,
+				 "missing type property in node %s\n",
+				 child->name);
+			goto err_connector;
+		}
+
+		if (input_type > TVP5150_INPUT_NUM) {
+			ret = -EINVAL;
+			goto err_connector;
+		}
+
+		input = &decoder->input_ent[input_type];
+
+		/* Each input connector can only be defined once */
+		if (input->name) {
+			v4l2_err(&decoder->sd,
+				 "input %s with same type already exists\n",
+				 input->name);
+			ret = -EINVAL;
+			goto err_connector;
+		}
+
+		switch (input_type) {
+		case TVP5150_COMPOSITE0:
+		case TVP5150_COMPOSITE1:
+			input->function = MEDIA_ENT_F_CONN_COMPOSITE;
+			break;
+		case TVP5150_SVIDEO:
+			input->function = MEDIA_ENT_F_CONN_SVIDEO;
+			break;
+		case TVP5150_GENERATOR:
+			input->function = MEDIA_ENT_F_CONN_TEST;
+			break;
+		}
+
+		input->flags = MEDIA_ENT_FL_CONNECTOR;
+
+		ret = of_property_read_string(child, "label", &name);
+		if (ret < 0) {
+			v4l2_err(&decoder->sd,
+				 "missing label property in node %s\n",
+				 child->name);
+			goto err_connector;
+		}
+
+		input->name = name;
+	}
+
+err_connector:
+	of_node_put(connectors);
+#endif
 err:
 	of_node_put(ep);
 	return ret;
@@ -1330,6 +1478,8 @@ static int tvp5150_probe(struct i2c_client *c,
 	res = media_entity_pads_init(&sd->entity, DEMOD_NUM_PADS, core->pads);
 	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 dc347569854f..d30865222082 100644
--- a/include/dt-bindings/media/tvp5150.h
+++ b/include/dt-bindings/media/tvp5150.h
@@ -27,6 +27,8 @@
 #define TVP5150_SVIDEO     2
 #define TVP5150_GENERATOR  3
 
+#define TVP5150_INPUT_NUM  4
+
 /* TVP5150 HW outputs */
 #define TVP5150_NORMAL       0
 #define TVP5150_BLACK_SCREEN 1
-- 
2.5.0

Re: [PATCH 7/8] [media] tvp5150: document input connectors DT bindings

[Javier Martinez Canillas]

Hello,

I noticed that I missed the DT folks in the cc list so I'm adding
them now, sorry for the noise...

On 02/05/2016 04:09 PM, Javier Martinez Canillas wrote:
> The tvp5150 decoder has different input connectors so extend the device
> tree binding to allow device tree source files to define the connectors
> that are available on a given board.
>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
>
>   .../devicetree/bindings/media/i2c/tvp5150.txt      | 43 ++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> index 8c0fc1a26bf0..daa20e43a8e3 100644
> --- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> @@ -12,6 +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.
>
> +Optional nodes:
> +- connectors: The input connectors of tvp5150 have to be defined under
> +  a subnode name "connectors" using the following format:
> +
> +	input-connector-name {
> +		input connector properties
> +	};
> +
> +Each input connector must contain the following properties:
> +
> +	- label: a name for the connector.
> +	- input: the input connector.
> +
> +The possible values for the "input" property are:
> +	0: Composite0
> +	1: Composite1
> +	2: S-Video
> +
> +and on a tvp5150am1 and tvp5151 there is another:
> +	4: Signal generator
> +
> +The list of valid input connectors are defined in dt-bindings/media/tvp5150.h
> +header file and can be included by device tree source files.
> +
> +Each input connector can be defined only once.
> +
>   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.
> @@ -36,6 +62,23 @@ Example:
>   		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
>   		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
>
> +		connectors {
> +			composite0 {
> +				label = "Composite0";
> +				input = <TVP5150_COMPOSITE0>;
> +			};
> +
> +			composite1 {
> +				label = "Composite1";
> +				input = <TVP5150_COMPOSITE1>;
> +			};
> +
> +			s-video {
> +				label = "S-Video";
> +				input = <TVP5150_SVIDEO>;
> +			};
> +		};
> +
>   		port {
>   			tvp5150_1: endpoint {
>   				remote-endpoint = <&ccdc_ep>;
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 2/8] [media] v4l2-async: call registered_async after subdev registration

[Sakari Ailus]

Hi Javier,

On Fri, Feb 05, 2016 at 04:09:52PM -0300, Javier Martinez Canillas wrote:
> V4L2 sub-devices might need to do initialization that depends on being
> registered with a V4L2 device. As an example, sub-devices with Media
> Controller support may need to register entities and create pad links.
> 
> Execute the registered_async callback after the sub-device has been
> registered with the V4L2 device so the driver can do any needed init.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/media/v4l2-core/v4l2-async.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 5bada202b2d3..716bfd47daab 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -119,6 +119,13 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
>  		return ret;
>  	}
>  
> +	ret = v4l2_subdev_call(sd, core, registered_async);
> +	if (ret < 0) {
> +		if (notifier->unbind)
> +			notifier->unbind(notifier, sd, asd);
> +		return ret;
> +	}
> +
>  	if (list_empty(&notifier->waiting) && notifier->complete)
>  		return notifier->complete(notifier);

I noticed this just now but what do you need this and the next patch for?

We already have a callback for the same purpose: it's
v4l2_subdev_ops.internal_ops.registered(). And there's similar
unregistered() callback as well.

Could you use these callbacks instead?

What made me notice this is because the two patches break all other drivers
that do not implement registered_async(). This would be fixed by your
follow-up patch which is not merged, but the real question is: are these
patches needed to begin with?

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 2/8] [media] v4l2-async: call registered_async after subdev registration

[Sakari Ailus]

On Thu, Aug 11, 2016 at 02:10:43PM +0300, Sakari Ailus wrote:
> Hi Javier,
> 
> On Fri, Feb 05, 2016 at 04:09:52PM -0300, Javier Martinez Canillas wrote:
> > V4L2 sub-devices might need to do initialization that depends on being
> > registered with a V4L2 device. As an example, sub-devices with Media
> > Controller support may need to register entities and create pad links.
> > 
> > Execute the registered_async callback after the sub-device has been
> > registered with the V4L2 device so the driver can do any needed init.
> > 
> > Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> > ---
> > 
> >  drivers/media/v4l2-core/v4l2-async.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 5bada202b2d3..716bfd47daab 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -119,6 +119,13 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier,
> >  		return ret;
> >  	}
> >  
> > +	ret = v4l2_subdev_call(sd, core, registered_async);
> > +	if (ret < 0) {
> > +		if (notifier->unbind)
> > +			notifier->unbind(notifier, sd, asd);
> > +		return ret;
> > +	}
> > +
> >  	if (list_empty(&notifier->waiting) && notifier->complete)
> >  		return notifier->complete(notifier);
> 
> I noticed this just now but what do you need this and the next patch for?
> 
> We already have a callback for the same purpose: it's
> v4l2_subdev_ops.internal_ops.registered(). And there's similar
> unregistered() callback as well.
> 
> Could you use these callbacks instead?
> 
> What made me notice this is because the two patches break all other drivers
> that do not implement registered_async(). This would be fixed by your
> follow-up patch which is not merged, but the real question is: are these
> patches needed to begin with?

Ah, it's actually merged now. So all is well. The question still remains.
:-)

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 2/8] [media] v4l2-async: call registered_async after subdev registration

[Javier Martinez Canillas]

Hello Sakari,

Thanks a lot for your feedback.

On 08/11/2016 07:18 AM, Sakari Ailus wrote:
> On Thu, Aug 11, 2016 at 02:10:43PM +0300, Sakari Ailus wrote:

[snip]

>>>  
>>> +	ret = v4l2_subdev_call(sd, core, registered_async);
>>> +	if (ret < 0) {
>>> +		if (notifier->unbind)
>>> +			notifier->unbind(notifier, sd, asd);
>>> +		return ret;
>>> +	}
>>> +
>>>  	if (list_empty(&notifier->waiting) && notifier->complete)
>>>  		return notifier->complete(notifier);
>>
>> I noticed this just now but what do you need this and the next patch for?
>>
>> We already have a callback for the same purpose: it's
>> v4l2_subdev_ops.internal_ops.registered(). And there's similar
>> unregistered() callback as well.
>>

Oh, I missed we already had those calls. When adding the connector
support, I looked at struct v4l2_subdev_core_ops and didn't find a
callback that fit but didn't notice we already had a .registered()
in struct struct v4l2_subdev_internal_ops. Sorry about that...

>> Could you use these callbacks instead?

Yes, those can be used indeed. I'll post patches using that instead
and removing the .registered_async callback since as you said isn't
really needed.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 2/8] [media] v4l2-async: call registered_async after subdev registration

[Sakari Ailus]

Hi Javier,

On Thu, Aug 11, 2016 at 12:14:01PM -0400, Javier Martinez Canillas wrote:
> Hello Sakari,
> 
> Thanks a lot for your feedback.
> 
> On 08/11/2016 07:18 AM, Sakari Ailus wrote:
> > On Thu, Aug 11, 2016 at 02:10:43PM +0300, Sakari Ailus wrote:
> 
> [snip]
> 
> >>>  
> >>> +	ret = v4l2_subdev_call(sd, core, registered_async);
> >>> +	if (ret < 0) {
> >>> +		if (notifier->unbind)
> >>> +			notifier->unbind(notifier, sd, asd);
> >>> +		return ret;
> >>> +	}
> >>> +
> >>>  	if (list_empty(&notifier->waiting) && notifier->complete)
> >>>  		return notifier->complete(notifier);
> >>
> >> I noticed this just now but what do you need this and the next patch for?
> >>
> >> We already have a callback for the same purpose: it's
> >> v4l2_subdev_ops.internal_ops.registered(). And there's similar
> >> unregistered() callback as well.
> >>
> 
> Oh, I missed we already had those calls. When adding the connector
> support, I looked at struct v4l2_subdev_core_ops and didn't find a
> callback that fit but didn't notice we already had a .registered()
> in struct struct v4l2_subdev_internal_ops. Sorry about that...

No worries. I hadn't time to review the patches either.

> >> Could you use these callbacks instead?
> 
> Yes, those can be used indeed. I'll post patches using that instead
> and removing the .registered_async callback since as you said isn't
> really needed.

I'd appreciate that! Please cc me when you do so.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk