[PATCH v2 00/13] media: Add multiplexed support to R-Car CSI-2 and GMSL

[PATCH v2 00/13] media: Add multiplexed support to R-Car CSI-2 and GMSL

[Jacopo Mondi]

Hello,
  this series is based on v9 of Tomi's
"v4l: subdev internal routing and streams":

With a few out-of-tree patches for GMSL support on top.

The series aims to
1) Plumb into Tomi's v4l2 subdev streams and routing to compare it with the
   previous implementations of multistream support
2) Add support for multiplexed streams to R-Car CSI-2 and MAX9286

For testing, I have re-proposed Niklas' patches on top of v4l2-ctl to control
routing and ported them to this last version. Support for state-based format
handling has been added on top. Two simple scripts to be deployed in vin-test
have been used to set routing and capture frames. Both are available at:
https://git.sr.ht/~jmondi_/v4l2-utils
https://git.sr.ht/~jmondi_/vin-test-multi

v2 contains changes to max9286 and R-Car CSI-2 while changes to support
routing VIN are still WIP and not included.

Tested on Eagle V3M by routing VC0 and VC1 to VIN0 and VIN3 respectively.

Thanks
   j

v1->v2:
- Rebase on Tomi's v9
- Break-out max9286 and CSI-2 mux support
- Add for_each_active_route() macro
- Add get_frame_desc to R-Car CSI-2

Jacopo Mondi (13):
  media: max9286: Add support for v4l2_subdev_state
  media: max9286: Implement set_routing
  media: max9286: Use enabled routes to calculate pixelrate
  media: max9286: Use routes to configure link order
  media: max9286: Move format to subdev state
  media: subdev: Add for_each_active_route() macro
  media: max9286: Implement get_frame_desc()
  media: rcar-csi2: Add support for v4l2_subdev_state
  media: rcar-csi2: Implement set_routing
  media: rcar-csi2: Move format to subdev state
  media: rcar-csi2: Config by using the remote frame_desc
  media: rcar-csi2: Implement .get_frame_desc()
  media: rcar-vin: Support multiplexed CSI-2 receiver

 drivers/media/i2c/ds90ub913.c               |   8 +-
 drivers/media/i2c/ds90ub953.c               |   7 +-
 drivers/media/i2c/ds90ub960.c               |   8 +-
 drivers/media/i2c/max9286.c                 | 448 ++++++++++++++------
 drivers/media/platform/rcar-vin/rcar-csi2.c | 338 ++++++++++++---
 drivers/media/platform/rcar-vin/rcar-dma.c  |   3 +-
 drivers/media/platform/ti-vpe/cal-video.c   |   9 +-
 drivers/media/v4l2-core/v4l2-subdev.c       |  18 +
 include/media/v4l2-subdev.h                 |  11 +
 9 files changed, 640 insertions(+), 210 deletions(-)

--
2.33.0

[PATCH v2 01/13] media: max9286: Add support for v4l2_subdev_state

[Jacopo Mondi]

Create and initialize the v4l2_subdev_state for the max9286 driver in
order to prepare to support routing operations and multiplexed streams.

Create the subdevice state with v4l2_subdev_init_finalize() and
implement the init_cfg() operation to guarantee the state is initialized
correctly with the default device format.

Remove the max9286_open() subdev internal operation as the format of the
file-handle state is now initialized by init_cfg().

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 90 +++++++++++++++++++++++--------------
 1 file changed, 57 insertions(+), 33 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1b92d18a1f94..5997fe40509f 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -215,6 +215,17 @@ static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
 	return container_of(sd, struct max9286_priv, sd);
 }
 
+static const struct v4l2_mbus_framefmt max9286_default_format = {
+	.width		= 1280,
+	.height		= 800,
+	.code		= MEDIA_BUS_FMT_UYVY8_1X16,
+	.colorspace	= V4L2_COLORSPACE_SRGB,
+	.field		= V4L2_FIELD_NONE,
+	.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
+	.quantization	= V4L2_QUANTIZATION_DEFAULT,
+	.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
+};
+
 /* -----------------------------------------------------------------------------
  * I2C IO
  */
@@ -822,11 +833,45 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int max9286_init_cfg(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_state *state)
+{
+	struct v4l2_subdev_route routes[MAX9286_NUM_GMSL];
+	struct max9286_priv *priv = sd_to_max9286(sd);
+	struct v4l2_subdev_krouting routing;
+	struct max9286_source *source;
+	unsigned int num_routes = 0;
+	int ret;
+
+	/* Create a route for each enable source. */
+	for_each_source(priv, source) {
+		struct v4l2_subdev_route *route = &routes[num_routes++];
+		unsigned int idx = to_index(priv, source);
+
+		route->sink_pad = idx;
+		route->sink_stream = 0;
+		route->source_pad = MAX9286_SRC_PAD;
+		route->source_stream = idx;
+		route->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE;
+	}
+
+	routing.num_routes = num_routes;
+	routing.routes = routes;
+
+	state = v4l2_subdev_validate_and_lock_state(sd, state);
+	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
+					       &max9286_default_format);
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
 static const struct v4l2_subdev_video_ops max9286_video_ops = {
 	.s_stream	= max9286_s_stream,
 };
 
 static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
+	.init_cfg	= max9286_init_cfg,
 	.enum_mbus_code = max9286_enum_mbus_code,
 	.get_fmt	= max9286_get_fmt,
 	.set_fmt	= max9286_set_fmt,
@@ -837,35 +882,6 @@ static const struct v4l2_subdev_ops max9286_subdev_ops = {
 	.pad		= &max9286_pad_ops,
 };
 
-static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
-{
-	fmt->width		= 1280;
-	fmt->height		= 800;
-	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
-	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
-	fmt->field		= V4L2_FIELD_NONE;
-	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
-	fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
-	fmt->xfer_func		= V4L2_XFER_FUNC_DEFAULT;
-}
-
-static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
-{
-	struct v4l2_mbus_framefmt *format;
-	unsigned int i;
-
-	for (i = 0; i < MAX9286_N_SINKS; i++) {
-		format = v4l2_subdev_get_try_format(subdev, fh->state, i);
-		max9286_init_format(format);
-	}
-
-	return 0;
-}
-
-static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
-	.open = max9286_open,
-};
-
 static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	switch (ctrl->id) {
@@ -897,11 +913,11 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 	/* Configure V4L2 for the MAX9286 itself */
 
 	for (i = 0; i < MAX9286_N_SINKS; i++)
-		max9286_init_format(&priv->fmt[i]);
+		priv->fmt[i] = max9286_default_format;
 
 	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
-	priv->sd.internal_ops = &max9286_subdev_internal_ops;
-	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			  V4L2_SUBDEV_FL_MULTIPLEXED;
 
 	v4l2_ctrl_handler_init(&priv->ctrls, 1);
 	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
@@ -933,14 +949,21 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 	}
 	priv->sd.fwnode = ep;
 
+	ret = v4l2_subdev_init_finalize(&priv->sd);
+	if (ret)
+		goto err_put_node;
+
 	ret = v4l2_async_register_subdev(&priv->sd);
 	if (ret < 0) {
 		dev_err(dev, "Unable to register subdevice\n");
-		goto err_put_node;
+		goto err_free_state;
 	}
 
 	return 0;
 
+err_free_state:
+	v4l2_subdev_cleanup(&priv->sd);
+
 err_put_node:
 	fwnode_handle_put(ep);
 err_async:
@@ -953,6 +976,7 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
 {
 	fwnode_handle_put(priv->sd.fwnode);
 	v4l2_async_unregister_subdev(&priv->sd);
+	v4l2_subdev_cleanup(&priv->sd);
 	max9286_v4l2_notifier_unregister(priv);
 }
 
-- 
2.33.0

[PATCH v2 02/13] media: max9286: Implement set_routing

[Jacopo Mondi]

Add the set_routing() subdev operation to allow userspace to configure
routing on the max9286 deserializer.

