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

[Christian Marangi <ansuelsmth@gmail.com>]

On Fri, Feb 02, 2024 at 02:39:15PM -0600, Rob Herring wrote:
> 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@gmail.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.
> 
> 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@gmail.com>
> > > > +  - Robert Marko <robert.marko@sartura.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
> 
> 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.
>

Oh! No idea the dependency was already enforced, guess I don't have to
fix everything ahahahha

-- 
	Ansuel