mbox series

[v2,0/2] media: i2c: add imx415 cmos image sensor driver

Message ID 20230124060107.3922237-1-michael.riesch@wolfvision.net
Headers show
Series media: i2c: add imx415 cmos image sensor driver | expand

Message

Michael Riesch Jan. 24, 2023, 6:01 a.m. UTC 
Hi all,

This series adds a driver for the Sony IMX415 CMOS image sensor.
The Sony IMX415 is a diagonal 6.4 mm (Type 1/2.8) CMOS active pixel type
solid-state image sensor with a square pixel array and 8.46 M effective
pixels. This chip operates with analog 2.9 V, digital 1.1 V, and interface
1.8 V triple power supply, and has low power consumption.
The IMX415 is programmable through I2C interface. The sensor output is
available via CSI-2 serial data output (two or four lanes).

Version 2 of this series fixes the port description in the devicetree
binding as pointed out by Rob Herring.

Looking forward to your comments!

Best regards,
Michael

Gerald Loacker (1):
  media: i2c: add imx415 cmos image sensor driver

Michael Riesch (1):
  dt-bindings: media: i2c: add imx415 cmos image sensor

 .../bindings/media/i2c/sony,imx415.yaml       |  130 ++
 MAINTAINERS                                   |    8 +
 drivers/media/i2c/Kconfig                     |   14 +
 drivers/media/i2c/Makefile                    |    1 +
 drivers/media/i2c/imx415.c                    | 1296 +++++++++++++++++
 5 files changed, 1449 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
 create mode 100644 drivers/media/i2c/imx415.c


base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2

Comments

Michael Riesch Jan. 27, 2023, 10:43 a.m. UTC | #1 
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

> [...]