davinci: VPIF: add DT support

Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from)

am437x-vpfe.c

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
 include/media/davinci/vpif_types.h            |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

drivers/media/platform/davinci/vpif_capture.c +0 -0

index 94ee6cf03f02..47a4699157e7 100644

drivers/media/platform/davinci/vpif_capture.c +47 -1

 #include "vpif.h"
 #include "vpif_capture.h"

@@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
 
 	vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+	if (!chan_cfg)
+		return -1;
+	if (input_index >= chan_cfg->input_count)
+		return -1;
 	subdev_name = chan_cfg->inputs[input_index].subdev_name;
 	if (subdev_name == NULL)
 		return -1;

@@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
 	/* loop through the sub device list to get the sub device info */
 	for (i = 0; i < vpif_cfg->subdev_count; i++) {
 		subdev_info = &vpif_cfg->subdev_info[i];
-		if (!strcmp(subdev_info->name, subdev_name))
+		if (subdev_info && !strcmp(subdev_info->name, subdev_name))
 			return i;
 	}
 	return -1;

@@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct v4l2_async_notifier *notifier,
 {
 	int i;
 
+	for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+		struct v4l2_async_subdev *_asd = vpif_obj.config->asd[i];
+		const struct device_node *node = _asd->match.of.node;
+
+		if (node == subdev->of_node) {
+			vpif_obj.sd[i] = subdev;
+			vpif_obj.config->chan_config->inputs[i].subdev_name =
+				(char *)subdev->of_node->full_name;
+			vpif_dbg(2, debug,
+				 "%s: setting input %d subdev_name = %s\n",
+				 __func__, i, subdev->of_node->full_name);
+			return 0;
+		}
+	}
+
 	for (i = 0; i < vpif_obj.config->subdev_count; i++)
 		if (!strcmp(vpif_obj.config->subdev_info[i].name,
 			    subdev->name)) {

@@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct v4l2_async_notifier *notifier)
 	return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+	struct device_node *endpoint = NULL;
+	struct v4l2_of_endpoint bus_cfg;
+	struct vpif_capture_config *pdata;
+	struct vpif_subdev_info *sdinfo;
+	struct vpif_capture_chan_config *chan;
+	unsigned int i;
+
+	dev_dbg(&pdev->dev, "vpif_get_pdata\n");
+
+	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+		return pdev->dev.platform_data;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+	pdata->subdev_info =
+		devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
+			     VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+	if (!pdata->subdev_info)
+		return NULL;
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	for (i = 0; ; i++) {

drivers/media/platform/davinci/vpif_capture.c +25 -0

+			chan->inputs[i].input_route = INPUT_CVBS_VI2B;
+		else
+			chan->inputs[i].input_route = INPUT_SVIDEO_VI2C_VI1C;
+		chan->inputs[i].output_route = OUTPUT_10BIT_422_EMBEDDED_SYNC;
+
+		err = v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+		if (err) {
+			dev_err(&pdev->dev, "Could not parse the endpoint\n");
+			goto done;
+		}
+		dev_dbg(&pdev->dev, "Endpoint %s, bus_width = %d\n",
+			endpoint->full_name, bus_cfg.bus.parallel.bus_width);
+		flags = bus_cfg.bus.parallel.flags;
+
+		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+			chan->vpif_if.hd_pol = 1;
+
+		if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
+			chan->vpif_if.vd_pol = 1;
+
+		chan->vpif_if.if_type = VPIF_IF_BT656;
+		rem = of_graph_get_remote_port_parent(endpoint);
+		if (!rem) {
+			dev_dbg(&pdev->dev, "Remote device at %s not found\n",
+				endpoint->full_name);

drivers/media/platform/davinci/vpif_capture.c +21 -1

+		if (!pdata->asd[i]) {
+			of_node_put(rem);
+			pdata = NULL;
+			goto done;
+		}
+
+		pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_OF;
+		pdata->asd[i]->match.of.node = rem;
+		of_node_put(rem);
+	}
+
+done:
+	pdata->asd_sizes[0] = i;
+	pdata->subdev_count = i;
+	pdata->card_name = "DA850/OMAP-L138 Video Capture";
+
+	return pdata;
+}
+
 /**
  * vpif_probe : This function probes the vpif capture driver
  * @pdev: platform device pointer

@@ -1438,6 +1563,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 	int res_idx = 0;
 	int i, err;
 
+	pdev->dev.platform_data = vpif_capture_get_pdata(pdev);
 	if (!pdev->dev.platform_data) {
 		dev_warn(&pdev->dev, "Missing platform data.  Giving up.\n");
 		return -EINVAL;

@@ -1480,7 +1606,7 @@ static __init int vpif_probe(struct platform_device *pdev)
 		goto vpif_unregister;
 	}
 
-	if (!vpif_obj.config->asd_sizes) {
+	if (!vpif_obj.config->asd_sizes[0]) {
 		i2c_adap = i2c_get_adapter(1);
 		for (i = 0; i < subdev_count; i++) {
 			subdevdata = &vpif_obj.config->subdev_info[i];

include/media/davinci/vpif_types.h +5 -4

index 3cb1704a0650..4ee3b41975db 100644
@@ -65,14 +65,14 @@ struct vpif_display_config {
 
 struct vpif_input {
 	struct v4l2_input input;
-	const char *subdev_name;
+	char *subdev_name;
 	u32 input_route;
 	u32 output_route;
 };
 
 struct vpif_capture_chan_config {
 	struct vpif_interface vpif_if;
-	const struct vpif_input *inputs;
+	struct vpif_input *inputs;
 	int input_count;
 };
 

@@ -83,7 +83,8 @@ struct vpif_capture_config {
 	struct vpif_subdev_info *subdev_info;
 	int subdev_count;
 	const char *card_name;
-	struct v4l2_async_subdev **asd;	/* Flat array, arranged in groups */
-	int *asd_sizes;		/* 0-terminated array of asd group sizes */
+
+	struct v4l2_async_subdev *asd[VPIF_CAPTURE_MAX_CHANNELS];
+	int asd_sizes[VPIF_CAPTURE_MAX_CHANNELS];
 };
 #endif /* _VPIF_TYPES_H */

Quoted 3 lines · Kevin Hilman

Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from)

vpif_capture_get_pdata and "largely"?

Quoted 15 lines

am437x-vpfe.c

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
 include/media/davinci/vpif_types.h            |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

drivers/media/platform/davinci/vpif_capture.c +0 -0

index 94ee6cf03f02..47a4699157e7 100644
@@ -26,6 +26,8 @@
 #include <linux/slab.h>
 

drivers/media/platform/davinci/vpif_capture.c line 30

Quoted 3 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-of.h>
+#include <media/i2c/tvp514x.h>

Do you need this header?

Quoted 80 lines

drivers/media/platform/davinci/vpif_capture.c +51 -1

 #include "vpif.h"
 #include "vpif_capture.h"

@@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
 
 	vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+	if (!chan_cfg)
+		return -1;
+	if (input_index >= chan_cfg->input_count)
+		return -1;
 	subdev_name = chan_cfg->inputs[input_index].subdev_name;
 	if (subdev_name == NULL)
 		return -1;

@@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
 	/* loop through the sub device list to get the sub device info */
 	for (i = 0; i < vpif_cfg->subdev_count; i++) {
 		subdev_info = &vpif_cfg->subdev_info[i];
-		if (!strcmp(subdev_info->name, subdev_name))
+		if (subdev_info && !strcmp(subdev_info->name, subdev_name))
 			return i;
 	}
 	return -1;

@@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct v4l2_async_notifier *notifier,
 {
 	int i;
 
+	for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+		struct v4l2_async_subdev *_asd = vpif_obj.config->asd[i];
+		const struct device_node *node = _asd->match.of.node;
+
+		if (node == subdev->of_node) {
+			vpif_obj.sd[i] = subdev;
+			vpif_obj.config->chan_config->inputs[i].subdev_name =
+				(char *)subdev->of_node->full_name;
+			vpif_dbg(2, debug,
+				 "%s: setting input %d subdev_name = %s\n",
+				 __func__, i, subdev->of_node->full_name);
+			return 0;
+		}
+	}
+
 	for (i = 0; i < vpif_obj.config->subdev_count; i++)
 		if (!strcmp(vpif_obj.config->subdev_info[i].name,
 			    subdev->name)) {

@@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct v4l2_async_notifier *notifier)
 	return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+	struct device_node *endpoint = NULL;
+	struct v4l2_of_endpoint bus_cfg;
+	struct vpif_capture_config *pdata;
+	struct vpif_subdev_info *sdinfo;
+	struct vpif_capture_chan_config *chan;
+	unsigned int i;
+
+	dev_dbg(&pdev->dev, "vpif_get_pdata\n");
+
+	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+		return pdev->dev.platform_data;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+	pdata->subdev_info =
+		devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
+			     VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+	if (!pdata->subdev_info)
+		return NULL;
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	for (i = 0; ; i++) {
+		struct device_node *rem;
+		unsigned int flags;
+		int err;
+

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1482

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +6 -0

+		endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
+						      endpoint);
+		if (!endpoint)
+			break;
+
+		sdinfo = &pdata->subdev_info[i];

subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1490

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c +8 -0

+		chan = &pdata->chan_config[i];
+		chan->inputs = devm_kzalloc(&pdev->dev,
+					    sizeof(*chan->inputs) *
+					    VPIF_DISPLAY_MAX_CHANNELS,
+					    GFP_KERNEL);
+
+		chan->input_count++;
+		chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;

I wonder what's the purpose of using index i on this array as well.

If you use that to access a corresponding entry in a different array, I'd just create a struct that contains the port configuration and the async sub-device. The omap3isp driver does that, for instance; see isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if you're interested. Up to you.

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1495

Quoted 5 lines

drivers/media/platform/davinci/vpif_capture.c +5 -0

+		chan->inputs[i].input.std = V4L2_STD_ALL;
+		chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
+
+		/* FIXME: need a new property? ch0:composite ch1: s-video */
+		if (i == 0)

Can you assume that the first endopoint has got a particular kind of input? What if it's not connected?

If this is a different physical port (not in the meaning another) in the device, I'd use the reg property for this. Please see Documentation/devicetree/bindings/media/video-interfaces.txt .

Quoted 30 lines

drivers/media/platform/davinci/vpif_capture.c +30 -0

+			chan->inputs[i].input_route = INPUT_CVBS_VI2B;
+		else
+			chan->inputs[i].input_route = INPUT_SVIDEO_VI2C_VI1C;
+		chan->inputs[i].output_route = OUTPUT_10BIT_422_EMBEDDED_SYNC;
+
+		err = v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+		if (err) {
+			dev_err(&pdev->dev, "Could not parse the endpoint\n");
+			goto done;
+		}
+		dev_dbg(&pdev->dev, "Endpoint %s, bus_width = %d\n",
+			endpoint->full_name, bus_cfg.bus.parallel.bus_width);
+		flags = bus_cfg.bus.parallel.flags;
+
+		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+			chan->vpif_if.hd_pol = 1;
+
+		if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
+			chan->vpif_if.vd_pol = 1;
+
+		chan->vpif_if.if_type = VPIF_IF_BT656;
+		rem = of_graph_get_remote_port_parent(endpoint);
+		if (!rem) {
+			dev_dbg(&pdev->dev, "Remote device at %s not found\n",
+				endpoint->full_name);
+			goto done;
+		}
+
+		dev_dbg(&pdev->dev, "Remote device %s, %s found\n",
+			rem->name, rem->full_name);

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1530

Quoted 5 lines

drivers/media/platform/davinci/vpif_capture.c +5 -0

