[PATCH v3 00/12] Small fixes and cleanups (ov2740 and ccs)

[PATCH v3 00/12] Small fixes and cleanups (ov2740 and ccs)

[Sakari Ailus]

Hi folks,

This small set contains fixes and cleanups, mainly for the ccs and ov2740
drivers. I wrote these while working on the metadata set, but these could
and should be merged earlier.

since v2:

- Wrap init_cfg callback long line.

- Remove "pad_" from variable names in ccs_init_cfg.

- Fix media_entity_pads_init() error handling bug (was introduced in the
  last patch).

- Print frame descriptor in less verbose way.

since v1:

- Add a comment on ov2740 active state patch on serialising sensor access.

- Improved commit message of ov2740 patch enabling runtime PM earlier.

- Added patches for printing and zeroing frame descriptor, (debug)
  printing of frame descriptor, switching ccs to init_cfg and sub-device
  state and checking pad flag validity.

Sakari Ailus (12):
  media: Documentation: Align numbered list, make it a proper ReST
  media: ccs: Fix driver quirk struct documentation
  media: ccs: Correctly initialise try compose rectangle
  media: ccs: Correct error handling in ccs_register_subdev
  media: ccs: Switch to init_cfg
  media: ccs: Use sub-device active state
  media: ov2740: Enable runtime PM before registering the async subdev
  media: ov2740: Use sub-device active state
  media: ov2740: Return -EPROBE_DEFER if no endpoint is found
  media: v4l: subdev: Clear frame descriptor before get_frame_desc
  media: v4l: subdev: Print debug information on frame descriptor
  media: mc: Check pad flag validity

 .../userspace-api/media/v4l/dev-subdev.rst    |  49 ++-
 drivers/media/i2c/ccs/ccs-core.c              | 302 +++++++-----------
 drivers/media/i2c/ccs/ccs-quirk.h             |   4 +-
 drivers/media/i2c/ccs/ccs.h                   |   4 +-
 drivers/media/i2c/ds90ub913.c                 |   2 -
 drivers/media/i2c/ds90ub953.c                 |   2 -
 drivers/media/i2c/ds90ub960.c                 |   2 -
 drivers/media/i2c/ov2740.c                    | 138 ++++----
 drivers/media/mc/mc-entity.c                  |  14 +-
 drivers/media/platform/nxp/imx-mipi-csis.c    |   2 -
 drivers/media/v4l2-core/v4l2-subdev.c         |  39 +++
 11 files changed, 252 insertions(+), 306 deletions(-)

-- 
2.39.2

[PATCH v3 01/12] media: Documentation: Align numbered list, make it a proper ReST

[Sakari Ailus]

Align lines for numbered list so that Sphinx produces an uniform output
for all list entries. Also indent paragraphs of such list entries for
consistency.

Also use ReST numbered list syntax for the entries.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../userspace-api/media/v4l/dev-subdev.rst    | 49 +++++++++----------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
index a4f1df7093e8..43988516acdd 100644
--- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
+++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
@@ -579,20 +579,19 @@ is started.
 
 There are three steps in configuring the streams:
 
-1) Set up links. Connect the pads between sub-devices using the :ref:`Media
-Controller API <media_controller>`
+1. Set up links. Connect the pads between sub-devices using the
+   :ref:`Media Controller API <media_controller>`
 
-2) Streams. Streams are declared and their routing is configured by
-setting the routing table for the sub-device using
-:ref:`VIDIOC_SUBDEV_S_ROUTING <VIDIOC_SUBDEV_G_ROUTING>` ioctl. Note that
-setting the routing table will reset formats and selections in the
-sub-device to default values.
+2. Streams. Streams are declared and their routing is configured by setting the
+   routing table for the sub-device using :ref:`VIDIOC_SUBDEV_S_ROUTING
+   <VIDIOC_SUBDEV_G_ROUTING>` ioctl. Note that setting the routing table will
+   reset formats and selections in the sub-device to default values.
 
-3) Configure formats and selections. Formats and selections of each stream
-are configured separately as documented for plain sub-devices in
-:ref:`format-propagation`. The stream ID is set to the same stream ID
-associated with either sink or source pads of routes configured using the
-:ref:`VIDIOC_SUBDEV_S_ROUTING <VIDIOC_SUBDEV_G_ROUTING>` ioctl.
+3. Configure formats and selections. Formats and selections of each stream are
+   configured separately as documented for plain sub-devices in
+   :ref:`format-propagation`. The stream ID is set to the same stream ID
+   associated with either sink or source pads of routes configured using the
+   :ref:`VIDIOC_SUBDEV_S_ROUTING <VIDIOC_SUBDEV_G_ROUTING>` ioctl.
 
 Multiplexed streams setup example
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -618,11 +617,11 @@ modeled as V4L2 devices, exposed to userspace via /dev/videoX nodes.
 
 To configure this pipeline, the userspace must take the following steps:
 
-1) Set up media links between entities: connect the sensors to the bridge,
-bridge to the receiver, and the receiver to the DMA engines. This step does
-not differ from normal non-multiplexed media controller setup.
+1. Set up media links between entities: connect the sensors to the bridge,
+   bridge to the receiver, and the receiver to the DMA engines. This step does
+   not differ from normal non-multiplexed media controller setup.
 
-2) Configure routing
+2. Configure routing
 
 .. flat-table:: Bridge routing table
     :header-rows:  1
@@ -656,14 +655,14 @@ not differ from normal non-multiplexed media controller setup.
       - V4L2_SUBDEV_ROUTE_FL_ACTIVE
       - Pixel data stream from Sensor B
 
-3) Configure formats and selections
+3. Configure formats and selections
 
-After configuring routing, the next step is configuring the formats and
-selections for the streams. This is similar to performing this step without
-streams, with just one exception: the ``stream`` field needs to be assigned
-to the value of the stream ID.
+   After configuring routing, the next step is configuring the formats and
+   selections for the streams. This is similar to performing this step without
+   streams, with just one exception: the ``stream`` field needs to be assigned
+   to the value of the stream ID.
 
-A common way to accomplish this is to start from the sensors and propagate the
-configurations along the stream towards the receiver,
-using :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls to configure each
-stream endpoint in each sub-device.
+   A common way to accomplish this is to start from the sensors and propagate
+   the configurations along the stream towards the receiver, using
+   :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls to configure each
+   stream endpoint in each sub-device.
-- 
2.39.2

[PATCH v3 02/12] media: ccs: Fix driver quirk struct documentation

[Sakari Ailus]

Fix documentation for struct ccs_quirk, a device specific struct for
managing deviations from the standard. The flags field was drifted away
from where it should have been.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ccs/ccs-quirk.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ccs/ccs-quirk.h b/drivers/media/i2c/ccs/ccs-quirk.h
index 5838fcda92fd..0b1a64958d71 100644
--- a/drivers/media/i2c/ccs/ccs-quirk.h
+++ b/drivers/media/i2c/ccs/ccs-quirk.h
@@ -32,12 +32,10 @@ struct ccs_sensor;
  *		@reg: Pointer to the register to access
  *		@value: Register value, set by the caller on write, or
  *			by the quirk on read
- *
- * @flags: Quirk flags
- *
  *		@return: 0 on success, -ENOIOCTLCMD if no register
  *			 access may be done by the caller (default read
  *			 value is zero), else negative error code on error
+ * @flags: Quirk flags
  */
 struct ccs_quirk {
 	int (*limits)(struct ccs_sensor *sensor);
-- 
2.39.2

[PATCH v3 03/12] media: ccs: Correctly initialise try compose rectangle

[Sakari Ailus]

Initialise the try sink compose rectangle size to the sink compose
rectangle for binner and scaler sub-devices. This was missed due to the
faulty condition that lead to the compose rectangles to be initialised for
the pixel array sub-device where it is not relevant.

Fixes: ccfc97bdb5ae ("[media] smiapp: Add driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ccs/ccs-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index 49e0d9a09530..6f8fbd82e21c 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -3097,7 +3097,7 @@ static int ccs_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 		try_fmt->code = sensor->internal_csi_format->code;
 		try_fmt->field = V4L2_FIELD_NONE;
 
-		if (ssd != sensor->pixel_array)
+		if (ssd == sensor->pixel_array)
 			continue;
 
 		try_comp = v4l2_subdev_get_try_compose(sd, fh->state, i);
-- 
2.39.2

[PATCH v3 04/12] media: ccs: Correct error handling in ccs_register_subdev

[Sakari Ailus]

ccs_register_subdev() did not clean up the media entity in error case, do
that now. Also switch to goto based error handling.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ccs/ccs-core.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index 6f8fbd82e21c..3fed071b65ab 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -2968,7 +2968,7 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
 	rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev, &ssd->sd);
 	if (rval) {
 		dev_err(&client->dev, "v4l2_device_register_subdev failed\n");
-		return rval;
+		goto out_media_entity_cleanup;
 	}
 
 	rval = media_create_pad_link(&ssd->sd.entity, source_pad,
@@ -2976,11 +2976,18 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
 				     link_flags);
 	if (rval) {
 		dev_err(&client->dev, "media_create_pad_link failed\n");
-		v4l2_device_unregister_subdev(&ssd->sd);
-		return rval;
+		goto out_v4l2_device_unregister_subdev;
 	}
 
 	return 0;
+
+out_v4l2_device_unregister_subdev:
+	v4l2_device_unregister_subdev(&ssd->sd);
+
+out_media_entity_cleanup:
+	media_entity_cleanup(&ssd->sd.entity);
+
+	return rval;
 }
 
 static void ccs_unregistered(struct v4l2_subdev *subdev)
-- 
2.39.2

[PATCH v3 05/12] media: ccs: Switch to init_cfg

[Sakari Ailus]

Use init_cfg() instead of manually setting up defaults in file handle
open.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ccs/ccs-core.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index 3fed071b65ab..a4f593866ad6 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -2945,7 +2945,6 @@ static int ccs_identify_module(struct ccs_sensor *sensor)
 }
 
 static const struct v4l2_subdev_ops ccs_ops;
