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

[Krzysztof Kozlowski <krzk@kernel.org>]

On Tue, Sep 15, 2026 at 10:20:24PM +0200, Richard Leitner wrote:
> Add bindings for the Vision Components MIPI Camera Module Controller.
> 
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  .../vision-components,mipi-module-controller.yaml  | 92 ++++++++++++++++++++++
>  MAINTAINERS                                        |  7 ++
>  2 files changed, 99 insertions(+)

This fails tests, so a very brief review / a few comments:

> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/vision-components,mipi-module-controller.yaml b/Documentation/devicetree/bindings/media/i2c/vision-components,mipi-module-controller.yaml
> new file mode 100644
> index 0000000000000..2a031aea68457
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/vision-components,mipi-module-controller.yaml
> @@ -0,0 +1,92 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/vision-components,mipi-module-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Vision Components MIPI Camera Module Controller
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +  - Richard Leitner <richard.leitner@linux.dev>
> +
> +description: |-
> +  The MIPI camera module controller is an FPGA-based, I2C-accessible system
> +  controller found on MIPI camera modules from Vision Components. It abstracts
> +  the camera sensor behind a unified register interface, and controls and
> +  sequences the on-board power supplies and clocks.
> +
> +  The camera sensor abstraction is optional. The controller exposes a tunneled
> +  downstream I2C bus used by the attached image sensor. The controller node
> +  acts as the upstream device on the host bus, while child node below the
> +  controller represent the sensor device reachable through the tunnel.
> +
> +properties:
> +  compatible:
> +    const: vision-components,mipi-module-controller

There is no model name, no version, nothing identifying it better?
Compatible must be specific to the device (see also writing bindings).

> +
> +  reg:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 0
> +
> +  clock-frequency:
> +    description: Frequency of the sensor clock provided by the module

Drop, implied by the compatible

> +
> +  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?

> +
> +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?

Best regards,
Krzysztof