[PATCH 0/7] Small fixes and cleanups (ov2740 and ccs)

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

Sakari Ailus (7):
  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: 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

 .../userspace-api/media/v4l/dev-subdev.rst    |  49 +++----
 drivers/media/i2c/ccs/ccs-core.c              |  15 +-
 drivers/media/i2c/ccs/ccs-quirk.h             |   4 +-
 drivers/media/i2c/ov2740.c                    | 137 ++++++++----------
 4 files changed, 94 insertions(+), 111 deletions(-)

-- 
2.39.2

[PATCH 1/7] 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>
---
 .../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 2/7] 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>
---
 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 3/7] 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>
---
 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 4/7] 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>
---
 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 5/7] media: ov2740: Enable runtime PM before registering the async subdev

[Sakari Ailus]

Enable runtime PM before registering the async sub-device as the ipu
bridge may try to resume the device immediately after the async sub-device
has been registered. If runtime PM is still disabled, this will fail.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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 6/7] 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>
---
 drivers/media/i2c/ov2740.c | 121 +++++++++++++++----------------------
 1 file changed, 49 insertions(+), 72 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 319dc00e47b4..de39a66b1b81 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,10 @@ 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);
+	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
 
 	if (nvm->nvm_buffer) {
 		memcpy(val, nvm->nvm_buffer + off, count);
@@ -1082,7 +1054,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 +1125,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 +1132,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 +1143,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 +1156,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 +1165,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 +1175,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 7/7] 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>
---
 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 de39a66b1b81..a132e8a283b4 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

Re: [PATCH 1/7] media: Documentation: Align numbered list, make it a proper ReST

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Fri, Sep 15, 2023 at 10:28:03AM +0300, Sakari Ailus wrote:
> 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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/7] media: ccs: Fix driver quirk struct documentation

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Fri, Sep 15, 2023 at 10:28:04AM +0300, Sakari Ailus wrote:
> 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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/7] media: ccs: Correctly initialise try compose rectangle

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Fri, Sep 15, 2023 at 10:28:05AM +0300, Sakari Ailus wrote:
> 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>

But you should instead use .init_cfg() and the subdev active state API.
That can be done in a separate patch series.

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/7] media: ccs: Correct error handling in ccs_register_subdev

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Fri, Sep 15, 2023 at 10:28:06AM +0300, Sakari Ailus wrote:
> 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)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/7] media: ov2740: Enable runtime PM before registering the async subdev

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Fri, Sep 15, 2023 at 10:28:07AM +0300, Sakari Ailus wrote:
> Enable runtime PM before registering the async sub-device as the ipu
> bridge may try to resume the device immediately after the async sub-device

I wouldn't mention ipu bridge there, as this driver is not specific to a
particular CSI-2 receiver.

> has been registered. If runtime PM is still disabled, this will fail.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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);

I wonder why we need this in drivers. If ACPI has powered the device on
prior to calling probe(), couldn't it also set the PM runtime state
accordingly ?

> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_idle(&client->dev);
> +

With the commit message fixed,

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

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/7] media: ccs: Correctly initialise try compose rectangle

[Sakari Ailus]

Hi Laurent,

On Fri, Sep 15, 2023 at 12:19:16PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Fri, Sep 15, 2023 at 10:28:05AM +0300, Sakari Ailus wrote:
> > 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>
> 
> But you should instead use .init_cfg() and the subdev active state API.
> That can be done in a separate patch series.

Thank you for the review.

I have that patch still in the metadata series currently. If I can test it
in a timely manner for this set, I'll include it, too.

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

-- 
Regards,

Sakari Ailus

Re: [PATCH 6/7] media: ov2740: Use sub-device active state

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Fri, Sep 15, 2023 at 10:28:08AM +0300, Sakari Ailus wrote:
> 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>
> ---
>  drivers/media/i2c/ov2740.c | 121 +++++++++++++++----------------------
>  1 file changed, 49 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index 319dc00e47b4..de39a66b1b81 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;
>  }

This conflicts with a series I've just sent. As my series contains 57
patches, I would appreciate not to have to rebase it :-) You could pick
up the ov2740 patches and include them in this series, before this one.

> @@ -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,10 @@ 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);
> +	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);

This function seems unrelated to the state. What was the lock protecting
against ?

>  
>  	if (nvm->nvm_buffer) {
>  		memcpy(val, nvm->nvm_buffer + off, count);
> @@ -1082,7 +1054,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 +1125,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 +1132,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 +1143,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 +1156,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 +1165,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 +1175,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;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] media: ov2740: Return -EPROBE_DEFER if no endpoint is found

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Fri, Sep 15, 2023 at 10:28:09AM +0300, Sakari Ailus wrote:
> With ipu bridge, endpoints may only be created when ipu bridge has
> initialised. This may happen after the sensor driver has first probed.