+		sdinfo->name = rem->full_name;
+
+		pdata->asd[i] = devm_kzalloc(&pdev->dev,
+					     sizeof(struct v4l2_async_subdev),
+					     GFP_KERNEL);

Do you ensure somewhere that i isn't overrunning the pdata->asd[] array? It's got VPIF_CAPTURE_MAX_CHANNELS entries.

Hi Sakari,

Quoted 6 lines · Sakari Ailus

Quoted 3 lines · Kevin Hilman

Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from)

vpif_capture_get_pdata and "largely"?

Yes, thanks.

Quoted 15 lines

Quoted 15 lines

am437x-vpfe.c

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
 include/media/davinci/vpif_types.h            |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

drivers/media/platform/davinci/vpif_capture.c +0 -0

index 94ee6cf03f02..47a4699157e7 100644
@@ -26,6 +26,8 @@
 #include <linux/slab.h>
 

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c line 30

Quoted 3 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-of.h>
+#include <media/i2c/tvp514x.h>

Do you need this header?

Yes, based on discussion with Hans, since there is no DT binding for selecting the input pins of the TVP514x, I have to select it in the driver, so I need the defines from this header. More on this below...

Quoted 80 lines

Quoted 80 lines

drivers/media/platform/davinci/vpif_capture.c +51 -1

 #include "vpif.h"
 #include "vpif_capture.h"

@@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
 
 	vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+	if (!chan_cfg)
+		return -1;
+	if (input_index >= chan_cfg->input_count)
+		return -1;
 	subdev_name = chan_cfg->inputs[input_index].subdev_name;
 	if (subdev_name == NULL)
 		return -1;

@@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
 	/* loop through the sub device list to get the sub device info */
 	for (i = 0; i < vpif_cfg->subdev_count; i++) {
 		subdev_info = &vpif_cfg->subdev_info[i];
-		if (!strcmp(subdev_info->name, subdev_name))
+		if (subdev_info && !strcmp(subdev_info->name, subdev_name))
 			return i;
 	}
 	return -1;

@@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct v4l2_async_notifier *notifier,
 {
 	int i;
 
+	for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+		struct v4l2_async_subdev *_asd = vpif_obj.config->asd[i];
+		const struct device_node *node = _asd->match.of.node;
+
+		if (node == subdev->of_node) {
+			vpif_obj.sd[i] = subdev;
+			vpif_obj.config->chan_config->inputs[i].subdev_name =
+				(char *)subdev->of_node->full_name;
+			vpif_dbg(2, debug,
+				 "%s: setting input %d subdev_name = %s\n",
+				 __func__, i, subdev->of_node->full_name);
+			return 0;
+		}
+	}
+
 	for (i = 0; i < vpif_obj.config->subdev_count; i++)
 		if (!strcmp(vpif_obj.config->subdev_info[i].name,
 			    subdev->name)) {

@@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct v4l2_async_notifier *notifier)
 	return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+	struct device_node *endpoint = NULL;
+	struct v4l2_of_endpoint bus_cfg;
+	struct vpif_capture_config *pdata;
+	struct vpif_subdev_info *sdinfo;
+	struct vpif_capture_chan_config *chan;
+	unsigned int i;
+
+	dev_dbg(&pdev->dev, "vpif_get_pdata\n");
+
+	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+		return pdev->dev.platform_data;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+	pdata->subdev_info =
+		devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
+			     VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+	if (!pdata->subdev_info)
+		return NULL;
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	for (i = 0; ; i++) {
+		struct device_node *rem;
+		unsigned int flags;
+		int err;
+

Quoted 9 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1482

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +6 -0

+		endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
+						      endpoint);
+		if (!endpoint)
+			break;
+
+		sdinfo = &pdata->subdev_info[i];

subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.

Right, I need to make the loop only go for a max of VPIF_CAPTURE_MAX_CHANNELS iterations.

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1490

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c +8 -0

+		chan = &pdata->chan_config[i];
+		chan->inputs = devm_kzalloc(&pdev->dev,
+					    sizeof(*chan->inputs) *
+					    VPIF_DISPLAY_MAX_CHANNELS,
+					    GFP_KERNEL);
+
+		chan->input_count++;
+		chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;

I wonder what's the purpose of using index i on this array as well.

The number of endpoints in DT is the number of input channels configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)

Quoted 5 lines

If you use that to access a corresponding entry in a different array, I'd just create a struct that contains the port configuration and the async sub-device. The omap3isp driver does that, for instance; see isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if you're interested. Up to you.

OK, I'll have a look at that driver. The goal here with this series is just to get this working with DT, but also not break the existing legacy platform_device support, so I'm trying not to mess with the driver-interal data structures too much.

Quoted 9 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1495

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +5 -0

+		chan->inputs[i].input.std = V4L2_STD_ALL;
+		chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
+
+		/* FIXME: need a new property? ch0:composite ch1: s-video */
+		if (i == 0)

Can you assume that the first endopoint has got a particular kind of input? What if it's not connected?

On all the boards I know of (there aren't many using this SoC), it's a safe assumption.

Quoted 3 lines

If this is a different physical port (not in the meaning another) in the device, I'd use the reg property for this. Please see Documentation/devicetree/bindings/media/video-interfaces.txt .

My understanding (which is admittedly somewhat fuzzy) of the TVP514x is that it's not physically a different port. Instead, it's just telling the TVP514x which pin(s) will be active inputs (and what kind of signal will be present.)

I'm open to a better way to describe this input select from DT, but based on what I heard from Hans, there isn't currently a good way to do that except for in the driver:
(c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)

Based on further discussion in that thread, it sounds like there may be a way forward coming soon, and I'll be glad to switch to that when it arrives.

Hi Kevin,

Quoted 152 lines · Kevin Hilman

Hi Sakari,

Quoted 6 lines · Sakari Ailus

Quoted 3 lines · Kevin Hilman

Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from)

vpif_capture_get_pdata and "largely"?

Yes, thanks.

Quoted 15 lines

Quoted 15 lines

am437x-vpfe.c

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
 include/media/davinci/vpif_types.h            |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

drivers/media/platform/davinci/vpif_capture.c +0 -0

index 94ee6cf03f02..47a4699157e7 100644
@@ -26,6 +26,8 @@
 #include <linux/slab.h>
 

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c line 30

Quoted 3 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-of.h>
+#include <media/i2c/tvp514x.h>

Do you need this header?

Yes, based on discussion with Hans, since there is no DT binding for selecting the input pins of the TVP514x, I have to select it in the driver, so I need the defines from this header. More on this below...

Quoted 80 lines

Quoted 80 lines

drivers/media/platform/davinci/vpif_capture.c +51 -1

 #include "vpif.h"
 #include "vpif_capture.h"

@@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
 
 	vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+	if (!chan_cfg)
+		return -1;
+	if (input_index >= chan_cfg->input_count)
+		return -1;
 	subdev_name = chan_cfg->inputs[input_index].subdev_name;
 	if (subdev_name == NULL)
 		return -1;

@@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
 	/* loop through the sub device list to get the sub device info */
 	for (i = 0; i < vpif_cfg->subdev_count; i++) {
 		subdev_info = &vpif_cfg->subdev_info[i];
-		if (!strcmp(subdev_info->name, subdev_name))
+		if (subdev_info && !strcmp(subdev_info->name, subdev_name))
 			return i;
 	}
 	return -1;

@@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct v4l2_async_notifier *notifier,
 {
 	int i;
 
+	for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+		struct v4l2_async_subdev *_asd = vpif_obj.config->asd[i];
+		const struct device_node *node = _asd->match.of.node;
+
+		if (node == subdev->of_node) {
+			vpif_obj.sd[i] = subdev;
+			vpif_obj.config->chan_config->inputs[i].subdev_name =
+				(char *)subdev->of_node->full_name;
+			vpif_dbg(2, debug,
+				 "%s: setting input %d subdev_name = %s\n",
+				 __func__, i, subdev->of_node->full_name);
+			return 0;
+		}
+	}
+
 	for (i = 0; i < vpif_obj.config->subdev_count; i++)
 		if (!strcmp(vpif_obj.config->subdev_info[i].name,
 			    subdev->name)) {

@@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct v4l2_async_notifier *notifier)
 	return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+	struct device_node *endpoint = NULL;
+	struct v4l2_of_endpoint bus_cfg;
+	struct vpif_capture_config *pdata;
+	struct vpif_subdev_info *sdinfo;
+	struct vpif_capture_chan_config *chan;
+	unsigned int i;
+
+	dev_dbg(&pdev->dev, "vpif_get_pdata\n");
+
+	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+		return pdev->dev.platform_data;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+	pdata->subdev_info =
+		devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
+			     VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+	if (!pdata->subdev_info)
+		return NULL;
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	for (i = 0; ; i++) {
+		struct device_node *rem;
+		unsigned int flags;
+		int err;
+

Quoted 9 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1482

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +6 -0

+		endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
+						      endpoint);
+		if (!endpoint)
+			break;
+
+		sdinfo = &pdata->subdev_info[i];

subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.

Right, I need to make the loop only go for a max of VPIF_CAPTURE_MAX_CHANNELS iterations.

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1490

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c +8 -0

+		chan = &pdata->chan_config[i];
+		chan->inputs = devm_kzalloc(&pdev->dev,
+					    sizeof(*chan->inputs) *
+					    VPIF_DISPLAY_MAX_CHANNELS,
+					    GFP_KERNEL);
+
+		chan->input_count++;
+		chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;

I wonder what's the purpose of using index i on this array as well.

The number of endpoints in DT is the number of input channels configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)

Quoted 5 lines

If you use that to access a corresponding entry in a different array, I'd just create a struct that contains the port configuration and the async sub-device. The omap3isp driver does that, for instance; see isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if you're interested. Up to you.

Quoted 4 lines

OK, I'll have a look at that driver. The goal here with this series is just to get this working with DT, but also not break the existing legacy platform_device support, so I'm trying not to mess with the driver-interal data structures too much.

Ack.

Quoted 22 lines

Quoted 9 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1495

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +5 -0

+		chan->inputs[i].input.std = V4L2_STD_ALL;
+		chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
+
+		/* FIXME: need a new property? ch0:composite ch1: s-video */
+		if (i == 0)

Can you assume that the first endopoint has got a particular kind of input? What if it's not connected?

On all the boards I know of (there aren't many using this SoC), it's a safe assumption.

Quoted 3 lines

If this is a different physical port (not in the meaning another) in the device, I'd use the reg property for this. Please see Documentation/devicetree/bindings/media/video-interfaces.txt .

My understanding (which is admittedly somewhat fuzzy) of the TVP514x is that it's not physically a different port. Instead, it's just telling the TVP514x which pin(s) will be active inputs (and what kind of signal will be present.)

Quoted 8 lines

I'm open to a better way to describe this input select from DT, but based on what I heard from Hans, there isn't currently a good way to do that except for in the driver:
(c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)

Based on further discussion in that thread, it sounds like there may be a way forward coming soon, and I'll be glad to switch to that when it arrives.

I'm not sure that properly supporting connectors will provide any help here.

Looking at the s_routing() API, it's the calling driver that has to be aware of sub-device specific function parameters. As such it's not a very good idea to require that a driver is aware of the value range of another driver's parameter. I wonder if a simple enumeration interface would help here --- if I understand correctly, the purpose is just to provide a way to choose the input using VIDIOC_S_INPUT.

I guess that's somehow ok as long as you have no other combinations of these devices but this is hardly future-proof. (And certainly not a problem created by this patch.)

It'd be still nice to fix that as presumably we don't have the option of reworking how we expect the device tree to look like.

Quoted 202 lines · Sakari Ailus

Hi Kevin,

Quoted 152 lines · Kevin Hilman

Hi Sakari,

Quoted 6 lines · Sakari Ailus

Quoted 3 lines · Kevin Hilman

Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from)

