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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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