Re: [PATCH v3 1/7] dt-bindings: connector: add GE SUNH hotplug addon connector

[Rob Herring <robh@kernel.org>]

On Fri, Aug 09, 2024 at 05:34:49PM +0200, Luca Ceresoli wrote:
> Add bindings for the GE SUNH add-on connector. This is a physical,
> hot-pluggable connector that allows to attach and detach at runtime an
> add-on adding peripherals on non-discoverable busses.

Overall, looks pretty good.

> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Changed in v3:
>  - change the layout to only add subnodes, not properties
>  - add the 'nobus-devices' node description to hold devices not on any bus
>  - add 'i2c-*' nodes for the I2C busses, using a i2c-parent phandle
>  - and the 'dsi' node for the DSI bus
>  - move the entire port@1 node to the overlay (not only the remote-endpoint
>    property)
>  - remove the overlay examples (Overlays in examples are not supported)
>  - add more clarifying descriptions and comments for examples
>  - some rewording
> 
> This patch was added in v2.
> ---
>  .../connector/ge,sunh-addon-connector.yaml         | 185 +++++++++++++++++++++
>  MAINTAINERS                                        |   5 +
>  2 files changed, 190 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml b/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
> new file mode 100644
> index 000000000000..2a0b4e0fd089
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
> @@ -0,0 +1,185 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/connector/ge,sunh-addon-connector.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GE SUNH hotplug add-on connector
> +
> +maintainers:
> +  - Luca Ceresoli <luca.ceresoli@bootlin.com>
> +
> +description:

You need '|' or '>' to preserve line breaks. Elsewhere too.

> +  Represent the physical connector present on GE SUNH devices that allows
> +  to attach and detach at runtime an add-on adding peripherals on
> +  non-discoverable busses. Peripherals on the add-on include I2C sensors
> +  and a video bridge controlled via I2C.
> +
> +  The connector has status GPIOs to notify the connection status to the CPU
> +  and a reset GPIO to allow the CPU to reset all the peripherals on the
> +  add-on. It also has I2C busses and a 4-lane MIPI DSI bus.
> +
> +  Different add-on models can be connected, each having different
> +  peripherals. For this reason each add-on has a model ID stored in a
> +  non-volatile memory, which is accessed in the same way on all add-ons.
> +
> +  Add-on removal can happen at any moment under user control and without
> +  prior notice to the CPU, making all of its components not usable
> +  anymore. Later on, the same or a different add-on model can be connected.
> +
> +properties:
> +  compatible:
> +    const: ge,sunh-addon-connector
> +
> +  reset-gpios:
> +    description: An output GPIO to reset the peripherals on the add-on.

Active state is?

> +    maxItems: 1
> +
> +  plugged-gpios:
> +    description: An input GPIO that is asserted if and only if an add-on is
> +      physically connected.

Asserted is high or low?

> +    maxItems: 1
> +
> +  powergood-gpios:
> +    description: An input GPIO that is asserted if and only if power rails
> +      on the add-on are stable.

Active state is?

> +    maxItems: 1
> +
> +  nobus-devices:

Just 'devices'.

> +    description:
> +      A container for devices not accessible via any data bus. Common use
> +      cases include fixed and GPIO regulators, simple video panels and LED
> +      or GPIO backlight devices. When not hot-pluggable, nodes such devices
> +      are children of the root node.
> +
> +      This node should not be present in the connector description in the
> +      base device tree. It should be added by overlays along with a subnode
> +      per device.
> +
> +    type: object
> +    additionalProperties: false

The schema needs to work with the overlay applied too. 'true' is fine 
here.

> +
> +  dsi:
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      ports:
> +        $ref: /schemas/graph.yaml#/properties/ports
> +
> +        description:
> +          OF graph bindings modeling the MIPI DSI bus across the connector. The
> +          connector splits the video pipeline in a fixed part and a removable
> +          part.
> +
> +          The fixed part of the video pipeline includes all components up to
> +          the display controller and 0 or more bridges. The removable part
> +          includes any bridges and any other components up to the panel.
> +
> +        properties:
> +          port@0:
> +            $ref: /schemas/graph.yaml#/properties/port
> +            description:
> +              The last point of the non-removable part of the MIPI DSI bus
> +              line. The remote-endpoint sub-node must point to the last
> +              non-removable video component of the video pipeline.
> +
> +          port@1:
> +            $ref: /schemas/graph.yaml#/properties/port
> +            description:
> +              The first point of the removable part of the MIPI DSI bus
> +              line.  The remote-endpoint sub-node must point to the first
> +              video pipeline component on the add-on. As it describes the
> +              hot-pluggable hardware, the endpoint node cannot be filled
> +              until an add-on is detected, so this node needs to be added
> +              by a device tree overlay at runtime.
> +
> +        required:
> +          - port@0
> +          # port@1 is added by the overlay for any add-on using the DSI lines
> +
> +    required:
> +      - ports
> +
> +patternProperties:
> +  '^i2c-(dbat|gp|btp)$':
> +    description:
> +      An I2C bus that goes through the connector. The adapter (and possibly
> +      some clients) are on the fixed side. Add-ons that have any clients on
> +      this bus have to be added by the add-on overlay, inside this node.
> +
> +    $ref: /schemas/i2c/i2c-controller.yaml#
> +    unevaluatedProperties: false
> +    type: object
> +
> +    properties:
> +      i2c-parent:
> +        $ref: /schemas/types.yaml#/definitions/phandle
> +        description:
> +          Phandle pointing to the I2C bus controller on the fixed side that
> +          drives this bus
> +
> +required:
> +  - compatible
> +  - i2c-dbat
> +  - i2c-gp
> +  - i2c-btp
> +  - dsi
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  # This is the description of the connector as it should appear in the
> +  # main DTS describing the "main" board up to the connector. This is
> +  # supposed to be used together with the overlays in the two following
> +  # examples. The addon_connector and hotplug_conn_dsi_out labels are
> +  # referenced by the overlays in those examples.
> +  - |
> +    / {
> +        #include <dt-bindings/gpio/gpio.h>
> +
> +        addon_connector: addon-connector {
> +            compatible = "ge,sunh-addon-connector";
> +            reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
> +            plugged-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>;
> +            powergood-gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>;
> +
> +            i2c-dbat {
> +                i2c-parent = <&i2c5_ch2>;
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                // device subnodes to be added by overlays
> +            };
> +
> +            i2c-gp {
> +                i2c-parent = <&i2c4>;
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                // device subnodes to be added by overlays
> +            };
> +
> +            i2c-btp {
> +                i2c-parent = <&i2c3>;
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                // device subnodes to be added by overlays
> +            };
> +
> +            dsi {
> +                ports {
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +
> +                    port@0 {
> +                        reg = <0>;
> +
> +                        endpoint {
> +                            remote-endpoint = <&previous_bridge_out>;
> +                        };
> +                    };
> +
> +                    // port@1 to be added by overlay
> +                };
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 42decde38320..9e902db825d7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10254,6 +10254,11 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/pressure/honeywell,mprls0025pa.yaml
>  F:	drivers/iio/pressure/mprls0025pa*
>  
> +HOTPLUG CONNECTOR FOR GE SUNH ADDONS
> +M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/connector/ge,sunh-addon-connector.yaml
> +
>  HP BIOSCFG DRIVER
>  M:	Jorge Lopez <jorge.lopez2@hp.com>
>  L:	platform-driver-x86@vger.kernel.org
> 
> -- 
> 2.34.1
>