-static const struct v4l2_subdev_internal_ops ccs_internal_ops;
 static const struct media_entity_operations ccs_entity_ops;
 
 static int ccs_register_subdev(struct ccs_sensor *sensor,
@@ -3076,13 +3075,13 @@ static void ccs_create_subdev(struct ccs_sensor *sensor,
 	if (ssd == sensor->src)
 		return;
 
-	ssd->sd.internal_ops = &ccs_internal_ops;
 	ssd->sd.owner = THIS_MODULE;
 	ssd->sd.dev = &client->dev;
 	v4l2_set_subdevdata(&ssd->sd, client);
 }
 
-static int ccs_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+static int ccs_init_cfg(struct v4l2_subdev *sd,
+			struct v4l2_subdev_state *sd_state)
 {
 	struct ccs_subdev *ssd = to_ccs_subdev(sd);
 	struct ccs_sensor *sensor = ssd->sensor;
@@ -3092,9 +3091,9 @@ static int ccs_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 	for (i = 0; i < ssd->npads; i++) {
 		struct v4l2_mbus_framefmt *try_fmt =
-			v4l2_subdev_get_try_format(sd, fh->state, i);
+			v4l2_subdev_get_try_format(sd, sd_state, i);
 		struct v4l2_rect *try_crop =
-			v4l2_subdev_get_try_crop(sd, fh->state, i);
+			v4l2_subdev_get_try_crop(sd, sd_state, i);
 		struct v4l2_rect *try_comp;
 
 		ccs_get_native_size(ssd, try_crop);
@@ -3107,7 +3106,7 @@ static int ccs_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 		if (ssd == sensor->pixel_array)
 			continue;
 
-		try_comp = v4l2_subdev_get_try_compose(sd, fh->state, i);
+		try_comp = v4l2_subdev_get_try_compose(sd, sd_state, i);
 		*try_comp = *try_crop;
 	}
 
@@ -3123,6 +3122,7 @@ static const struct v4l2_subdev_video_ops ccs_video_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops ccs_pad_ops = {
+	.init_cfg = ccs_init_cfg,
 	.enum_mbus_code = ccs_enum_mbus_code,
 	.get_fmt = ccs_get_format,
 	.set_fmt = ccs_set_format,
@@ -3148,11 +3148,6 @@ static const struct media_entity_operations ccs_entity_ops = {
 static const struct v4l2_subdev_internal_ops ccs_internal_src_ops = {
 	.registered = ccs_registered,
 	.unregistered = ccs_unregistered,
-	.open = ccs_open,
-};
-
-static const struct v4l2_subdev_internal_ops ccs_internal_ops = {
-	.open = ccs_open,
 };
 
 /* -----------------------------------------------------------------------------
-- 
2.39.2

[PATCH v3 06/12] media: ccs: Use sub-device active state

[Sakari Ailus]

Make use of sub-device active state. In most cases the effect on need for
acquiring the mutex is non-existent as access to the driver's core data
structure still needs to be serialised.

This still removes a lot of code as the code paths for active and try
state are the same in many cases.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ccs/ccs-core.c | 278 ++++++++++++-------------------
 drivers/media/i2c/ccs/ccs.h      |   4 +-
 2 files changed, 103 insertions(+), 179 deletions(-)

diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c
index a4f593866ad6..7c53bbda5b8d 100644
--- a/drivers/media/i2c/ccs/ccs-core.c
+++ b/drivers/media/i2c/ccs/ccs-core.c
@@ -508,9 +508,8 @@ static void __ccs_update_exposure_limits(struct ccs_sensor *sensor)
 	struct v4l2_ctrl *ctrl = sensor->exposure;
 	int max;
 
-	max = sensor->pixel_array->crop[CCS_PA_PAD_SRC].height
-		+ sensor->vblank->val
-		- CCS_LIM(sensor, COARSE_INTEGRATION_TIME_MAX_MARGIN);
+	max = sensor->pa_src.height + sensor->vblank->val -
+		CCS_LIM(sensor, COARSE_INTEGRATION_TIME_MAX_MARGIN);
 
 	__v4l2_ctrl_modify_range(ctrl, ctrl->minimum, max, ctrl->step, max);
 }
@@ -728,15 +727,12 @@ static int ccs_set_ctrl(struct v4l2_ctrl *ctrl)
 		break;
 	case V4L2_CID_VBLANK:
 		rval = ccs_write(sensor, FRAME_LENGTH_LINES,
-				 sensor->pixel_array->crop[
-					 CCS_PA_PAD_SRC].height
-				 + ctrl->val);
+				 sensor->pa_src.height + ctrl->val);
 
 		break;
 	case V4L2_CID_HBLANK:
 		rval = ccs_write(sensor, LINE_LENGTH_PCK,
-				 sensor->pixel_array->crop[CCS_PA_PAD_SRC].width
-				 + ctrl->val);
+				 sensor->pa_src.width + ctrl->val);
 
 		break;
 	case V4L2_CID_TEST_PATTERN:
@@ -1214,15 +1210,13 @@ static void ccs_update_blanking(struct ccs_sensor *sensor)
 
 	min = max_t(int,
 		    CCS_LIM(sensor, MIN_FRAME_BLANKING_LINES),
-		    min_fll - sensor->pixel_array->crop[CCS_PA_PAD_SRC].height);
-	max = max_fll -	sensor->pixel_array->crop[CCS_PA_PAD_SRC].height;
+		    min_fll - sensor->pa_src.height);
+	max = max_fll -	sensor->pa_src.height;
 
 	__v4l2_ctrl_modify_range(vblank, min, max, vblank->step, min);
 
-	min = max_t(int,
-		    min_llp - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width,
-		    min_lbp);
-	max = max_llp - sensor->pixel_array->crop[CCS_PA_PAD_SRC].width;
+	min = max_t(int, min_llp - sensor->pa_src.width, min_lbp);
+	max = max_llp - sensor->pa_src.width;
 
 	__v4l2_ctrl_modify_range(hblank, min, max, hblank->step, min);
 
@@ -1246,10 +1240,8 @@ static int ccs_pll_blanking_update(struct ccs_sensor *sensor)
 
 	dev_dbg(&client->dev, "real timeperframe\t100/%d\n",
 		sensor->pll.pixel_rate_pixel_array /
-		((sensor->pixel_array->crop[CCS_PA_PAD_SRC].width
-		  + sensor->hblank->val) *
-		 (sensor->pixel_array->crop[CCS_PA_PAD_SRC].height
-		  + sensor->vblank->val) / 100));
+		((sensor->pa_src.width + sensor->hblank->val) *
+		 (sensor->pa_src.height + sensor->vblank->val) / 100));
 
 	return 0;
 }
@@ -1756,28 +1748,22 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
 		goto out;
 
 	/* Analog crop start coordinates */
-	rval = ccs_write(sensor, X_ADDR_START,
-			 sensor->pixel_array->crop[CCS_PA_PAD_SRC].left);
+	rval = ccs_write(sensor, X_ADDR_START, sensor->pa_src.left);
 	if (rval < 0)
 		goto out;
 
-	rval = ccs_write(sensor, Y_ADDR_START,
-			 sensor->pixel_array->crop[CCS_PA_PAD_SRC].top);
+	rval = ccs_write(sensor, Y_ADDR_START, sensor->pa_src.top);
 	if (rval < 0)
 		goto out;
 
 	/* Analog crop end coordinates */
-	rval = ccs_write(
-		sensor, X_ADDR_END,
-		sensor->pixel_array->crop[CCS_PA_PAD_SRC].left
-		+ sensor->pixel_array->crop[CCS_PA_PAD_SRC].width - 1);
+	rval = ccs_write(sensor, X_ADDR_END,
+			 sensor->pa_src.left + sensor->pa_src.width - 1);
 	if (rval < 0)
 		goto out;
 
-	rval = ccs_write(
-		sensor, Y_ADDR_END,
-		sensor->pixel_array->crop[CCS_PA_PAD_SRC].top
-		+ sensor->pixel_array->crop[CCS_PA_PAD_SRC].height - 1);
+	rval = ccs_write(sensor, Y_ADDR_END,
+			 sensor->pa_src.top + sensor->pa_src.height - 1);
 	if (rval < 0)
 		goto out;
 
@@ -1789,27 +1775,23 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
 	/* Digital crop */
 	if (CCS_LIM(sensor, DIGITAL_CROP_CAPABILITY)
 	    == CCS_DIGITAL_CROP_CAPABILITY_INPUT_CROP) {
-		rval = ccs_write(
-			sensor, DIGITAL_CROP_X_OFFSET,
-			sensor->scaler->crop[CCS_PAD_SINK].left);
+		rval = ccs_write(sensor, DIGITAL_CROP_X_OFFSET,
+				 sensor->scaler_sink.left);
 		if (rval < 0)
 			goto out;
 
-		rval = ccs_write(
-			sensor, DIGITAL_CROP_Y_OFFSET,
-			sensor->scaler->crop[CCS_PAD_SINK].top);
+		rval = ccs_write(sensor, DIGITAL_CROP_Y_OFFSET,
+				 sensor->scaler_sink.top);
 		if (rval < 0)
 			goto out;
 
-		rval = ccs_write(
-			sensor, DIGITAL_CROP_IMAGE_WIDTH,
-			sensor->scaler->crop[CCS_PAD_SINK].width);
+		rval = ccs_write(sensor, DIGITAL_CROP_IMAGE_WIDTH,
+				 sensor->scaler_sink.width);
 		if (rval < 0)
 			goto out;
 
-		rval = ccs_write(
-			sensor, DIGITAL_CROP_IMAGE_HEIGHT,
-			sensor->scaler->crop[CCS_PAD_SINK].height);
+		rval = ccs_write(sensor, DIGITAL_CROP_IMAGE_HEIGHT,
+				 sensor->scaler_sink.height);
 		if (rval < 0)
 			goto out;
 	}
@@ -1827,12 +1809,10 @@ static int ccs_start_streaming(struct ccs_sensor *sensor)
 	}
 
 	/* Output size from sensor */
-	rval = ccs_write(sensor, X_OUTPUT_SIZE,
-			 sensor->src->crop[CCS_PAD_SRC].width);
+	rval = ccs_write(sensor, X_OUTPUT_SIZE, sensor->src_src.width);
 	if (rval < 0)
 		goto out;
-	rval = ccs_write(sensor, Y_OUTPUT_SIZE,
-			 sensor->src->crop[CCS_PAD_SRC].height);
+	rval = ccs_write(sensor, Y_OUTPUT_SIZE, sensor->src_src.height);
 	if (rval < 0)
 		goto out;
 
@@ -2053,24 +2033,8 @@ static int __ccs_get_format(struct v4l2_subdev *subdev,
 			    struct v4l2_subdev_state *sd_state,
 			    struct v4l2_subdev_format *fmt)
 {
-	struct ccs_subdev *ssd = to_ccs_subdev(subdev);
-
-	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-		fmt->format = *v4l2_subdev_get_try_format(subdev, sd_state,
-							  fmt->pad);
-	} else {
-		struct v4l2_rect *r;
-
-		if (fmt->pad == ssd->source_pad)
-			r = &ssd->crop[ssd->source_pad];
-		else
-			r = &ssd->sink_fmt;
-
-		fmt->format.code = __ccs_get_mbus_code(subdev, fmt->pad);
-		fmt->format.width = r->width;
-		fmt->format.height = r->height;
-		fmt->format.field = V4L2_FIELD_NONE;
-	}
+	fmt->format = *v4l2_subdev_get_pad_format(subdev, sd_state, fmt->pad);
+	fmt->format.code = __ccs_get_mbus_code(subdev, fmt->pad);
 
 	return 0;
 }
@@ -2092,28 +2056,18 @@ static int ccs_get_format(struct v4l2_subdev *subdev,
 static void ccs_get_crop_compose(struct v4l2_subdev *subdev,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_rect **crops,
-				 struct v4l2_rect **comps, int which)
+				 struct v4l2_rect **comps)
 {
 	struct ccs_subdev *ssd = to_ccs_subdev(subdev);
 	unsigned int i;
 
-	if (which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		if (crops)
-			for (i = 0; i < subdev->entity.num_pads; i++)
-				crops[i] = &ssd->crop[i];
-		if (comps)
-			*comps = &ssd->compose;
-	} else {
-		if (crops) {
-			for (i = 0; i < subdev->entity.num_pads; i++)
-				crops[i] = v4l2_subdev_get_try_crop(subdev,
-								    sd_state,
-								    i);
-		}
-		if (comps)
-			*comps = v4l2_subdev_get_try_compose(subdev, sd_state,
-							     CCS_PAD_SINK);
-	}
+	if (crops)
+		for (i = 0; i < subdev->entity.num_pads; i++)
+			crops[i] =
+				v4l2_subdev_get_pad_crop(subdev, sd_state, i);
+	if (comps)
+		*comps = v4l2_subdev_get_pad_compose(subdev, sd_state,
+						     ssd->sink_pad);
 }
 
 /* Changes require propagation only on sink pad. */
@@ -2125,7 +2079,7 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
 	struct ccs_subdev *ssd = to_ccs_subdev(subdev);
 	struct v4l2_rect *comp, *crops[CCS_PADS];
 
-	ccs_get_crop_compose(subdev, sd_state, crops, &comp, which);
+	ccs_get_crop_compose(subdev, sd_state, crops, &comp);
 
 	switch (target) {
 	case V4L2_SEL_TGT_CROP:
@@ -2136,6 +2090,7 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
 				sensor->scale_m = CCS_LIM(sensor, SCALER_N_MIN);
 				sensor->scaling_mode =
 					CCS_SCALING_MODE_NO_SCALING;
+				sensor->scaler_sink = *comp;
 			} else if (ssd == sensor->binner) {
 				sensor->binning_horizontal = 1;
 				sensor->binning_vertical = 1;
@@ -2144,6 +2099,8 @@ static void ccs_propagate(struct v4l2_subdev *subdev,
 		fallthrough;
 	case V4L2_SEL_TGT_COMPOSE:
 		*crops[CCS_PAD_SRC] = *comp;
+		if (which == V4L2_SUBDEV_FORMAT_ACTIVE && ssd == sensor->src)
+			sensor->src_src = *crops[CCS_PAD_SRC];
 		break;
 	default:
 		WARN_ON_ONCE(1);
@@ -2252,14 +2209,12 @@ static int ccs_set_format(struct v4l2_subdev *subdev,
 		      CCS_LIM(sensor, MIN_Y_OUTPUT_SIZE),
 		      CCS_LIM(sensor, MAX_Y_OUTPUT_SIZE));
 
-	ccs_get_crop_compose(subdev, sd_state, crops, NULL, fmt->which);
+	ccs_get_crop_compose(subdev, sd_state, crops, NULL);
 
 	crops[ssd->sink_pad]->left = 0;
 	crops[ssd->sink_pad]->top = 0;
 	crops[ssd->sink_pad]->width = fmt->format.width;
 	crops[ssd->sink_pad]->height = fmt->format.height;
-	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		ssd->sink_fmt = *crops[ssd->sink_pad];
 	ccs_propagate(subdev, sd_state, fmt->which, V4L2_SEL_TGT_CROP);
 
 	mutex_unlock(&sensor->mutex);
@@ -2482,7 +2437,7 @@ static int ccs_set_compose(struct v4l2_subdev *subdev,
 	struct ccs_subdev *ssd = to_ccs_subdev(subdev);
 	struct v4l2_rect *comp, *crops[CCS_PADS];
 
-	ccs_get_crop_compose(subdev, sd_state, crops, &comp, sel->which);
+	ccs_get_crop_compose(subdev, sd_state, crops, &comp);
 
 	sel->r.top = 0;
 	sel->r.left = 0;
@@ -2501,8 +2456,8 @@ static int ccs_set_compose(struct v4l2_subdev *subdev,
 	return 0;
 }
 
-static int __ccs_sel_supported(struct v4l2_subdev *subdev,
-			       struct v4l2_subdev_selection *sel)
+static int ccs_sel_supported(struct v4l2_subdev *subdev,
+			     struct v4l2_subdev_selection *sel)
 {
 	struct ccs_sensor *sensor = to_ccs_sensor(subdev);
 	struct ccs_subdev *ssd = to_ccs_subdev(subdev);
@@ -2545,33 +2500,18 @@ static int ccs_set_crop(struct v4l2_subdev *subdev,
 {
 	struct ccs_sensor *sensor = to_ccs_sensor(subdev);
 	struct ccs_subdev *ssd = to_ccs_subdev(subdev);
-	struct v4l2_rect *src_size, *crops[CCS_PADS];
-	struct v4l2_rect _r;
+	struct v4l2_rect src_size = { 0 }, *crops[CCS_PADS], *comp;
 
-	ccs_get_crop_compose(subdev, sd_state, crops, NULL, sel->which);
+	ccs_get_crop_compose(subdev, sd_state, crops, &comp);
 
-	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		if (sel->pad == ssd->sink_pad)
-			src_size = &ssd->sink_fmt;
-		else
-			src_size = &ssd->compose;
+	if (sel->pad == ssd->sink_pad) {
+		struct v4l2_mbus_framefmt *mfmt =
+			v4l2_subdev_get_pad_format(subdev, sd_state, sel->pad);
+
+		src_size.width = mfmt->width;
+		src_size.height = mfmt->height;
 	} else {
-		if (sel->pad == ssd->sink_pad) {
-			_r.left = 0;
-			_r.top = 0;
-			_r.width = v4l2_subdev_get_try_format(subdev,
-							      sd_state,
-							      sel->pad)
-				->width;
-			_r.height = v4l2_subdev_get_try_format(subdev,
-							       sd_state,
-							       sel->pad)
-				->height;
-			src_size = &_r;
-		} else {
-			src_size = v4l2_subdev_get_try_compose(
-				subdev, sd_state, ssd->sink_pad);
-		}
+		src_size = *comp;
 	}
 
 	if (ssd == sensor->src && sel->pad == CCS_PAD_SRC) {
@@ -2579,16 +2519,19 @@ static int ccs_set_crop(struct v4l2_subdev *subdev,
 		sel->r.top = 0;
 	}
 
-	sel->r.width = min(sel->r.width, src_size->width);
-	sel->r.height = min(sel->r.height, src_size->height);
+	sel->r.width = min(sel->r.width, src_size.width);
+	sel->r.height = min(sel->r.height, src_size.height);
 
-	sel->r.left = min_t(int, sel->r.left, src_size->width - sel->r.width);
-	sel->r.top = min_t(int, sel->r.top, src_size->height - sel->r.height);
+	sel->r.left = min_t(int, sel->r.left, src_size.width - sel->r.width);
+	sel->r.top = min_t(int, sel->r.top, src_size.height - sel->r.height);
 
 	*crops[sel->pad] = sel->r;
 
 	if (ssd != sensor->pixel_array && sel->pad == CCS_PAD_SINK)
 		ccs_propagate(subdev, sd_state, sel->which, V4L2_SEL_TGT_CROP);
+	else if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
+		 ssd == sensor->pixel_array)
+		sensor->pa_src = sel->r;
 
 	return 0;
 }
@@ -2601,44 +2544,36 @@ static void ccs_get_native_size(struct ccs_subdev *ssd, struct v4l2_rect *r)
 	r->height = CCS_LIM(ssd->sensor, Y_ADDR_MAX) + 1;
 }
 
-static int __ccs_get_selection(struct v4l2_subdev *subdev,
-			       struct v4l2_subdev_state *sd_state,
-			       struct v4l2_subdev_selection *sel)
+static int ccs_get_selection(struct v4l2_subdev *subdev,
+			     struct v4l2_subdev_state *sd_state,
+			     struct v4l2_subdev_selection *sel)
 {
 	struct ccs_sensor *sensor = to_ccs_sensor(subdev);
 	struct ccs_subdev *ssd = to_ccs_subdev(subdev);
 	struct v4l2_rect *comp, *crops[CCS_PADS];
-	struct v4l2_rect sink_fmt;
 	int ret;
 
-	ret = __ccs_sel_supported(subdev, sel);
+	ret = ccs_sel_supported(subdev, sel);
 	if (ret)
 		return ret;
 
-	ccs_get_crop_compose(subdev, sd_state, crops, &comp, sel->which);
-
-	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		sink_fmt = ssd->sink_fmt;
-	} else {
-		struct v4l2_mbus_framefmt *fmt =
-			v4l2_subdev_get_try_format(subdev, sd_state,
-						   ssd->sink_pad);
-
-		sink_fmt.left = 0;
-		sink_fmt.top = 0;
-		sink_fmt.width = fmt->width;
-		sink_fmt.height = fmt->height;
-	}
+	ccs_get_crop_compose(subdev, sd_state, crops, &comp);
 
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP_BOUNDS:
 	case V4L2_SEL_TGT_NATIVE_SIZE:
-		if (ssd == sensor->pixel_array)
+		if (ssd == sensor->pixel_array) {
 			ccs_get_native_size(ssd, &sel->r);
-		else if (sel->pad == ssd->sink_pad)
-			sel->r = sink_fmt;
-		else
+		} else if (sel->pad == ssd->sink_pad) {
+			struct v4l2_mbus_framefmt *sink_fmt =
+				v4l2_subdev_get_pad_format(subdev, sd_state,
+							   ssd->sink_pad);
+			sel->r.top = sel->r.left = 0;
+			sel->r.width = sink_fmt->width;
+			sel->r.height = sink_fmt->height;
+		} else {
 			sel->r = *comp;
+		}
 		break;
 	case V4L2_SEL_TGT_CROP:
 	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
@@ -2652,20 +2587,6 @@ static int __ccs_get_selection(struct v4l2_subdev *subdev,
 	return 0;
 }
 
-static int ccs_get_selection(struct v4l2_subdev *subdev,
-			     struct v4l2_subdev_state *sd_state,
-			     struct v4l2_subdev_selection *sel)
-{
-	struct ccs_sensor *sensor = to_ccs_sensor(subdev);
-	int rval;
-
-	mutex_lock(&sensor->mutex);
-	rval = __ccs_get_selection(subdev, sd_state, sel);
-	mutex_unlock(&sensor->mutex);
-
-	return rval;
-}
-
 static int ccs_set_selection(struct v4l2_subdev *subdev,
 			     struct v4l2_subdev_state *sd_state,
 			     struct v4l2_subdev_selection *sel)
@@ -2673,7 +2594,7 @@ static int ccs_set_selection(struct v4l2_subdev *subdev,
 	struct ccs_sensor *sensor = to_ccs_sensor(subdev);
 	int ret;
 
-	ret = __ccs_sel_supported(subdev, sel);
+	ret = ccs_sel_supported(subdev, sel);
 	if (ret)
 		return ret;
 
@@ -2964,10 +2885,14 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
 		return rval;
 	}
 
+	rval = v4l2_subdev_init_finalize(&ssd->sd);
+	if (rval)
+		goto out_media_entity_cleanup;
+
 	rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev, &ssd->sd);
 	if (rval) {
 		dev_err(&client->dev, "v4l2_device_register_subdev failed\n");
-		goto out_media_entity_cleanup;
+		goto out_v4l2_subdev_cleanup;
 	}
 
 	rval = media_create_pad_link(&ssd->sd.entity, source_pad,
@@ -2983,6 +2908,9 @@ static int ccs_register_subdev(struct ccs_sensor *sensor,
 out_v4l2_device_unregister_subdev:
 	v4l2_device_unregister_subdev(&ssd->sd);
 
+out_v4l2_subdev_cleanup:
+	v4l2_subdev_cleanup(&ssd->sd);
+
 out_media_entity_cleanup:
 	media_entity_cleanup(&ssd->sd.entity);
 
@@ -3059,16 +2987,9 @@ static void ccs_create_subdev(struct ccs_sensor *sensor,
 
 	v4l2_i2c_subdev_set_name(&ssd->sd, client, sensor->minfo.name, name);
 
-	ccs_get_native_size(ssd, &ssd->sink_fmt);
-
-	ssd->compose.width = ssd->sink_fmt.width;
-	ssd->compose.height = ssd->sink_fmt.height;
-	ssd->crop[ssd->source_pad] = ssd->compose;
 	ssd->pads[ssd->source_pad].flags = MEDIA_PAD_FL_SOURCE;
-	if (ssd != sensor->pixel_array) {
-		ssd->crop[ssd->sink_pad] = ssd->compose;
+	if (ssd != sensor->pixel_array)
 		ssd->pads[ssd->sink_pad].flags = MEDIA_PAD_FL_SINK;
-	}
 
 	ssd->sd.entity.ops = &ccs_entity_ops;
 
@@ -3090,24 +3011,24 @@ static int ccs_init_cfg(struct v4l2_subdev *sd,
 	mutex_lock(&sensor->mutex);
 
 	for (i = 0; i < ssd->npads; i++) {
-		struct v4l2_mbus_framefmt *try_fmt =
-			v4l2_subdev_get_try_format(sd, sd_state, i);
-		struct v4l2_rect *try_crop =
-			v4l2_subdev_get_try_crop(sd, sd_state, i);
-		struct v4l2_rect *try_comp;
+		struct v4l2_mbus_framefmt *fmt =
+			v4l2_subdev_get_pad_format(sd, sd_state, i);
+		struct v4l2_rect *crop =
+			v4l2_subdev_get_pad_crop(sd, sd_state, i);
+		struct v4l2_rect *comp;
 
-		ccs_get_native_size(ssd, try_crop);
+		ccs_get_native_size(ssd, crop);
 
-		try_fmt->width = try_crop->width;
-		try_fmt->height = try_crop->height;
-		try_fmt->code = sensor->internal_csi_format->code;
-		try_fmt->field = V4L2_FIELD_NONE;
+		fmt->width = crop->width;
+		fmt->height = crop->height;
+		fmt->code = sensor->internal_csi_format->code;
+		fmt->field = V4L2_FIELD_NONE;
 
 		if (ssd == sensor->pixel_array)
 			continue;
 
-		try_comp = v4l2_subdev_get_try_compose(sd, sd_state, i);
-		*try_comp = *try_crop;
+		comp = v4l2_subdev_get_pad_compose(sd, sd_state, i);
+		*comp = *crop;
 	}
 
 	mutex_unlock(&sensor->mutex);
@@ -3632,6 +3553,10 @@ static int ccs_probe(struct i2c_client *client)
 	if (rval < 0)
 		goto out_media_entity_cleanup;
 
+	rval = v4l2_subdev_init_finalize(&sensor->src->sd);
+	if (rval)
+		goto out_media_entity_cleanup;
+
 	rval = ccs_write_msr_regs(sensor);
 	if (rval)
 		goto out_media_entity_cleanup;
@@ -3691,6 +3616,7 @@ static void ccs_remove(struct i2c_client *client)
 
 	for (i = 0; i < sensor->ssds_used; i++) {
 		v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
+		v4l2_subdev_cleanup(subdev);
 		media_entity_cleanup(&sensor->ssds[i].sd.entity);
 	}
 	ccs_cleanup(sensor);
diff --git a/drivers/media/i2c/ccs/ccs.h b/drivers/media/i2c/ccs/ccs.h
index a94c796cea48..9c3587b2fbe7 100644
--- a/drivers/media/i2c/ccs/ccs.h
+++ b/drivers/media/i2c/ccs/ccs.h
@@ -182,9 +182,6 @@ struct ccs_binning_subtype {
 struct ccs_subdev {
 	struct v4l2_subdev sd;
 	struct media_pad pads[CCS_PADS];
-	struct v4l2_rect sink_fmt;
-	struct v4l2_rect crop[CCS_PADS];
-	struct v4l2_rect compose; /* compose on sink */
 	unsigned short sink_pad;
 	unsigned short source_pad;
 	int npads;
@@ -220,6 +217,7 @@ struct ccs_sensor {
 	u32 mbus_frame_fmts;
 	const struct ccs_csi_data_format *csi_format;
 	const struct ccs_csi_data_format *internal_csi_format;
+	struct v4l2_rect pa_src, scaler_sink, src_src;
 	u32 default_mbus_frame_fmts;
 	int default_pixel_order;
 	struct ccs_data_container sdata, mdata;
-- 
2.39.2

[PATCH v3 07/12] media: ov2740: Enable runtime PM before registering the async subdev

[Sakari Ailus]

Enable runtime PM before registering the async subdev as the driver UAPI
may become accessible immediately after the registration. Runtime PM needs
to be enabled by that time.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov2740.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 41d4f85470fd..319dc00e47b4 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -1172,6 +1172,12 @@ static int ov2740_probe(struct i2c_client *client)
 		goto probe_error_v4l2_ctrl_handler_free;
 	}
 
+	/* Set the device's state to active if it's in D0 state. */
+	if (full_power)
+		pm_runtime_set_active(&client->dev);
+	pm_runtime_enable(&client->dev);
+	pm_runtime_idle(&client->dev);
+
 	ret = v4l2_async_register_subdev_sensor(&ov2740->sd);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "failed to register V4L2 subdev\n");
@@ -1182,16 +1188,12 @@ static int ov2740_probe(struct i2c_client *client)
 	if (ret)
 		dev_warn(&client->dev, "register nvmem failed, ret %d\n", ret);
 
-	/* Set the device's state to active if it's in D0 state. */
-	if (full_power)
-		pm_runtime_set_active(&client->dev);
-	pm_runtime_enable(&client->dev);
-	pm_runtime_idle(&client->dev);
-
 	return 0;
 
 probe_error_media_entity_cleanup:
 	media_entity_cleanup(&ov2740->sd.entity);
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
 
 probe_error_v4l2_ctrl_handler_free:
 	v4l2_ctrl_handler_free(ov2740->sd.ctrl_handler);
-- 
2.39.2

[PATCH v3 08/12] media: ov2740: Use sub-device active state

[Sakari Ailus]

Use sub-device active state. Rely on control handler lock to serialise
access to the active state. Also clean up locking on s_stream handler.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov2740.c | 122 +++++++++++++++----------------------
 1 file changed, 50 insertions(+), 72 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 319dc00e47b4..2c00e653ec47 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -336,9 +336,6 @@ struct ov2740 {
 	/* Current mode */
 	const struct ov2740_mode *cur_mode;
 
-	/* To serialize asynchronus callbacks */
-	struct mutex mutex;
-
 	/* Streaming on/off */
 	bool streaming;
 
@@ -582,7 +579,6 @@ static int ov2740_init_controls(struct ov2740 *ov2740)
 	if (ret)
 		return ret;
 
-	ctrl_hdlr->lock = &ov2740->mutex;
 	cur_mode = ov2740->cur_mode;
 	size = ARRAY_SIZE(link_freq_menu_items);
 
@@ -792,18 +788,18 @@ static int ov2740_set_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct ov2740 *ov2740 = to_ov2740(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct v4l2_subdev_state *sd_state;
 	int ret = 0;
 
+	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
+
 	if (ov2740->streaming == enable)
-		return 0;
+		goto out_unlock;
 
-	mutex_lock(&ov2740->mutex);
 	if (enable) {
 		ret = pm_runtime_resume_and_get(&client->dev);
-		if (ret < 0) {
-			mutex_unlock(&ov2740->mutex);
-			return ret;
-		}
+		if (ret < 0)
+			goto out_unlock;
 
 		ret = ov2740_start_streaming(ov2740);
 		if (ret) {
@@ -817,7 +813,9 @@ static int ov2740_set_stream(struct v4l2_subdev *sd, int enable)
 	}
 
 	ov2740->streaming = enable;
-	mutex_unlock(&ov2740->mutex);
+
+out_unlock:
+	v4l2_subdev_unlock_state(sd_state);
 
 	return ret;
 }
@@ -826,12 +824,13 @@ static int ov2740_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct ov2740 *ov2740 = to_ov2740(sd);
+	struct v4l2_subdev_state *sd_state;
 
-	mutex_lock(&ov2740->mutex);
+	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
 	if (ov2740->streaming)
 		ov2740_stop_streaming(ov2740);
 
-	mutex_unlock(&ov2740->mutex);
+	v4l2_subdev_unlock_state(sd_state);
 
 	return 0;
 }
@@ -840,9 +839,10 @@ static int ov2740_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct ov2740 *ov2740 = to_ov2740(sd);
+	struct v4l2_subdev_state *sd_state;
 	int ret = 0;
 
-	mutex_lock(&ov2740->mutex);
+	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
 	if (!ov2740->streaming)
 		goto exit;
 
@@ -853,7 +853,7 @@ static int ov2740_resume(struct device *dev)
 	}
 
 exit:
-	mutex_unlock(&ov2740->mutex);
+	v4l2_subdev_unlock_state(sd_state);
 	return ret;
 }
 
@@ -870,48 +870,26 @@ static int ov2740_set_format(struct v4l2_subdev *sd,
 				      height, fmt->format.width,
 				      fmt->format.height);
 
-	mutex_lock(&ov2740->mutex);
 	ov2740_update_pad_format(mode, &fmt->format);
-	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-		*v4l2_subdev_get_try_format(sd, sd_state, fmt->pad) = fmt->format;
-	} else {
-		ov2740->cur_mode = mode;
-		__v4l2_ctrl_s_ctrl(ov2740->link_freq, mode->link_freq_index);
-		__v4l2_ctrl_s_ctrl_int64(ov2740->pixel_rate,
-					 to_pixel_rate(mode->link_freq_index));
-
-		/* Update limits and set FPS to default */
-		vblank_def = mode->vts_def - mode->height;
-		__v4l2_ctrl_modify_range(ov2740->vblank,
-					 mode->vts_min - mode->height,
-					 OV2740_VTS_MAX - mode->height, 1,
-					 vblank_def);
-		__v4l2_ctrl_s_ctrl(ov2740->vblank, vblank_def);
-		h_blank = to_pixels_per_line(mode->hts, mode->link_freq_index) -
-			  mode->width;
-		__v4l2_ctrl_modify_range(ov2740->hblank, h_blank, h_blank, 1,
-					 h_blank);
-	}
-	mutex_unlock(&ov2740->mutex);
-
-	return 0;
-}
+	*v4l2_subdev_get_pad_format(sd, sd_state, fmt->pad) = fmt->format;
 
-static int ov2740_get_format(struct v4l2_subdev *sd,
-			     struct v4l2_subdev_state *sd_state,
-			     struct v4l2_subdev_format *fmt)
-{
-	struct ov2740 *ov2740 = to_ov2740(sd);
-
-	mutex_lock(&ov2740->mutex);
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
-		fmt->format = *v4l2_subdev_get_try_format(&ov2740->sd,
-							  sd_state,
-							  fmt->pad);
-	else
-		ov2740_update_pad_format(ov2740->cur_mode, &fmt->format);
+		return 0;
 
-	mutex_unlock(&ov2740->mutex);
+	ov2740->cur_mode = mode;
+	__v4l2_ctrl_s_ctrl(ov2740->link_freq, mode->link_freq_index);
+	__v4l2_ctrl_s_ctrl_int64(ov2740->pixel_rate,
+				 to_pixel_rate(mode->link_freq_index));
+
+	/* Update limits and set FPS to default */
+	vblank_def = mode->vts_def - mode->height;
+	__v4l2_ctrl_modify_range(ov2740->vblank,
+				 mode->vts_min - mode->height,
+				 OV2740_VTS_MAX - mode->height, 1, vblank_def);
+	__v4l2_ctrl_s_ctrl(ov2740->vblank, vblank_def);
+	h_blank = to_pixels_per_line(mode->hts, mode->link_freq_index) -
+		mode->width;
+	__v4l2_ctrl_modify_range(ov2740->hblank, h_blank, h_blank, 1, h_blank);
 
 	return 0;
 }
@@ -946,14 +924,11 @@ static int ov2740_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int ov2740_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+static int ov2740_init_cfg(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_state *sd_state)
 {
-	struct ov2740 *ov2740 = to_ov2740(sd);
-
-	mutex_lock(&ov2740->mutex);
 	ov2740_update_pad_format(&supported_modes[0],
-				 v4l2_subdev_get_try_format(sd, fh->state, 0));
-	mutex_unlock(&ov2740->mutex);
+				 v4l2_subdev_get_pad_format(sd, sd_state, 0));
 
 	return 0;
 }
@@ -963,10 +938,11 @@ static const struct v4l2_subdev_video_ops ov2740_video_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops ov2740_pad_ops = {
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = ov2740_set_format,
-	.get_fmt = ov2740_get_format,
 	.enum_mbus_code = ov2740_enum_mbus_code,
 	.enum_frame_size = ov2740_enum_frame_size,
+	.init_cfg = ov2740_init_cfg,
 };
 
 static const struct v4l2_subdev_ops ov2740_subdev_ops = {
@@ -978,10 +954,6 @@ static const struct media_entity_operations ov2740_subdev_entity_ops = {
 	.link_validate = v4l2_subdev_link_validate,
 };
 
-static const struct v4l2_subdev_internal_ops ov2740_internal_ops = {
-	.open = ov2740_open,
-};
-
 static int ov2740_check_hwcfg(struct device *dev)
 {
 	struct fwnode_handle *ep;
@@ -1047,13 +1019,12 @@ static int ov2740_check_hwcfg(struct device *dev)
 static void ov2740_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
-	struct ov2740 *ov2740 = to_ov2740(sd);
 
 	v4l2_async_unregister_subdev(sd);
 	media_entity_cleanup(&sd->entity);
+	v4l2_subdev_cleanup(sd);
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 	pm_runtime_disable(&client->dev);
-	mutex_destroy(&ov2740->mutex);
 }
 
 static int ov2740_nvmem_read(void *priv, unsigned int off, void *val,
@@ -1062,9 +1033,11 @@ static int ov2740_nvmem_read(void *priv, unsigned int off, void *val,
 	struct nvm_data *nvm = priv;
 	struct device *dev = regmap_get_device(nvm->regmap);
 	struct ov2740 *ov2740 = to_ov2740(dev_get_drvdata(dev));
+	struct v4l2_subdev_state *sd_state;
 	int ret = 0;
 
-	mutex_lock(&ov2740->mutex);
+	/* Serialise sensor access */
+	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
 
 	if (nvm->nvm_buffer) {
 		memcpy(val, nvm->nvm_buffer + off, count);
@@ -1082,7 +1055,7 @@ static int ov2740_nvmem_read(void *priv, unsigned int off, void *val,
 
 	pm_runtime_put(dev);
 exit:
-	mutex_unlock(&ov2740->mutex);
+	v4l2_subdev_unlock_state(sd_state);
 	return ret;
 }
 
@@ -1153,7 +1126,6 @@ static int ov2740_probe(struct i2c_client *client)
 			return dev_err_probe(dev, ret, "failed to find sensor\n");
 	}
 
-	mutex_init(&ov2740->mutex);
 	ov2740->cur_mode = &supported_modes[0];
 	ret = ov2740_init_controls(ov2740);
 	if (ret) {
@@ -1161,7 +1133,7 @@ static int ov2740_probe(struct i2c_client *client)
 		goto probe_error_v4l2_ctrl_handler_free;
 	}
 
-	ov2740->sd.internal_ops = &ov2740_internal_ops;
+	ov2740->sd.state_lock = ov2740->ctrl_handler.lock;
 	ov2740->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	ov2740->sd.entity.ops = &ov2740_subdev_entity_ops;
 	ov2740->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
@@ -1172,6 +1144,10 @@ static int ov2740_probe(struct i2c_client *client)
 		goto probe_error_v4l2_ctrl_handler_free;
 	}
 
+	ret = v4l2_subdev_init_finalize(&ov2740->sd);
+	if (ret)
+		goto probe_error_media_entity_cleanup;
+
 	/* Set the device's state to active if it's in D0 state. */
 	if (full_power)
 		pm_runtime_set_active(&client->dev);
@@ -1181,7 +1157,7 @@ static int ov2740_probe(struct i2c_client *client)
 	ret = v4l2_async_register_subdev_sensor(&ov2740->sd);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "failed to register V4L2 subdev\n");
-		goto probe_error_media_entity_cleanup;
+		goto probe_error_v4l2_subdev_cleanup;
 	}
 
 	ret = ov2740_register_nvmem(client, ov2740);
@@ -1190,6 +1166,9 @@ static int ov2740_probe(struct i2c_client *client)
 
 	return 0;
 
+probe_error_v4l2_subdev_cleanup:
+	v4l2_subdev_cleanup(&ov2740->sd);
+
 probe_error_media_entity_cleanup:
 	media_entity_cleanup(&ov2740->sd.entity);
 	pm_runtime_disable(&client->dev);
@@ -1197,7 +1176,6 @@ static int ov2740_probe(struct i2c_client *client)
 
 probe_error_v4l2_ctrl_handler_free:
 	v4l2_ctrl_handler_free(ov2740->sd.ctrl_handler);
-	mutex_destroy(&ov2740->mutex);
 
 	return ret;
 }
-- 
2.39.2

[PATCH v3 09/12] media: ov2740: Return -EPROBE_DEFER if no endpoint is found

[Sakari Ailus]

With ipu bridge, endpoints may only be created when ipu bridge has
initialised. This may happen after the sensor driver has first probed.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ov2740.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 2c00e653ec47..ccbb15e730ae 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -976,7 +976,7 @@ static int ov2740_check_hwcfg(struct device *dev)
 
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
 	if (!ep)
-		return -ENXIO;
+		return -EPROBE_DEFER;
 
 	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
 	fwnode_handle_put(ep);
-- 
2.39.2

[PATCH v3 10/12] media: v4l: subdev: Clear frame descriptor before get_frame_desc

[Sakari Ailus]

Clear frame descriptor before calling transmitter's get_frame_desc() op.
Also remove the corresponding memset() calls from drivers.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/i2c/ds90ub913.c              | 2 --
 drivers/media/i2c/ds90ub953.c              | 2 --
 drivers/media/i2c/ds90ub960.c              | 2 --
 drivers/media/platform/nxp/imx-mipi-csis.c | 2 --
 drivers/media/v4l2-core/v4l2-subdev.c      | 9 +++++++++
 5 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index 4bfa3b3cf619..8e9ebed09f64 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -362,8 +362,6 @@ static int ub913_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 	if (ret)
 		return ret;
 
-	memset(fd, 0, sizeof(*fd));
-
 	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL;
 
 	state = v4l2_subdev_lock_and_get_active_state(sd);
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c
index dc394e22a42c..644022312833 100644
--- a/drivers/media/i2c/ds90ub953.c
+++ b/drivers/media/i2c/ds90ub953.c
@@ -499,8 +499,6 @@ static int ub953_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 	if (ret)
 		return ret;
 
-	memset(fd, 0, sizeof(*fd));
-
 	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
 
 	state = v4l2_subdev_lock_and_get_active_state(sd);
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 8ba5750f5a23..b8f3e5ca03ef 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -2786,8 +2786,6 @@ static int ub960_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 	if (!ub960_pad_is_source(priv, pad))
 		return -EINVAL;
 
-	memset(fd, 0, sizeof(*fd));
-
 	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
 
 	state = v4l2_subdev_lock_and_get_active_state(&priv->sd);
diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 16f19a640130..aac9cffe503c 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -1114,8 +1114,6 @@ static int mipi_csis_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL;
 	fd->num_entries = 1;
 
-	memset(entry, 0, sizeof(*entry));
-
 	entry->flags = 0;
 	entry->pixelcode = csis_fmt->code;
 	entry->bus.csi2.vc = 0;
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index b92348ad61f6..7b087be3ff4f 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -306,6 +306,14 @@ static int call_set_selection(struct v4l2_subdev *sd,
 	       sd->ops->pad->set_selection(sd, state, sel);
 }
 
+static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+			       struct v4l2_mbus_frame_desc *fd)
+{
+	memset(fd, 0, sizeof(*fd));
+
+	return sd->ops->pad->get_frame_desc(sd, pad, fd);
+}
+
 static inline int check_edid(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_edid *edid)
 {
@@ -431,6 +439,7 @@ static const struct v4l2_subdev_pad_ops v4l2_subdev_call_pad_wrappers = {
 	.set_edid		= call_set_edid,
 	.dv_timings_cap		= call_dv_timings_cap,
 	.enum_dv_timings	= call_enum_dv_timings,
+	.get_frame_desc		= call_get_frame_desc,
 	.get_mbus_config	= call_get_mbus_config,
 };
 
-- 
2.39.2

[PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Sakari Ailus]

Print debug level information on returned frame descriptors.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 7b087be3ff4f..504ca625b2bd 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/overflow.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include <linux/types.h>
 #include <linux/version.h>
 #include <linux/videodev2.h>
@@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
 static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 			       struct v4l2_mbus_frame_desc *fd)
 {
+	unsigned int i;
+	int ret;
+
 	memset(fd, 0, sizeof(*fd));
 
-	return sd->ops->pad->get_frame_desc(sd, pad, fd);
+	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
+	if (ret)
+		return ret;
+
+	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
+		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
+		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
+		"unknown");
+
+	for (i = 0; i < fd->num_entries; i++) {
+		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
+		char buf[20] = "";
+
+		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
+			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
+				 entry->bus.csi2.vc, entry->bus.csi2.dt);
+
+		dev_dbg(sd->dev,
+			"\tstream %u, code 0x%4.4x, lenght %u, flags%s%s%s\n",
+			entry->stream, entry->pixelcode, entry->length,
+			entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
+			" LEN_MAX" : "",
+			entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
+			" BLOB" : "", buf);
+	}
+
+	return 0;
 }
 
 static inline int check_edid(struct v4l2_subdev *sd,
-- 
2.39.2

[PATCH v3 12/12] media: mc: Check pad flag validity

[Sakari Ailus]

Check the validity of pad flags on entity init. Exactly one of the flags
must be set.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/mc/mc-entity.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 83468d4a440b..0a54cf8bcca2 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -197,6 +197,7 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
 	struct media_device *mdev = entity->graph_obj.mdev;
 	struct media_pad *iter;
 	unsigned int i = 0;
+	int ret = 0;
 
 	if (num_pads >= MEDIA_ENTITY_MAX_PADS)
 		return -E2BIG;
@@ -210,15 +211,26 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
 	media_entity_for_each_pad(entity, iter) {
 		iter->entity = entity;
 		iter->index = i++;
+
+		if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK |
+					     MEDIA_PAD_FL_SOURCE)) != 1) {
+			ret = -EINVAL;
+			break;
+		}
+
 		if (mdev)
 			media_gobj_create(mdev, MEDIA_GRAPH_PAD,
 					  &iter->graph_obj);
 	}
 
