[PATCH v3 0/4] [media] davinci: VPIF: add DT support

[PATCH v3 0/4] [media] davinci: VPIF: add DT support

[Kevin Hilman]

Add DT support, including getting subdevs from DT ports/endpoints.

Changes since v2:
- DT binding doc: fix example to use correct compatible

Changes since v1:
- more specific compatible strings, based on SoC: ti,da850-vpif*
- fix locking bug when unlocking over subdev s_stream

Kevin Hilman (4):
  [media] davinci: add support for DT init
  [media] davinci: vpif_capture: don't lock over s_stream
  [media] davinci: vpif_capture: get subdevs from DT
  [media] dt-bindings: add TI VPIF documentation

 .../bindings/media/ti,da850-vpif-capture.txt       |  65 +++++++++
 .../devicetree/bindings/media/ti,da850-vpif.txt    |   8 ++
 drivers/media/platform/davinci/vpif.c              |   9 ++
 drivers/media/platform/davinci/vpif_capture.c      | 147 ++++++++++++++++++++-
 include/media/davinci/vpif_types.h                 |   9 +-
 5 files changed, 232 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
 create mode 100644 Documentation/devicetree/bindings/media/ti,da850-vpif.txt

-- 
2.9.3

[PATCH v3 1/4] [media] davinci: add support for DT init

[Kevin Hilman]

Add basic support for initialization via DT.

Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
 drivers/media/platform/davinci/vpif.c         |  9 +++++++++
 drivers/media/platform/davinci/vpif_capture.c | 14 ++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/media/platform/davinci/vpif.c b/drivers/media/platform/davinci/vpif.c
index 0380cf2e5775..d4434f614141 100644
--- a/drivers/media/platform/davinci/vpif.c
+++ b/drivers/media/platform/davinci/vpif.c
@@ -464,8 +464,17 @@ static const struct dev_pm_ops vpif_pm = {
 #define vpif_pm_ops NULL
 #endif
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id vpif_of_match[] = {
+	{ .compatible = "ti,da850-vpif", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, vpif_of_match);
+#endif
+
 static struct platform_driver vpif_driver = {
 	.driver = {
+		.of_match_table = of_match_ptr(vpif_of_match),
 		.name	= "vpif",
 		.pm	= vpif_pm_ops,
 	},
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 5104cc0ee40e..87ee1e2c3864 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1435,6 +1435,11 @@ static __init int vpif_probe(struct platform_device *pdev)
 	int res_idx = 0;
 	int i, err;
 
+	if (!pdev->dev.platform_data) {
+		dev_warn(&pdev->dev, "Missing platform data.  Giving up.\n");
+		return -EINVAL;
+	}
+
 	vpif_dev = &pdev->dev;
 
 	err = initialize_vpif();
@@ -1618,8 +1623,17 @@ static int vpif_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(vpif_pm_ops, vpif_suspend, vpif_resume);
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id vpif_capture_of_match[] = {
+	{ .compatible = "ti,da850-vpif-capture", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, vpif_capture_of_match);
+#endif
+
 static __refdata struct platform_driver vpif_driver = {
 	.driver	= {
+		.of_match_table = of_match_ptr(vpif_capture_of_match),
 		.name	= VPIF_DRIVER_NAME,
 		.pm	= &vpif_pm_ops,
 	},
-- 
2.9.3

[PATCH v3 2/4] [media] davinci: vpif_capture: don't lock over s_stream

[Kevin Hilman]

Video capture subdevs may be over I2C and may sleep during xfer, so we
cannot do IRQ-disabled locking when calling the subdev.

Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
 drivers/media/platform/davinci/vpif_capture.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 87ee1e2c3864..94ee6cf03f02 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -193,7 +193,10 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
 		}
 	}
 
+	spin_unlock_irqrestore(&common->irqlock, flags);
 	ret = v4l2_subdev_call(ch->sd, video, s_stream, 1);
+	spin_lock_irqsave(&common->irqlock, flags);
+
 	if (ret && ret != -ENOIOCTLCMD && ret != -ENODEV) {
 		vpif_dbg(1, debug, "stream on failed in subdev\n");
 		goto err;
-- 
2.9.3

[PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[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@baylibre.com>
---
 drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
 include/media/davinci/vpif_types.h            |   9 +-
 2 files changed, 133 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 94ee6cf03f02..47a4699157e7 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -26,6 +26,8 @@
 #include <linux/slab.h>
 
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-of.h>
+#include <media/i2c/tvp514x.h>
 
 #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;
+
+		endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
+						      endpoint);
+		if (!endpoint)
+			break;
+
+		sdinfo = &pdata->subdev_info[i];
+		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;
+		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)
+			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);
+		sdinfo->name = rem->full_name;
+
+		pdata->asd[i] = devm_kzalloc(&pdev->dev,
+					     sizeof(struct v4l2_async_subdev),
+					     GFP_KERNEL);
+		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];
diff --git a/include/media/davinci/vpif_types.h b/include/media/davinci/vpif_types.h
index 3cb1704a0650..4ee3b41975db 100644
--- a/include/media/davinci/vpif_types.h
+++ b/include/media/davinci/vpif_types.h
@@ -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 */
-- 
2.9.3

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Sakari Ailus]

Hi Kevin,

On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
> 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"?

> am437x-vpfe.c
> 
> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> ---
>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
>  include/media/davinci/vpif_types.h            |   9 +-
>  2 files changed, 133 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> index 94ee6cf03f02..47a4699157e7 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -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?

>  
>  #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;
> +
> +		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.

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

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

> +			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);
> +		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.

