[PATCH v7 00/10] v4l2-subdev: Introduce [g|s]et_mbus_format pad op

[PATCH v7 00/10] v4l2-subdev: Introduce [g|s]et_mbus_format pad op

[Jacopo Mondi]

Minor update to add a new patch [6/10] to v4l2-mediabus.h with a usage
note on the V4L2_MBUS_* flags.

Also add Niklas' tag to [10/10] and address his final comment there.

Hans, this should now be ready to be hopefully collected.

Thanks
  j

v6.1->v7
- Add [6/10] as suggested by Hans
- Add Niklas tag and fix his last comment in [10/10]

v6->v6.1
- Address Niklas' comments in the last patch for rcar-csi2

v5->v6:
- Report V4L2_MBUS_DATA_ACTIVE_HIGH in ov6650 get_mbus_config
- Check for the return value of get_mbus_config() at the end of
  set_mbus_config() in ov6650 driver

v4->v5:
- Address Sakari's comment on documentation (s/should/shall)
- Use a local variable for the number of active lanes in 9/9
- Add Kieran's tags to 7/9 and 8/9
- Fix a warning on operator precedence on 3/9

v3->v4:
- Remove g/s_mbus_config video operation
- Adjust pxa quick capture interface to properly handle bus mastering
- Reword the two new operations documentation

v2->v3:
- Re-use v4l2_mbus_config and V4L2_MBUS_* flags
- Port existing drivers
- Update adv748x and rcar-csi2 patches to use V4L2_MBUS_* flags

v1->v2:
- Address Sakari's comment to use unsigned int in place of bools
- Add two new patches to address documentation
- Adjust rcar-csi2 patch as much as possible according to Niklas comments
- Add Niklas's tags

Jacopo Mondi (10):
  media: v4l2-subdev: Introduce [get|set]_mbus_config pad ops
  media: i2c: Use the new get_mbus_config pad op
  media: i2c: ov6650: Use new [get|set]_mbus_config ops
  media: pxa_camera: Use the new set_mbus_config op
  media: v4l2-subdev: Remove [s|g]_mbus_config video ops
  media: v4l2- mediabus: Add usage note for V4L2_MBUS_*
  staging: media: imx: Update TODO entry
  media: i2c: adv748x: Adjust TXA data lanes number
  media: i2c: adv748x: Implement get_mbus_config
  media: rcar-csi2: Negotiate data lanes number

 drivers/media/i2c/adv7180.c                 |   7 +-
 drivers/media/i2c/adv748x/adv748x-core.c    |  31 +++-
 drivers/media/i2c/adv748x/adv748x-csi2.c    |  31 ++++
 drivers/media/i2c/adv748x/adv748x.h         |   1 +
 drivers/media/i2c/ml86v7667.c               |   7 +-
 drivers/media/i2c/mt9m001.c                 |   7 +-
 drivers/media/i2c/mt9m111.c                 |   7 +-
 drivers/media/i2c/ov6650.c                  |  56 ++++--
 drivers/media/i2c/ov9640.c                  |   7 +-
 drivers/media/i2c/tc358743.c                |   7 +-
 drivers/media/i2c/tvp5150.c                 |   7 +-
 drivers/media/platform/pxa_camera.c         | 189 ++++++--------------
 drivers/media/platform/rcar-vin/rcar-csi2.c |  75 +++++++-
 drivers/staging/media/imx/TODO              |   4 +
 include/media/v4l2-mediabus.h               |  33 +++-
 include/media/v4l2-subdev.h                 |  37 ++--
 16 files changed, 301 insertions(+), 205 deletions(-)

--
2.27.0

[PATCH v7 01/10] media: v4l2-subdev: Introduce [get|set]_mbus_config pad ops

[Jacopo Mondi]

Introduce two new pad operations to allow retrieving and configuring the
media bus parameters on a subdevice pad.

The newly introduced operations aims to replace the s/g_mbus_config video
operations, which have been on their way for deprecation since a long
time.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index f7fe78a6f65a..d8b9d5735307 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config {
  *
  * @set_frame_desc: set the low level media bus frame parameters, @fd array
  *                  may be adjusted by the subdev driver to device capabilities.
+ *
+ * @get_mbus_config: get the media bus configuration of a remote sub-device.
+ *		     The media bus configuration is usually retrieved from the
+ *		     firmware interface at sub-device probe time, immediately
+ *		     applied to the hardware and eventually adjusted by the
+ *		     driver. Remote sub-devices (usually video receivers) shall
+ *		     use this operation to query the transmitting end bus
+ *		     configuration in order to adjust their own one accordingly.
+ *		     Callers should make sure they get the most up-to-date as
+ *		     possible configuration from the remote end, likely calling
+ *		     this operation as close as possible to stream on time. The
+ *		     operation shall fail if the pad index it has been called on
+ *		     is not valid.
+ *
+ * @set_mbus_config: set the media bus configuration of a remote sub-device.
+ *		     This operations is intended to allow, in combination with
+ *		     the get_mbus_config operation, the negotiation of media bus
+ *		     configuration parameters between media sub-devices. The
+ *		     operation shall not fail if the requested configuration is
+ *		     not supported, but the driver shall update the content of
+ *		     the %config argument to reflect what has been actually
+ *		     applied to the hardware. The operation shall fail if the
+ *		     pad index it has been called on is not valid.
  */
 struct v4l2_subdev_pad_ops {
 	int (*init_cfg)(struct v4l2_subdev *sd,
@@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops {
 			      struct v4l2_mbus_frame_desc *fd);
 	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
 			      struct v4l2_mbus_frame_desc *fd);
+	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
+			       struct v4l2_mbus_config *config);
+	int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
+			       struct v4l2_mbus_config *config);
 };
 
 /**
-- 
2.27.0

[PATCH v7 02/10] media: i2c: Use the new get_mbus_config pad op

[Jacopo Mondi]

Move the existing users of the g_mbus_config video operation to use the
newly introduced get_mbus_config pad operations.

All the ported drivers report a static media bus configuration and do no
support s_mbus_config so the operation implementation has not changed.

Bridge drivers needs to call the new pad operation and will receive an
-ENOICTLCMD when calling the old g_mbus_config video operation

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv7180.c   | 7 ++++---
 drivers/media/i2c/ml86v7667.c | 7 ++++---
 drivers/media/i2c/mt9m001.c   | 7 ++++---
 drivers/media/i2c/mt9m111.c   | 7 ++++---
 drivers/media/i2c/ov9640.c    | 7 ++++---
 drivers/media/i2c/tc358743.c  | 7 ++++---
 drivers/media/i2c/tvp5150.c   | 7 ++++---
 7 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 00159daa6fcd..e8744efe3cf0 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -760,8 +760,9 @@ static int adv7180_init_cfg(struct v4l2_subdev *sd,
 	return adv7180_set_pad_format(sd, cfg, &fmt);
 }
 
-static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
-				 struct v4l2_mbus_config *cfg)
+static int adv7180_get_mbus_config(struct v4l2_subdev *sd,
+				   unsigned int pad,
+				   struct v4l2_mbus_config *cfg)
 {
 	struct adv7180_state *state = to_state(sd);
 
@@ -852,7 +853,6 @@ static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 	.querystd = adv7180_querystd,
 	.g_input_status = adv7180_g_input_status,
 	.s_routing = adv7180_s_routing,
-	.g_mbus_config = adv7180_g_mbus_config,
 	.g_pixelaspect = adv7180_g_pixelaspect,
 	.g_tvnorms = adv7180_g_tvnorms,
 	.s_stream = adv7180_s_stream,
@@ -869,6 +869,7 @@ static const struct v4l2_subdev_pad_ops adv7180_pad_ops = {
 	.enum_mbus_code = adv7180_enum_mbus_code,
 	.set_fmt = adv7180_set_pad_format,
 	.get_fmt = adv7180_get_pad_format,
+	.get_mbus_config = adv7180_get_mbus_config,
 };
 
 static const struct v4l2_subdev_sensor_ops adv7180_sensor_ops = {
diff --git a/drivers/media/i2c/ml86v7667.c b/drivers/media/i2c/ml86v7667.c
index c444bd6a0658..ff212335326a 100644
--- a/drivers/media/i2c/ml86v7667.c
+++ b/drivers/media/i2c/ml86v7667.c
@@ -219,8 +219,9 @@ static int ml86v7667_fill_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int ml86v7667_g_mbus_config(struct v4l2_subdev *sd,
-				   struct v4l2_mbus_config *cfg)
+static int ml86v7667_get_mbus_config(struct v4l2_subdev *sd,
+				     unsigned int pad,
+				     struct v4l2_mbus_config *cfg)
 {
 	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
 		     V4L2_MBUS_DATA_ACTIVE_HIGH;
@@ -291,13 +292,13 @@ static const struct v4l2_subdev_video_ops ml86v7667_subdev_video_ops = {
 	.s_std = ml86v7667_s_std,
 	.querystd = ml86v7667_querystd,
 	.g_input_status = ml86v7667_g_input_status,
-	.g_mbus_config = ml86v7667_g_mbus_config,
 };
 
 static const struct v4l2_subdev_pad_ops ml86v7667_subdev_pad_ops = {
 	.enum_mbus_code = ml86v7667_enum_mbus_code,
 	.get_fmt = ml86v7667_fill_fmt,
 	.set_fmt = ml86v7667_fill_fmt,
+	.get_mbus_config = ml86v7667_get_mbus_config,
 };
 
 static const struct v4l2_subdev_core_ops ml86v7667_subdev_core_ops = {
diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index 210ea76adb53..3b0ba8ed5233 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -689,8 +689,9 @@ static int mt9m001_enum_mbus_code(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int mt9m001_g_mbus_config(struct v4l2_subdev *sd,
-				struct v4l2_mbus_config *cfg)
+static int mt9m001_get_mbus_config(struct v4l2_subdev *sd,
+				   unsigned int pad,
+				   struct v4l2_mbus_config *cfg)
 {
 	/* MT9M001 has all capture_format parameters fixed */
 	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_FALLING |
