Date:   Mon, 2 Aug 2021 13:26:32 -0600
From:   Rob Herring <robh@kernel.org>
To:     Matt Johnston <matt@codeconstruct.com.au>
Cc:     devicetree@vger.kernel.org, Wolfram Sang <wsa@kernel.org>,
        Jeremy Kerr <jk@codeconstruct.com.au>
Subject: Re: [RFC PATCH 2/2] dt-bindings: net: Add mctp-i2c bus-attach
 property
Message-ID: <YQhG6N421d7+Y0GO@robh.at.kernel.org>
References: <20210802040458.334732-1-matt@codeconstruct.com.au>
 <20210802040458.334732-3-matt@codeconstruct.com.au>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210802040458.334732-3-matt@codeconstruct.com.au>
Precedence: bulk

On Mon, Aug 02, 2021 at 12:04:58PM +0800, Matt Johnston wrote:
> Allows attaching multiple child busses in a mux topology
> to an mctp-i2c instance on the root bus. In general I2C
> slave mode does not make sense for mux busses, but the MCTP
> request/response protocol means the the root can switch
> between child muxes for incoming I2C messages.

Perhaps a diagram of what I2C buses look like would help because I don't 
understand this.

> 
> Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
> ---
>  .../devicetree/bindings/net/mctp-i2c.yaml     | 42 +++++++++++++++++--
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/mctp-i2c.yaml b/Documentation/devicetree/bindings/net/mctp-i2c.yaml
> index f9378cd845d4..45429cbcc6a1 100644
> --- a/Documentation/devicetree/bindings/net/mctp-i2c.yaml
> +++ b/Documentation/devicetree/bindings/net/mctp-i2c.yaml
> @@ -12,11 +12,10 @@ maintainers:
>  description:
>    The MCTP I2C binding defines an MCTP endpoint on the I2C bus to
>    communicate with I2C peripherals using MCTP (DMTF specification DSP0237).
> -
> -  An mctp-i2c device must be attached to a hardware bus adapter which supports
> +  A single binding node can attach to multiple child busses in a mux topology.
> +  An mctp-i2c node's parent must be a hardware bus adapter which supports
>    slave functionality. The reg address must include I2C_OWN_SLAVE_ADDRESS.
>  
> -
>  properties:
>    compatible:
>      const: mctp-i2c
> @@ -24,6 +23,17 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  bus-attach:
> +    $ref: "/schemas/types.yaml#/definitions/phandle-array"
> +    description: |
> +      List of phandles of I2C busses to attach to. I2C mux busses may only
> +      be attached to an mctp-i2c binding on their parent root adapter in the
> +      mux topology.
> +      If no bus-attach property is specified then only the direct parent
> +      I2C bus is attached. Otherwise to include a direct parent bus it
> +      must be included in the bus-attach list if needed.
> +
> +
>  additionalProperties: true
>  
>  required:
> @@ -33,12 +43,36 @@ required:
>  examples:
>    - |
>      #include <dt-bindings/i2c/i2c.h>
> -    i2c {
> +
> +    // simple attaching to a root adapter i2c0
> +    i2c0: i2cbus0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        mctp@50 {
> +            compatible = "mctp-i2c";
> +            reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
> +        };
> +    };
> +
> +    // attaching to a root adapter i2c5 and a child mux bus i2c14
> +    i2c5: i2cbus5 {
>          #address-cells = <1>;
>          #size-cells = <0>;
>  
>          mctp@50 {
>              compatible = "mctp-i2c";
>              reg = <(0x50 | I2C_OWN_SLAVE_ADDRESS)>;
> +            attach-bus = <&i2c5 &i2c14>;
>          };
>      };
> +
> +    i2cmux0 {
> +      compatible = "i2c-mux-gpio";
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      i2c-parent = <&i2c0>;
> +      i2c14: i2c@5 {

This is not how "i2c-mux-gpio" works. First you are missing mux-gpios. 
Second, how is this a mux with only 1 mux selection as the child nodes 
are the I2C buses for each mux selection.

> +        reg = <0>;

Either reg should be 5 here or the unit-address should be 0.

> +      };
> +    };
> -- 
> 2.30.2
> 
>