That's hard to understand for someone not familiar with the ipu-bridge
driver. Could you please expand the commit message ?

Also, is there a way to avoid the ov2740 probing before the required
initialization is complete ?

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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 de39a66b1b81..a132e8a283b4 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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/7] media: ov2740: Use sub-device active state

[Sakari Ailus]

Hi Laurent,

On Fri, Sep 15, 2023 at 12:48:50PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Fri, Sep 15, 2023 at 10:28:08AM +0300, Sakari Ailus wrote:
> > 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>
> > ---
> >  drivers/media/i2c/ov2740.c | 121 +++++++++++++++----------------------
> >  1 file changed, 49 insertions(+), 72 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > index 319dc00e47b4..de39a66b1b81 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;
> >  }
> 
> This conflicts with a series I've just sent. As my series contains 57
> patches, I would appreciate not to have to rebase it :-) You could pick
> up the ov2740 patches and include them in this series, before this one.
> 
> > @@ -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,10 @@ 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);
> > +	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
> 
> This function seems unrelated to the state. What was the lock protecting
> against ?

It is. I guess I could acquire the lock directly from the control handler
but I think this is cleaner.

Acquiring the lock is needed to serialise access to the sensor by other
users.

> 
> >  
> >  	if (nvm->nvm_buffer) {
> >  		memcpy(val, nvm->nvm_buffer + off, count);
> > @@ -1082,7 +1054,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 +1125,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 +1132,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 +1143,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 +1156,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 +1165,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 +1175,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;
> >  }
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH 5/7] media: ov2740: Enable runtime PM before registering the async subdev

[Sakari Ailus]

Hi Laurent,

On Fri, Sep 15, 2023 at 12:42:39PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Fri, Sep 15, 2023 at 10:28:07AM +0300, Sakari Ailus wrote:
> > Enable runtime PM before registering the async sub-device as the ipu
> > bridge may try to resume the device immediately after the async sub-device
> 
> I wouldn't mention ipu bridge there, as this driver is not specific to a
> particular CSI-2 receiver.
> 
> > has been registered. If runtime PM is still disabled, this will fail.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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);
> 
> I wonder why we need this in drivers. If ACPI has powered the device on
> prior to calling probe(), couldn't it also set the PM runtime state
> accordingly ?

What happens here is that the ipu bridge creates a VCM device and it
resumes the related sensor before instantiating that device (see
ipu_bridge_instantiate_vcm_work()). However this may take place already
right after registering the async sub-device. Resuming the sensor will fail
if runtime PM isn't enabled.

I'll add something along these lines to the commit message.

> 
> > +	pm_runtime_enable(&client->dev);
> > +	pm_runtime_idle(&client->dev);
> > +
> 
> With the commit message fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you.

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

-- 
Regards,

Sakari Ailus

Re: [PATCH 6/7] media: ov2740: Use sub-device active state

[Laurent Pinchart]

Hi Sakari,

On Fri, Sep 15, 2023 at 09:54:12AM +0000, Sakari Ailus wrote:
> On Fri, Sep 15, 2023 at 12:48:50PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 15, 2023 at 10:28:08AM +0300, Sakari Ailus wrote:
> > > 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>
> > > ---
> > >  drivers/media/i2c/ov2740.c | 121 +++++++++++++++----------------------
> > >  1 file changed, 49 insertions(+), 72 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > > index 319dc00e47b4..de39a66b1b81 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;
> > >  }
> > 
> > This conflicts with a series I've just sent. As my series contains 57
> > patches, I would appreciate not to have to rebase it :-) You could pick
> > up the ov2740 patches and include them in this series, before this one.
> > 
> > > @@ -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,10 @@ 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);
> > > +	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
> > 
> > This function seems unrelated to the state. What was the lock protecting
> > against ?
> 
> It is. I guess I could acquire the lock directly from the control handler
> but I think this is cleaner.
> 
> Acquiring the lock is needed to serialise access to the sensor by other
> users.

Why so ? What data does this lock protect ?

> > >  
> > >  	if (nvm->nvm_buffer) {
> > >  		memcpy(val, nvm->nvm_buffer + off, count);
> > > @@ -1082,7 +1054,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 +1125,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 +1132,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 +1143,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 +1156,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 +1165,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 +1175,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;
> > >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/7] media: ov2740: Use sub-device active state

[Sakari Ailus]

Hi Laurent,

