Re: [PATCH 2/2] media: i2c: ov5647: Switch to using the sub-device state lock
From: xiaolei wang
Date: Sun Dec 28 2025 - 01:28:37 EST
Hi Laurent,
On 12/27/25 19:21, Laurent Pinchart wrote:
CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.
Hi Xiaolei,
Thank you for the patch.
On Fri, Dec 26, 2025 at 11:13:11AM +0800, Xiaolei Wang wrote:
Switch to using the sub-device state lock and properly call
v4l2_subdev_init_finalize() / v4l2_subdev_cleanup() on probe() /
remove().
We try to convert image sensor drivers to the .enable_streams() and
.disable_streams() operations at the same time as the conversion to
v4l2_subdev_init_finalize(), to standardize on recent APIs. This hasn't
been entirely successful so far (there are 40 drivers using
v4l2_subdev_init_finalize() and only 22 of those use the new
operations), but let's avoid making it worse :-)
Could you please include a patch in v2 to move to the new
.enable_streams() and .disable_streams() operations ?
Thank you very much for reviewing my patch and for the valuable feedback.
You're absolutely right about the importance of standardizing on the recent
APIs. I completely understand your concern about the inconsistency between
drivers using v4l2_subdev_init_finalize() and those using the new stream
operations, and I appreciate you pointing this out.
I apologize for not considering the .enable_streams() and .disable_streams()
operations in my initial patch. I will add a patch in version V2 to
enable the
new .enable_streams() and .disable_streams() operations.
Best regards,
Xiaolei
Signed-off-by: Xiaolei Wang <xiaolei.wang@xxxxxxxxxxxxx>
---
drivers/media/i2c/ov5647.c | 40 +++++++++++++-------------------------
1 file changed, 14 insertions(+), 26 deletions(-)
diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index 1f8e173417b8..2619971353fd 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -96,7 +96,6 @@ struct ov5647 {
struct v4l2_subdev sd;
struct regmap *regmap;
struct media_pad pad;
- struct mutex lock;
struct clk *xclk;
struct gpio_desc *pwdn;
bool clock_ncont;
@@ -657,7 +656,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
}
/* Apply customized values from user when stream starts. */
- ret = __v4l2_ctrl_handler_setup(sd->ctrl_handler);
+ ret = v4l2_ctrl_handler_setup(sd->ctrl_handler);
if (ret)
return ret;
@@ -821,15 +820,12 @@ __ov5647_get_pad_crop(struct ov5647 *ov5647,
static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
- struct ov5647 *sensor = to_sensor(sd);
int ret;
- mutex_lock(&sensor->lock);
-
if (enable) {
ret = pm_runtime_resume_and_get(&client->dev);
if (ret < 0)
- goto error_unlock;
+ return ret;
ret = ov5647_stream_on(sd);
if (ret < 0) {
@@ -845,14 +841,10 @@ static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
pm_runtime_put(&client->dev);
}
- mutex_unlock(&sensor->lock);
-
return 0;
error_pm:
pm_runtime_put(&client->dev);
-error_unlock:
- mutex_unlock(&sensor->lock);
return ret;
}
@@ -900,7 +892,6 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
const struct v4l2_mbus_framefmt *sensor_format;
struct ov5647 *sensor = to_sensor(sd);
- mutex_lock(&sensor->lock);
switch (format->which) {
case V4L2_SUBDEV_FORMAT_TRY:
sensor_format = v4l2_subdev_state_get_format(sd_state,
@@ -912,7 +903,6 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
}
*fmt = *sensor_format;
- mutex_unlock(&sensor->lock);
return 0;
}
@@ -930,7 +920,6 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
fmt->width, fmt->height);
/* Update the sensor mode and apply at it at streamon time. */
- mutex_lock(&sensor->lock);
if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
*v4l2_subdev_state_get_format(sd_state, format->pad) = mode->format;
} else {
@@ -959,7 +948,6 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
exposure_def);
}
*fmt = mode->format;
- mutex_unlock(&sensor->lock);
return 0;
}
@@ -972,10 +960,8 @@ static int ov5647_get_selection(struct v4l2_subdev *sd,
case V4L2_SEL_TGT_CROP: {
struct ov5647 *sensor = to_sensor(sd);
- mutex_lock(&sensor->lock);
sel->r = *__ov5647_get_pad_crop(sensor, sd_state, sel->pad,
sel->which);
- mutex_unlock(&sensor->lock);
return 0;
}
@@ -1149,9 +1135,6 @@ static int ov5647_s_ctrl(struct v4l2_ctrl *ctrl)
struct i2c_client *client = v4l2_get_subdevdata(sd);
int ret = 0;
-
- /* v4l2_ctrl_lock() locks our own mutex */
-
if (ctrl->id == V4L2_CID_VBLANK) {
int exposure_max, exposure_def;
@@ -1351,13 +1334,11 @@ static int ov5647_probe(struct i2c_client *client)
return -EINVAL;
}
- mutex_init(&sensor->lock);
-
sensor->mode = OV5647_DEFAULT_MODE;
ret = ov5647_init_controls(sensor);
if (ret)
- goto mutex_destroy;
+ return ret;
sd = &sensor->sd;
v4l2_i2c_subdev_init(sd, client, &ov5647_subdev_ops);
@@ -1385,9 +1366,16 @@ static int ov5647_probe(struct i2c_client *client)
if (ret < 0)
goto power_off;
+ sd->state_lock = sensor->ctrls.lock;
+ ret = v4l2_subdev_init_finalize(sd);
+ if (ret < 0) {
+ dev_err(&client->dev, "failed to init subdev: %d", ret);
+ goto power_off;
+ }
+
ret = v4l2_async_register_subdev(sd);
if (ret < 0)
- goto power_off;
+ goto v4l2_subdev_cleanup;
/* Enable runtime PM and turn off the device */
pm_runtime_set_active(dev);
@@ -1398,14 +1386,14 @@ static int ov5647_probe(struct i2c_client *client)
return 0;
+v4l2_subdev_cleanup:
+ v4l2_subdev_cleanup(sd);
power_off:
ov5647_power_off(dev);
entity_cleanup:
media_entity_cleanup(&sd->entity);
ctrl_handler_free:
v4l2_ctrl_handler_free(&sensor->ctrls);
-mutex_destroy:
- mutex_destroy(&sensor->lock);
return ret;
}
@@ -1416,11 +1404,11 @@ static void ov5647_remove(struct i2c_client *client)
struct ov5647 *sensor = to_sensor(sd);
v4l2_async_unregister_subdev(&sensor->sd);
+ v4l2_subdev_cleanup(sd);
media_entity_cleanup(&sensor->sd.entity);
v4l2_ctrl_handler_free(&sensor->ctrls);
v4l2_device_unregister_subdev(sd);
pm_runtime_disable(&client->dev);
- mutex_destroy(&sensor->lock);
}
static const struct dev_pm_ops ov5647_pm_ops = {
--
Regards,
Laurent Pinchart