vpif_capture_get_pdata and "largely"?

Yes, thanks.

Quoted 15 lines

Quoted 15 lines

am437x-vpfe.c

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
 include/media/davinci/vpif_types.h            |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

drivers/media/platform/davinci/vpif_capture.c +0 -0

index 94ee6cf03f02..47a4699157e7 100644
@@ -26,6 +26,8 @@
 #include <linux/slab.h>
 

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c line 30

Quoted 3 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-of.h>
+#include <media/i2c/tvp514x.h>

Do you need this header?

Yes, based on discussion with Hans, since there is no DT binding for selecting the input pins of the TVP514x, I have to select it in the driver, so I need the defines from this header. More on this below...

Quoted 80 lines

Quoted 80 lines

drivers/media/platform/davinci/vpif_capture.c +51 -1

 #include "vpif.h"
 #include "vpif_capture.h"

@@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
 
 	vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+	if (!chan_cfg)
+		return -1;
+	if (input_index >= chan_cfg->input_count)
+		return -1;
 	subdev_name = chan_cfg->inputs[input_index].subdev_name;
 	if (subdev_name == NULL)
 		return -1;

@@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
 	/* loop through the sub device list to get the sub device info */
 	for (i = 0; i < vpif_cfg->subdev_count; i++) {
 		subdev_info = &vpif_cfg->subdev_info[i];
-		if (!strcmp(subdev_info->name, subdev_name))
+		if (subdev_info && !strcmp(subdev_info->name, subdev_name))
 			return i;
 	}
 	return -1;

@@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct v4l2_async_notifier *notifier,
 {
 	int i;
 
+	for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+		struct v4l2_async_subdev *_asd = vpif_obj.config->asd[i];
+		const struct device_node *node = _asd->match.of.node;
+
+		if (node == subdev->of_node) {
+			vpif_obj.sd[i] = subdev;
+			vpif_obj.config->chan_config->inputs[i].subdev_name =
+				(char *)subdev->of_node->full_name;
+			vpif_dbg(2, debug,
+				 "%s: setting input %d subdev_name = %s\n",
+				 __func__, i, subdev->of_node->full_name);
+			return 0;
+		}
+	}
+
 	for (i = 0; i < vpif_obj.config->subdev_count; i++)
 		if (!strcmp(vpif_obj.config->subdev_info[i].name,
 			    subdev->name)) {

@@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct v4l2_async_notifier *notifier)
 	return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+	struct device_node *endpoint = NULL;
+	struct v4l2_of_endpoint bus_cfg;
+	struct vpif_capture_config *pdata;
+	struct vpif_subdev_info *sdinfo;
+	struct vpif_capture_chan_config *chan;
+	unsigned int i;
+
+	dev_dbg(&pdev->dev, "vpif_get_pdata\n");
+
+	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+		return pdev->dev.platform_data;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+	pdata->subdev_info =
+		devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
+			     VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+	if (!pdata->subdev_info)
+		return NULL;
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	for (i = 0; ; i++) {
+		struct device_node *rem;
+		unsigned int flags;
+		int err;
+

Quoted 9 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1482

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +6 -0

+		endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
+						      endpoint);
+		if (!endpoint)
+			break;
+
+		sdinfo = &pdata->subdev_info[i];

subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.

Right, I need to make the loop only go for a max of VPIF_CAPTURE_MAX_CHANNELS iterations.

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1490

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c +8 -0

+		chan = &pdata->chan_config[i];
+		chan->inputs = devm_kzalloc(&pdev->dev,
+					    sizeof(*chan->inputs) *
+					    VPIF_DISPLAY_MAX_CHANNELS,
+					    GFP_KERNEL);
+
+		chan->input_count++;
+		chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;

I wonder what's the purpose of using index i on this array as well.

The number of endpoints in DT is the number of input channels configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)

Quoted 5 lines

If you use that to access a corresponding entry in a different array, I'd just create a struct that contains the port configuration and the async sub-device. The omap3isp driver does that, for instance; see isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if you're interested. Up to you.

Quoted 4 lines

OK, I'll have a look at that driver. The goal here with this series is just to get this working with DT, but also not break the existing legacy platform_device support, so I'm trying not to mess with the driver-interal data structures too much.

Ack.

Quoted 22 lines

Quoted 9 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1495

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +5 -0

+		chan->inputs[i].input.std = V4L2_STD_ALL;
+		chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
+
+		/* FIXME: need a new property? ch0:composite ch1: s-video */
+		if (i == 0)

Can you assume that the first endopoint has got a particular kind of input? What if it's not connected?

On all the boards I know of (there aren't many using this SoC), it's a safe assumption.

Quoted 3 lines

If this is a different physical port (not in the meaning another) in the device, I'd use the reg property for this. Please see Documentation/devicetree/bindings/media/video-interfaces.txt .

My understanding (which is admittedly somewhat fuzzy) of the TVP514x is that it's not physically a different port. Instead, it's just telling the TVP514x which pin(s) will be active inputs (and what kind of signal will be present.)

Quoted 8 lines

I'm open to a better way to describe this input select from DT, but based on what I heard from Hans, there isn't currently a good way to do that except for in the driver:
(c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)

Based on further discussion in that thread, it sounds like there may be a way forward coming soon, and I'll be glad to switch to that when it arrives.

I'm not sure that properly supporting connectors will provide any help here.

Looking at the s_routing() API, it's the calling driver that has to be aware of sub-device specific function parameters. As such it's not a very good idea to require that a driver is aware of the value range of another driver's parameter. I wonder if a simple enumeration interface would help here --- if I understand correctly, the purpose is just to provide a way to choose the input using VIDIOC_S_INPUT.

Quoted 3 lines

I guess that's somehow ok as long as you have no other combinations of these devices but this is hardly future-proof. (And certainly not a problem created by this patch.)

Yeah, this is far from future proof.

Quoted 2 lines

It'd be still nice to fix that as presumably we don't have the option of reworking how we expect the device tree to look like.

Agreed.

I'm just hoping someone can shed som light on "how we expect the device tree to look". ;)

Quoted 31 lines · Kevin Hilman

Quoted 30 lines · Sakari Ailus

Quoted 29 lines · Kevin Hilman

Quoted 27 lines · Sakari Ailus

Quoted 19 lines · Kevin Hilman

Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from) am437x-vpfe.c

Signed-off-by Kevin Hilman <[email protected]>

---

 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
 include/media/davinci/vpif_types.h     
       |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/media/platform/davinci/vpif_capture.c index 94ee6cf03f02..47a4699157e7 100644

drivers/media/platform/davinci/vpif_capture.c line 30

Quoted 5 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

@@ -26,6 +26,8 @@
 #include <linux/slab.h>
 
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-of.h>
+#include <media/i2c/tvp514x.h>

Do you need this header?

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Yes, based on discussion with Hans, since there is no DT binding for selecting the input pins of the TVP514x, I have to select it in the driver, so I need the defines from this header. More on this below...

Quoted 29 lines

Quoted 29 lines

Quoted 29 lines

Quoted 29 lines

Quoted 29 lines

drivers/media/platform/davinci/vpif_capture.c +5 -1

 #include "vpif.h"
 #include "vpif_capture.h"

@@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
 
 	vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+	if (!chan_cfg)
+		return -1;
+	if (input_index >= chan_cfg->input_count)
+		return -1;
 	subdev_name = chan_cfg->inputs[input_index].subdev_name;
 	if (subdev_name == NULL)
 		return -1;

@@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
 	/* loop through the sub device list to get the sub device info
 	*/
 	for (i = 0; i < vpif_cfg->subdev_count; i++) {
 		subdev_info = &vpif_cfg->subdev_info[i];
-		if (!strcmp(subdev_info->name, subdev_name))
+		if (subdev_info && !strcmp(subdev_info->name,
 subdev_name))
 			return i;

drivers/media/platform/davinci/vpif_capture.c +0 -0

 	}
 	return -1;

@@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct

v4l2_async_notifier *notifier,> >> >>
{
int i;

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c +8 -2

+	for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+		struct v4l2_async_subdev *_asd = vpif_obj.config
->asd[i];
+		const struct device_node *node = _asd->match.of.node;
+
+		if (node == subdev->of_node) {
+			vpif_obj.sd[i] = subdev;
+			vpif_obj.config->chan_config
->inputs[i].subdev_name =
+				(char *)subdev->of_node->full_name;

Quoted 110 lines

Quoted 110 lines

Quoted 81 lines

Quoted 49 lines

Quoted 49 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

+			vpif_dbg(2, debug,
+				 "%s: setting input %d subdev_name =

%s\n",

drivers/media/platform/davinci/vpif_capture.c +37 -1

+				 __func__, i, subdev->of_node
->full_name);
+			return 0;
+		}
+	}
+
 	for (i = 0; i < vpif_obj.config->subdev_count; i++)
 		if (!strcmp(vpif_obj.config->subdev_info[i].name,
 			    subdev->name)) {

@@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct
 v4l2_async_notifier *notifier)
 	return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+	struct device_node *endpoint = NULL;
+	struct v4l2_of_endpoint bus_cfg;
+	struct vpif_capture_config *pdata;
+	struct vpif_subdev_info *sdinfo;
+	struct vpif_capture_chan_config *chan;
+	unsigned int i;
+
+	dev_dbg(&pdev->dev, "vpif_get_pdata\n");
+
+	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+		return pdev->dev.platform_data;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+	pdata->subdev_info =
+		devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
+			     VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+	if (!pdata->subdev_info)
+		return NULL;
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	for (i = 0; ; i++) {
+		struct device_node *rem;
+		unsigned int flags;
+		int err;
+
+		endpoint = of_graph_get_next_endpoint(pdev

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct · line 1483

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +5 -1

->dev.of_node,
+						      endpoint);
+		if (!endpoint)
+			break;
+
+		sdinfo = &pdata->subdev_info[i];

subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.

Right, I need to make the loop only go for a max of VPIF_CAPTURE_MAX_CHANNELS iterations.

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct · line 1491

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c +8 -0

+		chan = &pdata->chan_config[i];
+		chan->inputs = devm_kzalloc(&pdev->dev,
+					    sizeof(*chan->inputs) *
+					    VPIF_DISPLAY_MAX_CHANNELS,
+					    GFP_KERNEL);
+
+		chan->input_count++;
+		chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;

I wonder what's the purpose of using index i on this array as well.

The number of endpoints in DT is the number of input channels configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)

Quoted 5 lines

If you use that to access a corresponding entry in a different array, I'd just create a struct that contains the port configuration and the async sub-device. The omap3isp driver does that, for instance; see isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if you're interested. Up to you.

Quoted 4 lines

OK, I'll have a look at that driver. The goal here with this series is just to get this working with DT, but also not break the existing legacy platform_device support, so I'm trying not to mess with the driver-interal data structures too much.

Ack.

Quoted 22 lines

Quoted 9 lines

drivers/media/platform/davinci/vpif_capture.c

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +4 -0

+		chan->inputs[i].input.std = V4L2_STD_ALL;
+		chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
+
+		/* FIXME: need a new property? ch0:composite ch1:

s-video */

drivers/media/platform/davinci/vpif_capture.c +1 -0

+		if (i == 0)

Can you assume that the first endopoint has got a particular kind of input? What if it's not connected?

