Switch to using the sub-device state lock and properly call
v4l2_subdev_init_finalize() / v4l2_subdev_cleanup() on probe() /
remove().
Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
---
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 fd69f1616794..f0ca8cc14794 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -91,7 +91,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;
@@ -652,7 +651,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;
@@ -807,15 +806,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) {
@@ -831,14 +827,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;
}
@@ -886,7 +878,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,
@@ -898,7 +889,6 @@ static int ov5647_get_pad_fmt(struct v4l2_subdev *sd,
}
*fmt = *sensor_format;
- mutex_unlock(&sensor->lock);
return 0;
}
@@ -916,7 +906,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 {
@@ -945,7 +934,6 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
exposure_def);
}
*fmt = mode->format;
- mutex_unlock(&sensor->lock);
return 0;
}
@@ -958,10 +946,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;
}
@@ -1114,9 +1100,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;
@@ -1316,13 +1299,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);
@@ -1350,9 +1331,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);
@@ -1363,14 +1351,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;
}
@@ -1381,11 +1369,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 = {
--
2.43.0Hi Xiaolei,
On Mon, Dec 29, 2025 at 10:30:17AM +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().
>
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> ---
> 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 fd69f1616794..f0ca8cc14794 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -91,7 +91,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;
> @@ -652,7 +651,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;
>
> @@ -807,15 +806,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);
Note that you shouldn't remove mutex_lock() here quite yet -- s_stream()
callback won't involve sub-device state and thus the caller won't take the
state lock either. In other words, the end result is fine after the third
patch so you should explicitly lock the active state and remove that in the
third patch (see e.g. v4l2_subdev_lock_and_get_active_state() in
drivers/media/i2c/imx290.c).
> -
> 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) {
> @@ -831,14 +827,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;
> }
--
Kind regards,
Sakari Ailus
On 12/30/25 02:55, Sakari Ailus 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,
>
> On Mon, Dec 29, 2025 at 10:30:17AM +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().
>>
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>> ---
>> 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 fd69f1616794..f0ca8cc14794 100644
>> --- a/drivers/media/i2c/ov5647.c
>> +++ b/drivers/media/i2c/ov5647.c
>> @@ -91,7 +91,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;
>> @@ -652,7 +651,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;
>>
>> @@ -807,15 +806,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);
> Note that you shouldn't remove mutex_lock() here quite yet -- s_stream()
> callback won't involve sub-device state and thus the caller won't take the
> state lock either. In other words, the end result is fine after the third
> patch so you should explicitly lock the active state and remove that in the
> third patch (see e.g. v4l2_subdev_lock_and_get_active_state() in
> drivers/media/i2c/imx290.c).
Hi Hans,
Thank you for the detailed review and suggestions.
You're absolutely right about the approach. Using regmap_multi_reg_write()
with struct reg_sequence would indeed be cleaner and result in a much
smaller, more reviewable diff.
I'll revise the patch to:
- Use struct reg_sequence instead of struct cci_reg_sequence
- Call regmap_multi_reg_write() instead of cci_multi_reg_write()
- Keep the existing array initializer values unchanged
This will maintain the same functionality while making the conversion
more straightforward and consistent with other driver conversions.
I'll send v3 shortly.
>
>> -
>> 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) {
>> @@ -831,14 +827,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;
>> }
> --
> Kind regards,
>
> Sakari Ailus
© 2016 - 2026 Red Hat, Inc.