> +		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];
> diff --git a/include/media/davinci/vpif_types.h b/include/media/davinci/vpif_types.h
> index 3cb1704a0650..4ee3b41975db 100644
> --- a/include/media/davinci/vpif_types.h
> +++ b/include/media/davinci/vpif_types.h
> @@ -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 */

-- 
Kind regards,

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

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Kevin Hilman]

Hi Sakari,

Sakari Ailus <sakari.ailus@iki.fi> writes:

> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>> 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.

>> am437x-vpfe.c
>> 
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
>>  include/media/davinci/vpif_types.h            |   9 +-
>>  2 files changed, 133 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
>> index 94ee6cf03f02..47a4699157e7 100644
>> --- a/drivers/media/platform/davinci/vpif_capture.c
>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>> @@ -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?
>

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

>>  
>>  #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;
>> +
>> +		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.

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

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

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

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

>> +			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);
>> +		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.

Oops, no.  That will be fixed by making the outer for loop only iterate
for i = i..VPIF_CAPTURE_MAX_CHANNELS.


Thanks for the review,

Kevin

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Sakari Ailus]

Hi Kevin,

On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> Hi Sakari,
> 
> Sakari Ailus <sakari.ailus@iki.fi> writes:
> 
> > On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
> >> 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.
> 
> >> am437x-vpfe.c
> >> 
> >> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> >> ---
> >>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
> >>  include/media/davinci/vpif_types.h            |   9 +-
> >>  2 files changed, 133 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> >> index 94ee6cf03f02..47a4699157e7 100644
> >> --- a/drivers/media/platform/davinci/vpif_capture.c
> >> +++ b/drivers/media/platform/davinci/vpif_capture.c
> >> @@ -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?
> >
> 
> 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...
> 
> >>  
> >>  #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;
> >> +
> >> +		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.
> 
> >> +		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.)
> 
> > 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.

Ack.

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

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.

Cc Laurent.

-- 
Kind regards,

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

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Kevin Hilman]

Sakari Ailus <sakari.ailus@iki.fi> writes:

> Hi Kevin,
>
> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>> Hi Sakari,
>> 
>> Sakari Ailus <sakari.ailus@iki.fi> writes:
>> 
>> > On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>> >> 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.
>> 
>> >> am437x-vpfe.c
>> >> 
>> >> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> >> ---
>> >>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
>> >>  include/media/davinci/vpif_types.h            |   9 +-
>> >>  2 files changed, 133 insertions(+), 6 deletions(-)
>> >> 
>> >> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
>> >> index 94ee6cf03f02..47a4699157e7 100644
>> >> --- a/drivers/media/platform/davinci/vpif_capture.c
>> >> +++ b/drivers/media/platform/davinci/vpif_capture.c
>> >> @@ -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?
>> >
>> 
>> 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...
>> 
>> >>  
>> >>  #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;
>> >> +
>> >> +		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.
>> 
>> >> +		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.)
>> 
>> > 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.
>
> Ack.
>
>> 
>> >> +		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.
>> 
>> > 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.
>
> 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.)

