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

Sakari Ailus
2026-05-04T07:56:38Z

Hi Svyatoslav,

On Mon, May 04, 2026 at 10:37:40AM +0300, Svyatoslav Ryhel wrote:
> пн, 4 трав. 2026 р. о 09:26 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.
> >
>
> Control handler mutexes are per device, but both devices share the
> same hardware so those mutexes will not prevent simultaneous access
> from both devices. For this reason driver's own mutex is used.

Right. You could still use one for the other handler.

Feel free to keep it as-is, too.

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

--
Sakari Ailus