Re: [PATCH v2 2/3] media: i2c: Add Sony IMX681 sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] V4L2 controls are silently ignored when CONFIG_PM is disabled.
--

commit 399d495a390ace2e779563ec50b4c97d653a6b76
Author: Andre Gilerson <andre.gilerson@gmail.com>

media: i2c: Add Sony IMX681 sensor driver
    
The Sony IMX681 is the user-facing camera on the Microsoft Surface Pro 11
for Business (Intel Lunar Lake, IPU7), where it is enumerated as ACPI
device SONY0681. Without a driver the camera does not appear at all.

> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 5c52007f9cbeb..5f7b0d22c8f07 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -310,6 +310,16 @@ config VIDEO_IMX678
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called imx678.
>  
> +config VIDEO_IMX681
> +	tristate "Sony IMX681 sensor support"
> +	select V4L2_CCI_I2C
> +	help
> +	  This is a Video4Linux2 sensor driver for the Sony
> +	  IMX681 camera.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called imx681.
> +

[ ... ]

> diff --git a/drivers/media/i2c/imx681.c b/drivers/media/i2c/imx681.c
> new file mode 100644
> index 0000000000000..bdaf435541a82
> --- /dev/null
> +++ b/drivers/media/i2c/imx681.c
> @@ -0,0 +1,880 @@

[ ... ]

> +static int imx681_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct imx681 *imx681 = container_of(ctrl->handler, struct imx681,
> +					     ctrl_handler);
> +	s64 exposure_max;
> +	int ret = 0;
> +
> +	/* Update exposure max when VBLANK changes (even when not streaming) */
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		exposure_max = IMX681_HEIGHT + ctrl->val - IMX681_EXPOSURE_OFFSET;
> +		__v4l2_ctrl_modify_range(imx681->exposure,
> +					 IMX681_EXPOSURE_MIN, exposure_max,
> +					 1, IMX681_EXPOSURE_DEFAULT);
> +	}
> +
> +	/*
> +	 * Only apply controls to hardware when streaming. The call returns 1
> +	 * having taken a reference, 0 if the device is not active, or -EINVAL
> +	 * if runtime PM is disabled - and in that last case no reference is
> +	 * taken, so it must not reach the pm_runtime_put() below.
> +	 */
> +	if (pm_runtime_get_if_active(imx681->dev) <= 0)
> +		return 0;

[Severity: Medium]
When the driver is built without CONFIG_PM, pm_runtime_get_if_active()
returns -EINVAL. Does this code misinterpret that error code as the device 
being inactive?

Since VIDEO_IMX681 doesn't select or depend on PM in Kconfig, returning 0
here when pm_runtime_get_if_active() returns -EINVAL will silently ignore 
all V4L2 control writes (exposure, gain, blanking, test pattern) when the 
kernel is compiled without CONFIG_PM.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909193953.86192-1-lsa.uz@pm.me?part=2