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

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/[e-mail]

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/[e-mail]

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

Comments

Dave Stevenson Sept. 2, 2024, 5:55 p.m. UTC | #1

Hi Benjamin

On Mon, 2 Sept 2024 at 16:57, <[e-mail]> 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/[e-mail]
>
> 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/[e-mail]
>
> ---
> 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 <[e-mail]>
>
>

Benjamin Bara Sept. 2, 2024, 6:18 p.m. UTC | #2

Hi Dave!

Thank you for your feedback!

On Mon, 2 Sept 2024 at 19:55, Dave Stevenson
<[e-mail]> wrote:
> On Mon, 2 Sept 2024 at 16:57, <[e-mail]> 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/[e-mail]
> >
> > 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/[e-mail]
> >
> > ---
> > 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 <[e-mail]>
> >
> >