[PATCH v8 0/2] rcar-vin EDID control ioctls

[PATCH v8 0/2] rcar-vin EDID control ioctls

[Ulrich Hecht]

Hi!

This is a spinoff of "Lager/Koelsch board HDMI input support" that excludes
the DT portions, and that works without the unmerged subdevice abstraction
layer.

This revision improves over v7 from earlier today in that it does not break
analog input devices...

CU
Uli


Changes since v7:
- do not fail if there is no sink pad

Changes since v6:
- work without subdev abstraction layer
- split off DT parts, to be handled separately

Changes since v5:
- implement vin/subdev pad translation
- move i2c devices

Changes since v4:
- drop merged patches
- adv7604: always fall back to input 0 if nothing else is specified
- rcar-vin: implement G_EDID, S_EDID in place of hard-coded EDID blob

Changes since v3:
- rvin_enum_dv_timings(): use vin->src_pad_idx
- rvin_dv_timings_cap(): likewise
- rvin_s_dv_timings(): update vin->format
- add Koelsch support

Changes since v2:
- rebased on top of rcar-vin driver v4
- removed "adv7604: fix SPA register location for ADV7612" (picked up)
- changed prefix of dts patch to "ARM: dts: lager: "


Ulrich Hecht (2):
  media: adv7604: automatic "default-input" selection
  rcar-vin: implement EDID control ioctls

 drivers/media/i2c/adv7604.c                 |  5 +++-
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 +++++++++++++++++++++++++++++
 drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
 3 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.9.3

[PATCH v8 1/2] media: adv7604: automatic "default-input" selection

[Ulrich Hecht]

Fall back to input 0 if "default-input" property is not present.

