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

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

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