Re: [PATCH 2/2] media: i2c: Add driver for Sony IMX662 sensor

[Krzysztof Kozlowski <krzk@kernel.org>]

On Thu, Mar 12, 2026 at 06:04:37PM +0300, Alexander Shiyan wrote:
> This patch adds a V4L2 subdevice driver for the Sony IMX662 CMOS image

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v6.16/source/Documentation/process/submitting-patches.rst#L94

...

> +
> +static int imx662_power_on(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct imx662 *imx662 = to_imx662(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(imx662->supplies),
> +				    imx662->supplies);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");

Is this probe path?

> +
> +	ret = clk_prepare_enable(imx662->clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock\n");
> +		regulator_bulk_disable(ARRAY_SIZE(imx662->supplies),
> +				       imx662->supplies);

If yes, thus must be return dev_err_probe. If not, earlier code was
wrong.

> +		return ret;
> +	}
> +
> +	usleep_range(1, 2);
> +	gpiod_set_value_cansleep(imx662->reset, 1);

You did not test your code. Logical level 1 means reset is asserted, so
you leave your device in reset state after power on.

> +	usleep_range(30000, 31000);
> +
> +	return 0;
> +}
> +
> +static int imx662_power_off(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct imx662 *imx662 = to_imx662(sd);
> +
> +	gpiod_set_value_cansleep(imx662->reset, 0);

And you de-assert it in power off.

Look how other drivers do it. They even have explicit comment that above
is wrong.

Best regards,
Krzysztof