On Fri, Sep 15, 2023 at 01:13:05PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Fri, Sep 15, 2023 at 09:54:12AM +0000, Sakari Ailus wrote:
> > On Fri, Sep 15, 2023 at 12:48:50PM +0300, Laurent Pinchart wrote:
> > > On Fri, Sep 15, 2023 at 10:28:08AM +0300, Sakari Ailus wrote:
> > > > 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>
> > > > ---
> > > >  drivers/media/i2c/ov2740.c | 121 +++++++++++++++----------------------
> > > >  1 file changed, 49 insertions(+), 72 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > > > index 319dc00e47b4..de39a66b1b81 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;
> > > >  }
> > > 
> > > This conflicts with a series I've just sent. As my series contains 57
> > > patches, I would appreciate not to have to rebase it :-) You could pick
> > > up the ov2740 patches and include them in this series, before this one.

I can do that, yes.

> > > 
> > > > @@ -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,10 @@ 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);
> > > > +	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
> > > 
> > > This function seems unrelated to the state. What was the lock protecting
> > > against ?
> > 
> > It is. I guess I could acquire the lock directly from the control handler
> > but I think this is cleaner.
> > 
> > Acquiring the lock is needed to serialise access to the sensor by other
> > users.
> 
> Why so ? What data does this lock protect ?

The sensor has internal state, in this case it's streaming state in
particular.

> 
> > > >  
> > > >  	if (nvm->nvm_buffer) {
> > > >  		memcpy(val, nvm->nvm_buffer + off, count);
> > > > @@ -1082,7 +1054,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 +1125,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 +1132,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 +1143,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 +1156,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 +1165,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 +1175,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;
> > > >  }

-- 
Regards,

Sakari Ailus

Re: [PATCH 5/7] media: ov2740: Enable runtime PM before registering the async subdev

[Laurent Pinchart]

On Fri, Sep 15, 2023 at 10:09:37AM +0000, Sakari Ailus wrote:
> On Fri, Sep 15, 2023 at 12:42:39PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 15, 2023 at 10:28:07AM +0300, Sakari Ailus wrote:
> > > Enable runtime PM before registering the async sub-device as the ipu
> > > bridge may try to resume the device immediately after the async sub-device
> > 
> > I wouldn't mention ipu bridge there, as this driver is not specific to a
> > particular CSI-2 receiver.
> > 
> > > has been registered. If runtime PM is still disabled, this will fail.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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);
> > 
> > I wonder why we need this in drivers. If ACPI has powered the device on
> > prior to calling probe(), couldn't it also set the PM runtime state
> > accordingly ?
> 
> What happens here is that the ipu bridge creates a VCM device and it
> resumes the related sensor before instantiating that device (see
> ipu_bridge_instantiate_vcm_work()). However this may take place already
> right after registering the async sub-device. Resuming the sensor will fail
> if runtime PM isn't enabled.
> 
> I'll add something along these lines to the commit message.

I understand this, but that doesn't answer my question :-) Why is there
a need to call pm_runtime_set_active(), couldn't it be done somewhere in
common code ?

> > > +	pm_runtime_enable(&client->dev);
> > > +	pm_runtime_idle(&client->dev);
> > > +
> > 
> > With the commit message fixed,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thank you.
> 
> > >  	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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/7] media: ov2740: Use sub-device active state

[Laurent Pinchart]

On Fri, Sep 15, 2023 at 10:27:09AM +0000, Sakari Ailus wrote:
> On Fri, Sep 15, 2023 at 01:13:05PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 15, 2023 at 09:54:12AM +0000, Sakari Ailus wrote:
> > > On Fri, Sep 15, 2023 at 12:48:50PM +0300, Laurent Pinchart wrote:
> > > > On Fri, Sep 15, 2023 at 10:28:08AM +0300, Sakari Ailus wrote:
> > > > > 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>
> > > > > ---
> > > > >  drivers/media/i2c/ov2740.c | 121 +++++++++++++++----------------------
> > > > >  1 file changed, 49 insertions(+), 72 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > > > > index 319dc00e47b4..de39a66b1b81 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;
> > > > >  }
> > > > 
> > > > This conflicts with a series I've just sent. As my series contains 57
> > > > patches, I would appreciate not to have to rebase it :-) You could pick
> > > > up the ov2740 patches and include them in this series, before this one.
> 
> I can do that, yes.
> 
> > > > 
> > > > > @@ -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,10 @@ 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);
> > > > > +	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
> > > > 
> > > > This function seems unrelated to the state. What was the lock protecting
> > > > against ?
> > > 
> > > It is. I guess I could acquire the lock directly from the control handler
> > > but I think this is cleaner.
> > > 
> > > Acquiring the lock is needed to serialise access to the sensor by other
> > > users.
> > 
> > Why so ? What data does this lock protect ?
> 
> The sensor has internal state, in this case it's streaming state in
> particular.

Is the issue that you can't read the NVMEM while the sensor is streaming
? If so the lock won't help.

> > > > >  
> > > > >  	if (nvm->nvm_buffer) {
> > > > >  		memcpy(val, nvm->nvm_buffer + off, count);
> > > > > @@ -1082,7 +1054,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 +1125,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 +1132,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 +1143,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 +1156,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 +1165,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 +1175,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;
> > > > >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/7] media: ov2740: Enable runtime PM before registering the async subdev

