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

[Michael Riesch <michael.riesch@wolfvision.net>]

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.


Best regards,
Michael

> [...]