Re: [net-next PATCH v5 6/9] dt-bindings: net: Document Qcom QCA807x PHY package

[Rob Herring]

On Fri, Feb 02, 2024 at 04:12:53PM +0100, Christian Marangi wrote:
> 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@xxxxxxxxx>
> > > ---
> > >  .../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.

Declare them all or provide some justification such as the exact model 
is discoverable (and better be sure power on is the same in order to do 
discovery).

> 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@xxxxxxxxx>
> > > +  - Robert Marko <robert.marko@xxxxxxxxxx>
> > > +
> > > +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

You only need 'gpio-controller: true'. The core already defines the 
type.

> > > +
> > > +      '#gpio-cells':
> > > +        description: number of GPIO cells for the PHY

Not a useful description. Normally, you'd describe what's in the cells, 
but GPIO is standardized so no need (unless you are deviating from the 
norm).

> > > +        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.

If you think everything else is wrong, then please fix them. :)

> 
> 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?

The core schema enforces this dependency, so you don't need to.

Rob