From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: "Mauro Carvalho Chehab" <mchehab@kernel.org>,
"Kieran Bingham" <kieran.bingham+renesas@ideasonboard.com>,
"Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
"Hans Verkuil" <hverkuil-cisco@xs4all.nl>,
"Sakari Ailus" <sakari.ailus@iki.fi>,
"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
"Thomas NIZAN" <tnizan@witekio.com>,
linux-renesas-soc@vger.kernel.org, linux-media@vger.kernel.org
Subject: Re: [RFC 0/5] media: i2c: Add MAX9271 subdevice driver
Date: Mon, 23 Aug 2021 05:06:51 +0300 [thread overview]
Message-ID: <YSMCu1zQ0xOkj7/y@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20210817072703.1167-1-jacopo+renesas@jmondi.org>
Hi Jacopo,
On Tue, Aug 17, 2021 at 09:26:58AM +0200, Jacopo Mondi wrote:
> Hello,
> as noticed during the inclusion of the RDACM20/21 cameras, their driver make
> use of a library driver that exports functions to control the MAX9271 GMSL
> serializer embedded in the camera module.
>
> This series attempts to create an i2c subdevice driver for the MAX9271
> serializer, to support the camera module operations using the v4l2 subdev
> operations.
>
> The series is based on the currently in-review:
> https://patchwork.linuxtv.org/project/linux-media/list/?series=5847
> https://patchwork.linuxtv.org/project/linux-media/list/?series=5949
>
> The series:
> 1) Introduced an i2c subdev driver for the MAX9271 GMSL serializer
> 2) Adapt the RDACM20 driver by removing the MAX9271 handling from there
> 3) Modify the DTS layout to describe the MAX9271 chip and the camera module
> separately
>
> To be done:
> - bindings
> - handling of reset lines between max9271 and image sensor
> - the camera module drivers could be made sensor drivers
>
> However I'm not fully convinced this really brings any benefit as the serializer
> and the image sensor are actually packed together in the same camera module
> and are tightly coupled.
I'm not convinced either. More than that, I think it will make it
impossible to handle more complex camera topologies.
> The biggest issue I'm facing, and for which I would be happy to receive pointers
> to is the following one.
>
> The new DTS layout now looks like
>
> max9286 {
>
> i2c-mux {
> i2c@0 {
> max9271 {
> }
>
> rdacm20{
> }
> }
> }
> }
>
> If I do rely on the probe sequence implemented by the instantiation of the
> i2c-mux child nodes:
>
> - max9286
> -max9271
> -sensor
>
> -max9271
> -sensor
>
> ...
>
> As per each i2c-mux subnode the max9271 and the connected sensor are probed once
> after the other.
>
> This unfortunately doesn't play well with the requirements of GMSL bus, for
> which the post_register operation is being introduced. With the current
> RDACM20/21 drivers and post_register in place with two cameras connected to the
> system, the desired initialization sequence looks like:
>
> MAX9286 RDACM20/21
>
> probe()
> |
> ---------------------> |
> camera 1 probe() {
> enable_threshold()
> }
> |<--------------------|
> v4l2 async bound {
> completed = no
> |
> ---------------------> |
> camera 2 probe() {
> enable_threshold()
> }
> |<--------------------|
> completed = yes
>
> compensate_amplitude()
>
> call post_register()
> |-------------------->|
> camera 1 post_register()
> access camera registers()
> }
> |<-------------------
> |-------------------->|
> camera 2 post_register()
> access camera registers()
> }
> |<-------------------
> }
>
> Which guarantees that the bulk access to the camera registers happens after the
> deserializer has compensated the channel amplitude.
>
> With the new model I do have a race between the sensor probe and the
> post_register() of the serializer in case a single camera is connected.
>
> What happes is that as soon as the max9271 registers its async subdev the
> max9286 notifier completes an call max9271->post_register(). But at that time
> the sensor subdev has not probed yet, so there is no subdev on which to call
> post_register in the max9271
>
> following:
>
> MAX9286 MAX9271 SENSOR
>
> probe()
> |
> ---------------------> |
> probe() {
> enable_threshold()
> }
> }
> |<--------------------|
> v4l2 async bound {
> completed = yes
> subdev->post_register()
> |-------------------->|
> post_register()
> gmsl_bus_config()
> subdev->post_register(NULL)
> segfault
> }
> probe()
> }
>
> If I instead do not use post_register() between the max9271 and the sensor,
> then the model works for a single camera only (as it is implemented in this
> version)
>
> MAX9286 MAX9271 SENSOR
>
> probe()
> |
> ---------------------> |
> probe() {
> enable_threshold()
> }
> }
> |<--------------------|
> v4l2 async bound {
> completed = no
> |-------------------->|
> probe() {
> i2c writes to
> the sensor without
> GMSL configuration
> }
> }
>
> So, my question is: are there examples on how to have the max9271 driver
> control the probe time the connected sensor without relying on the probe
> sequence of the I2C-mux device nodes ? If I could do so, what I would like to
> realize looks like
How about making the sensor a child of the max9271 in DT ?
>
> MAX9286 MAX9271 SENSOR
>
> probe()
> |
> ---------------------> |
> camera 1 probe() {
> --------------------->|
> sensor probe()
> enable_threshold()
> }
> }
> |<--------------------|
> v4l2 async bound {
> completed = no
> |-------------------->|
> camera 2 probe() {
> --------------------->|
> sensor probe()
> enable_threshold()
> }
> |<--------------------|
> completed = yes
>
> compensate_amplitude()
> for (subdev)
> subdev->post_register()
> |----------------->|
> camera 1 post_register()
> subdev->post_register()
> --------------------->|
> post_register()
> i2c writes
> subdev->post_register()
> |----------------->|
> camera 2 post_register()
> subdev->post_register()
> --------------------->|
> post_register()
> i2c writes
> }
>
>
> I recall Mauro pointed me to an example when he first suggested to make the
> MAX9271 library a proper i2c subdevice driver. Do you happen to recall which one
> was it ?
>
> Thanks
> j
>
> Jacopo Mondi (5):
> media: i2c: max9271: Rename max9271 library driver
> media: i2c: Add MAX9271 I2C driver
> media: i2c: rdacm20: Adapt to work with MAX9271
> media: i2c: max9286: Fetch PIXEL_RATE in s_stream
> arm64: dts: GMSL: Adapt to the use max9271 driver
>
> MAINTAINERS | 17 +-
> arch/arm64/boot/dts/renesas/gmsl-cameras.dtsi | 34 +-
> .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 6 +-
> drivers/media/i2c/Kconfig | 12 +
> drivers/media/i2c/Makefile | 3 +-
> drivers/media/i2c/max9271-lib.c | 374 +++++++++++++
> .../media/i2c/{max9271.h => max9271-lib.h} | 0
> drivers/media/i2c/max9271.c | 528 +++++++++++++++---
> drivers/media/i2c/max9286.c | 6 +-
> drivers/media/i2c/rdacm20.c | 139 +----
> drivers/media/i2c/rdacm21.c | 2 +-
> 11 files changed, 917 insertions(+), 204 deletions(-)
> create mode 100644 drivers/media/i2c/max9271-lib.c
> rename drivers/media/i2c/{max9271.h => max9271-lib.h} (100%)
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2021-08-23 2:07 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-17 7:26 [RFC 0/5] media: i2c: Add MAX9271 subdevice driver Jacopo Mondi
2021-08-17 7:26 ` [RFC 1/5] media: i2c: max9271: Rename max9271 library driver Jacopo Mondi
2021-08-18 12:48 ` Kieran Bingham
2021-08-23 2:09 ` Laurent Pinchart
2021-08-17 7:27 ` [RFC 2/5] media: i2c: Add MAX9271 I2C driver Jacopo Mondi
2021-08-17 15:49 ` Kieran Bingham
2021-08-18 8:27 ` Jacopo Mondi
2021-08-18 12:38 ` Kieran Bingham
2021-08-23 2:22 ` Laurent Pinchart
2021-08-23 7:21 ` Jacopo Mondi
2021-08-23 12:05 ` Geert Uytterhoeven
2021-08-17 7:27 ` [RFC 3/5] media: i2c: rdacm20: Adapt to work with MAX9271 Jacopo Mondi
2021-08-17 7:27 ` [RFC 4/5] media: i2c: max9286: Fetch PIXEL_RATE in s_stream Jacopo Mondi
2021-08-23 2:17 ` Laurent Pinchart
2021-08-23 7:20 ` Jacopo Mondi
2021-08-23 9:34 ` Laurent Pinchart
2021-08-17 7:27 ` [RFC 5/5] arm64: dts: GMSL: Adapt to the use max9271 driver Jacopo Mondi
2021-08-18 12:47 ` [RFC 0/5] media: i2c: Add MAX9271 subdevice driver Kieran Bingham
2021-08-23 2:12 ` Laurent Pinchart
2021-08-23 2:06 ` Laurent Pinchart [this message]
2021-09-13 7:59 ` Jacopo Mondi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YSMCu1zQ0xOkj7/y@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=jacopo+renesas@jmondi.org \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=mchehab@kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=sakari.ailus@iki.fi \
--cc=tnizan@witekio.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).