[PATCH v4 0/9] v4l2-subdev: Introduce [g|s]et_mbus_format pad op

[PATCH v4 0/9] v4l2-subdev: Introduce [g|s]et_mbus_format pad op

[Jacopo Mondi]

Hello
   in this v4 I have addressed few minor comments received on v3, but mostly,
this new version removes g|s_mbus_config video operations instead of just
deprecating them.

I also changed a few things in the pxa and ov6650 drivers, ported to use the new
operations.

Quiting v3 cover letter:
-------------------------------------------------------------------------------
Most of the existing users are i2c camera drivers reporting a static media bus
configuration though g_mbus_config. Porting them is performed in a single
hopefully not controversial patch [2/8]

Two existing users stand-out, and they've probably been developed together:
pxa_camera and ov6650. Those have bee ported separately in single patches
with extensive change logs as their operations semantic had to change to port
them to use the new operations. Not having any of those two platforms, the
changes have been compile-tested only.

The only existing users of the s|g_mbus_config ops are now the soc_camera based
drivers currently living in staging.

The last three patches are similar to the ones posted in v2, with the exception
that they have been updated to use the V4L2_MBUS_* flags as well.
-------------------------------------------------------------------------------

Will report again the use cases I'm trying to address here:
------------------------------------------------------------------------------
Quoting:
https://patchwork.kernel.org/cover/10855919/
"The use case this series cover is the following one:
the Gen-3 R-Car boards include an ADV748x HDMI/CVBS to CSI-2 converter
connected to its CSI-2 receivers. The ADV748x chip has recently gained support
for routing both HDMI and analogue video streams through its 4 lanes TXA
transmitter, specifically to support the Ebisu board that has a single CSI-2
receiver, compared to all other Gen-3 board where the ADV748x TXes are connected
to different CSI-2 receivers, and where analogue video is streamed out from the
ADV748x single lane TXB transmitter.
To properly support transmission of analogue video through TXA, the number of
data lanes shall be dynamically reduced to 1, in order to comply with the MIPI
CSI-2 minimum clock frequency requirements"

During the discussion of the RFC, Dave reported another use case for media
bus parameter negotiation on his platform:
https://patchwork.kernel.org/patch/10855923/#22569149

Hyun is now using this series to configure GMSL devices.
------------------------------------------------------------------------------

Thanks
   j

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 (9):
  media: v4l2-subdv: 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
  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 |  61 ++++++-
 drivers/staging/media/imx/TODO              |   4 +
 include/media/v4l2-subdev.h                 |  37 ++--
 15 files changed, 263 insertions(+), 196 deletions(-)

--
2.27.0

[PATCH v4 1/9] media: v4l2-subdv: 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..90d9dfa92cf0 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 is allowed to 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 is allowed to 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

Re: [PATCH v4 1/9] media: v4l2-subdv: Introduce [get|set]_mbus_config pad ops

[Sakari Ailus]

Hi Jacopo,

s/subd\Kv/ev/ in the subject.

On Thu, Jun 11, 2020 at 06:16:43PM +0200, 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..90d9dfa92cf0 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 is allowed to fail if the pad index it has been
> + *		     called on is not valid.

Shouldn't it always fail in that case? I.e. s/is allowed to/shall/ .

> + *
> + * @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 is allowed to 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);
>  };
>  
>  /**

-- 
Kind regards,

Sakari Ailus

[PATCH v4 2/9] 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 482609665305..0ef5af026d09 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 eb39cf5ea089..53b344a64983 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);
 
@@ -1719,7 +1720,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 = {
@@ -1737,6 +1737,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 v4 3/9] 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, 39 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 91906b94f978..efc798bac3a4 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -921,46 +921,68 @@ 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
+		   |  (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);
 
+error:
+	/*
+	 * Update the configuration to report what is actually applied to
+	 * the hardware.
+	 */
+	ov6650_get_mbus_config(sd, pad, cfg);
+
 	return ret;
 }
 
@@ -968,8 +990,6 @@ 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 +998,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

Re: [PATCH v4 3/9] media: i2c: ov6650: Use new [get|set]_mbus_config ops

[kernel test robot]

Hi Jacopo,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jacopo-Mondi/v4l2-subdev-Introduce-g-s-et_mbus_format-pad-op/20200612-001600
base:   git://linuxtv.org/media_tree.git master
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


cppcheck warnings: (new ones prefixed by >>)