Implement route verification but do not take routing into consideration
when configuring the CSI-2 output and pixel rate yet.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 88 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 5997fe40509f..54b4067168df 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -833,6 +833,90 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int max9286_routing_verify(struct max9286_priv *priv,
+				  struct v4l2_subdev_krouting *routing)
+{
+	unsigned int i;
+	int ret;
+
+	ret = v4l2_routing_simple_verify(routing);
+	if (ret)
+		return ret;
+
+	/*
+	 * Make sure all routes points to the single source pad which can have
+	 * up to 4 streams. All routes shall start from a sink pad and shall not
+	 * have more than one sink stream. The GMSL link for the sink has to be
+	 * enabled.
+	 */
+	for (i = 0; i < routing->num_routes; ++i) {
+		const struct v4l2_subdev_route *route = &routing->routes[i];
+		struct max9286_source *source = &priv->sources[i];
+
+		if (route->source_pad != MAX9286_SRC_PAD ||
+		    route->source_stream > 4) {
+			dev_err(&priv->client->dev,
+				"Invalid (%u,%u) source in route %u\n",
+				route->source_pad, route->source_stream, i);
+			return -EINVAL;
+		}
+
+		if (route->sink_pad >= MAX9286_N_SINKS ||
+		    route->sink_stream != 0) {
+			dev_err(&priv->client->dev,
+				"Invalid (%u,%u) sink in route %u\n",
+				route->sink_pad, route->sink_stream, i);
+			return -EINVAL;
+		}
+
+		source = &priv->sources[route->sink_pad];
+		if (!source->fwnode) {
+			dev_err(&priv->client->dev,
+				"Cannot set route for non-active source %u\n",
+				route->sink_pad);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int _max9286_set_routing(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_krouting *routing)
+{
+	struct max9286_priv *priv = sd_to_max9286(sd);
+	int ret;
+
+	ret = max9286_routing_verify(priv, routing);
+	if (ret)
+		return ret;
+
+	/* Re-initialize the format on a routing change. */
+	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing,
+					       &max9286_default_format);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int max9286_set_routing(struct v4l2_subdev *sd,
+			       struct v4l2_subdev_state *state,
+			       enum v4l2_subdev_format_whence which,
+			       struct v4l2_subdev_krouting *routing)
+{
+	struct max9286_priv *priv = sd_to_max9286(sd);
+	unsigned int i;
+	int ret;
+
+	state = v4l2_subdev_validate_and_lock_state(sd, state);
+	ret = _max9286_set_routing(sd, state, routing);
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
 static int max9286_init_cfg(struct v4l2_subdev *sd,
 			    struct v4l2_subdev_state *state)
 {
@@ -859,8 +943,7 @@ static int max9286_init_cfg(struct v4l2_subdev *sd,
 	routing.routes = routes;
 
 	state = v4l2_subdev_validate_and_lock_state(sd, state);
-	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
-					       &max9286_default_format);
+	ret = _max9286_set_routing(sd, state, &routing);
 	v4l2_subdev_unlock_state(state);
 
 	return ret;
@@ -875,6 +958,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
 	.enum_mbus_code = max9286_enum_mbus_code,
 	.get_fmt	= max9286_get_fmt,
 	.set_fmt	= max9286_set_fmt,
+	.set_routing	= max9286_set_routing,
 };
 
 static const struct v4l2_subdev_ops max9286_subdev_ops = {
-- 
2.33.0

[PATCH v2 03/13] media: max9286: Use enabled routes to calculate pixelrate

[Jacopo Mondi]

Now that the device supports routing, use the enabled routes instead
of the connected sources to compute the output pixel rate.

To do so, update the route_mask after a set_routing() call with the
ACTIVE format, and re-calculate the pixel rate just after.

At the same time, start and stop only the enabled routes at s_stream()
time.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 63 ++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 54b4067168df..2e635179aec0 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -189,8 +189,11 @@ struct max9286_priv {
 	struct v4l2_async_notifier notifier;
 };
 
+#define to_index(priv, source) ((source) - &(priv)->sources[0])
+
 static struct max9286_source *next_source(struct max9286_priv *priv,
-					  struct max9286_source *source)
+					  struct max9286_source *source,
+					  bool routed)
 {
 	if (!source)
 		source = &priv->sources[0];
@@ -198,17 +201,27 @@ static struct max9286_source *next_source(struct max9286_priv *priv,
 		source++;
 
 	for (; source < &priv->sources[MAX9286_NUM_GMSL]; source++) {
-		if (source->fwnode)
+		unsigned int index = to_index(priv, source);
+
+		if (!source->fwnode)
+			continue;
+
+		/*
+		 * Careful here! A very unfortunate call to set_routing() can
+		 * change priv->route_mask behind our back!
+		 */
+		if (!routed || priv->route_mask & BIT(index))
 			return source;
 	}
 
 	return NULL;
 }
 
-#define for_each_source(priv, source) \
-	for ((source) = NULL; ((source) = next_source((priv), (source))); )
+#define for_each_route(priv, source) \
+	for ((source) = NULL; ((source) = next_source((priv), (source), true)); )
 
-#define to_index(priv, source) ((source) - &(priv)->sources[0])
+#define for_each_source(priv, source) \
+	for ((source) = NULL; ((source) = next_source((priv), (source), false)); )
 
 static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
 {
@@ -410,7 +423,7 @@ static int max9286_check_video_links(struct max9286_priv *priv)
 		if (ret < 0)
 			return -EIO;
 
-		if ((ret & MAX9286_VIDEO_DETECT_MASK) == priv->source_mask)
+		if ((ret & MAX9286_VIDEO_DETECT_MASK) == priv->route_mask)
 			break;
 
 		usleep_range(350, 500);
@@ -494,9 +507,10 @@ static int max9286_check_config_link(struct max9286_priv *priv,
 static int max9286_set_pixelrate(struct max9286_priv *priv)
 {
 	struct max9286_source *source = NULL;
+	unsigned int num_routes = 0;
 	u64 pixelrate = 0;
 
-	for_each_source(priv, source) {
+	for_each_route(priv, source) {
 		struct v4l2_ctrl *ctrl;
 		u64 source_rate = 0;
 
@@ -517,6 +531,8 @@ static int max9286_set_pixelrate(struct max9286_priv *priv)
 				"Unable to calculate pixel rate\n");
 			return -EINVAL;
 		}
+
+		num_routes++;
 	}
 
 	if (!pixelrate) {
@@ -530,7 +546,7 @@ static int max9286_set_pixelrate(struct max9286_priv *priv)
 	 * by the number of available sources.
 	 */
 	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate,
-				      pixelrate * priv->nsources);
+				      pixelrate * num_routes);
 }
 
 static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
@@ -692,8 +708,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		 */
 		max9286_i2c_mux_open(priv);
 
-		/* Start all cameras. */
-		for_each_source(priv, source) {
+		/* Start cameras. */
+		for_each_route(priv, source) {
 			ret = v4l2_subdev_call(source->sd, video, s_stream, 1);
 			if (ret)
 				return ret;
@@ -734,8 +750,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	} else {
 		max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
 
-		/* Stop all cameras. */
-		for_each_source(priv, source)
+		/* Stop cameras. */
+		for_each_route(priv, source)
 			v4l2_subdev_call(source->sd, video, s_stream, 0);
 
 		max9286_i2c_mux_close(priv);
@@ -912,6 +928,29 @@ static int max9286_set_routing(struct v4l2_subdev *sd,
 
 	state = v4l2_subdev_validate_and_lock_state(sd, state);
 	ret = _max9286_set_routing(sd, state, routing);
+	if (ret)
+		goto out;
+
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		goto out;
+
+	/*
+	 * Update the active routes mask and the pixel rate according to the
+	 * routed sources.
+	 */
+	priv->route_mask = 0;
+	for (i = 0; i < routing->num_routes; ++i) {
+		struct v4l2_subdev_route *route = &routing->routes[i];
+
+		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
+			continue;
+
+		priv->route_mask |= BIT(route->sink_pad);
+	}
+
+	max9286_set_pixelrate(priv);
+
+out:
 	v4l2_subdev_unlock_state(state);
 
 	return ret;
-- 
2.33.0

[PATCH v2 04/13] media: max9286: Use routes to configure link order

[Jacopo Mondi]

Use the routing table to configure the link output order and link
masking.

The link output order defines the CSI-2 virtual channel a GSML stream
is output on. Configure ordering at stream start time and at chip
setup time. This last step requires to move the chip initialization
function after the V4L2 setup phase as it requires the subdev state from
where the routing table is retrieved from to be initialized.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 103 ++++++++++++++++++++++--------------
 1 file changed, 64 insertions(+), 39 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 2e635179aec0..3485478f08a5 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -500,6 +500,61 @@ static int max9286_check_config_link(struct max9286_priv *priv,
 	return 0;
 }
 
+/*
+ * Configure the links output order (which defines on which CSI-2 VC a
+ * link is output on) and configure link masking.
+ */
+static void max9286_config_links(struct max9286_priv *priv)
+{
+	const struct v4l2_subdev_krouting *routing;
+	struct v4l2_subdev_state *state;
+	u8 link_order = 0;
+	u8 vc_mask = 0xf;
+	unsigned int i;
+
+	state = v4l2_subdev_lock_active_state(&priv->sd);
+	routing = &state->routing;
+
+	for (i = 0; i < routing->num_routes; ++i) {
+		struct v4l2_subdev_route *route = &routing->routes[i];
+
+		if (!(priv->route_mask & BIT(i)))
+			continue;
+
+		/* Assign the CSI-2 VC using the source stream number. */
+		link_order |= route->source_stream << (2 * route->sink_pad);
+		vc_mask &= ~BIT(route->source_stream);
+	}
+
+	/*
+	 * This might look rather silly, but now that we have assigned a
+	 * VC to the enabled routes, we have to assign one to the disabled
+	 * routes as well, as there cannot be two sources with the same VC.
+	 */
+	for (i = 0; i < MAX9286_NUM_GMSL; ++i) {
+		unsigned int vc;
+
+		if (priv->route_mask & BIT(i))
+			continue;
+
+		/* ffs() counts from 1. */
+		vc = ffs(vc_mask) - 1;
+		link_order |= vc << (2 * i);
+		vc_mask &= ~BIT(vc);
+	}
+
+	/*
+	 * Use the enabled routes to enable GMSL links, configure the CSI-2
+	 * output order, mask unused links and autodetect link used as CSI
+	 * clock source.
+	 */
+	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
+	max9286_write(priv, 0x0b, link_order);
+	max9286_write(priv, 0x69, 0xf & ~priv->route_mask);
+
+	v4l2_subdev_unlock_state(state);
+}
+
 /* -----------------------------------------------------------------------------
  * V4L2 Subdev
  */
@@ -701,6 +756,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	int ret;
 
 	if (enable) {
+		max9286_config_links(priv);
+
 		/*
 		 * The frame sync between cameras is transmitted across the
 		 * reverse channel as GPIO. We must open all channels while
@@ -1109,32 +1166,6 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
 
 static int max9286_setup(struct max9286_priv *priv)
 {
-	/*
-	 * Link ordering values for all enabled links combinations. Orders must
-	 * be assigned sequentially from 0 to the number of enabled links
-	 * without leaving any hole for disabled links. We thus assign orders to
-	 * enabled links first, and use the remaining order values for disabled
-	 * links are all links must have a different order value;
-	 */
-	static const u8 link_order[] = {
-		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxxx */
-		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxx0 */
-		(3 << 6) | (2 << 4) | (0 << 2) | (1 << 0), /* xx0x */
-		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xx10 */
-		(3 << 6) | (0 << 4) | (2 << 2) | (1 << 0), /* x0xx */
-		(3 << 6) | (1 << 4) | (2 << 2) | (0 << 0), /* x1x0 */
-		(3 << 6) | (1 << 4) | (0 << 2) | (2 << 0), /* x10x */
-		(3 << 6) | (1 << 4) | (1 << 2) | (0 << 0), /* x210 */
-		(0 << 6) | (3 << 4) | (2 << 2) | (1 << 0), /* 0xxx */
-		(1 << 6) | (3 << 4) | (2 << 2) | (0 << 0), /* 1xx0 */
-		(1 << 6) | (3 << 4) | (0 << 2) | (2 << 0), /* 1x0x */
-		(2 << 6) | (3 << 4) | (1 << 2) | (0 << 0), /* 2x10 */
-		(1 << 6) | (0 << 4) | (3 << 2) | (2 << 0), /* 10xx */
-		(2 << 6) | (1 << 4) | (3 << 2) | (0 << 0), /* 21x0 */
-		(2 << 6) | (1 << 4) | (0 << 2) | (3 << 0), /* 210x */
-		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* 3210 */
-	};
-
 	/*
 	 * Set the I2C bus speed.
 	 *
@@ -1144,13 +1175,7 @@ static int max9286_setup(struct max9286_priv *priv)
 	max9286_configure_i2c(priv, true);
 	max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
 
-	/*
-	 * Enable GMSL links, mask unused ones and autodetect link
-	 * used as CSI clock source.
-	 */
-	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
-	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
-	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
+	max9286_config_links(priv);
 
 	/*
 	 * Video format setup:
@@ -1324,12 +1349,6 @@ static int max9286_init(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = max9286_setup(priv);
-	if (ret) {
-		dev_err(dev, "Unable to setup max9286\n");
-		goto err_poc_disable;
-	}
-
 	/*
 	 * Register all V4L2 interactions for the MAX9286 and notifiers for
 	 * any subdevices connected.
@@ -1340,6 +1359,12 @@ static int max9286_init(struct device *dev)
 		goto err_poc_disable;
 	}
 
+	ret = max9286_setup(priv);
+	if (ret) {
+		dev_err(dev, "Unable to setup max9286\n");
+		goto err_poc_disable;
+	}
+
 	ret = max9286_i2c_mux_init(priv);
 	if (ret) {
 		dev_err(dev, "Unable to initialize I2C multiplexer\n");
-- 
2.33.0

[PATCH v2 05/13] media: max9286: Move format to subdev state

[Jacopo Mondi]

Move format handling to the v4l2_subdev state and store it per
(pad, stream) combination.

Now that the image format is stored in the subdev state, it can be
accessed through v4l2_subdev_get_fmt() instead of open-coding it.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 85 ++++++++++++-------------------------
 1 file changed, 27 insertions(+), 58 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 3485478f08a5..e51771d99437 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -175,8 +175,6 @@ struct max9286_priv {
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *pixelrate;
 
-	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
-
 	/* Protects controls and fmt structures */
 	struct mutex mutex;
 
@@ -829,28 +827,18 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static struct v4l2_mbus_framefmt *
-max9286_get_pad_format(struct max9286_priv *priv,
-		       struct v4l2_subdev_state *sd_state,
-		       unsigned int pad, u32 which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_get_try_format(&priv->sd, sd_state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &priv->fmt[pad];
-	default:
-		return NULL;
-	}
-}
-
 static int max9286_set_fmt(struct v4l2_subdev *sd,
 			   struct v4l2_subdev_state *sd_state,
 			   struct v4l2_subdev_format *format)
 {
-	struct max9286_priv *priv = sd_to_max9286(sd);
-	struct v4l2_mbus_framefmt *cfg_fmt;
+	struct v4l2_mbus_framefmt *fmt;
+	struct v4l2_subdev_state *state;
+	int ret = 0;
 
+	/*
+	 * Refuse to set format on the multiplexed source pad.
+	 * Format is propagated from sinks streams to source streams.
+	 */
 	if (format->pad == MAX9286_SRC_PAD)
 		return -EINVAL;
 
@@ -866,44 +854,28 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
 		break;
 	}
 
-	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
-					 format->which);
-	if (!cfg_fmt)
-		return -EINVAL;
-
-	mutex_lock(&priv->mutex);
-	*cfg_fmt = format->format;
-	mutex_unlock(&priv->mutex);
-
-	return 0;
-}
-
-static int max9286_get_fmt(struct v4l2_subdev *sd,
-			   struct v4l2_subdev_state *sd_state,
-			   struct v4l2_subdev_format *format)
-{
-	struct max9286_priv *priv = sd_to_max9286(sd);
-	struct v4l2_mbus_framefmt *cfg_fmt;
-	unsigned int pad = format->pad;
-
-	/*
-	 * Multiplexed Stream Support: Support link validation by returning the
-	 * format of the first bound link. All links must have the same format,
-	 * as we do not support mixing and matching of cameras connected to the
-	 * max9286.
-	 */
-	if (pad == MAX9286_SRC_PAD)
-		pad = __ffs(priv->bound_sources);
+	state = v4l2_subdev_validate_and_lock_state(sd, sd_state);
+	fmt = v4l2_state_get_stream_format(state, format->pad,
+					   format->stream);
+	if (!fmt) {
+		ret = -EINVAL;
+		goto out;
+	}
+	*fmt = format->format;
 
-	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
-	if (!cfg_fmt)
-		return -EINVAL;
+	/* Propagate format to the other end of the route. */
+	fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
+						    format->stream);
+	if (!fmt) {
+		ret = -EINVAL;
+		goto out;
+	}
+	*fmt = format->format;
 
-	mutex_lock(&priv->mutex);
-	format->format = *cfg_fmt;
-	mutex_unlock(&priv->mutex);
+out:
+	v4l2_subdev_unlock_state(state);
 
-	return 0;
+	return ret;
 }
 
 static int max9286_routing_verify(struct max9286_priv *priv,
@@ -1052,7 +1024,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
 static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
 	.init_cfg	= max9286_init_cfg,
 	.enum_mbus_code = max9286_enum_mbus_code,
-	.get_fmt	= max9286_get_fmt,
+	.get_fmt	= v4l2_subdev_get_fmt,
 	.set_fmt	= max9286_set_fmt,
 	.set_routing	= max9286_set_routing,
 };
@@ -1092,9 +1064,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 
 	/* Configure V4L2 for the MAX9286 itself */
 
-	for (i = 0; i < MAX9286_N_SINKS; i++)
-		priv->fmt[i] = max9286_default_format;
-
 	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
 	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
 			  V4L2_SUBDEV_FL_MULTIPLEXED;
-- 
2.33.0

[PATCH v2 06/13] media: subdev: Add for_each_active_route() macro

[Jacopo Mondi]

Add a for_each_active_route() macro to replace the repeated pattern
of iterating on the active routes of a routing table.

Replace the existing occurrences of such pattern in the codebase.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/ds90ub913.c             |  8 ++------
 drivers/media/i2c/ds90ub953.c             |  7 ++-----
 drivers/media/i2c/ds90ub960.c             |  8 ++------
 drivers/media/i2c/max9286.c               | 10 ++--------
 drivers/media/platform/ti-vpe/cal-video.c |  9 ++-------
 drivers/media/v4l2-core/v4l2-subdev.c     | 18 ++++++++++++++++++
 include/media/v4l2-subdev.h               | 11 +++++++++++
 7 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index d675faa2c571..478717e05480 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -342,8 +342,8 @@ static int ub913_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 	struct ub913_data *priv = sd_to_ub913(sd);
 	const struct v4l2_subdev_krouting *routing;
 	struct v4l2_mbus_frame_desc source_fd;
+	struct v4l2_subdev_route *route;
 	struct v4l2_subdev_state *state;
-	unsigned int i;
 	int ret = 0;
 
 	if (pad != 1) /* first tx pad */
@@ -361,13 +361,9 @@ static int ub913_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 
 	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL;
 
-	for (i = 0; i < routing->num_routes; ++i) {
-		const struct v4l2_subdev_route *route = &routing->routes[i];
+	for_each_active_route(routing, route) {
 		unsigned int j;
 
-		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
-			continue;
-
 		if (route->source_pad != pad)
 			continue;
 
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index d0b47e31be90..8615d8e996ee 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -383,6 +383,7 @@ static int ub953_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 	struct ub953_data *priv = sd_to_ub953(sd);
 	const struct v4l2_subdev_krouting *routing;
 	struct v4l2_mbus_frame_desc source_fd;
+	struct v4l2_subdev_route *route;
 	struct v4l2_subdev_state *state;
 	unsigned int i;
 	int ret = 0;
@@ -402,14 +403,10 @@ static int ub953_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 
 	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
 
-	for (i = 0; i < routing->num_routes; ++i) {
-		const struct v4l2_subdev_route *route = &routing->routes[i];
+	for_each_active_route(routing, route) {
 		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
 		unsigned int j;
 
-		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
-			continue;
-
 		if (route->source_pad != pad)
 			continue;
 
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index c655cb3e1ad6..9f79efddb138 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -1521,9 +1521,9 @@ static int ub960_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 {
 	struct ub960_data *priv = sd_to_ub960(sd);
 	const struct v4l2_subdev_krouting *routing;
+	struct v4l2_subdev_route *route;
 	struct v4l2_subdev_state *state;
 	int ret = 0;
-	unsigned int i;
 	struct device *dev = &priv->client->dev;
 
 	dev_dbg(dev, "%s for pad %d\n", __func__, pad);
@@ -1539,15 +1539,11 @@ static int ub960_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 
 	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
 
-	for (i = 0; i < routing->num_routes; ++i) {
-		const struct v4l2_subdev_route *route = &routing->routes[i];
+	for_each_active_route(routing, route) {
 		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
 		struct v4l2_mbus_frame_desc source_fd;
 		unsigned int j;
 
-		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
-			continue;
-
 		if (route->source_pad != pad)
 			continue;
 
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index e51771d99437..8bfb88db9976 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -952,7 +952,7 @@ static int max9286_set_routing(struct v4l2_subdev *sd,
 			       struct v4l2_subdev_krouting *routing)
 {
 	struct max9286_priv *priv = sd_to_max9286(sd);
-	unsigned int i;
+	struct v4l2_subdev_route *route;
 	int ret;
 
 	state = v4l2_subdev_validate_and_lock_state(sd, state);
@@ -968,14 +968,8 @@ static int max9286_set_routing(struct v4l2_subdev *sd,
 	 * routed sources.
 	 */
 	priv->route_mask = 0;
-	for (i = 0; i < routing->num_routes; ++i) {
-		struct v4l2_subdev_route *route = &routing->routes[i];
-
-		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
-			continue;
-
+	for_each_active_route(routing, route)
 		priv->route_mask |= BIT(route->sink_pad);
-	}
 
 	max9286_set_pixelrate(priv);
 
diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti-vpe/cal-video.c
index f945d737c5e5..cdd279a32beb 100644
--- a/drivers/media/platform/ti-vpe/cal-video.c
+++ b/drivers/media/platform/ti-vpe/cal-video.c
@@ -771,6 +771,7 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	if (cal_mc_api) {
 		struct v4l2_subdev_route *route = NULL;
+		struct v4l2_subdev_route *r;
 		struct media_pad *remote_pad;
 		unsigned int i;
 		struct v4l2_subdev_state *state;
@@ -790,13 +791,7 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 		/* Find the stream */
 
-		for (i = 0; i < state->routing.num_routes; ++i) {
-			struct v4l2_subdev_route *r =
-				&state->routing.routes[i];
-
-			if (!(r->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
-				continue;
-
+		for_each_active_route(routing, r) {
 			if (r->source_pad != remote_pad->index)
 				continue;
 
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 2a64ff003e4b..7dde44467c9a 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1593,3 +1593,21 @@ int v4l2_routing_simple_verify(const struct v4l2_subdev_krouting *routing)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_routing_simple_verify);
+
+struct v4l2_subdev_route *next_active_route(const struct v4l2_subdev_krouting *routing,
+					    struct v4l2_subdev_route *route)
+{
+	if (route)
+		++route;
+	else
+		route = &routing->routes[0];
+
+	for (; route < routing->routes + routing->num_routes; ++route) {
+		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
+			continue;
+
+		return route;
+	}
+
+	return NULL;
+}
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d0354e507970..ffce66e88952 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1582,4 +1582,15 @@ int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
  */
 int v4l2_routing_simple_verify(const struct v4l2_subdev_krouting *routing);
 
+struct v4l2_subdev_route *next_active_route(const struct v4l2_subdev_krouting *routing,
+					    struct v4l2_subdev_route *route);
+/**
+ * for_each_active_route - iterate on all active routes of a routing table
+ * @routing: The routing table
+ * @route: The route iterator
+ */
+#define for_each_active_route(routing, route)			\
+	for ((route) = NULL;					\
+	    ((route) = next_active_route((routing), (route)));)
+
 #endif
-- 
2.33.0

[PATCH v2 07/13] media: max9286: Implement get_frame_desc()

[Jacopo Mondi]

Implement the get_frame_desc pad operation to allow retrieving the
stream configuration of the max9286 subdevice.

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

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 8bfb88db9976..aca8455d6d56 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -878,6 +878,58 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static int max9286_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				  struct v4l2_mbus_frame_desc *fd)
+{
+	struct v4l2_subdev_route *route;
+	struct v4l2_subdev_state *state;
+	int ret = 0;
+
+	if (pad != MAX9286_SRC_PAD)
+		return -EINVAL;
+
+	state = v4l2_subdev_lock_active_state(sd);
+
+	memset(fd, 0, sizeof(*fd));
+
+	/* One stream entry per each connected route. */
+	for_each_active_route(&state->routing, route) {
+		struct v4l2_mbus_frame_desc_entry *entry =
+						&fd->entry[fd->num_entries];
+		struct v4l2_mbus_framefmt *fmt;
+
+		fmt = v4l2_state_get_stream_format(state, pad,
+						   route->source_stream);
+		if (!fmt) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/*
+		 * Assume a YUYV format (0x1e DT) and 16 bpp: we only support
+		 * these formats at the moment.
+		 */
+		entry->stream = fd->num_entries++;
+		entry->flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
+		entry->length = fmt->width * fmt->height * 16 / 8;
+		entry->pixelcode = fmt->code;
+
+		/*
+		 * The source stream id corresponds to the virtual channel a
+		 * stream is output on.
+		 */
+		entry->bus.csi2.vc = route->source_stream;
+		entry->bus.csi2.dt = 0x1e;
+	}
+
+	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
+
+out:
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
 static int max9286_routing_verify(struct max9286_priv *priv,
 				  struct v4l2_subdev_krouting *routing)
 {
@@ -1020,6 +1072,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
 	.enum_mbus_code = max9286_enum_mbus_code,
 	.get_fmt	= v4l2_subdev_get_fmt,
 	.set_fmt	= max9286_set_fmt,
+	.get_frame_desc = max9286_get_frame_desc,
 	.set_routing	= max9286_set_routing,
 };
 
-- 
2.33.0

[PATCH v2 08/13] media: rcar-csi2: Add support for v4l2_subdev_state

[Jacopo Mondi]

Create and initialize the v4l2_subdev_state for the R-Car CSI-2 receiver
rder to prepare to support routing operations and multiplexed streams.

Create the subdevice state with v4l2_subdev_init_finalize() and
implement the init_cfg() operation to guarantee the state is initialized
correctly with a set of default routes.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index e28eff039688..a74087b49e71 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -752,11 +752,65 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int rcsi2_init_cfg(struct v4l2_subdev *sd,
+			  struct v4l2_subdev_state *state)
+{
+	/* Initialize 4 routes from each source pad to the single sink pad. */
+	struct v4l2_subdev_route routes[] = {
+		{
+			.sink_pad = RCAR_CSI2_SINK,
+			.sink_stream = 0,
+			.source_pad = RCAR_CSI2_SOURCE_VC0,
+			.source_stream = 0,
+			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
+		},
+		{
+			.sink_pad = RCAR_CSI2_SINK,
+			.sink_stream = 1,
+			.source_pad = RCAR_CSI2_SOURCE_VC1,
+			.source_stream = 0,
+			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
+		},
+		{
+			.sink_pad = RCAR_CSI2_SINK,
+			.sink_stream = 2,
+			.source_pad = RCAR_CSI2_SOURCE_VC2,
+			.source_stream = 0,
+			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
+		},
+		{
+			.sink_pad = RCAR_CSI2_SINK,
+			.sink_stream = 3,
+			.source_pad = RCAR_CSI2_SOURCE_VC3,
+			.source_stream = 0,
+			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
+		},
+	};
+
+	struct v4l2_subdev_krouting routing = {
+		.num_routes = ARRAY_SIZE(routes),
+		.routes = routes,
+	};
+
+	int ret = v4l2_routing_simple_verify(&routing);
+	if (ret)
+		return ret;
+
+	state = v4l2_subdev_validate_and_lock_state(sd, state);
+
+	ret = v4l2_subdev_set_routing(sd, state, &routing);
+
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
 static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
 	.s_stream = rcsi2_s_stream,
 };
 
 static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
+	.init_cfg = rcsi2_init_cfg,
 	.set_fmt = rcsi2_set_pad_format,
 	.get_fmt = rcsi2_get_pad_format,
 };
@@ -1260,7 +1314,8 @@ static int rcsi2_probe(struct platform_device *pdev)
 	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
 	snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
 		 KBUILD_MODNAME, dev_name(&pdev->dev));
-	priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
+	priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE |
+			     V4L2_SUBDEV_FL_MULTIPLEXED;
 
 	priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
 	priv->subdev.entity.ops = &rcar_csi2_entity_ops;
@@ -1276,14 +1331,22 @@ static int rcsi2_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(&pdev->dev);
 
+	ret = v4l2_subdev_init_finalize(&priv->subdev);
+	if (ret)
+		goto error_pm;
+
 	ret = v4l2_async_register_subdev(&priv->subdev);
 	if (ret < 0)
-		goto error;
+		goto error_subdev;
 
 	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
 
 	return 0;
 
+error_subdev:
+	v4l2_subdev_cleanup(&priv->subdev);
+error_pm:
+	pm_runtime_disable(&pdev->dev);
 error:
 	v4l2_async_notifier_unregister(&priv->notifier);
 	v4l2_async_notifier_cleanup(&priv->notifier);
@@ -1298,6 +1361,7 @@ static int rcsi2_remove(struct platform_device *pdev)
 	v4l2_async_notifier_unregister(&priv->notifier);
 	v4l2_async_notifier_cleanup(&priv->notifier);
 	v4l2_async_unregister_subdev(&priv->subdev);
+	v4l2_subdev_cleanup(&priv->subdev);
 
 	pm_runtime_disable(&pdev->dev);
 
-- 
2.33.0

[PATCH v2 09/13] media: rcar-csi2: Implement set_routing

[Jacopo Mondi]

Add the set_routing() subdev operation to allow userspace to configure
routing on the R-Car CSI-2 receiver.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index a74087b49e71..451a6da26e03 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -752,6 +752,33 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int _rcsi2_set_routing(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_state *state,
+			      struct v4l2_subdev_krouting *routing)
+{
+	int ret;
+
+	ret = v4l2_routing_simple_verify(routing);
+	if (ret)
+		return ret;
+
+	state = v4l2_subdev_validate_and_lock_state(sd, state);
+
+	ret = v4l2_subdev_set_routing(sd, state, routing);
+
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
+static int rcsi2_set_routing(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_state *state,
+			     enum v4l2_subdev_format_whence which,
+			     struct v4l2_subdev_krouting *routing)
+{
+	return _rcsi2_set_routing(sd, state, routing);
+}
+
 static int rcsi2_init_cfg(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *state)
 {
@@ -792,17 +819,7 @@ static int rcsi2_init_cfg(struct v4l2_subdev *sd,
 		.routes = routes,
 	};
 
-	int ret = v4l2_routing_simple_verify(&routing);
-	if (ret)
-		return ret;
-
-	state = v4l2_subdev_validate_and_lock_state(sd, state);
-
-	ret = v4l2_subdev_set_routing(sd, state, &routing);
-
-	v4l2_subdev_unlock_state(state);
-
-	return ret;
+	return _rcsi2_set_routing(sd, state, &routing);
 }
 
 static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
@@ -813,6 +830,7 @@ static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
 	.init_cfg = rcsi2_init_cfg,
 	.set_fmt = rcsi2_set_pad_format,
 	.get_fmt = rcsi2_get_pad_format,
+	.set_routing = rcsi2_set_routing,
 };
 
 static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
-- 
2.33.0

[PATCH v2 10/13] media: rcar-csi2: Move format to subdev state

[Jacopo Mondi]

Move format handling to the v4l2_subdev state and store it per
(pad, stream) combination.

Now that the image format is stored in the subdev state, it can be
accessed through v4l2_subdev_get_fmt() instead of open-coding it.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 451a6da26e03..b60845b1e563 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -722,34 +722,42 @@ static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *sd_state,
 				struct v4l2_subdev_format *format)
 {
-	struct rcar_csi2 *priv = sd_to_csi2(sd);
-	struct v4l2_mbus_framefmt *framefmt;
+	struct v4l2_mbus_framefmt *fmt;
+	struct v4l2_subdev_state *state;
+	int ret = 0;
 
 	if (!rcsi2_code_to_fmt(format->format.code))
 		format->format.code = rcar_csi2_formats[0].code;
 
-	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		priv->mf = format->format;
-	} else {
-		framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
-		*framefmt = format->format;
-	}
+	/*
+	 * Format is propagated from sink streams to source streams, so
+	 * disallow setting format on the source pads.
+	 */
+	if (format->pad > RCAR_CSI2_SINK)
+		return -EINVAL;
 
-	return 0;
-}
+	state = v4l2_subdev_validate_and_lock_state(sd, sd_state);
+	fmt = v4l2_state_get_stream_format(state, format->pad,
+					   format->stream);
+	if (!fmt) {
+		ret = -EINVAL;
+		goto out;
+	}
+	*fmt = format->format;
 
-static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *sd_state,
-				struct v4l2_subdev_format *format)
-{
-	struct rcar_csi2 *priv = sd_to_csi2(sd);
+	/* Propagate format to the other end of the route. */
+	fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
+						    format->stream);
+	if (!fmt) {
+		ret = -EINVAL;
+		goto out;
+	}
+	*fmt = format->format;
 
-	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		format->format = priv->mf;
-	else
-		format->format = *v4l2_subdev_get_try_format(sd, sd_state, 0);
+out:
+	v4l2_subdev_unlock_state(state);
 
-	return 0;
+	return ret;
 }
 
 static int _rcsi2_set_routing(struct v4l2_subdev *sd,
@@ -829,7 +837,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
 static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
 	.init_cfg = rcsi2_init_cfg,
 	.set_fmt = rcsi2_set_pad_format,
-	.get_fmt = rcsi2_get_pad_format,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_routing = rcsi2_set_routing,
 };
 
-- 
2.33.0

[PATCH v2 11/13] media: rcar-csi2: Config by using the remote frame_desc

[Jacopo Mondi]

Configure the CSI-2 receiver by inspecting the remote subdev frame_desc.

Configure the link clock rate, field handling and CSI-2 Virtual Channel
and DT filtering using the frame descriptor retrieved from the
transmitter.

This change also makes mandatory for any subdevice that operates with
the R-Car CSI-2 receiver to implement the .get_frame_desc() operation.

[based on a patch from]
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[rework based on lates multistream support]
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 145 +++++++++++++++-----
 1 file changed, 110 insertions(+), 35 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index b60845b1e563..4ef7b9cb1ab7 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -69,10 +69,7 @@ struct rcar_csi2;
 #define FLD_REG				0x1c
 #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
 #define FLD_DET_SEL(n)			(((n) & 0x3) << 4)
-#define FLD_FLD_EN4			BIT(3)
-#define FLD_FLD_EN3			BIT(2)
-#define FLD_FLD_EN2			BIT(1)
-#define FLD_FLD_EN			BIT(0)
+#define FLD_FLD_EN(n)			BIT((n) & 0xf)
 
 /* Automatic Standby Control */
 #define ASTBY_REG			0x20
@@ -339,6 +336,17 @@ static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code)
 	return NULL;
 }
 
+static const struct rcar_csi2_format *rcsi2_datatype_to_fmt(unsigned int dt)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
+		if (rcar_csi2_formats[i].datatype == dt)
+			return &rcar_csi2_formats[i];
+
+	return NULL;
+}
+
 enum rcar_csi2_pads {
 	RCAR_CSI2_SINK,
 	RCAR_CSI2_SOURCE_VC0,
@@ -370,8 +378,6 @@ struct rcar_csi2 {
 	struct v4l2_subdev *remote;
 	unsigned int remote_pad;
 
-	struct v4l2_mbus_framefmt mf;
-
 	struct mutex lock;
 	int stream_count;
 
@@ -421,6 +427,32 @@ static int rcsi2_exit_standby(struct rcar_csi2 *priv)
 	return 0;
 }
 
+static int rcsi2_get_remote_frame_desc(struct rcar_csi2 *priv,
+				       struct v4l2_mbus_frame_desc *fd)
+{
+	struct media_pad *pad;
+	int ret;
+
+	if (!priv->remote)
+		return -ENODEV;
+
+	pad = media_entity_remote_pad(&priv->pads[RCAR_CSI2_SINK]);
+	if (!pad)
+		return -ENODEV;
+
+	ret = v4l2_subdev_call(priv->remote, pad, get_frame_desc,
+			       pad->index, fd);
+	if (ret)
+		return ret;
+
+	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
+		dev_err(priv->dev, "Frame desc do not describe CSI-2 link");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int rcsi2_wait_phy_start(struct rcar_csi2 *priv,
 				unsigned int lanes)
 {
@@ -460,11 +492,14 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 	return 0;
 }
 
-static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
-			   unsigned int lanes)
+static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
+			   struct v4l2_mbus_frame_desc *fd, unsigned int lanes)
 {
+	const struct v4l2_mbus_frame_desc_entry_csi2 *csi2_desc;
+	const struct rcar_csi2_format *format;
 	struct v4l2_subdev *source;
 	struct v4l2_ctrl *ctrl;
+	unsigned int i;
 	u64 mbps;
 
 	if (!priv->remote)
@@ -480,12 +515,30 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
 		return -EINVAL;
 	}
 
+	/* Verify that all remote streams send the same datatype. */
+	csi2_desc = NULL;
+	for (i = 0; i < fd->num_entries; i++) {
+		struct v4l2_mbus_frame_desc_entry_csi2 *stream_desc;
+
+		stream_desc = &fd->entry[i].bus.csi2;
+		if (!csi2_desc)
+			csi2_desc = stream_desc;
+
+		if (csi2_desc->dt != stream_desc->dt) {
+			dev_err(priv->dev,
+				"Remote streams with different DT: %u - %u\n",
+				csi2_desc->dt, stream_desc->dt);
+			return -EINVAL;
+		}
+	}
+	format = rcsi2_datatype_to_fmt(csi2_desc->dt);
+
 	/*
 	 * Calculate the phypll in mbps.
 	 * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
 	 * bps = link_freq * 2
 	 */
-	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
+	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * format->bpp;
 	do_div(mbps, lanes * 1000000);
 
 	return mbps;
@@ -541,49 +594,64 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
 
 static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 {
-	const struct rcar_csi2_format *format;
+	const struct v4l2_subdev_stream_configs *configs;
+	struct v4l2_subdev_state *state;
 	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
+	struct v4l2_mbus_frame_desc fd;
 	unsigned int lanes;
 	unsigned int i;
 	int mbps, ret;
 
-	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
-		priv->mf.width, priv->mf.height,
-		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
-
-	/* Code is validated in set_fmt. */
-	format = rcsi2_code_to_fmt(priv->mf.code);
+	/* Get information about multiplexed link. */
+	ret = rcsi2_get_remote_frame_desc(priv, &fd);
+	if (ret)
+		return ret;
 
-	/*
-	 * Enable all supported CSI-2 channels with virtual channel and
-	 * data type matching.
-	 *
-	 * NOTE: It's not possible to get individual datatype for each
-	 *       source virtual channel. Once this is possible in V4L2
-	 *       it should be used here.
-	 */
-	for (i = 0; i < priv->info->num_channels; i++) {
+	for (i = 0; i < fd.num_entries; i++) {
+		struct v4l2_mbus_frame_desc_entry *entry = &fd.entry[i];
 		u32 vcdt_part;
 
-		vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
-			VCDT_SEL_DT(format->datatype);
+		vcdt_part = VCDT_SEL_VC(entry->bus.csi2.vc) |
+			    VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
+			    VCDT_SEL_DT(entry->bus.csi2.dt);
 
 		/* Store in correct reg and offset. */
-		if (i < 2)
-			vcdt |= vcdt_part << ((i % 2) * 16);
+		if (entry->bus.csi2.vc < 2)
+			vcdt |= vcdt_part <<
+				((entry->bus.csi2.vc % 2) * 16);
 		else
-			vcdt2 |= vcdt_part << ((i % 2) * 16);
+			vcdt2 |= vcdt_part <<
+				((entry->bus.csi2.vc % 2) * 16);
+
+		dev_dbg(priv->dev, "VC: %d datatype: 0x%x\n",
+			entry->bus.csi2.vc, entry->bus.csi2.dt);
 	}
 
-	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
-		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
-			| FLD_FLD_EN;
+	/*
+	 * Configure field handling inspecting the formats of the
+	 * sink pad streams.
+	 */
+	state = v4l2_subdev_lock_active_state(&priv->subdev);
+	configs = &state->stream_configs;
+	for (i = 0; i < configs->num_configs; ++i) {
+		const struct v4l2_subdev_stream_config *config = configs->configs;
+
+		if (config->pad != RCAR_CSI2_SINK)
+			continue;
+
+		if (config->fmt.field != V4L2_FIELD_ALTERNATE)
+			continue;
 
-		if (priv->mf.height == 240)
+		fld |= FLD_DET_SEL(1);
+		fld |= FLD_FLD_EN(config->stream);
+
+		/* PAL vs NTSC. */
+		if (config->fmt.height == 240)
 			fld |= FLD_FLD_NUM(0);
 		else
 			fld |= FLD_FLD_NUM(1);
 	}
+	v4l2_subdev_unlock_state(state);
 
 	/*
 	 * Get the number of active data lanes inspecting the remote mbus
@@ -596,7 +664,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
 	phycnt = PHYCNT_ENABLECLK;
 	phycnt |= (1 << lanes) - 1;
 
-	mbps = rcsi2_calc_mbps(priv, format->bpp, lanes);
+	mbps = rcsi2_calc_mbps(priv, &fd, lanes);
 	if (mbps < 0)
 		return mbps;
 
@@ -894,6 +962,13 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier,
 	struct rcar_csi2 *priv = notifier_to_csi2(notifier);
 	int pad;
 
+	if (!v4l2_subdev_has_op(subdev, pad, get_frame_desc)) {
+		dev_err(priv->dev,
+			"Subdev %s bound failed: missing get_frame_desc()\n",
+			subdev->name);
+		return -EINVAL;
+	}
+
 	pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
 					  MEDIA_PAD_FL_SOURCE);
 	if (pad < 0) {
-- 
2.33.0

[PATCH v2 12/13] media: rcar-csi2: Implement .get_frame_desc()

[Jacopo Mondi]

Implement the get_frame_desc subdev pad operation.

The operation informs the VIN entity to which the CSI-2 output channel
is routed to about the CSI-2 Virtual Channel and the Datatype the
transported stream is tagged with.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 4ef7b9cb1ab7..7084ee06e99e 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -828,6 +828,68 @@ static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
 	return ret;
 }
 
+/*
+ * The R-Car CSI-2 receivers are connected to the VINs (or ISP) by 'channels'
+ * not to be confused with CSI-2 Virtual Channels.
+ *
+ * Inform the VIN instance, to which the channel identified by the source pad is
+ * routed to, about the CSI-2 Virtual Channel and CSI-2 Datatype of the
+ * transported data stream.
+ */
+static int rcsi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				struct v4l2_mbus_frame_desc *fd)
+{
+	struct v4l2_subdev_state *state;
+	struct v4l2_subdev_route *route;
+	int ret;
+
+	if (pad < RCAR_CSI2_SOURCE_VC0 || pad > RCAR_CSI2_SOURCE_VC3)
+		return -EINVAL;
+
+	state = v4l2_subdev_validate_and_lock_state(sd, state);
+
+	memset(fd, 0, sizeof(*fd));
+
+	for_each_active_route(&state->routing, route) {
+		const struct rcar_csi2_format *rcsi2_fmt;
+		struct v4l2_mbus_framefmt *fmt;
+
+		if (route->source_pad != pad)
+			continue;
+
+		fmt = v4l2_state_get_stream_format(state, pad,
+						   route->source_stream);
+		if (!fmt) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		rcsi2_fmt = rcsi2_code_to_fmt(fmt->code);
+
+		/* Only one stream per 'channel' is allowed. */
+		fd->entry[0].pixelcode = fmt->code;
+		fd->entry[0].length = fmt->width * fmt->height * rcsi2_fmt->bpp / 8;
+		fd->entry[0].flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
+
+		/*
+		 * The sink stream id corresponds to the CSI-2 Virtual Channel
+		 * which is output on the channel identified by the current
+		 * source pad.
+		 */
+		fd->entry[0].bus.csi2.vc = route->source_stream;
+		fd->entry[0].bus.csi2.dt = rcsi2_fmt->datatype;
+
+		break;
+	}
+	fd->num_entries = 1;
+	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
+
+out:
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
 static int _rcsi2_set_routing(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_state *state,
 			      struct v4l2_subdev_krouting *routing)
@@ -906,6 +968,7 @@ static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
 	.init_cfg = rcsi2_init_cfg,
 	.set_fmt = rcsi2_set_pad_format,
 	.get_fmt = v4l2_subdev_get_fmt,
+	.get_frame_desc = rcsi2_get_frame_desc,
 	.set_routing = rcsi2_set_routing,
 };
 
-- 
2.33.0

[PATCH v2 13/13] media: rcar-vin: Support multiplexed CSI-2 receiver

[Jacopo Mondi]

The R-Car CSI-2 receiver supports multiplexed streams.

The VIN driver inspects the CSI-2 subdevice format with the intent of
validating formats at stream start time.

Each VIN instance is connected to a CSI-2 receiver which exposes a
single stream and since it has acquired support for multiplexed streams
maintains the format information in its v4l2_subdev_state per-stream.

Instrument the VIN driver to fetch the CSI-2 receiver format from the
streams configuration by setting the stream identifier to 0 and pass to
the subdevice its own state where to retrieve format information from.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 83b2f923cf98..a6f3701f3f3f 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1119,7 +1119,8 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
 	};
 
 	fmt.pad = pad->index;
-	if (v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt))
+	fmt.stream = 0;
+	if (v4l2_subdev_call(sd, pad, get_fmt, sd->state, &fmt))
 		return -EPIPE;
 
 	switch (fmt.format.code) {
-- 
2.33.0

Re: [PATCH v2 06/13] media: subdev: Add for_each_active_route() macro

[Tomi Valkeinen]

Hi Jacopo,

On 17/10/2021 21:24, Jacopo Mondi wrote:
> Add a for_each_active_route() macro to replace the repeated pattern
> of iterating on the active routes of a routing table.
> 
> Replace the existing occurrences of such pattern in the codebase.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>   drivers/media/i2c/ds90ub913.c             |  8 ++------
>   drivers/media/i2c/ds90ub953.c             |  7 ++-----
>   drivers/media/i2c/ds90ub960.c             |  8 ++------
>   drivers/media/i2c/max9286.c               | 10 ++--------
>   drivers/media/platform/ti-vpe/cal-video.c |  9 ++-------
>   drivers/media/v4l2-core/v4l2-subdev.c     | 18 ++++++++++++++++++
>   include/media/v4l2-subdev.h               | 11 +++++++++++
>   7 files changed, 39 insertions(+), 32 deletions(-)

I'll pick this one to my branch, if you don't mind.

  Tomi

Re: [PATCH v2 06/13] media: subdev: Add for_each_active_route() macro

[Tomi Valkeinen]

On 17/10/2021 21:24, Jacopo Mondi wrote:
> Add a for_each_active_route() macro to replace the repeated pattern
> of iterating on the active routes of a routing table.
> 
> Replace the existing occurrences of such pattern in the codebase.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>   drivers/media/i2c/ds90ub913.c             |  8 ++------
>   drivers/media/i2c/ds90ub953.c             |  7 ++-----
>   drivers/media/i2c/ds90ub960.c             |  8 ++------
>   drivers/media/i2c/max9286.c               | 10 ++--------
>   drivers/media/platform/ti-vpe/cal-video.c |  9 ++-------
>   drivers/media/v4l2-core/v4l2-subdev.c     | 18 ++++++++++++++++++
>   include/media/v4l2-subdev.h               | 11 +++++++++++
>   7 files changed, 39 insertions(+), 32 deletions(-)
> 

...

> +struct v4l2_subdev_route *next_active_route(const struct v4l2_subdev_krouting *routing,
> +					    struct v4l2_subdev_route *route)
> +{
> +	if (route)
> +		++route;
> +	else
> +		route = &routing->routes[0];
> +
> +	for (; route < routing->routes + routing->num_routes; ++route) {
> +		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> +			continue;
> +
> +		return route;
> +	}
> +
> +	return NULL;
> +}

Also, this must be exported. I'll add that. And probably better to have 
a prefix in the function name.

  Tomi

Re: [PATCH v2 06/13] media: subdev: Add for_each_active_route() macro

[Jacopo Mondi]

Hi Tomi,

On Thu, Oct 28, 2021 at 11:27:05AM +0300, Tomi Valkeinen wrote:
> Hi Jacopo,
>
> On 17/10/2021 21:24, Jacopo Mondi wrote:
> > Add a for_each_active_route() macro to replace the repeated pattern
> > of iterating on the active routes of a routing table.
> >
> > Replace the existing occurrences of such pattern in the codebase.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >   drivers/media/i2c/ds90ub913.c             |  8 ++------
> >   drivers/media/i2c/ds90ub953.c             |  7 ++-----
> >   drivers/media/i2c/ds90ub960.c             |  8 ++------
> >   drivers/media/i2c/max9286.c               | 10 ++--------
> >   drivers/media/platform/ti-vpe/cal-video.c |  9 ++-------
> >   drivers/media/v4l2-core/v4l2-subdev.c     | 18 ++++++++++++++++++
> >   include/media/v4l2-subdev.h               | 11 +++++++++++
> >   7 files changed, 39 insertions(+), 32 deletions(-)
>
> I'll pick this one to my branch, if you don't mind.

My pleasure! Go ahead please!

>
>  Tomi

Re: [PATCH v2 06/13] media: subdev: Add for_each_active_route() macro

[Jacopo Mondi]

Hi Tomi,

On Thu, Oct 28, 2021 at 11:32:12AM +0300, Tomi Valkeinen wrote:
> On 17/10/2021 21:24, Jacopo Mondi wrote:
> > Add a for_each_active_route() macro to replace the repeated pattern
> > of iterating on the active routes of a routing table.
> >
> > Replace the existing occurrences of such pattern in the codebase.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >   drivers/media/i2c/ds90ub913.c             |  8 ++------
> >   drivers/media/i2c/ds90ub953.c             |  7 ++-----
> >   drivers/media/i2c/ds90ub960.c             |  8 ++------
> >   drivers/media/i2c/max9286.c               | 10 ++--------
> >   drivers/media/platform/ti-vpe/cal-video.c |  9 ++-------
> >   drivers/media/v4l2-core/v4l2-subdev.c     | 18 ++++++++++++++++++
> >   include/media/v4l2-subdev.h               | 11 +++++++++++
> >   7 files changed, 39 insertions(+), 32 deletions(-)
> >
>
> ...
>
> > +struct v4l2_subdev_route *next_active_route(const struct v4l2_subdev_krouting *routing,
> > +					    struct v4l2_subdev_route *route)
> > +{
> > +	if (route)
> > +		++route;
> > +	else
> > +		route = &routing->routes[0];
> > +
> > +	for (; route < routing->routes + routing->num_routes; ++route) {
> > +		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> > +			continue;
> > +
> > +		return route;
> > +	}
> > +
> > +	return NULL;
> > +}
>
> Also, this must be exported. I'll add that. And probably better to have a

Does it ? I would rather have it in the header, as this is only
meant to be called by the for_each_active_route() macro, and not by
other users. However it seemed to be rather long to be defined as a
static inline function in the header, so I opted to move it to the c
file.

To be honest, it's not clear to me what happens if a module calls the
for_each_active_route() macro that calls this non-exported function,
so you're probably correct.

However exporting the symbol makes it available globally, but I guess
that's not a big deal if it's clearly documented that drivers shall
not call this directly (or maybe we want it to be available globally,
why not...)

> prefix in the function name.

This might be a good idea!

Thanks
   j

>
>  Tomi

Re: [PATCH v2 06/13] media: subdev: Add for_each_active_route() macro

[Tomi Valkeinen]

On 28/10/2021 12:03, Jacopo Mondi wrote:
> Hi Tomi,
> 
> On Thu, Oct 28, 2021 at 11:32:12AM +0300, Tomi Valkeinen wrote:
>> On 17/10/2021 21:24, Jacopo Mondi wrote:
>>> Add a for_each_active_route() macro to replace the repeated pattern
>>> of iterating on the active routes of a routing table.
>>>
>>> Replace the existing occurrences of such pattern in the codebase.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>    drivers/media/i2c/ds90ub913.c             |  8 ++------
>>>    drivers/media/i2c/ds90ub953.c             |  7 ++-----
>>>    drivers/media/i2c/ds90ub960.c             |  8 ++------
>>>    drivers/media/i2c/max9286.c               | 10 ++--------
>>>    drivers/media/platform/ti-vpe/cal-video.c |  9 ++-------
>>>    drivers/media/v4l2-core/v4l2-subdev.c     | 18 ++++++++++++++++++
>>>    include/media/v4l2-subdev.h               | 11 +++++++++++
>>>    7 files changed, 39 insertions(+), 32 deletions(-)
>>>
>>
>> ...
>>
>>> +struct v4l2_subdev_route *next_active_route(const struct v4l2_subdev_krouting *routing,
>>> +					    struct v4l2_subdev_route *route)
>>> +{
>>> +	if (route)
>>> +		++route;
>>> +	else
>>> +		route = &routing->routes[0];
>>> +
>>> +	for (; route < routing->routes + routing->num_routes; ++route) {
>>> +		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
>>> +			continue;
>>> +
>>> +		return route;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>
>> Also, this must be exported. I'll add that. And probably better to have a
> 
> Does it ? I would rather have it in the header, as this is only
> meant to be called by the for_each_active_route() macro, and not by
> other users. However it seemed to be rather long to be defined as a
> static inline function in the header, so I opted to move it to the c
> file.

Yes, static inline is an option. The function is a bit long-ish, though, 
as you mention.

> To be honest, it's not clear to me what happens if a module calls the
> for_each_active_route() macro that calls this non-exported function,
> so you're probably correct.

The module cannot be loaded if it refers to a non-exported symbol.

> However exporting the symbol makes it available globally, but I guess

Yes, thus the prefix is a good thing =).

> that's not a big deal if it's clearly documented that drivers shall
> not call this directly (or maybe we want it to be available globally,
> why not...)

I'll see how long helper functions similar macros have as inline in 
other parts of the kernel. Maybe static inline is fine.

But if not, we'll just need to document the helper function. I don't see 
why we should say it shouldn't be called directly, though. But if that 
is the case, we can prefix it with __.

  Tomi

Re: [PATCH v2 06/13] media: subdev: Add for_each_active_route() macro

[Laurent Pinchart]

On Thu, Oct 28, 2021 at 01:17:10PM +0300, Tomi Valkeinen wrote:
> On 28/10/2021 12:03, Jacopo Mondi wrote:
> > On Thu, Oct 28, 2021 at 11:32:12AM +0300, Tomi Valkeinen wrote:
> >> On 17/10/2021 21:24, Jacopo Mondi wrote:
> >>> Add a for_each_active_route() macro to replace the repeated pattern
> >>> of iterating on the active routes of a routing table.
> >>>
> >>> Replace the existing occurrences of such pattern in the codebase.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>>    drivers/media/i2c/ds90ub913.c             |  8 ++------
> >>>    drivers/media/i2c/ds90ub953.c             |  7 ++-----
> >>>    drivers/media/i2c/ds90ub960.c             |  8 ++------
> >>>    drivers/media/i2c/max9286.c               | 10 ++--------
> >>>    drivers/media/platform/ti-vpe/cal-video.c |  9 ++-------
> >>>    drivers/media/v4l2-core/v4l2-subdev.c     | 18 ++++++++++++++++++
> >>>    include/media/v4l2-subdev.h               | 11 +++++++++++
> >>>    7 files changed, 39 insertions(+), 32 deletions(-)
> >>>
> >>
> >> ...
> >>
> >>> +struct v4l2_subdev_route *next_active_route(const struct v4l2_subdev_krouting *routing,
> >>> +					    struct v4l2_subdev_route *route)
> >>> +{
> >>> +	if (route)
> >>> +		++route;
> >>> +	else
> >>> +		route = &routing->routes[0];
> >>> +
> >>> +	for (; route < routing->routes + routing->num_routes; ++route) {
> >>> +		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> >>> +			continue;
> >>> +
> >>> +		return route;
> >>> +	}
> >>> +
> >>> +	return NULL;
> >>> +}
> >>
> >> Also, this must be exported. I'll add that. And probably better to have a
> > 
> > Does it ? I would rather have it in the header, as this is only
> > meant to be called by the for_each_active_route() macro, and not by
> > other users. However it seemed to be rather long to be defined as a
> > static inline function in the header, so I opted to move it to the c
> > file.
> 
> Yes, static inline is an option. The function is a bit long-ish, though, 
> as you mention.
> 
> > To be honest, it's not clear to me what happens if a module calls the
> > for_each_active_route() macro that calls this non-exported function,
> > so you're probably correct.
> 
> The module cannot be loaded if it refers to a non-exported symbol.
> 
> > However exporting the symbol makes it available globally, but I guess
> 
> Yes, thus the prefix is a good thing =).
> 
> > that's not a big deal if it's clearly documented that drivers shall
> > not call this directly (or maybe we want it to be available globally,
> > why not...)
> 
> I'll see how long helper functions similar macros have as inline in 
> other parts of the kernel. Maybe static inline is fine.
> 
> But if not, we'll just need to document the helper function. I don't see 
> why we should say it shouldn't be called directly, though. But if that 
> is the case, we can prefix it with __.

The __ prefix is exactly what I was going to propose.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 06/13] media: subdev: Add for_each_active_route() macro

[Jacopo Mondi]

Hello,

On Thu, Oct 28, 2021 at 01:18:44PM +0300, Laurent Pinchart wrote:
> On Thu, Oct 28, 2021 at 01:17:10PM +0300, Tomi Valkeinen wrote:
> > On 28/10/2021 12:03, Jacopo Mondi wrote:
> > > On Thu, Oct 28, 2021 at 11:32:12AM +0300, Tomi Valkeinen wrote:
> > >> On 17/10/2021 21:24, Jacopo Mondi wrote:
> > >>> Add a for_each_active_route() macro to replace the repeated pattern
> > >>> of iterating on the active routes of a routing table.
> > >>>
> > >>> Replace the existing occurrences of such pattern in the codebase.
> > >>>
> > >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >>> ---
> > >>>    drivers/media/i2c/ds90ub913.c             |  8 ++------
> > >>>    drivers/media/i2c/ds90ub953.c             |  7 ++-----
> > >>>    drivers/media/i2c/ds90ub960.c             |  8 ++------
> > >>>    drivers/media/i2c/max9286.c               | 10 ++--------
> > >>>    drivers/media/platform/ti-vpe/cal-video.c |  9 ++-------
> > >>>    drivers/media/v4l2-core/v4l2-subdev.c     | 18 ++++++++++++++++++
> > >>>    include/media/v4l2-subdev.h               | 11 +++++++++++
> > >>>    7 files changed, 39 insertions(+), 32 deletions(-)
> > >>>
> > >>
> > >> ...
> > >>
> > >>> +struct v4l2_subdev_route *next_active_route(const struct v4l2_subdev_krouting *routing,
> > >>> +					    struct v4l2_subdev_route *route)
> > >>> +{
> > >>> +	if (route)
> > >>> +		++route;
> > >>> +	else
> > >>> +		route = &routing->routes[0];
> > >>> +
> > >>> +	for (; route < routing->routes + routing->num_routes; ++route) {
> > >>> +		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> > >>> +			continue;
> > >>> +
> > >>> +		return route;
> > >>> +	}
> > >>> +
> > >>> +	return NULL;
> > >>> +}
> > >>
> > >> Also, this must be exported. I'll add that. And probably better to have a
> > >
> > > Does it ? I would rather have it in the header, as this is only
> > > meant to be called by the for_each_active_route() macro, and not by
> > > other users. However it seemed to be rather long to be defined as a
> > > static inline function in the header, so I opted to move it to the c
> > > file.
> >
> > Yes, static inline is an option. The function is a bit long-ish, though,
> > as you mention.
> >
> > > To be honest, it's not clear to me what happens if a module calls the
> > > for_each_active_route() macro that calls this non-exported function,
> > > so you're probably correct.
> >
> > The module cannot be loaded if it refers to a non-exported symbol.
> >

Yeah, dumb me, the macro will just expand and the symbol won't be
available.

> > > However exporting the symbol makes it available globally, but I guess
> >
> > Yes, thus the prefix is a good thing =).
> >
> > > that's not a big deal if it's clearly documented that drivers shall
> > > not call this directly (or maybe we want it to be available globally,
> > > why not...)
> >
> > I'll see how long helper functions similar macros have as inline in
> > other parts of the kernel. Maybe static inline is fine.
> >
> > But if not, we'll just need to document the helper function. I don't see
> > why we should say it shouldn't be called directly, though. But if that
> > is the case, we can prefix it with __.
>
> The __ prefix is exactly what I was going to propose.
>

Ack to the __ prefix and export the symbol from the .c file if it's
deemed too long to live in the header!

Thanks
   j

> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 01/13] media: max9286: Add support for v4l2_subdev_state

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Oct 17, 2021 at 08:24:37PM +0200, Jacopo Mondi wrote:
> Create and initialize the v4l2_subdev_state for the max9286 driver in
> order to prepare to support routing operations and multiplexed streams.
> 
> Create the subdevice state with v4l2_subdev_init_finalize() and
> implement the init_cfg() operation to guarantee the state is initialized
> correctly with the default device format.
> 
> Remove the max9286_open() subdev internal operation as the format of the
> file-handle state is now initialized by init_cfg().
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 90 +++++++++++++++++++++++--------------
>  1 file changed, 57 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1b92d18a1f94..5997fe40509f 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -215,6 +215,17 @@ static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
>  	return container_of(sd, struct max9286_priv, sd);
>  }
>  
> +static const struct v4l2_mbus_framefmt max9286_default_format = {
> +	.width		= 1280,
> +	.height		= 800,
> +	.code		= MEDIA_BUS_FMT_UYVY8_1X16,
> +	.colorspace	= V4L2_COLORSPACE_SRGB,
> +	.field		= V4L2_FIELD_NONE,
> +	.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
> +	.quantization	= V4L2_QUANTIZATION_DEFAULT,
> +	.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * I2C IO
>   */
> @@ -822,11 +833,45 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int max9286_init_cfg(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_state *state)
> +{
> +	struct v4l2_subdev_route routes[MAX9286_NUM_GMSL];
> +	struct max9286_priv *priv = sd_to_max9286(sd);
> +	struct v4l2_subdev_krouting routing;
> +	struct max9286_source *source;
> +	unsigned int num_routes = 0;
> +	int ret;
> +
> +	/* Create a route for each enable source. */
> +	for_each_source(priv, source) {
> +		struct v4l2_subdev_route *route = &routes[num_routes++];
> +		unsigned int idx = to_index(priv, source);
> +
> +		route->sink_pad = idx;
> +		route->sink_stream = 0;
> +		route->source_pad = MAX9286_SRC_PAD;
> +		route->source_stream = idx;
> +		route->flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE;
> +	}
> +
> +	routing.num_routes = num_routes;
> +	routing.routes = routes;
> +
> +	state = v4l2_subdev_validate_and_lock_state(sd, state);
> +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
> +					       &max9286_default_format);
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +
>  static const struct v4l2_subdev_video_ops max9286_video_ops = {
>  	.s_stream	= max9286_s_stream,
>  };
>  
>  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> +	.init_cfg	= max9286_init_cfg,
>  	.enum_mbus_code = max9286_enum_mbus_code,
>  	.get_fmt	= max9286_get_fmt,
>  	.set_fmt	= max9286_set_fmt,
> @@ -837,35 +882,6 @@ static const struct v4l2_subdev_ops max9286_subdev_ops = {
>  	.pad		= &max9286_pad_ops,
>  };
>  
> -static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
> -{
> -	fmt->width		= 1280;
> -	fmt->height		= 800;
> -	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
> -	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
> -	fmt->field		= V4L2_FIELD_NONE;
> -	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
> -	fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
> -	fmt->xfer_func		= V4L2_XFER_FUNC_DEFAULT;
> -}
> -
> -static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
> -{
> -	struct v4l2_mbus_framefmt *format;
> -	unsigned int i;
> -
> -	for (i = 0; i < MAX9286_N_SINKS; i++) {
> -		format = v4l2_subdev_get_try_format(subdev, fh->state, i);
> -		max9286_init_format(format);
> -	}
> -
> -	return 0;
> -}
> -
> -static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
> -	.open = max9286_open,
> -};
> -
>  static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	switch (ctrl->id) {
> @@ -897,11 +913,11 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  	/* Configure V4L2 for the MAX9286 itself */
>  
>  	for (i = 0; i < MAX9286_N_SINKS; i++)
> -		max9286_init_format(&priv->fmt[i]);
> +		priv->fmt[i] = max9286_default_format;
>  
>  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
> -	priv->sd.internal_ops = &max9286_subdev_internal_ops;
> -	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +			  V4L2_SUBDEV_FL_MULTIPLEXED;
>  
>  	v4l2_ctrl_handler_init(&priv->ctrls, 1);
>  	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
> @@ -933,14 +949,21 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  	}
>  	priv->sd.fwnode = ep;
>  
> +	ret = v4l2_subdev_init_finalize(&priv->sd);
> +	if (ret)
> +		goto err_put_node;
> +
>  	ret = v4l2_async_register_subdev(&priv->sd);
>  	if (ret < 0) {
>  		dev_err(dev, "Unable to register subdevice\n");
> -		goto err_put_node;
> +		goto err_free_state;
>  	}
>  
>  	return 0;
>  
> +err_free_state:

I'd call this new label err_subdev_cleanup.

> +	v4l2_subdev_cleanup(&priv->sd);
> +
>  err_put_node:
>  	fwnode_handle_put(ep);
>  err_async:
> @@ -953,6 +976,7 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
>  {
>  	fwnode_handle_put(priv->sd.fwnode);
>  	v4l2_async_unregister_subdev(&priv->sd);
> +	v4l2_subdev_cleanup(&priv->sd);
>  	max9286_v4l2_notifier_unregister(priv);
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 05/13] media: max9286: Move format to subdev state

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Oct 17, 2021 at 08:24:41PM +0200, Jacopo Mondi wrote:
> Move format handling to the v4l2_subdev state and store it per
> (pad, stream) combination.
> 
> Now that the image format is stored in the subdev state, it can be
> accessed through v4l2_subdev_get_fmt() instead of open-coding it.

Would it be possible to move this to 02/13 in the series ? Storing the
formats in the state doesn't depend on multiplexed streams support, we
could thus merge it early. The current 01/13 would need to be split in
two, with one part to allocate the active state and implement
.init_cfg() without muxed streams support, and another part to add muxed
streams support on top.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 85 ++++++++++++-------------------------
>  1 file changed, 27 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 3485478f08a5..e51771d99437 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -175,8 +175,6 @@ struct max9286_priv {
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *pixelrate;
>  
> -	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> -
>  	/* Protects controls and fmt structures */
>  	struct mutex mutex;
>  
> @@ -829,28 +827,18 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> -static struct v4l2_mbus_framefmt *
> -max9286_get_pad_format(struct max9286_priv *priv,
> -		       struct v4l2_subdev_state *sd_state,
> -		       unsigned int pad, u32 which)
> -{
> -	switch (which) {
> -	case V4L2_SUBDEV_FORMAT_TRY:
> -		return v4l2_subdev_get_try_format(&priv->sd, sd_state, pad);
> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> -		return &priv->fmt[pad];
> -	default:
> -		return NULL;
> -	}
> -}
> -
>  static int max9286_set_fmt(struct v4l2_subdev *sd,
>  			   struct v4l2_subdev_state *sd_state,
>  			   struct v4l2_subdev_format *format)
>  {
> -	struct max9286_priv *priv = sd_to_max9286(sd);
> -	struct v4l2_mbus_framefmt *cfg_fmt;
> +	struct v4l2_mbus_framefmt *fmt;
> +	struct v4l2_subdev_state *state;
> +	int ret = 0;
>  
> +	/*
> +	 * Refuse to set format on the multiplexed source pad.
> +	 * Format is propagated from sinks streams to source streams.
> +	 */
>  	if (format->pad == MAX9286_SRC_PAD)
>  		return -EINVAL;
>  
> @@ -866,44 +854,28 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  		break;
>  	}
>  
> -	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
> -					 format->which);
> -	if (!cfg_fmt)
> -		return -EINVAL;
> -
> -	mutex_lock(&priv->mutex);
> -	*cfg_fmt = format->format;
> -	mutex_unlock(&priv->mutex);
> -
> -	return 0;
> -}
> -
> -static int max9286_get_fmt(struct v4l2_subdev *sd,
> -			   struct v4l2_subdev_state *sd_state,
> -			   struct v4l2_subdev_format *format)
> -{
> -	struct max9286_priv *priv = sd_to_max9286(sd);
> -	struct v4l2_mbus_framefmt *cfg_fmt;
> -	unsigned int pad = format->pad;
> -
> -	/*
> -	 * Multiplexed Stream Support: Support link validation by returning the
> -	 * format of the first bound link. All links must have the same format,
> -	 * as we do not support mixing and matching of cameras connected to the
> -	 * max9286.
> -	 */
> -	if (pad == MAX9286_SRC_PAD)
> -		pad = __ffs(priv->bound_sources);
> +	state = v4l2_subdev_validate_and_lock_state(sd, sd_state);
> +	fmt = v4l2_state_get_stream_format(state, format->pad,
> +					   format->stream);
> +	if (!fmt) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	*fmt = format->format;
>  
> -	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
> -	if (!cfg_fmt)
> -		return -EINVAL;
> +	/* Propagate format to the other end of the route. */
> +	fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
> +						    format->stream);
> +	if (!fmt) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	*fmt = format->format;
>  
> -	mutex_lock(&priv->mutex);
> -	format->format = *cfg_fmt;
> -	mutex_unlock(&priv->mutex);
> +out:
> +	v4l2_subdev_unlock_state(state);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int max9286_routing_verify(struct max9286_priv *priv,
> @@ -1052,7 +1024,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
>  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
>  	.init_cfg	= max9286_init_cfg,
>  	.enum_mbus_code = max9286_enum_mbus_code,
> -	.get_fmt	= max9286_get_fmt,
> +	.get_fmt	= v4l2_subdev_get_fmt,
>  	.set_fmt	= max9286_set_fmt,
>  	.set_routing	= max9286_set_routing,
>  };
> @@ -1092,9 +1064,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  
>  	/* Configure V4L2 for the MAX9286 itself */
>  
> -	for (i = 0; i < MAX9286_N_SINKS; i++)
> -		priv->fmt[i] = max9286_default_format;
> -
>  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>  			  V4L2_SUBDEV_FL_MULTIPLEXED;
> -- 
> 2.33.0
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 02/13] media: max9286: Implement set_routing

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Oct 17, 2021 at 08:24:38PM +0200, Jacopo Mondi wrote:
> Add the set_routing() subdev operation to allow userspace to configure
> routing on the max9286 deserializer.
> 
> Implement route verification but do not take routing into consideration
> when configuring the CSI-2 output and pixel rate yet.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 88 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 5997fe40509f..54b4067168df 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -833,6 +833,90 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int max9286_routing_verify(struct max9286_priv *priv,
> +				  struct v4l2_subdev_krouting *routing)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	ret = v4l2_routing_simple_verify(routing);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Make sure all routes points to the single source pad which can have
> +	 * up to 4 streams. All routes shall start from a sink pad and shall not
> +	 * have more than one sink stream. The GMSL link for the sink has to be
> +	 * enabled.
> +	 */
> +	for (i = 0; i < routing->num_routes; ++i) {
> +		const struct v4l2_subdev_route *route = &routing->routes[i];
> +		struct max9286_source *source = &priv->sources[i];
> +
> +		if (route->source_pad != MAX9286_SRC_PAD ||
> +		    route->source_stream > 4) {
> +			dev_err(&priv->client->dev,
> +				"Invalid (%u,%u) source in route %u\n",
> +				route->source_pad, route->source_stream, i);
> +			return -EINVAL;
> +		}
> +
> +		if (route->sink_pad >= MAX9286_N_SINKS ||
> +		    route->sink_stream != 0) {
> +			dev_err(&priv->client->dev,
> +				"Invalid (%u,%u) sink in route %u\n",
> +				route->sink_pad, route->sink_stream, i);
> +			return -EINVAL;
> +		}
> +
> +		source = &priv->sources[route->sink_pad];
> +		if (!source->fwnode) {
> +			dev_err(&priv->client->dev,
> +				"Cannot set route for non-active source %u\n",
> +				route->sink_pad);
> +			return -EINVAL;
> +		}
> +	}

I have a patch on top of muxed streams v10 that will simplify this, I'll
post it shortly and CC you.

> +
> +	return 0;
> +}
> +
> +static int _max9286_set_routing(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_state *state,
> +				struct v4l2_subdev_krouting *routing)
> +{
> +	struct max9286_priv *priv = sd_to_max9286(sd);
> +	int ret;
> +
> +	ret = max9286_routing_verify(priv, routing);
> +	if (ret)
> +		return ret;
> +
> +	/* Re-initialize the format on a routing change. */
> +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing,
> +					       &max9286_default_format);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int max9286_set_routing(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_state *state,
> +			       enum v4l2_subdev_format_whence which,
> +			       struct v4l2_subdev_krouting *routing)
> +{
> +	struct max9286_priv *priv = sd_to_max9286(sd);
> +	unsigned int i;
> +	int ret;
> +
> +	state = v4l2_subdev_validate_and_lock_state(sd, state);
> +	ret = _max9286_set_routing(sd, state, routing);
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +
>  static int max9286_init_cfg(struct v4l2_subdev *sd,
>  			    struct v4l2_subdev_state *state)
>  {
> @@ -859,8 +943,7 @@ static int max9286_init_cfg(struct v4l2_subdev *sd,
>  	routing.routes = routes;
>  
>  	state = v4l2_subdev_validate_and_lock_state(sd, state);
> -	ret = v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
> -					       &max9286_default_format);
> +	ret = _max9286_set_routing(sd, state, &routing);
>  	v4l2_subdev_unlock_state(state);
>  
>  	return ret;
> @@ -875,6 +958,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
>  	.enum_mbus_code = max9286_enum_mbus_code,
>  	.get_fmt	= max9286_get_fmt,
>  	.set_fmt	= max9286_set_fmt,
> +	.set_routing	= max9286_set_routing,
>  };
>  
>  static const struct v4l2_subdev_ops max9286_subdev_ops = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 05/13] media: max9286: Move format to subdev state

[Laurent Pinchart]

Hi Jacopo,

On Thu, Dec 16, 2021 at 02:42:55PM +0200, Laurent Pinchart wrote:
> On Sun, Oct 17, 2021 at 08:24:41PM +0200, Jacopo Mondi wrote:
> > Move format handling to the v4l2_subdev state and store it per
> > (pad, stream) combination.
> > 
> > Now that the image format is stored in the subdev state, it can be
> > accessed through v4l2_subdev_get_fmt() instead of open-coding it.
> 
> Would it be possible to move this to 02/13 in the series ? Storing the
> formats in the state doesn't depend on multiplexed streams support, we
> could thus merge it early. The current 01/13 would need to be split in
> two, with one part to allocate the active state and implement
> .init_cfg() without muxed streams support, and another part to add muxed
> streams support on top.

It looks like not all patches required for this have been upstreamed yet
:-( Still, we may be able to merge the "V4L2 Subdev State" subset of the
muxed streams series sooner than latter, so moving this patch to the
beginning of the series may make sense.

> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 85 ++++++++++++-------------------------
> >  1 file changed, 27 insertions(+), 58 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 3485478f08a5..e51771d99437 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -175,8 +175,6 @@ struct max9286_priv {
> >  	struct v4l2_ctrl_handler ctrls;
> >  	struct v4l2_ctrl *pixelrate;
> >  
> > -	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > -
> >  	/* Protects controls and fmt structures */
> >  	struct mutex mutex;
> >  
> > @@ -829,28 +827,18 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > -static struct v4l2_mbus_framefmt *
> > -max9286_get_pad_format(struct max9286_priv *priv,
> > -		       struct v4l2_subdev_state *sd_state,
> > -		       unsigned int pad, u32 which)
> > -{
> > -	switch (which) {
> > -	case V4L2_SUBDEV_FORMAT_TRY:
> > -		return v4l2_subdev_get_try_format(&priv->sd, sd_state, pad);
> > -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > -		return &priv->fmt[pad];
> > -	default:
> > -		return NULL;
> > -	}
> > -}
> > -
> >  static int max9286_set_fmt(struct v4l2_subdev *sd,
> >  			   struct v4l2_subdev_state *sd_state,
> >  			   struct v4l2_subdev_format *format)
> >  {
> > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > -	struct v4l2_mbus_framefmt *cfg_fmt;
> > +	struct v4l2_mbus_framefmt *fmt;
> > +	struct v4l2_subdev_state *state;
> > +	int ret = 0;
> >  
> > +	/*
> > +	 * Refuse to set format on the multiplexed source pad.
> > +	 * Format is propagated from sinks streams to source streams.
> > +	 */
> >  	if (format->pad == MAX9286_SRC_PAD)
> >  		return -EINVAL;
> >  
> > @@ -866,44 +854,28 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> >  		break;
> >  	}
> >  
> > -	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
> > -					 format->which);
> > -	if (!cfg_fmt)
> > -		return -EINVAL;
> > -
> > -	mutex_lock(&priv->mutex);
> > -	*cfg_fmt = format->format;
> > -	mutex_unlock(&priv->mutex);
> > -
> > -	return 0;
> > -}
> > -
> > -static int max9286_get_fmt(struct v4l2_subdev *sd,
> > -			   struct v4l2_subdev_state *sd_state,
> > -			   struct v4l2_subdev_format *format)
> > -{
> > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > -	struct v4l2_mbus_framefmt *cfg_fmt;
> > -	unsigned int pad = format->pad;
> > -
> > -	/*
> > -	 * Multiplexed Stream Support: Support link validation by returning the
> > -	 * format of the first bound link. All links must have the same format,
> > -	 * as we do not support mixing and matching of cameras connected to the
> > -	 * max9286.
> > -	 */
> > -	if (pad == MAX9286_SRC_PAD)
> > -		pad = __ffs(priv->bound_sources);
> > +	state = v4l2_subdev_validate_and_lock_state(sd, sd_state);
> > +	fmt = v4l2_state_get_stream_format(state, format->pad,
> > +					   format->stream);
> > +	if (!fmt) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +	*fmt = format->format;
> >  
> > -	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
> > -	if (!cfg_fmt)
> > -		return -EINVAL;
> > +	/* Propagate format to the other end of the route. */
> > +	fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
> > +						    format->stream);
> > +	if (!fmt) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +	*fmt = format->format;
> >  
> > -	mutex_lock(&priv->mutex);
> > -	format->format = *cfg_fmt;
> > -	mutex_unlock(&priv->mutex);
> > +out:
> > +	v4l2_subdev_unlock_state(state);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int max9286_routing_verify(struct max9286_priv *priv,
> > @@ -1052,7 +1024,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
> >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> >  	.init_cfg	= max9286_init_cfg,
> >  	.enum_mbus_code = max9286_enum_mbus_code,
> > -	.get_fmt	= max9286_get_fmt,
> > +	.get_fmt	= v4l2_subdev_get_fmt,
> >  	.set_fmt	= max9286_set_fmt,
> >  	.set_routing	= max9286_set_routing,
> >  };
> > @@ -1092,9 +1064,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> >  
> >  	/* Configure V4L2 for the MAX9286 itself */
> >  
> > -	for (i = 0; i < MAX9286_N_SINKS; i++)
> > -		priv->fmt[i] = max9286_default_format;
> > -
> >  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
> >  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> >  			  V4L2_SUBDEV_FL_MULTIPLEXED;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 04/13] media: max9286: Use routes to configure link order

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Oct 17, 2021 at 08:24:40PM +0200, Jacopo Mondi wrote:
> Use the routing table to configure the link output order and link
> masking.
> 
> The link output order defines the CSI-2 virtual channel a GSML stream

s/GSML/GMSL/

> is output on. Configure ordering at stream start time and at chip
> setup time. This last step requires to move the chip initialization
> function after the V4L2 setup phase as it requires the subdev state from
> where the routing table is retrieved from to be initialized.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 103 ++++++++++++++++++++++--------------
>  1 file changed, 64 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 2e635179aec0..3485478f08a5 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -500,6 +500,61 @@ static int max9286_check_config_link(struct max9286_priv *priv,
>  	return 0;
>  }
>  
> +/*
> + * Configure the links output order (which defines on which CSI-2 VC a
> + * link is output on) and configure link masking.
> + */
> +static void max9286_config_links(struct max9286_priv *priv)
> +{
> +	const struct v4l2_subdev_krouting *routing;
> +	struct v4l2_subdev_state *state;
> +	u8 link_order = 0;
> +	u8 vc_mask = 0xf;
> +	unsigned int i;
> +
> +	state = v4l2_subdev_lock_active_state(&priv->sd);
> +	routing = &state->routing;
> +
> +	for (i = 0; i < routing->num_routes; ++i) {
> +		struct v4l2_subdev_route *route = &routing->routes[i];
> +
> +		if (!(priv->route_mask & BIT(i)))

Shouldn't this be BIT(route->sink_pad), has route_mask stores a bitmask
of sink pads (see max9286_set_routing()) ?

> +			continue;
> +
> +		/* Assign the CSI-2 VC using the source stream number. */
> +		link_order |= route->source_stream << (2 * route->sink_pad);
> +		vc_mask &= ~BIT(route->source_stream);
> +	}
> +
> +	/*
> +	 * This might look rather silly, but now that we have assigned a
> +	 * VC to the enabled routes, we have to assign one to the disabled
> +	 * routes as well, as there cannot be two sources with the same VC.
> +	 */
> +	for (i = 0; i < MAX9286_NUM_GMSL; ++i) {
> +		unsigned int vc;
> +
> +		if (priv->route_mask & BIT(i))
> +			continue;
> +
> +		/* ffs() counts from 1. */
> +		vc = ffs(vc_mask) - 1;
> +		link_order |= vc << (2 * i);
> +		vc_mask &= ~BIT(vc);
> +	}
> +
> +	/*
> +	 * Use the enabled routes to enable GMSL links, configure the CSI-2
> +	 * output order, mask unused links and autodetect link used as CSI
> +	 * clock source.
> +	 */
> +	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> +	max9286_write(priv, 0x0b, link_order);
> +	max9286_write(priv, 0x69, 0xf & ~priv->route_mask);
> +
> +	v4l2_subdev_unlock_state(state);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * V4L2 Subdev
>   */
> @@ -701,6 +756,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	int ret;
>  
>  	if (enable) {
> +		max9286_config_links(priv);
> +
>  		/*
>  		 * The frame sync between cameras is transmitted across the
>  		 * reverse channel as GPIO. We must open all channels while
> @@ -1109,32 +1166,6 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
>  
>  static int max9286_setup(struct max9286_priv *priv)
>  {
> -	/*
> -	 * Link ordering values for all enabled links combinations. Orders must
> -	 * be assigned sequentially from 0 to the number of enabled links
> -	 * without leaving any hole for disabled links. We thus assign orders to
> -	 * enabled links first, and use the remaining order values for disabled
> -	 * links are all links must have a different order value;
> -	 */
> -	static const u8 link_order[] = {
> -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxxx */
> -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxx0 */
> -		(3 << 6) | (2 << 4) | (0 << 2) | (1 << 0), /* xx0x */
> -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xx10 */
> -		(3 << 6) | (0 << 4) | (2 << 2) | (1 << 0), /* x0xx */
> -		(3 << 6) | (1 << 4) | (2 << 2) | (0 << 0), /* x1x0 */
> -		(3 << 6) | (1 << 4) | (0 << 2) | (2 << 0), /* x10x */
> -		(3 << 6) | (1 << 4) | (1 << 2) | (0 << 0), /* x210 */
> -		(0 << 6) | (3 << 4) | (2 << 2) | (1 << 0), /* 0xxx */
> -		(1 << 6) | (3 << 4) | (2 << 2) | (0 << 0), /* 1xx0 */
> -		(1 << 6) | (3 << 4) | (0 << 2) | (2 << 0), /* 1x0x */
> -		(2 << 6) | (3 << 4) | (1 << 2) | (0 << 0), /* 2x10 */
> -		(1 << 6) | (0 << 4) | (3 << 2) | (2 << 0), /* 10xx */
> -		(2 << 6) | (1 << 4) | (3 << 2) | (0 << 0), /* 21x0 */
> -		(2 << 6) | (1 << 4) | (0 << 2) | (3 << 0), /* 210x */
> -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* 3210 */
> -	};
> -
>  	/*
>  	 * Set the I2C bus speed.
>  	 *
> @@ -1144,13 +1175,7 @@ static int max9286_setup(struct max9286_priv *priv)
>  	max9286_configure_i2c(priv, true);
>  	max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
>  
> -	/*
> -	 * Enable GMSL links, mask unused ones and autodetect link
> -	 * used as CSI clock source.
> -	 */
> -	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> -	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
> -	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
> +	max9286_config_links(priv);

We could actually skip this, and only call

	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);

to enable the links. The link order and link masking is only needed when
starting the streams, which is done by calling max9286_config_links() in
max9286_s_stream().

>  
>  	/*
>  	 * Video format setup:
> @@ -1324,12 +1349,6 @@ static int max9286_init(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = max9286_setup(priv);
> -	if (ret) {
> -		dev_err(dev, "Unable to setup max9286\n");
> -		goto err_poc_disable;
> -	}
> -
>  	/*
>  	 * Register all V4L2 interactions for the MAX9286 and notifiers for
>  	 * any subdevices connected.
> @@ -1340,6 +1359,12 @@ static int max9286_init(struct device *dev)
>  		goto err_poc_disable;
>  	}
>  
> +	ret = max9286_setup(priv);
> +	if (ret) {
> +		dev_err(dev, "Unable to setup max9286\n");
> +		goto err_poc_disable;
> +	}
> +

And with the above change, I think you can keep the max9286_setup() call
where it was.

>  	ret = max9286_i2c_mux_init(priv);
>  	if (ret) {
>  		dev_err(dev, "Unable to initialize I2C multiplexer\n");

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 04/13] media: max9286: Use routes to configure link order

[Laurent Pinchart]

Hi Jacopo,

On Sat, Jan 22, 2022 at 03:14:33AM +0200, Laurent Pinchart wrote:
> On Sun, Oct 17, 2021 at 08:24:40PM +0200, Jacopo Mondi wrote:
> > Use the routing table to configure the link output order and link
> > masking.
> > 
> > The link output order defines the CSI-2 virtual channel a GSML stream
> 
> s/GSML/GMSL/
> 
> > is output on. Configure ordering at stream start time and at chip
> > setup time. This last step requires to move the chip initialization
> > function after the V4L2 setup phase as it requires the subdev state from
> > where the routing table is retrieved from to be initialized.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/max9286.c | 103 ++++++++++++++++++++++--------------
> >  1 file changed, 64 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 2e635179aec0..3485478f08a5 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -500,6 +500,61 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Configure the links output order (which defines on which CSI-2 VC a
> > + * link is output on) and configure link masking.
> > + */
> > +static void max9286_config_links(struct max9286_priv *priv)
> > +{
> > +	const struct v4l2_subdev_krouting *routing;
> > +	struct v4l2_subdev_state *state;
> > +	u8 link_order = 0;
> > +	u8 vc_mask = 0xf;
> > +	unsigned int i;
> > +
> > +	state = v4l2_subdev_lock_active_state(&priv->sd);
> > +	routing = &state->routing;
> > +
> > +	for (i = 0; i < routing->num_routes; ++i) {
> > +		struct v4l2_subdev_route *route = &routing->routes[i];

And I think we should also ignore disabled routes, so using

	for_each_active_route(&state->routing, route) {

is likely better too.

> > +
> > +		if (!(priv->route_mask & BIT(i)))
> 
> Shouldn't this be BIT(route->sink_pad), has route_mask stores a bitmask
> of sink pads (see max9286_set_routing()) ?
> 
> > +			continue;
> > +
> > +		/* Assign the CSI-2 VC using the source stream number. */
> > +		link_order |= route->source_stream << (2 * route->sink_pad);
> > +		vc_mask &= ~BIT(route->source_stream);
> > +	}
> > +
> > +	/*
> > +	 * This might look rather silly, but now that we have assigned a
> > +	 * VC to the enabled routes, we have to assign one to the disabled
> > +	 * routes as well, as there cannot be two sources with the same VC.
> > +	 */
> > +	for (i = 0; i < MAX9286_NUM_GMSL; ++i) {
> > +		unsigned int vc;
> > +
> > +		if (priv->route_mask & BIT(i))
> > +			continue;
> > +
> > +		/* ffs() counts from 1. */
> > +		vc = ffs(vc_mask) - 1;
> > +		link_order |= vc << (2 * i);
> > +		vc_mask &= ~BIT(vc);
> > +	}
> > +
> > +	/*
> > +	 * Use the enabled routes to enable GMSL links, configure the CSI-2
> > +	 * output order, mask unused links and autodetect link used as CSI
> > +	 * clock source.
> > +	 */
> > +	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> > +	max9286_write(priv, 0x0b, link_order);
> > +	max9286_write(priv, 0x69, 0xf & ~priv->route_mask);
> > +
> > +	v4l2_subdev_unlock_state(state);
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * V4L2 Subdev
> >   */
> > @@ -701,6 +756,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  	int ret;
> >  
> >  	if (enable) {
> > +		max9286_config_links(priv);
> > +
> >  		/*
> >  		 * The frame sync between cameras is transmitted across the
> >  		 * reverse channel as GPIO. We must open all channels while
> > @@ -1109,32 +1166,6 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
> >  
> >  static int max9286_setup(struct max9286_priv *priv)
> >  {
> > -	/*
> > -	 * Link ordering values for all enabled links combinations. Orders must
> > -	 * be assigned sequentially from 0 to the number of enabled links
> > -	 * without leaving any hole for disabled links. We thus assign orders to
> > -	 * enabled links first, and use the remaining order values for disabled
> > -	 * links are all links must have a different order value;
> > -	 */
> > -	static const u8 link_order[] = {
> > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxxx */
> > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxx0 */
> > -		(3 << 6) | (2 << 4) | (0 << 2) | (1 << 0), /* xx0x */
> > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xx10 */
> > -		(3 << 6) | (0 << 4) | (2 << 2) | (1 << 0), /* x0xx */
> > -		(3 << 6) | (1 << 4) | (2 << 2) | (0 << 0), /* x1x0 */
> > -		(3 << 6) | (1 << 4) | (0 << 2) | (2 << 0), /* x10x */
> > -		(3 << 6) | (1 << 4) | (1 << 2) | (0 << 0), /* x210 */
> > -		(0 << 6) | (3 << 4) | (2 << 2) | (1 << 0), /* 0xxx */
> > -		(1 << 6) | (3 << 4) | (2 << 2) | (0 << 0), /* 1xx0 */
> > -		(1 << 6) | (3 << 4) | (0 << 2) | (2 << 0), /* 1x0x */
> > -		(2 << 6) | (3 << 4) | (1 << 2) | (0 << 0), /* 2x10 */
> > -		(1 << 6) | (0 << 4) | (3 << 2) | (2 << 0), /* 10xx */
> > -		(2 << 6) | (1 << 4) | (3 << 2) | (0 << 0), /* 21x0 */
> > -		(2 << 6) | (1 << 4) | (0 << 2) | (3 << 0), /* 210x */
> > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* 3210 */
> > -	};
> > -
> >  	/*
> >  	 * Set the I2C bus speed.
> >  	 *
> > @@ -1144,13 +1175,7 @@ static int max9286_setup(struct max9286_priv *priv)
> >  	max9286_configure_i2c(priv, true);
> >  	max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
> >  
> > -	/*
> > -	 * Enable GMSL links, mask unused ones and autodetect link
> > -	 * used as CSI clock source.
> > -	 */
> > -	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> > -	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
> > -	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
> > +	max9286_config_links(priv);
> 
> We could actually skip this, and only call
> 
> 	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> 
> to enable the links. The link order and link masking is only needed when
> starting the streams, which is done by calling max9286_config_links() in
> max9286_s_stream().
> 
> >  
> >  	/*
> >  	 * Video format setup:
> > @@ -1324,12 +1349,6 @@ static int max9286_init(struct device *dev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = max9286_setup(priv);
> > -	if (ret) {
> > -		dev_err(dev, "Unable to setup max9286\n");
> > -		goto err_poc_disable;
> > -	}
> > -
> >  	/*
> >  	 * Register all V4L2 interactions for the MAX9286 and notifiers for
> >  	 * any subdevices connected.
> > @@ -1340,6 +1359,12 @@ static int max9286_init(struct device *dev)
> >  		goto err_poc_disable;
> >  	}
> >  
> > +	ret = max9286_setup(priv);
> > +	if (ret) {
> > +		dev_err(dev, "Unable to setup max9286\n");
> > +		goto err_poc_disable;
> > +	}
> > +
> 
> And with the above change, I think you can keep the max9286_setup() call
> where it was.
> 
> >  	ret = max9286_i2c_mux_init(priv);
> >  	if (ret) {
> >  		dev_err(dev, "Unable to initialize I2C multiplexer\n");

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 03/13] media: max9286: Use enabled routes to calculate pixelrate

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Oct 17, 2021 at 08:24:39PM +0200, Jacopo Mondi wrote:
> Now that the device supports routing, use the enabled routes instead
> of the connected sources to compute the output pixel rate.
> 
> To do so, update the route_mask after a set_routing() call with the
> ACTIVE format, and re-calculate the pixel rate just after.
> 
> At the same time, start and stop only the enabled routes at s_stream()
> time.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 63 ++++++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 54b4067168df..2e635179aec0 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -189,8 +189,11 @@ struct max9286_priv {
>  	struct v4l2_async_notifier notifier;
>  };
>  
> +#define to_index(priv, source) ((source) - &(priv)->sources[0])
> +
>  static struct max9286_source *next_source(struct max9286_priv *priv,
> -					  struct max9286_source *source)
> +					  struct max9286_source *source,
> +					  bool routed)
>  {
>  	if (!source)
>  		source = &priv->sources[0];
> @@ -198,17 +201,27 @@ static struct max9286_source *next_source(struct max9286_priv *priv,
>  		source++;
>  
>  	for (; source < &priv->sources[MAX9286_NUM_GMSL]; source++) {
> -		if (source->fwnode)
> +		unsigned int index = to_index(priv, source);
> +
> +		if (!source->fwnode)
> +			continue;
> +
> +		/*
> +		 * Careful here! A very unfortunate call to set_routing() can
> +		 * change priv->route_mask behind our back!
> +		 */
> +		if (!routed || priv->route_mask & BIT(index))
>  			return source;
>  	}
>  
>  	return NULL;
>  }
>  
> -#define for_each_source(priv, source) \
> -	for ((source) = NULL; ((source) = next_source((priv), (source))); )
> +#define for_each_route(priv, source) \
> +	for ((source) = NULL; ((source) = next_source((priv), (source), true)); )

Could we call this for_each_active_source() ? There's a
for_each_active_route() macro in include/media/v4l2-subdev.h, so
defining for_each_route() here is confusing. Furthermore, the macro
iterates over sources, not routes.

The routed parameter to next_source() could be renamed accordingly to
active.

>  
> -#define to_index(priv, source) ((source) - &(priv)->sources[0])
> +#define for_each_source(priv, source) \
> +	for ((source) = NULL; ((source) = next_source((priv), (source), false)); )
>  
>  static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
>  {
> @@ -410,7 +423,7 @@ static int max9286_check_video_links(struct max9286_priv *priv)
>  		if (ret < 0)
>  			return -EIO;
>  
> -		if ((ret & MAX9286_VIDEO_DETECT_MASK) == priv->source_mask)
> +		if ((ret & MAX9286_VIDEO_DETECT_MASK) == priv->route_mask)
>  			break;
>  
>  		usleep_range(350, 500);
> @@ -494,9 +507,10 @@ static int max9286_check_config_link(struct max9286_priv *priv,
>  static int max9286_set_pixelrate(struct max9286_priv *priv)
>  {
>  	struct max9286_source *source = NULL;
> +	unsigned int num_routes = 0;
>  	u64 pixelrate = 0;
>  
> -	for_each_source(priv, source) {
> +	for_each_route(priv, source) {
>  		struct v4l2_ctrl *ctrl;
>  		u64 source_rate = 0;
>  
> @@ -517,6 +531,8 @@ static int max9286_set_pixelrate(struct max9286_priv *priv)
>  				"Unable to calculate pixel rate\n");
>  			return -EINVAL;
>  		}
> +
> +		num_routes++;
>  	}
>  
>  	if (!pixelrate) {
> @@ -530,7 +546,7 @@ static int max9286_set_pixelrate(struct max9286_priv *priv)
>  	 * by the number of available sources.
>  	 */
>  	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate,
> -				      pixelrate * priv->nsources);
> +				      pixelrate * num_routes);
>  }
>  
>  static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> @@ -692,8 +708,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  		 */
>  		max9286_i2c_mux_open(priv);
>  
> -		/* Start all cameras. */
> -		for_each_source(priv, source) {
> +		/* Start cameras. */
> +		for_each_route(priv, source) {
>  			ret = v4l2_subdev_call(source->sd, video, s_stream, 1);
>  			if (ret)
>  				return ret;
> @@ -734,8 +750,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	} else {
>  		max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
>  
> -		/* Stop all cameras. */
> -		for_each_source(priv, source)
> +		/* Stop cameras. */
> +		for_each_route(priv, source)
>  			v4l2_subdev_call(source->sd, video, s_stream, 0);
>  
>  		max9286_i2c_mux_close(priv);
> @@ -912,6 +928,29 @@ static int max9286_set_routing(struct v4l2_subdev *sd,
>  
>  	state = v4l2_subdev_validate_and_lock_state(sd, state);
>  	ret = _max9286_set_routing(sd, state, routing);
> +	if (ret)
> +		goto out;
> +
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> +		goto out;
> +
> +	/*
> +	 * Update the active routes mask and the pixel rate according to the
> +	 * routed sources.
> +	 */
> +	priv->route_mask = 0;
> +	for (i = 0; i < routing->num_routes; ++i) {
> +		struct v4l2_subdev_route *route = &routing->routes[i];
> +
> +		if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> +			continue;
> +
> +		priv->route_mask |= BIT(route->sink_pad);
> +	}
> +
> +	max9286_set_pixelrate(priv);
> +
> +out:
>  	v4l2_subdev_unlock_state(state);
>  
>  	return ret;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 04/13] media: max9286: Use routes to configure link order

[Laurent Pinchart]

Hi Jacopo,

Thinking even more about this.

On Sat, Jan 22, 2022 at 03:23:19AM +0200, Laurent Pinchart wrote:
> On Sat, Jan 22, 2022 at 03:14:33AM +0200, Laurent Pinchart wrote:
> > On Sun, Oct 17, 2021 at 08:24:40PM +0200, Jacopo Mondi wrote:
> > > Use the routing table to configure the link output order and link
> > > masking.
> > > 
> > > The link output order defines the CSI-2 virtual channel a GSML stream
> > 
> > s/GSML/GMSL/
> > 
> > > is output on. Configure ordering at stream start time and at chip
> > > setup time. This last step requires to move the chip initialization
> > > function after the V4L2 setup phase as it requires the subdev state from
> > > where the routing table is retrieved from to be initialized.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/max9286.c | 103 ++++++++++++++++++++++--------------
> > >  1 file changed, 64 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index 2e635179aec0..3485478f08a5 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -500,6 +500,61 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> > >  	return 0;
> > >  }
> > >  
> > > +/*
> > > + * Configure the links output order (which defines on which CSI-2 VC a
> > > + * link is output on) and configure link masking.
> > > + */
> > > +static void max9286_config_links(struct max9286_priv *priv)
> > > +{
> > > +	const struct v4l2_subdev_krouting *routing;
> > > +	struct v4l2_subdev_state *state;
> > > +	u8 link_order = 0;
> > > +	u8 vc_mask = 0xf;
> > > +	unsigned int i;
> > > +
> > > +	state = v4l2_subdev_lock_active_state(&priv->sd);
> > > +	routing = &state->routing;
> > > +
> > > +	for (i = 0; i < routing->num_routes; ++i) {
> > > +		struct v4l2_subdev_route *route = &routing->routes[i];
> 
> And I think we should also ignore disabled routes, so using
> 
> 	for_each_active_route(&state->routing, route) {
> 
> is likely better too.
> 
> > > +
> > > +		if (!(priv->route_mask & BIT(i)))
> > 
> > Shouldn't this be BIT(route->sink_pad), has route_mask stores a bitmask
> > of sink pads (see max9286_set_routing()) ?
> > 
> > > +			continue;
> > > +
> > > +		/* Assign the CSI-2 VC using the source stream number. */

Is this even correct ? The source stream number is arbitrary selected by
userspace, and the MAX9286 requires (at least according to the comment
in max9286_setup()) to assign orders sequentially from 0 without any
hole. This code doesn't seem to guarantee that. Should we instead move
the link_order table to this function and index it with priv->route_mask
?

> > > +		link_order |= route->source_stream << (2 * route->sink_pad);
> > > +		vc_mask &= ~BIT(route->source_stream);
> > > +	}
> > > +
> > > +	/*
> > > +	 * This might look rather silly, but now that we have assigned a
> > > +	 * VC to the enabled routes, we have to assign one to the disabled
> > > +	 * routes as well, as there cannot be two sources with the same VC.
> > > +	 */
> > > +	for (i = 0; i < MAX9286_NUM_GMSL; ++i) {
> > > +		unsigned int vc;
> > > +
> > > +		if (priv->route_mask & BIT(i))
> > > +			continue;
> > > +
> > > +		/* ffs() counts from 1. */
> > > +		vc = ffs(vc_mask) - 1;
> > > +		link_order |= vc << (2 * i);
> > > +		vc_mask &= ~BIT(vc);
> > > +	}

This can then be dropped.

The max9286_get_frame_desc() function will need to be updated
accordingly.

> > > +
> > > +	/*
> > > +	 * Use the enabled routes to enable GMSL links, configure the CSI-2
> > > +	 * output order, mask unused links and autodetect link used as CSI
> > > +	 * clock source.
> > > +	 */
> > > +	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> > > +	max9286_write(priv, 0x0b, link_order);
> > > +	max9286_write(priv, 0x69, 0xf & ~priv->route_mask);
> > > +
> > > +	v4l2_subdev_unlock_state(state);
> > > +}
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * V4L2 Subdev
> > >   */
> > > @@ -701,6 +756,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > >  	int ret;
> > >  
> > >  	if (enable) {
> > > +		max9286_config_links(priv);
> > > +
> > >  		/*
> > >  		 * The frame sync between cameras is transmitted across the
> > >  		 * reverse channel as GPIO. We must open all channels while
> > > @@ -1109,32 +1166,6 @@ static void max9286_v4l2_unregister(struct max9286_priv *priv)
> > >  
> > >  static int max9286_setup(struct max9286_priv *priv)
> > >  {
> > > -	/*
> > > -	 * Link ordering values for all enabled links combinations. Orders must
> > > -	 * be assigned sequentially from 0 to the number of enabled links
> > > -	 * without leaving any hole for disabled links. We thus assign orders to
> > > -	 * enabled links first, and use the remaining order values for disabled
> > > -	 * links are all links must have a different order value;
> > > -	 */
> > > -	static const u8 link_order[] = {
> > > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxxx */
> > > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xxx0 */
> > > -		(3 << 6) | (2 << 4) | (0 << 2) | (1 << 0), /* xx0x */
> > > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* xx10 */
> > > -		(3 << 6) | (0 << 4) | (2 << 2) | (1 << 0), /* x0xx */
> > > -		(3 << 6) | (1 << 4) | (2 << 2) | (0 << 0), /* x1x0 */
> > > -		(3 << 6) | (1 << 4) | (0 << 2) | (2 << 0), /* x10x */
> > > -		(3 << 6) | (1 << 4) | (1 << 2) | (0 << 0), /* x210 */
> > > -		(0 << 6) | (3 << 4) | (2 << 2) | (1 << 0), /* 0xxx */
> > > -		(1 << 6) | (3 << 4) | (2 << 2) | (0 << 0), /* 1xx0 */
> > > -		(1 << 6) | (3 << 4) | (0 << 2) | (2 << 0), /* 1x0x */
> > > -		(2 << 6) | (3 << 4) | (1 << 2) | (0 << 0), /* 2x10 */
> > > -		(1 << 6) | (0 << 4) | (3 << 2) | (2 << 0), /* 10xx */
> > > -		(2 << 6) | (1 << 4) | (3 << 2) | (0 << 0), /* 21x0 */
> > > -		(2 << 6) | (1 << 4) | (0 << 2) | (3 << 0), /* 210x */
> > > -		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* 3210 */
> > > -	};
> > > -
> > >  	/*
> > >  	 * Set the I2C bus speed.
> > >  	 *
> > > @@ -1144,13 +1175,7 @@ static int max9286_setup(struct max9286_priv *priv)
> > >  	max9286_configure_i2c(priv, true);
> > >  	max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
> > >  
> > > -	/*
> > > -	 * Enable GMSL links, mask unused ones and autodetect link
> > > -	 * used as CSI clock source.
> > > -	 */
> > > -	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> > > -	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
> > > -	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
> > > +	max9286_config_links(priv);
> > 
> > We could actually skip this, and only call
> > 
> > 	max9286_write(priv, 0x00, MAX9286_MSTLINKSEL_AUTO | priv->route_mask);
> > 
> > to enable the links. The link order and link masking is only needed when
> > starting the streams, which is done by calling max9286_config_links() in
> > max9286_s_stream().
> > 
> > >  
> > >  	/*
> > >  	 * Video format setup:
> > > @@ -1324,12 +1349,6 @@ static int max9286_init(struct device *dev)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = max9286_setup(priv);
> > > -	if (ret) {
> > > -		dev_err(dev, "Unable to setup max9286\n");
> > > -		goto err_poc_disable;
> > > -	}
> > > -
> > >  	/*
> > >  	 * Register all V4L2 interactions for the MAX9286 and notifiers for
> > >  	 * any subdevices connected.
> > > @@ -1340,6 +1359,12 @@ static int max9286_init(struct device *dev)
> > >  		goto err_poc_disable;
> > >  	}
> > >  
> > > +	ret = max9286_setup(priv);
> > > +	if (ret) {
> > > +		dev_err(dev, "Unable to setup max9286\n");
> > > +		goto err_poc_disable;
> > > +	}
> > > +
> > 
> > And with the above change, I think you can keep the max9286_setup() call
> > where it was.
> > 
> > >  	ret = max9286_i2c_mux_init(priv);
> > >  	if (ret) {
> > >  		dev_err(dev, "Unable to initialize I2C multiplexer\n");

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 08/13] media: rcar-csi2: Add support for v4l2_subdev_state

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Oct 17, 2021 at 08:24:44PM +0200, Jacopo Mondi wrote:
> Create and initialize the v4l2_subdev_state for the R-Car CSI-2 receiver
> rder to prepare to support routing operations and multiplexed streams.

s/rder/in order/ ?

> 
> Create the subdevice state with v4l2_subdev_init_finalize() and
> implement the init_cfg() operation to guarantee the state is initialized
> correctly with a set of default routes.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 68 ++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index e28eff039688..a74087b49e71 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -752,11 +752,65 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int rcsi2_init_cfg(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_state *state)

This could be moved before rcsi2_set_pad_format() to match the order in
rcar_csi2_pad_ops. Up to you.

> +{
> +	/* Initialize 4 routes from each source pad to the single sink pad. */
> +	struct v4l2_subdev_route routes[] = {
> +		{
> +			.sink_pad = RCAR_CSI2_SINK,
> +			.sink_stream = 0,
> +			.source_pad = RCAR_CSI2_SOURCE_VC0,
> +			.source_stream = 0,
> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> +		},
> +		{
> +			.sink_pad = RCAR_CSI2_SINK,
> +			.sink_stream = 1,
> +			.source_pad = RCAR_CSI2_SOURCE_VC1,
> +			.source_stream = 0,
> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> +		},
> +		{
> +			.sink_pad = RCAR_CSI2_SINK,
> +			.sink_stream = 2,
> +			.source_pad = RCAR_CSI2_SOURCE_VC2,
> +			.source_stream = 0,
> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> +		},
> +		{
> +			.sink_pad = RCAR_CSI2_SINK,
> +			.sink_stream = 3,
> +			.source_pad = RCAR_CSI2_SOURCE_VC3,
> +			.source_stream = 0,
> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> +		},
> +	};
> +
> +	struct v4l2_subdev_krouting routing = {
> +		.num_routes = ARRAY_SIZE(routes),
> +		.routes = routes,
> +	};
> +
> +	int ret = v4l2_routing_simple_verify(&routing);
> +	if (ret)
> +		return ret;
> +
> +	state = v4l2_subdev_validate_and_lock_state(sd, state);
> +
> +	ret = v4l2_subdev_set_routing(sd, state, &routing);
> +
> +	v4l2_subdev_unlock_state(state);

I would squash this with 09/13 to avoid this intermediate state of
dealing with routes manually in the .init_cfg() operation. The patch
otherwise looks good to me.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	return ret;
> +}
> +
>  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
>  	.s_stream = rcsi2_s_stream,
>  };
>  
>  static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
> +	.init_cfg = rcsi2_init_cfg,
>  	.set_fmt = rcsi2_set_pad_format,
>  	.get_fmt = rcsi2_get_pad_format,
>  };
> @@ -1260,7 +1314,8 @@ static int rcsi2_probe(struct platform_device *pdev)
>  	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
>  	snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
>  		 KBUILD_MODNAME, dev_name(&pdev->dev));
> -	priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE |
> +			     V4L2_SUBDEV_FL_MULTIPLEXED;
>  
>  	priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
>  	priv->subdev.entity.ops = &rcar_csi2_entity_ops;
> @@ -1276,14 +1331,22 @@ static int rcsi2_probe(struct platform_device *pdev)
>  
>  	pm_runtime_enable(&pdev->dev);
>  
> +	ret = v4l2_subdev_init_finalize(&priv->subdev);
> +	if (ret)
> +		goto error_pm;
> +
>  	ret = v4l2_async_register_subdev(&priv->subdev);
>  	if (ret < 0)
> -		goto error;
> +		goto error_subdev;
>  
>  	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
>  
>  	return 0;
>  
> +error_subdev:
> +	v4l2_subdev_cleanup(&priv->subdev);
> +error_pm:
> +	pm_runtime_disable(&pdev->dev);
>  error:
>  	v4l2_async_notifier_unregister(&priv->notifier);
>  	v4l2_async_notifier_cleanup(&priv->notifier);
> @@ -1298,6 +1361,7 @@ static int rcsi2_remove(struct platform_device *pdev)
>  	v4l2_async_notifier_unregister(&priv->notifier);
>  	v4l2_async_notifier_cleanup(&priv->notifier);
>  	v4l2_async_unregister_subdev(&priv->subdev);
> +	v4l2_subdev_cleanup(&priv->subdev);
>  
>  	pm_runtime_disable(&pdev->dev);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 09/13] media: rcar-csi2: Implement set_routing

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Oct 17, 2021 at 08:24:45PM +0200, Jacopo Mondi wrote:
> Add the set_routing() subdev operation to allow userspace to configure
> routing on the R-Car CSI-2 receiver.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 40 +++++++++++++++------
>  1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index a74087b49e71..451a6da26e03 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -752,6 +752,33 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int _rcsi2_set_routing(struct v4l2_subdev *sd,

__ is more customary.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +			      struct v4l2_subdev_state *state,
> +			      struct v4l2_subdev_krouting *routing)
> +{
> +	int ret;
> +
> +	ret = v4l2_routing_simple_verify(routing);
> +	if (ret)
> +		return ret;
> +
> +	state = v4l2_subdev_validate_and_lock_state(sd, state);
> +
> +	ret = v4l2_subdev_set_routing(sd, state, routing);
> +
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +
> +static int rcsi2_set_routing(struct v4l2_subdev *sd,
> +			     struct v4l2_subdev_state *state,
> +			     enum v4l2_subdev_format_whence which,
> +			     struct v4l2_subdev_krouting *routing)
> +{
> +	return _rcsi2_set_routing(sd, state, routing);
> +}
> +
>  static int rcsi2_init_cfg(struct v4l2_subdev *sd,
>  			  struct v4l2_subdev_state *state)
>  {
> @@ -792,17 +819,7 @@ static int rcsi2_init_cfg(struct v4l2_subdev *sd,
>  		.routes = routes,
>  	};
>  
> -	int ret = v4l2_routing_simple_verify(&routing);
> -	if (ret)
> -		return ret;
> -
> -	state = v4l2_subdev_validate_and_lock_state(sd, state);
> -
> -	ret = v4l2_subdev_set_routing(sd, state, &routing);
> -
> -	v4l2_subdev_unlock_state(state);
> -
> -	return ret;
> +	return _rcsi2_set_routing(sd, state, &routing);
>  }
>  
>  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> @@ -813,6 +830,7 @@ static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
>  	.init_cfg = rcsi2_init_cfg,
>  	.set_fmt = rcsi2_set_pad_format,
>  	.get_fmt = rcsi2_get_pad_format,
> +	.set_routing = rcsi2_set_routing,
>  };
>  
>  static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 10/13] media: rcar-csi2: Move format to subdev state

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Oct 17, 2021 at 08:24:46PM +0200, Jacopo Mondi wrote:
> Move format handling to the v4l2_subdev state and store it per
> (pad, stream) combination.
> 
> Now that the image format is stored in the subdev state, it can be
> accessed through v4l2_subdev_get_fmt() instead of open-coding it.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 50 ++++++++++++---------
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 451a6da26e03..b60845b1e563 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -722,34 +722,42 @@ static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
>  				struct v4l2_subdev_state *sd_state,
>  				struct v4l2_subdev_format *format)
>  {
> -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> -	struct v4l2_mbus_framefmt *framefmt;
> +	struct v4l2_mbus_framefmt *fmt;
> +	struct v4l2_subdev_state *state;
> +	int ret = 0;
>  
>  	if (!rcsi2_code_to_fmt(format->format.code))
>  		format->format.code = rcar_csi2_formats[0].code;
>  
> -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -		priv->mf = format->format;
> -	} else {
> -		framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> -		*framefmt = format->format;
> -	}
> +	/*
> +	 * Format is propagated from sink streams to source streams, so
> +	 * disallow setting format on the source pads.
> +	 */
> +	if (format->pad > RCAR_CSI2_SINK)
> +		return -EINVAL;

