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

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

[Ioana Ciornei]

Convert the sff,sfp.txt bindings to the DT schema format.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 .../devicetree/bindings/net/sff,sfp.txt       |  85 ------------
 .../devicetree/bindings/net/sff,sfp.yaml      | 130 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 3 files changed, 131 insertions(+), 85 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/net/sff,sfp.txt
 create mode 100644 Documentation/devicetree/bindings/net/sff,sfp.yaml

diff --git a/Documentation/devicetree/bindings/net/sff,sfp.txt b/Documentation/devicetree/bindings/net/sff,sfp.txt
deleted file mode 100644
index 832139919f20..000000000000
--- a/Documentation/devicetree/bindings/net/sff,sfp.txt
+++ /dev/null
@@ -1,85 +0,0 @@
-Small Form Factor (SFF) Committee Small Form-factor Pluggable (SFP)
-Transceiver
-
-Required properties:
-
-- compatible : must be one of
-  "sff,sfp" for SFP modules
-  "sff,sff" for soldered down SFF modules
-
-- i2c-bus : phandle of an I2C bus controller for the SFP two wire serial
-  interface
-
-Optional Properties:
-
-- mod-def0-gpios : GPIO phandle and a specifier of the MOD-DEF0 (AKA Mod_ABS)
-  module presence input gpio signal, active (module absent) high. Must
-  not be present for SFF modules
-
-- los-gpios : GPIO phandle and a specifier of the Receiver Loss of Signal
-  Indication input gpio signal, active (signal lost) high
-
-- tx-fault-gpios : GPIO phandle and a specifier of the Module Transmitter
-  Fault input gpio signal, active (fault condition) high
-
-- tx-disable-gpios : GPIO phandle and a specifier of the Transmitter Disable
-  output gpio signal, active (Tx disable) high
-
-- rate-select0-gpios : GPIO phandle and a specifier of the Rx Signaling Rate
-  Select (AKA RS0) output gpio signal, low: low Rx rate, high: high Rx rate
-  Must not be present for SFF modules
-
-- rate-select1-gpios : 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
-
-- maximum-power-milliwatt : 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.
-
-Example #1: Direct serdes to SFP connection
-
-sfp_eth3: sfp-eth3 {
-	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 {
-	phy-names = "comphy";
-	phys = <&cps_comphy5 0>;
-	sfp = <&sfp_eth3>;
-};
-
-Example #2: Serdes to PHY to SFP connection
-
-sfp_eth0: sfp-eth0 {
-	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>;
-};
-
-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 {
-	phy = <&p0_phy>;
-	phy-mode = "10gbase-kr";
-};
diff --git a/Documentation/devicetree/bindings/net/sff,sfp.yaml b/Documentation/devicetree/bindings/net/sff,sfp.yaml
new file mode 100644
index 000000000000..bceeff5ccedb
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/sff,sfp.yaml
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/net/sff,sfp.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Small Form Factor (SFF) Committee Small Form-factor Pluggable (SFP)
+  Transceiver
+
+maintainers:
+  - Russell King <linux@armlinux.org.uk>
+
+properties:
+  compatible:
+    enum:
+      - sff,sfp  # for SFP modules
+      - sff,sff  # for soldered down SFF modules
+
+  i2c-bus:
+    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)?":
+    maxItems: 1
+    description:
+      GPIO phandle and a specifier of the MOD-DEF0 (AKA Mod_ABS) module
+      presence input gpio signal, active (module absent) high. Must not be
+      present for SFF modules
+
+  "los-gpio(s)?":
+    maxItems: 1
+    description:
+      GPIO phandle and a specifier of the Receiver Loss of Signal Indication
+      input gpio signal, active (signal lost) high
+
+  "tx-fault-gpio(s)?":
+    maxItems: 1
+    description:
+      GPIO phandle and a specifier of the Module Transmitter Fault input gpio
+      signal, active (fault condition) high
+
+  "tx-disable-gpio(s)?":
+    maxItems: 1
+    description:
+      GPIO phandle and a specifier of the Transmitter Disable output gpio
+      signal, active (Tx disable) high
+
+  "rate-select0-gpio(s)?":
+    maxItems: 1
+    description:
+      GPIO phandle and a specifier of the Rx Signaling Rate Select (AKA RS0)
+      output gpio signal, low - low Rx rate, high - high Rx rate Must not be
+      present for SFF modules
+
+  "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
+
+required:
+  - compatible
+  - i2c-bus
+
+additionalProperties: false
+
+examples:
+  - | # Direct serdes to SFP connection
+    #include <dt-bindings/gpio/gpio.h>
+
+    sfp_eth3: sfp-eth3 {
+      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 {
+      phy-names = "comphy";
+      phys = <&cps_comphy5 0>;
+      sfp = <&sfp_eth3>;
+    };
+
+  - | # Serdes to PHY to SFP connection
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    sfp_eth0: sfp-eth0 {
+      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 {
+      #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 {
+      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
 F:	drivers/net/phy/phylink.c
 F:	drivers/net/phy/sfp*
 F:	include/linux/mdio/mdio-i2c.h
-- 
2.33.1

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

[Krzysztof Kozlowski]

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>
> ---
>  .../devicetree/bindings/net/sff,sfp.txt       |  85 ------------
>  .../devicetree/bindings/net/sff,sfp.yaml      | 130 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  3 files changed, 131 insertions(+), 85 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/net/sff,sfp.txt
>  create mode 100644 Documentation/devicetree/bindings/net/sff,sfp.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/sff,sfp.txt b/Documentation/devicetree/bindings/net/sff,sfp.txt
> deleted file mode 100644
> index 832139919f20..000000000000
> --- a/Documentation/devicetree/bindings/net/sff,sfp.txt
> +++ /dev/null
> @@ -1,85 +0,0 @@
> -Small Form Factor (SFF) Committee Small Form-factor Pluggable (SFP)
> -Transceiver
> -
> -Required properties:
> -
> -- compatible : must be one of
> -  "sff,sfp" for SFP modules
> -  "sff,sff" for soldered down SFF modules
> -
> -- i2c-bus : phandle of an I2C bus controller for the SFP two wire serial
> -  interface
> -
> -Optional Properties:
> -
> -- mod-def0-gpios : GPIO phandle and a specifier of the MOD-DEF0 (AKA Mod_ABS)
> -  module presence input gpio signal, active (module absent) high. Must
> -  not be present for SFF modules
> -
> -- los-gpios : GPIO phandle and a specifier of the Receiver Loss of Signal
> -  Indication input gpio signal, active (signal lost) high
> -
> -- tx-fault-gpios : GPIO phandle and a specifier of the Module Transmitter
> -  Fault input gpio signal, active (fault condition) high
> -
> -- tx-disable-gpios : GPIO phandle and a specifier of the Transmitter Disable
> -  output gpio signal, active (Tx disable) high
> -
> -- rate-select0-gpios : GPIO phandle and a specifier of the Rx Signaling Rate
> -  Select (AKA RS0) output gpio signal, low: low Rx rate, high: high Rx rate
> -  Must not be present for SFF modules
> -
> -- rate-select1-gpios : 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
> -
> -- maximum-power-milliwatt : 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.
> -
> -Example #1: Direct serdes to SFP connection
> -
> -sfp_eth3: sfp-eth3 {
> -	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 {
> -	phy-names = "comphy";
> -	phys = <&cps_comphy5 0>;
> -	sfp = <&sfp_eth3>;
> -};
> -
> -Example #2: Serdes to PHY to SFP connection
> -
> -sfp_eth0: sfp-eth0 {
> -	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>;
> -};
> -
> -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 {
> -	phy = <&p0_phy>;
> -	phy-mode = "10gbase-kr";
> -};
> diff --git a/Documentation/devicetree/bindings/net/sff,sfp.yaml b/Documentation/devicetree/bindings/net/sff,sfp.yaml
> new file mode 100644
> index 000000000000..bceeff5ccedb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/sff,sfp.yaml
> @@ -0,0 +1,130 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/net/sff,sfp.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Small Form Factor (SFF) Committee Small Form-factor Pluggable (SFP)
> +  Transceiver
> +
> +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.

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

> +    maxItems: 1
> +    description:
> +      GPIO phandle and a specifier of the MOD-DEF0 (AKA Mod_ABS) module
> +      presence input gpio signal, active (module absent) high. Must not be
> +      present for SFF modules
> +
> +  "los-gpio(s)?":
> +    maxItems: 1
> +    description:
> +      GPIO phandle and a specifier of the Receiver Loss of Signal Indication
> +      input gpio signal, active (signal lost) high
> +
> +  "tx-fault-gpio(s)?":
> +    maxItems: 1
> +    description:
> +      GPIO phandle and a specifier of the Module Transmitter Fault input gpio
> +      signal, active (fault condition) high
> +
> +  "tx-disable-gpio(s)?":
> +    maxItems: 1
> +    description:
> +      GPIO phandle and a specifier of the Transmitter Disable output gpio
> +      signal, active (Tx disable) high
> +
> +  "rate-select0-gpio(s)?":
> +    maxItems: 1
> +    description:
> +      GPIO phandle and a specifier of the Rx Signaling Rate Select (AKA RS0)
> +      output gpio signal, low - low Rx rate, high - high Rx rate Must not be
> +      present for SFF modules
> +
> +  "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.

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

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

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

> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    sfp_eth0: sfp-eth0 {

Same node name - generic.

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

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

Thanks!

>  F:	drivers/net/phy/phylink.c
>  F:	drivers/net/phy/sfp*
>  F:	include/linux/mdio/mdio-i2c.h


Best regards,
Krzysztof

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

[Ioana Ciornei]

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

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

[Krzysztof Kozlowski]

On 15/03/2022 20:07, Ioana Ciornei wrote:
> 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?

Because original bindings required gpios, so DTS are wrong, and the
pattern makes it difficult to grep and read such simple property.

The DTSes which do not follow bindings should be corrected.

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

Usage (consumer) is not related to these bindings. The bindings for this
phy/mac should show the usage of sfp, but not the provider bindings.

The bindings of each clock provider do not contain examples for clock
consumer. Same for regulator, pinctrl, power domains, interconnect and
every other component. It would be a lot of code duplication to include
consumers in each provider. Instead, we out the example of consumer in
the consumer bindings.

The harm is - duplicated code and one more example which can be done
wrong (like here node name not conforming to DT spec).

If you insist to keep it, please share why these bindings are special,
different than all other bindings I mentioned above.

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

You have second same include so you will have doubled defines. Usually
it was an error...

Best regards,
Krzysztof

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

[Ioana Ciornei]

On Wed, Mar 16, 2022 at 09:23:45AM +0100, Krzysztof Kozlowski wrote:
> On 15/03/2022 20:07, Ioana Ciornei wrote:
> > 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?
> 
> Because original bindings required gpios, so DTS are wrong, and the
> pattern makes it difficult to grep and read such simple property.
> 
> The DTSes which do not follow bindings should be corrected.
> 

Russell, do you have any thoughts on this?
I am asking this because you were the one that added the "-gpios" suffix
in the dtbinding and the "-gpio" usage in the DT files so I wouldn't
want this to diverge from your thinking.

Do you have a preference?

> > 
> >>> +
> >>> +  "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.
> 
> Usage (consumer) is not related to these bindings. The bindings for this
> phy/mac should show the usage of sfp, but not the provider bindings.
> 
> The bindings of each clock provider do not contain examples for clock
> consumer. Same for regulator, pinctrl, power domains, interconnect and
> every other component. It would be a lot of code duplication to include
> consumers in each provider. Instead, we out the example of consumer in
> the consumer bindings.
> 
> The harm is - duplicated code and one more example which can be done
> wrong (like here node name not conforming to DT spec).

I suppose you refer to "sfp-eth3" which you suggested here to be just
"sfp". In an example, that's totally acceptable but on boards there can
be multiple SFPs which would mean that there would be multiple sfp
nodes. We have to discern somehow between them. Adding a unit name would
not be optimal since there is no "reg" property to go with it.

So "sfp-eth3" or variants I think are necessary even though not
conforming to the DT spec.

> 
> If you insist to keep it, please share why these bindings are special,
> different than all other bindings I mentioned above.

If it's that unheard of to have a somewhat complete example why are
there multiple dtschema files submitted even by yourself with this same
setup?
As an example for a consumer device being listed in the provider yaml
file is 'gpio-pca95xx.yaml' and for the reverse (provider described in
the consumer) I can list 'samsung,s5pv210-clock.yaml',
'samsung,exynos5260-clock.yaml' etc.

Ioana

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

[Krzysztof Kozlowski]

On 16/03/2022 11:18, Ioana Ciornei wrote:
>>>
>>> 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.
>>
>> Usage (consumer) is not related to these bindings. The bindings for this
>> phy/mac should show the usage of sfp, but not the provider bindings.
>>
>> The bindings of each clock provider do not contain examples for clock
>> consumer. Same for regulator, pinctrl, power domains, interconnect and
>> every other component. It would be a lot of code duplication to include
>> consumers in each provider. Instead, we out the example of consumer in
>> the consumer bindings.
>>
>> The harm is - duplicated code and one more example which can be done
>> wrong (like here node name not conforming to DT spec).
> 
> I suppose you refer to "sfp-eth3" which you suggested here to be just
> "sfp". 

I refer now to "cps_emac3" which uses specific name instead of generic
and underscore instead of hyphen (although underscore is actually listed
as allowed in DT spec, dtc will complain about it).

>In an example, that's totally acceptable but on boards there can
> be multiple SFPs which would mean that there would be multiple sfp
> nodes. We have to discern somehow between them. Adding a unit name would
> not be optimal since there is no "reg" property to go with it.

The common practice is adding a numbering suffix.

> 
> So "sfp-eth3" or variants I think are necessary even though not
> conforming to the DT spec.
> 
>>
>> If you insist to keep it, please share why these bindings are special,
>> different than all other bindings I mentioned above.
> 
> If it's that unheard of to have a somewhat complete example why are
> there multiple dtschema files submitted even by yourself with this same
> setup?

I am also learning and I wished many of my mistakes of early DT bindings
conversion were spotted. Especially my early bindings... but even now I
keep making mistakes. Human thing. :)

I converted quite a lot of bindings, so can you point to such examples
of my schema which includes consumer example in a provider bindings? If
you find such, please send a patch removing trivial code.


> As an example for a consumer device being listed in the provider yaml
> file is 'gpio-pca95xx.yaml'

Indeed, this is trivial and useless code which I kept from conversion,
should be removed.

>
 and for the reverse (provider described in
> the consumer) I can list 'samsung,s5pv210-clock.yaml',
> 'samsung,exynos5260-clock.yaml' etc.

These are different. This is an example how to model the input clock to
the device being described in the bindings. This is not an example how
to use the clock provider, like you created here. The input clock
sometimes is defined in Exynos clock controller, sometimes outside. The
example there shows the second case - when it has to come outside. It's
not showing the usage of clocks provided by this device, but I agree
that it also might be trivial and obvious. If you think it is obvious,
feel free to comment/send a patch.

Best regards,
Krzysztof

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

[Russell King (Oracle)]

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>
> > ---
> >  .../devicetree/bindings/net/sff,sfp.txt       |  85 ------------
> >  .../devicetree/bindings/net/sff,sfp.yaml      | 130 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  3 files changed, 131 insertions(+), 85 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/net/sff,sfp.txt
> >  create mode 100644 Documentation/devicetree/bindings/net/sff,sfp.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/net/sff,sfp.txt b/Documentation/devicetree/bindings/net/sff,sfp.txt
> > deleted file mode 100644
> > index 832139919f20..000000000000
> > --- a/Documentation/devicetree/bindings/net/sff,sfp.txt
> > +++ /dev/null
> > @@ -1,85 +0,0 @@
> > -Small Form Factor (SFF) Committee Small Form-factor Pluggable (SFP)
> > -Transceiver
> > -
> > -Required properties:
> > -
> > -- compatible : must be one of
> > -  "sff,sfp" for SFP modules
> > -  "sff,sff" for soldered down SFF modules
> > -
> > -- i2c-bus : phandle of an I2C bus controller for the SFP two wire serial
> > -  interface
> > -
> > -Optional Properties:
> > -
> > -- mod-def0-gpios : GPIO phandle and a specifier of the MOD-DEF0 (AKA Mod_ABS)
> > -  module presence input gpio signal, active (module absent) high. Must
> > -  not be present for SFF modules
> > -
> > -- los-gpios : GPIO phandle and a specifier of the Receiver Loss of Signal
> > -  Indication input gpio signal, active (signal lost) high
> > -
> > -- tx-fault-gpios : GPIO phandle and a specifier of the Module Transmitter
> > -  Fault input gpio signal, active (fault condition) high
> > -
> > -- tx-disable-gpios : GPIO phandle and a specifier of the Transmitter Disable
> > -  output gpio signal, active (Tx disable) high
> > -
> > -- rate-select0-gpios : GPIO phandle and a specifier of the Rx Signaling Rate
> > -  Select (AKA RS0) output gpio signal, low: low Rx rate, high: high Rx rate
> > -  Must not be present for SFF modules
> > -
> > -- rate-select1-gpios : 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
> > -
> > -- maximum-power-milliwatt : 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.
> > -
> > -Example #1: Direct serdes to SFP connection
> > -
> > -sfp_eth3: sfp-eth3 {
> > -	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 {
> > -	phy-names = "comphy";
> > -	phys = <&cps_comphy5 0>;
> > -	sfp = <&sfp_eth3>;
> > -};
> > -
> > -Example #2: Serdes to PHY to SFP connection
> > -
> > -sfp_eth0: sfp-eth0 {
> > -	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>;
> > -};
> > -
> > -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 {
> > -	phy = <&p0_phy>;
> > -	phy-mode = "10gbase-kr";
> > -};
> > diff --git a/Documentation/devicetree/bindings/net/sff,sfp.yaml b/Documentation/devicetree/bindings/net/sff,sfp.yaml
> > new file mode 100644
> > index 000000000000..bceeff5ccedb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/sff,sfp.yaml
> > @@ -0,0 +1,130 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/net/sff,sfp.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Small Form Factor (SFF) Committee Small Form-factor Pluggable (SFP)
> > +  Transceiver
> > +
> > +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.
> 
> > +    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.
> 
> > +    maxItems: 1
> > +    description:
> > +      GPIO phandle and a specifier of the MOD-DEF0 (AKA Mod_ABS) module
> > +      presence input gpio signal, active (module absent) high. Must not be
> > +      present for SFF modules
> > +
> > +  "los-gpio(s)?":
> > +    maxItems: 1
> > +    description:
> > +      GPIO phandle and a specifier of the Receiver Loss of Signal Indication
> > +      input gpio signal, active (signal lost) high
> > +
> > +  "tx-fault-gpio(s)?":
> > +    maxItems: 1
> > +    description:
> > +      GPIO phandle and a specifier of the Module Transmitter Fault input gpio
> > +      signal, active (fault condition) high
> > +
> > +  "tx-disable-gpio(s)?":
> > +    maxItems: 1
> > +    description:
> > +      GPIO phandle and a specifier of the Transmitter Disable output gpio
> > +      signal, active (Tx disable) high
> > +
> > +  "rate-select0-gpio(s)?":
> > +    maxItems: 1
> > +    description:
> > +      GPIO phandle and a specifier of the Rx Signaling Rate Select (AKA RS0)
> > +      output gpio signal, low - low Rx rate, high - high Rx rate Must not be
> > +      present for SFF modules
> > +
> > +  "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.
> 
> > +
> > +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"?

How does that work when you have several? If we have to have sfp@0,
sfp@1 etc then we also need a reg property which will never be parsed
and the number is meaningless.

In any case, this would be an _additional_ change over a pure conversion
so arguably should be done in a separate patch.

> 
> > +      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.
> 
> > +      phy-names = "comphy";
> > +      phys = <&cps_comphy5 0>;
> > +      sfp = <&sfp_eth3>;
> > +    };

Please explain why this is "not related" when it mentions the SFP.

> > +
> > +  - | # Serdes to PHY to SFP connection
> > +    #include <dt-bindings/gpio/gpio.h>
> 
> Are you sure it works fine? Double define?

Err what? Sorry, I don't understand what you're saying here, please
explain what the issue is.

> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    sfp_eth0: sfp-eth0 {
> 
> Same node name - generic.
> 
> > +      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.
> 
> > +      #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";
> > +    };

These examples are showing how the SFP gets hooked up directly to a MAC
or directly to a PHY. Would you prefer them to be in the ethernet-mac
and ethernet-phy yaml files instead? It seems utterly perverse to split
an example across several different yaml files.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Mar 16, 2022 at 10:18:55AM +0000, Ioana Ciornei wrote:
> On Wed, Mar 16, 2022 at 09:23:45AM +0100, Krzysztof Kozlowski wrote:
> > On 15/03/2022 20:07, Ioana Ciornei wrote:
> > > 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?
> > 
> > Because original bindings required gpios, so DTS are wrong, and the
> > pattern makes it difficult to grep and read such simple property.
> > 
> > The DTSes which do not follow bindings should be corrected.
> > 
> 
> Russell, do you have any thoughts on this?
> I am asking this because you were the one that added the "-gpios" suffix
> in the dtbinding and the "-gpio" usage in the DT files so I wouldn't
> want this to diverge from your thinking.
> 
> Do you have a preference?

SFP support predated (in my tree) the deprecation of the -gpio suffix,
and despite the SFP binding doc being sent for review, it didn't get
reviewed so the issue was never picked up.

My understanding is that GPIO will continue to accept either -gpio or
-gpios for ever, so there shouldn't be any issue here - so converting
all instances of -gpio to -gpios should be doable without issue.

> If it's that unheard of to have a somewhat complete example why are
> there multiple dtschema files submitted even by yourself with this same
> setup?
> As an example for a consumer device being listed in the provider yaml
> file is 'gpio-pca95xx.yaml' and for the reverse (provider described in
> the consumer) I can list 'samsung,s5pv210-clock.yaml',
> 'samsung,exynos5260-clock.yaml' etc.

My feels are it _is_ useful to show the consumer side in examples.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Mar 16, 2022 at 01:04:21PM +0100, Krzysztof Kozlowski wrote:
> On 16/03/2022 11:18, Ioana Ciornei wrote:
> >>>
> >>> 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.
> >>
> >> Usage (consumer) is not related to these bindings. The bindings for this
> >> phy/mac should show the usage of sfp, but not the provider bindings.
> >>
> >> The bindings of each clock provider do not contain examples for clock
> >> consumer. Same for regulator, pinctrl, power domains, interconnect and
> >> every other component. It would be a lot of code duplication to include
> >> consumers in each provider. Instead, we out the example of consumer in
> >> the consumer bindings.
> >>
> >> The harm is - duplicated code and one more example which can be done
> >> wrong (like here node name not conforming to DT spec).
> > 
> > I suppose you refer to "sfp-eth3" which you suggested here to be just
> > "sfp". 
> 
> I refer now to "cps_emac3" which uses specific name instead of generic
> and underscore instead of hyphen (although underscore is actually listed
> as allowed in DT spec, dtc will complain about it).
> 
> >In an example, that's totally acceptable but on boards there can
> > be multiple SFPs which would mean that there would be multiple sfp
> > nodes. We have to discern somehow between them. Adding a unit name would
> > not be optimal since there is no "reg" property to go with it.
> 
> The common practice is adding a numbering suffix.
> 
> > 
> > So "sfp-eth3" or variants I think are necessary even though not
> > conforming to the DT spec.
> > 
> >>
> >> If you insist to keep it, please share why these bindings are special,
> >> different than all other bindings I mentioned above.
> > 
> > If it's that unheard of to have a somewhat complete example why are
> > there multiple dtschema files submitted even by yourself with this same
> > setup?
> 
> I am also learning and I wished many of my mistakes of early DT bindings
> conversion were spotted. Especially my early bindings... but even now I
> keep making mistakes. Human thing. :)
> 
> I converted quite a lot of bindings, so can you point to such examples
> of my schema which includes consumer example in a provider bindings? If
> you find such, please send a patch removing trivial code.
> 
> 
> > As an example for a consumer device being listed in the provider yaml
> > file is 'gpio-pca95xx.yaml'
> 
> Indeed, this is trivial and useless code which I kept from conversion,
> should be removed.
> 
> >
>  and for the reverse (provider described in
> > the consumer) I can list 'samsung,s5pv210-clock.yaml',
> > 'samsung,exynos5260-clock.yaml' etc.
> 
> These are different. This is an example how to model the input clock to
> the device being described in the bindings. This is not an example how
> to use the clock provider, like you created here. The input clock
> sometimes is defined in Exynos clock controller, sometimes outside. The
> example there shows the second case - when it has to come outside. It's
> not showing the usage of clocks provided by this device, but I agree
> that it also might be trivial and obvious. If you think it is obvious,
> feel free to comment/send a patch.

Why is whether something is an input or output relevant? One can quite
rightly argue that SFPs are both input and output. :)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

Usefully, Krzysztof Kozlowski's email bounces for me.

On Wed, Mar 30, 2022 at 04:54:28PM +0100, Russell King (Oracle) wrote:
> On Wed, Mar 16, 2022 at 01:04:21PM +0100, Krzysztof Kozlowski wrote:
> > On 16/03/2022 11:18, Ioana Ciornei wrote:
> > >>>
> > >>> 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.
> > >>
> > >> Usage (consumer) is not related to these bindings. The bindings for this
> > >> phy/mac should show the usage of sfp, but not the provider bindings.
> > >>
> > >> The bindings of each clock provider do not contain examples for clock
> > >> consumer. Same for regulator, pinctrl, power domains, interconnect and
> > >> every other component. It would be a lot of code duplication to include
> > >> consumers in each provider. Instead, we out the example of consumer in
> > >> the consumer bindings.
> > >>
> > >> The harm is - duplicated code and one more example which can be done
> > >> wrong (like here node name not conforming to DT spec).
> > > 
> > > I suppose you refer to "sfp-eth3" which you suggested here to be just
> > > "sfp". 
> > 
> > I refer now to "cps_emac3" which uses specific name instead of generic
> > and underscore instead of hyphen (although underscore is actually listed
> > as allowed in DT spec, dtc will complain about it).
> > 
> > >In an example, that's totally acceptable but on boards there can
> > > be multiple SFPs which would mean that there would be multiple sfp
> > > nodes. We have to discern somehow between them. Adding a unit name would
> > > not be optimal since there is no "reg" property to go with it.
> > 
> > The common practice is adding a numbering suffix.
> > 
> > > 
> > > So "sfp-eth3" or variants I think are necessary even though not
> > > conforming to the DT spec.
> > > 
> > >>
> > >> If you insist to keep it, please share why these bindings are special,
> > >> different than all other bindings I mentioned above.
> > > 
> > > If it's that unheard of to have a somewhat complete example why are
> > > there multiple dtschema files submitted even by yourself with this same
> > > setup?
> > 
> > I am also learning and I wished many of my mistakes of early DT bindings
> > conversion were spotted. Especially my early bindings... but even now I
> > keep making mistakes. Human thing. :)
> > 
> > I converted quite a lot of bindings, so can you point to such examples
> > of my schema which includes consumer example in a provider bindings? If
> > you find such, please send a patch removing trivial code.
> > 
> > 
> > > As an example for a consumer device being listed in the provider yaml
> > > file is 'gpio-pca95xx.yaml'
> > 
> > Indeed, this is trivial and useless code which I kept from conversion,
> > should be removed.
> > 
> > >
> >  and for the reverse (provider described in
> > > the consumer) I can list 'samsung,s5pv210-clock.yaml',
> > > 'samsung,exynos5260-clock.yaml' etc.
> > 
> > These are different. This is an example how to model the input clock to
> > the device being described in the bindings. This is not an example how
> > to use the clock provider, like you created here. The input clock
> > sometimes is defined in Exynos clock controller, sometimes outside. The
> > example there shows the second case - when it has to come outside. It's
> > not showing the usage of clocks provided by this device, but I agree
> > that it also might be trivial and obvious. If you think it is obvious,
> > feel free to comment/send a patch.
> 
> Why is whether something is an input or output relevant? One can quite
> rightly argue that SFPs are both input and output. :)
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Rob Herring]