[Sakari Ailus]

Hi Laurent,

On Fri, Sep 15, 2023 at 02:30:12PM +0300, Laurent Pinchart wrote:
> On Fri, Sep 15, 2023 at 10:09:37AM +0000, Sakari Ailus wrote:
> > On Fri, Sep 15, 2023 at 12:42:39PM +0300, Laurent Pinchart wrote:
> > > On Fri, Sep 15, 2023 at 10:28:07AM +0300, Sakari Ailus wrote:
> > > > Enable runtime PM before registering the async sub-device as the ipu
> > > > bridge may try to resume the device immediately after the async sub-device
> > > 
> > > I wouldn't mention ipu bridge there, as this driver is not specific to a
> > > particular CSI-2 receiver.
> > > 
> > > > has been registered. If runtime PM is still disabled, this will fail.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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);
> > > 
> > > I wonder why we need this in drivers. If ACPI has powered the device on
> > > prior to calling probe(), couldn't it also set the PM runtime state
> > > accordingly ?
> > 
> > What happens here is that the ipu bridge creates a VCM device and it
> > resumes the related sensor before instantiating that device (see
> > ipu_bridge_instantiate_vcm_work()). However this may take place already
> > right after registering the async sub-device. Resuming the sensor will fail
> > if runtime PM isn't enabled.
> > 
> > I'll add something along these lines to the commit message.
> 
> I understand this, but that doesn't answer my question :-) Why is there
> a need to call pm_runtime_set_active(), couldn't it be done somewhere in
> common code ?

Ah, I think I misunderstood your earlier question.

The sensor may be left powered off for probe if the device's _DSC object
evaluates to 4. See Documentation/firmware-guide/acpi/non-d0-probe.rst for
more information.

> 
> > > > +	pm_runtime_enable(&client->dev);
> > > > +	pm_runtime_idle(&client->dev);
> > > > +
> > > 
> > > With the commit message fixed,
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Thank you.
> > 
> > > >  	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);

-- 
Regards,

Sakari Ailus

Re: [PATCH 6/7] media: ov2740: Use sub-device active state

[Sakari Ailus]

On Fri, Sep 15, 2023 at 02:31:06PM +0300, Laurent Pinchart wrote:
> On Fri, Sep 15, 2023 at 10:27:09AM +0000, Sakari Ailus wrote:
> > On Fri, Sep 15, 2023 at 01:13:05PM +0300, Laurent Pinchart wrote:
> > > On Fri, Sep 15, 2023 at 09:54:12AM +0000, Sakari Ailus wrote:
> > > > On Fri, Sep 15, 2023 at 12:48:50PM +0300, Laurent Pinchart wrote:
> > > > > On Fri, Sep 15, 2023 at 10:28:08AM +0300, Sakari Ailus wrote:
> > > > > > 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>
> > > > > > ---
> > > > > >  drivers/media/i2c/ov2740.c | 121 +++++++++++++++----------------------
> > > > > >  1 file changed, 49 insertions(+), 72 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > > > > > index 319dc00e47b4..de39a66b1b81 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;
> > > > > >  }
> > > > > 
> > > > > This conflicts with a series I've just sent. As my series contains 57
> > > > > patches, I would appreciate not to have to rebase it :-) You could pick
> > > > > up the ov2740 patches and include them in this series, before this one.
> > 
> > I can do that, yes.
> > 
> > > > > 
> > > > > > @@ -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,10 @@ 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);
> > > > > > +	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
> > > > > 
> > > > > This function seems unrelated to the state. What was the lock protecting
> > > > > against ?
> > > > 
> > > > It is. I guess I could acquire the lock directly from the control handler
> > > > but I think this is cleaner.
> > > > 
> > > > Acquiring the lock is needed to serialise access to the sensor by other
> > > > users.
> > > 
> > > Why so ? What data does this lock protect ?
> > 
> > The sensor has internal state, in this case it's streaming state in
> > particular.
> 
> Is the issue that you can't read the NVMEM while the sensor is streaming
> ? If so the lock won't help.

Actually not quite. The NVM is apparently read before streaming is started
the first time.

So this only prevents accessing controls while the NVM is read.