+	if (ret && mdev)
+		media_entity_for_each_pad(entity, iter)
+			media_gobj_destroy(&iter->graph_obj);
+
 	if (mdev)
 		mutex_unlock(&mdev->graph_mutex);
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(media_entity_pads_init);
 
-- 
2.39.2

Re: [PATCH v3 12/12] media: mc: Check pad flag validity

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tue, Sep 19, 2023 at 03:17:28PM +0300, Sakari Ailus wrote:
> Check the validity of pad flags on entity init. Exactly one of the flags
> must be set.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/mc/mc-entity.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 83468d4a440b..0a54cf8bcca2 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -197,6 +197,7 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
>  	struct media_device *mdev = entity->graph_obj.mdev;
>  	struct media_pad *iter;
>  	unsigned int i = 0;
> +	int ret = 0;
>  
>  	if (num_pads >= MEDIA_ENTITY_MAX_PADS)
>  		return -E2BIG;
> @@ -210,15 +211,26 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
>  	media_entity_for_each_pad(entity, iter) {
>  		iter->entity = entity;
>  		iter->index = i++;
> +
> +		if (hweight32(iter->flags & (MEDIA_PAD_FL_SINK |
> +					     MEDIA_PAD_FL_SOURCE)) != 1) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
>  		if (mdev)
>  			media_gobj_create(mdev, MEDIA_GRAPH_PAD,
>  					  &iter->graph_obj);
>  	}
>  
> +	if (ret && mdev)
> +		media_entity_for_each_pad(entity, iter)
> +			media_gobj_destroy(&iter->graph_obj);

