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

[Jacopo Mondi <jacopo@jmondi.org>]

Hi Laurent,

On Mon, Aug 23, 2021 at 05:06:51AM +0300, Laurent Pinchart wrote:
> 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 ?

I think that would ideally be feasible and it would match the actual
HW, as max9271 is actually an i2c multiplexer.

But is something like

 	max9286 {
 		i2c-mux {
 			i2c@0 {
 				max9271@51 {
                                        i2c-mux {
                                                i2c@0 {
                                                        ov13858@61 {

                                                        }
                                                }
                                        }
 				}

 			}
 		}
 	}

Acceptable as a DT layout ??


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