You should return v4l2_subdev_get_fmt() instead.

>  
> -	return 0;
> -}
> +	state = v4l2_subdev_validate_and_lock_state(sd, sd_state);

With v10 of the streams series I think you can use
v4l2_subdev_lock_state().

> +	fmt = v4l2_state_get_stream_format(state, format->pad,
> +					   format->stream);
> +	if (!fmt) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	*fmt = format->format;

I would get the pointers to both formats before updating any of them, to
avoid a partial update in case of error:

	sink_fmt = v4l2_state_get_stream_format(state, format->pad,
						format->stream);
	source_fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
							   format->stream);
	if (!sink_fmt || !source_fmt) {
		ret = -EINVAL;
		goto out;
	}

	*sink_fmt = format->format;
	*source_fmt = format->format;

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
> -static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> -				struct v4l2_subdev_state *sd_state,
> -				struct v4l2_subdev_format *format)
> -{
> -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +	/* Propagate format to the other end of the route. */
> +	fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
> +						    format->stream);
> +	if (!fmt) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	*fmt = format->format;
>  
> -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		format->format = priv->mf;
> -	else
> -		format->format = *v4l2_subdev_get_try_format(sd, sd_state, 0);
> +out:
> +	v4l2_subdev_unlock_state(state);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int _rcsi2_set_routing(struct v4l2_subdev *sd,
> @@ -829,7 +837,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
>  static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
>  	.init_cfg = rcsi2_init_cfg,
>  	.set_fmt = rcsi2_set_pad_format,
> -	.get_fmt = rcsi2_get_pad_format,
> +	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_routing = rcsi2_set_routing,
>  };
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 10/13] media: rcar-csi2: Move format to subdev state

