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

Commit Message

Kevin Hilman Nov. 22, 2016, 3:52 p.m. UTC

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

Comments

Sakari Ailus Nov. 23, 2016, 3:37 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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 */

Kevin Hilman Nov. 23, 2016, 11:25 p.m. UTC | #2

Hi Sakari,

Sakari Ailus <[e-mail]> 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 <[e-mail]>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman Nov. 23, 2016, 11:26 p.m. UTC | #3

Hi Sakari,

Sakari Ailus <[e-mail]> 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 <[e-mail]>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sakari Ailus Nov. 30, 2016, 10:51 p.m. UTC | #4

Hi Kevin,

On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> Hi Sakari,
> 
> Sakari Ailus <[e-mail]> 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 <[e-mail]>
> >> ---
> >>  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.

Kevin Hilman Dec. 1, 2016, 12:14 a.m. UTC | #5

Sakari Ailus <[e-mail]> writes:

> Hi Kevin,
>
> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>> Hi Sakari,
>> 
>> Sakari Ailus <[e-mail]> 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 <[e-mail]>
>> >> ---
>> >>  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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sakari Ailus Dec. 1, 2016, 7:57 a.m. UTC | #6

On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
> Sakari Ailus <[e-mail]> writes:
> 
> > Hi Kevin,
> >
> > On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> >> Hi Sakari,
> >> 
> >> Sakari Ailus <[e-mail]> 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 <[e-mail]>
> >> >> ---
> >> >>  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.

Laurent Pinchart Dec. 1, 2016, 9:16 a.m. UTC | #7

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 <[e-mail]> writes:
> >> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> >>> Sakari Ailus <[e-mail]> 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 <[e-mail]>
> >>>>> ---
> >>>>> 
> >>>>>  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.

Hans Verkuil Dec. 5, 2016, 12:27 p.m. UTC | #8

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 <[e-mail]> writes:
>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>>>> Sakari Ailus <[e-mail]> 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 <[e-mail]>
>>>>>>> ---
>>>>>>>
>>>>>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman Dec. 6, 2016, 5:40 p.m. UTC | #9

Hans Verkuil <[e-mail]> 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 <[e-mail]> writes:
>>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>>>>> Sakari Ailus <[e-mail]> 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 <[e-mail]>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>  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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman Dec. 6, 2016, 7:50 p.m. UTC | #10

On Tue, Dec 6, 2016 at 9:40 AM, Kevin Hilman <[e-mail]> wrote:
> Hans Verkuil <[e-mail]> 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 <[e-mail]> writes:
>>>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>>>>>> Sakari Ailus <[e-mail]> 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 <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sakari Ailus Dec. 7, 2016, 2:09 p.m. UTC | #11

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 <[e-mail]> wrote:
> > Hans Verkuil <[e-mail]> 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 <[e-mail]> writes:
> >>>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> >>>>>>> Sakari Ailus <[e-mail]> 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 <[e-mail]>
> >>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>>  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.

Patch

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 */