With curly braces for the if () { ... },

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

> +
>  	if (mdev)
>  		mutex_unlock(&mdev->graph_mutex);
>  
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(media_entity_pads_init);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
> Print debug level information on returned frame descriptors.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 7b087be3ff4f..504ca625b2bd 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/overflow.h>
>  #include <linux/slab.h>
> +#include <linux/string.h>
>  #include <linux/types.h>
>  #include <linux/version.h>
>  #include <linux/videodev2.h>
> @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
>  static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>  			       struct v4l2_mbus_frame_desc *fd)
>  {
> +	unsigned int i;
> +	int ret;
> +
>  	memset(fd, 0, sizeof(*fd));
>  
> -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
> +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> +		"unknown");
> +
> +	for (i = 0; i < fd->num_entries; i++) {
> +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> +		char buf[20] = "";

Should this be sized for the worst case ? The vc and dt should not be
large, but a buffer overflow on the stack in debug code if a subdev
returns an incorrect value would be bad.

> +
> +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",

0x%02x would be one character shorter ;-) Same below.

> +				 entry->bus.csi2.vc, entry->bus.csi2.dt);
> +
> +		dev_dbg(sd->dev,
> +			"\tstream %u, code 0x%4.4x, lenght %u, flags%s%s%s\n",

s/lenght/length/

> +			entry->stream, entry->pixelcode, entry->length,
> +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
> +			" LEN_MAX" : "",
> +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
> +			" BLOB" : "", buf);

