Hi Dave! Thank you for your feedback! On Mon, 2 Sept 2024 at 19:55, Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> wrote: > On Mon, 2 Sept 2024 at 16:57, <bbara93@xxxxxxxxx> wrote: > > 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. I guess I missed the pad format. My idea with 2+4 was to have "more free control ranges" while we don't have a mode set yet - but it is probably not a really good idea. Patch 5, as you said, can also be replaced with a bool. As Laurent brought up the point with the privacy LED, patch 7 is just an example how we can avoid powering the sensor up during probe time. > 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. This is rather related to patch 2+4. As I already expected that it has downsides to have an extra (invalid) "off mode", I didn't set it everytime streaming is stopped. I guess dropping 2+4 and replacing 5 with a bool is probably a better idea to have this. Thanks Benjamin > > 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@xxxxxxxxxxx > > > > 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@xxxxxxxxxxx > > > > --- > > 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@xxxxxxxxxxx> > > > >
