Re: [PATCH v2 2/2] media: i2c: Add Samsung S5KJN5 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The exposure hardware register is not updated when the maximum exposure limit changes due to a VBLANK modification, leaving the hardware out of sync.
- [Medium] V4L2 controls are silently ignored when CONFIG_PM is disabled, leaving the sensor unconfigured.
--

--- Patch [2]: [PATCH v2 2/2] media: i2c: Add Samsung S5KJN5 image sensor driver ---
commit 583bdec13ff5578760991a928a272b7f11a48522
Author: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>

media: i2c: Add Samsung S5KJN5 image sensor driver
    
This commit adds a V4L2 sub-device driver for the Samsung S5KJN5 50 MP
GBRG 10-bit RAW MIPI CSI-2 image sensor. It supports 4096x3072 mode and
exposes standard controls such as exposure, gain, and vblank/hblank.

> diff --git a/drivers/media/i2c/s5kjn5.c b/drivers/media/i2c/s5kjn5.c
> new file mode 100644
> index 0000000000000..865db808aabcf
> --- /dev/null
> +++ b/drivers/media/i2c/s5kjn5.c
[ ... ]
> +static int s5kjn5_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct s5kjn5 *s5kjn5 =
> +		container_of(ctrl->handler, struct s5kjn5, ctrl_handler);
> +	const struct s5kjn5_mode *mode = &s5kjn5_modes[0];
> +	struct device *dev = s5kjn5->sd.dev;
> +	u32 exposure_max;
> +	int ret = 0;
> +
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		/* Update max exposure while meeting expected vblanking */
> +		exposure_max = mode->height + ctrl->val - mode->exposure_margin;
> +		__v4l2_ctrl_modify_range(s5kjn5->exposure,
> +					 s5kjn5->exposure->minimum,
> +					 exposure_max,
> +					 s5kjn5->exposure->step,
> +					 s5kjn5->exposure->default_value);
> +	}

[Severity: High]
If a user decreases VBLANK causing the V4L2 framework to clamp
s5kjn5->exposure->val to the new exposure_max, will this leave the hardware
exposure register out of sync?

Since the vblank and exposure controls are not clustered, s_ctrl is only
invoked for VBLANK. It updates the VTS register but does not sync the newly
clamped exposure value to the hardware, potentially leaving it with an invalid
value that exceeds the new frame length.

> +
> +	if (pm_runtime_get_if_active(dev) <= 0)
> +		return 0;

[Severity: Medium]
When the kernel is built without CONFIG_PM, pm_runtime_get_if_active() is
stubbed to return -EINVAL.

Will this cause the check to evaluate to true and silently drop all hardware
writes in s_ctrl, leaving the camera unconfigurable?

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:
> +		ret = cci_write(s5kjn5->regmap, S5KJN5_REG_EXPOSURE,
> +				ctrl->val, NULL);
> +		break;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-sk5jn5-v2-0-871d3b9a2e47@oss.qualcomm.com?part=2