On Wed, Mar 30, 2022 at 10:52 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Wed, Mar 16, 2022 at 10:18:55AM +0000, Ioana Ciornei wrote:
> > On Wed, Mar 16, 2022 at 09:23:45AM +0100, Krzysztof Kozlowski wrote:
> > > On 15/03/2022 20:07, Ioana Ciornei wrote:
> > > > 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?
> > >
> > > Because original bindings required gpios, so DTS are wrong, and the
> > > pattern makes it difficult to grep and read such simple property.
> > >
> > > The DTSes which do not follow bindings should be corrected.
> > >
> >
> > Russell, do you have any thoughts on this?
> > I am asking this because you were the one that added the "-gpios" suffix
> > in the dtbinding and the "-gpio" usage in the DT files so I wouldn't
> > want this to diverge from your thinking.
> >
> > Do you have a preference?
>
> SFP support predated (in my tree) the deprecation of the -gpio suffix,
> and despite the SFP binding doc being sent for review, it didn't get
> reviewed so the issue was never picked up.

Really?

https://lore.kernel.org/all/CAL_JsqL_7gG8FSEJDXu=37DFpHjfLhQuUhPFRKcScYTzM4cNyg@mail.gmail.com/


> My understanding is that GPIO will continue to accept either -gpio or
> -gpios for ever, so there shouldn't be any issue here - so converting
> all instances of -gpio to -gpios should be doable without issue.
>
> > If it's that unheard of to have a somewhat complete example why are
> > there multiple dtschema files submitted even by yourself with this same
> > setup?
> > As an example for a consumer device being listed in the provider yaml
> > file is 'gpio-pca95xx.yaml' and for the reverse (provider described in
> > the consumer) I can list 'samsung,s5pv210-clock.yaml',
> > 'samsung,exynos5260-clock.yaml' etc.
>
> My feels are it _is_ useful to show the consumer side in examples.

