Message-ID: <CAPY8ntD4tbSF41+ShJ0WdP5=N3uJi2tWDEyr-ypcKha40T+Q=Q@mail.gmail.com>
Date: Mon, 2 Sep 2024 18:55:16 +0100
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: bbara93@...il.com
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, Hans de Goede <hdegoede@...hat.com>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Alexander Stein <alexander.stein@...tq-group.com>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Benjamin Bara <benjamin.bara@...data.com>
Subject: Re: [PATCH v3 0/7] media: i2c: imx290: check for availability in probe()

Hi Benjamin

On Mon, 2 Sept 2024 at 16:57, <bbara93@...il.com> wrote:
>
> 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.

I was reading through patches 2, 4, 5, and 7 and really not seeing
what you're trying to avoid here. Adding an option to avoid powering
up the sensor is one thing, but we've got masses of other changes to
fake HBLANK, VBLANK, and LINK_FREQ values, even though we appear to
still have a pad format defined as 1920x1080 1x10 set on the pad. All
those controls are therefore declaring invalid ranges at that point.

If it's solely trying to stop imx290_set_ctrl sending register writes
when not streaming, isn't it simpler to have a basic bool in the state
structure to denote whether we've passed through
imx290_set_stream(subdev, 1)? Set it in the same place as
pm_runtime_resume_and_get is called as it's mimicking exactly the same
behaviour.

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

What extra data do you see sent after stopping? On any system with
runtime PM, imx290_set_ctrl will stop at the "if
(!pm_runtime_get_if_in_use(imx290->dev))" check.

  Dave

> 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,
> --
> Benjamin Bara <benjamin.bara@...data.com>
>
>

Powered by blists - more mailing lists