Re: [PATCH net-next] dt-bindings: net: convert sff,sfp to dtschema

[Ioana Ciornei <ioana.ciornei@nxp.com>]

On Tue, Mar 15, 2022 at 07:21:59PM +0100, Krzysztof Kozlowski wrote:
> On 15/03/2022 13:33, Ioana Ciornei wrote:
> > Convert the sff,sfp.txt bindings to the DT schema format.
> > 
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---

(..)

> > +maintainers:
> > +  - Russell King <linux@armlinux.org.uk>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - sff,sfp  # for SFP modules
> > +      - sff,sff  # for soldered down SFF modules
> > +
> > +  i2c-bus:
> 
> Thanks for the conversion.
> 
> You need here a type because this does not look like standard property.

Ok.

> 
> > +    description:
> > +      phandle of an I2C bus controller for the SFP two wire serial
> > +
> > +  maximum-power-milliwatt:
> > +    maxItems: 1
> > +    description:
> > +      Maximum module power consumption Specifies the maximum power consumption
> > +      allowable by a module in the slot, in milli-Watts. Presently, modules can
> > +      be up to 1W, 1.5W or 2W.
> > +
> > +patternProperties:
> > +  "mod-def0-gpio(s)?":
> 
> This should be just "mod-def0-gpios", no need for pattern. The same in
> all other places.
> 

The GPIO subsystem accepts both suffixes: "gpio" and "gpios", see
gpio_suffixes[]. If I just use "mod-def0-gpios" multiple DT files will
fail the check because they are using the "gpio" suffix.

Why isn't this pattern acceptable?

> > +
> > +  "rate-select1-gpio(s)?":
> > +    maxItems: 1
> > +    description:
> > +      GPIO phandle and a specifier of the Tx Signaling Rate Select (AKA RS1)
> > +      output gpio signal (SFP+ only), low - low Tx rate, high - high Tx rate. Must
> > +      not be present for SFF modules
> 
> This and other cases should have a "allOf: if: ...." with a
> "rate-select1-gpios: false", to disallow this property on SFF modules.
> 

Ok, didn't know that's possible.

> > +
> > +required:
> > +  - compatible
> > +  - i2c-bus
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - | # Direct serdes to SFP connection
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    sfp_eth3: sfp-eth3 {
> 
> Generic node name please, so maybe "transceiver"? or just "sfp"?
> 

Ok, I can do just "sfp".

> > +      compatible = "sff,sfp";
> > +      i2c-bus = <&sfp_1g_i2c>;
> > +      los-gpios = <&cpm_gpio2 22 GPIO_ACTIVE_HIGH>;
> > +      mod-def0-gpios = <&cpm_gpio2 21 GPIO_ACTIVE_LOW>;
> > +      maximum-power-milliwatt = <1000>;
> > +      pinctrl-names = "default";
> > +      pinctrl-0 = <&cpm_sfp_1g_pins &cps_sfp_1g_pins>;
> > +      tx-disable-gpios = <&cps_gpio1 24 GPIO_ACTIVE_HIGH>;
> > +      tx-fault-gpios = <&cpm_gpio2 19 GPIO_ACTIVE_HIGH>;
> > +    };
> > +
> > +    cps_emac3 {
> 
> This is not related, so please remove.

It's related since it shows a generic usage pattern of the sfp node.
I wouldn't just remove it since it's just adds context to the example
not doing any harm.

> 
> > +      phy-names = "comphy";
> > +      phys = <&cps_comphy5 0>;
> > +      sfp = <&sfp_eth3>;
> > +    };
> > +
> > +  - | # Serdes to PHY to SFP connection
> > +    #include <dt-bindings/gpio/gpio.h>
> 
> Are you sure it works fine? Double define?

You mean that I added a second example? I don't understand the question.

And yes, I checked that the dtschema is ok and also that the DT files
are matching it correctly.
.
> 
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    sfp_eth0: sfp-eth0 {
> 
> Same node name - generic.

Ok.

> 
> > +      compatible = "sff,sfp";
> > +      i2c-bus = <&sfpp0_i2c>;
> > +      los-gpios = <&cps_gpio1 28 GPIO_ACTIVE_HIGH>;
> > +      mod-def0-gpios = <&cps_gpio1 27 GPIO_ACTIVE_LOW>;
> > +      pinctrl-names = "default";
> > +      pinctrl-0 = <&cps_sfpp0_pins>;
> > +      tx-disable-gpios = <&cps_gpio1 29 GPIO_ACTIVE_HIGH>;
> > +      tx-fault-gpios  = <&cps_gpio1 26 GPIO_ACTIVE_HIGH>;
> > +    };
> > +
> > +    mdio {
> 
> Not related.

See above answer. Russell adding these examples in the original txt file
I suppose just wanted to show how things work together.
Why remove it?

> 
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      p0_phy: ethernet-phy@0 {
> > +        compatible = "ethernet-phy-ieee802.3-c45";
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&cpm_phy0_pins &cps_phy0_pins>;
> > +        reg = <0>;
> > +        interrupt = <&cpm_gpio2 18 IRQ_TYPE_EDGE_FALLING>;
> > +        sfp = <&sfp_eth0>;
> > +      };
> > +    };
> > +
> > +    cpm_eth0 {
> 
> Also not related.
> 
> > +      phy = <&p0_phy>;
> > +      phy-mode = "10gbase-kr";
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1397a6b039fb..6da4872b4efb 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17498,6 +17498,7 @@ SFF/SFP/SFP+ MODULE SUPPORT
> >  M:	Russell King <linux@armlinux.org.uk>
> >  L:	netdev@vger.kernel.org
> >  S:	Maintained
> > +F:	Documentation/devicetree/bindings/net/sff,sfp.yaml
> 
> Mention this in the commit msg, please.

Ok, sure.

Ioana