Re: [RFT PATCH] [media] partial revert of "[media] tvp5150: add HW input connectors support"

[Philipp Zabel]

Hi,

On Tue, 2016-12-13 at 12:39 -0300, Javier Martinez Canillas wrote:
> Commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support")
> added input signals support for the tvp5150, but the approach was found
> to be incorrect so the corresponding DT binding commit 82c2ffeb217a
> ("[media] tvp5150: document input connectors DT bindings") was reverted.
> 
> This left the driver with an undocumented (and wrong) DT parsing logic,
> so lets get rid of this code as well until the input connectors support
> is implemented properly.
> 
> It's a partial revert due other patches added on top of mentioned commit
> not allowing the commit to be reverted cleanly anymore. But all the code
> related to the DT parsing logic and input entities creation are removed.
> 
> > Suggested-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>

what is the state of this patch? Was it forgotten or was the revert
deemed unnecessary?

regards
Philipp

> ---
> 
> Hello Laurent,
> 
> I've tested this patch on top of media/master on my IGEPv2 + tvp5150
> with the following:
> 
> $ media-ctl -r -l '"tvp5150 1-005c":1->"OMAP3 ISP CCDC":0[1], "OMAP3 ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]'
> $ media-ctl -v --set-format '"OMAP3 ISP CCDC":0 [UYVY2X8 720x240 field:alternate]'
> $ media-ctl -v --set-format '"OMAP3 ISP CCDC":1 [UYVY2X8 720x240 field:interlaced-tb]'
> $ yavta -f UYVY -s 720x480 -n 1 --field interlaced-tb --capture=1 -F /dev/video2
> $ raw2rgbpnm -f UYVY -s 720x480 frame-000000.bin frame.pnm
> 
> I've also tested the other composite input with the following change:
> 
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index 5fe5faefe212..973be68ff78c 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -1371,7 +1371,7 @@ static int tvp5150_probe(struct i2c_client *c,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn res;
> 
> ÂÂÂÂÂÂÂÂcore->norm = V4L2_STD_ALL;ÂÂÂÂÂÂ/* Default is autodetect */
> -ÂÂÂÂÂÂÂcore->input = TVP5150_COMPOSITE1;
> +ÂÂÂÂÂÂÂcore->input = TVP5150_COMPOSITE0;
> ÂÂÂÂÂÂÂÂcore->enable = true;
> 
> ÂÂÂÂÂÂÂÂv4l2_ctrl_handler_init(&core->hdl, 5);
> 
> But as mentioned, it also worked for me without the revert so please let
> me know if the driver works with your omap3 board.
> 
> Best regards,
> Javier
> 
> Âdrivers/media/i2c/tvp5150.c | 142 --------------------------------------------
> Â1 file changed, 142 deletions(-)
> 
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index 48646a7f3fb0..5fe5faefe212 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -42,8 +42,6 @@ struct tvp5150 {
> > Â	struct v4l2_subdev sd;
> Â#ifdef CONFIG_MEDIA_CONTROLLER
> > Â	struct media_pad pads[DEMOD_NUM_PADS];
> > -	struct media_entity input_ent[TVP5150_INPUT_NUM];
> > -	struct media_pad input_pad[TVP5150_INPUT_NUM];
> Â#endif
> > Â	struct v4l2_ctrl_handler hdl;
> > Â	struct v4l2_rect rect;
> @@ -1018,40 +1016,6 @@ static int tvp5150_enum_frame_size(struct v4l2_subdev *sd,
> Â}
> Â
> Â/****************************************************************************
> > -			Media entity ops
> - ****************************************************************************/
> -
> -#ifdef CONFIG_MEDIA_CONTROLLER
> -static int tvp5150_link_setup(struct media_entity *entity,
> > -			ÂÂÂÂÂÂconst struct media_pad *local,
> > -			ÂÂÂÂÂÂconst struct media_pad *remote, u32 flags)
> -{
> > -	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > -	struct tvp5150 *decoder = to_tvp5150(sd);
> > -	int i;
> -
> > -	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
> > -		if (remote->entity == &decoder->input_ent[i])
> > -			break;
> > -	}
> -
> > -	/* Do nothing for entities that are not input connectors */
> > -	if (i == TVP5150_INPUT_NUM)
> > -		return 0;
> -
> > -	decoder->input = i;
> -
> > -	tvp5150_selmux(sd);
> -
> > -	return 0;
> -}
> -
> -static const struct media_entity_operations tvp5150_sd_media_ops = {
> > -	.link_setup = tvp5150_link_setup,
> -};
> -#endif
> -
> -/****************************************************************************
> > Â			I2C Command
> Â ****************************************************************************/
> Â
> @@ -1188,42 +1152,6 @@ static int tvp5150_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *vt)
> > Â	return 0;
> Â}
> Â
> -static int tvp5150_registered(struct v4l2_subdev *sd)
> -{
> -#ifdef CONFIG_MEDIA_CONTROLLER
> > -	struct tvp5150 *decoder = to_tvp5150(sd);
> > -	int ret = 0;
> > -	int i;
> -
> > -	for (i = 0; i < TVP5150_INPUT_NUM; i++) {
> > -		struct media_entity *input = &decoder->input_ent[i];
> > -		struct media_pad *pad = &decoder->input_pad[i];
> -
> > -		if (!input->name)
> > -			continue;
> -
> > -		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
> -
> > -		ret = media_entity_pads_init(input, 1, pad);
> > -		if (ret < 0)
> > -			return ret;
> -
> > -		ret = media_device_register_entity(sd->v4l2_dev->mdev, input);
> > -		if (ret < 0)
> > -			return ret;
> -
> > -		ret = media_create_pad_link(input, 0, &sd->entity,
> > -					ÂÂÂÂDEMOD_PAD_IF_INPUT, 0);
> > -		if (ret < 0) {
> > -			media_device_unregister_entity(input);
> > -			return ret;
> > -		}
> > -	}
> -#endif
> -
> > -	return 0;
> -}
> -
> Â/* ----------------------------------------------------------------------- */
> Â
> Âstatic const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
> @@ -1274,11 +1202,6 @@ static const struct v4l2_subdev_ops tvp5150_ops = {
> > Â	.pad = &tvp5150_pad_ops,
> Â};
> Â
> -static const struct v4l2_subdev_internal_ops tvp5150_internal_ops = {
> > -	.registered = tvp5150_registered,
> -};
> -
> -
> Â/****************************************************************************
> > Â			I2C Client & Driver
> Â ****************************************************************************/
> @@ -1360,12 +1283,6 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> Â{
> > Â	struct v4l2_of_endpoint bus_cfg;
> > Â	struct device_node *ep;
> -#ifdef CONFIG_MEDIA_CONTROLLER
> > -	struct device_node *connectors, *child;
> > -	struct media_entity *input;
> > -	const char *name;
> > -	u32 input_type;
> -#endif
> > Â	unsigned int flags;
> > Â	int ret = 0;
> Â
> @@ -1389,63 +1306,6 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
> Â
> > Â	decoder->mbus_type = bus_cfg.bus_type;
> Â
> -#ifdef CONFIG_MEDIA_CONTROLLER
> > -	connectors = of_get_child_by_name(np, "connectors");
> -
> > -	if (!connectors)
> > -		goto err;
> -
> > -	for_each_available_child_of_node(connectors, child) {
> > -		ret = of_property_read_u32(child, "input", &input_type);
> > -		if (ret) {
> > -			dev_err(decoder->sd.dev,
> > -				Â"missing type property in node %s\n",
> > -				Âchild->name);
> > -			goto err_connector;
> > -		}
> -
> > -		if (input_type >= TVP5150_INPUT_NUM) {
> > -			ret = -EINVAL;
> > -			goto err_connector;
> > -		}
> -
> > -		input = &decoder->input_ent[input_type];
> -
> > -		/* Each input connector can only be defined once */
> > -		if (input->name) {
> > -			dev_err(decoder->sd.dev,
> > -				Â"input %s with same type already exists\n",
> > -				Âinput->name);
> > -			ret = -EINVAL;
> > -			goto err_connector;
> > -		}
> -
> > -		switch (input_type) {
> > -		case TVP5150_COMPOSITE0:
> > -		case TVP5150_COMPOSITE1:
> > -			input->function = MEDIA_ENT_F_CONN_COMPOSITE;
> > -			break;
> > -		case TVP5150_SVIDEO:
> > -			input->function = MEDIA_ENT_F_CONN_SVIDEO;
> > -			break;
> > -		}
> -
> > -		input->flags = MEDIA_ENT_FL_CONNECTOR;
> -
> > -		ret = of_property_read_string(child, "label", &name);
> > -		if (ret < 0) {
> > -			dev_err(decoder->sd.dev,
> > -				Â"missing label property in node %s\n",
> > -				Âchild->name);
> > -			goto err_connector;
> > -		}
> -
> > -		input->name = name;
> > -	}
> -
> -err_connector:
> > -	of_node_put(connectors);
> -#endif
> Âerr:
> > Â	of_node_put(ep);
> > Â	return ret;
> @@ -1491,7 +1351,6 @@ static int tvp5150_probe(struct i2c_client *c,
> > Â	}
> Â
> > Â	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
> > -	sd->internal_ops = &tvp5150_internal_ops;
> > Â	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> Â
> Â#if defined(CONFIG_MEDIA_CONTROLLER)
> @@ -1505,7 +1364,6 @@ static int tvp5150_probe(struct i2c_client *c,
> > Â	if (res < 0)
> > Â		return res;
> Â
> > -	sd->entity.ops = &tvp5150_sd_media_ops;
> Â#endif
> Â
> > Â	res = tvp5150_detect_version(core);