Date: Fri, 2 Feb 2024 16:12:53 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konrad.dybcio@...aro.org>,
	Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Frank Rowand <frowand.list@...il.com>,
	Robert Marko <robert.marko@...tura.hr>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org
Subject: Re: [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x
 PHY package

On Fri, Feb 02, 2024 at 08:45:52AM +0100, Krzysztof Kozlowski wrote:
> On 01/02/2024 16:17, Christian Marangi wrote:
> > Document Qcom QCA807x PHY package.
> > 
> > Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > 1000BASE-T PHY-s.
> > 
> > Document the required property to make the PHY package correctly
> > configure and work.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> > ---
> >  .../devicetree/bindings/net/qcom,qca807x.yaml | 142 ++++++++++++++++++
> 
> Your bindings header must be squashed here. Headers are not separate
> thing from the bindings.
> 
> >  1 file changed, 142 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/net/qcom,qca807x.yaml b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > new file mode 100644
> > index 000000000000..1c3692897b02
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/qcom,qca807x.yaml
> > @@ -0,0 +1,142 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/qcom,qca807x.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm QCA807X Ethernet PHY
> 
> What is "X"? Wildcards are usually not expected.
>

It's to identify the Ethrnet PHY family. Looks wrong to declare qca8072
or qca8074 since they would refer to a more generic Family of devices.

What would be the correct way? We have many other case on net with
schema called qca8k that refer to the family of Ethernet Switch but in
it refer to qca8327 qca8337 qca8334...

> > +
> > +maintainers:
> > +  - Christian Marangi <ansuelsmth@...il.com>
> > +  - Robert Marko <robert.marko@...tura.hr>
> > +
> > +description: |
> > +  Qualcomm QCA807X Ethernet PHY is PHY package of 2 or 5
> > +  IEEE 802.3 clause 22 compliant 10BASE-Te, 100BASE-TX and
> > +  1000BASE-T PHY-s.
> > +
> > +  They feature 2 SerDes, one for PSGMII or QSGMII connection with
> > +  MAC, while second one is SGMII for connection to MAC or fiber.
> > +
> > +  Both models have a combo port that supports 1000BASE-X and
> > +  100BASE-FX fiber.
> > +
> > +  Each PHY inside of QCA807x series has 4 digitally controlled
> > +  output only pins that natively drive LED-s for up to 2 attached
> > +  LEDs. Some vendor also use these 4 output for GPIO usage without
> > +  attaching LEDs.
> > +
> > +  Note that output pins can be set to drive LEDs OR GPIO, mixed
> > +  definition are not accepted.
> > +
> > +  PHY package can be configured in 3 mode following this table:
> > +
> > +                First Serdes mode       Second Serdes mode
> > +  Option 1      PSGMII for copper       Disabled
> > +                ports 0-4
> > +  Option 2      PSGMII for copper       1000BASE-X / 100BASE-FX
> > +                ports 0-4
> > +  Option 3      QSGMII for copper       SGMII for
> > +                ports 0-3               copper port 4
> > +
> > +$ref: ethernet-phy-package.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,qca807x-package
> > +
> > +  qcom,package-mode:
> 
> Where is definition of this property with type and description?
> 
> > +    enum:
> > +      - qsgmii
> > +      - psgmii
> > +
> > +  qcom,tx-driver-strength:
> 
> Use proper unit suffix.
> 
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> 
> > +    description: set the TX Amplifier value in mv.
> > +      If not defined, 600mw is set by default.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [140, 160, 180, 200, 220,
> > +           240, 260, 280, 300, 320,
> > +           400, 500, 600]
> > +
> > +patternProperties:
> > +  ^ethernet-phy(@[a-f0-9]+)?$:
> > +    $ref: ethernet-phy.yaml#
> > +
> > +    properties:
> > +      gpio-controller:
> > +        description: set the output lines as GPIO instead of LEDs
> > +        type: boolean
> > +
> > +      '#gpio-cells':
> > +        description: number of GPIO cells for the PHY
> > +        const: 2
> > +
> > +    dependencies:
> > +      gpio-controller: ['#gpio-cells']
> 
> Why do you need it? None of gpio-controllers do it, I think.
> 

Well gpio-controller property is optional and having that declared and
#gpio-cells skipped will result in an error on probe. I think it should
be the opposite with other schema having to declare this.

In usual way for gpio-controller both property are defined as required
but you can see some (to-be-converted) txt files whith comments where
they say:

/mux/adi,adgs1408.txt:10:- gpio-controller : if present, #gpio-cells is required.

Should I instead add this condition in the if right below?

> > +
> > +    if:
> > +      required:
> > +        - gpio-controller
> > +    then:
> > +      properties:
> > +        leds: false
> > +
> > +    unevaluatedProperties: false
> > +
> > +required:
> > +  - compatible
> > +
> > +unevaluatedProperties: false
> 
> 
> Best regards,
> Krzysztof
> 

-- 
	Ansuel

Powered by blists - more mailing lists