Re: [PATCH v5 3/6] media: i2c: lm3560: Optimize mutex lock usage

Sakari Ailus
2026-05-04T06:26:47Z

Hi Svyatoslav,

On Sun, May 03, 2026 at 07:44:42PM +0300, Svyatoslav Ryhel wrote:
> Pass the device's own mutex lock to the control handler so that the media
> framework can handle control access instead of managing it manually. The
> lock must be common to both sub-devices since they share same hardware,
> so the individual sub-device locks will not work here.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
> drivers/media/i2c/lm3560.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/lm3560.c b/drivers/media/i2c/lm3560.c
> index edfb07587cab..5b568ed9536b 100644
> --- a/drivers/media/i2c/lm3560.c
> +++ b/drivers/media/i2c/lm3560.c
> @@ -162,14 +162,12 @@ static int lm3560_get_ctrl(struct v4l2_ctrl *ctrl, enum lm3560_led_id led_no)
> struct lm3560_flash *flash = to_lm3560_flash(ctrl, led_no);
> int rval = -EINVAL;
>
> - mutex_lock(&flash->lock);
> -
> if (ctrl->id == V4L2_CID_FLASH_FAULT) {
> s32 fault = 0;
> unsigned int reg_val;
> rval = regmap_read(flash->regmap, REG_FLAG, &reg_val);
> if (rval < 0)
> - goto out;
> + return rval;
> if (reg_val & FAULT_SHORT_CIRCUIT)
> fault |= V4L2_FLASH_FAULT_SHORT_CIRCUIT;
> if (reg_val & FAULT_OVERTEMP)
> @@ -179,8 +177,6 @@ static int lm3560_get_ctrl(struct v4l2_ctrl *ctrl, enum lm3560_led_id led_no)
> ctrl->cur.val = fault;
> }
>
> -out:
> - mutex_unlock(&flash->lock);
> return rval;
> }
>
> @@ -190,8 +186,6 @@ static int lm3560_set_ctrl(struct v4l2_ctrl *ctrl, enum lm3560_led_id led_no)
> u8 tout_bits;
> int rval = -EINVAL;
>
> - mutex_lock(&flash->lock);
> -
> switch (ctrl->id) {
> case V4L2_CID_FLASH_LED_MODE:
> flash->led_mode = ctrl->val;
> @@ -202,14 +196,12 @@ static int lm3560_set_ctrl(struct v4l2_ctrl *ctrl, enum lm3560_led_id led_no)
> case V4L2_CID_FLASH_STROBE_SOURCE:
> rval = regmap_update_bits(flash->regmap,
> REG_CONFIG1, 0x04, (ctrl->val) << 2);
> - if (rval < 0)
> - goto err_out;
> break;
>
> case V4L2_CID_FLASH_STROBE:
> if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) {
> rval = -EBUSY;
> - goto err_out;
> + break;
> }
> flash->led_mode = V4L2_FLASH_LED_MODE_FLASH;
> rval = lm3560_mode_ctrl(flash);
> @@ -218,7 +210,7 @@ static int lm3560_set_ctrl(struct v4l2_ctrl *ctrl, enum lm3560_led_id led_no)
> case V4L2_CID_FLASH_STROBE_STOP:
> if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) {
> rval = -EBUSY;
> - goto err_out;
> + break;
> }
> flash->led_mode = V4L2_FLASH_LED_MODE_NONE;
> rval = lm3560_mode_ctrl(flash);
> @@ -239,8 +231,6 @@ static int lm3560_set_ctrl(struct v4l2_ctrl *ctrl, enum lm3560_led_id led_no)
> break;
> }
>
> -err_out:
> - mutex_unlock(&flash->lock);
> return rval;
> }
>
> @@ -328,6 +318,8 @@ static int lm3560_init_controls(struct lm3560_flash *flash,
> if (fault != NULL)
> fault->flags |= V4L2_CTRL_FLAG_VOLATILE;
>
> + hdl->lock = &flash->lock;
> +
> if (hdl->error)
> return hdl->error;
>
> @@ -363,6 +355,7 @@ static int lm3560_subdev_init(struct lm3560_flash *flash,
> if (rval < 0)
> goto err_out;
> flash->subdev_led[led_no].entity.function = MEDIA_ENT_F_FLASH;
> + flash->subdev_led[led_no].state_lock = &flash->lock;

I must have missed it earlier but you can use the control handler's mutex
here. As a result, I believe you can drop the driver's own mutex
altogether.

>
> rval = v4l2_async_register_subdev(&flash->subdev_led[led_no]);
> if (rval < 0) {

--
Kind regards,

Sakari Ailus