On all the boards I know of (there aren't many using this SoC), it's a safe assumption.

Quoted 3 lines

If this is a different physical port (not in the meaning another) in the device, I'd use the reg property for this. Please see Documentation/devicetree/bindings/media/video-interfaces.txt .

My understanding (which is admittedly somewhat fuzzy) of the TVP514x is that it's not physically a different port. Instead, it's just telling the TVP514x which pin(s) will be active inputs (and what kind of signal will be present.)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

I'm open to a better way to describe this input select from DT, but based on what I heard from Hans, there isn't currently a good way to do that except for in the driver:
(c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)

Based on further discussion in that thread, it sounds like there may be a way forward coming soon, and I'll be glad to switch to that when it arrives.

Quoted 16 lines

Quoted 10 lines

I'm not sure that properly supporting connectors will provide any help here.

Looking at the s_routing() API, it's the calling driver that has to be aware of sub-device specific function parameters. As such it's not a very good idea to require that a driver is aware of the value range of another driver's parameter. I wonder if a simple enumeration interface would help here --- if I understand correctly, the purpose is just to provide a way to choose the input using VIDIOC_S_INPUT.

Quoted 3 lines

I guess that's somehow ok as long as you have no other combinations of these devices but this is hardly future-proof. (And certainly not a problem created by this patch.)

Yeah, this is far from future proof.

Quoted 7 lines

Quoted 2 lines

It'd be still nice to fix that as presumably we don't have the option of reworking how we expect the device tree to look like.

Agreed.

I'm just hoping someone can shed som light on "how we expect the device tree to look". ;)

:-)

For the tvp514x, do you need more than a single endpoint on the receiver side? Does the input that's selected affect the bus parameters?

If it doesn't, you could create a custom endpoint property for the possible input values. The s_routing() really should be fixed though, but that could be postponed I guess. There are quite a few drivers using it.

Hello,

Quoted 32 lines · On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:

Quoted 31 lines · Kevin Hilman

Quoted 30 lines · Sakari Ailus

Quoted 29 lines · Kevin Hilman

Quoted 27 lines · Sakari Ailus

Quoted 19 lines · Kevin Hilman

Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from) am437x-vpfe.c

Signed-off-by Kevin Hilman <[email protected]>

---

 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
 include/media/davinci/vpif_types.h     
       |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/media/platform/davinci/vpif_capture.c index 94ee6cf03f02..47a4699157e7 100644

drivers/media/platform/davinci/vpif_capture.c line 30

Quoted 5 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

@@ -26,6 +26,8 @@
 #include <linux/slab.h>
 
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-of.h>
+#include <media/i2c/tvp514x.h>

Do you need this header?

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Yes, based on discussion with Hans, since there is no DT binding for selecting the input pins of the TVP514x, I have to select it in the driver, so I need the defines from this header. More on this below...

That's really ugly :-( The problem should be fixed properly instead of adding one more offender.

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

drivers/media/platform/davinci/vpif_capture.c +5 -1

 #include "vpif.h"
 #include "vpif_capture.h"

@@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
 
 	vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+	if (!chan_cfg)
+		return -1;
+	if (input_index >= chan_cfg->input_count)
+		return -1;
 	subdev_name = chan_cfg->inputs[input_index].subdev_name;
 	if (subdev_name == NULL)
 		return -1;

@@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
 	/* loop through the sub device list to get the sub device info
 	*/
 	for (i = 0; i < vpif_cfg->subdev_count; i++) {
 		subdev_info = &vpif_cfg->subdev_info[i];
-		if (!strcmp(subdev_info->name, subdev_name))
+		if (subdev_info && !strcmp(subdev_info->name,
 subdev_name))
 			return i;

drivers/media/platform/davinci/vpif_capture.c +0 -0

 	}
 	return -1;

@@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct

v4l2_async_notifier *notifier,> >> >>
{
int i;

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c +8 -2

+	for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+		struct v4l2_async_subdev *_asd = vpif_obj.config
->asd[i];
+		const struct device_node *node = _asd->match.of.node;
+
+		if (node == subdev->of_node) {
+			vpif_obj.sd[i] = subdev;
+			vpif_obj.config->chan_config
->inputs[i].subdev_name =
+				(char *)subdev->of_node->full_name;

Can subdev_name be made const instead of blindly casting the full_name pointer ? If not this is probably unsafe, and if yes it should be done :-)

Quoted 110 lines

Quoted 110 lines

Quoted 110 lines

Quoted 81 lines

Quoted 49 lines

Quoted 49 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

+			vpif_dbg(2, debug,
+				 "%s: setting input %d subdev_name =

%s\n",

drivers/media/platform/davinci/vpif_capture.c +37 -1

+				 __func__, i, subdev->of_node
->full_name);
+			return 0;
+		}
+	}
+
 	for (i = 0; i < vpif_obj.config->subdev_count; i++)
 		if (!strcmp(vpif_obj.config->subdev_info[i].name,
 			    subdev->name)) {

@@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct
 v4l2_async_notifier *notifier)
 	return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+	struct device_node *endpoint = NULL;
+	struct v4l2_of_endpoint bus_cfg;
+	struct vpif_capture_config *pdata;
+	struct vpif_subdev_info *sdinfo;
+	struct vpif_capture_chan_config *chan;
+	unsigned int i;
+
+	dev_dbg(&pdev->dev, "vpif_get_pdata\n");
+
+	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+		return pdev->dev.platform_data;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+	pdata->subdev_info =
+		devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
+			     VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+	if (!pdata->subdev_info)
+		return NULL;
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	for (i = 0; ; i++) {
+		struct device_node *rem;
+		unsigned int flags;
+		int err;
+
+		endpoint = of_graph_get_next_endpoint(pdev

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct · line 1483

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +5 -1

->dev.of_node,
+						      endpoint);
+		if (!endpoint)
+			break;
+
+		sdinfo = &pdata->subdev_info[i];

subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.

Right, I need to make the loop only go for a max of VPIF_CAPTURE_MAX_CHANNELS iterations.

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct · line 1491

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c +8 -0

+		chan = &pdata->chan_config[i];
+		chan->inputs = devm_kzalloc(&pdev->dev,
+					    sizeof(*chan->inputs) *
+					    VPIF_DISPLAY_MAX_CHANNELS,
+					    GFP_KERNEL);
+
+		chan->input_count++;
+		chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;

I wonder what's the purpose of using index i on this array as well.

The number of endpoints in DT is the number of input channels configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)

Quoted 5 lines

If you use that to access a corresponding entry in a different array, I'd just create a struct that contains the port configuration and the async sub-device. The omap3isp driver does that, for instance; see isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if you're interested. Up to you.

Quoted 4 lines

OK, I'll have a look at that driver. The goal here with this series is just to get this working with DT, but also not break the existing legacy platform_device support, so I'm trying not to mess with the driver-interal data structures too much.

Ack.

Quoted 22 lines

Quoted 9 lines

drivers/media/platform/davinci/vpif_capture.c

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +4 -0

+		chan->inputs[i].input.std = V4L2_STD_ALL;
+		chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
+
+		/* FIXME: need a new property? ch0:composite ch1:

s-video */

drivers/media/platform/davinci/vpif_capture.c +1 -0

+		if (i == 0)

Can you assume that the first endopoint has got a particular kind of input? What if it's not connected?

On all the boards I know of (there aren't many using this SoC), it's a safe assumption.

Quoted 3 lines

If this is a different physical port (not in the meaning another) in the device, I'd use the reg property for this. Please see Documentation/devicetree/bindings/media/video-interfaces.txt .

My understanding (which is admittedly somewhat fuzzy) of the TVP514x is that it's not physically a different port. Instead, it's just telling the TVP514x which pin(s) will be active inputs (and what kind of signal will be present.)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

I'm open to a better way to describe this input select from DT, but based on what I heard from Hans, there isn't currently a good way to do that except for in the driver:
(c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)

Based on further discussion in that thread, it sounds like there may be a way forward coming soon, and I'll be glad to switch to that when it arrives.

I'm afraid I have to disappoint Hans here, I don't have code for that yet.

Quoted 29 lines

Quoted 16 lines

Quoted 10 lines

I'm not sure that properly supporting connectors will provide any help here.

Looking at the s_routing() API, it's the calling driver that has to be aware of sub-device specific function parameters. As such it's not a very good idea to require that a driver is aware of the value range of another driver's parameter. I wonder if a simple enumeration interface would help here --- if I understand correctly, the purpose is just to provide a way to choose the input using VIDIOC_S_INPUT.

Quoted 3 lines

I guess that's somehow ok as long as you have no other combinations of these devices but this is hardly future-proof. (And certainly not a problem created by this patch.)

Yeah, this is far from future proof.

Quoted 7 lines

Quoted 2 lines

It'd be still nice to fix that as presumably we don't have the option of reworking how we expect the device tree to look like.

Agreed.

I'm just hoping someone can shed som light on "how we expect the device tree to look". ;)

:-)

For the tvp514x, do you need more than a single endpoint on the receiver side? Does the input that's selected affect the bus parameters?

Quoted 3 lines

If it doesn't, you could create a custom endpoint property for the possible input values. The s_routing() really should be fixed though, but that could be postponed I guess. There are quite a few drivers using it.

There's two ways to look at s_routing() in my opinion, as the calling driver should really not hardcode any knowledge specific to a particular subdev. We can either have the calling driver discover the possible routing options at runtime through the subdev API, or modify the s_routing() API.

Quoted 35 lines · Laurent Pinchart

Hello,

Quoted 32 lines · On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:

Quoted 31 lines · Kevin Hilman

Quoted 30 lines · Sakari Ailus

Quoted 29 lines · Kevin Hilman

Quoted 27 lines · Sakari Ailus

Quoted 19 lines · Kevin Hilman

Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from) am437x-vpfe.c

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---

 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
 include/media/davinci/vpif_types.h     
       |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/media/platform/davinci/vpif_capture.c index 94ee6cf03f02..47a4699157e7 100644

drivers/media/platform/davinci/vpif_capture.c line 30

Quoted 5 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

@@ -26,6 +26,8 @@
 #include <linux/slab.h>
 
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-of.h>
+#include <media/i2c/tvp514x.h>

Do you need this header?

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Yes, based on discussion with Hans, since there is no DT binding for selecting the input pins of the TVP514x, I have to select it in the driver, so I need the defines from this header. More on this below...

That's really ugly :-( The problem should be fixed properly instead of adding one more offender.

Do you have time for that, Laurent? I don't. Until that time we just need to make do with this workaround.

Quoted 194 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

drivers/media/platform/davinci/vpif_capture.c +5 -1

 #include "vpif.h"
 #include "vpif_capture.h"

@@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
 
 	vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+	if (!chan_cfg)
+		return -1;
+	if (input_index >= chan_cfg->input_count)
+		return -1;
 	subdev_name = chan_cfg->inputs[input_index].subdev_name;
 	if (subdev_name == NULL)
 		return -1;

@@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
 	/* loop through the sub device list to get the sub device info
 	*/
 	for (i = 0; i < vpif_cfg->subdev_count; i++) {
 		subdev_info = &vpif_cfg->subdev_info[i];
-		if (!strcmp(subdev_info->name, subdev_name))
+		if (subdev_info && !strcmp(subdev_info->name,
 subdev_name))
 			return i;

drivers/media/platform/davinci/vpif_capture.c +0 -0

 	}
 	return -1;

@@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct

