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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect hardware power-on/off sequence: reset is de-asserted without a running clock.
--

--- Patch [2]: [PATCH 2/2] media: i2c: Add Samsung S5KJN5 image sensor driver ---
commit 05e0426a4b9d13689d0f80d9c414692c4736124c
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 image
sensor. It supports the 4096x3072 mode and exposes standard controls for
exposure, gain, blanking, and test patterns.

> diff --git a/drivers/media/i2c/s5kjn5.c b/drivers/media/i2c/s5kjn5.c
> new file mode 100644
> index 0000000000000..294a4c3c5d2c5
> --- /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(S5KJN5_NUM_SUPPLIES, 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 sequence de-assert the reset line before the master clock is
running? Releasing reset without an active master clock can sometimes cause
the sensor's internal state machines to enter an undefined or locked-up
state, which might lead to I2C communication failures or unreliable
initialization.

[ ... ]

> +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]
Similarly, does this stop the clock before placing the sensor back into
reset? Typically, the sensor should be placed into reset while the clock is
still running to ensure a clean shutdown state.

> +	regulator_bulk_disable(S5KJN5_NUM_SUPPLIES, s5kjn5->supplies);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-sk5jn5-v1-0-da610d7fd494@oss.qualcomm.com?part=2