Yeah, this is far from future proof.

> 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".  ;)

Kevin

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Sakari Ailus]

On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
> Sakari Ailus <sakari.ailus@iki.fi> writes:
> 
> > Hi Kevin,
> >
> > On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> >> Hi Sakari,
> >> 
> >> Sakari Ailus <sakari.ailus@iki.fi> writes:
> >> 
> >> > On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
> >> >> 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.
> >> 
> >> >> am437x-vpfe.c
> >> >> 
> >> >> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> >> >> ---
> >> >>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
> >> >>  include/media/davinci/vpif_types.h            |   9 +-
> >> >>  2 files changed, 133 insertions(+), 6 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> >> >> index 94ee6cf03f02..47a4699157e7 100644
> >> >> --- a/drivers/media/platform/davinci/vpif_capture.c
> >> >> +++ b/drivers/media/platform/davinci/vpif_capture.c
> >> >> @@ -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?
> >> >
> >> 
> >> 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...
> >> 
> >> >>  
> >> >>  #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;
> >> >> +
> >> >> +		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.
> >> 
> >> >> +		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.)
> >> 
> >> > 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.
> >
> > Ack.
> >
> >> 
> >> >> +		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.
> >> 
> >> > 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.
> >
> > 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.)
> 
> Yeah, this is far from future proof.
> 
> > 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.

-- 
Kind regards,

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

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Laurent Pinchart]

Hello,

On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:
> On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
> > Sakari Ailus <sakari.ailus@iki.fi> writes:
> >> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> >>> Sakari Ailus <sakari.ailus@iki.fi> writes:
> >>>> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
> >>>>> 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@baylibre.com>
> >>>>> ---
> >>>>> 
> >>>>>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
> >>>>>  include/media/davinci/vpif_types.h     
> >>>>>        |   9 +-
> >>>>>  2 files changed, 133 insertions(+), 6 deletions(-)
> >>>>> 
> >>>>> diff --git a/drivers/media/platform/davinci/vpif_capture.c
> >>>>> b/drivers/media/platform/davinci/vpif_capture.c index
> >>>>> 94ee6cf03f02..47a4699157e7 100644
> >>>>> --- a/drivers/media/platform/davinci/vpif_capture.c
> >>>>> +++ b/drivers/media/platform/davinci/vpif_capture.c
> >>>>> @@ -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?
> >>> 
> >>> 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.

> >>>>>  #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;

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 :-)

> >>>>> +			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;
> >>>>> +
> >>>>> +		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.
> >>> 
> >>>>> +		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.)
> >>> 
> >>>> 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.
> >> 
> >> Ack.
> >> 
> >>>>> +		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.
> >>> 
> >>>> 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.

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

> >> 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.)
> > 
> > Yeah, this is far from future proof.
> > 
> >> 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.

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Hans Verkuil]

On 12/01/2016 10:16 AM, Laurent Pinchart wrote:
> Hello,
> 
> On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:
>> On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
>>> Sakari Ailus <sakari.ailus@iki.fi> writes:
>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>>>> Sakari Ailus <sakari.ailus@iki.fi> writes:
>>>>>> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>>>>>>> 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@baylibre.com>
>>>>>>> ---
>>>>>>>
>>>>>>>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
>>>>>>>  include/media/davinci/vpif_types.h     
>>>>>>>        |   9 +-
>>>>>>>  2 files changed, 133 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>> b/drivers/media/platform/davinci/vpif_capture.c index
>>>>>>> 94ee6cf03f02..47a4699157e7 100644
>>>>>>> --- a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>>>>>>> @@ -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?
>>>>>
>>>>> 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.

> 
>>>>>>>  #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;
> 
> 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 :-)
> 
>>>>>>> +			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;
>>>>>>> +
>>>>>>> +		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.
>>>>>
>>>>>>> +		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.)
>>>>>
>>>>>> 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.
>>>>
>>>> Ack.
>>>>
>>>>>>> +		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.
>>>>>
>>>>>> 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.
> 
> I'm afraid I have to disappoint Hans here, I don't have code for that yet.
> 
>>>> 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.)
>>>
>>> Yeah, this is far from future proof.
>>>
>>>> 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.
> 
> 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.