>> drivers/media/i2c/ov6650.c:941:34: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
        |  (comj & COMJ_VSYNC_HIGH  ? V4L2_MBUS_VSYNC_ACTIVE_HIGH
                                    ^
   drivers/media/i2c/ov6650.c:943:34: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
        |  (comf & COMF_HREF_LOW    ? V4L2_MBUS_HSYNC_ACTIVE_LOW
                                    ^
   drivers/media/i2c/ov6650.c:945:34: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
        |  (comj & COMJ_PCLK_RISING ? V4L2_MBUS_PCLK_SAMPLE_RISING
                                    ^
--
   In file included from fs/nfsd/trace.c:
>> fs/nfsd/trace.h:175:0: warning: There is an unknown macro here somewhere. Configuration is required. If DECLARE_EVENT_CLASS is a macro then please configure it. [unknownMacro]
   
   ^
>> fs/nfsd/nfsctl.c:1277:7: warning: Opposite inner 'if' condition leads to a dead code block. [oppositeInnerCondition]
     if (!files->name)
         ^
   fs/nfsd/nfsctl.c:1276:19: note: outer condition: files->name
    for (i = 0; files->name && files->name[0]; i++, files++) {
                     ^
   fs/nfsd/nfsctl.c:1277:7: note: opposite inner condition: !files->name
     if (!files->name)
         ^
>> fs/nfsd/nfsctl.c:524:5: warning: Variable 'rv' is reassigned a value before the old one has been used. [redundantAssignment]
    rv = nfsd_get_nrthreads(npools, nthreads, net);
       ^
   fs/nfsd/nfsctl.c:504:5: note: Variable 'rv' is reassigned a value before the old one has been used.
    rv = -ENOMEM;
       ^
   fs/nfsd/nfsctl.c:524:5: note: Variable 'rv' is reassigned a value before the old one has been used.
    rv = nfsd_get_nrthreads(npools, nthreads, net);
       ^
>> fs/nfsd/nfsctl.c:1201:6: warning: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment]
    ret = __nfsd_mkdir(d_inode(parent), dentry, S_IFDIR | 0600, ncl);
        ^
   fs/nfsd/nfsctl.c:1195:0: note: Variable 'ret' is reassigned a value before the old one has been used.
    int ret = -ENOMEM;
   ^
   fs/nfsd/nfsctl.c:1201:6: note: Variable 'ret' is reassigned a value before the old one has been used.
    ret = __nfsd_mkdir(d_inode(parent), dentry, S_IFDIR | 0600, ncl);
        ^

vim +941 drivers/media/i2c/ov6650.c

   922	
   923	/* Request bus settings on camera side */
   924	static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
   925					  unsigned int pad,
   926					  struct v4l2_mbus_config *cfg)
   927	{
   928		struct i2c_client *client = v4l2_get_subdevdata(sd);
   929		u8 comj, comf;
   930		int ret;
   931	
   932		ret = ov6650_reg_read(client, REG_COMJ, &comj);
   933		if (ret)
   934			return ret;
   935	
   936		ret = ov6650_reg_read(client, REG_COMF, &comf);
   937		if (ret)
   938			return ret;
   939	
   940		cfg->flags = V4L2_MBUS_MASTER
 > 941			   |  (comj & COMJ_VSYNC_HIGH  ? V4L2_MBUS_VSYNC_ACTIVE_HIGH
   942						       : V4L2_MBUS_VSYNC_ACTIVE_LOW)
   943			   |  (comf & COMF_HREF_LOW    ? V4L2_MBUS_HSYNC_ACTIVE_LOW
   944						       : V4L2_MBUS_HSYNC_ACTIVE_HIGH)
   945			   |  (comj & COMJ_PCLK_RISING ? V4L2_MBUS_PCLK_SAMPLE_RISING
   946						       : V4L2_MBUS_PCLK_SAMPLE_FALLING);
   947		cfg->type = V4L2_MBUS_PARALLEL;
   948	
   949		return 0;
   950	}
   951	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v4 3/9] media: i2c: ov6650: Use new [get|set]_mbus_config ops

[kernel test robot]

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

Hi Jacopo,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jacopo-Mondi/v4l2-subdev-Introduce-g-s-et_mbus_format-pad-op/20200612-001600
base:   git://linuxtv.org/media_tree.git master
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


cppcheck warnings: (new ones prefixed by >>)

>> drivers/media/i2c/ov6650.c:941:34: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
        |  (comj & COMJ_VSYNC_HIGH  ? V4L2_MBUS_VSYNC_ACTIVE_HIGH
                                    ^
   drivers/media/i2c/ov6650.c:943:34: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
        |  (comf & COMF_HREF_LOW    ? V4L2_MBUS_HSYNC_ACTIVE_LOW
                                    ^
   drivers/media/i2c/ov6650.c:945:34: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
        |  (comj & COMJ_PCLK_RISING ? V4L2_MBUS_PCLK_SAMPLE_RISING
                                    ^
--
   In file included from fs/nfsd/trace.c:
>> fs/nfsd/trace.h:175:0: warning: There is an unknown macro here somewhere. Configuration is required. If DECLARE_EVENT_CLASS is a macro then please configure it. [unknownMacro]
   
   ^
>> fs/nfsd/nfsctl.c:1277:7: warning: Opposite inner 'if' condition leads to a dead code block. [oppositeInnerCondition]
     if (!files->name)
         ^
   fs/nfsd/nfsctl.c:1276:19: note: outer condition: files->name
    for (i = 0; files->name && files->name[0]; i++, files++) {
                     ^
   fs/nfsd/nfsctl.c:1277:7: note: opposite inner condition: !files->name
     if (!files->name)
         ^
>> fs/nfsd/nfsctl.c:524:5: warning: Variable 'rv' is reassigned a value before the old one has been used. [redundantAssignment]
    rv = nfsd_get_nrthreads(npools, nthreads, net);
       ^
   fs/nfsd/nfsctl.c:504:5: note: Variable 'rv' is reassigned a value before the old one has been used.
    rv = -ENOMEM;
       ^
   fs/nfsd/nfsctl.c:524:5: note: Variable 'rv' is reassigned a value before the old one has been used.
    rv = nfsd_get_nrthreads(npools, nthreads, net);
       ^
>> fs/nfsd/nfsctl.c:1201:6: warning: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment]
    ret = __nfsd_mkdir(d_inode(parent), dentry, S_IFDIR | 0600, ncl);
        ^
   fs/nfsd/nfsctl.c:1195:0: note: Variable 'ret' is reassigned a value before the old one has been used.
    int ret = -ENOMEM;
   ^
   fs/nfsd/nfsctl.c:1201:6: note: Variable 'ret' is reassigned a value before the old one has been used.
    ret = __nfsd_mkdir(d_inode(parent), dentry, S_IFDIR | 0600, ncl);
        ^

vim +941 drivers/media/i2c/ov6650.c

   922	
   923	/* Request bus settings on camera side */
   924	static int ov6650_get_mbus_config(struct v4l2_subdev *sd,
   925					  unsigned int pad,
   926					  struct v4l2_mbus_config *cfg)
   927	{
   928		struct i2c_client *client = v4l2_get_subdevdata(sd);
   929		u8 comj, comf;
   930		int ret;
   931	
   932		ret = ov6650_reg_read(client, REG_COMJ, &comj);
   933		if (ret)
   934			return ret;
   935	
   936		ret = ov6650_reg_read(client, REG_COMF, &comf);
   937		if (ret)
   938			return ret;
   939	
   940		cfg->flags = V4L2_MBUS_MASTER
 > 941			   |  (comj & COMJ_VSYNC_HIGH  ? V4L2_MBUS_VSYNC_ACTIVE_HIGH
   942						       : V4L2_MBUS_VSYNC_ACTIVE_LOW)
   943			   |  (comf & COMF_HREF_LOW    ? V4L2_MBUS_HSYNC_ACTIVE_LOW
   944						       : V4L2_MBUS_HSYNC_ACTIVE_HIGH)
   945			   |  (comj & COMJ_PCLK_RISING ? V4L2_MBUS_PCLK_SAMPLE_RISING
   946						       : V4L2_MBUS_PCLK_SAMPLE_FALLING);
   947		cfg->type = V4L2_MBUS_PARALLEL;
   948	
   949		return 0;
   950	}
   951	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[PATCH v4 4/9] 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 3c5fe737d36f..3a2cd28178da 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

Re: [PATCH v4 4/9] media: pxa_camera: Use the new set_mbus_config op

[Jacopo Mondi]

Hi Robert,

On Thu, Jun 11, 2020 at 06:16:46PM +0200, Jacopo Mondi wrote:
> 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.

Are you still looking after the pxa_camera driver ? I failed (multiple
times) to cc you in this series, but I've seen you replying to other
patches for this device. Are you intrested to have a look here?

Thanks
  j

>
> 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 3c5fe737d36f..3a2cd28178da 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 v4 5/9] 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 90d9dfa92cf0..31986fa0b177 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 v4 6/9] 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 v4 7/9] 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
the two video format 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: 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

Re: [PATCH v4 7/9] media: i2c: adv748x: Adjust TXA data lanes number

[Kieran Bingham]

Hi Jacopo,

On 11/06/2020 17:16, Jacopo Mondi wrote:
> 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
> the two video format produced by the SD-Core (480i and 576i) generate a

s/the/that the/
s/format/formats/

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

Shouldn't tx->active_lanes still be set here though?

Ah, no - I see it's set as a default when parsed in
adv748x_parse_csi2_lanes().

Ok - that's fine by me then.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

>  			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;
> 

-- 
Regards
--
Kieran

[PATCH v4 8/9] 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: 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

Re: [PATCH v4 8/9] media: i2c: adv748x: Implement get_mbus_config

[Kieran Bingham]

Hi Jacopo,

On 11/06/2020 17:16, Jacopo Mondi wrote:
> 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: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 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,
>  };
>  
>  /* -----------------------------------------------------------------------------
> 

-- 
Regards
--
Kieran

[PATCH v4 9/9] 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.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
 1 file changed, 58 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 151e6a90c5fb..11769f004fd8 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -363,6 +363,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;
 
@@ -371,6 +372,7 @@ struct rcar_csi2 {
 
 	unsigned short lanes;
 	unsigned char lane_swap[4];
+	unsigned short active_lanes;
 };
 
 static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
@@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
 
 	/* 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 << priv->active_lanes) - 1;
 
 		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
 		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
@@ -471,11 +473,57 @@ 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, priv->active_lanes * 1000000);
 
 	return mbps;
 }
 
+static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
+{
+	struct v4l2_mbus_config mbus_config = { 0 };
+	unsigned int num_lanes = (-1U);
+	int ret;
+
+	priv->active_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;
+	}
+
+	priv->active_lanes = num_lanes;
+
+	return 0;
+}
+
 static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 {
 	const struct rcar_csi2_format *format;
@@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 	/* Code is validated in set_fmt. */
 	format = rcsi2_code_to_fmt(priv->mf.code);
 
