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

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

Hi Sakari,

We tried to gather more precise information from the IMX415 data sheet
and support package, but these documents are super secret and equally
super vague.

On 2/2/23 10:01, Sakari Ailus wrote:
> [...]
>>>> +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.

The data sheet mentions "Some time is required for sensor internal
circuit stabilization..." and "...after internal regulator stabilization
24 ms or more." Well, there you go -- we need to wait some time or more
for whatever reason :-)

>>
>>>> +
>>>> +	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.

It is, and it is far from obvious how this delay comes about. The data
sheet does not specify this register at all, it is described somewhere
in the support package (as the comment above states a delay of 63
microseconds is mentioned).

Our tests indicated that the delay
 a) read out the sensor info
 b) turn on the stream
can be different. Therefore, it might make sense to use different
msleep() calls as done in the v2 of our series.

We'll try to get an updated data sheet to sort this issue out, but for
the time being can we get v3 into mainline?

Best regards,
Michael