[Laurent Pinchart]

On Sun, Jan 23, 2022 at 04:19:16PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Sun, Oct 17, 2021 at 08:24:46PM +0200, Jacopo Mondi wrote:
> > Move format handling to the v4l2_subdev state and store it per
> > (pad, stream) combination.
> > 
> > Now that the image format is stored in the subdev state, it can be
> > accessed through v4l2_subdev_get_fmt() instead of open-coding it.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 50 ++++++++++++---------
> >  1 file changed, 29 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 451a6da26e03..b60845b1e563 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -722,34 +722,42 @@ static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
> >  				struct v4l2_subdev_state *sd_state,
> >  				struct v4l2_subdev_format *format)
> >  {
> > -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > -	struct v4l2_mbus_framefmt *framefmt;
> > +	struct v4l2_mbus_framefmt *fmt;
> > +	struct v4l2_subdev_state *state;
> > +	int ret = 0;
> >  
> >  	if (!rcsi2_code_to_fmt(format->format.code))
> >  		format->format.code = rcar_csi2_formats[0].code;
> >  
> > -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > -		priv->mf = format->format;
> > -	} else {
> > -		framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> > -		*framefmt = format->format;
> > -	}
> > +	/*
> > +	 * Format is propagated from sink streams to source streams, so
> > +	 * disallow setting format on the source pads.
> > +	 */
> > +	if (format->pad > RCAR_CSI2_SINK)
> > +		return -EINVAL;
> 
> You should return v4l2_subdev_get_fmt() instead.
> 
> >  
> > -	return 0;
> > -}
> > +	state = v4l2_subdev_validate_and_lock_state(sd, sd_state);
> 
> With v10 of the streams series I think you can use
> v4l2_subdev_lock_state().
> 
> > +	fmt = v4l2_state_get_stream_format(state, format->pad,
> > +					   format->stream);
> > +	if (!fmt) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +	*fmt = format->format;
> 
> I would get the pointers to both formats before updating any of them, to
> avoid a partial update in case of error:
> 
> 	sink_fmt = v4l2_state_get_stream_format(state, format->pad,
> 						format->stream);
> 	source_fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
> 							   format->stream);
> 	if (!sink_fmt || !source_fmt) {
> 		ret = -EINVAL;
> 		goto out;
> 	}
> 
> 	*sink_fmt = format->format;
> 	*source_fmt = format->format;
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Actually, the rest of the driver still uses priv->mf, so this patch will
cause a breakage. You can squash it with 11/13.