If no flags are set, you will get something like

	stream 0, code 0x1234, length ..., flags, vc 0, dt 0x2a

Maybe printing the hex value for the flags would be simpler and clearer
?

> +	}
> +
> +	return 0;
>  }
>  
>  static inline int check_edid(struct v4l2_subdev *sd,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
> > Print debug level information on returned frame descriptors.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 7b087be3ff4f..504ca625b2bd 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/module.h>
> >  #include <linux/overflow.h>
> >  #include <linux/slab.h>
> > +#include <linux/string.h>
> >  #include <linux/types.h>
> >  #include <linux/version.h>
> >  #include <linux/videodev2.h>
> > @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
> >  static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> >  			       struct v4l2_mbus_frame_desc *fd)
> >  {
> > +	unsigned int i;
> > +	int ret;
> > +
> >  	memset(fd, 0, sizeof(*fd));
> >  
> > -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
> > +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
> > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> > +		"unknown");
> > +
> > +	for (i = 0; i < fd->num_entries; i++) {
> > +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> > +		char buf[20] = "";
> 
> Should this be sized for the worst case ? The vc and dt should not be
> large, but a buffer overflow on the stack in debug code if a subdev
> returns an incorrect value would be bad.

17 should be enough but it's not useful to use a size not divisible by 4 in
practice here.

> 
> > +
> > +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> > +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
> 
> 0x%02x would be one character shorter ;-) Same below.