@@ -703,7 +704,6 @@ static int mt9m001_g_mbus_config(struct v4l2_subdev *sd,
 
 static const struct v4l2_subdev_video_ops mt9m001_subdev_video_ops = {
 	.s_stream	= mt9m001_s_stream,
-	.g_mbus_config	= mt9m001_g_mbus_config,
 };
 
 static const struct v4l2_subdev_sensor_ops mt9m001_subdev_sensor_ops = {
@@ -717,6 +717,7 @@ static const struct v4l2_subdev_pad_ops mt9m001_subdev_pad_ops = {
 	.set_selection	= mt9m001_set_selection,
 	.get_fmt	= mt9m001_get_fmt,
 	.set_fmt	= mt9m001_set_fmt,
+	.get_mbus_config = mt9m001_get_mbus_config,
 };
 
 static const struct v4l2_subdev_ops mt9m001_subdev_ops = {
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 17e8253f5748..69697386ffcd 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -1137,8 +1137,9 @@ static int mt9m111_init_cfg(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
-				struct v4l2_mbus_config *cfg)
+static int mt9m111_get_mbus_config(struct v4l2_subdev *sd,
+				   unsigned int pad,
+				   struct v4l2_mbus_config *cfg)
 {
 	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
 
@@ -1155,7 +1156,6 @@ static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
 }
 
 static const struct v4l2_subdev_video_ops mt9m111_subdev_video_ops = {
-	.g_mbus_config	= mt9m111_g_mbus_config,
 	.s_stream	= mt9m111_s_stream,
 	.g_frame_interval = mt9m111_g_frame_interval,
 	.s_frame_interval = mt9m111_s_frame_interval,
@@ -1168,6 +1168,7 @@ static const struct v4l2_subdev_pad_ops mt9m111_subdev_pad_ops = {
 	.set_selection	= mt9m111_set_selection,
 	.get_fmt	= mt9m111_get_fmt,
 	.set_fmt	= mt9m111_set_fmt,
+	.get_mbus_config = mt9m111_get_mbus_config,
 };
 
 static const struct v4l2_subdev_ops mt9m111_subdev_ops = {
diff --git a/drivers/media/i2c/ov9640.c b/drivers/media/i2c/ov9640.c
index 3a21f51d9325..fbd5d7b75811 100644
--- a/drivers/media/i2c/ov9640.c
+++ b/drivers/media/i2c/ov9640.c
@@ -648,8 +648,9 @@ static const struct v4l2_subdev_core_ops ov9640_core_ops = {
 };
 
 /* Request bus settings on camera side */
-static int ov9640_g_mbus_config(struct v4l2_subdev *sd,
-				struct v4l2_mbus_config *cfg)
+static int ov9640_get_mbus_config(struct v4l2_subdev *sd,
+				  unsigned int pad,
+				  struct v4l2_mbus_config *cfg)
 {
 	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
 		V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_HIGH |
@@ -661,13 +662,13 @@ static int ov9640_g_mbus_config(struct v4l2_subdev *sd,
 
 static const struct v4l2_subdev_video_ops ov9640_video_ops = {
 	.s_stream	= ov9640_s_stream,
-	.g_mbus_config	= ov9640_g_mbus_config,
 };
 
 static const struct v4l2_subdev_pad_ops ov9640_pad_ops = {
 	.enum_mbus_code = ov9640_enum_mbus_code,
 	.get_selection	= ov9640_get_selection,
 	.set_fmt	= ov9640_set_fmt,
+	.get_mbus_config = ov9640_get_mbus_config,
 };
 
 static const struct v4l2_subdev_ops ov9640_subdev_ops = {
diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index dbbab75f135e..a03dcab5ce61 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1602,8 +1602,9 @@ static int tc358743_dv_timings_cap(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int tc358743_g_mbus_config(struct v4l2_subdev *sd,
-			     struct v4l2_mbus_config *cfg)
+static int tc358743_get_mbus_config(struct v4l2_subdev *sd,
+				    unsigned int pad,
+				    struct v4l2_mbus_config *cfg)
 {
 	struct tc358743_state *state = to_state(sd);
 
@@ -1836,7 +1837,6 @@ static const struct v4l2_subdev_video_ops tc358743_video_ops = {
 	.s_dv_timings = tc358743_s_dv_timings,
 	.g_dv_timings = tc358743_g_dv_timings,
 	.query_dv_timings = tc358743_query_dv_timings,
-	.g_mbus_config = tc358743_g_mbus_config,
 	.s_stream = tc358743_s_stream,
 };
 
@@ -1848,6 +1848,7 @@ static const struct v4l2_subdev_pad_ops tc358743_pad_ops = {
 	.set_edid = tc358743_s_edid,
 	.enum_dv_timings = tc358743_enum_dv_timings,
 	.dv_timings_cap = tc358743_dv_timings_cap,
+	.get_mbus_config = tc358743_get_mbus_config,
 };
 
 static const struct v4l2_subdev_ops tc358743_ops = {
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 9df575238952..1c2050944b92 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1191,8 +1191,9 @@ static int tvp5150_get_selection(struct v4l2_subdev *sd,
 	}
 }
 
-static int tvp5150_g_mbus_config(struct v4l2_subdev *sd,
-				 struct v4l2_mbus_config *cfg)
+static int tvp5150_get_mbus_config(struct v4l2_subdev *sd,
+				   unsigned int pad,
+				   struct v4l2_mbus_config *cfg)
 {
 	struct tvp5150 *decoder = to_tvp5150(sd);
 
@@ -1721,7 +1722,6 @@ static const struct v4l2_subdev_video_ops tvp5150_video_ops = {
 	.querystd = tvp5150_querystd,
 	.s_stream = tvp5150_s_stream,
 	.s_routing = tvp5150_s_routing,
-	.g_mbus_config = tvp5150_g_mbus_config,
 };
 
 static const struct v4l2_subdev_vbi_ops tvp5150_vbi_ops = {
@@ -1739,6 +1739,7 @@ static const struct v4l2_subdev_pad_ops tvp5150_pad_ops = {
 	.get_fmt = tvp5150_fill_fmt,
 	.get_selection = tvp5150_get_selection,
 	.set_selection = tvp5150_set_selection,
+	.get_mbus_config = tvp5150_get_mbus_config,
 };
 
 static const struct v4l2_subdev_ops tvp5150_ops = {
-- 
2.27.0

[PATCH v7 03/10] media: i2c: ov6650: Use new [get|set]_mbus_config ops

[Jacopo Mondi]

Use the new get_mbus_config and set_mbus_config pad operations in place
of the video operations currently in use.

Compared to other drivers where the same conversion has been performed,
ov6650 proved to be a bit more tricky, as the existing g_mbus_config
implementation did not report the currently applied configuration but
the set of all possible configuration options.

Adapt the driver to support the semantic of the two newly introduced
operations:
- get_mbus_config reports the current media bus configuration
- set_mbus_config applies only changes explicitly requested and updates
  the provided cfg parameter to report what has actually been applied to
  the hardware.

Compile-tested only.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/ov6650.c | 56 ++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 91906b94f978..b349a9d5986a 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -921,55 +921,73 @@ static const struct v4l2_subdev_core_ops ov6650_core_ops = {
 };
 
 /* Request bus settings on camera side */
-static int ov6650_g_mbus_config(struct v4l2_subdev *sd,
-				struct v4l2_mbus_config *cfg)
+static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
+				  unsigned int pad,
+				  struct v4l2_mbus_config *cfg)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	u8 comj, comf;
+	int ret;
+
+	ret = ov6650_reg_read(client, REG_COMJ, &comj);
+	if (ret)
+		return ret;
 
-	cfg->flags = V4L2_MBUS_MASTER |
-		V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_PCLK_SAMPLE_FALLING |
-		V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_HSYNC_ACTIVE_LOW |
-		V4L2_MBUS_VSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_LOW |
-		V4L2_MBUS_DATA_ACTIVE_HIGH;
+	ret = ov6650_reg_read(client, REG_COMF, &comf);
+	if (ret)
+		return ret;
+
+	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_DATA_ACTIVE_HIGH
+		   | ((comj & COMJ_VSYNC_HIGH)  ? V4L2_MBUS_VSYNC_ACTIVE_HIGH
+						: V4L2_MBUS_VSYNC_ACTIVE_LOW)
+		   | ((comf & COMF_HREF_LOW)    ? V4L2_MBUS_HSYNC_ACTIVE_LOW
+						: V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+		   | ((comj & COMJ_PCLK_RISING) ? V4L2_MBUS_PCLK_SAMPLE_RISING
+						: V4L2_MBUS_PCLK_SAMPLE_FALLING);
 	cfg->type = V4L2_MBUS_PARALLEL;
 
 	return 0;
 }
 
 /* Alter bus settings on camera side */
-static int ov6650_s_mbus_config(struct v4l2_subdev *sd,
-				const struct v4l2_mbus_config *cfg)
+static int ov6650_set_mbus_config(struct v4l2_subdev *sd,
+				  unsigned int pad,
+				  struct v4l2_mbus_config *cfg)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	int ret;
+	int ret = 0;
 
 	if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
 		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
-	else
+	else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
 		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
 	if (ret)
-		return ret;
+		goto error;
 
 	if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
 		ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0);
-	else
+	else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
 		ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW);
 	if (ret)
-		return ret;
+		goto error;
 
 	if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
 		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0);
-	else
+	else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
 		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH);
 
-	return ret;
+error:
+	/*
+	 * Update the configuration to report what is actually applied to
+	 * the hardware.
+	 */
+	return ov6650_get_mbus_config(sd, pad, cfg);
 }
 
 static const struct v4l2_subdev_video_ops ov6650_video_ops = {
 	.s_stream	= ov6650_s_stream,
 	.g_frame_interval = ov6650_g_frame_interval,
 	.s_frame_interval = ov6650_s_frame_interval,
-	.g_mbus_config	= ov6650_g_mbus_config,
-	.s_mbus_config	= ov6650_s_mbus_config,
 };
 
 static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
@@ -978,6 +996,8 @@ static const struct v4l2_subdev_pad_ops ov6650_pad_ops = {
 	.set_selection	= ov6650_set_selection,
 	.get_fmt	= ov6650_get_fmt,
 	.set_fmt	= ov6650_set_fmt,
+	.get_mbus_config = ov6650_get_mbus_config,
+	.set_mbus_config = ov6650_set_mbus_config,
 };
 
 static const struct v4l2_subdev_ops ov6650_subdev_ops = {
-- 
2.27.0

[PATCH v7 04/10] media: pxa_camera: Use the new set_mbus_config op

[Jacopo Mondi]

Move the PXA camera driver to use the new set_mbus_config pad operation.
For this platform the change is not only cosmetic, as the pxa driver is
currently the only driver in mainline to make use of the g_mbus_config
and s_mbus_config video operations.

The existing driver semantic is the following:
- Collect all supported mbus config flags from the remote end
- Match them with the supported PXA mbus configuration flags
- If the remote subdevice allows multiple options for for VSYNC, HSYNC
  and PCLK polarity, use platform data requested settings

The semantic of the new get_mbus_config and set_mbus_config differs from
the corresponding video ops, particularly in the fact get_mbus_config
reports the current mbus configuration and not the set of supported
configuration options, with set_mbus_config always reporting the actual
mbus configuration applied to the remote subdevice.

Adapt the driver to perform the following
- Set the remote subdevice mbus configuration according to the PXA
  platform data preferences.
- If the applied configuration differs from the requested one (i.e. the
  remote subdevice does not allow changing one setting) make sure that
  - The remote end does not claim for DATA_ACTIVE_LOW, which seems not
    supported by the platform
  - The bus mastering roles match

While at there remove a few checks performed on the media bus
configuration at get_format() time as they do not belong there.

Compile-tested only.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/pxa_camera.c | 189 ++++++++--------------------
 1 file changed, 51 insertions(+), 138 deletions(-)

diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index 6dce33f35041..0366c4a813ce 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -605,42 +605,6 @@ static const struct pxa_mbus_pixelfmt *pxa_mbus_get_fmtdesc(
 	return pxa_mbus_find_fmtdesc(code, mbus_fmt, ARRAY_SIZE(mbus_fmt));
 }
 
-static unsigned int pxa_mbus_config_compatible(const struct v4l2_mbus_config *cfg,
-					unsigned int flags)
-{
-	unsigned long common_flags;
-	bool hsync = true, vsync = true, pclk, data, mode;
-	bool mipi_lanes, mipi_clock;
-
-	common_flags = cfg->flags & flags;
-
-	switch (cfg->type) {
-	case V4L2_MBUS_PARALLEL:
-		hsync = common_flags & (V4L2_MBUS_HSYNC_ACTIVE_HIGH |
-					V4L2_MBUS_HSYNC_ACTIVE_LOW);
-		vsync = common_flags & (V4L2_MBUS_VSYNC_ACTIVE_HIGH |
-					V4L2_MBUS_VSYNC_ACTIVE_LOW);
-		/* fall through */
-	case V4L2_MBUS_BT656:
-		pclk = common_flags & (V4L2_MBUS_PCLK_SAMPLE_RISING |
-				       V4L2_MBUS_PCLK_SAMPLE_FALLING);
-		data = common_flags & (V4L2_MBUS_DATA_ACTIVE_HIGH |
-				       V4L2_MBUS_DATA_ACTIVE_LOW);
-		mode = common_flags & (V4L2_MBUS_MASTER | V4L2_MBUS_SLAVE);
-		return (!hsync || !vsync || !pclk || !data || !mode) ?
-			0 : common_flags;
-	case V4L2_MBUS_CSI2_DPHY:
-		mipi_lanes = common_flags & V4L2_MBUS_CSI2_LANES;
-		mipi_clock = common_flags & (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK |
-					     V4L2_MBUS_CSI2_CONTINUOUS_CLOCK);
-		return (!mipi_lanes || !mipi_clock) ? 0 : common_flags;
-	default:
-		WARN_ON(1);
-		return -EINVAL;
-	}
-	return 0;
-}
-
 /**
  * struct pxa_camera_format_xlate - match between host and sensor formats
  * @code: code of a sensor provided format
@@ -1231,31 +1195,6 @@ static irqreturn_t pxa_camera_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static int test_platform_param(struct pxa_camera_dev *pcdev,
-			       unsigned char buswidth, unsigned long *flags)
-{
-	/*
-	 * Platform specified synchronization and pixel clock polarities are
-	 * only a recommendation and are only used during probing. The PXA270
-	 * quick capture interface supports both.
-	 */
-	*flags = (pcdev->platform_flags & PXA_CAMERA_MASTER ?
-		  V4L2_MBUS_MASTER : V4L2_MBUS_SLAVE) |
-		V4L2_MBUS_HSYNC_ACTIVE_HIGH |
-		V4L2_MBUS_HSYNC_ACTIVE_LOW |
-		V4L2_MBUS_VSYNC_ACTIVE_HIGH |
-		V4L2_MBUS_VSYNC_ACTIVE_LOW |
-		V4L2_MBUS_DATA_ACTIVE_HIGH |
-		V4L2_MBUS_PCLK_SAMPLE_RISING |
-		V4L2_MBUS_PCLK_SAMPLE_FALLING;
-
-	/* If requested data width is supported by the platform, use it */
-	if ((1 << (buswidth - 1)) & pcdev->width_flags)
-		return 0;
-
-	return -EINVAL;
-}
-
 static void pxa_camera_setup_cicr(struct pxa_camera_dev *pcdev,
 				  unsigned long flags, __u32 pixfmt)
 {
@@ -1598,99 +1537,78 @@ static int pxa_camera_init_videobuf2(struct pxa_camera_dev *pcdev)
  */
 static int pxa_camera_set_bus_param(struct pxa_camera_dev *pcdev)
 {
+	unsigned int bus_width = pcdev->current_fmt->host_fmt->bits_per_sample;
 	struct v4l2_mbus_config cfg = {.type = V4L2_MBUS_PARALLEL,};
 	u32 pixfmt = pcdev->current_fmt->host_fmt->fourcc;
-	unsigned long bus_flags, common_flags;
+	int mbus_config;
 	int ret;
 
-	ret = test_platform_param(pcdev,
-				  pcdev->current_fmt->host_fmt->bits_per_sample,
-				  &bus_flags);
-	if (ret < 0)
-		return ret;
-
-	ret = sensor_call(pcdev, video, g_mbus_config, &cfg);
-	if (!ret) {
-		common_flags = pxa_mbus_config_compatible(&cfg,
-							  bus_flags);
-		if (!common_flags) {
-			dev_warn(pcdev_to_dev(pcdev),
-				 "Flags incompatible: camera 0x%x, host 0x%lx\n",
-				 cfg.flags, bus_flags);
-			return -EINVAL;
-		}
-	} else if (ret != -ENOIOCTLCMD) {
-		return ret;
-	} else {
-		common_flags = bus_flags;
+	if (!((1 << (bus_width - 1)) & pcdev->width_flags)) {
+		dev_err(pcdev_to_dev(pcdev), "Unsupported bus width %u",
+			bus_width);
+		return -EINVAL;
 	}
 
 	pcdev->channels = 1;
 
 	/* Make choices, based on platform preferences */
-	if ((common_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) &&
-	    (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)) {
-		if (pcdev->platform_flags & PXA_CAMERA_HSP)
-			common_flags &= ~V4L2_MBUS_HSYNC_ACTIVE_HIGH;
-		else
-			common_flags &= ~V4L2_MBUS_HSYNC_ACTIVE_LOW;
-	}
+	mbus_config = 0;
+	if (pcdev->platform_flags & PXA_CAMERA_MASTER)
+		mbus_config |= V4L2_MBUS_MASTER;
+	else
+		mbus_config |= V4L2_MBUS_SLAVE;
 
-	if ((common_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) &&
-	    (common_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)) {
-		if (pcdev->platform_flags & PXA_CAMERA_VSP)
-			common_flags &= ~V4L2_MBUS_VSYNC_ACTIVE_HIGH;
-		else
-			common_flags &= ~V4L2_MBUS_VSYNC_ACTIVE_LOW;
-	}
+	if (pcdev->platform_flags & PXA_CAMERA_HSP)
+		mbus_config |= V4L2_MBUS_HSYNC_ACTIVE_HIGH;
+	else
+		mbus_config |= V4L2_MBUS_HSYNC_ACTIVE_LOW;
 
-	if ((common_flags & V4L2_MBUS_PCLK_SAMPLE_RISING) &&
-	    (common_flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)) {
-		if (pcdev->platform_flags & PXA_CAMERA_PCP)
-			common_flags &= ~V4L2_MBUS_PCLK_SAMPLE_RISING;
-		else
-			common_flags &= ~V4L2_MBUS_PCLK_SAMPLE_FALLING;
-	}
+	if (pcdev->platform_flags & PXA_CAMERA_VSP)
+		mbus_config |= V4L2_MBUS_VSYNC_ACTIVE_HIGH;
+	else
+		mbus_config |= V4L2_MBUS_VSYNC_ACTIVE_LOW;
 
-	cfg.flags = common_flags;
-	ret = sensor_call(pcdev, video, s_mbus_config, &cfg);
+	if (pcdev->platform_flags & PXA_CAMERA_PCP)
+		mbus_config |= V4L2_MBUS_PCLK_SAMPLE_RISING;
+	else
+		mbus_config |= V4L2_MBUS_PCLK_SAMPLE_FALLING;
+	mbus_config |= V4L2_MBUS_DATA_ACTIVE_HIGH;
+
+	cfg.flags = mbus_config;
+	ret = sensor_call(pcdev, pad, set_mbus_config, 0, &cfg);
 	if (ret < 0 && ret != -ENOIOCTLCMD) {
-		dev_dbg(pcdev_to_dev(pcdev),
-			"camera s_mbus_config(0x%lx) returned %d\n",
-			common_flags, ret);
+		dev_err(pcdev_to_dev(pcdev),
+			"Failed to call set_mbus_config: %d\n", ret);
 		return ret;
 	}
 
-	pxa_camera_setup_cicr(pcdev, common_flags, pixfmt);
-
-	return 0;
-}
-
-static int pxa_camera_try_bus_param(struct pxa_camera_dev *pcdev,
-				    unsigned char buswidth)
-{
-	struct v4l2_mbus_config cfg = {.type = V4L2_MBUS_PARALLEL,};
-	unsigned long bus_flags, common_flags;
-	int ret = test_platform_param(pcdev, buswidth, &bus_flags);
-
-	if (ret < 0)
-		return ret;
+	/*
+	 * If the requested media bus configuration has not been fully applied
+	 * make sure it is supported by the platform.
+	 *
+	 * PXA does not support V4L2_MBUS_DATA_ACTIVE_LOW and the bus mastering
+	 * roles should match.
+	 */
+	if (cfg.flags != mbus_config) {
+		unsigned int pxa_mbus_role = mbus_config & (V4L2_MBUS_MASTER |
+							    V4L2_MBUS_SLAVE);
+		if (pxa_mbus_role != (cfg.flags & (V4L2_MBUS_MASTER |
+						   V4L2_MBUS_SLAVE))) {
+			dev_err(pcdev_to_dev(pcdev),
+				"Unsupported mbus configuration: bus mastering\n");
+			return -EINVAL;
+		}
 
-	ret = sensor_call(pcdev, video, g_mbus_config, &cfg);
-	if (!ret) {
-		common_flags = pxa_mbus_config_compatible(&cfg,
-							  bus_flags);
-		if (!common_flags) {
-			dev_warn(pcdev_to_dev(pcdev),
-				 "Flags incompatible: camera 0x%x, host 0x%lx\n",
-				 cfg.flags, bus_flags);
+		if (cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW) {
+			dev_err(pcdev_to_dev(pcdev),
+				"Unsupported mbus configuration: DATA_ACTIVE_LOW\n");
 			return -EINVAL;
 		}
-	} else if (ret == -ENOIOCTLCMD) {
-		ret = 0;
 	}
 
-	return ret;
+	pxa_camera_setup_cicr(pcdev, cfg.flags, pixfmt);
+
+	return 0;
 }
 
 static const struct pxa_mbus_pixelfmt pxa_camera_formats[] = {
@@ -1738,11 +1656,6 @@ static int pxa_camera_get_formats(struct v4l2_device *v4l2_dev,
 		return 0;
 	}
 
-	/* This also checks support for the requested bits-per-sample */
-	ret = pxa_camera_try_bus_param(pcdev, fmt->bits_per_sample);
-	if (ret < 0)
-		return 0;
-
 	switch (code.code) {
 	case MEDIA_BUS_FMT_UYVY8_2X8:
 		formats++;
-- 
2.27.0

[PATCH v7 05/10] media: v4l2-subdev: Remove [s|g]_mbus_config video ops

[Jacopo Mondi]

With all sensor and platform drivers now converted to use the new
get_mbus_config and set_mbus_config pad operations, remove the
deprecated video operations g_mbus_config and s_mbus_config.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-subdev.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d8b9d5735307..98975248a033 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -402,12 +402,6 @@ struct v4l2_mbus_frame_desc {
  *
  * @query_dv_timings: callback for VIDIOC_QUERY_DV_TIMINGS() ioctl handler code.
  *
- * @g_mbus_config: get supported mediabus configurations
- *
- * @s_mbus_config: set a certain mediabus configuration. This operation is added
- *	for compatibility with soc-camera drivers and should not be used by new
- *	software.
- *
  * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev
  *	can adjust @size to a lower value and must not write more data to the
  *	buffer starting at @data than the original value of @size.
@@ -435,10 +429,6 @@ struct v4l2_subdev_video_ops {
 			struct v4l2_dv_timings *timings);
 	int (*query_dv_timings)(struct v4l2_subdev *sd,
 			struct v4l2_dv_timings *timings);
-	int (*g_mbus_config)(struct v4l2_subdev *sd,
-			     struct v4l2_mbus_config *cfg);
-	int (*s_mbus_config)(struct v4l2_subdev *sd,
-			     const struct v4l2_mbus_config *cfg);
 	int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf,
 			   unsigned int *size);
 };
-- 
2.27.0

[PATCH v7 06/10] media: v4l2- mediabus: Add usage note for V4L2_MBUS_*

[Jacopo Mondi]

With the removal of the legacy g_mbus_config and s_mbus_config video
operations, the sole users of V4L2_MBUS_* flags are now the newly
introduced get_mbus_config and set_mbus_config pad operations.

As the semantic of the new operations differs from the semantic of
the legacy ones, add a usage note in the v4l2-mediabus.h header to
specify how to use the flags.

Also add a TODO note to record that we intend to replace the existing
flags with fields, to prevent users from mixing conflicting values
in a single operation call.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/media/v4l2-mediabus.h | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 45f88f0248c4..14818901e713 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -11,9 +11,34 @@
 #include <linux/v4l2-mediabus.h>
 #include <linux/bitops.h>

+/*
+ * How to use the V4L2_MBUS_* flags:
+ * Flags are defined for each of the possible states and values of a media
+ * bus configuration parameter. One and only one bit of each group of flags
+ * shall be set by the users of the v4l2_subdev_pad_ops.get_mbus_config and
+ * v4l2_subdev_pad_ops.set_mbus_config operations to ensure that no
+ * conflicting settings are specified when reporting and setting the media bus
+ * configuration with the two operations respectively. For example, it is
+ * invalid to set or clear both the V4L2_MBUS_HSYNC_ACTIVE_HIGH and the
+ * V4L2_MBUS_HSYNC_ACTIVE_LOW flag at the same time. Instead either flag
+ * V4L2_MBUS_HSYNC_ACTIVE_HIGH or flag V4L2_MBUS_HSYNC_ACTIVE_LOW shall be
+ * set. The same is true for the V4L2_MBUS_CSI2_1/2/3/4_LANE flags group: only
+ * one of these four bits shall be set.
+ *
+ * TODO: replace the existing V4L2_MBUS_* flags with structures of fields
+ * to avoid conflicting settings.
+ *
+ * In example:
+ *     #define V4L2_MBUS_HSYNC_ACTIVE_HIGH             BIT(2)
+ *     #define V4L2_MBUS_HSYNC_ACTIVE_LOW              BIT(3)
+ * will be replaced by a field whose value reports the intended active state of
+ * the signal:
+ *     unsigned int v4l2_mbus_hsync_active : 1;
+ */
+
 /* Parallel flags */
 /*
- * Can the client run in master or in slave mode. By "Master mode" an operation
+ * The client runs in master or in slave mode. By "Master mode" an operation
  * mode is meant, when the client (e.g., a camera sensor) is producing
  * horizontal and vertical synchronisation. In "Slave mode" the host is
  * providing these signals to the slave.
@@ -45,17 +70,17 @@
 #define V4L2_MBUS_DATA_ENABLE_LOW		BIT(15)

 /* Serial flags */
-/* How many lanes the client can use */
+/* CSI-2 D-PHY number of data lanes. */
 #define V4L2_MBUS_CSI2_1_LANE			BIT(0)
 #define V4L2_MBUS_CSI2_2_LANE			BIT(1)
 #define V4L2_MBUS_CSI2_3_LANE			BIT(2)
 #define V4L2_MBUS_CSI2_4_LANE			BIT(3)
-/* On which channels it can send video data */
+/* CSI-2 Virtual Channel identifiers. */
 #define V4L2_MBUS_CSI2_CHANNEL_0		BIT(4)
 #define V4L2_MBUS_CSI2_CHANNEL_1		BIT(5)
 #define V4L2_MBUS_CSI2_CHANNEL_2		BIT(6)
 #define V4L2_MBUS_CSI2_CHANNEL_3		BIT(7)
-/* Does it support only continuous or also non-continuous clock mode */
+/* Clock non-continuous mode support. */
 #define V4L2_MBUS_CSI2_CONTINUOUS_CLOCK		BIT(8)
 #define V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK	BIT(9)

--
2.27.0

[PATCH v7 07/10] staging: media: imx: Update TODO entry

[Jacopo Mondi]

Update the TODO entry that mentioned a potential use case for the now
removed g_mbus_config video operation.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/staging/media/imx/TODO | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
index a371cdedcdb0..9cfc1c1e78dc 100644
--- a/drivers/staging/media/imx/TODO
+++ b/drivers/staging/media/imx/TODO
@@ -10,6 +10,10 @@
   driver uses the parsed DT bus config method until this issue is
   resolved.
 
+  2020-06: g_mbus has been removed in favour of the get_mbus_config pad
+  operation which should be used to avoid parsing the remote endpoint
+  configuration.
+
 - This media driver supports inheriting V4L2 controls to the
   video capture devices, from the subdevices in the capture device's
   pipeline. The controls for each capture device are updated in the
-- 
2.27.0

[PATCH v7 08/10] media: i2c: adv748x: Adjust TXA data lanes number

[Jacopo Mondi]

When outputting SD-Core output through the TXA MIPI CSI-2 interface,
the number of enabled data lanes should be reduced in order to guarantee
that the two video formats produced by the SD-Core (480i and 576i)
generate a MIPI CSI-2 link clock frequency compatible with the MIPI D-PHY
specifications.

Limit the number of enabled data lanes to 2, which is guaranteed to
support 480i and 576i formats.

Cache the number of enabled data lanes to be able to report it through
the new get_mbus_config operation.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 31 ++++++++++++++++++------
 drivers/media/i2c/adv748x/adv748x.h      |  1 +
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 23e02ff27b17..1fe7f97c6d52 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -241,10 +241,10 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
 	int ret = 0;
 
 	/* Enable n-lane MIPI */
-	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0x80 | tx->active_lanes, &ret);
 
 	/* Set Auto DPHY Timing */
-	adv748x_write_check(state, page, 0x00, 0xa0 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0xa0 | tx->active_lanes, &ret);
 
 	/* ADI Required Write */
 	if (tx->src == &state->hdmi.sd) {
@@ -270,7 +270,7 @@ static int adv748x_power_up_tx(struct adv748x_csi2 *tx)
 	usleep_range(2000, 2500);
 
 	/* Power-up CSI-TX */
-	adv748x_write_check(state, page, 0x00, 0x20 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0x20 | tx->active_lanes, &ret);
 	usleep_range(1000, 1500);
 
 	/* ADI Required Writes */
@@ -292,7 +292,7 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
 	adv748x_write_check(state, page, 0x1e, 0x00, &ret);
 
 	/* Enable n-lane MIPI */
-	adv748x_write_check(state, page, 0x00, 0x80 | tx->num_lanes, &ret);
+	adv748x_write_check(state, page, 0x00, 0x80 | tx->active_lanes, &ret);
 
 	/* i2c_mipi_pll_en - 1'b1 */
 	adv748x_write_check(state, page, 0xda, 0x01, &ret);
@@ -357,14 +357,29 @@ static int adv748x_link_setup(struct media_entity *entity,
 	if (state->afe.tx) {
 		/* AFE Requires TXA enabled, even when output to TXB */
 		io10 |= ADV748X_IO_10_CSI4_EN;
-		if (is_txa(tx))
+		if (is_txa(tx)) {
+			/*
+			 * Output from the SD-core (480i and 576i) from the TXA
+			 * interface requires reducing the number of enabled
+			 * data lanes in order to guarantee a valid link
+			 * frequency.
+			 */
+			tx->active_lanes = min(tx->num_lanes, 2U);
 			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
-		else
+		} else {
+			/* TXB has a single data lane, no need to adjust. */
 			io10 |= ADV748X_IO_10_CSI1_EN;
+		}
 	}
 
-	if (state->hdmi.tx)
+	if (state->hdmi.tx) {
+		/*
+		 * Restore the number of active lanes, in case we have gone
+		 * through an AFE->TXA streaming sessions.
+		 */
+		tx->active_lanes = tx->num_lanes;
 		io10 |= ADV748X_IO_10_CSI4_EN;
+	}
 
 	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
 }
@@ -596,6 +611,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
 		}
 
 		state->txa.num_lanes = num_lanes;
+		state->txa.active_lanes = num_lanes;
 		adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
 	}
 
@@ -607,6 +623,7 @@ static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
 		}
 
 		state->txb.num_lanes = num_lanes;
+		state->txb.active_lanes = num_lanes;
 		adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
 	}
 
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index fccb388ce179..1061f425ece5 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -79,6 +79,7 @@ struct adv748x_csi2 {
 	unsigned int page;
 	unsigned int port;
 	unsigned int num_lanes;
+	unsigned int active_lanes;
 
 	struct media_pad pads[ADV748X_CSI2_NR_PADS];
 	struct v4l2_ctrl_handler ctrl_hdl;
-- 
2.27.0

[PATCH v7 09/10] media: i2c: adv748x: Implement get_mbus_config

[Jacopo Mondi]

Implement the newly introduced get_mbus_config operation to report the
number of currently used data lanes on the MIPI CSI-2 interface.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 31 ++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 2091cda50935..99bb63d05eef 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -214,9 +214,40 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
+					struct v4l2_mbus_config *config)
+{
+	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
+
+	if (pad != ADV748X_CSI2_SOURCE)
+		return -EINVAL;
+
+	config->type = V4L2_MBUS_CSI2_DPHY;
+	switch (tx->active_lanes) {
+	case 1:
+		config->flags = V4L2_MBUS_CSI2_1_LANE;
+		break;
+
+	case 2:
+		config->flags = V4L2_MBUS_CSI2_2_LANE;
+		break;
+
+	case 3:
+		config->flags = V4L2_MBUS_CSI2_3_LANE;
+		break;
+
+	case 4:
+		config->flags = V4L2_MBUS_CSI2_4_LANE;
+		break;
+	}
+
+	return 0;
+}
+
 static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
 	.get_fmt = adv748x_csi2_get_format,
 	.set_fmt = adv748x_csi2_set_format,
+	.get_mbus_config = adv748x_csi2_get_mbus_config,
 };
 
 /* -----------------------------------------------------------------------------
-- 
2.27.0

[PATCH v7 10/10] media: rcar-csi2: Negotiate data lanes number

[Jacopo Mondi]

Use the newly introduced get_mbus_config() subdevice pad operation to
retrieve the remote subdevice MIPI CSI-2 bus configuration and configure
the number of active data lanes accordingly.

In order to be able to call the remote subdevice operation cache the
index of the remote pad connected to the single CSI-2 input port.

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 75 +++++++++++++++++++--
 1 file changed, 68 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index c6cc4f473a07..cbb3e8a2887d 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -364,6 +364,7 @@ struct rcar_csi2 {
 	struct v4l2_async_notifier notifier;
 	struct v4l2_async_subdev asd;
 	struct v4l2_subdev *remote;
+	unsigned int remote_pad;
 
 	struct v4l2_mbus_framefmt mf;
 
@@ -409,13 +410,14 @@ static void rcsi2_exit_standby(struct rcar_csi2 *priv)
 	reset_control_deassert(priv->rstc);
 }
 
-static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
+static int rcsi2_wait_phy_start(struct rcar_csi2 *priv,
+				unsigned int lanes)
 {
 	unsigned int timeout;
 
 	/* Wait for the clock and data lanes to enter LP-11 state. */
 	for (timeout = 0; timeout <= 20; timeout++) {
-		const u32 lane_mask = (1 << priv->lanes) - 1;
+		const u32 lane_mask = (1 << lanes) - 1;
 
 		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
 		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
@@ -447,7 +449,8 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 	return 0;
 }
 
-static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
+static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
+			   unsigned int lanes)
 {
 	struct v4l2_subdev *source;
 	struct v4l2_ctrl *ctrl;
@@ -472,15 +475,64 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
 	 * bps = link_freq * 2
 	 */
 	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
-	do_div(mbps, priv->lanes * 1000000);
+	do_div(mbps, lanes * 1000000);
 
 	return mbps;
 }
 
+static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
+				  unsigned int *lanes)
+{
+	struct v4l2_mbus_config mbus_config = { 0 };
+	unsigned int num_lanes = UINT_MAX;
+	int ret;
+
+	*lanes = priv->lanes;
+
+	ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config,
+			       priv->remote_pad, &mbus_config);
+	if (ret == -ENOIOCTLCMD) {
+		dev_dbg(priv->dev, "No remote mbus configuration available\n");
+		return 0;
+	}
+
+	if (ret) {
+		dev_err(priv->dev, "Failed to get remote mbus configuration\n");
+		return ret;
+	}
+
+	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
+		dev_err(priv->dev, "Unsupported media bus type %u\n",
+			mbus_config.type);
+		return -EINVAL;
+	}
+
+	if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE)
+		num_lanes = 1;
+	else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE)
+		num_lanes = 2;
+	else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE)
+		num_lanes = 3;
+	else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE)
+		num_lanes = 4;
+
+	if (num_lanes > priv->lanes) {
+		dev_err(priv->dev,
+			"Unsupported mbus config: too many data lanes %u\n",
+			num_lanes);
+		return -EINVAL;
+	}
+
+	*lanes = num_lanes;
+
+	return 0;
+}
+
 static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 {
 	const struct rcar_csi2_format *format;
 	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
+	unsigned int lanes;
 	unsigned int i;
 	int mbps, ret;
 
@@ -522,10 +574,18 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 			fld |= FLD_FLD_NUM(1);
 	}
 