+	/* Get the remote mbus config to get the number of enabled lanes. */
+	ret = rcsi2_config_active_lanes(priv);
+	if (ret)
+		return ret;
+
 	/*
 	 * Enable all supported CSI-2 channels with virtual channel and
 	 * data type matching.
@@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 	}
 
 	phycnt = PHYCNT_ENABLECLK;
-	phycnt |= (1 << priv->lanes) - 1;
+	phycnt |= (1 << priv->active_lanes) - 1;
 
 	mbps = rcsi2_calc_mbps(priv, format->bpp);
 	if (mbps < 0)
@@ -748,6 +801,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);
 
@@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
 			priv->lanes);
 		return -EINVAL;
 	}
+	priv->active_lanes = priv->lanes;
 
 	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
 		priv->lane_swap[i] = i < priv->lanes ?
-- 
2.27.0

Re: [PATCH v4 9/9] media: rcar-csi2: Negotiate data lanes number

[Sakari Ailus]

Hi Jacopo,

On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote:
> 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.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
>  1 file changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 151e6a90c5fb..11769f004fd8 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -363,6 +363,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;
>  
> @@ -371,6 +372,7 @@ struct rcar_csi2 {
>  
>  	unsigned short lanes;
>  	unsigned char lane_swap[4];
> +	unsigned short active_lanes;

Do you need this? I.e. should you not always request this from the
transmitter device?

>  };
>  
>  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>  
>  	/* 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 << priv->active_lanes) - 1;
>  
>  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
>  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> @@ -471,11 +473,57 @@ 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, priv->active_lanes * 1000000);
>  
>  	return mbps;
>  }
>  
> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
> +{
> +	struct v4l2_mbus_config mbus_config = { 0 };
> +	unsigned int num_lanes = (-1U);
> +	int ret;
> +
> +	priv->active_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;

This is the downside of using flags... Anyway, I guess this is certainly
fine now.

> +
> +	if (num_lanes > priv->lanes) {
> +		dev_err(priv->dev,
> +			"Unsupported mbus config: too many data lanes %u\n",
> +			num_lanes);
> +		return -EINVAL;
> +	}
> +
> +	priv->active_lanes = num_lanes;
> +
> +	return 0;
> +}
> +
>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  {
>  	const struct rcar_csi2_format *format;
> @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	/* Code is validated in set_fmt. */
>  	format = rcsi2_code_to_fmt(priv->mf.code);
>  
> +	/* Get the remote mbus config to get the number of enabled lanes. */
> +	ret = rcsi2_config_active_lanes(priv);
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * Enable all supported CSI-2 channels with virtual channel and
>  	 * data type matching.
> @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	}
>  
>  	phycnt = PHYCNT_ENABLECLK;
> -	phycnt |= (1 << priv->lanes) - 1;
> +	phycnt |= (1 << priv->active_lanes) - 1;
>  
>  	mbps = rcsi2_calc_mbps(priv, format->bpp);
>  	if (mbps < 0)
> @@ -748,6 +801,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);
>  
> @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  			priv->lanes);
>  		return -EINVAL;
>  	}
> +	priv->active_lanes = priv->lanes;
>  
>  	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
>  		priv->lane_swap[i] = i < priv->lanes ?

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 9/9] media: rcar-csi2: Negotiate data lanes number