> 
> > > > > >  
> > > > > >  	if (nvm->nvm_buffer) {
> > > > > >  		memcpy(val, nvm->nvm_buffer + off, count);
> > > > > > @@ -1082,7 +1054,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 +1125,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 +1132,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 +1143,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 +1156,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 +1165,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 +1175,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;
> > > > > >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Sakari Ailus

Re: [PATCH 6/7] media: ov2740: Use sub-device active state

[Laurent Pinchart]

On Fri, Sep 15, 2023 at 11:53:06AM +0000, Sakari Ailus wrote:
> On Fri, Sep 15, 2023 at 02:31:06PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 15, 2023 at 10:27:09AM +0000, Sakari Ailus wrote:
> > > On Fri, Sep 15, 2023 at 01:13:05PM +0300, Laurent Pinchart wrote:
> > > > On Fri, Sep 15, 2023 at 09:54:12AM +0000, Sakari Ailus wrote:
> > > > > On Fri, Sep 15, 2023 at 12:48:50PM +0300, Laurent Pinchart wrote:
> > > > > > On Fri, Sep 15, 2023 at 10:28:08AM +0300, Sakari Ailus wrote:
> > > > > > > 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>
> > > > > > > ---
> > > > > > >  drivers/media/i2c/ov2740.c | 121 +++++++++++++++----------------------
> > > > > > >  1 file changed, 49 insertions(+), 72 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > > > > > > index 319dc00e47b4..de39a66b1b81 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;
> > > > > > >  }
> > > > > > 
> > > > > > This conflicts with a series I've just sent. As my series contains 57
> > > > > > patches, I would appreciate not to have to rebase it :-) You could pick
> > > > > > up the ov2740 patches and include them in this series, before this one.
> > > 
> > > I can do that, yes.
> > > 
> > > > > > 
> > > > > > > @@ -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,10 @@ 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);
> > > > > > > +	sd_state = v4l2_subdev_lock_and_get_active_state(&ov2740->sd);
> > > > > > 
> > > > > > This function seems unrelated to the state. What was the lock protecting
> > > > > > against ?
> > > > > 
> > > > > It is. I guess I could acquire the lock directly from the control handler
> > > > > but I think this is cleaner.
> > > > > 
> > > > > Acquiring the lock is needed to serialise access to the sensor by other
> > > > > users.
> > > > 
> > > > Why so ? What data does this lock protect ?
> > > 
> > > The sensor has internal state, in this case it's streaming state in
> > > particular.
> > 
> > Is the issue that you can't read the NVMEM while the sensor is streaming
> > ? If so the lock won't help.
> 
> Actually not quite. The NVM is apparently read before streaming is started
> the first time.
> 
> So this only prevents accessing controls while the NVM is read.

A comment to explain that would be nice. With it,

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

> > > > > > >  
> > > > > > >  	if (nvm->nvm_buffer) {
> > > > > > >  		memcpy(val, nvm->nvm_buffer + off, count);
> > > > > > > @@ -1082,7 +1054,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 +1125,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 +1132,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 +1143,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 +1156,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 +1165,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 +1175,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;
> > > > > > >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/7] media: ov2740: Enable runtime PM before registering the async subdev

[Laurent Pinchart]

On Fri, Sep 15, 2023 at 11:48:43AM +0000, Sakari Ailus wrote:
> On Fri, Sep 15, 2023 at 02:30:12PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 15, 2023 at 10:09:37AM +0000, Sakari Ailus wrote:
> > > On Fri, Sep 15, 2023 at 12:42:39PM +0300, Laurent Pinchart wrote:
> > > > On Fri, Sep 15, 2023 at 10:28:07AM +0300, Sakari Ailus wrote:
> > > > > Enable runtime PM before registering the async sub-device as the ipu
> > > > > bridge may try to resume the device immediately after the async sub-device
> > > > 
> > > > I wouldn't mention ipu bridge there, as this driver is not specific to a
> > > > particular CSI-2 receiver.
> > > > 
> > > > > has been registered. If runtime PM is still disabled, this will fail.
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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);
> > > > 
> > > > I wonder why we need this in drivers. If ACPI has powered the device on
> > > > prior to calling probe(), couldn't it also set the PM runtime state
> > > > accordingly ?
> > > 
> > > What happens here is that the ipu bridge creates a VCM device and it
> > > resumes the related sensor before instantiating that device (see
> > > ipu_bridge_instantiate_vcm_work()). However this may take place already
> > > right after registering the async sub-device. Resuming the sensor will fail
> > > if runtime PM isn't enabled.
> > > 
> > > I'll add something along these lines to the commit message.
> > 
> > I understand this, but that doesn't answer my question :-) Why is there
> > a need to call pm_runtime_set_active(), couldn't it be done somewhere in
> > common code ?
> 
> Ah, I think I misunderstood your earlier question.
> 
> The sensor may be left powered off for probe if the device's _DSC object
> evaluates to 4. See Documentation/firmware-guide/acpi/non-d0-probe.rst for
> more information.

That information should be available to core code, given that it's
retrieved in the driver with a call to acpi_dev_state_d0(). Couldn't it
thus be handled in core code ?

> > > > > +	pm_runtime_enable(&client->dev);
> > > > > +	pm_runtime_idle(&client->dev);
> > > > > +
> > > > 
> > > > With the commit message fixed,
> > > > 
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > Thank you.
> > > 
> > > > >  	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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/7] media: ov2740: Enable runtime PM before registering the async subdev