It would be, but I prefer the above notation as it's more generic.

> 
> > +				 entry->bus.csi2.vc, entry->bus.csi2.dt);
> > +
> > +		dev_dbg(sd->dev,
> > +			"\tstream %u, code 0x%4.4x, lenght %u, flags%s%s%s\n",
> 
> s/lenght/length/

Thanks, I'll fix this.

> 
> > +			entry->stream, entry->pixelcode, entry->length,
> > +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
> > +			" LEN_MAX" : "",
> > +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
> > +			" BLOB" : "", buf);
> 
> If no flags are set, you will get something like
> 
> 	stream 0, code 0x1234, length ..., flags, vc 0, dt 0x2a
> 
> Maybe printing the hex value for the flags would be simpler and clearer
> ?

How about adding the numerical value in addition to the strings?

The use of these flags is rare though. I could just remove the strings,
too.

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Laurent Pinchart]

On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > Thank you for the patch.
> > 
> > On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
> > > Print debug level information on returned frame descriptors.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
> > >  1 file changed, 31 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 7b087be3ff4f..504ca625b2bd 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -15,6 +15,7 @@
> > >  #include <linux/module.h>
> > >  #include <linux/overflow.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/string.h>
> > >  #include <linux/types.h>
> > >  #include <linux/version.h>
> > >  #include <linux/videodev2.h>
> > > @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
> > >  static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > >  			       struct v4l2_mbus_frame_desc *fd)
> > >  {
> > > +	unsigned int i;
> > > +	int ret;
> > > +
> > >  	memset(fd, 0, sizeof(*fd));
> > >  
> > > -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
> > > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> > > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> > > +		"unknown");
> > > +
> > > +	for (i = 0; i < fd->num_entries; i++) {
> > > +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> > > +		char buf[20] = "";
> > 
> > Should this be sized for the worst case ? The vc and dt should not be
> > large, but a buffer overflow on the stack in debug code if a subdev
> > returns an incorrect value would be bad.
> 
> 17 should be enough but it's not useful to use a size not divisible by 4 in
> practice here.

18 with the terminating 0. But indeed, it's large enough as vc and dt
are u8. I'm just a bit worried we're opening the door to hard to debug
problems if we later change the vc and dt types.

> > > +
> > > +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> > > +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
> > 
> > 0x%02x would be one character shorter ;-) Same below.
> 
> It would be, but I prefer the above notation as it's more generic.

Out of curiosity, how so ?

> > > +				 entry->bus.csi2.vc, entry->bus.csi2.dt);
> > > +
> > > +		dev_dbg(sd->dev,
> > > +			"\tstream %u, code 0x%4.4x, lenght %u, flags%s%s%s\n",
> > 
> > s/lenght/length/
> 
> Thanks, I'll fix this.
> 
> > > +			entry->stream, entry->pixelcode, entry->length,
> > > +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
> > > +			" LEN_MAX" : "",
> > > +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
> > > +			" BLOB" : "", buf);
> > 
> > If no flags are set, you will get something like
> > 
> > 	stream 0, code 0x1234, length ..., flags, vc 0, dt 0x2a
> > 
> > Maybe printing the hex value for the flags would be simpler and clearer
> > ?
> 
> How about adding the numerical value in addition to the strings?
> 
> The use of these flags is rare though. I could just remove the strings,
> too.

Up to you.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Sakari Ailus]

On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote:
> On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote:
> > > Hi Sakari,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
> > > > Print debug level information on returned frame descriptors.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
> > > >  1 file changed, 31 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > index 7b087be3ff4f..504ca625b2bd 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > @@ -15,6 +15,7 @@
> > > >  #include <linux/module.h>
> > > >  #include <linux/overflow.h>
> > > >  #include <linux/slab.h>
> > > > +#include <linux/string.h>
> > > >  #include <linux/types.h>
> > > >  #include <linux/version.h>
> > > >  #include <linux/videodev2.h>
> > > > @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
> > > >  static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > > >  			       struct v4l2_mbus_frame_desc *fd)
> > > >  {
> > > > +	unsigned int i;
> > > > +	int ret;
> > > > +
> > > >  	memset(fd, 0, sizeof(*fd));
> > > >  
> > > > -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > > +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
> > > > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> > > > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> > > > +		"unknown");
> > > > +
> > > > +	for (i = 0; i < fd->num_entries; i++) {
> > > > +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> > > > +		char buf[20] = "";
> > > 
> > > Should this be sized for the worst case ? The vc and dt should not be
> > > large, but a buffer overflow on the stack in debug code if a subdev
> > > returns an incorrect value would be bad.
> > 
> > 17 should be enough but it's not useful to use a size not divisible by 4 in
> > practice here.
> 
> 18 with the terminating 0. But indeed, it's large enough as vc and dt

I can count only 17 --- there's no newline.

I guess it's most probably either of these then. X-)

> are u8. I'm just a bit worried we're opening the door to hard to debug
> problems if we later change the vc and dt types.

I can add a WARN_ON() to cover this.

> 
> > > > +
> > > > +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> > > > +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
> > > 
> > > 0x%02x would be one character shorter ;-) Same below.
> > 
> > It would be, but I prefer the above notation as it's more generic.
> 
> Out of curiosity, how so ?

It works with data that would span more than 9 characters when printed.

> 
> > > > +				 entry->bus.csi2.vc, entry->bus.csi2.dt);
> > > > +
> > > > +		dev_dbg(sd->dev,
> > > > +			"\tstream %u, code 0x%4.4x, lenght %u, flags%s%s%s\n",
> > > 
> > > s/lenght/length/
> > 
> > Thanks, I'll fix this.
> > 
> > > > +			entry->stream, entry->pixelcode, entry->length,
> > > > +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
> > > > +			" LEN_MAX" : "",
> > > > +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
> > > > +			" BLOB" : "", buf);
> > > 
> > > If no flags are set, you will get something like
> > > 
> > > 	stream 0, code 0x1234, length ..., flags, vc 0, dt 0x2a
> > > 
> > > Maybe printing the hex value for the flags would be simpler and clearer
> > > ?
> > 
> > How about adding the numerical value in addition to the strings?
> > 
> > The use of these flags is rare though. I could just remove the strings,
> > too.
> 
> Up to you.

On second thought, I'll make it an integer only. If we'll get more commonly
used flags, we can rethink about strings.

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Laurent Pinchart]

On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote:
> On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote:
> > On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote:
> > > On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote:
> > > > On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
> > > > > Print debug level information on returned frame descriptors.
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
> > > > >  1 file changed, 31 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > index 7b087be3ff4f..504ca625b2bd 100644
> > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > @@ -15,6 +15,7 @@
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/overflow.h>
> > > > >  #include <linux/slab.h>
> > > > > +#include <linux/string.h>
> > > > >  #include <linux/types.h>
> > > > >  #include <linux/version.h>
> > > > >  #include <linux/videodev2.h>
> > > > > @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
> > > > >  static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > > > >  			       struct v4l2_mbus_frame_desc *fd)
> > > > >  {
> > > > > +	unsigned int i;
> > > > > +	int ret;
> > > > > +
> > > > >  	memset(fd, 0, sizeof(*fd));
> > > > >  
> > > > > -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > > > +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
> > > > > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> > > > > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> > > > > +		"unknown");
> > > > > +
> > > > > +	for (i = 0; i < fd->num_entries; i++) {
> > > > > +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> > > > > +		char buf[20] = "";
> > > > 
> > > > Should this be sized for the worst case ? The vc and dt should not be
> > > > large, but a buffer overflow on the stack in debug code if a subdev
> > > > returns an incorrect value would be bad.
> > > 
> > > 17 should be enough but it's not useful to use a size not divisible by 4 in
> > > practice here.
> > 
> > 18 with the terminating 0. But indeed, it's large enough as vc and dt
> 
> I can count only 17 --- there's no newline.
> 
> I guess it's most probably either of these then. X-)
> 
> > are u8. I'm just a bit worried we're opening the door to hard to debug
> > problems if we later change the vc and dt types.
> 
> I can add a WARN_ON() to cover this.
> 
> > > > > +
> > > > > +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> > > > > +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
> > > > 
> > > > 0x%02x would be one character shorter ;-) Same below.
> > > 
> > > It would be, but I prefer the above notation as it's more generic.
> > 
> > Out of curiosity, how so ?
> 
> It works with data that would span more than 9 characters when printed.

And 0x%02x doesn't ?

> > > > > +				 entry->bus.csi2.vc, entry->bus.csi2.dt);
> > > > > +
> > > > > +		dev_dbg(sd->dev,
> > > > > +			"\tstream %u, code 0x%4.4x, lenght %u, flags%s%s%s\n",
> > > > 
> > > > s/lenght/length/
> > > 
> > > Thanks, I'll fix this.
> > > 
> > > > > +			entry->stream, entry->pixelcode, entry->length,
> > > > > +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_LEN_MAX ?
> > > > > +			" LEN_MAX" : "",
> > > > > +			entry->flags & V4L2_MBUS_FRAME_DESC_FL_BLOB ?
> > > > > +			" BLOB" : "", buf);
> > > > 
> > > > If no flags are set, you will get something like
> > > > 
> > > > 	stream 0, code 0x1234, length ..., flags, vc 0, dt 0x2a
> > > > 
> > > > Maybe printing the hex value for the flags would be simpler and clearer
> > > > ?
> > > 
> > > How about adding the numerical value in addition to the strings?
> > > 
> > > The use of these flags is rare though. I could just remove the strings,
> > > too.
> > 
> > Up to you.
> 
> On second thought, I'll make it an integer only. If we'll get more commonly
> used flags, we can rethink about strings.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Sakari Ailus]

