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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello,

On Wed, Aug 18, 2021 at 01:47:02PM +0100, Kieran Bingham wrote:
> On 17/08/2021 08:26, 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.
> > 
> > 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{
> > 				}
> > 			}
> > 		}
> > 	}
> 
> Is there any feasibility, or benefit to us modelling like:
> 
> > 	max9286 {
> >
> > 		i2c-mux {
> > 			i2c@0 {
> > 				rdacm20 {
> >	 				max9271{
> >					}
> >					ov13858{
> 					}
> > 				}

I don't see the point to be honest. If we have an rdacm20 device and a
corresponding driver, it's pointless to have the max9271 and ov13858
devices in DT. All the information needed would already be known to the
rdacm20 driver. A "detailed" DT description would only make sense if we
modelled the max9271 and ov13858, but not the rdacm20. In that case, I'd
make the ov13858 a child of the max9271.

I had misread this when replying to the cover letter btw, and thought
the rdacm20 node beside the max9271 was the ov13858.

> > 			}
> > 		}
> > 	}
> 
> Perhaps that doesn't actually give much benefit, but I feel like the
> max9271 is a part of the rdacm20, so it should be contained within it,
> not besides it ..
> 
> > 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
> >     }
> > 
> 
> If we're still likely to have an RDACM20 container 'device' - I wonder
> if it's possible that it could be responsible for making sure both of
> it's subdevices (the max9271, and the sensor) are handled in the correct
> sequence...
> 
> > 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