+	/*
+	 * Get the number of active data lanes inspecting the remote mbus
+	 * configuration.
+	 */
+	ret = rcsi2_get_active_lanes(priv, &lanes);
+	if (ret)
+		return ret;
+
 	phycnt = PHYCNT_ENABLECLK;
-	phycnt |= (1 << priv->lanes) - 1;
+	phycnt |= (1 << lanes) - 1;
 
-	mbps = rcsi2_calc_mbps(priv, format->bpp);
+	mbps = rcsi2_calc_mbps(priv, format->bpp, lanes);
 	if (mbps < 0)
 		return mbps;
 
@@ -572,7 +632,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ);
 	rcsi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | PHYCNT_RSTZ);
 
-	ret = rcsi2_wait_phy_start(priv);
+	ret = rcsi2_wait_phy_start(priv, lanes);
 	if (ret)
 		return ret;
 
@@ -749,6 +809,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier,
 	}
 
 	priv->remote = subdev;
+	priv->remote_pad = pad;
 
 	dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad);
 
-- 
2.27.0

Re: [PATCH v7 01/10] media: v4l2-subdev: Introduce [get|set]_mbus_config pad ops

[Janusz Krzysztofik]

Hi Jacopo,

On Thursday, July 16, 2020 4:27:04 P.M. CEST Jacopo Mondi wrote:
> Introduce two new pad operations to allow retrieving and configuring the
> media bus parameters on a subdevice pad.
> 
> The newly introduced operations aims to replace the s/g_mbus_config video
> operations, which have been on their way for deprecation since a long
> time.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index f7fe78a6f65a..d8b9d5735307 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config {
>   *
>   * @set_frame_desc: set the low level media bus frame parameters, @fd array
>   *                  may be adjusted by the subdev driver to device capabilities.
> + *
> + * @get_mbus_config: get the media bus configuration of a remote sub-device.
> + *		     The media bus configuration is usually retrieved from the
> + *		     firmware interface at sub-device probe time, immediately
> + *		     applied to the hardware and eventually adjusted by the
> + *		     driver. Remote sub-devices (usually video receivers) shall
> + *		     use this operation to query the transmitting end bus
> + *		     configuration in order to adjust their own one accordingly.
> + *		     Callers should make sure they get the most up-to-date as
> + *		     possible configuration from the remote end, likely calling
> + *		     this operation as close as possible to stream on time. The
> + *		     operation shall fail if the pad index it has been called on
> + *		     is not valid.
> + *
> + * @set_mbus_config: set the media bus configuration of a remote sub-device.
> + *		     This operations is intended to allow, in combination with
> + *		     the get_mbus_config operation, the negotiation of media bus
> + *		     configuration parameters between media sub-devices. The
> + *		     operation shall not fail if the requested configuration is
> + *		     not supported, but the driver shall update the content of
> + *		     the %config argument to reflect what has been actually
> + *		     applied to the hardware. The operation shall fail if the
> + *		     pad index it has been called on is not valid.

Could this description also clarify what results are expected in case of 
hardware errors?  The ov6650 implementation you propose may suggest such
errors may be expected to be ignored silently as long as current configuration 
can be successfully obtained from hardware and passed back to the caller.

Moreover, since validity of the pad argument is expected to be verified, I 
think this should be handled by the media infrastructure layer with the 
drivers/media/v4l2-core/v4l2-subdev.c:check_pad() helper called from a 
.set_mbus_config() wrapper added to v4l2_subdev_call_pad_wrappers, freeing 
drivers from reimplementing it.

Thanks,
Janusz

>   */
>  struct v4l2_subdev_pad_ops {
>  	int (*init_cfg)(struct v4l2_subdev *sd,
> @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops {
>  			      struct v4l2_mbus_frame_desc *fd);
>  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
>  			      struct v4l2_mbus_frame_desc *fd);
> +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> +			       struct v4l2_mbus_config *config);
> +	int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> +			       struct v4l2_mbus_config *config);
>  };
>  
>  /**
> 

Re: [PATCH v7 01/10] media: v4l2-subdev: Introduce [get|set]_mbus_config pad ops

[Jacopo Mondi]

Hi Janusz,
  thanks for review

On Fri, Jul 17, 2020 at 12:15:27AM +0200, Janusz Krzysztofik wrote:
> Hi Jacopo,
>
> On Thursday, July 16, 2020 4:27:04 P.M. CEST Jacopo Mondi wrote:
> > Introduce two new pad operations to allow retrieving and configuring the
> > media bus parameters on a subdevice pad.
> >
> > The newly introduced operations aims to replace the s/g_mbus_config video
> > operations, which have been on their way for deprecation since a long
> > time.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index f7fe78a6f65a..d8b9d5735307 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config {
> >   *
> >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> >   *                  may be adjusted by the subdev driver to device capabilities.
> > + *
> > + * @get_mbus_config: get the media bus configuration of a remote sub-device.
> > + *		     The media bus configuration is usually retrieved from the
> > + *		     firmware interface at sub-device probe time, immediately
> > + *		     applied to the hardware and eventually adjusted by the
> > + *		     driver. Remote sub-devices (usually video receivers) shall
> > + *		     use this operation to query the transmitting end bus
> > + *		     configuration in order to adjust their own one accordingly.
> > + *		     Callers should make sure they get the most up-to-date as
> > + *		     possible configuration from the remote end, likely calling
> > + *		     this operation as close as possible to stream on time. The
> > + *		     operation shall fail if the pad index it has been called on
> > + *		     is not valid.
> > + *
> > + * @set_mbus_config: set the media bus configuration of a remote sub-device.
> > + *		     This operations is intended to allow, in combination with
> > + *		     the get_mbus_config operation, the negotiation of media bus
> > + *		     configuration parameters between media sub-devices. The
> > + *		     operation shall not fail if the requested configuration is
> > + *		     not supported, but the driver shall update the content of
> > + *		     the %config argument to reflect what has been actually
> > + *		     applied to the hardware. The operation shall fail if the
> > + *		     pad index it has been called on is not valid.

First of all, Hans sent a pull request for this series yesterday. Are
you ok with that and with sending patches on top, or do you think the
inclusion of the series should be post-poned ? (you can imagine what
my preference is :)

>
> Could this description also clarify what results are expected in case of
> hardware errors?  The ov6650 implementation you propose may suggest such
> errors may be expected to be ignored silently as long as current configuration
> can be successfully obtained from hardware and passed back to the caller.

I guess "it depends"(tm)
I know this doesn't sound like a good answer :)

A failure in applying or reading register likely means the driver is
trying to access the hardware while it is in low power state. In this
case I would consider this a driver bug, as it should try to be smart
and cache settings and apply them at the proper time and return the
current configuration, which should be cached as well.

Of course things could go wrong for several reasons, and in the case
if any unexpected error occurs I think an error is expected to be
reported. Please note that this v7 of ov6650 does that by returning
the return value of ov6650_get_mbus_config() at the end of
ov6650_set_mbus_config.

I guess this is pretty regular behaviour, although I missed it in the
previous version, so it might be worth adding an additional line to
the documentation.

>
> Moreover, since validity of the pad argument is expected to be verified, I
> think this should be handled by the media infrastructure layer with the
> drivers/media/v4l2-core/v4l2-subdev.c:check_pad() helper called from a
> .set_mbus_config() wrapper added to v4l2_subdev_call_pad_wrappers, freeing
> drivers from reimplementing it.
>

Good point, and indeed my bad as I thought v4l2_subdev_call() would
already do that by default, but looking at the actual implementation a
wrapper might be a good idea indeed.

Patches on top ?

Thanks
  j

> Thanks,
> Janusz
>
> >   */
> >  struct v4l2_subdev_pad_ops {
> >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops {
> >  			      struct v4l2_mbus_frame_desc *fd);
> >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> >  			      struct v4l2_mbus_frame_desc *fd);
> > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > +			       struct v4l2_mbus_config *config);
> > +	int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > +			       struct v4l2_mbus_config *config);
> >  };
> >
> >  /**
> >
>
>
>
>

Re: [PATCH v7 01/10] media: v4l2-subdev: Introduce [get|set]_mbus_config pad ops

[Hans Verkuil]

On 17/07/2020 09:20, Jacopo Mondi wrote:
> Hi Janusz,
>   thanks for review
> 
> On Fri, Jul 17, 2020 at 12:15:27AM +0200, Janusz Krzysztofik wrote:
>> Hi Jacopo,
>>
>> On Thursday, July 16, 2020 4:27:04 P.M. CEST Jacopo Mondi wrote:
>>> Introduce two new pad operations to allow retrieving and configuring the
>>> media bus parameters on a subdevice pad.
>>>
>>> The newly introduced operations aims to replace the s/g_mbus_config video
>>> operations, which have been on their way for deprecation since a long
>>> time.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>
>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>> index f7fe78a6f65a..d8b9d5735307 100644
>>> --- a/include/media/v4l2-subdev.h
>>> +++ b/include/media/v4l2-subdev.h
>>> @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config {
>>>   *
>>>   * @set_frame_desc: set the low level media bus frame parameters, @fd array
>>>   *                  may be adjusted by the subdev driver to device capabilities.
>>> + *
>>> + * @get_mbus_config: get the media bus configuration of a remote sub-device.
>>> + *		     The media bus configuration is usually retrieved from the
>>> + *		     firmware interface at sub-device probe time, immediately
>>> + *		     applied to the hardware and eventually adjusted by the
>>> + *		     driver. Remote sub-devices (usually video receivers) shall
>>> + *		     use this operation to query the transmitting end bus
>>> + *		     configuration in order to adjust their own one accordingly.
>>> + *		     Callers should make sure they get the most up-to-date as
>>> + *		     possible configuration from the remote end, likely calling
>>> + *		     this operation as close as possible to stream on time. The
>>> + *		     operation shall fail if the pad index it has been called on
>>> + *		     is not valid.
>>> + *
>>> + * @set_mbus_config: set the media bus configuration of a remote sub-device.
>>> + *		     This operations is intended to allow, in combination with
>>> + *		     the get_mbus_config operation, the negotiation of media bus
>>> + *		     configuration parameters between media sub-devices. The
>>> + *		     operation shall not fail if the requested configuration is
>>> + *		     not supported, but the driver shall update the content of
>>> + *		     the %config argument to reflect what has been actually
>>> + *		     applied to the hardware. The operation shall fail if the
>>> + *		     pad index it has been called on is not valid.
> 
> First of all, Hans sent a pull request for this series yesterday. Are
> you ok with that and with sending patches on top, or do you think the
> inclusion of the series should be post-poned ? (you can imagine what
> my preference is :)
> 
>>
>> Could this description also clarify what results are expected in case of
>> hardware errors?  The ov6650 implementation you propose may suggest such
>> errors may be expected to be ignored silently as long as current configuration
>> can be successfully obtained from hardware and passed back to the caller.
> 
> I guess "it depends"(tm)
> I know this doesn't sound like a good answer :)
> 
> A failure in applying or reading register likely means the driver is
> trying to access the hardware while it is in low power state. In this
> case I would consider this a driver bug, as it should try to be smart
> and cache settings and apply them at the proper time and return the
> current configuration, which should be cached as well.
> 
> Of course things could go wrong for several reasons, and in the case
> if any unexpected error occurs I think an error is expected to be
> reported. Please note that this v7 of ov6650 does that by returning
> the return value of ov6650_get_mbus_config() at the end of
> ov6650_set_mbus_config.
> 
> I guess this is pretty regular behaviour, although I missed it in the
> previous version, so it might be worth adding an additional line to
> the documentation.
> 
>>
>> Moreover, since validity of the pad argument is expected to be verified, I
>> think this should be handled by the media infrastructure layer with the
>> drivers/media/v4l2-core/v4l2-subdev.c:check_pad() helper called from a
>> .set_mbus_config() wrapper added to v4l2_subdev_call_pad_wrappers, freeing
>> drivers from reimplementing it.
>>
> 
> Good point, and indeed my bad as I thought v4l2_subdev_call() would
> already do that by default, but looking at the actual implementation a
> wrapper might be a good idea indeed.
> 
> Patches on top ?

I prefer a v8. I'll just drop the PR in patchwork. This pad check is
IMHO important enough to get that in the series. I completely forgot
about that check...

Regards,

	Hans

> 
> Thanks
>   j
> 
>> Thanks,
>> Janusz
>>
>>>   */
>>>  struct v4l2_subdev_pad_ops {
>>>  	int (*init_cfg)(struct v4l2_subdev *sd,
>>> @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops {
>>>  			      struct v4l2_mbus_frame_desc *fd);
>>>  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
>>>  			      struct v4l2_mbus_frame_desc *fd);
>>> +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
>>> +			       struct v4l2_mbus_config *config);
>>> +	int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
>>> +			       struct v4l2_mbus_config *config);
>>>  };
>>>
>>>  /**
>>>
>>
>>
>>
>>

Re: [PATCH v7 01/10] media: v4l2-subdev: Introduce [get|set]_mbus_config pad ops

[Janusz Krzysztofik]

Hi Jacopo,

On Friday, July 17, 2020 9:20:18 A.M. CEST Jacopo Mondi wrote:
> Hi Janusz,
>   thanks for review
> 
> On Fri, Jul 17, 2020 at 12:15:27AM +0200, Janusz Krzysztofik wrote:
> > Hi Jacopo,
> >
> > On Thursday, July 16, 2020 4:27:04 P.M. CEST Jacopo Mondi wrote:
> > > Introduce two new pad operations to allow retrieving and configuring the
> > > media bus parameters on a subdevice pad.
> > >
> > > The newly introduced operations aims to replace the s/g_mbus_config video
> > > operations, which have been on their way for deprecation since a long
> > > time.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > >
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index f7fe78a6f65a..d8b9d5735307 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config {
> > >   *
> > >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> > >   *                  may be adjusted by the subdev driver to device capabilities.
> > > + *
> > > + * @get_mbus_config: get the media bus configuration of a remote sub-device.
> > > + *		     The media bus configuration is usually retrieved from the
> > > + *		     firmware interface at sub-device probe time, immediately
> > > + *		     applied to the hardware and eventually adjusted by the
> > > + *		     driver. Remote sub-devices (usually video receivers) shall
> > > + *		     use this operation to query the transmitting end bus
> > > + *		     configuration in order to adjust their own one accordingly.
> > > + *		     Callers should make sure they get the most up-to-date as
> > > + *		     possible configuration from the remote end, likely calling
> > > + *		     this operation as close as possible to stream on time. The
> > > + *		     operation shall fail if the pad index it has been called on
> > > + *		     is not valid.
> > > + *
> > > + * @set_mbus_config: set the media bus configuration of a remote sub-device.
> > > + *		     This operations is intended to allow, in combination with
> > > + *		     the get_mbus_config operation, the negotiation of media bus
> > > + *		     configuration parameters between media sub-devices. The
> > > + *		     operation shall not fail if the requested configuration is
> > > + *		     not supported, but the driver shall update the content of
> > > + *		     the %config argument to reflect what has been actually
> > > + *		     applied to the hardware. The operation shall fail if the
> > > + *		     pad index it has been called on is not valid.
> 
> First of all, Hans sent a pull request for this series yesterday. Are
> you ok with that and with sending patches on top, or do you think the
> inclusion of the series should be post-poned ? (you can imagine what
> my preference is :)
> 
> >
> > Could this description also clarify what results are expected in case of
> > hardware errors?  The ov6650 implementation you propose may suggest such
> > errors may be expected to be ignored silently as long as current configuration
> > can be successfully obtained from hardware and passed back to the caller.
> 
> I guess "it depends"(tm)
> I know this doesn't sound like a good answer :)
> 
> A failure in applying or reading register likely means the driver is
> trying to access the hardware while it is in low power state. In this
> case I would consider this a driver bug, as it should try to be smart
> and cache settings and apply them at the proper time and return the
> current configuration, which should be cached as well.
> 
> Of course things could go wrong for several reasons, and in the case
> if any unexpected error occurs I think an error is expected to be
> reported. Please note that this v7 of ov6650 does that by returning
> the return value of ov6650_get_mbus_config() at the end of
> ov6650_set_mbus_config.

Current configuration is not cached in your implementation proposed for ov6650.  
If it was, would ov6650_set_mbus_config() always succeed, just passing it back 
updated with successful writes and ignoring write errors?  In other words, 
is this the expected behavior of .set_mbus_config() to always treat 
unsuccessful writes as recoverable errors and never report them to the caller 
as long as there is a current configuration available that can passed back?

Thanks,
Janusz

> 
> I guess this is pretty regular behaviour, although I missed it in the
> previous version, so it might be worth adding an additional line to
> the documentation.
>
> >
> > Moreover, since validity of the pad argument is expected to be verified, I
> > think this should be handled by the media infrastructure layer with the
> > drivers/media/v4l2-core/v4l2-subdev.c:check_pad() helper called from a
> > .set_mbus_config() wrapper added to v4l2_subdev_call_pad_wrappers, freeing
> > drivers from reimplementing it.
> >
> 
> Good point, and indeed my bad as I thought v4l2_subdev_call() would
> already do that by default, but looking at the actual implementation a
> wrapper might be a good idea indeed.
> 
> Patches on top ?
> 
> Thanks
>   j
> 
> > Thanks,
> > Janusz
> >
> > >   */
> > >  struct v4l2_subdev_pad_ops {
> > >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > > @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops {
> > >  			      struct v4l2_mbus_frame_desc *fd);
> > >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > >  			      struct v4l2_mbus_frame_desc *fd);
> > > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > +			       struct v4l2_mbus_config *config);
> > > +	int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > +			       struct v4l2_mbus_config *config);
> > >  };
> > >
> > >  /**
> > >
> >
> >
> >
> >
> 

Re: [PATCH v7 01/10] media: v4l2-subdev: Introduce [get|set]_mbus_config pad ops

[Jacopo Mondi]

Hi Janusz,

On Sat, Jul 18, 2020 at 01:23:24AM +0200, Janusz Krzysztofik wrote:
> Hi Jacopo,
>
> On Friday, July 17, 2020 9:20:18 A.M. CEST Jacopo Mondi wrote:
> > Hi Janusz,
> >   thanks for review
> >
> > On Fri, Jul 17, 2020 at 12:15:27AM +0200, Janusz Krzysztofik wrote:
> > > Hi Jacopo,
> > >
> > > On Thursday, July 16, 2020 4:27:04 P.M. CEST Jacopo Mondi wrote:
> > > > Introduce two new pad operations to allow retrieving and configuring the
> > > > media bus parameters on a subdevice pad.
> > > >
> > > > The newly introduced operations aims to replace the s/g_mbus_config video
> > > > operations, which have been on their way for deprecation since a long
> > > > time.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++
> > > >  1 file changed, 27 insertions(+)
> > > >
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index f7fe78a6f65a..d8b9d5735307 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config {
> > > >   *
> > > >   * @set_frame_desc: set the low level media bus frame parameters, @fd array
> > > >   *                  may be adjusted by the subdev driver to device capabilities.
> > > > + *
> > > > + * @get_mbus_config: get the media bus configuration of a remote sub-device.
> > > > + *		     The media bus configuration is usually retrieved from the
> > > > + *		     firmware interface at sub-device probe time, immediately
> > > > + *		     applied to the hardware and eventually adjusted by the
> > > > + *		     driver. Remote sub-devices (usually video receivers) shall
> > > > + *		     use this operation to query the transmitting end bus
> > > > + *		     configuration in order to adjust their own one accordingly.
> > > > + *		     Callers should make sure they get the most up-to-date as
> > > > + *		     possible configuration from the remote end, likely calling
> > > > + *		     this operation as close as possible to stream on time. The
> > > > + *		     operation shall fail if the pad index it has been called on
> > > > + *		     is not valid.
> > > > + *
> > > > + * @set_mbus_config: set the media bus configuration of a remote sub-device.
> > > > + *		     This operations is intended to allow, in combination with
> > > > + *		     the get_mbus_config operation, the negotiation of media bus
> > > > + *		     configuration parameters between media sub-devices. The
> > > > + *		     operation shall not fail if the requested configuration is
> > > > + *		     not supported, but the driver shall update the content of
> > > > + *		     the %config argument to reflect what has been actually
> > > > + *		     applied to the hardware. The operation shall fail if the
> > > > + *		     pad index it has been called on is not valid.
> >
> > First of all, Hans sent a pull request for this series yesterday. Are
> > you ok with that and with sending patches on top, or do you think the
> > inclusion of the series should be post-poned ? (you can imagine what
> > my preference is :)
> >
> > >
> > > Could this description also clarify what results are expected in case of
> > > hardware errors?  The ov6650 implementation you propose may suggest such
> > > errors may be expected to be ignored silently as long as current configuration
> > > can be successfully obtained from hardware and passed back to the caller.
> >
> > I guess "it depends"(tm)
> > I know this doesn't sound like a good answer :)
> >
> > A failure in applying or reading register likely means the driver is
> > trying to access the hardware while it is in low power state. In this
> > case I would consider this a driver bug, as it should try to be smart
> > and cache settings and apply them at the proper time and return the
> > current configuration, which should be cached as well.
> >
> > Of course things could go wrong for several reasons, and in the case
> > if any unexpected error occurs I think an error is expected to be
> > reported. Please note that this v7 of ov6650 does that by returning
> > the return value of ov6650_get_mbus_config() at the end of
> > ov6650_set_mbus_config.
>
> Current configuration is not cached in your implementation proposed for ov6650.
> If it was, would ov6650_set_mbus_config() always succeed, just passing it back
> updated with successful writes and ignoring write errors?  In other words,
> is this the expected behavior of .set_mbus_config() to always treat
> unsuccessful writes as recoverable errors and never report them to the caller
> as long as there is a current configuration available that can passed back?

I don't see that driver implementing any sort of power ref-counting at
the moment... In example s_ftm goes to the register without actually
making sure the chip is powered or not.

I guess this is /fine/, currently there's a big confusion in the i2c
sensor drivers land on where this has to be implemented... However
this is a 'legacy' driver, with no media controller support and no
devnode exposed to userspace, so I guess the bridge driver is in
charge of making sure it interacts with the sensor after s_power(1)
has been called.

Modern sensor drivers, which use runtime_pm and implement the
VIDIOC_SUBDEV_ API to userspace, are expected to handle power properly
as they can receive  calls from applications at any time. The most
trivial solution would be to power up the sensor at fops.open() time
and keep it powered, but that's clearly a waste, hence if the driver
implements a smarter power management scheme it should take care of
caching, as it would do for formats and controls.

Does that make sense to you ?

Thanks
  j

>
> Thanks,
> Janusz
>
> >
> > I guess this is pretty regular behaviour, although I missed it in the
> > previous version, so it might be worth adding an additional line to
> > the documentation.
> >
> > >
> > > Moreover, since validity of the pad argument is expected to be verified, I
> > > think this should be handled by the media infrastructure layer with the
> > > drivers/media/v4l2-core/v4l2-subdev.c:check_pad() helper called from a
> > > .set_mbus_config() wrapper added to v4l2_subdev_call_pad_wrappers, freeing
> > > drivers from reimplementing it.
> > >
> >
> > Good point, and indeed my bad as I thought v4l2_subdev_call() would
> > already do that by default, but looking at the actual implementation a
> > wrapper might be a good idea indeed.
> >
> > Patches on top ?
> >
> > Thanks
> >   j
> >
> > > Thanks,
> > > Janusz
> > >
> > > >   */
> > > >  struct v4l2_subdev_pad_ops {
> > > >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > > > @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops {
> > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > +			       struct v4l2_mbus_config *config);
> > > > +	int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > +			       struct v4l2_mbus_config *config);
> > > >  };
> > > >
> > > >  /**
> > > >
> > >
> > >
> > >
> > >
> >
>
>
>
>

Re: [PATCH v7 01/10] media: v4l2-subdev: Introduce [get|set]_mbus_config pad ops

[Janusz Krzysztofik]

On Monday, July 20, 2020 5:56:50 P.M. CEST Jacopo Mondi wrote:
> Hi Janusz,
> 
> On Sat, Jul 18, 2020 at 01:23:24AM +0200, Janusz Krzysztofik wrote:
> > Hi Jacopo,
> >
> > On Friday, July 17, 2020 9:20:18 A.M. CEST Jacopo Mondi wrote:
> > > Hi Janusz,
> > >   thanks for review
> > >
> > > On Fri, Jul 17, 2020 at 12:15:27AM +0200, Janusz Krzysztofik wrote:
> > > > Hi Jacopo,
> > > >
> > > > On Thursday, July 16, 2020 4:27:04 P.M. CEST Jacopo Mondi wrote:
> > > > > Introduce two new pad operations to allow retrieving and configuring 
the
> > > > > media bus parameters on a subdevice pad.
> > > > >
> > > > > The newly introduced operations aims to replace the s/g_mbus_config 
video
> > > > > operations, which have been on their way for deprecation since a 
long
> > > > > time.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > ---
> > > > >  include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++
> > > > >  1 file changed, 27 insertions(+)
> > > > >
> > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-
subdev.h
> > > > > index f7fe78a6f65a..d8b9d5735307 100644
> > > > > --- a/include/media/v4l2-subdev.h
> > > > > +++ b/include/media/v4l2-subdev.h
> > > > > @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config {
> > > > >   *
> > > > >   * @set_frame_desc: set the low level media bus frame parameters, 
@fd array
> > > > >   *                  may be adjusted by the subdev driver to device 
capabilities.
> > > > > + *
> > > > > + * @get_mbus_config: get the media bus configuration of a remote sub-
device.
> > > > > + *		     The media bus configuration is usually retrieved 
from the
> > > > > + *		     firmware interface at sub-device probe time, 
immediately
> > > > > + *		     applied to the hardware and eventually adjusted 
by the
> > > > > + *		     driver. Remote sub-devices (usually video 
receivers) shall
> > > > > + *		     use this operation to query the transmitting 
end bus
> > > > > + *		     configuration in order to adjust their own one 
accordingly.
> > > > > + *		     Callers should make sure they get the most up-
to-date as
> > > > > + *		     possible configuration from the remote end, 
likely calling
> > > > > + *		     this operation as close as possible to stream 
on time. The
> > > > > + *		     operation shall fail if the pad index it has 
been called on
> > > > > + *		     is not valid.
> > > > > + *
> > > > > + * @set_mbus_config: set the media bus configuration of a remote sub-
device.
> > > > > + *		     This operations is intended to allow, in 
combination with
> > > > > + *		     the get_mbus_config operation, the negotiation 
of media bus
> > > > > + *		     configuration parameters between media sub-
devices. The
> > > > > + *		     operation shall not fail if the requested 
configuration is
> > > > > + *		     not supported, but the driver shall update the 
content of
> > > > > + *		     the %config argument to reflect what has been 
actually
> > > > > + *		     applied to the hardware. The operation shall 
fail if the
> > > > > + *		     pad index it has been called on is not valid.
> > >
> > > First of all, Hans sent a pull request for this series yesterday. Are
> > > you ok with that and with sending patches on top, or do you think the
> > > inclusion of the series should be post-poned ? (you can imagine what
> > > my preference is :)
> > >
> > > >
> > > > Could this description also clarify what results are expected in case 
of
> > > > hardware errors?  The ov6650 implementation you propose may suggest 
such
> > > > errors may be expected to be ignored silently as long as current 
configuration
> > > > can be successfully obtained from hardware and passed back to the 
caller.
> > >
> > > I guess "it depends"(tm)
> > > I know this doesn't sound like a good answer :)
> > >
> > > A failure in applying or reading register likely means the driver is
> > > trying to access the hardware while it is in low power state. In this
> > > case I would consider this a driver bug, as it should try to be smart
> > > and cache settings and apply them at the proper time and return the
> > > current configuration, which should be cached as well.
> > >
> > > Of course things could go wrong for several reasons, and in the case
> > > if any unexpected error occurs I think an error is expected to be
> > > reported. Please note that this v7 of ov6650 does that by returning
> > > the return value of ov6650_get_mbus_config() at the end of
> > > ov6650_set_mbus_config.
> >
> > Current configuration is not cached in your implementation proposed for 
ov6650.
> > If it was, would ov6650_set_mbus_config() always succeed, just passing it 
back
> > updated with successful writes and ignoring write errors?  In other words,
> > is this the expected behavior of .set_mbus_config() to always treat
> > unsuccessful writes as recoverable errors and never report them to the 
caller
> > as long as there is a current configuration available that can passed back?
> 
> I don't see that driver implementing any sort of power ref-counting at
> the moment... In example s_ftm goes to the register without actually
> making sure the chip is powered or not.
> 
> I guess this is /fine/, currently there's a big confusion in the i2c
> sensor drivers land on where this has to be implemented... However
> this is a 'legacy' driver, with no media controller support and no
> devnode exposed to userspace, so I guess the bridge driver is in
> charge of making sure it interacts with the sensor after s_power(1)
> has been called.
> 
> Modern sensor drivers, which use runtime_pm and implement the
> VIDIOC_SUBDEV_ API to userspace, are expected to handle power properly
> as they can receive  calls from applications at any time. The most
> trivial solution would be to power up the sensor at fops.open() time
> and keep it powered, but that's clearly a waste, hence if the driver
> implements a smarter power management scheme it should take care of
> caching, as it would do for formats and controls.
> 
> Does that make sense to you ?

I still think hardware errors should be returned, not ignored, regardless of 
power management support level.  Ignoring them can be confusing.  Specific 
configuration requests shouldn't be able to give different results while still 
returning success when hardware errors occur, I believe.  And that would be 
the case with your ov6650 implementation if for example writes were failing 
sporadically and reads always succeeding.

Unless such behavior is really expected from pad operations, and that's what 
my question was about.

Thanks,
Janusz

> 
> Thanks
>   j
> 
> >
> > Thanks,
> > Janusz
> >
> > >
> > > I guess this is pretty regular behaviour, although I missed it in the
> > > previous version, so it might be worth adding an additional line to
> > > the documentation.
> > >
> > > >
> > > > Moreover, since validity of the pad argument is expected to be 
verified, I
> > > > think this should be handled by the media infrastructure layer with 
the
> > > > drivers/media/v4l2-core/v4l2-subdev.c:check_pad() helper called from a
> > > > .set_mbus_config() wrapper added to v4l2_subdev_call_pad_wrappers, 
freeing
> > > > drivers from reimplementing it.
> > > >
> > >
> > > Good point, and indeed my bad as I thought v4l2_subdev_call() would
> > > already do that by default, but looking at the actual implementation a
> > > wrapper might be a good idea indeed.
> > >
> > > Patches on top ?
> > >
> > > Thanks
> > >   j
> > >
> > > > Thanks,
> > > > Janusz
> > > >
> > > > >   */
> > > > >  struct v4l2_subdev_pad_ops {
> > > > >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > > > > @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops {
> > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > +			       struct v4l2_mbus_config *config);
> > > > > +	int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > +			       struct v4l2_mbus_config *config);
> > > > >  };
> > > > >
> > > > >  /**
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> >
> >
> >
> >
> 

Re: [PATCH v7 01/10] media: v4l2-subdev: Introduce [get|set]_mbus_config pad ops

[Jacopo Mondi]

Hi Janusz,

On Mon, Jul 20, 2020 at 09:04:38PM +0200, Janusz Krzysztofik wrote:
> On Monday, July 20, 2020 5:56:50 P.M. CEST Jacopo Mondi wrote:
> > Hi Janusz,
> >
> > On Sat, Jul 18, 2020 at 01:23:24AM +0200, Janusz Krzysztofik wrote:
> > > Hi Jacopo,
> > >
> > > On Friday, July 17, 2020 9:20:18 A.M. CEST Jacopo Mondi wrote:
> > > > Hi Janusz,
> > > >   thanks for review
> > > >
> > > > On Fri, Jul 17, 2020 at 12:15:27AM +0200, Janusz Krzysztofik wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > On Thursday, July 16, 2020 4:27:04 P.M. CEST Jacopo Mondi wrote:
> > > > > > Introduce two new pad operations to allow retrieving and configuring
> the
> > > > > > media bus parameters on a subdevice pad.
> > > > > >
> > > > > > The newly introduced operations aims to replace the s/g_mbus_config
> video
> > > > > > operations, which have been on their way for deprecation since a
> long
> > > > > > time.
> > > > > >
> > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > ---
> > > > > >  include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++
> > > > > >  1 file changed, 27 insertions(+)
> > > > > >
> > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-
> subdev.h
> > > > > > index f7fe78a6f65a..d8b9d5735307 100644
> > > > > > --- a/include/media/v4l2-subdev.h
> > > > > > +++ b/include/media/v4l2-subdev.h
> > > > > > @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config {
> > > > > >   *
> > > > > >   * @set_frame_desc: set the low level media bus frame parameters,
> @fd array
> > > > > >   *                  may be adjusted by the subdev driver to device
> capabilities.
> > > > > > + *
> > > > > > + * @get_mbus_config: get the media bus configuration of a remote sub-
> device.
> > > > > > + *		     The media bus configuration is usually retrieved
> from the
> > > > > > + *		     firmware interface at sub-device probe time,
> immediately
> > > > > > + *		     applied to the hardware and eventually adjusted
> by the
> > > > > > + *		     driver. Remote sub-devices (usually video
> receivers) shall
> > > > > > + *		     use this operation to query the transmitting
> end bus
> > > > > > + *		     configuration in order to adjust their own one
> accordingly.
> > > > > > + *		     Callers should make sure they get the most up-
> to-date as
> > > > > > + *		     possible configuration from the remote end,
> likely calling
> > > > > > + *		     this operation as close as possible to stream
> on time. The
> > > > > > + *		     operation shall fail if the pad index it has
> been called on
> > > > > > + *		     is not valid.
> > > > > > + *
> > > > > > + * @set_mbus_config: set the media bus configuration of a remote sub-
> device.
> > > > > > + *		     This operations is intended to allow, in
> combination with
> > > > > > + *		     the get_mbus_config operation, the negotiation
> of media bus
> > > > > > + *		     configuration parameters between media sub-
> devices. The
> > > > > > + *		     operation shall not fail if the requested
> configuration is
> > > > > > + *		     not supported, but the driver shall update the
> content of
> > > > > > + *		     the %config argument to reflect what has been
> actually
> > > > > > + *		     applied to the hardware. The operation shall
> fail if the
> > > > > > + *		     pad index it has been called on is not valid.
> > > >
> > > > First of all, Hans sent a pull request for this series yesterday. Are
> > > > you ok with that and with sending patches on top, or do you think the
> > > > inclusion of the series should be post-poned ? (you can imagine what
> > > > my preference is :)
> > > >
> > > > >
> > > > > Could this description also clarify what results are expected in case
> of
> > > > > hardware errors?  The ov6650 implementation you propose may suggest
> such
> > > > > errors may be expected to be ignored silently as long as current
> configuration
> > > > > can be successfully obtained from hardware and passed back to the
> caller.
> > > >
> > > > I guess "it depends"(tm)
> > > > I know this doesn't sound like a good answer :)
> > > >
> > > > A failure in applying or reading register likely means the driver is
> > > > trying to access the hardware while it is in low power state. In this
> > > > case I would consider this a driver bug, as it should try to be smart
> > > > and cache settings and apply them at the proper time and return the
> > > > current configuration, which should be cached as well.
> > > >
> > > > Of course things could go wrong for several reasons, and in the case
> > > > if any unexpected error occurs I think an error is expected to be
> > > > reported. Please note that this v7 of ov6650 does that by returning
> > > > the return value of ov6650_get_mbus_config() at the end of
> > > > ov6650_set_mbus_config.
> > >
> > > Current configuration is not cached in your implementation proposed for
> ov6650.
> > > If it was, would ov6650_set_mbus_config() always succeed, just passing it
> back
> > > updated with successful writes and ignoring write errors?  In other words,
> > > is this the expected behavior of .set_mbus_config() to always treat
> > > unsuccessful writes as recoverable errors and never report them to the
> caller
> > > as long as there is a current configuration available that can passed back?
> >
> > I don't see that driver implementing any sort of power ref-counting at
> > the moment... In example s_ftm goes to the register without actually
> > making sure the chip is powered or not.
> >
> > I guess this is /fine/, currently there's a big confusion in the i2c
> > sensor drivers land on where this has to be implemented... However
> > this is a 'legacy' driver, with no media controller support and no
> > devnode exposed to userspace, so I guess the bridge driver is in
> > charge of making sure it interacts with the sensor after s_power(1)
> > has been called.
> >
> > Modern sensor drivers, which use runtime_pm and implement the
> > VIDIOC_SUBDEV_ API to userspace, are expected to handle power properly
> > as they can receive  calls from applications at any time. The most
> > trivial solution would be to power up the sensor at fops.open() time
> > and keep it powered, but that's clearly a waste, hence if the driver
> > implements a smarter power management scheme it should take care of
> > caching, as it would do for formats and controls.
> >
> > Does that make sense to you ?
>
> I still think hardware errors should be returned, not ignored, regardless of
> power management support level.  Ignoring them can be confusing.  Specific
> configuration requests shouldn't be able to give different results while still
> returning success when hardware errors occur, I believe.  And that would be
> the case with your ov6650 implementation if for example writes were failing
> sporadically and reads always succeeding.
>
> Unless such behavior is really expected from pad operations, and that's what
> my question was about.

Oh I see what you mean! I was sure I was returning errors on failed
writes :/

I will send a 8.1 soon and it will look like this

/* Alter bus settings on camera side */
static int ov6650_set_mbus_config(struct v4l2_subdev *sd,
				  unsigned int pad,
				  struct v4l2_mbus_config *cfg)
{
	struct i2c_client *client = v4l2_get_subdevdata(sd);
	int ret = 0;