v4l2_async_notifier *notifier,> >> >>
{
int i;

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c +8 -2

+	for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+		struct v4l2_async_subdev *_asd = vpif_obj.config
->asd[i];
+		const struct device_node *node = _asd->match.of.node;
+
+		if (node == subdev->of_node) {
+			vpif_obj.sd[i] = subdev;
+			vpif_obj.config->chan_config
->inputs[i].subdev_name =
+				(char *)subdev->of_node->full_name;

Can subdev_name be made const instead of blindly casting the full_name pointer ? If not this is probably unsafe, and if yes it should be done :-)

Quoted 110 lines

Quoted 110 lines

Quoted 110 lines

Quoted 81 lines

Quoted 49 lines

Quoted 49 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

+			vpif_dbg(2, debug,
+				 "%s: setting input %d subdev_name =

%s\n",

drivers/media/platform/davinci/vpif_capture.c +37 -1

+				 __func__, i, subdev->of_node
->full_name);
+			return 0;
+		}
+	}
+
 	for (i = 0; i < vpif_obj.config->subdev_count; i++)
 		if (!strcmp(vpif_obj.config->subdev_info[i].name,
 			    subdev->name)) {

@@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct
 v4l2_async_notifier *notifier)
 	return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+	struct device_node *endpoint = NULL;
+	struct v4l2_of_endpoint bus_cfg;
+	struct vpif_capture_config *pdata;
+	struct vpif_subdev_info *sdinfo;
+	struct vpif_capture_chan_config *chan;
+	unsigned int i;
+
+	dev_dbg(&pdev->dev, "vpif_get_pdata\n");
+
+	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+		return pdev->dev.platform_data;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+	pdata->subdev_info =
+		devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
+			     VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+	if (!pdata->subdev_info)
+		return NULL;
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	for (i = 0; ; i++) {
+		struct device_node *rem;
+		unsigned int flags;
+		int err;
+
+		endpoint = of_graph_get_next_endpoint(pdev

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct · line 1483

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +5 -1

->dev.of_node,
+						      endpoint);
+		if (!endpoint)
+			break;
+
+		sdinfo = &pdata->subdev_info[i];

subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.

Right, I need to make the loop only go for a max of VPIF_CAPTURE_MAX_CHANNELS iterations.

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct · line 1491

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c +8 -0

+		chan = &pdata->chan_config[i];
+		chan->inputs = devm_kzalloc(&pdev->dev,
+					    sizeof(*chan->inputs) *
+					    VPIF_DISPLAY_MAX_CHANNELS,
+					    GFP_KERNEL);
+
+		chan->input_count++;
+		chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;

I wonder what's the purpose of using index i on this array as well.

The number of endpoints in DT is the number of input channels configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)

Quoted 5 lines

If you use that to access a corresponding entry in a different array, I'd just create a struct that contains the port configuration and the async sub-device. The omap3isp driver does that, for instance; see isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if you're interested. Up to you.

Quoted 4 lines

OK, I'll have a look at that driver. The goal here with this series is just to get this working with DT, but also not break the existing legacy platform_device support, so I'm trying not to mess with the driver-interal data structures too much.

Ack.

Quoted 22 lines

Quoted 9 lines

drivers/media/platform/davinci/vpif_capture.c

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +4 -0

+		chan->inputs[i].input.std = V4L2_STD_ALL;
+		chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
+
+		/* FIXME: need a new property? ch0:composite ch1:

s-video */

drivers/media/platform/davinci/vpif_capture.c +1 -0

+		if (i == 0)

Can you assume that the first endopoint has got a particular kind of input? What if it's not connected?

On all the boards I know of (there aren't many using this SoC), it's a safe assumption.

Quoted 3 lines

If this is a different physical port (not in the meaning another) in the device, I'd use the reg property for this. Please see Documentation/devicetree/bindings/media/video-interfaces.txt .

My understanding (which is admittedly somewhat fuzzy) of the TVP514x is that it's not physically a different port. Instead, it's just telling the TVP514x which pin(s) will be active inputs (and what kind of signal will be present.)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

I'm open to a better way to describe this input select from DT, but based on what I heard from Hans, there isn't currently a good way to do that except for in the driver:
(c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)

Based on further discussion in that thread, it sounds like there may be a way forward coming soon, and I'll be glad to switch to that when it arrives.

I'm afraid I have to disappoint Hans here, I don't have code for that yet.

Quoted 29 lines

Quoted 16 lines

Quoted 10 lines

I'm not sure that properly supporting connectors will provide any help here.

Looking at the s_routing() API, it's the calling driver that has to be aware of sub-device specific function parameters. As such it's not a very good idea to require that a driver is aware of the value range of another driver's parameter. I wonder if a simple enumeration interface would help here --- if I understand correctly, the purpose is just to provide a way to choose the input using VIDIOC_S_INPUT.

Quoted 3 lines

I guess that's somehow ok as long as you have no other combinations of these devices but this is hardly future-proof. (And certainly not a problem created by this patch.)

Yeah, this is far from future proof.

Quoted 7 lines

Quoted 2 lines

It'd be still nice to fix that as presumably we don't have the option of reworking how we expect the device tree to look like.

Agreed.

I'm just hoping someone can shed som light on "how we expect the device tree to look". ;)

:-)

For the tvp514x, do you need more than a single endpoint on the receiver side? Does the input that's selected affect the bus parameters?

Quoted 9 lines

Quoted 3 lines

If it doesn't, you could create a custom endpoint property for the possible input values. The s_routing() really should be fixed though, but that could be postponed I guess. There are quite a few drivers using it.

There's two ways to look at s_routing() in my opinion, as the calling driver should really not hardcode any knowledge specific to a particular subdev. We can either have the calling driver discover the possible routing options at runtime through the subdev API, or modify the s_routing() API.

Some historical perspective: s_routing was added well before the device tree was ever used for ARM. And at that time the vast majority of drivers were PCI or USB drivers, very few platform drivers existed (and those typically used sensors, not video receivers).

Before s_routing existed the situation was even worse.

Basically what s_routing does is a poor-man's device tree entry, telling the subdev how to route video or audio from connector to the output of the chip. Typically the card tables in PCI or USB drivers contain the correct arguments for s_routing. Of course, today we'd do that with the DT, but that was not an option years ago.

Quoted 255 lines · Hans Verkuil

Quoted 35 lines · Laurent Pinchart

Hello,

Quoted 32 lines · On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:

Quoted 31 lines · Kevin Hilman

Quoted 30 lines · Sakari Ailus

Quoted 29 lines · Kevin Hilman

Quoted 27 lines · Sakari Ailus

Quoted 19 lines · Kevin Hilman

Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from) am437x-vpfe.c

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---

 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
 include/media/davinci/vpif_types.h
       |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/media/platform/davinci/vpif_capture.c index 94ee6cf03f02..47a4699157e7 100644

drivers/media/platform/davinci/vpif_capture.c line 30

Quoted 5 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

@@ -26,6 +26,8 @@
 #include <linux/slab.h>
 
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-of.h>
+#include <media/i2c/tvp514x.h>

Do you need this header?

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Yes, based on discussion with Hans, since there is no DT binding for selecting the input pins of the TVP514x, I have to select it in the driver, so I need the defines from this header. More on this below...

That's really ugly :-( The problem should be fixed properly instead of adding one more offender.

Do you have time for that, Laurent? I don't. Until that time we just need to make do with this workaround.

Quoted 194 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

drivers/media/platform/davinci/vpif_capture.c +5 -1

 #include "vpif.h"
 #include "vpif_capture.h"

@@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
 
       vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+      if (!chan_cfg)
+              return -1;
+      if (input_index >= chan_cfg->input_count)
+              return -1;
       subdev_name = chan_cfg->inputs[input_index].subdev_name;
       if (subdev_name == NULL)
               return -1;

@@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
       /* loop through the sub device list to get the sub device info
       */
       for (i = 0; i < vpif_cfg->subdev_count; i++) {
               subdev_info = &vpif_cfg->subdev_info[i];
-              if (!strcmp(subdev_info->name, subdev_name))
+              if (subdev_info && !strcmp(subdev_info->name,
 subdev_name))
                       return i;

       }
       return -1;

drivers/media/platform/davinci/vpif_capture.c +0 -0

@@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct

v4l2_async_notifier *notifier,> >> >>

 {
       int i;

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c +8 -2

+      for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+              struct v4l2_async_subdev *_asd = vpif_obj.config
->asd[i];
+              const struct device_node *node = _asd->match.of.node;
+
+              if (node == subdev->of_node) {
+                      vpif_obj.sd[i] = subdev;
+                      vpif_obj.config->chan_config
->inputs[i].subdev_name =
+                              (char *)subdev->of_node->full_name;

Can subdev_name be made const instead of blindly casting the full_name pointer ? If not this is probably unsafe, and if yes it should be done :-)

Quoted 110 lines

Quoted 110 lines

Quoted 110 lines

Quoted 81 lines

Quoted 49 lines

Quoted 49 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

+                      vpif_dbg(2, debug,
+                               "%s: setting input %d subdev_name =

%s\n",

drivers/media/platform/davinci/vpif_capture.c +37 -1

+                               __func__, i, subdev->of_node
->full_name);
+                      return 0;
+              }
+      }
+
       for (i = 0; i < vpif_obj.config->subdev_count; i++)
               if (!strcmp(vpif_obj.config->subdev_info[i].name,
                           subdev->name)) {

@@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct
 v4l2_async_notifier *notifier)
       return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+      struct device_node *endpoint = NULL;
+      struct v4l2_of_endpoint bus_cfg;
+      struct vpif_capture_config *pdata;
+      struct vpif_subdev_info *sdinfo;
+      struct vpif_capture_chan_config *chan;
+      unsigned int i;
+
+      dev_dbg(&pdev->dev, "vpif_get_pdata\n");
+
+      if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+              return pdev->dev.platform_data;
+
+      pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+      if (!pdata)
+              return NULL;
+      pdata->subdev_info =
+              devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
+                           VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+      if (!pdata->subdev_info)
+              return NULL;
+      dev_dbg(&pdev->dev, "%s\n", __func__);
+
+      for (i = 0; ; i++) {
+              struct device_node *rem;
+              unsigned int flags;
+              int err;
+
+              endpoint = of_graph_get_next_endpoint(pdev

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct · line 1483

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +5 -1

->dev.of_node,
+                                                    endpoint);
+              if (!endpoint)
+                      break;
+
+              sdinfo = &pdata->subdev_info[i];

subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.

Right, I need to make the loop only go for a max of VPIF_CAPTURE_MAX_CHANNELS iterations.

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct · line 1491

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c +8 -0

+              chan = &pdata->chan_config[i];
+              chan->inputs = devm_kzalloc(&pdev->dev,
+                                          sizeof(*chan->inputs) *
+                                          VPIF_DISPLAY_MAX_CHANNELS,
+                                          GFP_KERNEL);
+
+              chan->input_count++;
+              chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;

I wonder what's the purpose of using index i on this array as well.

The number of endpoints in DT is the number of input channels configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)

Quoted 5 lines

If you use that to access a corresponding entry in a different array, I'd just create a struct that contains the port configuration and the async sub-device. The omap3isp driver does that, for instance; see isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if you're interested. Up to you.

Quoted 4 lines

OK, I'll have a look at that driver. The goal here with this series is just to get this working with DT, but also not break the existing legacy platform_device support, so I'm trying not to mess with the driver-interal data structures too much.

Ack.

Quoted 22 lines

Quoted 9 lines

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +4 -0

+              chan->inputs[i].input.std = V4L2_STD_ALL;
+              chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
+
+              /* FIXME: need a new property? ch0:composite ch1:

s-video */
+ if (i == 0)

Can you assume that the first endopoint has got a particular kind of input? What if it's not connected?