[Kieran Bingham]

Hi Jacopo, Sakari,

On 15/06/2020 09:34, Sakari Ailus wrote:
> Hi Jacopo,
> 
> On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote:
>> 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.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> ---
>>  drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
>>  1 file changed, 58 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
>> index 151e6a90c5fb..11769f004fd8 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
>> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
>> @@ -363,6 +363,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;
>>  
>> @@ -371,6 +372,7 @@ struct rcar_csi2 {
>>  
>>  	unsigned short lanes;
>>  	unsigned char lane_swap[4];
>> +	unsigned short active_lanes;
> 
> Do you need this? I.e. should you not always request this from the
> transmitter device?
> 
>>  };
>>  
>>  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
>> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>>  
>>  	/* 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 << priv->active_lanes) - 1;
>>  
>>  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
>>  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
>> @@ -471,11 +473,57 @@ 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, priv->active_lanes * 1000000);
>>  
>>  	return mbps;
>>  }
>>  
>> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
>> +{
>> +	struct v4l2_mbus_config mbus_config = { 0 };
>> +	unsigned int num_lanes = (-1U);
>> +	int ret;
>> +
>> +	priv->active_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;
> 
> This is the downside of using flags... Anyway, I guess this is certainly
> fine now.

Given that the mbus_config has a .type, can't we easily extend
  struct v4l2_mbus_config

to contain a type specific union ?

Or is that something to do on top of this series instead?

I mean, it could even go after the flags, and keep the flags as a common
attribute - so that no other changes are needed to other components...

--
Kieran

> 
>> +
>> +	if (num_lanes > priv->lanes) {
>> +		dev_err(priv->dev,
>> +			"Unsupported mbus config: too many data lanes %u\n",
>> +			num_lanes);
>> +		return -EINVAL;
>> +	}
>> +
>> +	priv->active_lanes = num_lanes;
>> +
>> +	return 0;
>> +}
>> +
>>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>  {
>>  	const struct rcar_csi2_format *format;
>> @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>  	/* Code is validated in set_fmt. */
>>  	format = rcsi2_code_to_fmt(priv->mf.code);
>>  
>> +	/* Get the remote mbus config to get the number of enabled lanes. */
>> +	ret = rcsi2_config_active_lanes(priv);
>> +	if (ret)
>> +		return ret;
>> +
>>  	/*
>>  	 * Enable all supported CSI-2 channels with virtual channel and
>>  	 * data type matching.
>> @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>  	}
>>  
>>  	phycnt = PHYCNT_ENABLECLK;
>> -	phycnt |= (1 << priv->lanes) - 1;
>> +	phycnt |= (1 << priv->active_lanes) - 1;
>>  
>>  	mbps = rcsi2_calc_mbps(priv, format->bpp);
>>  	if (mbps < 0)
>> @@ -748,6 +801,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);
>>  
>> @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>>  			priv->lanes);
>>  		return -EINVAL;
>>  	}
>> +	priv->active_lanes = priv->lanes;
>>  
>>  	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
>>  		priv->lane_swap[i] = i < priv->lanes ?
> 

-- 
Regards
--
Kieran

Re: [PATCH v4 9/9] media: rcar-csi2: Negotiate data lanes number

[Jacopo Mondi]

Hi Sakari,

On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote:
> > 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.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
> >  1 file changed, 58 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 151e6a90c5fb..11769f004fd8 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -363,6 +363,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;
> >
> > @@ -371,6 +372,7 @@ struct rcar_csi2 {
> >
> >  	unsigned short lanes;
> >  	unsigned char lane_swap[4];
> > +	unsigned short active_lanes;
>
> Do you need this? I.e. should you not always request this from the
> transmitter device?

It's actually the other way around. The receiver queries the
transmitter to know how many data lanes it intends to use and adjusts
its configuration to accommodate it.

>
> >  };
> >
> >  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> > @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> >
> >  	/* 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 << priv->active_lanes) - 1;
> >
> >  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
> >  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> > @@ -471,11 +473,57 @@ 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, priv->active_lanes * 1000000);
> >
> >  	return mbps;
> >  }
> >
> > +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
> > +{
> > +	struct v4l2_mbus_config mbus_config = { 0 };
> > +	unsigned int num_lanes = (-1U);
> > +	int ret;
> > +
> > +	priv->active_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;
>
> This is the downside of using flags... Anyway, I guess this is certainly
> fine now.
>

Let's change this on top, if we like to (and I would like to :)

Thanks
  j

> > +
> > +	if (num_lanes > priv->lanes) {
> > +		dev_err(priv->dev,
> > +			"Unsupported mbus config: too many data lanes %u\n",
> > +			num_lanes);
> > +		return -EINVAL;
> > +	}
> > +
> > +	priv->active_lanes = num_lanes;
> > +
> > +	return 0;
> > +}
> > +
> >  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  {
> >  	const struct rcar_csi2_format *format;
> > @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  	/* Code is validated in set_fmt. */
> >  	format = rcsi2_code_to_fmt(priv->mf.code);
> >
> > +	/* Get the remote mbus config to get the number of enabled lanes. */
> > +	ret = rcsi2_config_active_lanes(priv);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/*
> >  	 * Enable all supported CSI-2 channels with virtual channel and
> >  	 * data type matching.
> > @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >  	}
> >
> >  	phycnt = PHYCNT_ENABLECLK;
> > -	phycnt |= (1 << priv->lanes) - 1;
> > +	phycnt |= (1 << priv->active_lanes) - 1;
> >
> >  	mbps = rcsi2_calc_mbps(priv, format->bpp);
> >  	if (mbps < 0)
> > @@ -748,6 +801,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);
> >
> > @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> >  			priv->lanes);
> >  		return -EINVAL;
> >  	}
> > +	priv->active_lanes = priv->lanes;
> >
> >  	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
> >  		priv->lane_swap[i] = i < priv->lanes ?
>
> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH v4 9/9] media: rcar-csi2: Negotiate data lanes number

[Kieran Bingham]

Hi Jacopo,

On 15/06/2020 09:43, Jacopo Mondi wrote:
> Hi Sakari,
> 
> On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote:
>> Hi Jacopo,
>>
>> On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote:
>>> 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.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
>>>  1 file changed, 58 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
>>> index 151e6a90c5fb..11769f004fd8 100644
>>> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
>>> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
>>> @@ -363,6 +363,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;
>>>
>>> @@ -371,6 +372,7 @@ struct rcar_csi2 {
>>>
>>>  	unsigned short lanes;
>>>  	unsigned char lane_swap[4];
>>> +	unsigned short active_lanes;
>>
>> Do you need this? I.e. should you not always request this from the
>> transmitter device?
> 
> It's actually the other way around. The receiver queries the
> transmitter to know how many data lanes it intends to use and adjusts
> its configuration to accommodate it.
> 
>>
>>>  };
>>>
>>>  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
>>> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>>>
>>>  	/* 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 << priv->active_lanes) - 1;
>>>
>>>  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
>>>  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
>>> @@ -471,11 +473,57 @@ 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, priv->active_lanes * 1000000);
>>>
>>>  	return mbps;
>>>  }
>>>
>>> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
>>> +{
>>> +	struct v4l2_mbus_config mbus_config = { 0 };
>>> +	unsigned int num_lanes = (-1U);
>>> +	int ret;
>>> +
>>> +	priv->active_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;
>>
>> This is the downside of using flags... Anyway, I guess this is certainly
>> fine now.
>>
> 
> Let's change this on top, if we like to (and I would like to :)

That answers my question then ;-)

--
Kieran


> 
> Thanks
>   j
> 
>>> +
>>> +	if (num_lanes > priv->lanes) {
>>> +		dev_err(priv->dev,
>>> +			"Unsupported mbus config: too many data lanes %u\n",
>>> +			num_lanes);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	priv->active_lanes = num_lanes;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>>  {
>>>  	const struct rcar_csi2_format *format;
>>> @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>>  	/* Code is validated in set_fmt. */
>>>  	format = rcsi2_code_to_fmt(priv->mf.code);
>>>
>>> +	/* Get the remote mbus config to get the number of enabled lanes. */
>>> +	ret = rcsi2_config_active_lanes(priv);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>  	/*
>>>  	 * Enable all supported CSI-2 channels with virtual channel and
>>>  	 * data type matching.
>>> @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>>  	}
>>>
>>>  	phycnt = PHYCNT_ENABLECLK;
>>> -	phycnt |= (1 << priv->lanes) - 1;
>>> +	phycnt |= (1 << priv->active_lanes) - 1;
>>>
>>>  	mbps = rcsi2_calc_mbps(priv, format->bpp);
>>>  	if (mbps < 0)
>>> @@ -748,6 +801,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);
>>>
>>> @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>>>  			priv->lanes);
>>>  		return -EINVAL;
>>>  	}
>>> +	priv->active_lanes = priv->lanes;
>>>
>>>  	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
>>>  		priv->lane_swap[i] = i < priv->lanes ?
>>
>> --
>> Kind regards,
>>
>> Sakari Ailus

-- 
Regards
--
Kieran

Re: [PATCH v4 9/9] media: rcar-csi2: Negotiate data lanes number

[Jacopo Mondi]

Hi Kieran,

On Mon, Jun 15, 2020 at 09:40:42AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 15/06/2020 09:43, Jacopo Mondi wrote:
> > Hi Sakari,
> >
> > On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote:
> >> Hi Jacopo,
> >>
> >> On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote:
> >>> 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.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>>  drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
> >>>  1 file changed, 58 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >>> index 151e6a90c5fb..11769f004fd8 100644
> >>> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >>> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >>> @@ -363,6 +363,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;
> >>>
> >>> @@ -371,6 +372,7 @@ struct rcar_csi2 {
> >>>
> >>>  	unsigned short lanes;
> >>>  	unsigned char lane_swap[4];
> >>> +	unsigned short active_lanes;
> >>
> >> Do you need this? I.e. should you not always request this from the
> >> transmitter device?
> >
> > It's actually the other way around. The receiver queries the
> > transmitter to know how many data lanes it intends to use and adjusts
> > its configuration to accommodate it.
> >
> >>
> >>>  };
> >>>
> >>>  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> >>> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> >>>
> >>>  	/* 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 << priv->active_lanes) - 1;
> >>>
> >>>  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
> >>>  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> >>> @@ -471,11 +473,57 @@ 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, priv->active_lanes * 1000000);
> >>>
> >>>  	return mbps;
> >>>  }
> >>>
> >>> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
> >>> +{
> >>> +	struct v4l2_mbus_config mbus_config = { 0 };
> >>> +	unsigned int num_lanes = (-1U);
> >>> +	int ret;
> >>> +
> >>> +	priv->active_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;
> >>
> >> This is the downside of using flags... Anyway, I guess this is certainly
> >> fine now.
> >>
> >
> > Let's change this on top, if we like to (and I would like to :)
>
> That answers my question then ;-)
>

There's been a discussion on one of the first version of the series
where I tried replacing flags and introduced a union of structures
with fields, most likely similar to what you suggested.

Based on the received comments I decided to use the existing
V4L2_MBUS_* flags to ease the replacement of the video ops
g|s_mbus_config() to minimize the changes. We could then on top
replace flags.

Thanks
  j

> --
> Kieran
>
>
> >
> > Thanks
> >   j
> >
> >>> +
> >>> +	if (num_lanes > priv->lanes) {
> >>> +		dev_err(priv->dev,
> >>> +			"Unsupported mbus config: too many data lanes %u\n",
> >>> +			num_lanes);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	priv->active_lanes = num_lanes;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>>  {
> >>>  	const struct rcar_csi2_format *format;
> >>> @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>>  	/* Code is validated in set_fmt. */
> >>>  	format = rcsi2_code_to_fmt(priv->mf.code);
> >>>
> >>> +	/* Get the remote mbus config to get the number of enabled lanes. */
> >>> +	ret = rcsi2_config_active_lanes(priv);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>>  	/*
> >>>  	 * Enable all supported CSI-2 channels with virtual channel and
> >>>  	 * data type matching.
> >>> @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
> >>>  	}
> >>>
> >>>  	phycnt = PHYCNT_ENABLECLK;
> >>> -	phycnt |= (1 << priv->lanes) - 1;
> >>> +	phycnt |= (1 << priv->active_lanes) - 1;
> >>>
> >>>  	mbps = rcsi2_calc_mbps(priv, format->bpp);
> >>>  	if (mbps < 0)
> >>> @@ -748,6 +801,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);
> >>>
> >>> @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
> >>>  			priv->lanes);
> >>>  		return -EINVAL;
> >>>  	}
> >>> +	priv->active_lanes = priv->lanes;
> >>>
> >>>  	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
> >>>  		priv->lane_swap[i] = i < priv->lanes ?
> >>
> >> --
> >> Kind regards,
> >>
> >> Sakari Ailus
>
> --
> Regards
> --
> Kieran

Re: [PATCH v4 9/9] media: rcar-csi2: Negotiate data lanes number

[Kieran Bingham]

Hi Jacopo,

On 15/06/2020 09:47, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Mon, Jun 15, 2020 at 09:40:42AM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 15/06/2020 09:43, Jacopo Mondi wrote:
>>> Hi Sakari,
>>>
>>> On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote:
>>>> Hi Jacopo,
>>>>
>>>> On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote:
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>>> ---
>>>>>  drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
>>>>>  1 file changed, 58 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
>>>>> index 151e6a90c5fb..11769f004fd8 100644
>>>>> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
>>>>> @@ -363,6 +363,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;
>>>>>
>>>>> @@ -371,6 +372,7 @@ struct rcar_csi2 {
>>>>>
>>>>>  	unsigned short lanes;
>>>>>  	unsigned char lane_swap[4];
>>>>> +	unsigned short active_lanes;
>>>>
>>>> Do you need this? I.e. should you not always request this from the
>>>> transmitter device?
>>>
>>> It's actually the other way around. The receiver queries the
>>> transmitter to know how many data lanes it intends to use and adjusts
>>> its configuration to accommodate it.
>>>
>>>>
>>>>>  };
>>>>>
>>>>>  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
>>>>> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>>>>>
>>>>>  	/* 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 << priv->active_lanes) - 1;
>>>>>
>>>>>  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
>>>>>  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
>>>>> @@ -471,11 +473,57 @@ 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, priv->active_lanes * 1000000);
>>>>>
>>>>>  	return mbps;
>>>>>  }
>>>>>
>>>>> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
>>>>> +{
>>>>> +	struct v4l2_mbus_config mbus_config = { 0 };
>>>>> +	unsigned int num_lanes = (-1U);
>>>>> +	int ret;
>>>>> +
>>>>> +	priv->active_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;
>>>>
>>>> This is the downside of using flags... Anyway, I guess this is certainly
>>>> fine now.
>>>>
>>>
>>> Let's change this on top, if we like to (and I would like to :)
>>
>> That answers my question then ;-)
>>
> 
> There's been a discussion on one of the first version of the series
> where I tried replacing flags and introduced a union of structures
> with fields, most likely similar to what you suggested.
> 
> Based on the received comments I decided to use the existing
> V4L2_MBUS_* flags to ease the replacement of the video ops
> g|s_mbus_config() to minimize the changes. We could then on top
> replace flags.

That sound fine to me.

When I first saw this patch, I thought that this series was introducing
the V4L2_MBUS_CSI2_4_LANE flag types, but I see they are pre-existing.

I think it's perfectly reasonable to use the existing infrastructure for
this series, and adapt after.

--
Kieran



> 
> Thanks
>   j
> 
>> --
>> Kieran
>>
>>
>>>
>>> Thanks
>>>   j
>>>
>>>>> +
>>>>> +	if (num_lanes > priv->lanes) {
>>>>> +		dev_err(priv->dev,
>>>>> +			"Unsupported mbus config: too many data lanes %u\n",
>>>>> +			num_lanes);
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	priv->active_lanes = num_lanes;
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>>>>  {
>>>>>  	const struct rcar_csi2_format *format;
>>>>> @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>>>>  	/* Code is validated in set_fmt. */
>>>>>  	format = rcsi2_code_to_fmt(priv->mf.code);
>>>>>
>>>>> +	/* Get the remote mbus config to get the number of enabled lanes. */
>>>>> +	ret = rcsi2_config_active_lanes(priv);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>>  	/*
>>>>>  	 * Enable all supported CSI-2 channels with virtual channel and
>>>>>  	 * data type matching.
>>>>> @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>>>>  	}
>>>>>
>>>>>  	phycnt = PHYCNT_ENABLECLK;
>>>>> -	phycnt |= (1 << priv->lanes) - 1;
>>>>> +	phycnt |= (1 << priv->active_lanes) - 1;
>>>>>
>>>>>  	mbps = rcsi2_calc_mbps(priv, format->bpp);
>>>>>  	if (mbps < 0)
>>>>> @@ -748,6 +801,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);
>>>>>
>>>>> @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>>>>>  			priv->lanes);
>>>>>  		return -EINVAL;
>>>>>  	}
>>>>> +	priv->active_lanes = priv->lanes;
>>>>>
>>>>>  	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
>>>>>  		priv->lane_swap[i] = i < priv->lanes ?
>>>>
>>>> --
>>>> Kind regards,
>>>>
>>>> Sakari Ailus
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran

Re: [PATCH v4 9/9] media: rcar-csi2: Negotiate data lanes number

[Sakari Ailus]

Jacopo,

On Mon, Jun 15, 2020 at 10:43:06AM +0200, Jacopo Mondi wrote:
> Hi Sakari,
> 
> On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote:
> > > 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.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
> > >  1 file changed, 58 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > index 151e6a90c5fb..11769f004fd8 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > @@ -363,6 +363,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;
> > >
> > > @@ -371,6 +372,7 @@ struct rcar_csi2 {
> > >
> > >  	unsigned short lanes;
> > >  	unsigned char lane_swap[4];
> > > +	unsigned short active_lanes;
> >
> > Do you need this? I.e. should you not always request this from the
> > transmitter device?
> 
> It's actually the other way around. The receiver queries the
> transmitter to know how many data lanes it intends to use and adjusts
> its configuration to accommodate it.

I think we didn't have a different view on the process. But you basically
need this when you're starting streaming, so why is the information stored
here?

> 
> >
> > >  };
> > >
> > >  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> > > @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> > >
> > >  	/* 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 << priv->active_lanes) - 1;
> > >
> > >  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
> > >  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> > > @@ -471,11 +473,57 @@ 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, priv->active_lanes * 1000000);
> > >
> > >  	return mbps;
> > >  }
> > >
> > > +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
> > > +{
> > > +	struct v4l2_mbus_config mbus_config = { 0 };
> > > +	unsigned int num_lanes = (-1U);
> > > +	int ret;
> > > +
> > > +	priv->active_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;
> >
> > This is the downside of using flags... Anyway, I guess this is certainly
> > fine now.
> >
> 
> Let's change this on top, if we like to (and I would like to :)

Agreed.

-- 
Sakari Ailus

Re: [PATCH v4 9/9] media: rcar-csi2: Negotiate data lanes number

[Jacopo Mondi]

Hi Sakari,

On Mon, Jun 15, 2020 at 11:53:35AM +0300, Sakari Ailus wrote:
> Jacopo,
>
> On Mon, Jun 15, 2020 at 10:43:06AM +0200, Jacopo Mondi wrote:
> > Hi Sakari,
> >
> > On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote:
> > > Hi Jacopo,
> > >
> > > On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote:
> > > > 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.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
> > > >  1 file changed, 58 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > index 151e6a90c5fb..11769f004fd8 100644
> > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > @@ -363,6 +363,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;
> > > >
> > > > @@ -371,6 +372,7 @@ struct rcar_csi2 {
> > > >
> > > >  	unsigned short lanes;
> > > >  	unsigned char lane_swap[4];
> > > > +	unsigned short active_lanes;
> > >
> > > Do you need this? I.e. should you not always request this from the
> > > transmitter device?
> >
> > It's actually the other way around. The receiver queries the
> > transmitter to know how many data lanes it intends to use and adjusts
> > its configuration to accommodate it.
>
> I think we didn't have a different view on the process. But you basically
> need this when you're starting streaming, so why is the information stored
> here?
>

Still not sure I got your question, so pardon me if the reply is
something obvious to you already, and I'm missing the real point.

But, basically what happens is there at probe time the number of
'available' data lanes is parsed from firmware and stored in
priv->lanes. At stream start time, the remote end gets queried and the
number of 'active' lanes adjusted according to what it's reported.

Then, during the whole CSI-2 interface startup process, the number of
'active' lanes is used in a few different places (ie.
rcsi2_wait_phy_start() and rcsi2_calc_mbps()) so I had to store it
somewhere.

Now that I wrote that, I realize you might be wondering why a
parameter that is valid for a single streaming session is stored in
the main driver structure. This might not be optimal, but the driver
struct already contains, in example, a v4l2_mbus_framefmt entry which
is a session specific paramter as well, so I thought it was no harm
adding the number of active 'lanes' there. Is this what's bothering
you ?

Thanks
  j

> >
> > >
> > > >  };
> > > >
> > > >  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> > > > @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> > > >
> > > >  	/* 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 << priv->active_lanes) - 1;
> > > >
> > > >  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
> > > >  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> > > > @@ -471,11 +473,57 @@ 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, priv->active_lanes * 1000000);
> > > >
> > > >  	return mbps;
> > > >  }
> > > >
> > > > +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
> > > > +{
> > > > +	struct v4l2_mbus_config mbus_config = { 0 };
> > > > +	unsigned int num_lanes = (-1U);
> > > > +	int ret;
> > > > +
> > > > +	priv->active_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;
> > >
> > > This is the downside of using flags... Anyway, I guess this is certainly
> > > fine now.
> > >
> >
> > Let's change this on top, if we like to (and I would like to :)
>
> Agreed.
>
> --
> Sakari Ailus

Re: [PATCH v4 9/9] media: rcar-csi2: Negotiate data lanes number

[Sakari Ailus]

Hi Jacopo,

On Mon, Jun 15, 2020 at 11:19:49AM +0200, Jacopo Mondi wrote:
> Hi Sakari,
> 
> On Mon, Jun 15, 2020 at 11:53:35AM +0300, Sakari Ailus wrote:
> > Jacopo,
> >
> > On Mon, Jun 15, 2020 at 10:43:06AM +0200, Jacopo Mondi wrote:
> > > Hi Sakari,
> > >
> > > On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote:
> > > > Hi Jacopo,
> > > >
> > > > On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote:
> > > > > 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.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > ---
> > > > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
> > > > >  1 file changed, 58 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > > index 151e6a90c5fb..11769f004fd8 100644
> > > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > > @@ -363,6 +363,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;
> > > > >
> > > > > @@ -371,6 +372,7 @@ struct rcar_csi2 {
> > > > >
> > > > >  	unsigned short lanes;
> > > > >  	unsigned char lane_swap[4];
> > > > > +	unsigned short active_lanes;
> > > >
> > > > Do you need this? I.e. should you not always request this from the
> > > > transmitter device?
> > >
> > > It's actually the other way around. The receiver queries the
> > > transmitter to know how many data lanes it intends to use and adjusts
> > > its configuration to accommodate it.
> >
> > I think we didn't have a different view on the process. But you basically
> > need this when you're starting streaming, so why is the information stored
> > here?
> >
> 
> Still not sure I got your question, so pardon me if the reply is
> something obvious to you already, and I'm missing the real point.
> 
> But, basically what happens is there at probe time the number of
> 'available' data lanes is parsed from firmware and stored in
> priv->lanes. At stream start time, the remote end gets queried and the
> number of 'active' lanes adjusted according to what it's reported.
> 
> Then, during the whole CSI-2 interface startup process, the number of
> 'active' lanes is used in a few different places (ie.
> rcsi2_wait_phy_start() and rcsi2_calc_mbps()) so I had to store it
> somewhere.
> 
> Now that I wrote that, I realize you might be wondering why a
> parameter that is valid for a single streaming session is stored in
> the main driver structure. This might not be optimal, but the driver
> struct already contains, in example, a v4l2_mbus_framefmt entry which
> is a session specific paramter as well, so I thought it was no harm
> adding the number of active 'lanes' there. Is this what's bothering
> you ?

The frame format is needed there as that's set by the user outside the
s_stream call. The number of active lanes is not needed elsewhere, so
therefore I wouldn't store in the device context either. It can be a local
variable which you may pass to another function.

-- 
Sakari Ailus