Re: [PATCH 8/8] media: i2c: Add ON Semiconductor LC898123F40 lens autofocus driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] media: i2c: lc898123: unlocked __v4l2_ctrl_handler_setup() in
  lc898123_open()
- [Medium] media: i2c: lc898123: missing runtime PM and unconditional
  init on every open()
- [Low] media: i2c: unsorted CONFIG_VIDEO_LC898123 entry in
  drivers/media/i2c/Makefile
--

commit 90488b1ee2e93be2704532224dc3a72b0184545b
Author: Gaurav Suman <gauravsuman007@gmail.com>

media: i2c: Add ON Semiconductor LC898123F40 lens autofocus driver

This commit adds a V4L2 lens sub-device driver for the autofocus function
of the ON Semiconductor LC898123F40 OIS/AF controller.

> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 7e714dbcbf69d..836bf5b8a44e2 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
>  obj-$(CONFIG_VIDEO_DW9719) += dw9719.o
>  obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
>  obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
> +obj-$(CONFIG_VIDEO_LC898123) += lc898123.o
>  obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
>  obj-$(CONFIG_VIDEO_GC0308) += gc0308.o
>  obj-$(CONFIG_VIDEO_GC0310) += gc0310.o

[Severity: Low]
This isn't a bug, but entries in this Makefile are sorted alphabetically by
CONFIG symbol, so CONFIG_VIDEO_LC898123 belongs between CONFIG_VIDEO_KS0127
and CONFIG_VIDEO_LM3560.

> diff --git a/drivers/media/i2c/lc898123.c b/drivers/media/i2c/lc898123.c
> new file mode 100644
> index 0000000000000..69516ad964d60
> --- /dev/null
> +++ b/drivers/media/i2c/lc898123.c

[ ... ]

> +static int lc898123_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	struct lc898123 *lc = container_of(sd, struct lc898123, sd);
> +	int ret = 0;
> +
> +	cci_write(lc->regmap, LC898123_REG_OIS_CTRL, 0, &ret);
> +	cci_write(lc->regmap, LC898123_REG_SERVO, 1, &ret);
> +	if (!ret)
> +		ret = __v4l2_ctrl_handler_setup(&lc->ctrls);
> +	return ret;
> +}

[Severity: Medium]
Should lc898123_open() call v4l2_ctrl_handler_setup(&lc->ctrls) instead of
__v4l2_ctrl_handler_setup(&lc->ctrls)?

subdev_open() invokes sd->internal_ops->open() without holding
lc->ctrls.lock, so calling the unlocked helper triggers
lockdep_assert_held(hdl->lock) on every open of the subdev node and races
with concurrent V4L2 control ioctls or open() calls when updating control
state and writing LC898123_REG_AF_POS.

> +
> +static const struct v4l2_subdev_internal_ops lc898123_internal_ops = {
> +	.open = lc898123_open,
> +};
> +
> +static const struct v4l2_subdev_ops lc898123_ops = { };
> +
> +static int lc898123_probe(struct i2c_client *client)
> +{
> +	struct lc898123 *lc;
> +	u64 val;
> +	int ret;
> +
> +	lc = devm_kzalloc(&client->dev, sizeof(*lc), GFP_KERNEL);
> +	if (!lc)
> +		return -ENOMEM;
> +	ret = devm_regulator_get_enable_optional(&client->dev, "vdd");
> +	if (ret && ret != -ENODEV)
> +		return dev_err_probe(&client->dev, ret, "vdd supply\n");
> +	if (!ret)
> +		usleep_range(10000, 12000);	/* controller boot from flash */
> +
> +	lc->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(lc->regmap))
> +		return PTR_ERR(lc->regmap);
> +	ret = cci_read(lc->regmap, LC898123_REG_AF_POS, &val, NULL);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "no response\n");
> +	cci_write(lc->regmap, LC898123_REG_SERVO, 1, NULL);

[Severity: Medium]
Does enabling the vdd supply and LC898123_REG_SERVO here without runtime PM
or a .close callback leave the regulator and VCM servo energized permanently
from boot?

Although linux/pm_runtime.h is included, runtime PM is never set up, so the
actuator draws current continuously even when the camera is idle or the
system is suspended.  In addition, the return value of cci_write() here is
ignored, and lc898123_open() unconditionally rewrites LC898123_REG_OIS_CTRL,
LC898123_REG_SERVO and LC898123_REG_AF_POS on every open() of the subdev
node, which can disturb an active camera session if another process opens
the device node.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-sm8150-camera-v1-0-3d2e7b407516@gmail.com?part=8