[PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[Niklas Söderlund]

Hi,

This series address issues with the R-Car Gen2 VIN driver. The most 
serious issue is the OPS when unbind and rebinding the i2c driver for 
the video source subdevice which have popped up as a blocker for other 
work.

This series is broken out of my much larger R-Car Gen3 enablement series 
'[PATCHv2 00/32] rcar-vin: Add Gen3 with media controller support'. I 
plan to remove that series form patchwork and focus on these fixes first 
as they are blocking other development. Once the blocking issues are 
removed I will rebase and repost the larger Gen3 series.

Patch 1-4 fix simple problems found while testing
    1-2 Fix format problems when the format is (re)set.
    3   Fix media pad errors
    4   Fix standard enumeration problem

Patch 5 adds a wrapper function to retrieve the active video source 
subdevice. This is strictly not needed on Gen2 which only have one 
possible video source per VIN instance (This will change on Gen3). But 
patch 6-8,11 which fixes real issues on Gen2 make use of this wrapper, as 
not risk breaking things by removing this wrapper in this series and 
then readding it in a later Gen3 series I have chosen to keep the patch.  
Please let me know if I should drop it and rewrite patch 6-11 (if 
possible I would like to avoid that).

Patch 6-8 deals with video source subdevice pad index handling by moving 
the information from struct rvin_dev to struct rvin_graph_entity and 
moving the pad index probing to the struct v4l2_async_notifier complete 
callback. This is needed to allow the bind/unbind fix in patch 10-11.

Patch 9 use the pad information when calling enum_mbus_code.

Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.

# echo 2-0020 > /sys/bus/i2c/drivers/adv7180/unbind
# echo 2-0020 > /sys/bus/i2c/drivers/adv7180/bind

 adv7180 2-0020: chip found @ 0x20 (e6530000.i2c)
 kobject (eaaab118): tried to init an initialized object, something is seriously wrong.
 CPU: 0 PID: 1640 Comm: bash Not tainted 4.10.0-rc4-00029-g19b80f8913cad837 #1
 Hardware name: Generic R8A7791 (Flattened Device Tree)
 Backtrace: 
 [<c010a858>] (dump_backtrace) from [<c010aaa4>] (show_stack+0x18/0x1c)
  r7:00000016 r6:60070013 r5:00000000 r4:c0a14dd8
 [<c010aa8c>] (show_stack) from [<c02de09c>] (dump_stack+0x84/0xa0)
 [<c02de018>] (dump_stack) from [<c02dfee4>] (kobject_init+0x3c/0x98)
  r7:00000016 r6:eaaab2e4 r5:c0a1f4dc r4:eaaab118
 [<c02dfea8>] (kobject_init) from [<c03b9244>] (device_initialize+0x28/0xb0)
  r5:c0a70be8 r4:eaaab110
 [<c03b921c>] (device_initialize) from [<c03baa34>] (device_register+0x14/0x20)
  r5:eaaab110 r4:eaaab110
 [<c03baa20>] (device_register) from [<c04a02c0>] (__video_register_device+0xb38/0x11cc)
  r5:eaaab110 r4:eaaab020
 [<c049f788>] (__video_register_device) from [<c04c91a0>] (rvin_v4l2_probe+0x17c/0x1e8)
  r10:00000000 r9:eaa3c050 r8:c0a270a8 r7:eaaab3a0 r6:eaaab020 r5:c0790068
  r4:eaaab010
 [<c04c9024>] (rvin_v4l2_probe) from [<c04c6da0>] (rvin_digital_notify_complete+0x174/0x184)
  r7:00002006 r6:eaaab010 r5:00000000 r4:eaaab3e0
 [<c04c6c2c>] (rvin_digital_notify_complete) from [<c04af180>] (v4l2_async_test_notify+0xe8/0xf0)
  r7:eaaab410 r6:eaa3c050 r5:c04c6c2c r4:eaaab3e0
 [<c04af098>] (v4l2_async_test_notify) from [<c04af560>] (v4l2_async_register_subdev+0xa4/0xcc)
  r7:eaa3c0fc r6:c0a27094 r5:eaaab3e0 r4:eaa3c050
 [<c04af4bc>] (v4l2_async_register_subdev) from [<c0497740>] (adv7180_probe+0x350/0x3e0)
  r9:eaa3c050 r8:00000000 r7:00000000 r6:00000000 r5:eb2cbe00 r4:eaa3c010
 [<c04973f0>] (adv7180_probe) from [<c048e9f4>] (i2c_device_probe+0x238/0x250)
  r9:0000000e r8:c0a264dc r7:eb2cbe20 r6:c0a264dc r5:c04973f0 r4:eb2cbe00
 [<c048e7bc>] (i2c_device_probe) from [<c03bd4f4>] (driver_probe_device+0x1f8/0x2c0)
  r9:0000000e r8:c0a264dc r7:00000000 r6:c0a70c18 r5:c0a70c0c r4:eb2cbe20
 [<c03bd2fc>] (driver_probe_device) from [<c03bbcd0>] (bind_store+0x94/0xe8)
  r10:00000000 r9:00000051 r8:00000007 r7:c0a26058 r6:eb2cbe54 r5:c0a264dc
  r4:eb2cbe20 r3:ea60b000
 [<c03bbc3c>] (bind_store) from [<c03bb710>] (drv_attr_store+0x2c/0x38)
  r9:00000051 r8:eb2daa0c r7:ea58ff80 r6:eb2daa00 r5:ea87a4c0 r4:c03bbc3c
 [<c03bb6e4>] (drv_attr_store) from [<c023e5e4>] (sysfs_kf_write+0x40/0x4c)
  r5:ea87a4c0 r4:c03bb6e4
 [<c023e5a4>] (sysfs_kf_write) from [<c023dc50>] (kernfs_fop_write+0x13c/0x1ac)
  r5:ea87a4c0 r4:00000007
 [<c023db14>] (kernfs_fop_write) from [<c01e0c78>] (__vfs_write+0x34/0x114)
  r9:ea58e000 r8:00000000 r7:00000007 r6:ea58ff80 r5:ea52a480 r4:c023db14
 [<c01e0c44>] (__vfs_write) from [<c01e0ee4>] (vfs_write+0xc4/0x150)
  r7:ea58ff80 r6:00167028 r5:00000007 r4:ea52a480
 [<c01e0e20>] (vfs_write) from [<c01e1038>] (SyS_write+0x48/0x80)
  r9:ea58e000 r8:c0106ee4 r7:00000007 r6:00167028 r5:ea52a480 r4:ea52a480
 [<c01e0ff0>] (SyS_write) from [<c0106d20>] (ret_fast_syscall+0x0/0x3c)
  r7:00000004 r6:b6dfed50 r5:00167028 r4:00000007

Niklas Söderlund (11):
  media: rcar-vin: reset bytesperline and sizeimage when resetting
    format
  media: rcar-vin: use rvin_reset_format() in S_DV_TIMINGS
  media: rcar-vin: fix how pads are handled for v4l2 subdeivce
    operations
  media: rcar-vin: fix standard in input enumeration
  media: rcar-vin: add wrapper to get rvin_graph_entity
  media: rcar-vin: move subdev source and sink pad index to
    rvin_graph_entity
  media: rcar-vin: move pad index discovery to async complete handler
  media: rcar-vin: refactor pad lookup code
  media: rcar-vin: use pad information when verifying media bus format
  media: rcar-vin: split rvin_s_fmt_vid_cap()
  media: rcar-vin: register the video device early

 drivers/media/platform/rcar-vin/rcar-core.c |  40 +++-
 drivers/media/platform/rcar-vin/rcar-dma.c  |  15 +-
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 346 +++++++++++++++-------------
 drivers/media/platform/rcar-vin/rcar-vin.h  |  20 +-
 4 files changed, 244 insertions(+), 177 deletions(-)

-- 
2.11.0

[PATCH 01/11] media: rcar-vin: reset bytesperline and sizeimage when resetting format

[Niklas Söderlund]

These two fields where forgotten when refactoring the format reset code
path. If they are not also reset at the same time as width and hight the
format read using G_FMT will not match reality.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 2bbe6d495fa634da..69bc4cfea6a8aeb5 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -151,6 +151,9 @@ static int rvin_reset_format(struct rvin_dev *vin)
 
 	rvin_reset_crop_compose(vin);
 
+	vin->format.bytesperline = rvin_format_bytesperline(&vin->format);
+	vin->format.sizeimage = rvin_format_sizeimage(&vin->format);
+
 	return 0;
 }
 
-- 
2.11.0

[PATCH 02/11] media: rcar-vin: use rvin_reset_format() in S_DV_TIMINGS

[Niklas Söderlund]

Use rvin_reset_format() in rvin_s_dv_timings() instead if just resetting
a few fields. This fixes an issue where the field format was not
properly set after S_DV_TIMINGS.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 69bc4cfea6a8aeb5..7ca27599b9982ffc 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -573,12 +573,8 @@ static int rvin_s_dv_timings(struct file *file, void *priv_fh,
 	if (ret)
 		return ret;
 
-	vin->source.width = timings->bt.width;
-	vin->source.height = timings->bt.height;
-	vin->format.width = timings->bt.width;
-	vin->format.height = timings->bt.height;
-
-	return 0;
+	/* Changing the timings will change the width/height */
+	return rvin_reset_format(vin);
 }
 
 static int rvin_g_dv_timings(struct file *file, void *priv_fh,
-- 
2.11.0

[PATCH 03/11] media: rcar-vin: fix how pads are handled for v4l2 subdeivce operations

[Niklas Söderlund]

The rcar-vin driver only uses one pad, pad number 0.

 All v4l2 operations which did not check that the requested operation
 was for pad 0 have been updated with a check to enforce this.

 All v4l2 operations that stored (and later restore) the requested pad
 before substituting it for the subdeivce pad number have been update
 not store the incoming pad and simply restore it to 0 after the
 subdevice operation is complete.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 7ca27599b9982ffc..610f59e2a9142622 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -550,14 +550,16 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
 {
 	struct rvin_dev *vin = video_drvdata(file);
 	struct v4l2_subdev *sd = vin_to_source(vin);
-	int pad, ret;
+	int ret;
+
+	if (timings->pad)
+		return -EINVAL;
 
-	pad = timings->pad;
 	timings->pad = vin->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
 
-	timings->pad = pad;
+	timings->pad = 0;
 
 	return ret;
 }
@@ -600,14 +602,16 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
 {
 	struct rvin_dev *vin = video_drvdata(file);
 	struct v4l2_subdev *sd = vin_to_source(vin);
-	int pad, ret;
+	int ret;
+
+	if (cap->pad)
+		return -EINVAL;
 
-	pad = cap->pad;
 	cap->pad = vin->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
 
-	cap->pad = pad;
+	cap->pad = 0;
 
 	return ret;
 }
@@ -616,17 +620,16 @@ 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;
+	int ret;
 
 	if (edid->pad)
 		return -EINVAL;
 
-	input = edid->pad;
 	edid->pad = vin->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
 
-	edid->pad = input;
+	edid->pad = 0;
 
 	return ret;
 }
