Re: [PATCH v2 3/4] media: i2c: imx585: Add Sony IMX585 image-sensor driver

[Krzysztof Kozlowski]

On Sun, Aug 10, 2025 at 11:09:20PM +0100, Will Whang wrote:
> +
> +/* --------------------------------------------------------------------------
> + * Power / runtime PM
> + * --------------------------------------------------------------------------
> + */
> +
> +static int imx585_power_on(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct imx585 *imx585 = to_imx585(sd);
> +	int ret;
> +
> +	dev_dbg(imx585->clientdev, "power_on\n");
> +
> +	ret = regulator_bulk_enable(IMX585_NUM_SUPPLIES, imx585->supplies);
> +	if (ret) {
> +		dev_err(imx585->clientdev, "Failed to enable regulators\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(imx585->xclk);
> +	if (ret) {
> +		dev_err(imx585->clientdev, "Failed to enable clock\n");
> +		goto reg_off;
> +	}
> +
> +	gpiod_set_value_cansleep(imx585->reset_gpio, 1);

You asserted reset gpio causing it to enter reset and you call this
"power on"?

> +	usleep_range(IMX585_XCLR_MIN_DELAY_US,
> +		     IMX585_XCLR_MIN_DELAY_US + IMX585_XCLR_DELAY_RANGE_US);
> +	return 0;
> +
> +reg_off:
> +	regulator_bulk_disable(IMX585_NUM_SUPPLIES, imx585->supplies);
> +	return ret;
> +}
> +
> +static int imx585_power_off(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct imx585 *imx585 = to_imx585(sd);
> +
> +	dev_dbg(imx585->clientdev, "power_off\n");
> +
> +	gpiod_set_value_cansleep(imx585->reset_gpio, 0);

And here device comes up, but you call it power off? Your functions or
reset gpio code are completely reversed/wrong.

Best regards,
Krzysztof