mbox series

[v3,0/7] media: i2c: imx290: check for availability in probe()

Message ID 20240902-imx290-avail-v3-0-b32a12799fed@skidata.com
Headers show
Series media: i2c: imx290: check for availability in probe() | expand

Message

Benjamin Bara Sept. 2, 2024, 3:57 p.m. UTC 
Hi!

This series introduces i2c communication with the imx290 sensor during
probe s.t. the v4l2 subdev is not initialized when the chip is not
reachable.

The datasheets show that INCKSEL* registers have a different default
value after reset on imx290[1] and imx327[2], however I am not sure if
this is a sufficient identification option - therefore I just removed
the current CHIP_ID register for now.

Thank you all for the feedback and the discussion, I updated the series
to contain some of the ideas that came up.

For now, I kept reading back the content of the STANDBY register, as
suggested by Sakari and Alexander. If not wanted (as suggested by
Laurent), I can re-add my "optional read" commit from the first version
of this series instead.

*Overview:*
Patch 1/7 is a split from the old 1/2 which defines the values of the
STANDBY register and uses them.
Patch 2/7 is new and defines the whole supported range of the controls.
Patch 3/7 is the old 2/2 to drop the CHIP_ID register.
Patch 4+5/7 are new and target the avoidable communication during
probe(). I decided to use a variant that also works on machines without
runtime PM active, and falls back to the values of 2/7 instead.
Additionally, imx290_runtime_suspend() is using v4l_subdev, and
therefore depends on the subdevice. If we move the autosuspend stuff
before the subdevice creation, we basically have a race between the
delay and the subdevice creation. Although it is not very close, I don't
think it is a good idea to potentially autosuspend before the subdev is
created.
Patch 6/7 is the old 1/2.
Patch 7/7 is a new PoC to decide to check the availability based on the
power state of the sensor during probe().

Note: dummy-regulators, which are used when no supplies are set in the
DT, are always-on.
Note2: The "off" mode is currently only active after probe(). If this
approach is of interest, I would also ensure that it is active once
streaming has stopped - need to dig deeper if it is necessary to store
the "old current" before stopping, therefore only "initial" for now.

thanks & regards
Benjamin

[1] https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf
[2] https://e2e.ti.com/cfs-file/__key/communityserver-discussions-components-files/138/IMX327LQR_2D00_C_5F00_TechnicalDatasheet_5F00_E_5F00_Rev0.2.pdf

---
Changes in v3:
- probably better readable in the overview
- 1/2 -> 1+6/7
- 2/2 -> 3/7 (added R-b - thx for that)
- others are new based on the discussions/suggestions
- Link to v2: https://lore.kernel.org/r/20240828-imx290-avail-v2-0-bd320ac8e8fa@skidata.com

Changes in v2:
- dropped 1/2 to ignore the read value in cci_read() (old 2/2 -> new 1/2)
- 1/2: read-back standby mode instead and ensure that it is really in standby
- new 2/2: drop chip_id register definition which seems to be incorrect
- Link to v1: https://lore.kernel.org/r/20240807-imx290-avail-v1-0-666c130c7601@skidata.com

---
Benjamin Bara (7):
      media: i2c: imx290: Define standby mode values
      media: i2c: imx290: Define absolute control ranges
      media: i2c: imx290: Remove CHIP_ID reg definition
      media: i2c: imx290: Introduce initial "off" mode & link freq
      media: i2c: imx290: Avoid communication during probe()
      media: i2c: imx290: Check for availability in probe()
      media: i2c: imx290: Implement a "privacy mode" for probe()

 drivers/media/i2c/imx290.c | 153 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 124 insertions(+), 29 deletions(-)
---
base-commit: eec5d86d5bac6b3e972eb9c1898af3c08303c52d
change-id: 20240807-imx290-avail-85795c27d988

Best regards,