@@ -635,17 +638,16 @@ 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;
+	int ret;
 
 	if (edid->pad)
 		return -EINVAL;
 
-	input = edid->pad;
 	edid->pad = vin->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
 
-	edid->pad = input;
+	edid->pad = 0;
 
 	return ret;
 }
-- 
2.11.0

[PATCH 04/11] media: rcar-vin: fix standard in input enumeration

[Niklas Söderlund]

If the subdevice supports dv_timings_cap the driver should not fill in
the standard. Also don't use the standard from probe time ask the
subdevice each time.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 610f59e2a9142622..f9218f230322eb0b 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -483,10 +483,16 @@ static int rvin_enum_input(struct file *file, void *priv,
 		return ret;
 
 	i->type = V4L2_INPUT_TYPE_CAMERA;
-	i->std = vin->vdev.tvnorms;
 
-	if (v4l2_subdev_has_op(sd, pad, dv_timings_cap))
+	if (v4l2_subdev_has_op(sd, pad, dv_timings_cap)) {
 		i->capabilities = V4L2_IN_CAP_DV_TIMINGS;
+		i->std = 0;
+	} else {
+		i->capabilities = V4L2_IN_CAP_STD;
+		ret = v4l2_subdev_call(sd, video, g_tvnorms, &i->std);
+		if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
+			return ret;
+	}
 
 	strlcpy(i->name, "Camera", sizeof(i->name));
 
-- 
2.11.0

[PATCH 05/11] media: rcar-vin: add wrapper to get rvin_graph_entity

[Niklas Söderlund]

Update the driver to retrieve the code and mbus_cfg values from a
rvin_graph_entity retrieved from a wrapper function instead of directly
accessing the entity for the digital port. This is done to prepare for
Gen3 support where the subdeivce might change during runtime, so to
directly accesses a specific rvin_graph_entity is bad.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c |  9 +++++++++
 drivers/media/platform/rcar-vin/rcar-dma.c  | 15 ++++++++++-----
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  6 +++++-
 drivers/media/platform/rcar-vin/rcar-vin.h  |  1 +
 4 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 098a0b1cc10a26ba..89a9280efa05aa0c 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -26,6 +26,15 @@
 #include "rcar-vin.h"
 
 /* -----------------------------------------------------------------------------
+ * Subdevice helpers
+ */
+
+struct rvin_graph_entity *vin_to_entity(struct rvin_dev *vin)
+{
+	return &vin->digital;
+}
+
+/* -----------------------------------------------------------------------------
  * Async notifier
  */
 
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 9ccd5ff55e192514..eac5c192b887ea45 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -131,10 +131,15 @@ static u32 rvin_read(struct rvin_dev *vin, u32 offset)
 
 static int rvin_setup(struct rvin_dev *vin)
 {
+	struct rvin_graph_entity *rent;
 	u32 vnmc, dmr, dmr2, interrupts;
 	v4l2_std_id std;
 	bool progressive = false, output_is_yuv = false, input_is_yuv = false;
 
+	rent = vin_to_entity(vin);
+	if (!rent)
+		return -ENODEV;
+
 	switch (vin->format.field) {
 	case V4L2_FIELD_TOP:
 		vnmc = VNMC_IM_ODD;
@@ -174,7 +179,7 @@ static int rvin_setup(struct rvin_dev *vin)
 	/*
 	 * Input interface
 	 */
-	switch (vin->digital.code) {
+	switch (rent->code) {
 	case MEDIA_BUS_FMT_YUYV8_1X16:
 		/* BT.601/BT.1358 16bit YCbCr422 */
 		vnmc |= VNMC_INF_YUV16;
@@ -182,7 +187,7 @@ static int rvin_setup(struct rvin_dev *vin)
 		break;
 	case MEDIA_BUS_FMT_UYVY8_2X8:
 		/* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
-		vnmc |= vin->digital.mbus_cfg.type == V4L2_MBUS_BT656 ?
+		vnmc |= rent->mbus_cfg.type == V4L2_MBUS_BT656 ?
 			VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
 		input_is_yuv = true;
 		break;
@@ -191,7 +196,7 @@ static int rvin_setup(struct rvin_dev *vin)
 		break;
 	case MEDIA_BUS_FMT_UYVY10_2X10:
 		/* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
-		vnmc |= vin->digital.mbus_cfg.type == V4L2_MBUS_BT656 ?
+		vnmc |= rent->mbus_cfg.type == V4L2_MBUS_BT656 ?
 			VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601;
 		input_is_yuv = true;
 		break;
@@ -203,11 +208,11 @@ static int rvin_setup(struct rvin_dev *vin)
 	dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
 
 	/* Hsync Signal Polarity Select */
-	if (!(vin->digital.mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
+	if (!(rent->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
 		dmr2 |= VNDMR2_HPS;
 
 	/* Vsync Signal Polarity Select */
-	if (!(vin->digital.mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
+	if (!(rent->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
 		dmr2 |= VNDMR2_VPS;
 
 	/*
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index f9218f230322eb0b..370bb184e5faace7 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -164,6 +164,7 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
 {
 	struct v4l2_subdev *sd;
 	struct v4l2_subdev_pad_config *pad_cfg;
+	struct rvin_graph_entity *rent;
 	struct v4l2_subdev_format format = {
 		.which = which,
 	};
@@ -171,8 +172,11 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
 	int ret;
 
 	sd = vin_to_source(vin);
+	rent = vin_to_entity(vin);
+	if (!rent)
+		return -ENODEV;
 
-	v4l2_fill_mbus_format(&format.format, pix, vin->digital.code);
+	v4l2_fill_mbus_format(&format.format, pix, rent->code);
 
 	pad_cfg = v4l2_subdev_alloc_pad_config(sd);
 	if (pad_cfg == NULL)
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 727e215c08718eb9..daec26a38d896b21 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -144,6 +144,7 @@ struct rvin_dev {
 	struct v4l2_rect compose;
 };
 
+struct rvin_graph_entity *vin_to_entity(struct rvin_dev *vin);
 #define vin_to_source(vin)		vin->digital.subdev
 
 /* Debug */
-- 
2.11.0

[PATCH 06/11] media: rcar-vin: move subdev source and sink pad index to rvin_graph_entity

[Niklas Söderlund]

It makes more sens to store the sink and source pad index in struct
rvin_graph_entity since that contains other subdevice related
information. Add complete documentation for struct rvin_graph_entity
while we are at it.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 45 ++++++++++++++++++++++-------
 drivers/media/platform/rcar-vin/rcar-vin.h  | 17 ++++++-----
 2 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 370bb184e5faace7..f8ff7c43944dd64a 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -109,9 +109,14 @@ static int rvin_reset_format(struct rvin_dev *vin)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
 	struct v4l2_mbus_framefmt *mf = &fmt.format;
+	struct rvin_graph_entity *rent;
 	int ret;
 
-	fmt.pad = vin->src_pad_idx;
+	rent = vin_to_entity(vin);
+	if (!rent)
+		return -ENODEV;
+
+	fmt.pad = rent->source_pad_idx;
 
 	ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);
 	if (ret)
@@ -182,7 +187,7 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
 	if (pad_cfg == NULL)
 		return -ENOMEM;
 
-	format.pad = vin->src_pad_idx;
+	format.pad = rent->source_pad_idx;
 
 	field = pix->field;
 
@@ -560,12 +565,17 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
 {
 	struct rvin_dev *vin = video_drvdata(file);
 	struct v4l2_subdev *sd = vin_to_source(vin);
+	struct rvin_graph_entity *rent;
 	int ret;
 
+	rent = vin_to_entity(vin);
+	if (!rent)
+		return -ENODEV;
+
 	if (timings->pad)
 		return -EINVAL;
 
-	timings->pad = vin->sink_pad_idx;
+	timings->pad = rent->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
 
@@ -612,12 +622,17 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
 {
 	struct rvin_dev *vin = video_drvdata(file);
 	struct v4l2_subdev *sd = vin_to_source(vin);
+	struct rvin_graph_entity *rent;
 	int ret;
 
+	rent = vin_to_entity(vin);
+	if (!rent)
+		return -ENODEV;
+
 	if (cap->pad)
 		return -EINVAL;
 
-	cap->pad = vin->sink_pad_idx;
+	cap->pad = rent->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
 
@@ -630,12 +645,17 @@ 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);
+	struct rvin_graph_entity *rent;
 	int ret;
 
+	rent = vin_to_entity(vin);
+	if (!rent)
+		return -ENODEV;
+
 	if (edid->pad)
 		return -EINVAL;
 
-	edid->pad = vin->sink_pad_idx;
+	edid->pad = rent->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
 
@@ -648,12 +668,17 @@ 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);
+	struct rvin_graph_entity *rent;
 	int ret;
 
+	rent = vin_to_entity(vin);
+	if (!rent)
+		return -ENODEV;
+
 	if (edid->pad)
 		return -EINVAL;
 
-	edid->pad = vin->sink_pad_idx;
+	edid->pad = rent->sink_pad_idx;
 
 	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
 
@@ -926,19 +951,19 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
 		V4L2_CAP_READWRITE;
 
-	vin->src_pad_idx = 0;
+	vin->digital.source_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_SOURCE)
 			break;
 	if (pad_idx >= sd->entity.num_pads)
 		return -EINVAL;
 
-	vin->src_pad_idx = pad_idx;
+	vin->digital.source_pad_idx = pad_idx;
 
-	vin->sink_pad_idx = 0;
+	vin->digital.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;
+			vin->digital.sink_pad_idx = pad_idx;
 			break;
 		}
 
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index daec26a38d896b21..d31212a992e15506 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -70,10 +70,12 @@ struct rvin_video_format {
 
 /**
  * struct rvin_graph_entity - Video endpoint from async framework
- * @asd:	sub-device descriptor for async framework
- * @subdev:	subdevice matched using async framework
- * @code:	Media bus format from source
- * @mbus_cfg:	Media bus format from DT
+ * @asd:		sub-device descriptor for async framework
+ * @subdev:		subdevice matched using async framework
+ * @code:		Media bus format from source
+ * @mbus_cfg:		Media bus format from DT
+ * @source_pad_idx:	source pad index on remote device
+ * @sink_pad_idx:	sink pad index on remote device
  */
 struct rvin_graph_entity {
 	struct v4l2_async_subdev asd;
@@ -81,6 +83,9 @@ struct rvin_graph_entity {
 
 	u32 code;
 	struct v4l2_mbus_config mbus_cfg;
+
+	int source_pad_idx;
+	int sink_pad_idx;
 };
 
 /**
@@ -91,8 +96,6 @@ struct rvin_graph_entity {
  *
  * @vdev:		V4L2 video device associated with VIN
  * @v4l2_dev:		V4L2 device
- * @src_pad_idx:	source pad index for media controller drivers
- * @sink_pad_idx:	sink pad index for media controller drivers
  * @ctrl_handler:	V4L2 control handler
  * @notifier:		V4L2 asynchronous subdevs notifier
  * @digital:		entity in the DT for local digital subdevice
@@ -121,8 +124,6 @@ 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.11.0

[PATCH 07/11] media: rcar-vin: move pad index discovery to async complete handler

[Niklas Söderlund]

To fix support for unbind and rebinding of subdevices the
rvin_v4l2_probe() needs to be called before there might be any subdevice
bound. Move pad index discovery to when we know the subdevice is
present.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 23 +++++++++++++++++++++++
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 18 +-----------------
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 89a9280efa05aa0c..2c40b6a1a93f108c 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -68,6 +68,8 @@ static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
 static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct v4l2_subdev *sd = vin->digital.subdev;
+	unsigned int pad_idx;
 	int ret;
 
 	/* Verify subdevices mbus format */
@@ -80,6 +82,27 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 	vin_dbg(vin, "Found media bus format for %s: %d\n",
 		vin->digital.subdev->name, vin->digital.code);
 
+	/* Figure out source and sink pad ids */
+	vin->digital.source_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_SOURCE)
+			break;
+	if (pad_idx >= sd->entity.num_pads)
+		return -EINVAL;
+
+	vin->digital.source_pad_idx = pad_idx;
+
+	vin->digital.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->digital.sink_pad_idx = pad_idx;
+			break;
+		}
+
+	vin_dbg(vin, "Found media pads for %s source: %d sink %d\n",
+		vin->digital.subdev->name, vin->digital.source_pad_idx,
+		vin->digital.sink_pad_idx);
+
 	ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
 	if (ret < 0) {
 		vin_err(vin, "Failed to register subdev nodes\n");
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index f8ff7c43944dd64a..51324c6d826f76ea 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -905,7 +905,7 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 {
 	struct video_device *vdev = &vin->vdev;
 	struct v4l2_subdev *sd = vin_to_source(vin);
-	int pad_idx, ret;
+	int ret;
 
 	v4l2_set_subdev_hostdata(sd, vin);
 
@@ -951,22 +951,6 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
 		V4L2_CAP_READWRITE;
 
-	vin->digital.source_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_SOURCE)
-			break;
-	if (pad_idx >= sd->entity.num_pads)
-		return -EINVAL;
-
-	vin->digital.source_pad_idx = pad_idx;
-
-	vin->digital.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->digital.sink_pad_idx = pad_idx;
-			break;
-		}
-
 	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
 	rvin_reset_format(vin);
 
-- 
2.11.0

[PATCH 08/11] media: rcar-vin: refactor pad lookup code

[Niklas Söderlund]

If the subdeivce did not supply pad information the driver will return
-EINVAL, this is not what we want so remove that check. The code can
then be broken out to a helper function reducing duplication.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 2c40b6a1a93f108c..e9373d9ab97fb827 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -65,11 +65,21 @@ static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
 	return false;
 }
 
+static unsigned int rvin_pad_idx(struct v4l2_subdev *sd, int direction)
+{
+	unsigned int pad_idx;
+
+	for (pad_idx = 0; pad_idx < sd->entity.num_pads; pad_idx++)
+		if (sd->entity.pads[pad_idx].flags == direction)
+			return pad_idx;
+
+	return 0;
+}
+
 static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
 	struct v4l2_subdev *sd = vin->digital.subdev;
-	unsigned int pad_idx;
 	int ret;
 
 	/* Verify subdevices mbus format */
@@ -83,21 +93,8 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 		vin->digital.subdev->name, vin->digital.code);
 
 	/* Figure out source and sink pad ids */
-	vin->digital.source_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_SOURCE)
-			break;
-	if (pad_idx >= sd->entity.num_pads)
-		return -EINVAL;
-
-	vin->digital.source_pad_idx = pad_idx;
-
-	vin->digital.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->digital.sink_pad_idx = pad_idx;
-			break;
-		}
+	vin->digital.source_pad_idx = rvin_pad_idx(sd, MEDIA_PAD_FL_SOURCE);
+	vin->digital.sink_pad_idx = rvin_pad_idx(sd, MEDIA_PAD_FL_SINK);
 
 	vin_dbg(vin, "Found media pads for %s source: %d sink %d\n",
 		vin->digital.subdev->name, vin->digital.source_pad_idx,
-- 
2.11.0

[PATCH 09/11] media: rcar-vin: use pad information when verifying media bus format

[Niklas Söderlund]

Use information about pad index when enumerating mbus codes.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index e9373d9ab97fb827..50058fe9e37d8771 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -48,6 +48,7 @@ static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
 	};
 
 	code.index = 0;
+	code.pad = entity->source_pad_idx;
 	while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code)) {
 		code.index++;
 		switch (code.code) {
-- 
2.11.0

[PATCH 10/11] media: rcar-vin: split rvin_s_fmt_vid_cap()

[Niklas Söderlund]

To support unbind and rebinding of subdevices rvin_s_fmt_vid_cap() needs
to be called from with in the driver itself. Rename the function
__rvin_s_fmt_vid_cap() and create a wrapper which can be used by
vidioc_s_fmt_vid_cap.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 51324c6d826f76ea..929f58b49b06154d 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -313,10 +313,8 @@ static int rvin_try_fmt_vid_cap(struct file *file, void *priv,
 				 &source);
 }
 
-static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
-			      struct v4l2_format *f)
+static int __rvin_s_fmt_vid_cap(struct rvin_dev *vin, struct v4l2_format *f)
 {
-	struct rvin_dev *vin = video_drvdata(file);
 	struct rvin_source_fmt source;
 	int ret;
 
@@ -338,6 +336,14 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
 	return 0;
 }
 
+static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
+			      struct v4l2_format *f)
+{
+	struct rvin_dev *vin = video_drvdata(file);
+
+	return __rvin_s_fmt_vid_cap(vin, f);
+}
+
 static int rvin_g_fmt_vid_cap(struct file *file, void *priv,
 			      struct v4l2_format *f)
 {
-- 
2.11.0

[PATCH 11/11] media: rcar-vin: register the video device early

[Niklas Söderlund]

To support unbind and rebinding of video source subdevices while keeping
a constant video device the subdevice needs to be attached when the
first user opens the video device and detached when the last user closes
it.

This changes the rcar-vin behavior in such way that the video device is
registered at probe time while attaching to a subdeivce is done only
when the video device is opened. If at that time there is no subdevice
bound to the rcar-vin driver the open will fail with -EBUSY until a
subdevice are bound to the rcar-vin driver using the V4L2 asynchronous
subdevice API.

This changes fix an OPS when first unbinding a subdevice and later
rebinding it.

 # echo 2-0020 > /sys/bus/i2c/drivers/adv7180/unbind
 # echo 2-0020 > /sys/bus/i2c/drivers/adv7180/bind

 adv7180 2-0020: chip found @ 0x20 (e6530000.i2c)
 kobject (eaaab118): tried to init an initialized object, something is seriously wrong.
 CPU: 0 PID: 1640 Comm: bash Not tainted 4.10.0-rc4-00029-g19b80f8913cad837 #1
 Hardware name: Generic R8A7791 (Flattened Device Tree)
 Backtrace:
 [<c010a858>] (dump_backtrace) from [<c010aaa4>] (show_stack+0x18/0x1c)
  r7:00000016 r6:60070013 r5:00000000 r4:c0a14dd8
 [<c010aa8c>] (show_stack) from [<c02de09c>] (dump_stack+0x84/0xa0)
 [<c02de018>] (dump_stack) from [<c02dfee4>] (kobject_init+0x3c/0x98)
  r7:00000016 r6:eaaab2e4 r5:c0a1f4dc r4:eaaab118
 [<c02dfea8>] (kobject_init) from [<c03b9244>] (device_initialize+0x28/0xb0)
  r5:c0a70be8 r4:eaaab110
 [<c03b921c>] (device_initialize) from [<c03baa34>] (device_register+0x14/0x20)
  r5:eaaab110 r4:eaaab110
 [<c03baa20>] (device_register) from [<c04a02c0>] (__video_register_device+0xb38/0x11cc)
  r5:eaaab110 r4:eaaab020
 [<c049f788>] (__video_register_device) from [<c04c91a0>] (rvin_v4l2_probe+0x17c/0x1e8)
  r10:00000000 r9:eaa3c050 r8:c0a270a8 r7:eaaab3a0 r6:eaaab020 r5:c0790068
  r4:eaaab010
 [<c04c9024>] (rvin_v4l2_probe) from [<c04c6da0>] (rvin_digital_notify_complete+0x174/0x184)
  r7:00002006 r6:eaaab010 r5:00000000 r4:eaaab3e0
 [<c04c6c2c>] (rvin_digital_notify_complete) from [<c04af180>] (v4l2_async_test_notify+0xe8/0xf0)
  r7:eaaab410 r6:eaa3c050 r5:c04c6c2c r4:eaaab3e0
 [<c04af098>] (v4l2_async_test_notify) from [<c04af560>] (v4l2_async_register_subdev+0xa4/0xcc)
  r7:eaa3c0fc r6:c0a27094 r5:eaaab3e0 r4:eaa3c050
 [<c04af4bc>] (v4l2_async_register_subdev) from [<c0497740>] (adv7180_probe+0x350/0x3e0)
  r9:eaa3c050 r8:00000000 r7:00000000 r6:00000000 r5:eb2cbe00 r4:eaa3c010
 [<c04973f0>] (adv7180_probe) from [<c048e9f4>] (i2c_device_probe+0x238/0x250)
  r9:0000000e r8:c0a264dc r7:eb2cbe20 r6:c0a264dc r5:c04973f0 r4:eb2cbe00
 [<c048e7bc>] (i2c_device_probe) from [<c03bd4f4>] (driver_probe_device+0x1f8/0x2c0)
  r9:0000000e r8:c0a264dc r7:00000000 r6:c0a70c18 r5:c0a70c0c r4:eb2cbe20
 [<c03bd2fc>] (driver_probe_device) from [<c03bbcd0>] (bind_store+0x94/0xe8)
  r10:00000000 r9:00000051 r8:00000007 r7:c0a26058 r6:eb2cbe54 r5:c0a264dc
  r4:eb2cbe20 r3:ea60b000
 [<c03bbc3c>] (bind_store) from [<c03bb710>] (drv_attr_store+0x2c/0x38)
  r9:00000051 r8:eb2daa0c r7:ea58ff80 r6:eb2daa00 r5:ea87a4c0 r4:c03bbc3c
 [<c03bb6e4>] (drv_attr_store) from [<c023e5e4>] (sysfs_kf_write+0x40/0x4c)
  r5:ea87a4c0 r4:c03bb6e4
 [<c023e5a4>] (sysfs_kf_write) from [<c023dc50>] (kernfs_fop_write+0x13c/0x1ac)
  r5:ea87a4c0 r4:00000007
 [<c023db14>] (kernfs_fop_write) from [<c01e0c78>] (__vfs_write+0x34/0x114)
  r9:ea58e000 r8:00000000 r7:00000007 r6:ea58ff80 r5:ea52a480 r4:c023db14
 [<c01e0c44>] (__vfs_write) from [<c01e0ee4>] (vfs_write+0xc4/0x150)
  r7:ea58ff80 r6:00167028 r5:00000007 r4:ea52a480
 [<c01e0e20>] (vfs_write) from [<c01e1038>] (SyS_write+0x48/0x80)
  r9:ea58e000 r8:c0106ee4 r7:00000007 r6:00167028 r5:ea52a480 r4:ea52a480
 [<c01e0ff0>] (SyS_write) from [<c0106d20>] (ret_fast_syscall+0x0/0x3c)
  r7:00000004 r6:b6dfed50 r5:00167028 r4:00000007

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c |  10 +-
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 226 ++++++++++++++--------------
 drivers/media/platform/rcar-vin/rcar-vin.h  |   2 +
 3 files changed, 121 insertions(+), 117 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 50058fe9e37d8771..c86e71ad369cb929 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -107,7 +107,7 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 		return ret;
 	}
 
-	return rvin_v4l2_probe(vin);
+	return 0;
 }
 
 static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
@@ -118,7 +118,6 @@ static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
 
 	if (vin->digital.subdev == subdev) {
 		vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
-		rvin_v4l2_remove(vin);
 		vin->digital.subdev = NULL;
 		return;
 	}
@@ -283,6 +282,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
 
 	vin->dev = &pdev->dev;
 	vin->chip = (enum chip_id)match->data;
+	vin->last_input = NULL;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (mem == NULL)
@@ -304,6 +304,10 @@ static int rcar_vin_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error;
 
+	ret = rvin_v4l2_probe(vin);
+	if (ret)
+		goto error;
+
 	pm_suspend_ignore_children(&pdev->dev, true);
 	pm_runtime_enable(&pdev->dev);
 
@@ -324,6 +328,8 @@ static int rcar_vin_remove(struct platform_device *pdev)
 
 	v4l2_async_notifier_unregister(&vin->notifier);
 
+	rvin_v4l2_remove(vin);
+
 	rvin_dma_remove(vin);
 
 	return 0;
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 929f58b49b06154d..47137d770290084a 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -483,6 +483,94 @@ static int rvin_cropcap(struct file *file, void *priv,
 	return v4l2_subdev_call(sd, video, g_pixelaspect, &crop->pixelaspect);
 }
 
+static int rvin_attach_subdevices(struct rvin_dev *vin)
+{
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+	struct v4l2_mbus_framefmt *mf = &fmt.format;
+	struct v4l2_subdev *sd = vin_to_source(vin);
+	struct rvin_graph_entity *rent;
+	struct v4l2_format f;
+	int ret;
+
+	rent = vin_to_entity(vin);
+	if (!rent)
+		return -ENODEV;
+
+	ret = v4l2_subdev_call(sd, core, s_power, 1);
+	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
+		return ret;
+
+	if (rent != vin->last_input) {
+		/* Input source have changed, reset our format */
+
+		vin->vdev.tvnorms = 0;
+		ret = v4l2_subdev_call(sd, video, g_tvnorms,
+				       &vin->vdev.tvnorms);
+		if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
+			goto error;
+
+		/* Free old controls (safe even if there where none) */
+		v4l2_ctrl_handler_free(&vin->ctrl_handler);
+
+		ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
+		if (ret < 0)
+			goto error;
+
+		/* Add new controls */
+		ret = v4l2_ctrl_add_handler(&vin->ctrl_handler,
+					    sd->ctrl_handler, NULL);
+		if (ret < 0)
+			goto error;
+
+		v4l2_ctrl_handler_setup(&vin->ctrl_handler);
+
+		fmt.pad = rent->source_pad_idx;
+
+		/* Try to improve our guess of a reasonable window format */
+		ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
+		if (ret)
+			goto error;
+
+		/* Set default format */
+		vin->format.width	= mf->width;
+		vin->format.height	= mf->height;
+		vin->format.colorspace	= mf->colorspace;
+		vin->format.field	= mf->field;
+		vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
+
+		/* Set initial crop and compose */
+		vin->crop.top = vin->crop.left = 0;
+		vin->crop.width = mf->width;
+		vin->crop.height = mf->height;
+
+		vin->compose.top = vin->compose.left = 0;
+		vin->compose.width = mf->width;
+		vin->compose.height = mf->height;
+
+		f.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+		f.fmt.pix = vin->format;
+		ret = __rvin_s_fmt_vid_cap(vin, &f);
+		if (ret)
+			goto error;
+	}
+
+	vin->last_input = rent;
+
+	return 0;
+error:
+	v4l2_subdev_call(sd, core, s_power, 0);
+	return ret;
+}
+
+static void rvin_detach_subdevices(struct rvin_dev *vin)
+{
+	struct v4l2_subdev *sd = vin_to_source(vin);
+
+	v4l2_subdev_call(sd, core, s_power, 0);
+}
+
 static int rvin_enum_input(struct file *file, void *priv,
 			   struct v4l2_input *i)
 {
@@ -741,80 +829,6 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
  * File Operations
  */
 
-static int rvin_power_on(struct rvin_dev *vin)
-{
-	int ret;
-	struct v4l2_subdev *sd = vin_to_source(vin);
-
-	pm_runtime_get_sync(vin->v4l2_dev.dev);
-
-	ret = v4l2_subdev_call(sd, core, s_power, 1);
-	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
-		return ret;
-	return 0;
-}
-
-static int rvin_power_off(struct rvin_dev *vin)
-{
-	int ret;
-	struct v4l2_subdev *sd = vin_to_source(vin);
-
-	ret = v4l2_subdev_call(sd, core, s_power, 0);
-
-	pm_runtime_put(vin->v4l2_dev.dev);
-
-	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
-		return ret;
-
-	return 0;
-}
-
-static int rvin_initialize_device(struct file *file)
-{
-	struct rvin_dev *vin = video_drvdata(file);
-	int ret;
-
-	struct v4l2_format f = {
-		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
-		.fmt.pix = {
-			.width		= vin->format.width,
-			.height		= vin->format.height,
-			.field		= vin->format.field,
-			.colorspace	= vin->format.colorspace,
-			.pixelformat	= vin->format.pixelformat,
-		},
-	};
-
-	ret = rvin_power_on(vin);
-	if (ret < 0)
-		return ret;
-
-	pm_runtime_enable(&vin->vdev.dev);
-	ret = pm_runtime_resume(&vin->vdev.dev);
-	if (ret < 0 && ret != -ENOSYS)
-		goto eresume;
-
-	/*
-	 * Try to configure with default parameters. Notice: this is the
-	 * very first open, so, we cannot race against other calls,
-	 * apart from someone else calling open() simultaneously, but
-	 * .host_lock is protecting us against it.
-	 */
-	ret = rvin_s_fmt_vid_cap(file, NULL, &f);
-	if (ret < 0)
-		goto esfmt;
-
-	v4l2_ctrl_handler_setup(&vin->ctrl_handler);
-
-	return 0;
-esfmt:
-	pm_runtime_disable(&vin->vdev.dev);
-eresume:
-	rvin_power_off(vin);
-
-	return ret;
-}
-
 static int rvin_open(struct file *file)
 {
 	struct rvin_dev *vin = video_drvdata(file);
@@ -826,17 +840,31 @@ static int rvin_open(struct file *file)
 
 	ret = v4l2_fh_open(file);
 	if (ret)
-		goto unlock;
+		goto err_out;
 
-	if (!v4l2_fh_is_singular_file(file))
-		goto unlock;
+	/* If there is no subdevice there is not much we can do */
+	if (!vin_to_source(vin)) {
+		ret = -EBUSY;
+		goto err_open;
+	}
 
-	if (rvin_initialize_device(file)) {
-		v4l2_fh_release(file);
-		ret = -ENODEV;
+	if (v4l2_fh_is_singular_file(file)) {
+		pm_runtime_get_sync(vin->dev);
+		ret = rvin_attach_subdevices(vin);
+		if (ret) {
+			vin_err(vin, "Error attaching subdevice\n");
+			goto err_power;
+		}
 	}
 
-unlock:
+	mutex_unlock(&vin->lock);
+
+	return 0;
+err_power:
+	pm_runtime_put(vin->dev);
+err_open:
+	v4l2_fh_release(file);
+err_out:
 	mutex_unlock(&vin->lock);
 	return ret;
 }
@@ -860,9 +888,8 @@ static int rvin_release(struct file *file)
 	 * Then de-initialize hw module.
 	 */
 	if (fh_singular) {
-		pm_runtime_suspend(&vin->vdev.dev);
-		pm_runtime_disable(&vin->vdev.dev);
-		rvin_power_off(vin);
+		rvin_detach_subdevices(vin);
+		pm_runtime_put(vin->dev);
 	}
 
 	mutex_unlock(&vin->lock);
@@ -910,41 +937,10 @@ static void rvin_notify(struct v4l2_subdev *sd,
 int rvin_v4l2_probe(struct rvin_dev *vin)
 {
 	struct video_device *vdev = &vin->vdev;
-	struct v4l2_subdev *sd = vin_to_source(vin);
 	int ret;
 
-	v4l2_set_subdev_hostdata(sd, vin);
-
 	vin->v4l2_dev.notify = rvin_notify;
 
-	ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
-	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
-		return ret;
-
-	if (vin->vdev.tvnorms == 0) {
-		/* Disable the STD API if there are no tvnorms defined */
-		v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
-		v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
-		v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
-		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
-	}
-
-	/* Add the controls */
-	/*
-	 * Currently the subdev with the largest number of controls (13) is
-	 * ov6550. So let's pick 16 as a hint for the control handler. Note
-	 * that this is a hint only: too large and you waste some memory, too
-	 * small and there is a (very) small performance hit when looking up
-	 * controls in the internal hash.
-	 */
-	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
-	if (ret < 0)
-		return ret;
-
-	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
-	if (ret < 0)
-		return ret;
-
 	/* video node */
 	vdev->fops = &rvin_fops;
 	vdev->v4l2_dev = &vin->v4l2_dev;
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index d31212a992e15506..2a1b1908ec1d52b5 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -111,6 +111,7 @@ struct rvin_graph_entity {
  * @sequence:		V4L2 buffers sequence number
  * @state:		keeps track of operation state
  *
+ * @last_input:		points to the last active input source
  * @source:		active format from the video source
  * @format:		active V4L2 pixel format
  *
@@ -138,6 +139,7 @@ struct rvin_dev {
 	unsigned int sequence;
 	enum rvin_dma_state state;
 
+	struct rvin_graph_entity *last_input;
 	struct rvin_source_fmt source;
 	struct v4l2_pix_format format;
 
-- 
2.11.0

Re: [PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]


> Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.

I can happily confirm that this series finally makes the I2C demuxer
work on the I2C bus with the HDMI clients because rebinding works now!
Note that I didn't test inputting any actual video but only the
rebinding capabilites. But since rebinding was a major motivation for
this series to be factored out of a bigger one:

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[Hans Verkuil]

Hi Niklas,

One general remark: in many commit logs you mistype 'subdeivce'. Can you
fix that for the v2?

On 01/31/2017 04:40 PM, Niklas Söderlund wrote:
> Hi,
> 
> This series address issues with the R-Car Gen2 VIN driver. The most 
> serious issue is the OPS when unbind and rebinding the i2c driver for 
> the video source subdevice which have popped up as a blocker for other 
> work.
> 
> This series is broken out of my much larger R-Car Gen3 enablement series 
> '[PATCHv2 00/32] rcar-vin: Add Gen3 with media controller support'. I 
> plan to remove that series form patchwork and focus on these fixes first 
> as they are blocking other development. Once the blocking issues are 
> removed I will rebase and repost the larger Gen3 series.
> 
> Patch 1-4 fix simple problems found while testing
>     1-2 Fix format problems when the format is (re)set.
>     3   Fix media pad errors
>     4   Fix standard enumeration problem
> 
> Patch 5 adds a wrapper function to retrieve the active video source 
> subdevice. This is strictly not needed on Gen2 which only have one 
> possible video source per VIN instance (This will change on Gen3). But 
> patch 6-8,11 which fixes real issues on Gen2 make use of this wrapper, as 
> not risk breaking things by removing this wrapper in this series and 
> then readding it in a later Gen3 series I have chosen to keep the patch.  
> Please let me know if I should drop it and rewrite patch 6-11 (if 
> possible I would like to avoid that).
> 
> Patch 6-8 deals with video source subdevice pad index handling by moving 
> the information from struct rvin_dev to struct rvin_graph_entity and 
> moving the pad index probing to the struct v4l2_async_notifier complete 
> callback. This is needed to allow the bind/unbind fix in patch 10-11.
> 
> Patch 9 use the pad information when calling enum_mbus_code.
> 
> Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.

This will not help: you can unbind a subdev at any time, including when
it is in use.

But why do you need this at all? You can also set suppress_bind_attrs in
the adv7180 driver to prevent the bind/unbind files from appearing.

It really makes no sense for subdevs. In fact, all subdevs should set this
flag since in the current implementation this is completely impossible to
implement safely.

I suggest you drop the patches relating to this and instead set the suppress
flag.

Regards,

	Hans

> 
> # echo 2-0020 > /sys/bus/i2c/drivers/adv7180/unbind
> # echo 2-0020 > /sys/bus/i2c/drivers/adv7180/bind
> 
>  adv7180 2-0020: chip found @ 0x20 (e6530000.i2c)
>  kobject (eaaab118): tried to init an initialized object, something is seriously wrong.
>  CPU: 0 PID: 1640 Comm: bash Not tainted 4.10.0-rc4-00029-g19b80f8913cad837 #1
>  Hardware name: Generic R8A7791 (Flattened Device Tree)
>  Backtrace: 
>  [<c010a858>] (dump_backtrace) from [<c010aaa4>] (show_stack+0x18/0x1c)
>   r7:00000016 r6:60070013 r5:00000000 r4:c0a14dd8
>  [<c010aa8c>] (show_stack) from [<c02de09c>] (dump_stack+0x84/0xa0)
>  [<c02de018>] (dump_stack) from [<c02dfee4>] (kobject_init+0x3c/0x98)
>   r7:00000016 r6:eaaab2e4 r5:c0a1f4dc r4:eaaab118
>  [<c02dfea8>] (kobject_init) from [<c03b9244>] (device_initialize+0x28/0xb0)
>   r5:c0a70be8 r4:eaaab110
>  [<c03b921c>] (device_initialize) from [<c03baa34>] (device_register+0x14/0x20)
>   r5:eaaab110 r4:eaaab110
>  [<c03baa20>] (device_register) from [<c04a02c0>] (__video_register_device+0xb38/0x11cc)
>   r5:eaaab110 r4:eaaab020
>  [<c049f788>] (__video_register_device) from [<c04c91a0>] (rvin_v4l2_probe+0x17c/0x1e8)
>   r10:00000000 r9:eaa3c050 r8:c0a270a8 r7:eaaab3a0 r6:eaaab020 r5:c0790068
>   r4:eaaab010
>  [<c04c9024>] (rvin_v4l2_probe) from [<c04c6da0>] (rvin_digital_notify_complete+0x174/0x184)
>   r7:00002006 r6:eaaab010 r5:00000000 r4:eaaab3e0
>  [<c04c6c2c>] (rvin_digital_notify_complete) from [<c04af180>] (v4l2_async_test_notify+0xe8/0xf0)
>   r7:eaaab410 r6:eaa3c050 r5:c04c6c2c r4:eaaab3e0
>  [<c04af098>] (v4l2_async_test_notify) from [<c04af560>] (v4l2_async_register_subdev+0xa4/0xcc)
>   r7:eaa3c0fc r6:c0a27094 r5:eaaab3e0 r4:eaa3c050
>  [<c04af4bc>] (v4l2_async_register_subdev) from [<c0497740>] (adv7180_probe+0x350/0x3e0)
>   r9:eaa3c050 r8:00000000 r7:00000000 r6:00000000 r5:eb2cbe00 r4:eaa3c010
>  [<c04973f0>] (adv7180_probe) from [<c048e9f4>] (i2c_device_probe+0x238/0x250)
>   r9:0000000e r8:c0a264dc r7:eb2cbe20 r6:c0a264dc r5:c04973f0 r4:eb2cbe00
>  [<c048e7bc>] (i2c_device_probe) from [<c03bd4f4>] (driver_probe_device+0x1f8/0x2c0)
>   r9:0000000e r8:c0a264dc r7:00000000 r6:c0a70c18 r5:c0a70c0c r4:eb2cbe20
>  [<c03bd2fc>] (driver_probe_device) from [<c03bbcd0>] (bind_store+0x94/0xe8)
>   r10:00000000 r9:00000051 r8:00000007 r7:c0a26058 r6:eb2cbe54 r5:c0a264dc
>   r4:eb2cbe20 r3:ea60b000
>  [<c03bbc3c>] (bind_store) from [<c03bb710>] (drv_attr_store+0x2c/0x38)
>   r9:00000051 r8:eb2daa0c r7:ea58ff80 r6:eb2daa00 r5:ea87a4c0 r4:c03bbc3c
>  [<c03bb6e4>] (drv_attr_store) from [<c023e5e4>] (sysfs_kf_write+0x40/0x4c)
>   r5:ea87a4c0 r4:c03bb6e4
>  [<c023e5a4>] (sysfs_kf_write) from [<c023dc50>] (kernfs_fop_write+0x13c/0x1ac)
>   r5:ea87a4c0 r4:00000007
>  [<c023db14>] (kernfs_fop_write) from [<c01e0c78>] (__vfs_write+0x34/0x114)
>   r9:ea58e000 r8:00000000 r7:00000007 r6:ea58ff80 r5:ea52a480 r4:c023db14
>  [<c01e0c44>] (__vfs_write) from [<c01e0ee4>] (vfs_write+0xc4/0x150)
>   r7:ea58ff80 r6:00167028 r5:00000007 r4:ea52a480
>  [<c01e0e20>] (vfs_write) from [<c01e1038>] (SyS_write+0x48/0x80)
>   r9:ea58e000 r8:c0106ee4 r7:00000007 r6:00167028 r5:ea52a480 r4:ea52a480
>  [<c01e0ff0>] (SyS_write) from [<c0106d20>] (ret_fast_syscall+0x0/0x3c)
>   r7:00000004 r6:b6dfed50 r5:00167028 r4:00000007
> 
> Niklas Söderlund (11):
>   media: rcar-vin: reset bytesperline and sizeimage when resetting
>     format
>   media: rcar-vin: use rvin_reset_format() in S_DV_TIMINGS
>   media: rcar-vin: fix how pads are handled for v4l2 subdeivce
>     operations
>   media: rcar-vin: fix standard in input enumeration
>   media: rcar-vin: add wrapper to get rvin_graph_entity
>   media: rcar-vin: move subdev source and sink pad index to
>     rvin_graph_entity
>   media: rcar-vin: move pad index discovery to async complete handler
>   media: rcar-vin: refactor pad lookup code
>   media: rcar-vin: use pad information when verifying media bus format
>   media: rcar-vin: split rvin_s_fmt_vid_cap()
>   media: rcar-vin: register the video device early
> 
>  drivers/media/platform/rcar-vin/rcar-core.c |  40 +++-
>  drivers/media/platform/rcar-vin/rcar-dma.c  |  15 +-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 346 +++++++++++++++-------------
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  20 +-
>  4 files changed, 244 insertions(+), 177 deletions(-)
> 

Re: [PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[Laurent Pinchart]

Hi Hans,

On Monday 13 Feb 2017 15:19:13 Hans Verkuil wrote:
> Hi Niklas,
> 
> One general remark: in many commit logs you mistype 'subdeivce'. Can you
> fix that for the v2?
> 
> On 01/31/2017 04:40 PM, Niklas Söderlund wrote:
> > Hi,
> > 
> > This series address issues with the R-Car Gen2 VIN driver. The most
> > serious issue is the OPS when unbind and rebinding the i2c driver for
> > the video source subdevice which have popped up as a blocker for other
> > work.
> > 
> > This series is broken out of my much larger R-Car Gen3 enablement series
> > '[PATCHv2 00/32] rcar-vin: Add Gen3 with media controller support'. I
> > plan to remove that series form patchwork and focus on these fixes first
> > as they are blocking other development. Once the blocking issues are
> > removed I will rebase and repost the larger Gen3 series.
> > 
> > Patch 1-4 fix simple problems found while testing
> > 
> >     1-2 Fix format problems when the format is (re)set.
> >     3   Fix media pad errors
> >     4   Fix standard enumeration problem
> > 
> > Patch 5 adds a wrapper function to retrieve the active video source
> > subdevice. This is strictly not needed on Gen2 which only have one
> > possible video source per VIN instance (This will change on Gen3). But
> > patch 6-8,11 which fixes real issues on Gen2 make use of this wrapper, as
> > not risk breaking things by removing this wrapper in this series and
> > then readding it in a later Gen3 series I have chosen to keep the patch.
> > Please let me know if I should drop it and rewrite patch 6-11 (if
> > possible I would like to avoid that).
> > 
> > Patch 6-8 deals with video source subdevice pad index handling by moving
> > the information from struct rvin_dev to struct rvin_graph_entity and
> > moving the pad index probing to the struct v4l2_async_notifier complete
> > callback. This is needed to allow the bind/unbind fix in patch 10-11.
> > 
> > Patch 9 use the pad information when calling enum_mbus_code.
> > 
> > Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.
> 
> This will not help: you can unbind a subdev at any time, including when
> it is in use.
> 
> But why do you need this at all? You can also set suppress_bind_attrs in
> the adv7180 driver to prevent the bind/unbind files from appearing.
> 
> It really makes no sense for subdevs. In fact, all subdevs should set this
> flag since in the current implementation this is completely impossible to
> implement safely.
> 
> I suggest you drop the patches relating to this and instead set the suppress
> flag.

The adv7180 is connected to an I2C controller that can be unbound. Setting the 
suppress_bind_attrs flag in the driver thus won't prevent the device from 
being unbound. suppress_bind_attrs is not a good solution for I2C drivers.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[Hans Verkuil]

On 02/13/2017 04:31 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 13 Feb 2017 15:19:13 Hans Verkuil wrote:
>> Hi Niklas,
>>
>> One general remark: in many commit logs you mistype 'subdeivce'. Can you
>> fix that for the v2?
>>
>> On 01/31/2017 04:40 PM, Niklas Söderlund wrote:
>>> Hi,
>>>
>>> This series address issues with the R-Car Gen2 VIN driver. The most
>>> serious issue is the OPS when unbind and rebinding the i2c driver for
>>> the video source subdevice which have popped up as a blocker for other
>>> work.
>>>
>>> This series is broken out of my much larger R-Car Gen3 enablement series
>>> '[PATCHv2 00/32] rcar-vin: Add Gen3 with media controller support'. I
>>> plan to remove that series form patchwork and focus on these fixes first
>>> as they are blocking other development. Once the blocking issues are
>>> removed I will rebase and repost the larger Gen3 series.
>>>
>>> Patch 1-4 fix simple problems found while testing
>>>
>>>     1-2 Fix format problems when the format is (re)set.
>>>     3   Fix media pad errors
>>>     4   Fix standard enumeration problem
>>>
>>> Patch 5 adds a wrapper function to retrieve the active video source
>>> subdevice. This is strictly not needed on Gen2 which only have one
>>> possible video source per VIN instance (This will change on Gen3). But
>>> patch 6-8,11 which fixes real issues on Gen2 make use of this wrapper, as
>>> not risk breaking things by removing this wrapper in this series and
>>> then readding it in a later Gen3 series I have chosen to keep the patch.
>>> Please let me know if I should drop it and rewrite patch 6-11 (if
>>> possible I would like to avoid that).
>>>
>>> Patch 6-8 deals with video source subdevice pad index handling by moving
>>> the information from struct rvin_dev to struct rvin_graph_entity and
>>> moving the pad index probing to the struct v4l2_async_notifier complete
>>> callback. This is needed to allow the bind/unbind fix in patch 10-11.
>>>
>>> Patch 9 use the pad information when calling enum_mbus_code.
>>>
>>> Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.
>>
>> This will not help: you can unbind a subdev at any time, including when
>> it is in use.
>>
>> But why do you need this at all? You can also set suppress_bind_attrs in
>> the adv7180 driver to prevent the bind/unbind files from appearing.
>>
>> It really makes no sense for subdevs. In fact, all subdevs should set this
>> flag since in the current implementation this is completely impossible to
>> implement safely.
>>
>> I suggest you drop the patches relating to this and instead set the suppress
>> flag.
> 
> The adv7180 is connected to an I2C controller that can be unbound. Setting the 
> suppress_bind_attrs flag in the driver thus won't prevent the device from 
> being unbound. suppress_bind_attrs is not a good solution for I2C drivers.

Then just drop these patches, since those don't solve anything either. Without
some of the things we discussed during the refcounting meeting (i.e. a locking
scheme before calling into subdev ops) anything you do will at best just give
you an illusion that you're safe.

Regards,

	Hans

Re: [PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

On 2017-02-13 15:19:13 +0100, Hans Verkuil wrote:
> Hi Niklas,
> 
> One general remark: in many commit logs you mistype 'subdeivce'. Can you
> fix that for the v2?

Will fix for v2.

> 
> On 01/31/2017 04:40 PM, Niklas Söderlund wrote:
> > Hi,
> > 
> > This series address issues with the R-Car Gen2 VIN driver. The most 
> > serious issue is the OPS when unbind and rebinding the i2c driver for 
> > the video source subdevice which have popped up as a blocker for other 
> > work.
> > 
> > This series is broken out of my much larger R-Car Gen3 enablement series 
> > '[PATCHv2 00/32] rcar-vin: Add Gen3 with media controller support'. I 
> > plan to remove that series form patchwork and focus on these fixes first 
> > as they are blocking other development. Once the blocking issues are 
> > removed I will rebase and repost the larger Gen3 series.
> > 
> > Patch 1-4 fix simple problems found while testing
> >     1-2 Fix format problems when the format is (re)set.
> >     3   Fix media pad errors
> >     4   Fix standard enumeration problem
> > 
> > Patch 5 adds a wrapper function to retrieve the active video source 
> > subdevice. This is strictly not needed on Gen2 which only have one 
> > possible video source per VIN instance (This will change on Gen3). But 
> > patch 6-8,11 which fixes real issues on Gen2 make use of this wrapper, as 
> > not risk breaking things by removing this wrapper in this series and 
> > then readding it in a later Gen3 series I have chosen to keep the patch.  
> > Please let me know if I should drop it and rewrite patch 6-11 (if 
> > possible I would like to avoid that).
> > 
> > Patch 6-8 deals with video source subdevice pad index handling by moving 
> > the information from struct rvin_dev to struct rvin_graph_entity and 
> > moving the pad index probing to the struct v4l2_async_notifier complete 
> > callback. This is needed to allow the bind/unbind fix in patch 10-11.
> > 
> > Patch 9 use the pad information when calling enum_mbus_code.
> > 
> > Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.
> 
> This will not help: you can unbind a subdev at any time, including when
> it is in use.

Yes, this series will not help remedy the problem if a device is in use.  
But I still find it useful since it unblocks other work, solves a OPS 
and if there are no current users the driver can supports unbind/bind of 
its subdevices, it's not perfect but it do make things a little better 
IMHO.

If it where not for me wishing to reuse the behavior introduced in patch 
10-11 on R-Car Gen3 when a subdevice could not available for for other 
reasons than it's not bound (see bellow) I would be happy to drop the 
patches from this series.

> 
> But why do you need this at all? You can also set suppress_bind_attrs in
> the adv7180 driver to prevent the bind/unbind files from appearing.

The primary use-case for this is on R-Car Gen3 where there are a limited 
number of possible routing options to connect CSI-2 devices to VIN 
devices (set in hardware), see table bellow. The routing possibilities 
are set per VIN group (VIN0-3 and VIN4-7) and not individually for each 
VIN device. Given this limitation some routing options will result in an 
N/A video source for one or more VIN devices in a VIN "group".

   - VIN0-3 controlled by chsel register in VIN0
   chsel    VIN0        VIN1        VIN2        VIN3
   0        CSI40/VC0   CSI20/VC0   N/A         CSI40/VC1
   1        CSI20/VC0   N/A         CSI40/VC0   CSI20/VC1
   2        N/A         CSI40/VC0   CSI20/VC0   N/A
   3        CSI40/VC0   CSI40/VC1   CSI40/VC2   CSI40/VC3
   4        CSI20/VC0   CSI20/VC1   CSI20/VC2   CSI20/VC3

   - VIN4-7 controlled by chsel register in VIN4
   chsel    VIN4        VIN5        VIN6        VIN7
   0        CSI40/VC0   CSI20/VC0   N/A         CSI40/VC1
   1        CSI20/VC0   N/A         CSI40/VC0   CSI20/VC1
   2        N/A         CSI40/VC0   CSI20/VC0   N/A
   3        CSI40/VC0   CSI40/VC1   CSI40/VC2   CSI40/VC3
   4        CSI20/VC0   CSI20/VC1   CSI20/VC2   CSI20/VC3

Example: If a VIN1 device is exposed as /dev/video1 and the current 
CSI-2 to VIN routing configuration controlled by the chsel register in 
VIN0 is set to 1 the video source routed to VIN1 is N/A. The idea then 
is that any open of /dev/video1 should result in -EBUSY until the CSI-2 
to VIN routing changes so that VIN1 is connected to a valid subdevice 
again. (side note: changing chsel value will not be allowed while any 
VIN device is opened and is done using MC)

This series was originally part of my R-Car Gen3 enablement series so I
chose to keep this behavior even if the underlying Gen2 OPS could be 
fixed in a different way. With this solution a unavailable subdevice 
(subdev not bound on Gen2+Gen3 or a N/A subdevice due to routing setup 
on Gen3) would be handled the same (-EBUSY) on both Gen2 and Gen3.

All testing I have done on the driver both on Gen2 and Gen3 have been 
based on this solution for quiet a while now. And it seemed strange for 
me to try and solve the Gen2 issue differently only to rework it again 
later in the Gen3 enablement series.

I'm sorry that I did not explain more about this in the original cover 
letter. Did this explanation help clear things up? And is the idea of 
returning -EBUSY a OK solution in general to the problem that a video 
device who once had all its subdevices available no longer do so, but it 
might get them back in the future? I'm happy too keep working and 
improving this solution, this is only the best one i found so far :-)

> 
> It really makes no sense for subdevs. In fact, all subdevs should set this
> flag since in the current implementation this is completely impossible to
> implement safely.
> 
> I suggest you drop the patches relating to this and instead set the suppress
> flag.

If possible I would like to explore the possibility to keep it in the 
series. I think it would be an advantage to treat on unbound subdevice 
on Gen2 in the same way as a VIN instance on Gen3 would treat a CSI-2 to 
VIN routing configuration with a N/A route.

I am of course willing to rework the behavior to something else then 
returning -EBUSY if a VIN instance currently have all subdevices 
available for some reason. I would like input on how such a scheme could 
look like since the -EBUSY one is the only solution I have managed to 
figure out and implement.

And thanks again for your feedback, I really love to see some R-Car VIN 
work move forward. Let me know if I can do anything to ease the process.

> 
> Regards,
> 
> 	Hans
> 
> > 
> > # echo 2-0020 > /sys/bus/i2c/drivers/adv7180/unbind
> > # echo 2-0020 > /sys/bus/i2c/drivers/adv7180/bind
> > 
> >  adv7180 2-0020: chip found @ 0x20 (e6530000.i2c)
> >  kobject (eaaab118): tried to init an initialized object, something is seriously wrong.
> >  CPU: 0 PID: 1640 Comm: bash Not tainted 4.10.0-rc4-00029-g19b80f8913cad837 #1
> >  Hardware name: Generic R8A7791 (Flattened Device Tree)
> >  Backtrace: 
> >  [<c010a858>] (dump_backtrace) from [<c010aaa4>] (show_stack+0x18/0x1c)
> >   r7:00000016 r6:60070013 r5:00000000 r4:c0a14dd8
> >  [<c010aa8c>] (show_stack) from [<c02de09c>] (dump_stack+0x84/0xa0)
> >  [<c02de018>] (dump_stack) from [<c02dfee4>] (kobject_init+0x3c/0x98)
> >   r7:00000016 r6:eaaab2e4 r5:c0a1f4dc r4:eaaab118
> >  [<c02dfea8>] (kobject_init) from [<c03b9244>] (device_initialize+0x28/0xb0)
> >   r5:c0a70be8 r4:eaaab110
> >  [<c03b921c>] (device_initialize) from [<c03baa34>] (device_register+0x14/0x20)
> >   r5:eaaab110 r4:eaaab110
> >  [<c03baa20>] (device_register) from [<c04a02c0>] (__video_register_device+0xb38/0x11cc)
> >   r5:eaaab110 r4:eaaab020
> >  [<c049f788>] (__video_register_device) from [<c04c91a0>] (rvin_v4l2_probe+0x17c/0x1e8)
> >   r10:00000000 r9:eaa3c050 r8:c0a270a8 r7:eaaab3a0 r6:eaaab020 r5:c0790068
> >   r4:eaaab010
> >  [<c04c9024>] (rvin_v4l2_probe) from [<c04c6da0>] (rvin_digital_notify_complete+0x174/0x184)
> >   r7:00002006 r6:eaaab010 r5:00000000 r4:eaaab3e0
> >  [<c04c6c2c>] (rvin_digital_notify_complete) from [<c04af180>] (v4l2_async_test_notify+0xe8/0xf0)
> >   r7:eaaab410 r6:eaa3c050 r5:c04c6c2c r4:eaaab3e0
> >  [<c04af098>] (v4l2_async_test_notify) from [<c04af560>] (v4l2_async_register_subdev+0xa4/0xcc)
> >   r7:eaa3c0fc r6:c0a27094 r5:eaaab3e0 r4:eaa3c050
> >  [<c04af4bc>] (v4l2_async_register_subdev) from [<c0497740>] (adv7180_probe+0x350/0x3e0)
> >   r9:eaa3c050 r8:00000000 r7:00000000 r6:00000000 r5:eb2cbe00 r4:eaa3c010
> >  [<c04973f0>] (adv7180_probe) from [<c048e9f4>] (i2c_device_probe+0x238/0x250)
> >   r9:0000000e r8:c0a264dc r7:eb2cbe20 r6:c0a264dc r5:c04973f0 r4:eb2cbe00
> >  [<c048e7bc>] (i2c_device_probe) from [<c03bd4f4>] (driver_probe_device+0x1f8/0x2c0)
> >   r9:0000000e r8:c0a264dc r7:00000000 r6:c0a70c18 r5:c0a70c0c r4:eb2cbe20
> >  [<c03bd2fc>] (driver_probe_device) from [<c03bbcd0>] (bind_store+0x94/0xe8)
> >   r10:00000000 r9:00000051 r8:00000007 r7:c0a26058 r6:eb2cbe54 r5:c0a264dc
> >   r4:eb2cbe20 r3:ea60b000
> >  [<c03bbc3c>] (bind_store) from [<c03bb710>] (drv_attr_store+0x2c/0x38)
> >   r9:00000051 r8:eb2daa0c r7:ea58ff80 r6:eb2daa00 r5:ea87a4c0 r4:c03bbc3c
> >  [<c03bb6e4>] (drv_attr_store) from [<c023e5e4>] (sysfs_kf_write+0x40/0x4c)
> >   r5:ea87a4c0 r4:c03bb6e4
> >  [<c023e5a4>] (sysfs_kf_write) from [<c023dc50>] (kernfs_fop_write+0x13c/0x1ac)
> >   r5:ea87a4c0 r4:00000007
> >  [<c023db14>] (kernfs_fop_write) from [<c01e0c78>] (__vfs_write+0x34/0x114)
> >   r9:ea58e000 r8:00000000 r7:00000007 r6:ea58ff80 r5:ea52a480 r4:c023db14
> >  [<c01e0c44>] (__vfs_write) from [<c01e0ee4>] (vfs_write+0xc4/0x150)
> >   r7:ea58ff80 r6:00167028 r5:00000007 r4:ea52a480
> >  [<c01e0e20>] (vfs_write) from [<c01e1038>] (SyS_write+0x48/0x80)
> >   r9:ea58e000 r8:c0106ee4 r7:00000007 r6:00167028 r5:ea52a480 r4:ea52a480
> >  [<c01e0ff0>] (SyS_write) from [<c0106d20>] (ret_fast_syscall+0x0/0x3c)
> >   r7:00000004 r6:b6dfed50 r5:00167028 r4:00000007
> > 
> > Niklas Söderlund (11):
> >   media: rcar-vin: reset bytesperline and sizeimage when resetting
> >     format
> >   media: rcar-vin: use rvin_reset_format() in S_DV_TIMINGS
> >   media: rcar-vin: fix how pads are handled for v4l2 subdeivce
> >     operations
> >   media: rcar-vin: fix standard in input enumeration
> >   media: rcar-vin: add wrapper to get rvin_graph_entity
> >   media: rcar-vin: move subdev source and sink pad index to
> >     rvin_graph_entity
> >   media: rcar-vin: move pad index discovery to async complete handler
> >   media: rcar-vin: refactor pad lookup code
> >   media: rcar-vin: use pad information when verifying media bus format
> >   media: rcar-vin: split rvin_s_fmt_vid_cap()
> >   media: rcar-vin: register the video device early
> > 
> >  drivers/media/platform/rcar-vin/rcar-core.c |  40 +++-
> >  drivers/media/platform/rcar-vin/rcar-dma.c  |  15 +-
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 346 +++++++++++++++-------------
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  20 +-
> >  4 files changed, 244 insertions(+), 177 deletions(-)
> > 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[Hans Verkuil]

On 02/13/2017 06:47 PM, Niklas Söderlund wrote:
> Hi Hans,
> 
> Thanks for your feedback.
> 
> On 2017-02-13 15:19:13 +0100, Hans Verkuil wrote:
>> Hi Niklas,
>>
>> One general remark: in many commit logs you mistype 'subdeivce'. Can you
>> fix that for the v2?
> 
> Will fix for v2.
> 
>>
>> On 01/31/2017 04:40 PM, Niklas Söderlund wrote:
>>> Hi,
>>>
>>> This series address issues with the R-Car Gen2 VIN driver. The most 
>>> serious issue is the OPS when unbind and rebinding the i2c driver for 
>>> the video source subdevice which have popped up as a blocker for other 
>>> work.
>>>
>>> This series is broken out of my much larger R-Car Gen3 enablement series 
>>> '[PATCHv2 00/32] rcar-vin: Add Gen3 with media controller support'. I 
>>> plan to remove that series form patchwork and focus on these fixes first 
>>> as they are blocking other development. Once the blocking issues are 
>>> removed I will rebase and repost the larger Gen3 series.
>>>
>>> Patch 1-4 fix simple problems found while testing
>>>     1-2 Fix format problems when the format is (re)set.
>>>     3   Fix media pad errors
>>>     4   Fix standard enumeration problem
>>>
>>> Patch 5 adds a wrapper function to retrieve the active video source 
>>> subdevice. This is strictly not needed on Gen2 which only have one 
>>> possible video source per VIN instance (This will change on Gen3). But 
>>> patch 6-8,11 which fixes real issues on Gen2 make use of this wrapper, as 
>>> not risk breaking things by removing this wrapper in this series and 
>>> then readding it in a later Gen3 series I have chosen to keep the patch.  
>>> Please let me know if I should drop it and rewrite patch 6-11 (if 
>>> possible I would like to avoid that).
>>>
>>> Patch 6-8 deals with video source subdevice pad index handling by moving 
>>> the information from struct rvin_dev to struct rvin_graph_entity and 
>>> moving the pad index probing to the struct v4l2_async_notifier complete 
>>> callback. This is needed to allow the bind/unbind fix in patch 10-11.
>>>
>>> Patch 9 use the pad information when calling enum_mbus_code.
>>>
>>> Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.
>>
>> This will not help: you can unbind a subdev at any time, including when
>> it is in use.
> 
> Yes, this series will not help remedy the problem if a device is in use.  
> But I still find it useful since it unblocks other work, solves a OPS 
> and if there are no current users the driver can supports unbind/bind of 
> its subdevices, it's not perfect but it do make things a little better 
> IMHO.
> 
> If it where not for me wishing to reuse the behavior introduced in patch 
> 10-11 on R-Car Gen3 when a subdevice could not available for for other 
> reasons than it's not bound (see bellow) I would be happy to drop the 
> patches from this series.
> 
>>
>> But why do you need this at all? You can also set suppress_bind_attrs in
>> the adv7180 driver to prevent the bind/unbind files from appearing.
> 
> The primary use-case for this is on R-Car Gen3 where there are a limited 
> number of possible routing options to connect CSI-2 devices to VIN 
> devices (set in hardware), see table bellow. The routing possibilities 
> are set per VIN group (VIN0-3 and VIN4-7) and not individually for each 
> VIN device. Given this limitation some routing options will result in an 
> N/A video source for one or more VIN devices in a VIN "group".
> 
>    - VIN0-3 controlled by chsel register in VIN0
>    chsel    VIN0        VIN1        VIN2        VIN3
>    0        CSI40/VC0   CSI20/VC0   N/A         CSI40/VC1
>    1        CSI20/VC0   N/A         CSI40/VC0   CSI20/VC1
>    2        N/A         CSI40/VC0   CSI20/VC0   N/A
>    3        CSI40/VC0   CSI40/VC1   CSI40/VC2   CSI40/VC3
>    4        CSI20/VC0   CSI20/VC1   CSI20/VC2   CSI20/VC3
> 
>    - VIN4-7 controlled by chsel register in VIN4
>    chsel    VIN4        VIN5        VIN6        VIN7
>    0        CSI40/VC0   CSI20/VC0   N/A         CSI40/VC1
>    1        CSI20/VC0   N/A         CSI40/VC0   CSI20/VC1
>    2        N/A         CSI40/VC0   CSI20/VC0   N/A
>    3        CSI40/VC0   CSI40/VC1   CSI40/VC2   CSI40/VC3
>    4        CSI20/VC0   CSI20/VC1   CSI20/VC2   CSI20/VC3
> 
> Example: If a VIN1 device is exposed as /dev/video1 and the current 
> CSI-2 to VIN routing configuration controlled by the chsel register in 
> VIN0 is set to 1 the video source routed to VIN1 is N/A. The idea then 
> is that any open of /dev/video1 should result in -EBUSY until the CSI-2 
> to VIN routing changes so that VIN1 is connected to a valid subdevice 
> again. (side note: changing chsel value will not be allowed while any 
> VIN device is opened and is done using MC)
> 
> This series was originally part of my R-Car Gen3 enablement series so I
> chose to keep this behavior even if the underlying Gen2 OPS could be 
> fixed in a different way. With this solution a unavailable subdevice 
> (subdev not bound on Gen2+Gen3 or a N/A subdevice due to routing setup 
> on Gen3) would be handled the same (-EBUSY) on both Gen2 and Gen3.
> 
> All testing I have done on the driver both on Gen2 and Gen3 have been 
> based on this solution for quiet a while now. And it seemed strange for 
> me to try and solve the Gen2 issue differently only to rework it again 
> later in the Gen3 enablement series.
> 
> I'm sorry that I did not explain more about this in the original cover 
> letter. Did this explanation help clear things up? And is the idea of 
> returning -EBUSY a OK solution in general to the problem that a video 
> device who once had all its subdevices available no longer do so, but it 
> might get them back in the future? I'm happy too keep working and 
> improving this solution, this is only the best one i found so far :-)
> 
>>
>> It really makes no sense for subdevs. In fact, all subdevs should set this
>> flag since in the current implementation this is completely impossible to
>> implement safely.
>>
>> I suggest you drop the patches relating to this and instead set the suppress
>> flag.
> 
> If possible I would like to explore the possibility to keep it in the 
> series. I think it would be an advantage to treat on unbound subdevice 
> on Gen2 in the same way as a VIN instance on Gen3 would treat a CSI-2 to 
> VIN routing configuration with a N/A route.
> 
> I am of course willing to rework the behavior to something else then 
> returning -EBUSY if a VIN instance currently have all subdevices 
> available for some reason. I would like input on how such a scheme could 
> look like since the -EBUSY one is the only solution I have managed to 
> figure out and implement.

I think the core problem here is perhaps less the patches but more the commit
logs you wrote. You say in several places that is it 'To support unbind and
rebinding of subdevices'. But it doesn't support that, instead it really
prepares for a specific Gen3 use-case.

All existing drivers with subdevs can be induced to oops if you unbind/bind
a subdev. It is broken at the core, and without fixing it at the core first,
anything you try to do in a driver is just a band-aid.

Take patch 07/11, the commit log says:

"To fix support for unbind and rebinding of subdevices the
rvin_v4l2_probe() needs to be called before there might be any subdevice
bound. Move pad index discovery to when we know the subdevice is
present."

This commit log is very misleading. It suggests that this patch fixes
unbind/rebind support, which it doesn't. Instead, it just prepares for
patch 11/11 where you want to (partially?) re-initialize the subdevs at
first open. And that in turn is for future Gen3 support.

None of this has anything to do with unbind/rebind. The fact that you
can now do this if there are no filehandles open is a side-effect, and not
the main reason for the patches.

I recommend that you take another look and rewrite the commit logs so they
reflect the real reason you do this.

> And thanks again for your feedback, I really love to see some R-Car VIN 
> work move forward. Let me know if I can do anything to ease the process.

Sure, no problem. I've not been a good reviewer recently, but it's getting
better as I have more time now.

Regards,

	Hans

Re: [PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

[Niklas Söderlund]

Hi Hans,

On 2017-02-13 21:57:48 +0100, Hans Verkuil wrote:
> On 02/13/2017 06:47 PM, Niklas Söderlund wrote:
> > Hi Hans,
> > 
> > Thanks for your feedback.
> > 
> > On 2017-02-13 15:19:13 +0100, Hans Verkuil wrote:
> >> Hi Niklas,
> >>
> >> One general remark: in many commit logs you mistype 'subdeivce'. Can you
> >> fix that for the v2?
> > 
> > Will fix for v2.
> > 
> >>
> >> On 01/31/2017 04:40 PM, Niklas Söderlund wrote:
> >>> Hi,
> >>>
> >>> This series address issues with the R-Car Gen2 VIN driver. The most 
> >>> serious issue is the OPS when unbind and rebinding the i2c driver for 
> >>> the video source subdevice which have popped up as a blocker for other 
> >>> work.
> >>>
> >>> This series is broken out of my much larger R-Car Gen3 enablement series 
> >>> '[PATCHv2 00/32] rcar-vin: Add Gen3 with media controller support'. I 
> >>> plan to remove that series form patchwork and focus on these fixes first 
> >>> as they are blocking other development. Once the blocking issues are 
> >>> removed I will rebase and repost the larger Gen3 series.
> >>>
> >>> Patch 1-4 fix simple problems found while testing
> >>>     1-2 Fix format problems when the format is (re)set.
> >>>     3   Fix media pad errors
> >>>     4   Fix standard enumeration problem
> >>>
> >>> Patch 5 adds a wrapper function to retrieve the active video source 
> >>> subdevice. This is strictly not needed on Gen2 which only have one 
> >>> possible video source per VIN instance (This will change on Gen3). But 
> >>> patch 6-8,11 which fixes real issues on Gen2 make use of this wrapper, as 
> >>> not risk breaking things by removing this wrapper in this series and 
> >>> then readding it in a later Gen3 series I have chosen to keep the patch.  
> >>> Please let me know if I should drop it and rewrite patch 6-11 (if 
> >>> possible I would like to avoid that).
> >>>
> >>> Patch 6-8 deals with video source subdevice pad index handling by moving 
> >>> the information from struct rvin_dev to struct rvin_graph_entity and 
> >>> moving the pad index probing to the struct v4l2_async_notifier complete 
> >>> callback. This is needed to allow the bind/unbind fix in patch 10-11.
> >>>
> >>> Patch 9 use the pad information when calling enum_mbus_code.
> >>>
> >>> Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.
> >>
> >> This will not help: you can unbind a subdev at any time, including when
> >> it is in use.
> > 
> > Yes, this series will not help remedy the problem if a device is in use.  
> > But I still find it useful since it unblocks other work, solves a OPS 
> > and if there are no current users the driver can supports unbind/bind of 
> > its subdevices, it's not perfect but it do make things a little better 
> > IMHO.
> > 
> > If it where not for me wishing to reuse the behavior introduced in patch 
> > 10-11 on R-Car Gen3 when a subdevice could not available for for other 
> > reasons than it's not bound (see bellow) I would be happy to drop the 
> > patches from this series.
> > 
> >>
> >> But why do you need this at all? You can also set suppress_bind_attrs in
> >> the adv7180 driver to prevent the bind/unbind files from appearing.
> > 
> > The primary use-case for this is on R-Car Gen3 where there are a limited 
> > number of possible routing options to connect CSI-2 devices to VIN 
> > devices (set in hardware), see table bellow. The routing possibilities 
> > are set per VIN group (VIN0-3 and VIN4-7) and not individually for each 
> > VIN device. Given this limitation some routing options will result in an 
> > N/A video source for one or more VIN devices in a VIN "group".
> > 
> >    - VIN0-3 controlled by chsel register in VIN0
> >    chsel    VIN0        VIN1        VIN2        VIN3
> >    0        CSI40/VC0   CSI20/VC0   N/A         CSI40/VC1
> >    1        CSI20/VC0   N/A         CSI40/VC0   CSI20/VC1
> >    2        N/A         CSI40/VC0   CSI20/VC0   N/A
> >    3        CSI40/VC0   CSI40/VC1   CSI40/VC2   CSI40/VC3
> >    4        CSI20/VC0   CSI20/VC1   CSI20/VC2   CSI20/VC3
> > 
> >    - VIN4-7 controlled by chsel register in VIN4
> >    chsel    VIN4        VIN5        VIN6        VIN7
> >    0        CSI40/VC0   CSI20/VC0   N/A         CSI40/VC1
> >    1        CSI20/VC0   N/A         CSI40/VC0   CSI20/VC1
> >    2        N/A         CSI40/VC0   CSI20/VC0   N/A
> >    3        CSI40/VC0   CSI40/VC1   CSI40/VC2   CSI40/VC3
> >    4        CSI20/VC0   CSI20/VC1   CSI20/VC2   CSI20/VC3
> > 
> > Example: If a VIN1 device is exposed as /dev/video1 and the current 
> > CSI-2 to VIN routing configuration controlled by the chsel register in 
> > VIN0 is set to 1 the video source routed to VIN1 is N/A. The idea then 
> > is that any open of /dev/video1 should result in -EBUSY until the CSI-2 
> > to VIN routing changes so that VIN1 is connected to a valid subdevice 
> > again. (side note: changing chsel value will not be allowed while any 
> > VIN device is opened and is done using MC)
> > 
> > This series was originally part of my R-Car Gen3 enablement series so I
> > chose to keep this behavior even if the underlying Gen2 OPS could be 
> > fixed in a different way. With this solution a unavailable subdevice 
> > (subdev not bound on Gen2+Gen3 or a N/A subdevice due to routing setup 
> > on Gen3) would be handled the same (-EBUSY) on both Gen2 and Gen3.
> > 
> > All testing I have done on the driver both on Gen2 and Gen3 have been 
> > based on this solution for quiet a while now. And it seemed strange for 
> > me to try and solve the Gen2 issue differently only to rework it again 
> > later in the Gen3 enablement series.
> > 
> > I'm sorry that I did not explain more about this in the original cover 
> > letter. Did this explanation help clear things up? And is the idea of 
> > returning -EBUSY a OK solution in general to the problem that a video 
> > device who once had all its subdevices available no longer do so, but it 
> > might get them back in the future? I'm happy too keep working and 
> > improving this solution, this is only the best one i found so far :-)
> > 
> >>
> >> It really makes no sense for subdevs. In fact, all subdevs should set this
> >> flag since in the current implementation this is completely impossible to
> >> implement safely.
> >>
> >> I suggest you drop the patches relating to this and instead set the suppress
> >> flag.
> > 
> > If possible I would like to explore the possibility to keep it in the 
> > series. I think it would be an advantage to treat on unbound subdevice 
> > on Gen2 in the same way as a VIN instance on Gen3 would treat a CSI-2 to 
> > VIN routing configuration with a N/A route.
> > 
> > I am of course willing to rework the behavior to something else then 
> > returning -EBUSY if a VIN instance currently have all subdevices 
> > available for some reason. I would like input on how such a scheme could 
> > look like since the -EBUSY one is the only solution I have managed to 
> > figure out and implement.
> 
> I think the core problem here is perhaps less the patches but more the commit
> logs you wrote. You say in several places that is it 'To support unbind and
> rebinding of subdevices'. But it doesn't support that, instead it really
> prepares for a specific Gen3 use-case.
> 
> All existing drivers with subdevs can be induced to oops if you unbind/bind
> a subdev. It is broken at the core, and without fixing it at the core first,
> anything you try to do in a driver is just a band-aid.

I see your point, the patches where extracted from my Gen3 enablement 
series to solve the OPS reported by Wolfram when unbinding/binding on 
Gen2. But yes they try to solve the issue by preparing for the wanted 
Gen3 behavior where this particular OPS do not happen (if the device is 
unused).

I do now understand that trying to fix OPS related to unbind/bind is 
futile without core support.

> 
> Take patch 07/11, the commit log says:
> 
> "To fix support for unbind and rebinding of subdevices the
> rvin_v4l2_probe() needs to be called before there might be any subdevice
> bound. Move pad index discovery to when we know the subdevice is
> present."
> 
> This commit log is very misleading. It suggests that this patch fixes
> unbind/rebind support, which it doesn't. Instead, it just prepares for
> patch 11/11 where you want to (partially?) re-initialize the subdevs at
> first open. And that in turn is for future Gen3 support.
> 
> None of this has anything to do with unbind/rebind. The fact that you
> can now do this if there are no filehandles open is a side-effect, and not
> the main reason for the patches.
> 
> I recommend that you take another look and rewrite the commit logs so they
> reflect the real reason you do this.

I will do so and post a new version, thanks for your feedback.

> 
> > And thanks again for your feedback, I really love to see some R-Car VIN 
> > work move forward. Let me know if I can do anything to ease the process.
> 
> Sure, no problem. I've not been a good reviewer recently, but it's getting
> better as I have more time now.
> 
> Regards,
> 
> 	Hans

-- 
Regards,
Niklas Söderlund