I think having it is fine here as long as the consumer has a schema.
This case is a bit different as there's really only 1 provider
instance and this is it.

It's the 100s of clock, gpio, interrupt, etc. schemas that we don't
need showing the consumer side over and over.

Rob

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

[Russell King (Oracle)]

On Wed, Mar 30, 2022 at 11:09:42AM -0500, Rob Herring wrote:
> On Wed, Mar 30, 2022 at 10:52 AM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Mar 16, 2022 at 10:18:55AM +0000, Ioana Ciornei wrote:
> > > On Wed, Mar 16, 2022 at 09:23:45AM +0100, Krzysztof Kozlowski wrote:
> > > > On 15/03/2022 20:07, Ioana Ciornei wrote:
> > > > > 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?
> > > >
> > > > Because original bindings required gpios, so DTS are wrong, and the
> > > > pattern makes it difficult to grep and read such simple property.
> > > >
> > > > The DTSes which do not follow bindings should be corrected.
> > > >
> > >
> > > Russell, do you have any thoughts on this?
> > > I am asking this because you were the one that added the "-gpios" suffix
> > > in the dtbinding and the "-gpio" usage in the DT files so I wouldn't
> > > want this to diverge from your thinking.
> > >
> > > Do you have a preference?
> >
> > SFP support predated (in my tree) the deprecation of the -gpio suffix,
> > and despite the SFP binding doc being sent for review, it didn't get
> > reviewed so the issue was never picked up.
> 
> Really?
> 
> https://lore.kernel.org/all/CAL_JsqL_7gG8FSEJDXu=37DFpHjfLhQuUhPFRKcScYTzM4cNyg@mail.gmail.com/