[Sakari Ailus]

On Fri, Sep 15, 2023 at 03:16:59PM +0300, Laurent Pinchart wrote:
> On Fri, Sep 15, 2023 at 11:48:43AM +0000, Sakari Ailus wrote:
> > On Fri, Sep 15, 2023 at 02:30:12PM +0300, Laurent Pinchart wrote:
> > > On Fri, Sep 15, 2023 at 10:09:37AM +0000, Sakari Ailus wrote:
> > > > On Fri, Sep 15, 2023 at 12:42:39PM +0300, Laurent Pinchart wrote:
> > > > > On Fri, Sep 15, 2023 at 10:28:07AM +0300, Sakari Ailus wrote:
> > > > > > Enable runtime PM before registering the async sub-device as the ipu
> > > > > > bridge may try to resume the device immediately after the async sub-device
> > > > > 
> > > > > I wouldn't mention ipu bridge there, as this driver is not specific to a
> > > > > particular CSI-2 receiver.
> > > > > 
> > > > > > has been registered. If runtime PM is still disabled, this will fail.
> > > > > > 
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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);
> > > > > 
> > > > > I wonder why we need this in drivers. If ACPI has powered the device on
> > > > > prior to calling probe(), couldn't it also set the PM runtime state
> > > > > accordingly ?
> > > > 
> > > > What happens here is that the ipu bridge creates a VCM device and it
> > > > resumes the related sensor before instantiating that device (see
> > > > ipu_bridge_instantiate_vcm_work()). However this may take place already
> > > > right after registering the async sub-device. Resuming the sensor will fail
> > > > if runtime PM isn't enabled.
> > > > 
> > > > I'll add something along these lines to the commit message.
> > > 
> > > I understand this, but that doesn't answer my question :-) Why is there
> > > a need to call pm_runtime_set_active(), couldn't it be done somewhere in
> > > common code ?
> > 
> > Ah, I think I misunderstood your earlier question.
> > 
> > The sensor may be left powered off for probe if the device's _DSC object
> > evaluates to 4. See Documentation/firmware-guide/acpi/non-d0-probe.rst for
> > more information.
> 
> That information should be available to core code, given that it's
> retrieved in the driver with a call to acpi_dev_state_d0(). Couldn't it
> thus be handled in core code ?

The I²C framework is responsible powering on the device before probe, not
the ACPI framework. The default runtime PM state is suspended, so I guess
this could be changed for ACPI devices in the I²C framework. But that
certainly does not belong to this patchset.

I was actually hoping the ACPI introduced I²C devices could have power
management working the same was as for DT: driver would resume the device
instead of the framework. This would simplify drivers.

> 
> > > > > > +	pm_runtime_enable(&client->dev);
> > > > > > +	pm_runtime_idle(&client->dev);
> > > > > > +
> > > > > 
> > > > > With the commit message fixed,
> > > > > 
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > 
> > > > Thank you.
> > > > 
> > > > > >  	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);

-- 
Regards,

Sakari Ailus

Re: [PATCH 5/7] media: ov2740: Enable runtime PM before registering the async subdev

[Laurent Pinchart]

On Fri, Sep 15, 2023 at 03:50:00PM +0000, Sakari Ailus wrote:
> On Fri, Sep 15, 2023 at 03:16:59PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 15, 2023 at 11:48:43AM +0000, Sakari Ailus wrote:
> > > On Fri, Sep 15, 2023 at 02:30:12PM +0300, Laurent Pinchart wrote:
> > > > On Fri, Sep 15, 2023 at 10:09:37AM +0000, Sakari Ailus wrote:
> > > > > On Fri, Sep 15, 2023 at 12:42:39PM +0300, Laurent Pinchart wrote:
> > > > > > On Fri, Sep 15, 2023 at 10:28:07AM +0300, Sakari Ailus wrote:
> > > > > > > Enable runtime PM before registering the async sub-device as the ipu
> > > > > > > bridge may try to resume the device immediately after the async sub-device
> > > > > > 
> > > > > > I wouldn't mention ipu bridge there, as this driver is not specific to a
> > > > > > particular CSI-2 receiver.
> > > > > > 
> > > > > > > has been registered. If runtime PM is still disabled, this will fail.
> > > > > > > 
> > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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);
> > > > > > 
> > > > > > I wonder why we need this in drivers. If ACPI has powered the device on
> > > > > > prior to calling probe(), couldn't it also set the PM runtime state
> > > > > > accordingly ?
> > > > > 
> > > > > What happens here is that the ipu bridge creates a VCM device and it
> > > > > resumes the related sensor before instantiating that device (see
> > > > > ipu_bridge_instantiate_vcm_work()). However this may take place already
> > > > > right after registering the async sub-device. Resuming the sensor will fail
> > > > > if runtime PM isn't enabled.
> > > > > 
> > > > > I'll add something along these lines to the commit message.
> > > > 
> > > > I understand this, but that doesn't answer my question :-) Why is there
> > > > a need to call pm_runtime_set_active(), couldn't it be done somewhere in
> > > > common code ?
> > > 
> > > Ah, I think I misunderstood your earlier question.
> > > 
> > > The sensor may be left powered off for probe if the device's _DSC object
> > > evaluates to 4. See Documentation/firmware-guide/acpi/non-d0-probe.rst for
> > > more information.
> > 
> > That information should be available to core code, given that it's
> > retrieved in the driver with a call to acpi_dev_state_d0(). Couldn't it
> > thus be handled in core code ?
> 
> The I²C framework is responsible powering on the device before probe, not
> the ACPI framework. The default runtime PM state is suspended, so I guess
> this could be changed for ACPI devices in the I²C framework. But that
> certainly does not belong to this patchset.

