Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 27 Jan 2020 10:42:35 -0600
From:   Rob Herring <robh@kernel.org>
To:     Yuti Amonkar <yamonkar@cadence.com>
Cc:     linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        kishon@ti.com, mark.rutland@arm.com, maxime@cerno.tech,
        jsarha@ti.com, tomi.valkeinen@ti.com, praneeth@ti.com,
        mparab@cadence.com, sjakhade@cadence.com
Subject: Re: [PATCH v3 13/14] dt-bindings: phy: phy-cadence-torrent: Add
 subnode bindings.
Message-ID: <20200127164235.GA7662@bogus>
References: <1579689918-7181-1-git-send-email-yamonkar@cadence.com>
 <1579689918-7181-14-git-send-email-yamonkar@cadence.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1579689918-7181-14-git-send-email-yamonkar@cadence.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jan 22, 2020 at 11:45:17AM +0100, Yuti Amonkar wrote:
> From: Swapnil Jakhade <sjakhade@cadence.com>
> 
> Add sub-node bindings for each group of PHY lanes based on PHY
> type. Only PHY_TYPE_DP is supported currently. Each sub-node

What the driver supports is not relevant to the binding. Define all 
modes.

> includes properties such as master lane number, link reset, phy
> type, number of lanes etc.

Given the conversion and this have no compatibility, just make the 
commits delete the old binding and add this new binding. I'd rather not 
have reviewed what just gets deleted here.

> 
> Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> ---
>  .../bindings/phy/phy-cadence-torrent.yaml          | 90 ++++++++++++++++++----
>  1 file changed, 73 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml b/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml
> index dbb8aa5..eb21615 100644
> --- a/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml
> +++ b/Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml
> @@ -19,6 +19,12 @@ properties:
>        - cdns,torrent-phy
>        - ti,j721e-serdes-10g
>  
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
>    clocks:
>      maxItems: 1
>      description:
> @@ -41,44 +47,94 @@ properties:
>        - const: torrent_phy
>        - const: dptx_phy
>  
> -  "#phy-cells":
> -    const: 0
> +  resets:
> +    description:
> +      Must contain an entry for each in reset-names.
> +      See Documentation/devicetree/bindings/reset/reset.txt

How many reset entries? Needs a 'maxItems: 1' or an 'items' list if more 
than 1.

>  
> -  num_lanes:
> +  reset-names:
>      description:
> -      Number of DisplayPort lanes.
> -    allOf:
> -      - $ref: /schemas/types.yaml#/definitions/uint32
> -      - enum: [1, 2, 4]
> +      Must be "torrent_reset". It controls the reset to the

Should be a schema, not freeform text. However, not really a useful name 
as there's only 1, so I'd just remove 'reset-names'.

> +      torrent PHY.
>  
> -  max_bit_rate:
> +patternProperties:
> +  '^torrent-phy@[0-7]+$':
> +    type: object
>      description:
> -      Maximum DisplayPort link bit rate to use, in Mbps
> -    allOf:
> -      - $ref: /schemas/types.yaml#/definitions/uint32
> -      - enum: [2160, 2430, 2700, 3240, 4320, 5400, 8100]
> +      Each group of PHY lanes with a single master lane should be represented as a sub-node.
> +    properties:
> +      reg:
> +        description:
> +          The master lane number. This is the lowest numbered lane in the lane group.

Why not make it the list of lane numbers. Then you don't need num-lanes.

> +
> +      resets:
> +        description:
> +          Contains list of resets to get all the link lanes out of reset.

Needs a schema for how many? 1 per lane?

> +
> +      "#phy-cells":
> +        description:
> +          Generic PHY binding.

Not a useful description. Remove.

> +        const: 0
> +
> +      cdns,phy-type:
> +        description:
> +          Should be PHY_TYPE_DP.

Sounds like a constraint.

> +        $ref: /schemas/types.yaml#/definitions/uint32
> +
> +      cdns,num-lanes:
> +        description:
> +          Number of DisplayPort lanes.
> +        allOf:
> +          - $ref: /schemas/types.yaml#/definitions/uint32
> +          - enum: [1, 2, 4]
> +
> +      cdns,max-bit-rate:
> +        description:
> +          Maximum DisplayPort link bit rate to use, in Mbps
> +        allOf:
> +          - $ref: /schemas/types.yaml#/definitions/uint32
> +          - enum: [2160, 2430, 2700, 3240, 4320, 5400, 8100]
> +
> +    required:
> +      - reg
> +      - resets
> +      - "#phy-cells"
> +      - cdns,phy-type

Add (for the child nodes):

       addtionalProperties: false

>  
>  required:
>    - compatible
> +  - "#address-cells"
> +  - "#size-cells"
>    - clocks
>    - clock-names
>    - reg
>    - reg-names
> -  - "#phy-cells"
> +  - resets
> +  - reset-names
>  
>  additionalProperties: false
>  
>  examples:
>    - |
> -    dp_phy: phy@f0fb500000 {
> +    #include <dt-bindings/phy/phy.h>
> +    torrent_phy: phy@f0fb500000 {
>            compatible = "cdns,torrent-phy";
>            reg = <0xf0 0xfb500000 0x0 0x00100000>,
>                  <0xf0 0xfb030a00 0x0 0x00000040>;
>            reg-names = "torrent_phy", "dptx_phy";
> -          num_lanes = <4>;
> -          max_bit_rate = <8100>;
> -          #phy-cells = <0>;
> +          resets = <&phyrst 0>;
> +          reset-names = "torrent_reset";
>            clocks = <&ref_clk>;
>            clock-names = "refclk";
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +          torrent_phy_dp: torrent-phy@0 {

Just 'phy@...'

> +                    reg = <0>;
> +                    resets = <&phyrst 1>;
> +                    #phy-cells = <0>;
> +                    cdns,phy-type = <PHY_TYPE_DP>;
> +                    cdns,num-lanes = <4>;
> +                    cdns,max-bit-rate = <8100>;
> +          };
>      };
>  ...
> -- 
> 2.4.5
>