On all the boards I know of (there aren't many using this SoC), it's a safe assumption.

Quoted 3 lines

If this is a different physical port (not in the meaning another) in the device, I'd use the reg property for this. Please see Documentation/devicetree/bindings/media/video-interfaces.txt .

My understanding (which is admittedly somewhat fuzzy) of the TVP514x is that it's not physically a different port. Instead, it's just telling the TVP514x which pin(s) will be active inputs (and what kind of signal will be present.)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

I'm open to a better way to describe this input select from DT, but based on what I heard from Hans, there isn't currently a good way to do that except for in the driver:
(c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)

Based on further discussion in that thread, it sounds like there may be a way forward coming soon, and I'll be glad to switch to that when it arrives.

I'm afraid I have to disappoint Hans here, I don't have code for that yet.

Quoted 29 lines

Quoted 16 lines

Quoted 10 lines

I'm not sure that properly supporting connectors will provide any help here.

Looking at the s_routing() API, it's the calling driver that has to be aware of sub-device specific function parameters. As such it's not a very good idea to require that a driver is aware of the value range of another driver's parameter. I wonder if a simple enumeration interface would help here --- if I understand correctly, the purpose is just to provide a way to choose the input using VIDIOC_S_INPUT.

Quoted 3 lines

I guess that's somehow ok as long as you have no other combinations of these devices but this is hardly future-proof. (And certainly not a problem created by this patch.)

Yeah, this is far from future proof.

Quoted 7 lines

Quoted 2 lines

It'd be still nice to fix that as presumably we don't have the option of reworking how we expect the device tree to look like.

Agreed.

I'm just hoping someone can shed som light on "how we expect the device tree to look". ;)

:-)

For the tvp514x, do you need more than a single endpoint on the receiver side? Does the input that's selected affect the bus parameters?

Quoted 9 lines

Quoted 3 lines

If it doesn't, you could create a custom endpoint property for the possible input values. The s_routing() really should be fixed though, but that could be postponed I guess. There are quite a few drivers using it.

There's two ways to look at s_routing() in my opinion, as the calling driver should really not hardcode any knowledge specific to a particular subdev. We can either have the calling driver discover the possible routing options at runtime through the subdev API, or modify the s_routing() API.

Some historical perspective: s_routing was added well before the device tree was ever used for ARM. And at that time the vast majority of drivers were PCI or USB drivers, very few platform drivers existed (and those typically used sensors, not video receivers).

Quoted 7 lines

Before s_routing existed the situation was even worse.

Basically what s_routing does is a poor-man's device tree entry, telling the subdev how to route video or audio from connector to the output of the chip. Typically the card tables in PCI or USB drivers contain the correct arguments for s_routing. Of course, today we'd do that with the DT, but that was not an option years ago.

So I'm still confused on the path forward here.

I do not have the time (or the V4L2 knowledge/experience) to rework the V4L2 internals to make this work, but I'm happy to test if someone else is working on it.

In the meantime, what do we do with this series? I have a couple minor things to fixup based on review comments, but other than that, the s_routing decision is blocking this from getting an update for use on DT platforms.

The alternative is to go the OMAP route for legacy drivers like this and just use pdata quirks for passing the legacy pdata (which has the input and output routes hard-coded in platform_data).

Quoted 276 lines · Kevin Hilman

Quoted 255 lines · Hans Verkuil

Quoted 35 lines · Laurent Pinchart

Hello,

Quoted 32 lines · On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:

Quoted 31 lines · Kevin Hilman

Quoted 30 lines · Sakari Ailus

Quoted 29 lines · Kevin Hilman

Quoted 27 lines · Sakari Ailus

Quoted 19 lines · Kevin Hilman

Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from) am437x-vpfe.c

Signed-off-by Kevin Hilman <[email protected]>

---

 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
 include/media/davinci/vpif_types.h
       |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

(quoted diff) +0 -0

b/drivers/media/platform/davinci/vpif_capture.c index 94ee6cf03f02..47a4699157e7 100644

drivers/media/platform/davinci/vpif_capture.c line 30

Quoted 5 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

@@ -26,6 +26,8 @@
 #include <linux/slab.h>
 
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-of.h>
+#include <media/i2c/tvp514x.h>

Do you need this header?

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Yes, based on discussion with Hans, since there is no DT binding for selecting the input pins of the TVP514x, I have to select it in the driver, so I need the defines from this header. More on this below...

That's really ugly :-( The problem should be fixed properly instead of adding one more offender.

Do you have time for that, Laurent? I don't. Until that time we just need to make do with this workaround.

Quoted 194 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

drivers/media/platform/davinci/vpif_capture.c +5 -1

 #include "vpif.h"
 #include "vpif_capture.h"

@@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
 
       vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+      if (!chan_cfg)
+              return -1;
+      if (input_index >= chan_cfg->input_count)
+              return -1;
       subdev_name = chan_cfg->inputs[input_index].subdev_name;
       if (subdev_name == NULL)
               return -1;

@@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
       /* loop through the sub device list to get the sub device info
       */
       for (i = 0; i < vpif_cfg->subdev_count; i++) {
               subdev_info = &vpif_cfg->subdev_info[i];
-              if (!strcmp(subdev_info->name, subdev_name))
+              if (subdev_info && !strcmp(subdev_info->name,
 subdev_name))
                       return i;

       }
       return -1;

drivers/media/platform/davinci/vpif_capture.c +0 -0

@@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct

v4l2_async_notifier *notifier,> >> >>

 {
       int i;

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c +8 -2

+      for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+              struct v4l2_async_subdev *_asd = vpif_obj.config
->asd[i];
+              const struct device_node *node = _asd->match.of.node;
+
+              if (node == subdev->of_node) {
+                      vpif_obj.sd[i] = subdev;
+                      vpif_obj.config->chan_config
->inputs[i].subdev_name =
+                              (char *)subdev->of_node->full_name;

Can subdev_name be made const instead of blindly casting the full_name pointer ? If not this is probably unsafe, and if yes it should be done :-)

Quoted 110 lines

Quoted 110 lines

Quoted 110 lines

Quoted 81 lines

Quoted 49 lines

Quoted 49 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

+                      vpif_dbg(2, debug,
+                               "%s: setting input %d subdev_name =

%s\n",

drivers/media/platform/davinci/vpif_capture.c +37 -1

+                               __func__, i, subdev->of_node
->full_name);
+                      return 0;
+              }
+      }
+
       for (i = 0; i < vpif_obj.config->subdev_count; i++)
               if (!strcmp(vpif_obj.config->subdev_info[i].name,
                           subdev->name)) {

@@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct
 v4l2_async_notifier *notifier)
       return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+      struct device_node *endpoint = NULL;
+      struct v4l2_of_endpoint bus_cfg;
+      struct vpif_capture_config *pdata;
+      struct vpif_subdev_info *sdinfo;
+      struct vpif_capture_chan_config *chan;
+      unsigned int i;
+
+      dev_dbg(&pdev->dev, "vpif_get_pdata\n");
+
+      if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+              return pdev->dev.platform_data;
+
+      pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+      if (!pdata)
+              return NULL;
+      pdata->subdev_info =
+              devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
+                           VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+      if (!pdata->subdev_info)
+              return NULL;
+      dev_dbg(&pdev->dev, "%s\n", __func__);
+
+      for (i = 0; ; i++) {
+              struct device_node *rem;
+              unsigned int flags;
+              int err;
+
+              endpoint = of_graph_get_next_endpoint(pdev

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct · line 1483

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +5 -1

->dev.of_node,
+                                                    endpoint);
+              if (!endpoint)
+                      break;
+
+              sdinfo = &pdata->subdev_info[i];

subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.

Right, I need to make the loop only go for a max of VPIF_CAPTURE_MAX_CHANNELS iterations.

Quoted 10 lines

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct · line 1491

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c +8 -0

+              chan = &pdata->chan_config[i];
+              chan->inputs = devm_kzalloc(&pdev->dev,
+                                          sizeof(*chan->inputs) *
+                                          VPIF_DISPLAY_MAX_CHANNELS,
+                                          GFP_KERNEL);
+
+              chan->input_count++;
+              chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;

I wonder what's the purpose of using index i on this array as well.

The number of endpoints in DT is the number of input channels configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)

Quoted 5 lines

If you use that to access a corresponding entry in a different array, I'd just create a struct that contains the port configuration and the async sub-device. The omap3isp driver does that, for instance; see isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if you're interested. Up to you.

Quoted 4 lines

OK, I'll have a look at that driver. The goal here with this series is just to get this working with DT, but also not break the existing legacy platform_device support, so I'm trying not to mess with the driver-interal data structures too much.

Ack.

Quoted 22 lines

Quoted 9 lines

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +4 -0

+              chan->inputs[i].input.std = V4L2_STD_ALL;
+              chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
+
+              /* FIXME: need a new property? ch0:composite ch1:

s-video */
+ if (i == 0)

Can you assume that the first endopoint has got a particular kind of input? What if it's not connected?

On all the boards I know of (there aren't many using this SoC), it's a safe assumption.

Quoted 3 lines

If this is a different physical port (not in the meaning another) in the device, I'd use the reg property for this. Please see Documentation/devicetree/bindings/media/video-interfaces.txt .

My understanding (which is admittedly somewhat fuzzy) of the TVP514x is that it's not physically a different port. Instead, it's just telling the TVP514x which pin(s) will be active inputs (and what kind of signal will be present.)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

I'm open to a better way to describe this input select from DT, but based on what I heard from Hans, there isn't currently a good way to do that except for in the driver:
(c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)

Based on further discussion in that thread, it sounds like there may be a way forward coming soon, and I'll be glad to switch to that when it arrives.

I'm afraid I have to disappoint Hans here, I don't have code for that yet.

Quoted 29 lines

Quoted 16 lines

Quoted 10 lines

I'm not sure that properly supporting connectors will provide any help here.

Looking at the s_routing() API, it's the calling driver that has to be aware of sub-device specific function parameters. As such it's not a very good idea to require that a driver is aware of the value range of another driver's parameter. I wonder if a simple enumeration interface would help here --- if I understand correctly, the purpose is just to provide a way to choose the input using VIDIOC_S_INPUT.

Quoted 3 lines

I guess that's somehow ok as long as you have no other combinations of these devices but this is hardly future-proof. (And certainly not a problem created by this patch.)

Yeah, this is far from future proof.

Quoted 7 lines

Quoted 2 lines

It'd be still nice to fix that as presumably we don't have the option of reworking how we expect the device tree to look like.

Agreed.

I'm just hoping someone can shed som light on "how we expect the device tree to look". ;)

:-)

For the tvp514x, do you need more than a single endpoint on the receiver side? Does the input that's selected affect the bus parameters?

Quoted 9 lines

Quoted 3 lines

If it doesn't, you could create a custom endpoint property for the possible input values. The s_routing() really should be fixed though, but that could be postponed I guess. There are quite a few drivers using it.

There's two ways to look at s_routing() in my opinion, as the calling driver should really not hardcode any knowledge specific to a particular subdev. We can either have the calling driver discover the possible routing options at runtime through the subdev API, or modify the s_routing() API.

Some historical perspective: s_routing was added well before the device tree was ever used for ARM. And at that time the vast majority of drivers were PCI or USB drivers, very few platform drivers existed (and those typically used sensors, not video receivers).

Quoted 7 lines

Before s_routing existed the situation was even worse.

Basically what s_routing does is a poor-man's device tree entry, telling the subdev how to route video or audio from connector to the output of the chip. Typically the card tables in PCI or USB drivers contain the correct arguments for s_routing. Of course, today we'd do that with the DT, but that was not an option years ago.

