Re: [PATCH v4 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] Incorrect ordering of clock and reset signals during power-on and power-off sequences.
- [Medium] Missing `pm_runtime_mark_last_busy()` before dropping the runtime PM reference defeats the autosuspend delay.
--

commit ba7e808a699f95cdf2ed0596c95aeb83fbeac746
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, a
50 MP 10-bit RAW MIPI CSI-2 image sensor. The driver supports the
4096x3072 mode and exposes standard v4l2 controls.

> diff --git a/drivers/media/i2c/s5kjn5.c b/drivers/media/i2c/s5kjn5.c
> new file mode 100644
> index 0000000000000..edf824228bb6f
> --- /dev/null
> +++ b/drivers/media/i2c/s5kjn5.c
[ ... ]
> +static int s5kjn5_power_on(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct s5kjn5 *s5kjn5 = sd_to_s5kjn5(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(s5kjn5_supply_names),
> +				    s5kjn5->supplies);
> +	if (ret) {
> +		dev_err(dev, "failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	gpiod_set_value_cansleep(s5kjn5->reset_gpio, 0);
> +	usleep_range(8000, 10000);
> +
> +	ret = clk_prepare_enable(s5kjn5->inclk);

[Severity: High]
Does this power-on sequence introduce a regression with hardware sequencing?
The reset line is de-asserted before the sensor clock is running. Transitioning
the reset signal without an active external clock violates the synchronous
reset requirements of the sensor's digital logic, which might leave the
internal state machine in an undefined or latch-up state.

> +	if (ret) {
> +		dev_err(dev, "failed to enable clock: %d\n", ret);
> +		gpiod_set_value_cansleep(s5kjn5->reset_gpio, 1);
> +		regulator_bulk_disable(ARRAY_SIZE(s5kjn5_supply_names),
> +				       s5kjn5->supplies);
> +		return ret;
> +	}
> +
> +	usleep_range(10000, 12000);
> +
> +	return 0;
> +}
> +
> +static int s5kjn5_power_off(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct s5kjn5 *s5kjn5 = sd_to_s5kjn5(sd);
> +
> +	clk_disable_unprepare(s5kjn5->inclk);
> +	gpiod_set_value_cansleep(s5kjn5->reset_gpio, 1);

[Severity: High]
Can this sequence cause a hardware latch-up? The clock is killed before the
sensor is put back into hardware reset, which reverses the typical power-off
ordering and violates hardware sequencing rules.

> +	regulator_bulk_disable(ARRAY_SIZE(s5kjn5_supply_names),
> +			       s5kjn5->supplies);
> +
> +	return 0;
> +}
[ ... ]
> +static int s5kjn5_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct s5kjn5 *s5kjn5 =
> +		container_of(ctrl->handler, struct s5kjn5, ctrl_handler);
> +	struct device *dev = s5kjn5->sd.dev;
> +	const struct s5kjn5_mode *mode = &s5kjn5_modes[0];
> +	u32 exposure_max;
> +	int ret;
[ ... ]
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:
> +		ret = cci_write(s5kjn5->regmap, S5KJN5_REG_EXPOSURE,
> +				ctrl->val, NULL);
> +		break;
> +
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		ret = cci_write(s5kjn5->regmap, S5KJN5_REG_AGAIN,
> +				ctrl->val, NULL);
> +		break;
[ ... ]
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put_autosuspend(dev);

[Severity: Medium]
Is a call to pm_runtime_mark_last_busy() missing before dropping the runtime
PM reference here?

Without marking the device as busy, the PM core evaluates the delay against a
stale last_busy timestamp (which was set when the device originally resumed).
This calculates that the delay has already expired and suspends the device
instantly, defeating the autosuspend optimization.

> +
> +	return ret;
> +}
[ ... ]
> +static int s5kjn5_disable_streams(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state,
> +				  u32 pad, u64 streams_mask)
> +{
> +	struct s5kjn5 *s5kjn5 = sd_to_s5kjn5(sd);
> +	struct device *dev = sd->dev;
> +	u64 val;
> +	int ret;
> +	int i;
> +
> +	ret = cci_write(s5kjn5->regmap, S5KJN5_REG_STREAMING,
> +			S5KJN5_STREAMING_OFF, NULL);
> +	if (ret)
> +		dev_warn(dev, "failed to stop streaming: %d\n", ret);
> +
> +	/* Poll frame count register until it reads standby */
> +	for (i = 0; i < S5KJN5_STANDBY_POLL_ITERS; i++) {
> +		usleep_range(5000, 6000);
> +		ret = cci_read(s5kjn5->regmap, S5KJN5_REG_FRAME_COUNT,
> +			       &val, NULL);
> +		if (!ret && val == S5KJN5_FRAME_COUNT_STANDBY)
> +			break;
> +	}
> +	if (i == S5KJN5_STANDBY_POLL_ITERS)
> +		dev_warn(dev, "timed out waiting for standby\n");
> +
> +	pm_runtime_put_autosuspend(dev);

[Severity: Medium]
Similarly, does this stream disable path need to mark the device as busy?

Because pm_runtime_mark_last_busy() is omitted before
pm_runtime_put_autosuspend(), the sensor powers down immediately when streaming
stops instead of remaining active for the configured 1000ms autosuspend window.
This can cause unnecessary power cycling latency if the stream is restarted
shortly after.

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-sk5jn5-v4-0-386e84cfb2b3@oss.qualcomm.com?part=2