On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote:
> On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote:
> > On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote:
> > > On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote:
> > > > On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote:
> > > > > On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
> > > > > > Print debug level information on returned frame descriptors.
> > > > > > 
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
> > > > > >  1 file changed, 31 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > > index 7b087be3ff4f..504ca625b2bd 100644
> > > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > > @@ -15,6 +15,7 @@
> > > > > >  #include <linux/module.h>
> > > > > >  #include <linux/overflow.h>
> > > > > >  #include <linux/slab.h>
> > > > > > +#include <linux/string.h>
> > > > > >  #include <linux/types.h>
> > > > > >  #include <linux/version.h>
> > > > > >  #include <linux/videodev2.h>
> > > > > > @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
> > > > > >  static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > > > > >  			       struct v4l2_mbus_frame_desc *fd)
> > > > > >  {
> > > > > > +	unsigned int i;
> > > > > > +	int ret;
> > > > > > +
> > > > > >  	memset(fd, 0, sizeof(*fd));
> > > > > >  
> > > > > > -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > > > > +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
> > > > > > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> > > > > > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> > > > > > +		"unknown");
> > > > > > +
> > > > > > +	for (i = 0; i < fd->num_entries; i++) {
> > > > > > +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> > > > > > +		char buf[20] = "";
> > > > > 
> > > > > Should this be sized for the worst case ? The vc and dt should not be
> > > > > large, but a buffer overflow on the stack in debug code if a subdev
> > > > > returns an incorrect value would be bad.
> > > > 
> > > > 17 should be enough but it's not useful to use a size not divisible by 4 in
> > > > practice here.
> > > 
> > > 18 with the terminating 0. But indeed, it's large enough as vc and dt
> > 
> > I can count only 17 --- there's no newline.
> > 
> > I guess it's most probably either of these then. X-)
> > 
> > > are u8. I'm just a bit worried we're opening the door to hard to debug
> > > problems if we later change the vc and dt types.
> > 
> > I can add a WARN_ON() to cover this.
> > 
> > > > > > +
> > > > > > +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> > > > > > +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
> > > > > 
> > > > > 0x%02x would be one character shorter ;-) Same below.
> > > > 
> > > > It would be, but I prefer the above notation as it's more generic.
> > > 
> > > Out of curiosity, how so ?
> > 
> > It works with data that would span more than 9 characters when printed.
> 
> And 0x%02x doesn't ?

Ah, it should indeed work, 0 is actually a flag here and part of the field
width or precision. I can use that in v4.

-- 
Sakari Ailus

Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Tomi Valkeinen]

On 22/09/2023 13:09, Sakari Ailus wrote:
> On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote:
>> On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote:
>>> On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote:
>>>> On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote:
>>>>> On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote:
>>>>>> On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
>>>>>>> Print debug level information on returned frame descriptors.
>>>>>>>
>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>>> ---
>>>>>>>   drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
>>>>>>>   1 file changed, 31 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>> index 7b087be3ff4f..504ca625b2bd 100644
>>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>> @@ -15,6 +15,7 @@
>>>>>>>   #include <linux/module.h>
>>>>>>>   #include <linux/overflow.h>
>>>>>>>   #include <linux/slab.h>
>>>>>>> +#include <linux/string.h>
>>>>>>>   #include <linux/types.h>
>>>>>>>   #include <linux/version.h>
>>>>>>>   #include <linux/videodev2.h>
>>>>>>> @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
>>>>>>>   static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>>>>>>>   			       struct v4l2_mbus_frame_desc *fd)
>>>>>>>   {
>>>>>>> +	unsigned int i;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>>   	memset(fd, 0, sizeof(*fd));
>>>>>>>   
>>>>>>> -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
>>>>>>> +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
>>>>>>> +	if (ret)
>>>>>>> +		return ret;
>>>>>>> +
>>>>>>> +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
>>>>>>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
>>>>>>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
>>>>>>> +		"unknown");
>>>>>>> +
>>>>>>> +	for (i = 0; i < fd->num_entries; i++) {
>>>>>>> +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
>>>>>>> +		char buf[20] = "";
>>>>>>
>>>>>> Should this be sized for the worst case ? The vc and dt should not be
>>>>>> large, but a buffer overflow on the stack in debug code if a subdev
>>>>>> returns an incorrect value would be bad.
>>>>>
>>>>> 17 should be enough but it's not useful to use a size not divisible by 4 in
>>>>> practice here.
>>>>
>>>> 18 with the terminating 0. But indeed, it's large enough as vc and dt
>>>
>>> I can count only 17 --- there's no newline.
>>>
>>> I guess it's most probably either of these then. X-)
>>>
>>>> are u8. I'm just a bit worried we're opening the door to hard to debug
>>>> problems if we later change the vc and dt types.
>>>
>>> I can add a WARN_ON() to cover this.
>>>
>>>>>>> +
>>>>>>> +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
>>>>>>> +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
>>>>>>
>>>>>> 0x%02x would be one character shorter ;-) Same below.
>>>>>
>>>>> It would be, but I prefer the above notation as it's more generic.
>>>>
>>>> Out of curiosity, how so ?
>>>
>>> It works with data that would span more than 9 characters when printed.
>>
>> And 0x%02x doesn't ?
> 
> Ah, it should indeed work, 0 is actually a flag here and part of the field
> width or precision. I can use that in v4.

Or %#04x, even shorter!

  Tomi

Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Laurent Pinchart]

On Fri, Sep 22, 2023 at 01:16:21PM +0300, Tomi Valkeinen wrote:
> On 22/09/2023 13:09, Sakari Ailus wrote:
> > On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote:
> >> On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote:
> >>> On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote:
> >>>> On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote:
> >>>>> On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote:
> >>>>>> On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
> >>>>>>> Print debug level information on returned frame descriptors.
> >>>>>>>
> >>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>>>>> ---
> >>>>>>>   drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
> >>>>>>>   1 file changed, 31 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>> index 7b087be3ff4f..504ca625b2bd 100644
> >>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>> @@ -15,6 +15,7 @@
> >>>>>>>   #include <linux/module.h>
> >>>>>>>   #include <linux/overflow.h>
> >>>>>>>   #include <linux/slab.h>
> >>>>>>> +#include <linux/string.h>
> >>>>>>>   #include <linux/types.h>
> >>>>>>>   #include <linux/version.h>
> >>>>>>>   #include <linux/videodev2.h>
> >>>>>>> @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
> >>>>>>>   static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> >>>>>>>   			       struct v4l2_mbus_frame_desc *fd)
> >>>>>>>   {
> >>>>>>> +	unsigned int i;
> >>>>>>> +	int ret;
> >>>>>>> +
> >>>>>>>   	memset(fd, 0, sizeof(*fd));
> >>>>>>>   
> >>>>>>> -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
> >>>>>>> +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> >>>>>>> +	if (ret)
> >>>>>>> +		return ret;
> >>>>>>> +
> >>>>>>> +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
> >>>>>>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> >>>>>>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> >>>>>>> +		"unknown");
> >>>>>>> +
> >>>>>>> +	for (i = 0; i < fd->num_entries; i++) {
> >>>>>>> +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> >>>>>>> +		char buf[20] = "";
> >>>>>>
> >>>>>> Should this be sized for the worst case ? The vc and dt should not be
> >>>>>> large, but a buffer overflow on the stack in debug code if a subdev
> >>>>>> returns an incorrect value would be bad.
> >>>>>
> >>>>> 17 should be enough but it's not useful to use a size not divisible by 4 in
> >>>>> practice here.
> >>>>
> >>>> 18 with the terminating 0. But indeed, it's large enough as vc and dt
> >>>
> >>> I can count only 17 --- there's no newline.
> >>>
> >>> I guess it's most probably either of these then. X-)
> >>>
> >>>> are u8. I'm just a bit worried we're opening the door to hard to debug
> >>>> problems if we later change the vc and dt types.
> >>>
> >>> I can add a WARN_ON() to cover this.
> >>>
> >>>>>>> +
> >>>>>>> +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> >>>>>>> +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
> >>>>>>
> >>>>>> 0x%02x would be one character shorter ;-) Same below.
> >>>>>
> >>>>> It would be, but I prefer the above notation as it's more generic.
> >>>>
> >>>> Out of curiosity, how so ?
> >>>
> >>> It works with data that would span more than 9 characters when printed.
> >>
> >> And 0x%02x doesn't ?
> > 
> > Ah, it should indeed work, 0 is actually a flag here and part of the field
> > width or precision. I can use that in v4.
> 
> Or %#04x, even shorter!

That's different though.

printf("0x%2.2x\n", 0);		-> 0x00
printf("0x%2.2x\n", 1);		-> 0x01
printf("0x%2.2x\n", 42);	-> 0x2a

printf("0x%02x\n", 0);		-> 0x00
printf("0x%02x\n", 1);		-> 0x01
printf("0x%02x\n", 42);		-> 0x2a

printf("%#2.2x\n", 0);		-> 00
printf("%#2.2x\n", 1);		-> 0x01
printf("%#2.2x\n", 42);		-> 0x2a

printf("%#02x\n", 0);		-> 00
printf("%#02x\n", 1);		-> 0x1
printf("%#02x\n", 42);		-> 0x2a

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Sakari Ailus]

On Fri, Sep 22, 2023 at 01:16:21PM +0300, Tomi Valkeinen wrote:
> On 22/09/2023 13:09, Sakari Ailus wrote:
> > On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote:
> > > On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote:
> > > > On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote:
> > > > > On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote:
> > > > > > On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote:
> > > > > > > On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
> > > > > > > > Print debug level information on returned frame descriptors.
> > > > > > > > 
> > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > > ---
> > > > > > > >   drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
> > > > > > > >   1 file changed, 31 insertions(+), 1 deletion(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > > > > index 7b087be3ff4f..504ca625b2bd 100644
> > > > > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > > > > @@ -15,6 +15,7 @@
> > > > > > > >   #include <linux/module.h>
> > > > > > > >   #include <linux/overflow.h>
> > > > > > > >   #include <linux/slab.h>
> > > > > > > > +#include <linux/string.h>
> > > > > > > >   #include <linux/types.h>
> > > > > > > >   #include <linux/version.h>
> > > > > > > >   #include <linux/videodev2.h>
> > > > > > > > @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
> > > > > > > >   static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > > > > > > >   			       struct v4l2_mbus_frame_desc *fd)
> > > > > > > >   {
> > > > > > > > +	unsigned int i;
> > > > > > > > +	int ret;
> > > > > > > > +
> > > > > > > >   	memset(fd, 0, sizeof(*fd));
> > > > > > > > -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > > > > > > +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> > > > > > > > +	if (ret)
> > > > > > > > +		return ret;
> > > > > > > > +
> > > > > > > > +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
> > > > > > > > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> > > > > > > > +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> > > > > > > > +		"unknown");
> > > > > > > > +
> > > > > > > > +	for (i = 0; i < fd->num_entries; i++) {
> > > > > > > > +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> > > > > > > > +		char buf[20] = "";
> > > > > > > 
> > > > > > > Should this be sized for the worst case ? The vc and dt should not be
> > > > > > > large, but a buffer overflow on the stack in debug code if a subdev
> > > > > > > returns an incorrect value would be bad.
> > > > > > 
> > > > > > 17 should be enough but it's not useful to use a size not divisible by 4 in
> > > > > > practice here.
> > > > > 
> > > > > 18 with the terminating 0. But indeed, it's large enough as vc and dt
> > > > 
> > > > I can count only 17 --- there's no newline.
> > > > 
> > > > I guess it's most probably either of these then. X-)
> > > > 
> > > > > are u8. I'm just a bit worried we're opening the door to hard to debug
> > > > > problems if we later change the vc and dt types.
> > > > 
> > > > I can add a WARN_ON() to cover this.
> > > > 
> > > > > > > > +
> > > > > > > > +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> > > > > > > > +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
> > > > > > > 
> > > > > > > 0x%02x would be one character shorter ;-) Same below.
> > > > > > 
> > > > > > It would be, but I prefer the above notation as it's more generic.
> > > > > 
> > > > > Out of curiosity, how so ?
> > > > 
> > > > It works with data that would span more than 9 characters when printed.
> > > 
> > > And 0x%02x doesn't ?
> > 
> > Ah, it should indeed work, 0 is actually a flag here and part of the field
> > width or precision. I can use that in v4.
> 
> Or %#04x, even shorter!