Regards,

	Hans

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Kevin Hilman]

Hans Verkuil <hverkuil@xs4all.nl> writes:

> On 12/01/2016 10:16 AM, Laurent Pinchart wrote:
>> Hello,
>> 
>> On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:
>>> On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
>>>> Sakari Ailus <sakari.ailus@iki.fi> writes:
>>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>>>>> Sakari Ailus <sakari.ailus@iki.fi> writes:
>>>>>>> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>>>>>>>> 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@baylibre.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
>>>>>>>>  include/media/davinci/vpif_types.h     
>>>>>>>>        |   9 +-
>>>>>>>>  2 files changed, 133 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>> b/drivers/media/platform/davinci/vpif_capture.c index
>>>>>>>> 94ee6cf03f02..47a4699157e7 100644
>>>>>>>> --- a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>> @@ -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?
>>>>>>
>>>>>> 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.
>
>> 
>>>>>>>>  #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;
>> 
>> 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 :-)
>> 
>>>>>>>> +			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;
>>>>>>>> +
>>>>>>>> +		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.
>>>>>>
>>>>>>>> +		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.)
>>>>>>
>>>>>>> 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.
>>>>>
>>>>> Ack.
>>>>>
>>>>>>>> +		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.
>>>>>>
>>>>>>> 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.
>> 
>> I'm afraid I have to disappoint Hans here, I don't have code for that yet.
>> 
>>>>> 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.)
>>>>
>>>> Yeah, this is far from future proof.
>>>>
>>>>> 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.
>> 
>> 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.

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

Kevin

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Kevin Hilman]

On Tue, Dec 6, 2016 at 9:40 AM, Kevin Hilman <khilman@baylibre.com> wrote:
> Hans Verkuil <hverkuil@xs4all.nl> writes:
>
>> On 12/01/2016 10:16 AM, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:
>>>> On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
>>>>> Sakari Ailus <sakari.ailus@iki.fi> writes:
>>>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>>>>>> Sakari Ailus <sakari.ailus@iki.fi> writes:
>>>>>>>> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>>>>>>>>> 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@baylibre.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
>>>>>>>>>  include/media/davinci/vpif_types.h
>>>>>>>>>        |   9 +-
>>>>>>>>>  2 files changed, 133 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>>> b/drivers/media/platform/davinci/vpif_capture.c index
>>>>>>>>> 94ee6cf03f02..47a4699157e7 100644
>>>>>>>>> --- a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>>> @@ -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?
>>>>>>>
>>>>>>> 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.
>>
>>>
>>>>>>>>>  #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;
>>>
>>> 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 :-)
>>>
>>>>>>>>> +                      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;
>>>>>>>>> +
>>>>>>>>> +              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.
>>>>>>>
>>>>>>>>> +              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.)
>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> Ack.
>>>>>>
>>>>>>>>> +              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.
>>>>>>>
>>>>>>>> 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.
>>>
>>> I'm afraid I have to disappoint Hans here, I don't have code for that yet.
>>>
>>>>>> 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.)
>>>>>
>>>>> Yeah, this is far from future proof.
>>>>>
>>>>>> 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.
>>>
>>> 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.
>
> 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).

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.

Kevin

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Sakari Ailus]

Hi Kevin,