Additionally, documentation in commit bf9c82278c34 ("[media]
media: adv7604: ability to read default input port from DT") states
that the "default-input" property should reside directly in the node
for adv7612. Hence, also adjust the parsing to make the implementation
consistent with this.

Based on patch by William Towle <william.towle@codethink.co.uk>.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/media/i2c/adv7604.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
index 4003831..055c9df 100644
--- a/drivers/media/i2c/adv7604.c
+++ b/drivers/media/i2c/adv7604.c
@@ -3077,10 +3077,13 @@ static int adv76xx_parse_dt(struct adv76xx_state *state)
 	if (!of_property_read_u32(endpoint, "default-input", &v))
 		state->pdata.default_input = v;
 	else
-		state->pdata.default_input = -1;
+		state->pdata.default_input = 0;
 
 	of_node_put(endpoint);
 
+	if (!of_property_read_u32(np, "default-input", &v))
+		state->pdata.default_input = v;
+
 	flags = bus_cfg.bus.parallel.flags;
 
 	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
-- 
2.9.3

Re: [PATCH v8 1/2] media: adv7604: automatic "default-input" selection

[Laurent Pinchart]

Hi Ulrich,

Thank you for the patch.

On Thursday 15 Sep 2016 15:24:07 Ulrich Hecht wrote:
> Fall back to input 0 if "default-input" property is not present.
> 
> Additionally, documentation in commit bf9c82278c34 ("[media]
> media: adv7604: ability to read default input port from DT") states
> that the "default-input" property should reside directly in the node
> for adv7612.

Actually it doesn't. The DT bindings specifies "default-input" as an endpoint 
property, even though the example sets it in the device node. That's 
inconsistent so the DT bindings document should be fixed. I believe the 
property should be set in the device node, it doesn't make much sense to have 
different default inputs per port.

> Hence, also adjust the parsing to make the implementation
> consistent with this.
> 
> Based on patch by William Towle <william.towle@codethink.co.uk>.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/media/i2c/adv7604.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 4003831..055c9df 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -3077,10 +3077,13 @@ static int adv76xx_parse_dt(struct adv76xx_state
> *state) if (!of_property_read_u32(endpoint, "default-input", &v))
>  		state->pdata.default_input = v;
>  	else
> -		state->pdata.default_input = -1;
> +		state->pdata.default_input = 0;
> 
>  	of_node_put(endpoint);
> 
> +	if (!of_property_read_u32(np, "default-input", &v))
> +		state->pdata.default_input = v;
> +
>  	flags = bus_cfg.bus.parallel.flags;
> 
>  	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 1/2] media: adv7604: automatic "default-input" selection

[Hans Verkuil]

Hi Ulrich,

What should I do with this? I dropped it for now.

I'm just going ahead and post the pull request without this patch as I
don't want this to be a blocker.

Regards,

	Hans

On 09/15/2016 06:42 PM, Laurent Pinchart wrote:
> Hi Ulrich,
> 
> Thank you for the patch.
> 
> On Thursday 15 Sep 2016 15:24:07 Ulrich Hecht wrote:
>> Fall back to input 0 if "default-input" property is not present.
>>
>> Additionally, documentation in commit bf9c82278c34 ("[media]
>> media: adv7604: ability to read default input port from DT") states
>> that the "default-input" property should reside directly in the node
>> for adv7612.
> 
> Actually it doesn't. The DT bindings specifies "default-input" as an endpoint 
> property, even though the example sets it in the device node. That's 
> inconsistent so the DT bindings document should be fixed. I believe the 
> property should be set in the device node, it doesn't make much sense to have 
> different default inputs per port.
> 
>> Hence, also adjust the parsing to make the implementation
>> consistent with this.
>>
>> Based on patch by William Towle <william.towle@codethink.co.uk>.
>>
>> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>> ---
>>  drivers/media/i2c/adv7604.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
>> index 4003831..055c9df 100644
>> --- a/drivers/media/i2c/adv7604.c
>> +++ b/drivers/media/i2c/adv7604.c
>> @@ -3077,10 +3077,13 @@ static int adv76xx_parse_dt(struct adv76xx_state
>> *state) if (!of_property_read_u32(endpoint, "default-input", &v))
>>  		state->pdata.default_input = v;
>>  	else
>> -		state->pdata.default_input = -1;
>> +		state->pdata.default_input = 0;
>>
>>  	of_node_put(endpoint);
>>
>> +	if (!of_property_read_u32(np, "default-input", &v))
>> +		state->pdata.default_input = v;
>> +
>>  	flags = bus_cfg.bus.parallel.flags;
>>
>>  	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> 

[PATCH v8 2/2] rcar-vin: implement EDID control ioctls

[Ulrich Hecht]

Adds G_EDID and S_EDID.

Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 +++++++++++++++++++++++++++++
 drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
 2 files changed, 43 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 62ca7e3..f679182 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -557,6 +557,38 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
 	return ret;
 }
 
+static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
+{
+	struct rvin_dev *vin = video_drvdata(file);
+	struct v4l2_subdev *sd = vin_to_source(vin);
+	int input, ret;
+
+	input = edid->pad;
+	edid->pad = vin->sink_pad_idx;
+
+	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
+
+	edid->pad = input;
+
+	return ret;
+}
+
+static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
+{
+	struct rvin_dev *vin = video_drvdata(file);
+	struct v4l2_subdev *sd = vin_to_source(vin);
+	int input, ret;
+
+	input = edid->pad;
+	edid->pad = vin->sink_pad_idx;
+
+	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
+
+	edid->pad = input;
+
+	return ret;
+}
+
 static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
 	.vidioc_querycap		= rvin_querycap,
 	.vidioc_try_fmt_vid_cap		= rvin_try_fmt_vid_cap,
@@ -579,6 +611,9 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
 	.vidioc_s_dv_timings		= rvin_s_dv_timings,
 	.vidioc_query_dv_timings	= rvin_query_dv_timings,
 
+	.vidioc_g_edid			= rvin_g_edid,
+	.vidioc_s_edid			= rvin_s_edid,
+
 	.vidioc_querystd		= rvin_querystd,
 	.vidioc_g_std			= rvin_g_std,
 	.vidioc_s_std			= rvin_s_std,
@@ -832,6 +867,13 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 	vin->src_pad_idx = pad_idx;
 	fmt.pad = vin->src_pad_idx;
 
+	vin->sink_pad_idx = 0;
+	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
+		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
+			vin->sink_pad_idx = pad_idx;
+			break;
+		}
+
 	/* Try to improve our guess of a reasonable window format */
 	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
 	if (ret) {
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 793184d..af815cc 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -121,6 +121,7 @@ struct rvin_dev {
 	struct video_device vdev;
 	struct v4l2_device v4l2_dev;
 	int src_pad_idx;
+	int sink_pad_idx;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_async_notifier notifier;
 	struct rvin_graph_entity digital;
-- 
2.9.3

Re: [PATCH v8 2/2] rcar-vin: implement EDID control ioctls

[Niklas Söderlund]

Hi Ulrich,

Thanks for your patch.

Can you append another patch ontop of this one whit the following 
change:

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index ac26738..7ba728d 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -494,7 +494,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
        int pad, ret;
 
        pad = timings->pad;
-       timings->pad = vin->src_pad_idx;
+       timings->pad = vin->sink_pad_idx;
 
        ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
 
@@ -548,7 +548,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
        int pad, ret;
 
        pad = cap->pad;
-       cap->pad = vin->src_pad_idx;
+       cap->pad = vin->sink_pad_idx;
 
        ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);


Whit that change the driver pass v4l2-compliance check for the HDMI 
input on Koelsch for me.

On 2016-09-15 15:24:08 +0200, Ulrich Hecht wrote:
> Adds G_EDID and S_EDID.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 +++++++++++++++++++++++++++++
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 62ca7e3..f679182 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -557,6 +557,38 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
>  	return ret;
>  }
>  
> +static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
> +{
> +	struct rvin_dev *vin = video_drvdata(file);
> +	struct v4l2_subdev *sd = vin_to_source(vin);
> +	int input, ret;
> +
> +	input = edid->pad;
> +	edid->pad = vin->sink_pad_idx;
> +
> +	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
> +
> +	edid->pad = input;
> +
> +	return ret;
> +}
> +
> +static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
> +{
> +	struct rvin_dev *vin = video_drvdata(file);
> +	struct v4l2_subdev *sd = vin_to_source(vin);
> +	int input, ret;
> +
> +	input = edid->pad;
> +	edid->pad = vin->sink_pad_idx;
> +
> +	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
> +
> +	edid->pad = input;
> +
> +	return ret;
> +}
> +
>  static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
>  	.vidioc_querycap		= rvin_querycap,
>  	.vidioc_try_fmt_vid_cap		= rvin_try_fmt_vid_cap,
> @@ -579,6 +611,9 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
>  	.vidioc_s_dv_timings		= rvin_s_dv_timings,
>  	.vidioc_query_dv_timings	= rvin_query_dv_timings,
>  
> +	.vidioc_g_edid			= rvin_g_edid,
> +	.vidioc_s_edid			= rvin_s_edid,
> +
>  	.vidioc_querystd		= rvin_querystd,
>  	.vidioc_g_std			= rvin_g_std,
>  	.vidioc_s_std			= rvin_s_std,
> @@ -832,6 +867,13 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>  	vin->src_pad_idx = pad_idx;
>  	fmt.pad = vin->src_pad_idx;
>  
> +	vin->sink_pad_idx = 0;
> +	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
> +		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
> +			vin->sink_pad_idx = pad_idx;
> +			break;
> +		}
> +
>  	/* Try to improve our guess of a reasonable window format */
>  	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
>  	if (ret) {
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 793184d..af815cc 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -121,6 +121,7 @@ struct rvin_dev {
>  	struct video_device vdev;
>  	struct v4l2_device v4l2_dev;
>  	int src_pad_idx;
> +	int sink_pad_idx;

sink_pad_idx should be documented in the comment above the struct. If 
you fix that feel free to add my:

Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_async_notifier notifier;
>  	struct rvin_graph_entity digital;
> -- 
> 2.9.3
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v8 2/2] rcar-vin: implement EDID control ioctls

[Hans Verkuil]

Hi Ulrich,

A patch series on top of my tree with Niklas' patches already merged:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=rcar

would be ideal. Since otherwise I have to resolve a conflict.

Regards,

	Hans

On 09/15/2016 03:59 PM, Niklas Söderlund wrote:
> Hi Ulrich,
> 
> Thanks for your patch.
> 
> Can you append another patch ontop of this one whit the following 
> change:
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index ac26738..7ba728d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -494,7 +494,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
>         int pad, ret;
>  
>         pad = timings->pad;
> -       timings->pad = vin->src_pad_idx;
> +       timings->pad = vin->sink_pad_idx;
>  
>         ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
>  
> @@ -548,7 +548,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
>         int pad, ret;
>  
>         pad = cap->pad;
> -       cap->pad = vin->src_pad_idx;
> +       cap->pad = vin->sink_pad_idx;
>  
>         ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
> 
> 
> Whit that change the driver pass v4l2-compliance check for the HDMI 
> input on Koelsch for me.
> 
> On 2016-09-15 15:24:08 +0200, Ulrich Hecht wrote:
>> Adds G_EDID and S_EDID.
>>
>> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>> ---
>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 +++++++++++++++++++++++++++++
>>  drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
>>  2 files changed, 43 insertions(+)
>>
>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> index 62ca7e3..f679182 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> @@ -557,6 +557,38 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
>>  	return ret;
>>  }
>>  
>> +static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>> +{
>> +	struct rvin_dev *vin = video_drvdata(file);
>> +	struct v4l2_subdev *sd = vin_to_source(vin);
>> +	int input, ret;
>> +
>> +	input = edid->pad;
>> +	edid->pad = vin->sink_pad_idx;
>> +
>> +	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
>> +
>> +	edid->pad = input;
>> +
>> +	return ret;
>> +}
>> +
>> +static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>> +{
>> +	struct rvin_dev *vin = video_drvdata(file);
>> +	struct v4l2_subdev *sd = vin_to_source(vin);
>> +	int input, ret;
>> +
>> +	input = edid->pad;
>> +	edid->pad = vin->sink_pad_idx;
>> +
>> +	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
>> +
>> +	edid->pad = input;
>> +
>> +	return ret;
>> +}
>> +
>>  static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
>>  	.vidioc_querycap		= rvin_querycap,
>>  	.vidioc_try_fmt_vid_cap		= rvin_try_fmt_vid_cap,
>> @@ -579,6 +611,9 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
>>  	.vidioc_s_dv_timings		= rvin_s_dv_timings,
>>  	.vidioc_query_dv_timings	= rvin_query_dv_timings,
>>  
>> +	.vidioc_g_edid			= rvin_g_edid,
>> +	.vidioc_s_edid			= rvin_s_edid,
>> +
>>  	.vidioc_querystd		= rvin_querystd,
>>  	.vidioc_g_std			= rvin_g_std,
>>  	.vidioc_s_std			= rvin_s_std,
>> @@ -832,6 +867,13 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>>  	vin->src_pad_idx = pad_idx;
>>  	fmt.pad = vin->src_pad_idx;
>>  
>> +	vin->sink_pad_idx = 0;
>> +	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
>> +		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
>> +			vin->sink_pad_idx = pad_idx;
>> +			break;
>> +		}
>> +
>>  	/* Try to improve our guess of a reasonable window format */
>>  	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
>>  	if (ret) {
>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
>> index 793184d..af815cc 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
>> @@ -121,6 +121,7 @@ struct rvin_dev {
>>  	struct video_device vdev;
>>  	struct v4l2_device v4l2_dev;
>>  	int src_pad_idx;
>> +	int sink_pad_idx;
> 
> sink_pad_idx should be documented in the comment above the struct. If 
> you fix that feel free to add my:
> 
> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
>>  	struct v4l2_ctrl_handler ctrl_handler;
>>  	struct v4l2_async_notifier notifier;
>>  	struct rvin_graph_entity digital;
>> -- 
>> 2.9.3
>>
> 

Re: [PATCH v8 2/2] rcar-vin: implement EDID control ioctls

[Hans Verkuil]

I've added patch v8 1/2 here as well. All I need is a correct patch 2/2 on top of
this. If you can do this tomorrow morning, then it will get into 4.9.

	Hans

On 09/15/2016 04:02 PM, Hans Verkuil wrote:
> Hi Ulrich,
> 
> A patch series on top of my tree with Niklas' patches already merged:
> 
> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=rcar
> 
> would be ideal. Since otherwise I have to resolve a conflict.
> 
> Regards,
> 
> 	Hans
> 
> On 09/15/2016 03:59 PM, Niklas Söderlund wrote:
>> Hi Ulrich,
>>
>> Thanks for your patch.
>>
>> Can you append another patch ontop of this one whit the following 
>> change:
>>
>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> index ac26738..7ba728d 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> @@ -494,7 +494,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
>>         int pad, ret;
>>  
>>         pad = timings->pad;
>> -       timings->pad = vin->src_pad_idx;
>> +       timings->pad = vin->sink_pad_idx;
>>  
>>         ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
>>  
>> @@ -548,7 +548,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
>>         int pad, ret;
>>  
>>         pad = cap->pad;
>> -       cap->pad = vin->src_pad_idx;
>> +       cap->pad = vin->sink_pad_idx;
>>  
>>         ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
>>
>>
>> Whit that change the driver pass v4l2-compliance check for the HDMI 
>> input on Koelsch for me.
>>
>> On 2016-09-15 15:24:08 +0200, Ulrich Hecht wrote:
>>> Adds G_EDID and S_EDID.
>>>
>>> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>>> ---
>>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 +++++++++++++++++++++++++++++
>>>  drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
>>>  2 files changed, 43 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> index 62ca7e3..f679182 100644
>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> @@ -557,6 +557,38 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
>>>  	return ret;
>>>  }
>>>  
>>> +static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>>> +{
>>> +	struct rvin_dev *vin = video_drvdata(file);
>>> +	struct v4l2_subdev *sd = vin_to_source(vin);
>>> +	int input, ret;
>>> +
>>> +	input = edid->pad;
>>> +	edid->pad = vin->sink_pad_idx;
>>> +
>>> +	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
>>> +
>>> +	edid->pad = input;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>>> +{
>>> +	struct rvin_dev *vin = video_drvdata(file);
>>> +	struct v4l2_subdev *sd = vin_to_source(vin);
>>> +	int input, ret;
>>> +
>>> +	input = edid->pad;
>>> +	edid->pad = vin->sink_pad_idx;
>>> +
>>> +	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
>>> +
>>> +	edid->pad = input;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
>>>  	.vidioc_querycap		= rvin_querycap,
>>>  	.vidioc_try_fmt_vid_cap		= rvin_try_fmt_vid_cap,
>>> @@ -579,6 +611,9 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
>>>  	.vidioc_s_dv_timings		= rvin_s_dv_timings,
>>>  	.vidioc_query_dv_timings	= rvin_query_dv_timings,
>>>  
>>> +	.vidioc_g_edid			= rvin_g_edid,
>>> +	.vidioc_s_edid			= rvin_s_edid,
>>> +
>>>  	.vidioc_querystd		= rvin_querystd,
>>>  	.vidioc_g_std			= rvin_g_std,
>>>  	.vidioc_s_std			= rvin_s_std,
>>> @@ -832,6 +867,13 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>>>  	vin->src_pad_idx = pad_idx;
>>>  	fmt.pad = vin->src_pad_idx;
>>>  
>>> +	vin->sink_pad_idx = 0;
>>> +	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
>>> +		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
>>> +			vin->sink_pad_idx = pad_idx;
>>> +			break;
>>> +		}
>>> +
>>>  	/* Try to improve our guess of a reasonable window format */
>>>  	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
>>>  	if (ret) {
>>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
>>> index 793184d..af815cc 100644
>>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
>>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
>>> @@ -121,6 +121,7 @@ struct rvin_dev {
>>>  	struct video_device vdev;
>>>  	struct v4l2_device v4l2_dev;
>>>  	int src_pad_idx;
>>> +	int sink_pad_idx;
>>
>> sink_pad_idx should be documented in the comment above the struct. If 
>> you fix that feel free to add my:
>>
>> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>
>>>  	struct v4l2_ctrl_handler ctrl_handler;
>>>  	struct v4l2_async_notifier notifier;
>>>  	struct rvin_graph_entity digital;
>>> -- 
>>> 2.9.3
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v8 2/2] rcar-vin: implement EDID control ioctls

[Laurent Pinchart]

Hi Ulrich,

Thank you for the patch.

On Thursday 15 Sep 2016 15:24:08 Ulrich Hecht wrote:
> Adds G_EDID and S_EDID.
> 
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 ++++++++++++++++++++++++++
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index 62ca7e3..f679182 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -557,6 +557,38 @@ static int rvin_dv_timings_cap(struct file *file, void
> *priv_fh, return ret;
>  }
> 
> +static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
> +{
> +	struct rvin_dev *vin = video_drvdata(file);
> +	struct v4l2_subdev *sd = vin_to_source(vin);
> +	int input, ret;
> +
> +	input = edid->pad;
> +	edid->pad = vin->sink_pad_idx;
> +
> +	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
> +
> +	edid->pad = input;
> +
> +	return ret;
> +}
> +
> +static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
> +{
> +	struct rvin_dev *vin = video_drvdata(file);
> +	struct v4l2_subdev *sd = vin_to_source(vin);
> +	int input, ret;
> +
> +	input = edid->pad;
> +	edid->pad = vin->sink_pad_idx;
> +
> +	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
> +
> +	edid->pad = input;
> +
> +	return ret;
> +}
> +
>  static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
>  	.vidioc_querycap		= rvin_querycap,
>  	.vidioc_try_fmt_vid_cap		= rvin_try_fmt_vid_cap,
> @@ -579,6 +611,9 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
>  	.vidioc_s_dv_timings		= rvin_s_dv_timings,
>  	.vidioc_query_dv_timings	= rvin_query_dv_timings,
> 
> +	.vidioc_g_edid			= rvin_g_edid,
> +	.vidioc_s_edid			= rvin_s_edid,
> +
>  	.vidioc_querystd		= rvin_querystd,
>  	.vidioc_g_std			= rvin_g_std,
>  	.vidioc_s_std			= rvin_s_std,
> @@ -832,6 +867,13 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>  	vin->src_pad_idx = pad_idx;
>  	fmt.pad = vin->src_pad_idx;
> 
> +	vin->sink_pad_idx = 0;
> +	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
> +		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
> +			vin->sink_pad_idx = pad_idx;
> +			break;
> +		}
> +

What if the subdev has multiple sink pads ? Shouldn't the pad number be 
instead computed in the get and set EDID handlers based on the input number 
passed in the struct v4l2_edid::pad field ?

>  	/* Try to improve our guess of a reasonable window format */
>  	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
>  	if (ret) {
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index 793184d..af815cc 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -121,6 +121,7 @@ struct rvin_dev {
>  	struct video_device vdev;
>  	struct v4l2_device v4l2_dev;
>  	int src_pad_idx;
> +	int sink_pad_idx;
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_async_notifier notifier;
>  	struct rvin_graph_entity digital;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 2/2] rcar-vin: implement EDID control ioctls

[Hans Verkuil]

On 09/15/2016 06:47 PM, Laurent Pinchart wrote:
> Hi Ulrich,
> 
> Thank you for the patch.
> 
> On Thursday 15 Sep 2016 15:24:08 Ulrich Hecht wrote:
>> Adds G_EDID and S_EDID.
>>
>> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
>> ---
>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 ++++++++++++++++++++++++++
>>  drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
>>  2 files changed, 43 insertions(+)
>>
>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index 62ca7e3..f679182 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> @@ -557,6 +557,38 @@ static int rvin_dv_timings_cap(struct file *file, void
>> *priv_fh, return ret;
>>  }
>>
>> +static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>> +{
>> +	struct rvin_dev *vin = video_drvdata(file);
>> +	struct v4l2_subdev *sd = vin_to_source(vin);
>> +	int input, ret;
>> +
>> +	input = edid->pad;
>> +	edid->pad = vin->sink_pad_idx;
>> +
>> +	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
>> +
>> +	edid->pad = input;
>> +
>> +	return ret;
>> +}
>> +
>> +static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>> +{
>> +	struct rvin_dev *vin = video_drvdata(file);
>> +	struct v4l2_subdev *sd = vin_to_source(vin);
>> +	int input, ret;
>> +
>> +	input = edid->pad;
>> +	edid->pad = vin->sink_pad_idx;
>> +
>> +	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
>> +
>> +	edid->pad = input;
>> +
>> +	return ret;
>> +}
>> +
>>  static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
>>  	.vidioc_querycap		= rvin_querycap,
>>  	.vidioc_try_fmt_vid_cap		= rvin_try_fmt_vid_cap,
>> @@ -579,6 +611,9 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
>>  	.vidioc_s_dv_timings		= rvin_s_dv_timings,
>>  	.vidioc_query_dv_timings	= rvin_query_dv_timings,
>>
>> +	.vidioc_g_edid			= rvin_g_edid,
>> +	.vidioc_s_edid			= rvin_s_edid,
>> +
>>  	.vidioc_querystd		= rvin_querystd,
>>  	.vidioc_g_std			= rvin_g_std,
>>  	.vidioc_s_std			= rvin_s_std,
>> @@ -832,6 +867,13 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>>  	vin->src_pad_idx = pad_idx;
>>  	fmt.pad = vin->src_pad_idx;
>>
>> +	vin->sink_pad_idx = 0;
>> +	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
>> +		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
>> +			vin->sink_pad_idx = pad_idx;
>> +			break;
>> +		}
>> +
> 
> What if the subdev has multiple sink pads ? Shouldn't the pad number be 
> instead computed in the get and set EDID handlers based on the input number 
> passed in the struct v4l2_edid::pad field ?

But there is only one input (VIDIOC_ENUM_INPUT), so this would not make sense.

What is wrong is that g/s_edid should check the pad and return -EINVAL if it
is non-zero. Odd that I missed that in the earlier reviews...

Regards,

	Hans

> 
>>  	/* Try to improve our guess of a reasonable window format */
>>  	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
>>  	if (ret) {
>> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
>> b/drivers/media/platform/rcar-vin/rcar-vin.h index 793184d..af815cc 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
>> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
>> @@ -121,6 +121,7 @@ struct rvin_dev {
>>  	struct video_device vdev;
>>  	struct v4l2_device v4l2_dev;
>>  	int src_pad_idx;
>> +	int sink_pad_idx;
>>  	struct v4l2_ctrl_handler ctrl_handler;
>>  	struct v4l2_async_notifier notifier;
>>  	struct rvin_graph_entity digital;
> 

Re: [PATCH v8 2/2] rcar-vin: implement EDID control ioctls

[Niklas Söderlund]

On 2016-09-15 19:01:06 +0200, Hans Verkuil wrote:
> 
> 
> On 09/15/2016 06:47 PM, Laurent Pinchart wrote:
> > Hi Ulrich,
> > 
> > Thank you for the patch.
> > 
> > On Thursday 15 Sep 2016 15:24:08 Ulrich Hecht wrote:
> >> Adds G_EDID and S_EDID.
> >>
> >> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> >> ---
> >>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 ++++++++++++++++++++++++++
> >>  drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
> >>  2 files changed, 43 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index 62ca7e3..f679182 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >> @@ -557,6 +557,38 @@ static int rvin_dv_timings_cap(struct file *file, void
> >> *priv_fh, return ret;
> >>  }
> >>
> >> +static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
> >> +{
> >> +	struct rvin_dev *vin = video_drvdata(file);
> >> +	struct v4l2_subdev *sd = vin_to_source(vin);
> >> +	int input, ret;
> >> +
> >> +	input = edid->pad;
> >> +	edid->pad = vin->sink_pad_idx;
> >> +
> >> +	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
> >> +
> >> +	edid->pad = input;
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
> >> +{
> >> +	struct rvin_dev *vin = video_drvdata(file);
> >> +	struct v4l2_subdev *sd = vin_to_source(vin);
> >> +	int input, ret;
> >> +
> >> +	input = edid->pad;
> >> +	edid->pad = vin->sink_pad_idx;
> >> +
> >> +	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
> >> +
> >> +	edid->pad = input;
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
> >>  	.vidioc_querycap		= rvin_querycap,
> >>  	.vidioc_try_fmt_vid_cap		= rvin_try_fmt_vid_cap,
> >> @@ -579,6 +611,9 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
> >>  	.vidioc_s_dv_timings		= rvin_s_dv_timings,
> >>  	.vidioc_query_dv_timings	= rvin_query_dv_timings,
> >>
> >> +	.vidioc_g_edid			= rvin_g_edid,
> >> +	.vidioc_s_edid			= rvin_s_edid,
> >> +
> >>  	.vidioc_querystd		= rvin_querystd,
> >>  	.vidioc_g_std			= rvin_g_std,
> >>  	.vidioc_s_std			= rvin_s_std,
> >> @@ -832,6 +867,13 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
> >>  	vin->src_pad_idx = pad_idx;
> >>  	fmt.pad = vin->src_pad_idx;
> >>
> >> +	vin->sink_pad_idx = 0;
> >> +	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
> >> +		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
> >> +			vin->sink_pad_idx = pad_idx;
> >> +			break;
> >> +		}
> >> +
> > 
> > What if the subdev has multiple sink pads ? Shouldn't the pad number be 
> > instead computed in the get and set EDID handlers based on the input number 
> > passed in the struct v4l2_edid::pad field ?
> 
> But there is only one input (VIDIOC_ENUM_INPUT), so this would not make sense.
> 
> What is wrong is that g/s_edid should check the pad and return -EINVAL if it
> is non-zero. Odd that I missed that in the earlier reviews...

Both Hans and Laurents comments are correct in this case I think.

The original patches was based on top of the Gen3 work where just as 
Laurent states the input number passed in the v4l2_edid::pad needs to be 
translated to a sink pad number of the subdevice. But since this is for 
Gen2 only there are only one input so no mapping is needed. All that is 
required is as Hans state to check that v4l2_edid::pad is a valid input 
number (equal to zero) from the rcar-vin perspective.

I do however still think there are value to find the subdevice sink pad 
id in rvin_v4l2_probe() and then use it in EDID handlers. The driver 
still need to use a sink pad number which is correct from the subdevice 
point of view.

> 
> Regards,
> 
> 	Hans
> 
> > 
> >>  	/* Try to improve our guess of a reasonable window format */
> >>  	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
> >>  	if (ret) {
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> >> b/drivers/media/platform/rcar-vin/rcar-vin.h index 793184d..af815cc 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> >> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> >> @@ -121,6 +121,7 @@ struct rvin_dev {
> >>  	struct video_device vdev;
> >>  	struct v4l2_device v4l2_dev;
> >>  	int src_pad_idx;
> >> +	int sink_pad_idx;
> >>  	struct v4l2_ctrl_handler ctrl_handler;
> >>  	struct v4l2_async_notifier notifier;
> >>  	struct rvin_graph_entity digital;
> > 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v8 2/2] rcar-vin: implement EDID control ioctls

[Laurent Pinchart]

Hi Hans,

On Thursday 15 Sep 2016 19:01:06 Hans Verkuil wrote:
> On 09/15/2016 06:47 PM, Laurent Pinchart wrote:
> > On Thursday 15 Sep 2016 15:24:08 Ulrich Hecht wrote:
> >> Adds G_EDID and S_EDID.
> >> 
> >> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> >> ---
> >> 
> >>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 +++++++++++++++++++++++
> >>  drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
> >>  2 files changed, 43 insertions(+)
> >> 
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index 62ca7e3..f679182
> >> 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >> @@ -557,6 +557,38 @@ static int rvin_dv_timings_cap(struct file *file,
> >> void *priv_fh, return ret;
> >>  }
> >> 
> >> +static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid
> >> *edid) +{
> >> +	struct rvin_dev *vin = video_drvdata(file);
> >> +	struct v4l2_subdev *sd = vin_to_source(vin);
> >> +	int input, ret;
> >> +
> >> +	input = edid->pad;
> >> +	edid->pad = vin->sink_pad_idx;
> >> +
> >> +	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
> >> +
> >> +	edid->pad = input;
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid
> >> *edid) +{
> >> +	struct rvin_dev *vin = video_drvdata(file);
> >> +	struct v4l2_subdev *sd = vin_to_source(vin);
> >> +	int input, ret;
> >> +
> >> +	input = edid->pad;
> >> +	edid->pad = vin->sink_pad_idx;
> >> +
> >> +	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
> >> +
> >> +	edid->pad = input;
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
> >>  	.vidioc_querycap		= rvin_querycap,
> >>  	.vidioc_try_fmt_vid_cap		= rvin_try_fmt_vid_cap,
> >> @@ -579,6 +611,9 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
> >>  	.vidioc_s_dv_timings		= rvin_s_dv_timings,
> >>  	.vidioc_query_dv_timings	= rvin_query_dv_timings,
> >> 
> >> +	.vidioc_g_edid			= rvin_g_edid,
> >> +	.vidioc_s_edid			= rvin_s_edid,
> >> +
> >>  	.vidioc_querystd		= rvin_querystd,
> >>  	.vidioc_g_std			= rvin_g_std,
> >>  	.vidioc_s_std			= rvin_s_std,
> >> @@ -832,6 +867,13 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
> >>  	vin->src_pad_idx = pad_idx;
> >>  	fmt.pad = vin->src_pad_idx;
> >> 
> >> +	vin->sink_pad_idx = 0;
> >> +	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
> >> +		if (sd->entity.pads[pad_idx].flags == MEDIA_PAD_FL_SINK) {
> >> +			vin->sink_pad_idx = pad_idx;
> >> +			break;
> >> +		}
> >> +
> > 
> > What if the subdev has multiple sink pads ? Shouldn't the pad number be
> > instead computed in the get and set EDID handlers based on the input
> > number passed in the struct v4l2_edid::pad field ?
> 
> But there is only one input (VIDIOC_ENUM_INPUT), so this would not make
> sense.

Indeed, my bad. We'll address that when implementing input selection support 
then.

> What is wrong is that g/s_edid should check the pad and return -EINVAL if it
> is non-zero. Odd that I missed that in the earlier reviews...

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 0/2] rcar-vin EDID control ioctls

[Hans Verkuil]

On 09/15/2016 03:24 PM, Ulrich Hecht wrote:
> Hi!
>
> This is a spinoff of "Lager/Koelsch board HDMI input support" that excludes
> the DT portions, and that works without the unmerged subdevice abstraction
> layer.

Are you going to post another patch series for the DT portions?

Regards,

	Hans

>
> This revision improves over v7 from earlier today in that it does not break
> analog input devices...
>
> CU
> Uli
>
>
> Changes since v7:
> - do not fail if there is no sink pad
>
> Changes since v6:
> - work without subdev abstraction layer
> - split off DT parts, to be handled separately
>
> Changes since v5:
> - implement vin/subdev pad translation
> - move i2c devices
>
> Changes since v4:
> - drop merged patches
> - adv7604: always fall back to input 0 if nothing else is specified
> - rcar-vin: implement G_EDID, S_EDID in place of hard-coded EDID blob
>
> Changes since v3:
> - rvin_enum_dv_timings(): use vin->src_pad_idx
> - rvin_dv_timings_cap(): likewise
> - rvin_s_dv_timings(): update vin->format
> - add Koelsch support
>
> Changes since v2:
> - rebased on top of rcar-vin driver v4
> - removed "adv7604: fix SPA register location for ADV7612" (picked up)
> - changed prefix of dts patch to "ARM: dts: lager: "
>
>
> Ulrich Hecht (2):
>   media: adv7604: automatic "default-input" selection
>   rcar-vin: implement EDID control ioctls
>
>  drivers/media/i2c/adv7604.c                 |  5 +++-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 42 +++++++++++++++++++++++++++++
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
>  3 files changed, 47 insertions(+), 1 deletion(-)
>

Re: [PATCH v8 0/2] rcar-vin EDID control ioctls

[Ulrich Hecht]

On Thu, Sep 15, 2016 at 3:25 PM, Hans Verkuil <hansverk@cisco.com> wrote:
> On 09/15/2016 03:24 PM, Ulrich Hecht wrote:
>>
>> Hi!
>>
>> This is a spinoff of "Lager/Koelsch board HDMI input support" that
>> excludes
>> the DT portions, and that works without the unmerged subdevice abstraction
>> layer.
>
>
> Are you going to post another patch series for the DT portions?

Yes, but not today.

CU
Uli