Re: [PATCH v5 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:
- [Medium] media: i2c: s5kjn5: incorrect hardware reset and clock sequencing
--

commit a671bdcf1777ec3f49c4fcba2384ef8b1cb05ef7
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 10-bit
RAW MIPI CSI-2 image sensor. The driver supports standard controls and exposes
a 4096x3072 streaming mode.

> diff --git a/drivers/media/i2c/s5kjn5.c b/drivers/media/i2c/s5kjn5.c
> new file mode 100644
> index 0000000000000..2b925a21f7274
> --- /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: Medium]
Does this sequence correctly initialize the hardware? The s5kjn5_power_on()
function de-asserts the hardware reset line before enabling the input clock.

Standard MIPI CSI-2 image sensors typically require the input clock to be
stable and running before the reset line is released, and to remain running
until the sensor is put back into reset. Leaving the sensor out of reset
without a driving clock can cause internal state machines to hang, fail
initialization unpredictably, or draw excessive current.

This might work accidentally on boards where the bootloader or platform leaves
the clock running, but could fail otherwise.

> +	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: Medium]
Similarly, s5kjn5_power_off() disables the clock before asserting the reset
line. Should the reset line be asserted before the clock is disabled?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-sk5jn5-v5-0-19aa0a0a68eb@oss.qualcomm.com?part=2