So I'm still confused on the path forward here.

I do not have the time (or the V4L2 knowledge/experience) to rework the V4L2 internals to make this work, but I'm happy to test if someone else is working on it.

In the meantime, what do we do with this series? I have a couple minor things to fixup based on review comments, but other than that, the s_routing decision is blocking this from getting an update for use on DT platforms.

Quoted 3 lines

The alternative is to go the OMAP route for legacy drivers like this and just use pdata quirks for passing the legacy pdata (which has the input and output routes hard-coded in platform_data).

Also, FYI, I have the same issue with the output/display side of this controller. It's using an I2C-connected adv7343, where the input and output routes are configured by the driver using s_routing, and the current code passes the routes in using platform_data.

Quoted 3 lines · Kevin Hilman

Allow getting of subdevs from DT ports and endpoints.

The _get_pdata() function was larely inspired by (i.e. stolen from)

vpif_capture_get_pdata and "largely"?

Quoted 15 lines

am437x-vpfe.c

Signed-off-by Kevin Hilman <[email protected]>

---
 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
 include/media/davinci/vpif_types.h            |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

drivers/media/platform/davinci/vpif_capture.c +0 -0

index 94ee6cf03f02..47a4699157e7 100644
@@ -26,6 +26,8 @@
 #include <linux/slab.h>
 

drivers/media/platform/davinci/vpif_capture.c line 30

Quoted 3 lines

drivers/media/platform/davinci/vpif_capture.c +2 -0

 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-of.h>
+#include <media/i2c/tvp514x.h>

Do you need this header?

Quoted 80 lines

drivers/media/platform/davinci/vpif_capture.c +51 -1

 #include "vpif.h"
 #include "vpif_capture.h"

@@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
 
 	vpif_dbg(2, debug, "vpif_input_to_subdev\n");
 
+	if (!chan_cfg)
+		return -1;
+	if (input_index >= chan_cfg->input_count)
+		return -1;
 	subdev_name = chan_cfg->inputs[input_index].subdev_name;
 	if (subdev_name == NULL)
 		return -1;

@@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
 	/* loop through the sub device list to get the sub device info */
 	for (i = 0; i < vpif_cfg->subdev_count; i++) {
 		subdev_info = &vpif_cfg->subdev_info[i];
-		if (!strcmp(subdev_info->name, subdev_name))
+		if (subdev_info && !strcmp(subdev_info->name, subdev_name))
 			return i;
 	}
 	return -1;

@@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct v4l2_async_notifier *notifier,
 {
 	int i;
 
+	for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
+		struct v4l2_async_subdev *_asd = vpif_obj.config->asd[i];
+		const struct device_node *node = _asd->match.of.node;
+
+		if (node == subdev->of_node) {
+			vpif_obj.sd[i] = subdev;
+			vpif_obj.config->chan_config->inputs[i].subdev_name =
+				(char *)subdev->of_node->full_name;
+			vpif_dbg(2, debug,
+				 "%s: setting input %d subdev_name = %s\n",
+				 __func__, i, subdev->of_node->full_name);
+			return 0;
+		}
+	}
+
 	for (i = 0; i < vpif_obj.config->subdev_count; i++)
 		if (!strcmp(vpif_obj.config->subdev_info[i].name,
 			    subdev->name)) {

@@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct v4l2_async_notifier *notifier)
 	return vpif_probe_complete();
 }
 
+static struct vpif_capture_config *
+vpif_capture_get_pdata(struct platform_device *pdev)
+{
+	struct device_node *endpoint = NULL;
+	struct v4l2_of_endpoint bus_cfg;
+	struct vpif_capture_config *pdata;
+	struct vpif_subdev_info *sdinfo;
+	struct vpif_capture_chan_config *chan;
+	unsigned int i;
+
+	dev_dbg(&pdev->dev, "vpif_get_pdata\n");
+
+	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+		return pdev->dev.platform_data;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+	pdata->subdev_info =
+		devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
+			     VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+	if (!pdata->subdev_info)
+		return NULL;
+	dev_dbg(&pdev->dev, "%s\n", __func__);
+
+	for (i = 0; ; i++) {
+		struct device_node *rem;
+		unsigned int flags;
+		int err;
+

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1482

Quoted 6 lines

drivers/media/platform/davinci/vpif_capture.c +6 -0

+		endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
+						      endpoint);
+		if (!endpoint)
+			break;
+
+		sdinfo = &pdata->subdev_info[i];

subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1490

Quoted 8 lines

drivers/media/platform/davinci/vpif_capture.c +8 -0

+		chan = &pdata->chan_config[i];
+		chan->inputs = devm_kzalloc(&pdev->dev,
+					    sizeof(*chan->inputs) *
+					    VPIF_DISPLAY_MAX_CHANNELS,
+					    GFP_KERNEL);
+
+		chan->input_count++;
+		chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;

I wonder what's the purpose of using index i on this array as well.

If you use that to access a corresponding entry in a different array, I'd just create a struct that contains the port configuration and the async sub-device. The omap3isp driver does that, for instance; see isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if you're interested. Up to you.

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1495

Quoted 5 lines

drivers/media/platform/davinci/vpif_capture.c +5 -0

+		chan->inputs[i].input.std = V4L2_STD_ALL;
+		chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
+
+		/* FIXME: need a new property? ch0:composite ch1: s-video */
+		if (i == 0)

Can you assume that the first endopoint has got a particular kind of input? What if it's not connected?

If this is a different physical port (not in the meaning another) in the device, I'd use the reg property for this. Please see Documentation/devicetree/bindings/media/video-interfaces.txt .

Quoted 30 lines

drivers/media/platform/davinci/vpif_capture.c +30 -0

+			chan->inputs[i].input_route = INPUT_CVBS_VI2B;
+		else
+			chan->inputs[i].input_route = INPUT_SVIDEO_VI2C_VI1C;
+		chan->inputs[i].output_route = OUTPUT_10BIT_422_EMBEDDED_SYNC;
+
+		err = v4l2_of_parse_endpoint(endpoint, &bus_cfg);
+		if (err) {
+			dev_err(&pdev->dev, "Could not parse the endpoint\n");
+			goto done;
+		}
+		dev_dbg(&pdev->dev, "Endpoint %s, bus_width = %d\n",
+			endpoint->full_name, bus_cfg.bus.parallel.bus_width);
+		flags = bus_cfg.bus.parallel.flags;
+
+		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+			chan->vpif_if.hd_pol = 1;
+
+		if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
+			chan->vpif_if.vd_pol = 1;
+
+		chan->vpif_if.if_type = VPIF_IF_BT656;
+		rem = of_graph_get_remote_port_parent(endpoint);
+		if (!rem) {
+			dev_dbg(&pdev->dev, "Remote device at %s not found\n",
+				endpoint->full_name);
+			goto done;
+		}
+
+		dev_dbg(&pdev->dev, "Remote device %s, %s found\n",
+			rem->name, rem->full_name);

drivers/media/platform/davinci/vpif_capture.c static int vpif_async_complete(struct v4l2_async_notifier *notifier) · line 1530

Quoted 5 lines

drivers/media/platform/davinci/vpif_capture.c +5 -0

+		sdinfo->name = rem->full_name;
+
+		pdata->asd[i] = devm_kzalloc(&pdev->dev,
+					     sizeof(struct v4l2_async_subdev),
+					     GFP_KERNEL);

Do you ensure somewhere that i isn't overrunning the pdata->asd[] array? It's got VPIF_CAPTURE_MAX_CHANNELS entries.

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
 .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +13 -0

new file mode 100644
index 000000000000..c447ac482c1d
@@ -0,0 +1,65 @@
+Texas Instruments VPIF Capture
+------------------------------
+
+The TI Video Port InterFace (VPIF) capture component is the primary
+component for video capture on the DA850 family of TI DaVinci SoCs.
+
+TI Document number reference: SPRUH82C
+
+Required properties:
+- compatible: must be "ti,da850-vpif-capture"
+- reg: physical base address and length of the registers set for the device;
+- interrupts: should contain IRQ line for the VPIF
+

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +30 -0

+				  reg = <1>;
+				  bus-width = <8>;
+				  data-shift = <8>;
+			};
+		};
+	};
+
+[ ... ]
+
+&i2c0 {
+
+	tvp5147@5d {
+		compatible = "ti,tvp5147";
+		reg = <0x5d>;
+		status = "okay";
+
+		port {
+			composite: endpoint {
+				hsync-active = <1>;
+				vsync-active = <1>;
+				pclk-sample = <0>;
+
+				/* VPIF channel 0 (lower 8-bits) */
+				remote-endpoint = <&vpif_ch0>;
+				bus-width = <8>;
+			};
+		};
+	};
+
+};

Documentation/devicetree/bindings/media/ti,da850-vpif.txt new file +0 -0

new file mode 100644
index 000000000000..d004e600aabe

Quoted 27 lines · Kevin Hilman

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
 .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +13 -0

new file mode 100644
index 000000000000..c447ac482c1d
@@ -0,0 +1,65 @@
+Texas Instruments VPIF Capture
+------------------------------
+
+The TI Video Port InterFace (VPIF) capture component is the primary
+component for video capture on the DA850 family of TI DaVinci SoCs.
+
+TI Document number reference: SPRUH82C
+
+Required properties:
+- compatible: must be "ti,da850-vpif-capture"
+- reg: physical base address and length of the registers set for the device;
+- interrupts: should contain IRQ line for the VPIF
+

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt line 23

Quoted 10 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +10 -0

+VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
+channels or a single 16-bit channel.  It should contain at least one
+port child node with child 'endpoint' node. Please refer to the
+bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example using 2 8-bit input channels, one of which is connected to an
+I2C-connected TVP5147 decoder:
+
+	vpif_capture: video-capture@0x00217000 {

Drop the 0x00.

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt line 30

Quoted 7 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +7 -0

+		compatible = "ti,da850-vpif-capture";
+		reg = <0x00217000 0x1000>;
+		interrupts = <92>;
+
+		port {
+			vpif_ch0: endpoint@0 {
+				  reg = <0>;

This is missing #size-cells and #addr-cells.

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt line 35

Quoted 5 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +5 -0

+				  bus-width = <8>;
+				  remote-endpoint = <&composite>;
+			};
+
+			vpif_ch1: endpoint@1 {

I think probably channels here should be ports rather than endpoints. AIUI, having multiple endpoints is for cases like a mux or 1 to many connections. There's only one data flow, but multiple sources or sinks.

Quoted 38 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +30 -0

+				  reg = <1>;
+				  bus-width = <8>;
+				  data-shift = <8>;
+			};
+		};
+	};
+
+[ ... ]
+
+&i2c0 {
+
+	tvp5147@5d {
+		compatible = "ti,tvp5147";
+		reg = <0x5d>;
+		status = "okay";
+
+		port {
+			composite: endpoint {
+				hsync-active = <1>;
+				vsync-active = <1>;
+				pclk-sample = <0>;
+
+				/* VPIF channel 0 (lower 8-bits) */
+				remote-endpoint = <&vpif_ch0>;
+				bus-width = <8>;
+			};
+		};
+	};
+
+};

Documentation/devicetree/bindings/media/ti,da850-vpif.txt new file +2 -0

new file mode 100644
index 000000000000..d004e600aabe
@@ -0,0 +1,8 @@
+Texas Instruments VPIF
+----------------------

Documentation/devicetree/bindings/media/ti,da850-vpif.txt line 8

Quoted 6 lines

Documentation/devicetree/bindings/media/ti,da850-vpif.txt new file +6 -0

