| Message ID | 20210817072703.1167-1-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: i2c: Add MAX9271 subdevice driver | expand |
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 >
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 ? > > 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%)
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%) > >
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
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