> >  
> > -static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> > -				struct v4l2_subdev_state *sd_state,
> > -				struct v4l2_subdev_format *format)
> > -{
> > -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +	/* Propagate format to the other end of the route. */
> > +	fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
> > +						    format->stream);
> > +	if (!fmt) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +	*fmt = format->format;
> >  
> > -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > -		format->format = priv->mf;
> > -	else
> > -		format->format = *v4l2_subdev_get_try_format(sd, sd_state, 0);
> > +out:
> > +	v4l2_subdev_unlock_state(state);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int _rcsi2_set_routing(struct v4l2_subdev *sd,
> > @@ -829,7 +837,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> >  static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
> >  	.init_cfg = rcsi2_init_cfg,
> >  	.set_fmt = rcsi2_set_pad_format,
> > -	.get_fmt = rcsi2_get_pad_format,
> > +	.get_fmt = v4l2_subdev_get_fmt,
> >  	.set_routing = rcsi2_set_routing,
> >  };
> >  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 10/13] media: rcar-csi2: Move format to subdev state

[Laurent Pinchart]

On Sun, Jan 23, 2022 at 04:20:01PM +0200, Laurent Pinchart wrote:
> On Sun, Jan 23, 2022 at 04:19:16PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> > 
> > Thank you for the patch.
> > 
> > On Sun, Oct 17, 2021 at 08:24:46PM +0200, Jacopo Mondi wrote:
> > > Move format handling to the v4l2_subdev state and store it per
> > > (pad, stream) combination.
> > > 
> > > Now that the image format is stored in the subdev state, it can be
> > > accessed through v4l2_subdev_get_fmt() instead of open-coding it.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 50 ++++++++++++---------
> > >  1 file changed, 29 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > index 451a6da26e03..b60845b1e563 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > @@ -722,34 +722,42 @@ static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
> > >  				struct v4l2_subdev_state *sd_state,
> > >  				struct v4l2_subdev_format *format)
> > >  {
> > > -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > > -	struct v4l2_mbus_framefmt *framefmt;
> > > +	struct v4l2_mbus_framefmt *fmt;
> > > +	struct v4l2_subdev_state *state;
> > > +	int ret = 0;
> > >  
> > >  	if (!rcsi2_code_to_fmt(format->format.code))
> > >  		format->format.code = rcar_csi2_formats[0].code;
> > >  
> > > -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > -		priv->mf = format->format;
> > > -	} else {
> > > -		framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> > > -		*framefmt = format->format;
> > > -	}
> > > +	/*
> > > +	 * Format is propagated from sink streams to source streams, so
> > > +	 * disallow setting format on the source pads.
> > > +	 */
> > > +	if (format->pad > RCAR_CSI2_SINK)
> > > +		return -EINVAL;
> > 
> > You should return v4l2_subdev_get_fmt() instead.
> > 
> > >  
> > > -	return 0;
> > > -}
> > > +	state = v4l2_subdev_validate_and_lock_state(sd, sd_state);
> > 
> > With v10 of the streams series I think you can use
> > v4l2_subdev_lock_state().
> > 
> > > +	fmt = v4l2_state_get_stream_format(state, format->pad,
> > > +					   format->stream);
> > > +	if (!fmt) {
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +	*fmt = format->format;
> > 
> > I would get the pointers to both formats before updating any of them, to
> > avoid a partial update in case of error:
> > 
> > 	sink_fmt = v4l2_state_get_stream_format(state, format->pad,
> > 						format->stream);
> > 	source_fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
> > 							   format->stream);
> > 	if (!sink_fmt || !source_fmt) {
> > 		ret = -EINVAL;
> > 		goto out;
> > 	}
> > 
> > 	*sink_fmt = format->format;
> > 	*source_fmt = format->format;
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Actually, the rest of the driver still uses priv->mf, so this patch will
> cause a breakage. You can squash it with 11/13.

