Re: [PATCH v6 2/5] media: i2c: imx355: Support devicetree and power management

[Richard Acayan <mailingradian@gmail.com>]

On Wed, Jan 07, 2026 at 08:49:02PM +0200, Sakari Ailus wrote:
> Hi Richard,
> On Tue, Jan 06, 2026 at 11:30:41PM -0500, Richard Acayan wrote:
> > A device tree compatible makes it possible for this driver to be used on
> > Open Firmware devices. Initialization of power-managed resources such as
> > the reset GPIO and voltage regulators can be specified in the device
> > tree and handled by the driver. Add support for this so the Pixel 3a can
> > use the driver.
> > 
> > Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> > Nacked-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Uh-oh.
(snip)
> > +static int imx355_power_on(struct device *dev)
> > +{
> > +	struct i2c_client *client = container_of(dev, struct i2c_client, dev);
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +	struct imx355 *imx355 = to_imx355(sd);
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(imx355->clk);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable clocks: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = regulator_bulk_enable(ARRAY_SIZE(imx355_supplies),
> > +				    imx355->supplies);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable regulators: %d\n", ret);
> > +		goto error_disable_clocks;
> > +	}
> > +
> > +	gpiod_set_value_cansleep(imx355->reset_gpio, 1);
> > +	usleep_range(5000, 5100);
> > +	gpiod_set_value_cansleep(imx355->reset_gpio, 0);
> > +	usleep_range(8000, 8100);
> 
> Where do these values come from? Apparently 1 ms is required (after lifting
> xshutdown) before reading identification register and 10 ms is required
> (from the same point of time) before streaming can be enabled.

I don't have the hardware specification. The sequence seemed to last
12-13 ms. Dividing the delays are a guess and the camera can usually
open at least a few times.

The final 8 ms wait can be extended to 10 ms (assuming xshutdown is the
same as the reset line). Is there a minimum time for asserting the reset
(currently 5 ms), or can it be reduced to 1 ms?

New sequence (maybe, if the docs support it):

	gpiod_set_value_cansleep(imx355->reset_gpio, 1);
	usleep_range(1000, 2000);
	gpiod_set_value_cansleep(imx355->reset_gpio, 0);
	usleep_range(10000, 11000);