Yes. I said "in my tree" not "in Linus' tree". It dates from shortly
after the first SolidRun Clearfog arrived here, first set of patches
were based on v4.3-rc2.

Remember, when development eventually gets submitted as patches, even
if there are no changes, if the patches are then applied, they get the
date of the _email_ not of their creation, and there can be several
years between creation and submission.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[Krzysztof Kozlowski]

On 30/03/2022 17:54, Russell King (Oracle) wrote:
>>
>> These are different. This is an example how to model the input clock to
>> the device being described in the bindings. This is not an example how
>> to use the clock provider, like you created here. The input clock
>> sometimes is defined in Exynos clock controller, sometimes outside. The
>> example there shows the second case - when it has to come outside. It's
>> not showing the usage of clocks provided by this device, but I agree
>> that it also might be trivial and obvious. If you think it is obvious,
>> feel free to comment/send a patch.
> 
> Why is whether something is an input or output relevant? One can quite
> rightly argue that SFPs are both input and output. :)
> 

I don't mind removing that example. Input - in the case of these
bindings - is quite specific. Output is opposite, not specific and can
vary, you can enable/disable, change frequency.

Discussion was two weeks ago, so all emails will bounce. :)

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 30/03/2022 17:40, Russell King (Oracle) wrote:
> 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>
>>> ---
>>>  .../devicetree/bindings/net/sff,sfp.txt       |  85 ------------
>>>  .../devicetree/bindings/net/sff,sfp.yaml      | 130 ++++++++++++++++++
>>>  MAINTAINERS                                   |   1 +
>>>  3 files changed, 131 insertions(+), 85 deletions(-)
>>>  delete mode 100644 Documentation/devicetree/bindings/net/sff,sfp.txt
>>>  create mode 100644 Documentation/devicetree/bindings/net/sff,sfp.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/sff,sfp.txt b/Documentation/devicetree/bindings/net/sff,sfp.txt
>>> deleted file mode 100644
>>> index 832139919f20..000000000000
>>> --- a/Documentation/devicetree/bindings/net/sff,sfp.txt
>>> +++ /dev/null
>>> @@ -1,85 +0,0 @@
>>> -Small Form Factor (SFF) Committee Small Form-factor Pluggable (SFP)
>>> -Transceiver
>>> -
>>> -Required properties:
>>> -
>>> -- compatible : must be one of
>>> -  "sff,sfp" for SFP modules
>>> -  "sff,sff" for soldered down SFF modules
>>> -
>>> -- i2c-bus : phandle of an I2C bus controller for the SFP two wire serial
>>> -  interface
>>> -
>>> -Optional Properties:
>>> -
>>> -- mod-def0-gpios : GPIO phandle and a specifier of the MOD-DEF0 (AKA Mod_ABS)
>>> -  module presence input gpio signal, active (module absent) high. Must
>>> -  not be present for SFF modules
>>> -
>>> -- los-gpios : GPIO phandle and a specifier of the Receiver Loss of Signal
>>> -  Indication input gpio signal, active (signal lost) high
>>> -
>>> -- tx-fault-gpios : GPIO phandle and a specifier of the Module Transmitter
>>> -  Fault input gpio signal, active (fault condition) high
>>> -
>>> -- tx-disable-gpios : GPIO phandle and a specifier of the Transmitter Disable
>>> -  output gpio signal, active (Tx disable) high
>>> -
>>> -- rate-select0-gpios : GPIO phandle and a specifier of the Rx Signaling Rate
>>> -  Select (AKA RS0) output gpio signal, low: low Rx rate, high: high Rx rate
>>> -  Must not be present for SFF modules
>>> -
>>> -- rate-select1-gpios : 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
>>> -
>>> -- maximum-power-milliwatt : 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.
>>> -
>>> -Example #1: Direct serdes to SFP connection
>>> -
>>> -sfp_eth3: sfp-eth3 {
>>> -	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 {
>>> -	phy-names = "comphy";
>>> -	phys = <&cps_comphy5 0>;
>>> -	sfp = <&sfp_eth3>;
>>> -};
>>> -
>>> -Example #2: Serdes to PHY to SFP connection
>>> -
>>> -sfp_eth0: sfp-eth0 {
>>> -	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>;
>>> -};
>>> -
>>> -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 {
>>> -	phy = <&p0_phy>;
>>> -	phy-mode = "10gbase-kr";
>>> -};
>>> diff --git a/Documentation/devicetree/bindings/net/sff,sfp.yaml b/Documentation/devicetree/bindings/net/sff,sfp.yaml
>>> new file mode 100644
>>> index 000000000000..bceeff5ccedb
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/sff,sfp.yaml
>>> @@ -0,0 +1,130 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: "http://devicetree.org/schemas/net/sff,sfp.yaml#"
>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>> +
>>> +title: Small Form Factor (SFF) Committee Small Form-factor Pluggable (SFP)
>>> +  Transceiver
>>> +
>>> +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.
>>
>>> +    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.
>>
>>> +    maxItems: 1
>>> +    description:
>>> +      GPIO phandle and a specifier of the MOD-DEF0 (AKA Mod_ABS) module
>>> +      presence input gpio signal, active (module absent) high. Must not be
>>> +      present for SFF modules
>>> +
>>> +  "los-gpio(s)?":
>>> +    maxItems: 1
>>> +    description:
>>> +      GPIO phandle and a specifier of the Receiver Loss of Signal Indication
>>> +      input gpio signal, active (signal lost) high
>>> +
>>> +  "tx-fault-gpio(s)?":
>>> +    maxItems: 1
>>> +    description:
>>> +      GPIO phandle and a specifier of the Module Transmitter Fault input gpio
>>> +      signal, active (fault condition) high
>>> +
>>> +  "tx-disable-gpio(s)?":
>>> +    maxItems: 1
>>> +    description:
>>> +      GPIO phandle and a specifier of the Transmitter Disable output gpio
>>> +      signal, active (Tx disable) high
>>> +
>>> +  "rate-select0-gpio(s)?":
>>> +    maxItems: 1
>>> +    description:
>>> +      GPIO phandle and a specifier of the Rx Signaling Rate Select (AKA RS0)
>>> +      output gpio signal, low - low Rx rate, high - high Rx rate Must not be
>>> +      present for SFF modules
>>> +
>>> +  "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.
>>
>>> +
>>> +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"?
> 
> How does that work when you have several? If we have to have sfp@0,
> sfp@1 etc then we also need a reg property which will never be parsed
> and the number is meaningless.
> 
> In any case, this would be an _additional_ change over a pure conversion
> so arguably should be done in a separate patch.

First of all, there is no such case here. It's only one node.
Second, it works exactly the same like for all other cases - leds,
regulators etc. I already described it in other email (led-0, led-1).

> 
>>
>>> +      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.
>>
>>> +      phy-names = "comphy";
>>> +      phys = <&cps_comphy5 0>;
>>> +      sfp = <&sfp_eth3>;
>>> +    };
> 
> Please explain why this is "not related" when it mentions the SFP.