Or, looking at 11/13, it may be best to replace all usages of priv->mf
with the format from the state in this patch, and keep 11/13 separate.

> > >  
> > > -static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> > > -				struct v4l2_subdev_state *sd_state,
> > > -				struct v4l2_subdev_format *format)
> > > -{
> > > -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > > +	/* Propagate format to the other end of the route. */
> > > +	fmt = v4l2_state_get_opposite_stream_format(state, format->pad,
> > > +						    format->stream);
> > > +	if (!fmt) {
> > > +		ret = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +	*fmt = format->format;
> > >  
> > > -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > > -		format->format = priv->mf;
> > > -	else
> > > -		format->format = *v4l2_subdev_get_try_format(sd, sd_state, 0);
> > > +out:
> > > +	v4l2_subdev_unlock_state(state);
> > >  
> > > -	return 0;
> > > +	return ret;
> > >  }
> > >  
> > >  static int _rcsi2_set_routing(struct v4l2_subdev *sd,
> > > @@ -829,7 +837,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> > >  static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
> > >  	.init_cfg = rcsi2_init_cfg,
> > >  	.set_fmt = rcsi2_set_pad_format,
> > > -	.get_fmt = rcsi2_get_pad_format,
> > > +	.get_fmt = v4l2_subdev_get_fmt,
> > >  	.set_routing = rcsi2_set_routing,
> > >  };
> > >  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 11/13] media: rcar-csi2: Config by using the remote frame_desc

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Oct 17, 2021 at 08:24:47PM +0200, Jacopo Mondi wrote:
> Configure the CSI-2 receiver by inspecting the remote subdev frame_desc.
> 
> Configure the link clock rate, field handling and CSI-2 Virtual Channel
> and DT filtering using the frame descriptor retrieved from the
> transmitter.
> 
> This change also makes mandatory for any subdevice that operates with
> the R-Car CSI-2 receiver to implement the .get_frame_desc() operation.
> 
> [based on a patch from]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> [rework based on lates multistream support]
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 145 +++++++++++++++-----
>  1 file changed, 110 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index b60845b1e563..4ef7b9cb1ab7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -69,10 +69,7 @@ struct rcar_csi2;
>  #define FLD_REG				0x1c
>  #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
>  #define FLD_DET_SEL(n)			(((n) & 0x3) << 4)
> -#define FLD_FLD_EN4			BIT(3)
> -#define FLD_FLD_EN3			BIT(2)
> -#define FLD_FLD_EN2			BIT(1)
> -#define FLD_FLD_EN			BIT(0)
> +#define FLD_FLD_EN(n)			BIT((n) & 0xf)
>  
>  /* Automatic Standby Control */
>  #define ASTBY_REG			0x20
> @@ -339,6 +336,17 @@ static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code)
>  	return NULL;
>  }
>  
> +static const struct rcar_csi2_format *rcsi2_datatype_to_fmt(unsigned int dt)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++)
> +		if (rcar_csi2_formats[i].datatype == dt)
> +			return &rcar_csi2_formats[i];
> +
> +	return NULL;
> +}
> +
>  enum rcar_csi2_pads {
>  	RCAR_CSI2_SINK,
>  	RCAR_CSI2_SOURCE_VC0,
> @@ -370,8 +378,6 @@ struct rcar_csi2 {
>  	struct v4l2_subdev *remote;
>  	unsigned int remote_pad;
>  
> -	struct v4l2_mbus_framefmt mf;
> -
>  	struct mutex lock;
>  	int stream_count;
>  
> @@ -421,6 +427,32 @@ static int rcsi2_exit_standby(struct rcar_csi2 *priv)
>  	return 0;
>  }
>  
> +static int rcsi2_get_remote_frame_desc(struct rcar_csi2 *priv,
> +				       struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct media_pad *pad;
> +	int ret;
> +
> +	if (!priv->remote)
> +		return -ENODEV;
> +
> +	pad = media_entity_remote_pad(&priv->pads[RCAR_CSI2_SINK]);
> +	if (!pad)
> +		return -ENODEV;
> +
> +	ret = v4l2_subdev_call(priv->remote, pad, get_frame_desc,
> +			       pad->index, fd);
> +	if (ret)
> +		return ret;
> +
> +	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> +		dev_err(priv->dev, "Frame desc do not describe CSI-2 link");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int rcsi2_wait_phy_start(struct rcar_csi2 *priv,
>  				unsigned int lanes)
>  {
> @@ -460,11 +492,14 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
>  	return 0;
>  }
>  
> -static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
> -			   unsigned int lanes)
> +static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
> +			   struct v4l2_mbus_frame_desc *fd, unsigned int lanes)
>  {
> +	const struct v4l2_mbus_frame_desc_entry_csi2 *csi2_desc;
> +	const struct rcar_csi2_format *format;
>  	struct v4l2_subdev *source;
>  	struct v4l2_ctrl *ctrl;
> +	unsigned int i;
>  	u64 mbps;
>  
>  	if (!priv->remote)
> @@ -480,12 +515,30 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
>  		return -EINVAL;
>  	}
>  
> +	/* Verify that all remote streams send the same datatype. */

