Re: [PATCH net-next 1/8] dt-bindings: net: dsa: dt bindings for microchip lan937x

On Sat, 2021-01-30 at 04:02 +0200, Vladimir Oltean wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe

Thanks for your time on reviewing the patch series.

On Thu, Jan 28, 2021 at 12:11:05PM +0530, Prasanna Vengateshan wrote:

quoted

+  spi-max-frequency:
+    maximum: 50000000

And it actually works at 50 MHz? Cool.

Yes.

quoted

+
+  reset-gpios:
+    description: Optional gpio specifier for a reset line
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    //Ethernet switch connected via spi to the host, CPU port
wired to eth1
+    eth1 {

So if you do bother to add the DSA master in the example, can this be
&eth1 so that we could associate with the phandle below?

Sure.

quoted

+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      fixed-link {
+        speed = <1000>;
+        full-duplex;
+      };
+    };
+
+    spi1 {

Is this a label or a node name? spi1 or spi@1?

This is a label.

quoted

+      #address-cells = <1>;
+      #size-cells = <0>;
+      pinctrl-0 = <&pinctrl_spi_ksz>;
+      cs-gpios = <0>, <0>, <0>, <&pioC 28 0>;
+      id = <1>;

I know this is the SPI controller and thus mostly irrelevant, but
what
is "id = <1>"?

id is not needed, i will remove it.

quoted

+
+      lan9374: switch@0 {
+        compatible = "microchip,lan9374";
+        reg = <0>;
+
+        spi-max-frequency = <44000000>;
+
+        ethernet-ports {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          port@0 {
+            reg = <0>;
+            label = "lan1";
+          };
+          port@1 {
+            reg = <1>;
+            label = "lan2";
+          };
+          port@2 {
+            reg = <7>;

reg should match node index (port@2), here and everywhere below. As
for
the net device labels, I'm not sure if the mismatch is deliberate
there.

reg & port node indexes are different here because to match with the
 physical to logical port mapping done in the LAN9374. I realized that
the description is missing and that is to be added. However, should reg
& port node index match for the net dev? 
If it should be the same, then the same can be acheived by renaming a
label (lanx) as well.

quoted

+            label = "lan3";
+          };
+          port@3 {
+            reg = <2>;
+            label = "lan4";
+          };
+          port@4 {
+            reg = <6>;
+            label = "lan5";
+          };
+          port@5 {
+            reg = <3>;
+            label = "lan6";
+          };
+          port@6 {
+            reg = <4>;
+            label = "cpu";

label for CPU port is not needed/used.

Sure, will remove it.

quoted

+            ethernet = <&eth1>;
+            fixed-link {
+              speed = <1000>;
+              full-duplex;
+            };
+          };
+          port@7 {
+            reg = <5>;
+            label = "lan7";
+            fixed-link {
+              speed = <1000>;
+              full-duplex;
+            };
+          };
+        };
+      };
+    };