Consumer, which it looks like, is not related to the binding. The same
as we do not put examples of clock consumers in clock providers.

I already explained in other mail to Ioana, so you can just refer there...

> 
>>> +
>>> +  - | # Serdes to PHY to SFP connection
>>> +    #include <dt-bindings/gpio/gpio.h>
>>
>> Are you sure it works fine? Double define?
> 
> Err what? Sorry, I don't understand what you're saying here, please
> explain what the issue is.

Including the same header twice causes duplicate defines, which should
be visible when testing the binding.

> 
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +
>>> +    sfp_eth0: sfp-eth0 {
>>
>> Same node name - generic.
>>
>>> +      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.
>>
>>> +      #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";
>>> +    };
> 
> These examples are showing how the SFP gets hooked up directly to a MAC
> or directly to a PHY. Would you prefer them to be in the ethernet-mac
> and ethernet-phy yaml files instead? It seems utterly perverse to split
> an example across several different yaml files.

Probably PHY or MAC is better place, because it defines the "sfp" property.

How is it different from other cases like this in bindings (clocks,
power domains, GPIOs)? IOW, why SFP is special? If it is, sure, let's
keep it here...

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 30/03/2022 18:51, Krzysztof Kozlowski wrote:

(...)

>>
>> These examples are showing how the SFP gets hooked up directly to a MAC
>> or directly to a PHY. Would you prefer them to be in the ethernet-mac
>> and ethernet-phy yaml files instead? It seems utterly perverse to split
>> an example across several different yaml files.
> 
> Probably PHY or MAC is better place, because it defines the "sfp" property.
> 
> How is it different from other cases like this in bindings (clocks,
> power domains, GPIOs)? IOW, why SFP is special? If it is, sure, let's
> keep it here...

