Re: [PATCH 2/3] dt-bindings: media: i2c: Add vision-components,mipi-module-controller

[Laurent Pinchart]

On Thu, Sep 17, 2026 at 08:30:05AM +0200, Krzysztof Kozlowski wrote:
> On 16/09/2026 16:57, Richard Leitner wrote:
> >>>>>>> +  vcc-supply:
> >>>>>>> +    description: Power supply of the module (3.3V)
> >>>>>>> +
> >>>>>>> +  '#address-cells':
> >>>>>>> +    const: 1
> >>>>>>> +
> >>>>>>> +  '#size-cells':
> >>>>>>> +    const: 0
> >>>>>>
> >>>>>> No children allowed, so why these two?
> >>>>>
> >>>>> Based on your and the bot feedback I would suggest for v2 to change this
> >>>>> "generic i2c bus" to a simple "i2c-tunnel" property which has
> >>>>> "$ref: /schemas/i2c/i2c-controller.yaml".
> >>>>>
> >>>>> This would better reflect the actual hardware, as there is only this one,
> >>>>> in firmware hard-coded I2C downstream bus.
> >>>>>
> >>>>> Would this be a sane approach?
> >>>>
> >>>> If the underlying I2C bus and sensor are important, then yes. But I have
> >>>> doubts that you need to describe the sensor if it is truly
> >>>> unadressable/invisible to the OS.
> >>>
> >>> Yes, the I2C bus and sensor is important. The device driver of the sensor
> >>> talks (via the tunneled I2C interface) directly to the sensor.
> >>>
> >>> The separate I2C controller/bus description is necessary as the tunneled
> >>> I2C bus has some quirks unfortunately. Those need to be addressed as 
> >>> otherwise the sensor drivers do not work.
> >>>
> >>> So the idea is to not have a vc-mipi module binding per "sensor variant"
> >>> of the camera modules, but provide a common controller driver which
> >>> provides the I2C bus for the sensor driver.
> >>
> >> Bindings must accurately describe the device and so far - based on the
> >> website - there is no device as mipi-module-controller alone.
> >>
> >> I don't get why you assume that all of the variants are exactly
> >> identical, thus sensor variant is not applicable.
> >>
> >> If they are identical in all aspects, then why clock-frequency property?
> >> That's obviously rhetorical question, because they are not identical in
> >> all aspects and must produce different clock at least.
> > 
> > From that point of view, of course all variants are different.
> > But the interface towards the host is (according to vision components)
> > stable. This is why I aimed for a separate device.
> 
> What makes you think that interface is stable and the same? Is the
> stable aspect or its versioning documented anywhere in the docs?
> 
> I can easily imagine that FPGA synthesis actually differs on some
> sensors or they ship a new, updated version, thus at some point the
> interface might change or already changed between models.

Alright, let's deep that in the real life aspect of this.

There is no stability guarantee in the sense of a strict, publicly
documented commitment. What we have, instead, is direct and honest
discussions with the vendor, and a decade of real-life experience with
those modules. the result is both better and worse than I'd like. The
good part is that the interface is stable with no plan from the vendor
to change it, and we haven't noticed observable difference in behaviour
between modules for different sensors.

The bad news, on the other hand, is that bugs happen. Newly manufactured
versions of the same module sometimes ship with updated FPGA bitstreams
that fix bugs. Previous module can't be fixed as the FPGA bitstream is
not field-upgradable. Those updates don't affect the interface in the
sense that the registers exposed by the FPGA stay the same, but they
require different software workarounds. A sensor-specific compatible
string wouldn't help here.

So far (again over the course of about a decade) the only two bugs I've
observed are related to I2C access. One of the bugs require access to
sensor registers to be broken into single-byte read/write operations
(this is the job of the controller's driver that register an I2C
"tunnel" adapter). The other bug causes the FPGA to shift data by one
byte when the host reads the controller's EEPROM data. The driver posted
by Richard works around the bugs unconditionally. It could be nice to
disable the workaround that split register access into bytes as that can
improve performance, and the plan would be to add a DT property to
indicate if the module is affected as that can't be inferred from the
compatible string (it depends on the revision of the FPGA bitstream as
explained above), nor can it be queried at runtime from the controller
(AFAIK).

> > So what's your suggestion on how to best solve this?
> > Provide a per sensor compatible like e.g. "vc-mipi-ov9281"?
> > 
> > Nonehteless this device must then provide a i2c sub node to place the
> > actual imaging sensor on.
> > 
> > If that's fine with you I'm personally fine with this approach too.
> 
> If you have certainty, like documented spec somewhere telling this, that
> programming interface of FPGA to the OS is the same, then go with a
> generic fallback compatible like you have and add front compatibles
> specific to the sensor.
> 
> If you do not have certainty, then I would go only with sensor-specific
> compatibles for both front and fallbacks, so:
> 
> vc-mipi-ov9281, vc-mipi-imx297
> 
> It does feel a bit odd, but that is usual way to handle any future
> differences when two vc-mipi modules turn out not to be compatible.

I'm not completely opposed to that, but my experience is that it will
create additional complexity (to update the DT binding when a module for
a new sensor is released) without much gain as bugs are not specific to
a module type.

-- 
Regards,

Laurent Pinchart