Re: [PATCH v2 1/4] dt-bindings: media: add Maxim MAX96714F GMSL2 Deserializer

[Krzysztof Kozlowski <krzk@kernel.org>]

On 08/12/2023 15:33, Julien Massot wrote:
> Add DT bindings for Maxim MAX96714F GMSL2 Deserializer.
> 
> Signed-off-by: Julien Massot <julien.massot@collabora.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

Limited review follows, as robot already pointed out issues...

> ---
>  .../bindings/media/i2c/maxim,max96714f.yaml   | 163 ++++++++++++++++++
>  1 file changed, 163 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/maxim,max96714f.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96714f.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96714f.yaml
> new file mode 100644
> index 000000000000..8a2a06e7e279
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96714f.yaml
> @@ -0,0 +1,163 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2023 Collabora Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/maxim,max96714f.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GMSL2 to CSI-2 Deserializer
> +
> +maintainers:
> +  - Julien Massot <julien.massot@collabora.com>
> +
> +description: |
> +  The MAX96714F deserializer converts GMSL2 serial inputs into MIPI
> +  CSI-2 D-PHY or C-PHY formatted output. The device allows the GMSL2 link to
> +  simultaneously transmit bidirectional control-channel data while forward
> +  video transmissions are in progress. The MAX96714F can connect to one
> +  remotely located serializer using industry-standard coax or STP
> +  interconnects. The device cans operate in pixel or tunnel mode. In pixel mode
> +  the MAX96714F can select individual video stream, while the tunnel mode forward all
> +  the MIPI data received by the serializer.
> +
> +  The GMSL2 serial link operates at a fixed rate of 3Gbps in the
> +  forward direction and 187.5Mbps in the reverse direction.
> +
> +properties:
> +  compatible:
> +    const: maxim,max96714f
> +
> +  reg:
> +    description: I2C device address

Drop

> +    maxItems: 1
> +
> +  enable-gpios: true
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        unevaluatedProperties: false
> +        description: GMSL Input
> +        properties:
> +          endpoint:
> +            $ref: /schemas/media/video-interfaces.yaml#
> +            unevaluatedProperties: false
> +            description:
> +              Endpoint for GMSL2-Link port.
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description: CSI-2 Output
> +
> +        properties:
> +          endpoint:
> +            $ref: /schemas/media/video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              data-lanes: true
> +              bus-type:
> +                enum:
> +                  - 4 # MEDIA_BUS_TYPE_CSI2_DPHY
> +
> +            required:
> +              - data-lanes
> +              - bus-type
> +
> +    required:
> +      - port@1
> +
> +  i2c-gate:
> +    $ref: /schemas/i2c/i2c-controller.yaml
> +    unevaluatedProperties: false
> +    description: |
> +      The MAX96714 will pass through and forward the I2C requests from the
> +      incoming I2C bus over the GMSL2 link. Therefore it supports an i2c-gate
> +      subnode to configure a serializer.
> +
> +  port0-poc-supply:
> +    description: Regulator providing Power over Coax for a particular port
> +
> +required:
> +  - compatible
> +  - reg
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/media/video-interfaces.h>
> +
> +    main_i2c2 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +        #address-cells = <1>;
> +        #size-cells = <0>;

> +            i2c-gate {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                gmsl-serializer@40 {
> +                    compatible = "maxim,max96717f";
> +                    reg = <0x40>;

Not documented.

> +...

Best regards,
Krzysztof