Hi Krzysztof,

On 31/05/22 15:38, Krzysztof Kozlowski wrote:

On 31/05/2022 11:51, Puranjay Mohan wrote:

quoted

Add a YAML binding document for the ICSSG Programmable real time unit
based Ethernet driver. This driver uses the PRU and PRUSS consumer APIs
to interface the PRUs and load/run the firmware for supporting ethernet
functionality.

Signed-off-by: Puranjay Mohan <redacted>
---
v1: https://lore.kernel.org/all/20220506052433.28087-2-p-mohan@ti.com/ (local) 
v1 -> v2:
* Addressed Rob's Comments

Nope, they were not addressed.

I am trying my best to address them but I am new to DT Schemas, so, I
misunderstood a few comments.

quoted

* It includes indentation, formatting, and other minor changes.
---
 .../bindings/net/ti,icssg-prueth.yaml         | 181 ++++++++++++++++++
 1 file changed, 181 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml

diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
new file mode 100644
index 000000000000..40af968e9178
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml

@@ -0,0 +1,181 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/ti,icssg-prueth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: |+

Missed Rob's comment.

Sorry, Will remove this in next version.

quoted

+  Texas Instruments ICSSG PRUSS Ethernet
+
+maintainers:
+  - Puranjay Mohan [off-list ref]
+
+description:
+  Ethernet based on the Programmable Real-Time
+  Unit and Industrial Communication Subsystem.
+
+allOf:
+  - $ref: /schemas/remoteproc/ti,pru-consumer.yaml#
+
+properties:
+  compatible:
+    enum:
+      - ti,am654-icssg-prueth  # for AM65x SoC family
+
+  pinctrl-0:
+    maxItems: 1
+
+  pinctrl-names:
+    items:
+      - const: default

You do not need these usually, they are coming from schema.

Will remove in next version.

quoted

+
+  sram:
+    description:
+      phandle to MSMC SRAM node
+
+  dmas:
+    maxItems: 10
+    description:
+      list of phandles and specifiers to UDMA.

Please follow Rob's comment - drop description.

I misunderstood his comment, I thought he is asking to remove the
reference to the .txt file (Which I removed). I will remove it in next
version.

quoted

+
+  dma-names:
+    items:
+      - const: tx0-0
+      - const: tx0-1
+      - const: tx0-2
+      - const: tx0-3
+      - const: tx1-0
+      - const: tx1-1
+      - const: tx1-2
+      - const: tx1-3
+      - const: rx0
+      - const: rx1
+
+  ethernet-ports:
+    type: object
+    properties:
+      '#address-cells':
+        const: 1
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      ^port@[0-1]$:

How did you implement Rob's comments here?

He said ethernet-port is preferred but all other drivers were using
"port" so I though it is not compulsory. Will change it if it compulsory
to use ethernet-port

quoted

+        type: object
+        description: ICSSG PRUETH external ports
+
+        $ref: ethernet-controller.yaml#
+
+        unevaluatedProperties: false
+        additionalProperties: true

No one proposed to add additionalProperties:true... Does it even work?

This is my mistake, will remove it in next version.

quoted

+        properties:
+          reg:
+            items:
+              - enum: [0, 1]
+            description: ICSSG PRUETH port number
+
+          ti,syscon-rgmii-delay:
+            $ref: /schemas/types.yaml#/definitions/phandle-array
+            description:
+              phandle to system controller node and register offset
+              to ICSSG control register for RGMII transmit delay
+
+        required:
+          - reg
+
+  ti,mii-g-rt:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: |
+      phandle to MII_G_RT module's syscon regmap.
+
+  ti,mii-rt:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: |
+      phandle to MII_RT module's syscon regmap
+
+  interrupts:
+    minItems: 2
+    maxItems: 2
+    description: |
+      Interrupt specifiers to TX timestamp IRQ.
+
+  interrupt-names:
+    items:
+      - const: tx_ts0
+      - const: tx_ts1
+
+required:
+  - compatible
+  - sram
+  - ti,mii-g-rt
+  - dmas
+  - dma-names
+  - ethernet-ports
+  - interrupts
+  - interrupt-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+
+    /* Example k3-am654 base board SR2.0, dual-emac */
+    pruss2_eth: pruss2_eth {
+            compatible = "ti,am654-icssg-prueth";

Again missed Rob's comment.

One of Rob's comment was to make the indentation as 4 which I have done.

The second comment was about 'ti,prus'.

So, ti,prus , firmware-name, and ti,pruss-gp-mux-sel are a part of
remoteproc/ti,pru-consumer.yaml which I have included with

allOf:
  - $ref: /schemas/remoteproc/ti,pru-consumer.yaml#

So, I thought it is not required to add them again.

I will add it in next version, if that is how it should be done.

Really, you ignored four of his comments. Please respect reviewers time
but not forcing them to repeat same review comments.

I am really sorry for this.

Thanks,
Puranjay Mohan

Best regards,
Krzysztof