Re: [PATCH 2/2] media: i2c: imx678: Add driver for Sony IMX678

[Krzysztof Kozlowski <krzk@kernel.org>]

On Wed, May 13, 2026 at 09:03:17PM +0530, Jai Luthra wrote:
> +static int imx678_disable_streams(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state,
> +				  u32 pad, u64 mask)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct imx678 *imx678 = to_imx678(sd);
> +	int ret = 0;
> +
> +	/* Master mode disable */
> +	cci_write(imx678->cci, IMX678_REG_XMSTA, 0x01, &ret);
> +	/* Standby */
> +	cci_write(imx678->cci, IMX678_REG_MODE_SELECT, IMX678_MODE_STANDBY, &ret);
> +	if (ret)
> +		dev_err(&client->dev, "%s failed to stop stream\n", __func__);
> +
> +	pm_runtime_put(&client->dev);
> +
> +	return ret;
> +}
> +
> +static int imx678_power_on(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx678 *imx678 = to_imx678(sd);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(imx678_supply_name), imx678->supplies);
> +	if (ret) {
> +		dev_err(&client->dev, "%s: failed to enable regulators\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	usleep_range(500, 550); /* Tlow */
> +
> +	gpiod_set_value_cansleep(imx678->reset_gpio, 1);

Never tested. How can you have device powered on when it is in reset
stage?

> +
> +	ret = clk_prepare_enable(imx678->xclk);
> +	if (ret) {
> +		dev_err(&client->dev, "%s: failed to enable clock\n",
> +			__func__);
> +		goto reg_off;
> +	}
> +
> +	usleep_range(20, 22); /* T4 */
> +
> +	return 0;
> +
> +reg_off:
> +	regulator_bulk_disable(ARRAY_SIZE(imx678_supply_name), imx678->supplies);
> +	return ret;
> +}
> +
> +static int imx678_power_off(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx678 *imx678 = to_imx678(sd);
> +
> +	gpiod_set_value_cansleep(imx678->reset_gpio, 0);

And here as well bug. And if you looked at existing drivers you would
see how to do that correctly.


> +	regulator_bulk_disable(ARRAY_SIZE(imx678_supply_name), imx678->supplies);
> +	clk_disable_unprepare(imx678->xclk);
> +
> +	/* Force reprogramming of the common registers when powered up again. */
> +	imx678->common_regs_written = false;

You should just use regcache.

Best regards,
Krzysztof