I don't think that's a good idea. Capturing image data and embedded data
is a common use case, and those use different data types.

> +	csi2_desc = NULL;
> +	for (i = 0; i < fd->num_entries; i++) {
> +		struct v4l2_mbus_frame_desc_entry_csi2 *stream_desc;
> +
> +		stream_desc = &fd->entry[i].bus.csi2;
> +		if (!csi2_desc)
> +			csi2_desc = stream_desc;
> +
> +		if (csi2_desc->dt != stream_desc->dt) {
> +			dev_err(priv->dev,
> +				"Remote streams with different DT: %u - %u\n",
> +				csi2_desc->dt, stream_desc->dt);
> +			return -EINVAL;
> +		}
> +	}
> +	format = rcsi2_datatype_to_fmt(csi2_desc->dt);
> +
>  	/*
>  	 * Calculate the phypll in mbps.
>  	 * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
>  	 * bps = link_freq * 2
>  	 */
> -	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> +	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * format->bpp;
>  	do_div(mbps, lanes * 1000000);
>  
>  	return mbps;
> @@ -541,49 +594,64 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
>  
>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  {
> -	const struct rcar_csi2_format *format;
> +	const struct v4l2_subdev_stream_configs *configs;
> +	struct v4l2_subdev_state *state;
>  	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> +	struct v4l2_mbus_frame_desc fd;
>  	unsigned int lanes;
>  	unsigned int i;
>  	int mbps, ret;
>  
> -	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> -		priv->mf.width, priv->mf.height,
> -		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> -
> -	/* Code is validated in set_fmt. */
> -	format = rcsi2_code_to_fmt(priv->mf.code);
> +	/* Get information about multiplexed link. */
> +	ret = rcsi2_get_remote_frame_desc(priv, &fd);
> +	if (ret)
> +		return ret;
>  
> -	/*
> -	 * Enable all supported CSI-2 channels with virtual channel and
> -	 * data type matching.
> -	 *
> -	 * NOTE: It's not possible to get individual datatype for each
> -	 *       source virtual channel. Once this is possible in V4L2
> -	 *       it should be used here.
> -	 */
> -	for (i = 0; i < priv->info->num_channels; i++) {
> +	for (i = 0; i < fd.num_entries; i++) {
> +		struct v4l2_mbus_frame_desc_entry *entry = &fd.entry[i];
>  		u32 vcdt_part;
>  
> -		vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> -			VCDT_SEL_DT(format->datatype);
> +		vcdt_part = VCDT_SEL_VC(entry->bus.csi2.vc) |
> +			    VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> +			    VCDT_SEL_DT(entry->bus.csi2.dt);
>  
>  		/* Store in correct reg and offset. */
> -		if (i < 2)
> -			vcdt |= vcdt_part << ((i % 2) * 16);
> +		if (entry->bus.csi2.vc < 2)
> +			vcdt |= vcdt_part <<
> +				((entry->bus.csi2.vc % 2) * 16);
>  		else
> -			vcdt2 |= vcdt_part << ((i % 2) * 16);
> +			vcdt2 |= vcdt_part <<
> +				((entry->bus.csi2.vc % 2) * 16);
> +
> +		dev_dbg(priv->dev, "VC: %d datatype: 0x%x\n",
> +			entry->bus.csi2.vc, entry->bus.csi2.dt);
>  	}
>  
> -	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> -		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
> -			| FLD_FLD_EN;
> +	/*
> +	 * Configure field handling inspecting the formats of the
> +	 * sink pad streams.
> +	 */
> +	state = v4l2_subdev_lock_active_state(&priv->subdev);
> +	configs = &state->stream_configs;
> +	for (i = 0; i < configs->num_configs; ++i) {
> +		const struct v4l2_subdev_stream_config *config = configs->configs;
> +
> +		if (config->pad != RCAR_CSI2_SINK)
> +			continue;
> +
> +		if (config->fmt.field != V4L2_FIELD_ALTERNATE)
> +			continue;
>  
> -		if (priv->mf.height == 240)
> +		fld |= FLD_DET_SEL(1);
> +		fld |= FLD_FLD_EN(config->stream);
> +
> +		/* PAL vs NTSC. */
> +		if (config->fmt.height == 240)
>  			fld |= FLD_FLD_NUM(0);
>  		else
>  			fld |= FLD_FLD_NUM(1);
>  	}
> +	v4l2_subdev_unlock_state(state);
>  
>  	/*
>  	 * Get the number of active data lanes inspecting the remote mbus
> @@ -596,7 +664,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	phycnt = PHYCNT_ENABLECLK;
>  	phycnt |= (1 << lanes) - 1;
>  
> -	mbps = rcsi2_calc_mbps(priv, format->bpp, lanes);
> +	mbps = rcsi2_calc_mbps(priv, &fd, lanes);
>  	if (mbps < 0)
>  		return mbps;
>  
> @@ -894,6 +962,13 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier,
>  	struct rcar_csi2 *priv = notifier_to_csi2(notifier);
>  	int pad;
>  
> +	if (!v4l2_subdev_has_op(subdev, pad, get_frame_desc)) {
> +		dev_err(priv->dev,
> +			"Subdev %s bound failed: missing get_frame_desc()\n",
> +			subdev->name);
> +		return -EINVAL;
> +	}
> +
>  	pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
>  					  MEDIA_PAD_FL_SOURCE);
>  	if (pad < 0) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 07/13] media: max9286: Implement get_frame_desc()

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Oct 17, 2021 at 08:24:43PM +0200, Jacopo Mondi wrote:
> Implement the get_frame_desc pad operation to allow retrieving the
> stream configuration of the max9286 subdevice.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/max9286.c | 53 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 8bfb88db9976..aca8455d6d56 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -878,6 +878,58 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  	return ret;
>  }
>  
> +static int max9286_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				  struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct v4l2_subdev_route *route;
> +	struct v4l2_subdev_state *state;
> +	int ret = 0;
> +
> +	if (pad != MAX9286_SRC_PAD)
> +		return -EINVAL;
> +
> +	state = v4l2_subdev_lock_active_state(sd);
> +
> +	memset(fd, 0, sizeof(*fd));
> +
> +	/* One stream entry per each connected route. */

s/connected/active/

> +	for_each_active_route(&state->routing, route) {
> +		struct v4l2_mbus_frame_desc_entry *entry =
> +						&fd->entry[fd->num_entries];
> +		struct v4l2_mbus_framefmt *fmt;
> +
> +		fmt = v4l2_state_get_stream_format(state, pad,

I would write s/pad/MAX9286_SRC_PAD/. This won't chanege the behaviour,
but would be more explicit.

> +						   route->source_stream);
> +		if (!fmt) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		/*
> +		 * Assume a YUYV format (0x1e DT) and 16 bpp: we only support
> +		 * these formats at the moment.
> +		 */

I'll rebase my pending max9286 series on top of your poc-gpio patches,
but in return I'll ask you to rebase this on top of my patches, which
add support for other formats :-)

> +		entry->stream = fd->num_entries++;
> +		entry->flags = V4L2_MBUS_FRAME_DESC_FL_LEN_MAX;
> +		entry->length = fmt->width * fmt->height * 16 / 8;
> +		entry->pixelcode = fmt->code;
> +
> +		/*
> +		 * The source stream id corresponds to the virtual channel a
> +		 * stream is output on.
> +		 */
> +		entry->bus.csi2.vc = route->source_stream;
> +		entry->bus.csi2.dt = 0x1e;
> +	}
> +
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +
> +out:
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +
>  static int max9286_routing_verify(struct max9286_priv *priv,
>  				  struct v4l2_subdev_krouting *routing)
>  {
> @@ -1020,6 +1072,7 @@ static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
>  	.enum_mbus_code = max9286_enum_mbus_code,
>  	.get_fmt	= v4l2_subdev_get_fmt,
>  	.set_fmt	= max9286_set_fmt,
> +	.get_frame_desc = max9286_get_frame_desc,
>  	.set_routing	= max9286_set_routing,
>  };
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 08/13] media: rcar-csi2: Add support for v4l2_subdev_state