BTW, Rob actually pointed out the difference here - it's only one such
provider binding (unlike clock controllers) - and said it's fine, so
good for me.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 30/03/2022 18:51, Krzysztof Kozlowski wrote:
> On 30/03/2022 17:40, Russell King (Oracle) wrote:
>>
>>>> +
>>>> +  - | # Serdes to PHY to SFP connection
>>>> +    #include <dt-bindings/gpio/gpio.h>
>>>
>>> Are you sure it works fine? Double define?
>>
>> Err what? Sorry, I don't understand what you're saying here, please
>> explain what the issue is.
> 
> Including the same header twice causes duplicate defines, which should
> be visible when testing the binding.

I don't see such errors now, so either something changed or I confused
with something else. It should work, so let's skip my comment here.

Best regards,
Krzysztof

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

[Rob Herring]

On Tue, Mar 15, 2022 at 6:33 AM Ioana Ciornei <ioana.ciornei@nxp.com> wrote:
>
> Convert the sff,sfp.txt bindings to the DT schema format.
>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  .../devicetree/bindings/net/sff,sfp.txt       |  85 ------------
>  .../devicetree/bindings/net/sff,sfp.yaml      | 130 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  3 files changed, 131 insertions(+), 85 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/net/sff,sfp.txt
>  create mode 100644 Documentation/devicetree/bindings/net/sff,sfp.yaml

Going to respin this patch? I don't normally care too much, but this
one is high (86 occurrences on arm64 dtbs) on my list of compatibles
without schemas and is generic.

Rob