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

[Richard Leitner <richard.leitner@linux.dev>]

On Thu, Sep 17, 2026 at 08:18:23AM +0200, Krzysztof Kozlowski wrote:
> On 16/09/2026 18:52, Laurent Pinchart wrote:
> > On Wed, Sep 16, 2026 at 03:41:16PM +0200, Krzysztof Kozlowski wrote:
> >> On 16/09/2026 11:01, Richard Leitner wrote:
> >>> On Wed, Sep 16, 2026 at 10:35:22AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 16/09/2026 09:33, Richard Leitner wrote:
> >>>>> On Wed, Sep 16, 2026 at 09:00:46AM +0200, Krzysztof Kozlowski wrote:
> >>>>>> On Tue, Sep 15, 2026 at 10:20:24PM +0200, Richard Leitner wrote:

[...]

> >>>>>>> Add bindings for the Vision Components MIPI Camera Module Controller.
> >>>>>>> +
> >>>>>>> +  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.
> > 
> > To be clear, the sensor *is* addressable from the OS. The FPGA sits in
> > the middle on the I2C bus instead of being a separate I2C device on the
> > same bus as the sensor, but it forwards all I2C writes addressed to the
> > sensor onto the backend I2C bus. This is why the DT binding is modelled
> > with a child I2C bus. The sensor DT node is located on that node, and
> > uses the regular DT bindings for the sensor.
> 
> Thanks, this should be explained in the description of the binding. It
> does mention "the sensor device reachable through the tunnel" but the
> rest about optionality of the sensor feels confusing in this case.

Thanks Laurent for the additional clarification!

I will adapt the description accordingly for v2.

> 
> > 
> >>> 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.
> > 
> > The FPGA has an input clock whose frequency depends on the module. It
> 
> There is no "clocks" property in the binding. If there is input, then
> there is a "clocks".
> 
> Then we never describe external oscillators with "clock-frequency"
> property. ACPI does, but not DT.

So how would that be best solved in DT?

Should there be a "fixed-clock" child node for that external oscillator?
Something like:

vc_mipi_ctrl: controller@10 {
  ...

  vc_mipi_clk: clock {
    compatible = "fixed-clock";
    #clock-cells = <0>;
    clock-frequency = <37125000>;
  };

  i2c-tunnel {
    ...

    vc_mipi_sensor: camera@60 {
      ...

      clocks = <&vc_mipi_clk>;
      clock-names = "inck";
    };
};

Or should (as discussed in the other branch of this thread) there be a
separate compatible for every vc-mipi sensor variant board? Then this
could also be part of the compatible-dependent driver data.

Or are there any other/better approaches to this?

> 
> 
> > uses it for internal purpose, and also to provide a clock to the sensor.
> > Very roughly speaking, and ignoring power supplies as we focus on the
> > clocks, the camera module is architectured this way:
> > 
> > Connector
> >    ||                                       +--------+
> >    || <------------ MIPI CSI-2 ------------ |        |
> >    ||               +-------+               |        |
> >    || <--- I2C ---> |       | <--- I2C ---> | Sensor |
> >    ||               | FPGA  |               |        |
> >    ||               |       | --- Clock --> |        |
> >    ||               +-------+               +--------+
> >                         ^
> >                         |
> >                     +-------+
> >                     | Clock |
> >                     | Osc.  |
> >                     +-------+
> > 
> > The frequency of the external clock oscillator is what the
> > clock-frequency models.
> > 
> >>>>>>> +
> >>>>>>> +required:
> >>>>>>> +  - compatible
> >>>>>>> +  - reg
> >>>>>>> +  - '#clock-cells'
> >>>>>>> +  - clock-frequency
> >>>>>>> +  - vcc-supply
> >>>>>>> +  - '#address-cells'
> >>>>>>> +  - '#size-cells'
> >>>>>>> +
> >>>>>>> +unevaluatedProperties: false
> >>>>>>
> >>>>>> additionalProperties instead, see writing bindings or writing schema.
> >>>>>> Unless you miss here some other schema $ref.
> >>>>>>
> >>>>>>> +
> >>>>>>> +examples:
> >>>>>>> +  - |
> >>>>>>> +    i2c {
> >>>>>>> +        #address-cells = <1>;
> >>>>>>> +        #size-cells = <0>;
> >>>>>>> +
> >>>>>>> +        vc_mipi_ctrl: controller@10 {
> >>>>>>> +            compatible = "vision-components,mipi-module-controller";
> >>>>>>> +            reg = <0x10>;
> >>>>>>> +            #clock-cells = <0>;
> >>>>>>> +            clock-frequency = <37125000>;
> >>>>>>> +            vcc-supply = <&cam_3v3>;
> >>>>>>> +
> >>>>>>> +            #address-cells = <1>;
> >>>>>>> +            #size-cells = <0>;
> >>>>>>> +
> >>>>>>> +            i2c@0 {
> >>>>>>> +                #address-cells = <1>;
> >>>>>>> +                #size-cells = <0>;
> >>>>>>> +
> >>>>>>> +                vc_mipi_sensor: camera@60 {
> >>>>>>> +                    compatible = "ovti,ov9281";
> >>>>>>> +                    reg = <0x60>;
> >>>>>>
> >>>>>> Why having the child abstraction if it is completely abstracted? I don't
> >>>>>> fully get the explanation from description. Completely optional means no
> >>>>>> benefits, no point in it, no?
> >>>>>
> >>>>> I will try to improve the description. The idea behind this device
> >>>>> driver/devicetree node is to not rely on the sensor abstraction from
> >>>>> vision components, but to use the upstream sensor specific driver.
> >>>>
> >>>> You can use driver even without these nodes... but fine, let's assume
> >>>> you have them, so driver will talk with OV9281 sensor for example?
> >>>
> >>> The vc-mipi driver does not talk to sensor at all. This is done by the
> >>> dedicated sensor driver. The vc-mipi driver is only controlling the
> >>> regulator, clock, etc. as described and sets up the "quirk aware"
> >>> tunneled i2c interface.
> >>
> >> I meant, driver for the sensor. So who controls sensor supplies? Not the
> >> sensor driver? It tells something how the hardware is managed, no?
> > 
> > Power supplies are managed in a similar way as the clock in the above
> > diagram. There's one input supply to the camera module on the connector.
> > Individual supplies for the sensor are generated from that main supply,
> > and are controlled by the FPGA which exposes a register over I2C to
> > enable/disable the supplies from the host (there's a single bit to
> > enable/disable them all, they can't be controlled individually).
> > 
> 
> Thanks, I see this also shown in the example DTS. I think the sensor
> node is redundant here, but removing it would require moving all of this
> to swnodes, so not that much better either. It's fine to keep the sensor
> subnode for me.

ACK.

Thanks Krzysztof!

regards;rl

> 
> Best regards,
> Krzysztof