Of course. Sorry if I didn't make it clear that this comment was
unrelated to this patch series.

> I was actually hoping the ACPI introduced I²C devices could have power
> management working the same was as for DT: driver would resume the device
> instead of the framework. This would simplify drivers.

You're the ACPI specialist (I'm sorry about that :-)), whatever you
think is best is fine with me, as long as we simplify drivers. Power
management in camera sensor drivers is way too complicated to implement,
with too much boilerplate code being copied (often incorrectly) between
drivers.

> > > > > > > +	pm_runtime_enable(&client->dev);
> > > > > > > +	pm_runtime_idle(&client->dev);
> > > > > > > +
> > > > > > 
> > > > > > With the commit message fixed,
> > > > > > 
> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > 
> > > > > Thank you.
> > > > > 
> > > > > > >  	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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] media: ov2740: Return -EPROBE_DEFER if no endpoint is found

[Sakari Ailus]

Hi Laurent,

On Fri, Sep 15, 2023 at 12:50:27PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Fri, Sep 15, 2023 at 10:28:09AM +0300, Sakari Ailus wrote:
> > With ipu bridge, endpoints may only be created when ipu bridge has
> > initialised. This may happen after the sensor driver has first probed.
> 
> That's hard to understand for someone not familiar with the ipu-bridge
> driver. Could you please expand the commit message ?
> 
> Also, is there a way to avoid the ov2740 probing before the required
> initialization is complete ?

As of now, nothing else than, well, not having any endpoints as they are
created by the ipu bridge.

The ACPI device node does have a couple of custom objects but I'd rather
not add checks for those in the ACPI framework itself.

The proper solution (from device driver point of view at least) would be to
squash the ipu bridge to the ACPI framework itself so all this could be
initialised before any drivers get probed. I'm not sure how that would be
received, and in any case, as I wrote in a reply to another patch in this
series, I'd like to have DisCo for Imaging support merged first.

-- 
Regards,

Sakari Ailus

Re: [PATCH 5/7] media: ov2740: Enable runtime PM before registering the async subdev

[Sakari Ailus]

On Fri, Sep 15, 2023 at 12:42:39PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Fri, Sep 15, 2023 at 10:28:07AM +0300, Sakari Ailus wrote:
> > Enable runtime PM before registering the async sub-device as the ipu
> > bridge may try to resume the device immediately after the async sub-device
> 
> I wouldn't mention ipu bridge there, as this driver is not specific to a
> particular CSI-2 receiver.

How exactly would you reword this?

The problem may only happen with the IPU bridge --- ACPI systems otherwise
handle device power states.

-- 
Sakari Ailus

Re: [PATCH 5/7] media: ov2740: Enable runtime PM before registering the async subdev

[Laurent Pinchart]

On Fri, Sep 15, 2023 at 08:31:24PM +0000, Sakari Ailus wrote:
> On Fri, Sep 15, 2023 at 12:42:39PM +0300, Laurent Pinchart wrote:
> > On Fri, Sep 15, 2023 at 10:28:07AM +0300, Sakari Ailus wrote:
> > > Enable runtime PM before registering the async sub-device as the ipu
> > > bridge may try to resume the device immediately after the async sub-device
> > 
> > I wouldn't mention ipu bridge there, as this driver is not specific to a
> > particular CSI-2 receiver.
> 
> How exactly would you reword this?
> 
> The problem may only happen with the IPU bridge --- ACPI systems otherwise
> handle device power states.

Once a subdev is registered is could be used immediately by the host
driver, regardless of what host driver that is. Making sure to finalize
all initialization before registering the subdev is a good overall
practice.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] media: ov2740: Return -EPROBE_DEFER if no endpoint is found

[Sakari Ailus]

Hi Laurent,

