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