[Jacopo Mondi]

Hi Laurent,

On Sun, Jan 23, 2022 at 04:11:56PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Sun, Oct 17, 2021 at 08:24:44PM +0200, Jacopo Mondi wrote:
> > Create and initialize the v4l2_subdev_state for the R-Car CSI-2 receiver
> > rder to prepare to support routing operations and multiplexed streams.
>
> s/rder/in order/ ?
>

Thanks for review, but please be aware that patches to the R-Car CSI-2
and VIN drivers are not required if Niklas' VIN channel rework gets in
https://patchwork.linuxtv.org/project/linux-media/patch/20211127164135.2617686-4-niklas.soderlund+renesas@ragnatech.se/

> >
> > Create the subdevice state with v4l2_subdev_init_finalize() and
> > implement the init_cfg() operation to guarantee the state is initialized
> > correctly with a set of default routes.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 68 ++++++++++++++++++++-
> >  1 file changed, 66 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index e28eff039688..a74087b49e71 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -752,11 +752,65 @@ static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >
> > +static int rcsi2_init_cfg(struct v4l2_subdev *sd,
> > +			  struct v4l2_subdev_state *state)
>
> This could be moved before rcsi2_set_pad_format() to match the order in
> rcar_csi2_pad_ops. Up to you.
>
> > +{
> > +	/* Initialize 4 routes from each source pad to the single sink pad. */
> > +	struct v4l2_subdev_route routes[] = {
> > +		{
> > +			.sink_pad = RCAR_CSI2_SINK,
> > +			.sink_stream = 0,
> > +			.source_pad = RCAR_CSI2_SOURCE_VC0,
> > +			.source_stream = 0,
> > +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> > +		},
> > +		{
> > +			.sink_pad = RCAR_CSI2_SINK,
> > +			.sink_stream = 1,
> > +			.source_pad = RCAR_CSI2_SOURCE_VC1,
> > +			.source_stream = 0,
> > +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> > +		},
> > +		{
> > +			.sink_pad = RCAR_CSI2_SINK,
> > +			.sink_stream = 2,
> > +			.source_pad = RCAR_CSI2_SOURCE_VC2,
> > +			.source_stream = 0,
> > +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> > +		},
> > +		{
> > +			.sink_pad = RCAR_CSI2_SINK,
> > +			.sink_stream = 3,
> > +			.source_pad = RCAR_CSI2_SOURCE_VC3,
> > +			.source_stream = 0,
> > +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> > +		},
> > +	};
> > +
> > +	struct v4l2_subdev_krouting routing = {
> > +		.num_routes = ARRAY_SIZE(routes),
> > +		.routes = routes,
> > +	};
> > +
> > +	int ret = v4l2_routing_simple_verify(&routing);
> > +	if (ret)
> > +		return ret;
> > +
> > +	state = v4l2_subdev_validate_and_lock_state(sd, state);
> > +
> > +	ret = v4l2_subdev_set_routing(sd, state, &routing);
> > +
> > +	v4l2_subdev_unlock_state(state);
>
> I would squash this with 09/13 to avoid this intermediate state of
> dealing with routes manually in the .init_cfg() operation. The patch
> otherwise looks good to me.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +
> > +	return ret;
> > +}
> > +
> >  static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> >  	.s_stream = rcsi2_s_stream,
> >  };
> >
> >  static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
> > +	.init_cfg = rcsi2_init_cfg,
> >  	.set_fmt = rcsi2_set_pad_format,
> >  	.get_fmt = rcsi2_get_pad_format,
> >  };
> > @@ -1260,7 +1314,8 @@ static int rcsi2_probe(struct platform_device *pdev)
> >  	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
> >  	snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s",
> >  		 KBUILD_MODNAME, dev_name(&pdev->dev));
> > -	priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +	priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE |
> > +			     V4L2_SUBDEV_FL_MULTIPLEXED;
> >
> >  	priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> >  	priv->subdev.entity.ops = &rcar_csi2_entity_ops;
> > @@ -1276,14 +1331,22 @@ static int rcsi2_probe(struct platform_device *pdev)
> >
> >  	pm_runtime_enable(&pdev->dev);
> >
> > +	ret = v4l2_subdev_init_finalize(&priv->subdev);
> > +	if (ret)
> > +		goto error_pm;
> > +
> >  	ret = v4l2_async_register_subdev(&priv->subdev);
> >  	if (ret < 0)
> > -		goto error;
> > +		goto error_subdev;
> >
> >  	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
> >
> >  	return 0;
> >
> > +error_subdev:
> > +	v4l2_subdev_cleanup(&priv->subdev);
> > +error_pm:
> > +	pm_runtime_disable(&pdev->dev);
> >  error:
> >  	v4l2_async_notifier_unregister(&priv->notifier);
> >  	v4l2_async_notifier_cleanup(&priv->notifier);
> > @@ -1298,6 +1361,7 @@ static int rcsi2_remove(struct platform_device *pdev)
> >  	v4l2_async_notifier_unregister(&priv->notifier);
> >  	v4l2_async_notifier_cleanup(&priv->notifier);
> >  	v4l2_async_unregister_subdev(&priv->subdev);
> > +	v4l2_subdev_cleanup(&priv->subdev);
> >
> >  	pm_runtime_disable(&pdev->dev);
> >
>
> --
> Regards,
>
> Laurent Pinchart