+
+The Video Port InterFace (VPIF) is the core component for video output
+and capture on DA850 TI Davinci SoCs.
+
+- compatible: must be "ti,da850-vpif"
+- reg: physical base address and length of the registers set for the device;

That's it? How does this block relate to the capture block?

Quoted 44 lines · Kevin Hilman

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
 .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +30 -0

new file mode 100644
index 000000000000..c447ac482c1d
@@ -0,0 +1,65 @@
+Texas Instruments VPIF Capture
+------------------------------
+
+The TI Video Port InterFace (VPIF) capture component is the primary
+component for video capture on the DA850 family of TI DaVinci SoCs.
+
+TI Document number reference: SPRUH82C
+
+Required properties:
+- compatible: must be "ti,da850-vpif-capture"
+- reg: physical base address and length of the registers set for the device;
+- interrupts: should contain IRQ line for the VPIF
+
+VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
+channels or a single 16-bit channel.  It should contain at least one
+port child node with child 'endpoint' node. Please refer to the
+bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example using 2 8-bit input channels, one of which is connected to an
+I2C-connected TVP5147 decoder:
+
+	vpif_capture: video-capture@0x00217000 {
+		reg = <0x00217000 0x1000>;
+		interrupts = <92>;
+
+		port {
+			vpif_ch0: endpoint@0 {
+				  reg = <0>;
+				  bus-width = <8>;

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt line 35

Quoted 4 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +4 -0

+				  remote-endpoint = <&composite>;
+			};
+
+			vpif_ch1: endpoint@1 {

I think probably channels here should be ports rather than endpoints. AIUI, having multiple endpoints is for cases like a mux or 1 to many connections. There's only one data flow, but multiple sources or sinks.

Hi Rob,

Quoted 45 lines · Rob Herring

Quoted 44 lines · Kevin Hilman

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
 .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +30 -0

new file mode 100644
index 000000000000..c447ac482c1d
@@ -0,0 +1,65 @@
+Texas Instruments VPIF Capture
+------------------------------
+
+The TI Video Port InterFace (VPIF) capture component is the primary
+component for video capture on the DA850 family of TI DaVinci SoCs.
+
+TI Document number reference: SPRUH82C
+
+Required properties:
+- compatible: must be "ti,da850-vpif-capture"
+- reg: physical base address and length of the registers set for the device;
+- interrupts: should contain IRQ line for the VPIF
+
+VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
+channels or a single 16-bit channel.  It should contain at least one
+port child node with child 'endpoint' node. Please refer to the
+bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example using 2 8-bit input channels, one of which is connected to an
+I2C-connected TVP5147 decoder:
+
+    vpif_capture: video-capture@0x00217000 {
+            reg = <0x00217000 0x1000>;
+            interrupts = <92>;
+
+            port {
+                    vpif_ch0: endpoint@0 {
+                              reg = <0>;
+                              bus-width = <8>;

Quoted 8 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt line 35

Quoted 4 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +4 -0

+                              remote-endpoint = <&composite>;
+                    };
+
+                    vpif_ch1: endpoint@1 {

I think probably channels here should be ports rather than endpoints. AIUI, having multiple endpoints is for cases like a mux or 1 to many connections. There's only one data flow, but multiple sources or sinks.

Looking at this closer... , I used an endpoint because it's bascially a 16-bit parallel bus, that can be configured as (up to) 2 8-bit "channels. So, based on the video-interfaces.txt doc, I configured this as a single port, with (up to) 2 endpoints. That also allows me to connect output of the decoder directly, using the remote-endpoint property.

So I guess I'm not fully understanding your suggestion.

Quoted 65 lines · Kevin Hilman

Hi Rob,

Quoted 45 lines · Rob Herring

Quoted 44 lines · Kevin Hilman

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
 .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +30 -0

new file mode 100644
index 000000000000..c447ac482c1d
@@ -0,0 +1,65 @@
+Texas Instruments VPIF Capture
+------------------------------
+
+The TI Video Port InterFace (VPIF) capture component is the primary
+component for video capture on the DA850 family of TI DaVinci SoCs.
+
+TI Document number reference: SPRUH82C
+
+Required properties:
+- compatible: must be "ti,da850-vpif-capture"
+- reg: physical base address and length of the registers set for the device;
+- interrupts: should contain IRQ line for the VPIF
+
+VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
+channels or a single 16-bit channel.  It should contain at least one
+port child node with child 'endpoint' node. Please refer to the
+bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example using 2 8-bit input channels, one of which is connected to an
+I2C-connected TVP5147 decoder:
+
+    vpif_capture: video-capture@0x00217000 {
+            reg = <0x00217000 0x1000>;
+            interrupts = <92>;
+
+            port {
+                    vpif_ch0: endpoint@0 {
+                              reg = <0>;
+                              bus-width = <8>;

Quoted 8 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt line 34

Quoted 4 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +4 -0

+                              remote-endpoint = <&composite>;
+                    };
+
+                    vpif_ch1: endpoint@1 {

I think probably channels here should be ports rather than endpoints. AIUI, having multiple endpoints is for cases like a mux or 1 to many connections. There's only one data flow, but multiple sources or sinks.

Looking at this closer... , I used an endpoint because it's bascially a 16-bit parallel bus, that can be configured as (up to) 2 8-bit "channels. So, based on the video-interfaces.txt doc, I configured this as a single port, with (up to) 2 endpoints. That also allows me to connect output of the decoder directly, using the remote-endpoint property.

Quoted 1 line

So I guess I'm not fully understanding your suggestion.

NM, looks like video-interfaces.txt actually spells out this case and defines doing it as you did.

Hi Rob and Kevin,

Quoted 66 lines · Rob Herring

Quoted 65 lines · Kevin Hilman

Hi Rob,

Quoted 45 lines · Rob Herring

Quoted 44 lines · Kevin Hilman

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
 .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +30 -0

new file mode 100644
index 000000000000..c447ac482c1d
@@ -0,0 +1,65 @@
+Texas Instruments VPIF Capture
+------------------------------
+
+The TI Video Port InterFace (VPIF) capture component is the primary
+component for video capture on the DA850 family of TI DaVinci SoCs.
+
+TI Document number reference: SPRUH82C
+
+Required properties:
+- compatible: must be "ti,da850-vpif-capture"
+- reg: physical base address and length of the registers set for the device;
+- interrupts: should contain IRQ line for the VPIF
+
+VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
+channels or a single 16-bit channel.  It should contain at least one
+port child node with child 'endpoint' node. Please refer to the
+bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example using 2 8-bit input channels, one of which is connected to an
+I2C-connected TVP5147 decoder:
+
+    vpif_capture: video-capture@0x00217000 {
+            reg = <0x00217000 0x1000>;
+            interrupts = <92>;
+
+            port {
+                    vpif_ch0: endpoint@0 {
+                              reg = <0>;
+                              bus-width = <8>;

Quoted 8 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt line 34

Quoted 4 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +4 -0

+                              remote-endpoint = <&composite>;
+                    };
+
+                    vpif_ch1: endpoint@1 {

I think probably channels here should be ports rather than endpoints. AIUI, having multiple endpoints is for cases like a mux or 1 to many connections. There's only one data flow, but multiple sources or sinks.

Looking at this closer... , I used an endpoint because it's bascially a 16-bit parallel bus, that can be configured as (up to) 2 8-bit "channels. So, based on the video-interfaces.txt doc, I configured this as a single port, with (up to) 2 endpoints. That also allows me to connect output of the decoder directly, using the remote-endpoint property.

Quoted 4 lines

Quoted 1 line

So I guess I'm not fully understanding your suggestion.

NM, looks like video-interfaces.txt actually spells out this case and defines doing it as you did.

It's actually the first time I read that portion (at least so that I could remember) of video-interfaces.txt. I'm not sure if anyone has implemented that previously, nor how we ended up with the text. The list archive could probably tell. Cc Guennadi who wrote it. :-) I couldn't immediately find DT source with this arrangement.

In case of splitting the port into two parallel interfaces, how do you determine which wires belong to which endpoint? I guess they'd be particular sets of wires but as there's just a single port it isn't defined by the port.

Quoted 80 lines · Sakari Ailus

Hi Rob and Kevin,

Quoted 66 lines · Rob Herring

Quoted 65 lines · Kevin Hilman

Hi Rob,

Quoted 45 lines · Rob Herring

Quoted 44 lines · Kevin Hilman

Signed-off-by Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/[email protected]>

---
 .../bindings/media/ti,da850-vpif-capture.txt       | 65 ++++++++++++++++++++++
 .../devicetree/bindings/media/ti,da850-vpif.txt    |  8 +++
 2 files changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +30 -0

new file mode 100644
index 000000000000..c447ac482c1d
@@ -0,0 +1,65 @@
+Texas Instruments VPIF Capture
+------------------------------
+
+The TI Video Port InterFace (VPIF) capture component is the primary
+component for video capture on the DA850 family of TI DaVinci SoCs.
+
+TI Document number reference: SPRUH82C
+
+Required properties:
+- compatible: must be "ti,da850-vpif-capture"
+- reg: physical base address and length of the registers set for the device;
+- interrupts: should contain IRQ line for the VPIF
+
+VPIF capture has a 16-bit parallel bus input, supporting 2 8-bit
+channels or a single 16-bit channel.  It should contain at least one
+port child node with child 'endpoint' node. Please refer to the
+bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example using 2 8-bit input channels, one of which is connected to an
+I2C-connected TVP5147 decoder:
+
+    vpif_capture: video-capture@0x00217000 {
+            reg = <0x00217000 0x1000>;
+            interrupts = <92>;
+
+            port {
+                    vpif_ch0: endpoint@0 {
+                              reg = <0>;
+                              bus-width = <8>;

Quoted 8 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt line 34

Quoted 4 lines

Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt new file +4 -0

+                              remote-endpoint = <&composite>;
+                    };
+
+                    vpif_ch1: endpoint@1 {

I think probably channels here should be ports rather than endpoints. AIUI, having multiple endpoints is for cases like a mux or 1 to many connections. There's only one data flow, but multiple sources or sinks.

Looking at this closer... , I used an endpoint because it's bascially a 16-bit parallel bus, that can be configured as (up to) 2 8-bit "channels. So, based on the video-interfaces.txt doc, I configured this as a single port, with (up to) 2 endpoints. That also allows me to connect output of the decoder directly, using the remote-endpoint property.

Quoted 4 lines

Quoted 1 line

So I guess I'm not fully understanding your suggestion.

NM, looks like video-interfaces.txt actually spells out this case and defines doing it as you did.

It's actually the first time I read that portion (at least so that I could remember) of video-interfaces.txt. I'm not sure if anyone has implemented that previously, nor how we ended up with the text. The list archive could probably tell. Cc Guennadi who wrote it. :-) I couldn't immediately find DT source with this arrangement.

Quoted 4 lines

In case of splitting the port into two parallel interfaces, how do you determine which wires belong to which endpoint? I guess they'd be particular sets of wires but as there's just a single port it isn't defined by the port.

Isn't that the point of data-shift?

e.g. it's a single 16-bit parallel bus, where the lower 8 bits are for channel 0 and the upper 8 bits are for channel 1. Alternately, the port can also be configured as a single 16-bit channel (e.g. for raw capture.)

If you want more details on this hardware, it's pretty well described in Chapter 35 of http://www.ti.com/lit/ug/spruh82c/spruh82c.pdf.

FWIW, I'm not really picky about how to do this. I'm trying to learn "the right way" and am happy to do that, but the feedback so far has been confusing (at least for someone relatively new to the DT side of the media framework.)