Message-ID: <CAJpcXm6YxT69nYy+j8zkswsmaV_93S+dyXFjaJet7Jfm-cHv7A@mail.gmail.com>
Date: Mon, 2 Sep 2024 20:18:45 +0200
From: Benjamin Bara <bbara93@...il.com>
To: Dave Stevenson <dave.stevenson@...pberrypi.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 Dave!

Thank you for your feedback!

On Mon, 2 Sept 2024 at 19:55, Dave Stevenson
<dave.stevenson@...pberrypi.com> wrote:
> On Mon, 2 Sept 2024 at 16:57, <bbara93@...il.com> 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@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