Re: [PATCH v2 2/2] media: i2c: add imx415 cmos image sensor driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Michael,

On Fri, Jan 27, 2023 at 11:43:33AM +0100, Michael Riesch wrote:
> Hi Sakari,
> 
> Thanks for your review. The majority of your comments are clear, I'll
> spin a v3 next week. Just a few things:
> 
> On 1/25/23 11:55, Sakari Ailus wrote:
> > [...]
> >> +++ b/drivers/media/i2c/imx415.c
> >> @@ -0,0 +1,1296 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Driver for the Sony IMX415 CMOS Image Sensor.
> >> + *
> >> + * Copyright (C) 2022 WolfVision GmbH.
> > 
> > You can use 2023 now.
> 
> Time flies, doesn't it... :-)
> 
> > [...]
> >> +static int imx415_stream_on(struct imx415 *sensor)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = imx415_write(sensor, IMX415_MODE, IMX415_MODE_OPERATING);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* wait at least 24 ms for internal regulator stabilization */
> >> +	msleep(30);
> > 
> > This is a very, very long time to wait for a regulator. Most probably
> > either the time is too long or we're waiting for something else.
> 
> I just realized that both msleep calls are after setting the mode to
> operating, i.e., after getting the sensor out of standby. The other
> instance of this code (see below) documents that clearly, but this
> "regulator stabilization" comment here is seems wrong indeed.
> 
> >> +
> >> +	return imx415_write(sensor, IMX415_XMSTA, IMX415_XMSTA_START);
> >> +}
> >> [...]>> +static int imx415_subdev_init(struct imx415 *sensor)
> >> +{
> >> +	struct i2c_client *client = to_i2c_client(sensor->dev);
> >> +	int ret;
> >> +
> >> +	v4l2_i2c_subdev_init(&sensor->subdev, client, &imx415_subdev_ops);
> >> +
> >> +	ret = imx415_ctrls_init(sensor);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > 
> > Add V4L2_SUBDEV_FL_HAS_EVENTS.
> 
> Just for my understanding: why is this required/a good idea?
> 
> >> [...]
> >> +static int imx415_identify_model(struct imx415 *sensor)
> >> +{
> >> +	int model, ret;
> >> +
> >> +	/*
> >> +	 * While most registers can be read when the sensor is in standby, this
> >> +	 * is not the case of the sensor info register :-(
> >> +	 */
> >> +	ret = imx415_write(sensor, IMX415_MODE, IMX415_MODE_OPERATING);
> >> +	if (ret < 0)
> >> +		return dev_err_probe(sensor->dev, ret,
> >> +				     "failed to get sensor out of standby\n");
> >> +
> >> +	/*
> >> +	 * According to the datasheet we have to wait at least 63 us after
> >> +	 * leaving standby mode. But this doesn't work even after 30 ms.
> >> +	 * So probably this should be 63 ms and therefore we wait for 80 ms.
> >> +	 */
> >> +	msleep(80);
> > 
> > Wow.
> 
> This is the other occurrence of this long sleep. We could refactor this
> code into a imx415_wakeup() method if desired. Otherwise, we need to
> align the sleep period and the explanation at least.

I'm ok with the code, it's just a very, very long delay.

-- 
Sakari Ailus