No-one has used that in the media tree so far. I'll use 0x%02x, that
appears to be common.

-- 
Sakari Ailus

Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Tomi Valkeinen]

On 22/09/2023 13:22, Laurent Pinchart wrote:
> On Fri, Sep 22, 2023 at 01:16:21PM +0300, Tomi Valkeinen wrote:
>> On 22/09/2023 13:09, Sakari Ailus wrote:
>>> On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote:
>>>> On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote:
>>>>> On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote:
>>>>>> On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote:
>>>>>>> On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote:
>>>>>>>> On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
>>>>>>>>> Print debug level information on returned frame descriptors.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>>>>>> ---
>>>>>>>>>    drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
>>>>>>>>>    1 file changed, 31 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>>>> index 7b087be3ff4f..504ca625b2bd 100644
>>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>>>>> @@ -15,6 +15,7 @@
>>>>>>>>>    #include <linux/module.h>
>>>>>>>>>    #include <linux/overflow.h>
>>>>>>>>>    #include <linux/slab.h>
>>>>>>>>> +#include <linux/string.h>
>>>>>>>>>    #include <linux/types.h>
>>>>>>>>>    #include <linux/version.h>
>>>>>>>>>    #include <linux/videodev2.h>
>>>>>>>>> @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
>>>>>>>>>    static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>>>>>>>>>    			       struct v4l2_mbus_frame_desc *fd)
>>>>>>>>>    {
>>>>>>>>> +	unsigned int i;
>>>>>>>>> +	int ret;
>>>>>>>>> +
>>>>>>>>>    	memset(fd, 0, sizeof(*fd));
>>>>>>>>>    
>>>>>>>>> -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
>>>>>>>>> +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
>>>>>>>>> +	if (ret)
>>>>>>>>> +		return ret;
>>>>>>>>> +
>>>>>>>>> +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
>>>>>>>>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
>>>>>>>>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
>>>>>>>>> +		"unknown");
>>>>>>>>> +
>>>>>>>>> +	for (i = 0; i < fd->num_entries; i++) {
>>>>>>>>> +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
>>>>>>>>> +		char buf[20] = "";
>>>>>>>>
>>>>>>>> Should this be sized for the worst case ? The vc and dt should not be
>>>>>>>> large, but a buffer overflow on the stack in debug code if a subdev
>>>>>>>> returns an incorrect value would be bad.
>>>>>>>
>>>>>>> 17 should be enough but it's not useful to use a size not divisible by 4 in
>>>>>>> practice here.
>>>>>>
>>>>>> 18 with the terminating 0. But indeed, it's large enough as vc and dt
>>>>>
>>>>> I can count only 17 --- there's no newline.
>>>>>
>>>>> I guess it's most probably either of these then. X-)
>>>>>
>>>>>> are u8. I'm just a bit worried we're opening the door to hard to debug
>>>>>> problems if we later change the vc and dt types.
>>>>>
>>>>> I can add a WARN_ON() to cover this.
>>>>>
>>>>>>>>> +
>>>>>>>>> +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
>>>>>>>>> +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
>>>>>>>>
>>>>>>>> 0x%02x would be one character shorter ;-) Same below.
>>>>>>>
>>>>>>> It would be, but I prefer the above notation as it's more generic.
>>>>>>
>>>>>> Out of curiosity, how so ?
>>>>>
>>>>> It works with data that would span more than 9 characters when printed.
>>>>
>>>> And 0x%02x doesn't ?
>>>
>>> Ah, it should indeed work, 0 is actually a flag here and part of the field
>>> width or precision. I can use that in v4.
>>
>> Or %#04x, even shorter!
> 
> That's different though.
> 
> printf("0x%2.2x\n", 0);		-> 0x00
> printf("0x%2.2x\n", 1);		-> 0x01
> printf("0x%2.2x\n", 42);	-> 0x2a
> 
> printf("0x%02x\n", 0);		-> 0x00
> printf("0x%02x\n", 1);		-> 0x01
> printf("0x%02x\n", 42);		-> 0x2a
> 
> printf("%#2.2x\n", 0);		-> 00
> printf("%#2.2x\n", 1);		-> 0x01
> printf("%#2.2x\n", 42);		-> 0x2a
> 
> printf("%#02x\n", 0);		-> 00
> printf("%#02x\n", 1);		-> 0x1
> printf("%#02x\n", 42);		-> 0x2a

The length should be 4 there, not 2. But even after fixing that, the 0 
case is printed wrong. Interesting, I haven't noticed that before. I 
wonder why it behaves that way...

0000
0x01
0x2a

  Tomi

Re: [PATCH v3 11/12] media: v4l: subdev: Print debug information on frame descriptor

[Laurent Pinchart]

On Fri, Sep 22, 2023 at 01:27:46PM +0300, Tomi Valkeinen wrote:
> On 22/09/2023 13:22, Laurent Pinchart wrote:
> > On Fri, Sep 22, 2023 at 01:16:21PM +0300, Tomi Valkeinen wrote:
> >> On 22/09/2023 13:09, Sakari Ailus wrote:
> >>> On Fri, Sep 22, 2023 at 12:53:20PM +0300, Laurent Pinchart wrote:
> >>>> On Fri, Sep 22, 2023 at 09:41:01AM +0000, Sakari Ailus wrote:
> >>>>> On Tue, Sep 19, 2023 at 06:21:23PM +0300, Laurent Pinchart wrote:
> >>>>>> On Tue, Sep 19, 2023 at 02:49:29PM +0000, Sakari Ailus wrote:
> >>>>>>> On Tue, Sep 19, 2023 at 04:32:48PM +0300, Laurent Pinchart wrote:
> >>>>>>>> On Tue, Sep 19, 2023 at 03:17:27PM +0300, Sakari Ailus wrote:
> >>>>>>>>> Print debug level information on returned frame descriptors.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>>>>>>> ---
> >>>>>>>>>    drivers/media/v4l2-core/v4l2-subdev.c | 32 ++++++++++++++++++++++++++-
> >>>>>>>>>    1 file changed, 31 insertions(+), 1 deletion(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>>>> index 7b087be3ff4f..504ca625b2bd 100644
> >>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>>>>>>> @@ -15,6 +15,7 @@
> >>>>>>>>>    #include <linux/module.h>
> >>>>>>>>>    #include <linux/overflow.h>
> >>>>>>>>>    #include <linux/slab.h>
> >>>>>>>>> +#include <linux/string.h>
> >>>>>>>>>    #include <linux/types.h>
> >>>>>>>>>    #include <linux/version.h>
> >>>>>>>>>    #include <linux/videodev2.h>
> >>>>>>>>> @@ -309,9 +310,38 @@ static int call_set_selection(struct v4l2_subdev *sd,
> >>>>>>>>>    static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> >>>>>>>>>    			       struct v4l2_mbus_frame_desc *fd)
> >>>>>>>>>    {
> >>>>>>>>> +	unsigned int i;
> >>>>>>>>> +	int ret;
> >>>>>>>>> +
> >>>>>>>>>    	memset(fd, 0, sizeof(*fd));
> >>>>>>>>>    
> >>>>>>>>> -	return sd->ops->pad->get_frame_desc(sd, pad, fd);
> >>>>>>>>> +	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> >>>>>>>>> +	if (ret)
> >>>>>>>>> +		return ret;
> >>>>>>>>> +
> >>>>>>>>> +	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
> >>>>>>>>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> >>>>>>>>> +		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> >>>>>>>>> +		"unknown");
> >>>>>>>>> +
> >>>>>>>>> +	for (i = 0; i < fd->num_entries; i++) {
> >>>>>>>>> +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> >>>>>>>>> +		char buf[20] = "";
> >>>>>>>>
> >>>>>>>> Should this be sized for the worst case ? The vc and dt should not be
> >>>>>>>> large, but a buffer overflow on the stack in debug code if a subdev
> >>>>>>>> returns an incorrect value would be bad.
> >>>>>>>
> >>>>>>> 17 should be enough but it's not useful to use a size not divisible by 4 in
> >>>>>>> practice here.
> >>>>>>
> >>>>>> 18 with the terminating 0. But indeed, it's large enough as vc and dt
> >>>>>
> >>>>> I can count only 17 --- there's no newline.
> >>>>>
> >>>>> I guess it's most probably either of these then. X-)
> >>>>>
> >>>>>> are u8. I'm just a bit worried we're opening the door to hard to debug
> >>>>>> problems if we later change the vc and dt types.
> >>>>>
> >>>>> I can add a WARN_ON() to cover this.
> >>>>>
> >>>>>>>>> +
> >>>>>>>>> +		if (fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> >>>>>>>>> +			snprintf(buf, sizeof(buf), ", vc %u, dt 0x%2.2x",
> >>>>>>>>
> >>>>>>>> 0x%02x would be one character shorter ;-) Same below.
> >>>>>>>
> >>>>>>> It would be, but I prefer the above notation as it's more generic.
> >>>>>>
> >>>>>> Out of curiosity, how so ?
> >>>>>
> >>>>> It works with data that would span more than 9 characters when printed.
> >>>>
> >>>> And 0x%02x doesn't ?
> >>>
> >>> Ah, it should indeed work, 0 is actually a flag here and part of the field
> >>> width or precision. I can use that in v4.
> >>
> >> Or %#04x, even shorter!
> > 
> > That's different though.
> > 
> > printf("0x%2.2x\n", 0);		-> 0x00
> > printf("0x%2.2x\n", 1);		-> 0x01
> > printf("0x%2.2x\n", 42);	-> 0x2a
> > 
> > printf("0x%02x\n", 0);		-> 0x00
> > printf("0x%02x\n", 1);		-> 0x01
> > printf("0x%02x\n", 42);		-> 0x2a
> > 
> > printf("%#2.2x\n", 0);		-> 00
> > printf("%#2.2x\n", 1);		-> 0x01
> > printf("%#2.2x\n", 42);		-> 0x2a
> > 
> > printf("%#02x\n", 0);		-> 00
> > printf("%#02x\n", 1);		-> 0x1
> > printf("%#02x\n", 42);		-> 0x2a
> 
> The length should be 4 there, not 2. But even after fixing that, the 0 
> case is printed wrong. Interesting, I haven't noticed that before. I 
> wonder why it behaves that way...

It's documented as such:

  #  The value should be converted to an "alternate form". [...] For x
     and X conversions, a nonzero result has the string "0x" (or "0X"
     for X conversions) prepended to it. [...]

> 0000
> 0x01
> 0x2a

-- 
Regards,

Laurent Pinchart