	if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
	else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
	if (ret)
		return ret;

	if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
		ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0);
	else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
		ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW);
	if (ret)
	        return ret;

	if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0);
	else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH);
	if (ret)
	        return ret;

	/*
	 * Update the configuration to report what is actually applied to
	 * the hardware.
	 */
	return ov6650_get_mbus_config(sd, pad, cfg);
}

Sorry about this, I should have noticed that you where suggesting
s/goto error/return ret/ and not just to return the get_mbus_config()
return value at function end.

v8.1 of this patch soon.

Hans sorry for the churn. Do you think you can still collect this
today, maybe with Janusz's ack if he feels to ?

Thanks
   j

>
> Thanks,
> Janusz
>
> >
> > Thanks
> >   j
> >
> > >
> > > Thanks,
> > > Janusz
> > >
> > > >
> > > > I guess this is pretty regular behaviour, although I missed it in the
> > > > previous version, so it might be worth adding an additional line to
> > > > the documentation.
> > > >
> > > > >
> > > > > Moreover, since validity of the pad argument is expected to be
> verified, I
> > > > > think this should be handled by the media infrastructure layer with
> the
> > > > > drivers/media/v4l2-core/v4l2-subdev.c:check_pad() helper called from a
> > > > > .set_mbus_config() wrapper added to v4l2_subdev_call_pad_wrappers,
> freeing
> > > > > drivers from reimplementing it.
> > > > >
> > > >
> > > > Good point, and indeed my bad as I thought v4l2_subdev_call() would
> > > > already do that by default, but looking at the actual implementation a
> > > > wrapper might be a good idea indeed.
> > > >
> > > > Patches on top ?
> > > >
> > > > Thanks
> > > >   j
> > > >
> > > > > Thanks,
> > > > > Janusz
> > > > >
> > > > > >   */
> > > > > >  struct v4l2_subdev_pad_ops {
> > > > > >  	int (*init_cfg)(struct v4l2_subdev *sd,
> > > > > > @@ -710,6 +733,10 @@ struct v4l2_subdev_pad_ops {
> > > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > > >  	int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > >  			      struct v4l2_mbus_frame_desc *fd);
> > > > > > +	int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > > +			       struct v4l2_mbus_config *config);
> > > > > > +	int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > > +			       struct v4l2_mbus_config *config);
> > > > > >  };
> > > > > >
> > > > > >  /**
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> > >
> > >
> > >
> >
>
>
>
>

Re: [PATCH v7 01/10] media: v4l2-subdev: Introduce [get|set]_mbus_config pad ops

[Hans Verkuil]

On 21/07/2020 09:27, Jacopo Mondi wrote:
> Hi Janusz,
> 
> On Mon, Jul 20, 2020 at 09:04:38PM +0200, Janusz Krzysztofik wrote:
>> On Monday, July 20, 2020 5:56:50 P.M. CEST Jacopo Mondi wrote:
>>> Hi Janusz,
>>>
>>> On Sat, Jul 18, 2020 at 01:23:24AM +0200, Janusz Krzysztofik wrote:
>>>> Hi Jacopo,
>>>>
>>>> On Friday, July 17, 2020 9:20:18 A.M. CEST Jacopo Mondi wrote:
>>>>> Hi Janusz,
>>>>>   thanks for review
>>>>>
>>>>> On Fri, Jul 17, 2020 at 12:15:27AM +0200, Janusz Krzysztofik wrote:
>>>>>> Hi Jacopo,
>>>>>>
>>>>>> On Thursday, July 16, 2020 4:27:04 P.M. CEST Jacopo Mondi wrote:
>>>>>>> Introduce two new pad operations to allow retrieving and configuring
>> the
>>>>>>> media bus parameters on a subdevice pad.
>>>>>>>
>>>>>>> The newly introduced operations aims to replace the s/g_mbus_config
>> video
>>>>>>> operations, which have been on their way for deprecation since a
>> long
>>>>>>> time.
>>>>>>>
>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>>>>> ---
>>>>>>>  include/media/v4l2-subdev.h | 27 +++++++++++++++++++++++++++
>>>>>>>  1 file changed, 27 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-
>> subdev.h
>>>>>>> index f7fe78a6f65a..d8b9d5735307 100644
>>>>>>> --- a/include/media/v4l2-subdev.h
>>>>>>> +++ b/include/media/v4l2-subdev.h
>>>>>>> @@ -670,6 +670,29 @@ struct v4l2_subdev_pad_config {
>>>>>>>   *
>>>>>>>   * @set_frame_desc: set the low level media bus frame parameters,
>> @fd array
>>>>>>>   *                  may be adjusted by the subdev driver to device
>> capabilities.
>>>>>>> + *
>>>>>>> + * @get_mbus_config: get the media bus configuration of a remote sub-
>> device.
>>>>>>> + *		     The media bus configuration is usually retrieved
>> from the
>>>>>>> + *		     firmware interface at sub-device probe time,
>> immediately
>>>>>>> + *		     applied to the hardware and eventually adjusted
>> by the
>>>>>>> + *		     driver. Remote sub-devices (usually video
>> receivers) shall
>>>>>>> + *		     use this operation to query the transmitting
>> end bus
>>>>>>> + *		     configuration in order to adjust their own one
>> accordingly.
>>>>>>> + *		     Callers should make sure they get the most up-
>> to-date as
>>>>>>> + *		     possible configuration from the remote end,
>> likely calling
>>>>>>> + *		     this operation as close as possible to stream
>> on time. The
>>>>>>> + *		     operation shall fail if the pad index it has
>> been called on
>>>>>>> + *		     is not valid.
>>>>>>> + *
>>>>>>> + * @set_mbus_config: set the media bus configuration of a remote sub-
>> device.
>>>>>>> + *		     This operations is intended to allow, in
>> combination with
>>>>>>> + *		     the get_mbus_config operation, the negotiation
>> of media bus
>>>>>>> + *		     configuration parameters between media sub-
>> devices. The
>>>>>>> + *		     operation shall not fail if the requested
>> configuration is
>>>>>>> + *		     not supported, but the driver shall update the
>> content of
>>>>>>> + *		     the %config argument to reflect what has been
>> actually
>>>>>>> + *		     applied to the hardware. The operation shall
>> fail if the
>>>>>>> + *		     pad index it has been called on is not valid.
>>>>>
>>>>> First of all, Hans sent a pull request for this series yesterday. Are
>>>>> you ok with that and with sending patches on top, or do you think the
>>>>> inclusion of the series should be post-poned ? (you can imagine what
>>>>> my preference is :)
>>>>>
>>>>>>
>>>>>> Could this description also clarify what results are expected in case
>> of
>>>>>> hardware errors?  The ov6650 implementation you propose may suggest
>> such
>>>>>> errors may be expected to be ignored silently as long as current
>> configuration
>>>>>> can be successfully obtained from hardware and passed back to the
>> caller.
>>>>>
>>>>> I guess "it depends"(tm)
>>>>> I know this doesn't sound like a good answer :)
>>>>>
>>>>> A failure in applying or reading register likely means the driver is
>>>>> trying to access the hardware while it is in low power state. In this
>>>>> case I would consider this a driver bug, as it should try to be smart
>>>>> and cache settings and apply them at the proper time and return the
>>>>> current configuration, which should be cached as well.
>>>>>
>>>>> Of course things could go wrong for several reasons, and in the case
>>>>> if any unexpected error occurs I think an error is expected to be
>>>>> reported. Please note that this v7 of ov6650 does that by returning
>>>>> the return value of ov6650_get_mbus_config() at the end of
>>>>> ov6650_set_mbus_config.
>>>>
>>>> Current configuration is not cached in your implementation proposed for
>> ov6650.
>>>> If it was, would ov6650_set_mbus_config() always succeed, just passing it
>> back
>>>> updated with successful writes and ignoring write errors?  In other words,
>>>> is this the expected behavior of .set_mbus_config() to always treat
>>>> unsuccessful writes as recoverable errors and never report them to the
>> caller
>>>> as long as there is a current configuration available that can passed back?
>>>
>>> I don't see that driver implementing any sort of power ref-counting at
>>> the moment... In example s_ftm goes to the register without actually
>>> making sure the chip is powered or not.
>>>
>>> I guess this is /fine/, currently there's a big confusion in the i2c
>>> sensor drivers land on where this has to be implemented... However
>>> this is a 'legacy' driver, with no media controller support and no
>>> devnode exposed to userspace, so I guess the bridge driver is in
>>> charge of making sure it interacts with the sensor after s_power(1)
>>> has been called.
>>>
>>> Modern sensor drivers, which use runtime_pm and implement the
>>> VIDIOC_SUBDEV_ API to userspace, are expected to handle power properly
>>> as they can receive  calls from applications at any time. The most
>>> trivial solution would be to power up the sensor at fops.open() time
>>> and keep it powered, but that's clearly a waste, hence if the driver
>>> implements a smarter power management scheme it should take care of
>>> caching, as it would do for formats and controls.
>>>
>>> Does that make sense to you ?
>>
>> I still think hardware errors should be returned, not ignored, regardless of
>> power management support level.  Ignoring them can be confusing.  Specific
>> configuration requests shouldn't be able to give different results while still
>> returning success when hardware errors occur, I believe.  And that would be
>> the case with your ov6650 implementation if for example writes were failing
>> sporadically and reads always succeeding.
>>
>> Unless such behavior is really expected from pad operations, and that's what
>> my question was about.
> 
> Oh I see what you mean! I was sure I was returning errors on failed
> writes :/
> 
> I will send a 8.1 soon and it will look like this
> 
> /* Alter bus settings on camera side */
> static int ov6650_set_mbus_config(struct v4l2_subdev *sd,
> 				  unsigned int pad,
> 				  struct v4l2_mbus_config *cfg)
> {
> 	struct i2c_client *client = v4l2_get_subdevdata(sd);
> 	int ret = 0;
> 
> 	if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> 		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_PCLK_RISING, 0);
> 	else if (cfg->flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> 		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_PCLK_RISING);
> 	if (ret)
> 		return ret;
> 
> 	if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> 		ret = ov6650_reg_rmw(client, REG_COMF, COMF_HREF_LOW, 0);
> 	else if (cfg->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> 		ret = ov6650_reg_rmw(client, REG_COMF, 0, COMF_HREF_LOW);
> 	if (ret)
> 	        return ret;
> 
> 	if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> 		ret = ov6650_reg_rmw(client, REG_COMJ, COMJ_VSYNC_HIGH, 0);
> 	else if (cfg->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> 		ret = ov6650_reg_rmw(client, REG_COMJ, 0, COMJ_VSYNC_HIGH);
> 	if (ret)
> 	        return ret;
> 
> 	/*
> 	 * Update the configuration to report what is actually applied to
> 	 * the hardware.
> 	 */
> 	return ov6650_get_mbus_config(sd, pad, cfg);
> }
> 
> Sorry about this, I should have noticed that you where suggesting
> s/goto error/return ret/ and not just to return the get_mbus_config()
> return value at function end.
> 
> v8.1 of this patch soon.
> 
> Hans sorry for the churn. Do you think you can still collect this
> today, maybe with Janusz's ack if he feels to ?

Sure!

	Hans