Hi Jacopo, 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{ } > } > } > } > } 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%) > > -- > 2.32.0 >