On Tue, Dec 06, 2016 at 11:50:58AM -0800, Kevin Hilman wrote:
> On Tue, Dec 6, 2016 at 9:40 AM, Kevin Hilman <khilman@baylibre.com> wrote:
> > Hans Verkuil <hverkuil@xs4all.nl> writes:
> >
> >> On 12/01/2016 10:16 AM, Laurent Pinchart wrote:
> >>> Hello,
> >>>
> >>> On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:
> >>>> On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
> >>>>> Sakari Ailus <sakari.ailus@iki.fi> writes:
> >>>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> >>>>>>> Sakari Ailus <sakari.ailus@iki.fi> writes:
> >>>>>>>> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
> >>>>>>>>> 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@baylibre.com>
> >>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
> >>>>>>>>>  include/media/davinci/vpif_types.h
> >>>>>>>>>        |   9 +-
> >>>>>>>>>  2 files changed, 133 insertions(+), 6 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/media/platform/davinci/vpif_capture.c
> >>>>>>>>> b/drivers/media/platform/davinci/vpif_capture.c index
> >>>>>>>>> 94ee6cf03f02..47a4699157e7 100644
> >>>>>>>>> --- a/drivers/media/platform/davinci/vpif_capture.c
> >>>>>>>>> +++ b/drivers/media/platform/davinci/vpif_capture.c
> >>>>>>>>> @@ -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?
> >>>>>>>
> >>>>>>> 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.
> >>
> >>>
> >>>>>>>>>  #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;
> >>>
> >>> 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 :-)
> >>>
> >>>>>>>>> +                      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;
> >>>>>>>>> +
> >>>>>>>>> +              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.
> >>>>>>>
> >>>>>>>>> +              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.)
> >>>>>>>
> >>>>>>>> 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.
> >>>>>>
> >>>>>> Ack.
> >>>>>>
> >>>>>>>>> +              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.
> >>>>>>>
> >>>>>>>> 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.
> >>>
> >>> I'm afraid I have to disappoint Hans here, I don't have code for that yet.
> >>>
> >>>>>> 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.)
> >>>>>
> >>>>> Yeah, this is far from future proof.
> >>>>>
> >>>>>> 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.
> >>>
> >>> 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.
> >
> > 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).
> 
> 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.

A quick and dirty way to get forward would be to add a davinci specific
property to tell the valid arguments for the s_routing() in DT. That's quite
ugly, but would avoid adding a dependency to the tvp514x driver.

A better solution would be to provide the davinci driver with an enumeration
of which routes the tvp514x supports, a bit like the enum_mbus_code() pad op
does.

It'd be an additional sub-device operation next to s_routing(). The valid
values could be found that way. Only the tvp514x would need to support that
from the beginning so we'd know which values are valid here. If there's a
need to limit what should be actually allowed on that board it'd be found in
the tvp514x DT node, not davinci's.

There are many, many drivers using s_routing() so there would be a lot more
work in changing how s_routing() works. An enumeration of what's possible is
also a cleaner way to achieve what's needed IMO.

Laurent, Hans; let me know if you agree / disagree.

-- 
Kind regards,

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

Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Kevin Hilman]

Hi Sakari,

Sakari Ailus <sakari.ailus@iki.fi> writes:

> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>> 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.

>> am437x-vpfe.c
>> 
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++++++++++++-
>>  include/media/davinci/vpif_types.h            |   9 +-
>>  2 files changed, 133 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
>> index 94ee6cf03f02..47a4699157e7 100644
>> --- a/drivers/media/platform/davinci/vpif_capture.c
>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>> @@ -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?
>

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

>>  
>>  #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;
>> +
>> +		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.

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

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

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

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

>> +			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);
>> +		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.

Oops, no.  That will be fixed by making the outer for loop only iterate
for i = i..VPIF_CAPTURE_MAX_CHANNELS.


Thanks for the review,

Kevin

[PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

[Kevin Hilman]

Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
 .../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

diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
new file mode 100644
index 000000000000..c447ac482c1d
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
@@ -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 {
+		compatible = "ti,da850-vpif-capture";
+		reg = <0x00217000 0x1000>;
+		interrupts = <92>;
+
+		port {
+			vpif_ch0: endpoint@0 {
+				  reg = <0>;
+				  bus-width = <8>;
+				  remote-endpoint = <&composite>;
+			};
+
+			vpif_ch1: endpoint@1 {
+				  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>;
+			};
+		};
+	};
+
+};
diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
new file mode 100644
index 000000000000..d004e600aabe
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
@@ -0,0 +1,8 @@
+Texas Instruments VPIF
+----------------------
+
+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;
-- 
2.9.3

Re: [PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

[Rob Herring]

On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> new file mode 100644
> index 000000000000..c447ac482c1d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> @@ -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 {

Drop the 0x00.

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

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

> +				  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>;
> +			};
> +		};
> +	};
> +
> +};
> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
> new file mode 100644
> index 000000000000..d004e600aabe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
> @@ -0,0 +1,8 @@
> +Texas Instruments VPIF
> +----------------------
> +
> +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?

Rob

