[RFC 0/5] media: i2c: Add MAX9271 subdevice driver

* [RFC 0/5] media: i2c: Add MAX9271 subdevice driver
@ 2021-08-17  7:26 Jacopo Mondi
  2021-08-17  7:26 ` [RFC 1/5] media: i2c: max9271: Rename max9271 library driver Jacopo Mondi
                   ` (6 more replies)
  0 siblings, 7 replies; 21+ messages in thread
From: Jacopo Mondi @ 2021-08-17  7:26 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Kieran Bingham, Laurent Pinchart,
	Niklas Söderlund
  Cc: Jacopo Mondi, Hans Verkuil, Sakari Ailus, Manivannan Sadhasivam,
	Thomas NIZAN, linux-renesas-soc, linux-media

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.

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

    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%)

--
2.32.0

^ permalink raw reply	[flat|nested] 21+ messages in thread