On Fri, Sep 15, 2023 at 12:50:27PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Fri, Sep 15, 2023 at 10:28:09AM +0300, Sakari Ailus wrote:
> > With ipu bridge, endpoints may only be created when ipu bridge has
> > initialised. This may happen after the sensor driver has first probed.
> 
> That's hard to understand for someone not familiar with the ipu-bridge
> driver. Could you please expand the commit message ?
> 
> Also, is there a way to avoid the ov2740 probing before the required
> initialization is complete ?

One possibility would be to move the ipu bridge functionality to the ACPI
framework itself. Then it'd be independent of probing any drivers. It
hasn't been discussed and I'm not sure what the result might be. In any
case I'd like to have DisCo for Imaging support there first.

Cc Hans.

> 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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 de39a66b1b81..a132e8a283b4 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);
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH 7/7] media: ov2740: Return -EPROBE_DEFER if no endpoint is found

[Hans de Goede]

Hi,

On 9/29/23 14:37, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Fri, Sep 15, 2023 at 12:50:27PM +0300, Laurent Pinchart wrote:
>> Hi Sakari,
>>
>> Thank you for the patch.
>>
>> On Fri, Sep 15, 2023 at 10:28:09AM +0300, Sakari Ailus wrote:
>>> With ipu bridge, endpoints may only be created when ipu bridge has
>>> initialised. This may happen after the sensor driver has first probed.
>>
>> That's hard to understand for someone not familiar with the ipu-bridge
>> driver. Could you please expand the commit message ?
>>
>> Also, is there a way to avoid the ov2740 probing before the required
>> initialization is complete ?
> 
> One possibility would be to move the ipu bridge functionality to the ACPI
> framework itself. Then it'd be independent of probing any drivers. It
> hasn't been discussed and I'm not sure what the result might be. In any
> case I'd like to have DisCo for Imaging support there first.

The problem is not the IPU bridge functionality per se. We already delay
sensor i2c-client instantiation on ipu3 and ipu6 till after the INT3472
driver has loaded since that does things like register gpio, clk and
regulator lookup tables to glue the ACPI approach into standard kernel
subsytems and probing before the lookups are there leads to dummy-regulators
and optional GPIOs and clks not working.

So one might argue that this bit of code should be moved to
the INT3472 code, making it delay i2c-client instantiation until
the fwnode-graph has been populated by the ipu-bridge code:

static int ov2740_check_hwcfg(struct device *dev)
{
        struct fwnode_handle *ep;
        struct fwnode_handle *fwnode = dev_fwnode(dev);

	...

	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
	if (!ep)
		return -EPROBE_DEFER;

But notice the `struct device *dev` parameter, that *is*
the i2c_client; and as soon as the INT3472 code has told
the ACPI core that it is ok to move forward with instantiating
the i2c_client by calling acpi_dev_clear_dependencies(INT3472-adev)
then it is too late to do this check inside INT3472 and before the
acpi_dev_clear_dependencies(INT3472-adev) call the i2c-client does
not exist.

So fixing this requires hacks in either the ACPI core and/or
in the i2c-core (to mark a device as initially unprobeable).

And then we are not talking about the atomisp2 yet where
the sensor ACPI device-nodes do not have a _DEP (dependency)
like INT3472 which we can use to delay instantiating
the i2c-clients.   With the INT3472 device we can use
the INT3472 hardware-id to trigger deferring instantiation
on that (by default the ACPI core does not honor _DEP for
various reasons). So delaying instantiation on atomisp2
hardware would mean maintaining a list of hardcoded sensor
hardware-ids for which we want to delay instantiation +
some other ACPI core hack to say: it is ok to move forward
with instantiation now.

Last time Sakari and I discussed this we came to
the conclusion that, e.g. (from ov2680.c):

	/*
         * Sometimes the fwnode graph is initialized by the bridge driver.
         * Bridge drivers doing this may also add GPIO mappings, wait for this.
         */
        ep_fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
        if (!ep_fwnode)
                return dev_err_probe(dev, -EPROBE_DEFER,
                                     "waiting for fwnode graph endpoint\n");

Is the least ugly way to deal with this, we have gone through
a lot of trouble to avoid adding ACPI-isms to the driver but this
one seems unavoidable.

Note though that the ov2680 code has both a comment and a
	return dev_err_probe(dev, -EPROBE_DEFER, ...)

This last one will make the kernel print messages about devices waiting
for deferred probe + the passed in reason 30 seconds after the last
successful probe() (any successful probe anywhere in the kernel
resets the 30 seconds counter).

This helps a lot to figure out why the driver is not binding if
it is not binding because of this code-path.

I believe it is the best to do the same thing as ov2680.c here too.

Regards,

Hans



> 
> Cc Hans.
> 
>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.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 de39a66b1b81..a132e8a283b4 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);
>>
>