Re: [PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

[Kevin Hilman]

Rob Herring <robh@kernel.org> writes:

> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>>  .../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
>> 
>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> new file mode 100644
>> index 000000000000..c447ac482c1d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> @@ -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 {
>
> Drop the 0x00.
>
>> +		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.
>

Yup.

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

OK.

>> +				  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>;
>> +			};
>> +		};
>> +	};
>> +
>> +};
>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> new file mode 100644
>> index 000000000000..d004e600aabe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif.txt
>> @@ -0,0 +1,8 @@
>> +Texas Instruments VPIF
>> +----------------------
>> +
>> +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?

I separated them because the current legacy drivers are separated into 3
different platform drivers.

However, after some discussions with Laurent, I'm going to just create a
single VPIF node with input (capture) and output (display) ports, and
then have to tweak the existing drivers a bit more than I had wanted to.

IOW, I was lazy.

Kevin

Re: [PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

[Kevin Hilman]

Hi Rob,

Rob Herring <robh@kernel.org> writes:

> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> ---
>>  .../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
>> 
>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> new file mode 100644
>> index 000000000000..c447ac482c1d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> @@ -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>;
>> +				  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.

Kevin

Re: [PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

[Rob Herring]

On Mon, Nov 28, 2016 at 4:30 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> Hi Rob,
>
> Rob Herring <robh@kernel.org> writes:
>
>> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>>> ---
>>>  .../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
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>> new file mode 100644
>>> index 000000000000..c447ac482c1d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>>> @@ -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>;
>>> +                              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.

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

Rob

Re: [PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

[Sakari Ailus]

Hi Rob and Kevin,

On Tue, Nov 29, 2016 at 08:41:44AM -0600, Rob Herring wrote:
> On Mon, Nov 28, 2016 at 4:30 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> > Hi Rob,
> >
> > Rob Herring <robh@kernel.org> writes:
> >
> >> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
> >>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> >>> ---
> >>>  .../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
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >>> new file mode 100644
> >>> index 000000000000..c447ac482c1d
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >>> @@ -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>;
> >>> +                              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.
> 
> 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.

-- 
Regards,

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

Re: [PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

[Kevin Hilman]

Sakari Ailus <sakari.ailus@iki.fi> writes:

> Hi Rob and Kevin,
>
> On Tue, Nov 29, 2016 at 08:41:44AM -0600, Rob Herring wrote:
>> On Mon, Nov 28, 2016 at 4:30 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>> > Hi Rob,
>> >
>> > Rob Herring <robh@kernel.org> writes:
>> >
>> >> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
>> >>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>> >>> ---
>> >>>  .../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
>> >>>
>> >>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> >>> new file mode 100644
>> >>> index 000000000000..c447ac482c1d
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
>> >>> @@ -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>;
>> >>> +                              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.
>> 
>> 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.

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

Kevin

Re: [PATCH v3 4/4] [media] dt-bindings: add TI VPIF documentation

[Sakari Ailus]

Hi Kevin,

On Wed, Nov 30, 2016 at 03:48:51PM -0800, Kevin Hilman wrote:
> Sakari Ailus <sakari.ailus@iki.fi> writes:
> 
> > Hi Rob and Kevin,
> >
> > On Tue, Nov 29, 2016 at 08:41:44AM -0600, Rob Herring wrote:
> >> On Mon, Nov 28, 2016 at 4:30 PM, Kevin Hilman <khilman@baylibre.com> wrote:
> >> > Hi Rob,
> >> >
> >> > Rob Herring <robh@kernel.org> writes:
> >> >
> >> >> On Tue, Nov 22, 2016 at 07:52:44AM -0800, Kevin Hilman wrote:
> >> >>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> >> >>> ---
> >> >>>  .../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
> >> >>>
> >> >>> diff --git a/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >> >>> new file mode 100644
> >> >>> index 000000000000..c447ac482c1d
> >> >>> --- /dev/null
> >> >>> +++ b/Documentation/devicetree/bindings/media/ti,da850-vpif-capture.txt
> >> >>> @@ -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>;
> >> >>> +                              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.
> >> 
> >> 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.
> 
> Isn't that the point of data-shift?

Right.

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

I have to admit I'm more familiar with cameras and serial busses than TV
tuners and parallel busses. Yeah, the data-shift is there for that purpose.

I think you should document which properties are expected to be found in the
port / endpoint nodes, the video-interfaces.txt has very many of them and I
don't think many of those are even relevant in this case.

-- 
Kind regards,

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