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

Svyatoslav Ryhel
2026-05-04T07:37:52Z

пн, 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.

> >
> > rval = v4l2_async_register_subdev(&flash->subdev_led[led_no]);
> > if (rval < 0) {
>
> --
> Kind regards,
>
> Sakari Ailus