Re: [PATCH v2 1/6] dt-bindings: phy: qcom,uniphy-pcie: Document PCIe uniphy

[Krzysztof Kozlowski]

On Wed, Dec 04, 2024 at 05:03:24PM +0530, Varadarajan Narayanan wrote:
> From: Nitheesh Sekar <quic_nsekar@xxxxxxxxxxx>
> 
> Document the Qualcomm UNIPHY PCIe 28LP present in IPQ5332.
> 
> Signed-off-by: Nitheesh Sekar <quic_nsekar@xxxxxxxxxxx>
> Signed-off-by: Varadarajan Narayanan <quic_varada@xxxxxxxxxxx>
> ---
> v2: Rename the file to match the compatible

Either I look at wrong v1 from your cover letter or there was no such
file in v1, so how it can be a rename?

What happened here?


>     Drop 'driver' from title
>     Dropped 'clock-names'
>     Fixed 'reset-names'
> --
>  .../bindings/phy/qcom,uniphy-pcie.yaml        | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml
> new file mode 100644
> index 000000000000..e0ad98a9f324
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,uniphy-pcie.yaml

This does not match compatible, so I don't see how it even matches your
changelog.

> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/qcom,uniphy-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm UNIPHY PCIe 28LP PHY
> +
> +maintainers:
> +  - Nitheesh Sekar <quic_nsekar@xxxxxxxxxxx>
> +  - Varadarajan Narayanan <quic_varada@xxxxxxxxxxx>
> +
> +description:
> +  PCIe and USB combo PHY found in Qualcomm IPQ5332 SoC
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq5332-uniphy-pcie-gen3x1

Odd naming. Did anyone suggest this? I would expect something matches
like everything else recent (see X1 for example).


> +      - qcom,ipq5332-uniphy-pcie-gen3x2
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 2

What happened here? This cannot be minItems and it never was.

> +
> +  resets:
> +    minItems: 2
> +    maxItems: 3

Why this varies?

This patch is odd. Confusing changelog, v1 entirely different and not
matching what is here, unusual and incorrect code in the binding itself.

Provide changelog explaining WHY you did such odd changes.

Open *LATEST* existing Qcom bindings and look how they do it. Do not
implement things differently.

Best regards,
Krzysztof