Hi Krzysztof, 

-----Original Message-----
From: Krzysztof Kozlowski <krzk@kernel.org>
Sent: 12 July 2025 01:44 PM
To: Pritam Manohar Sutar <redacted>; 'Krzysztof Kozlowski'
[off-list ref]
Cc: vkoul@kernel.org; kishon@kernel.org; robh@kernel.org;
krzk+dt@kernel.org; conor+dt@kernel.org; alim.akhtar@samsung.com;
andre.draszik@linaro.org; peter.griffin@linaro.org; neil.armstrong@linaro.org;
kauschluss@disroot.org; ivo.ivanov.ivanov1@gmail.com;
m.szyprowski@samsung.com; s.nawrocki@samsung.com; linux-
phy@lists.infradead.org; devicetree@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-
soc@vger.kernel.org; rosa.pila@samsung.com; dev.tailor@samsung.com;
faraz.ata@samsung.com; muhammed.ali@samsung.com;
selvarasu.g@samsung.com
Subject: Re: [PATCH v4 1/6] dt-bindings: phy: samsung,usb3-drd-phy: add
ExynosAutov920 HS phy compatible

On 09/07/2025 10:46, Pritam Manohar Sutar wrote:

quoted

Hi Krzysztof,

quoted

-----Original Message-----
From: Krzysztof Kozlowski <redacted>
Sent: 06 July 2025 03:11 PM
To: Pritam Manohar Sutar <redacted>
Cc: vkoul@kernel.org; kishon@kernel.org; robh@kernel.org;
krzk+dt@kernel.org; conor+dt@kernel.org; alim.akhtar@samsung.com;
andre.draszik@linaro.org; peter.griffin@linaro.org;
neil.armstrong@linaro.org; kauschluss@disroot.org;
ivo.ivanov.ivanov1@gmail.com; m.szyprowski@samsung.com;
s.nawrocki@samsung.com; linux- phy@lists.infradead.org;
devicetree@vger.kernel.org; linux- kernel@vger.kernel.org;
linux-arm-kernel@lists.infradead.org; linux-samsung-
soc@vger.kernel.org; rosa.pila@samsung.com; dev.tailor@samsung.com;
faraz.ata@samsung.com; muhammed.ali@samsung.com;
selvarasu.g@samsung.com
Subject: Re: [PATCH v4 1/6] dt-bindings: phy: samsung,usb3-drd-phy:
add
ExynosAutov920 HS phy compatible

On Tue, Jul 01, 2025 at 05:37:01PM +0530, Pritam Manohar Sutar wrote:

quoted

Add a dedicated compatible string for USB HS phy found in this SoC.
The SoC requires two clocks, named "phy" and "ref" (same as clocks
required by Exynos850).

It also requires various power supplies (regulators) for the
internal circuitry to work. The required voltages are:
* avdd075_usb - 0.75v
* avdd18_usb20 - 1.8v
* avdd33_usb20 - 3.3v

Reviewed-by: Krzysztof Kozlowski <redacted>

No, really. Look:

quoted

Signed-off-by: Pritam Manohar Sutar <redacted>
---
 .../bindings/phy/samsung,usb3-drd-phy.yaml    | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git
a/Documentation/devicetree/bindings/phy/samsung,usb3-drd-phy.yaml
b/Documentation/devicetree/bindings/phy/samsung,usb3-drd-phy.yaml
index e906403208c0..2e29ff749bba 100644
---
a/Documentation/devicetree/bindings/phy/samsung,usb3-drd-phy.yaml

+++ b/Documentation/devicetree/bindings/phy/samsung,usb3-drd-phy.yam
+++ l

@@ -34,6 +34,7 @@ properties:
       - samsung,exynos7870-usbdrd-phy
       - samsung,exynos850-usbdrd-phy
       - samsung,exynos990-usbdrd-phy
+      - samsung,exynosautov920-usbdrd-phy

   clocks:
     minItems: 1

@@ -110,6 +111,15 @@ properties:
   vddh-usbdp-supply:
     description: VDDh power supply for the USB DP phy.

+  avdd075_usb-supply:
+    description: 0.75V power supply for USB phy
+
+  avdd18_usb20-supply:
+    description: 1.8V power supply for USB phy
+
+  avdd33_usb20-supply:
+    description: 3.3V power supply for USB phy
+

None of these were here. Follow DTS coding style... but why are you
adding completely new supplies?

Digital supplies were here. Need Analog supplies for exynosautov920,
hence added new Analog supplies; 'a'vdd075_usb, 'a'vdd18_usb20,
'a'vdd33_usb20

Will add "full stops" for DTS coding style in description.

You cannot grow one line change into 50 line change and retain the review.

Yes agreed. Will remove "Reviewed-by" tag and requesting you to review the 
patches again and provide your valuable comments.

quoted

quoted

quoted

 required:
   - compatible
   - clocks

@@ -235,6 +245,33 @@ allOf:

         reg-names:
           maxItems: 1
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - samsung,exynosautov920-usbdrd-phy
+    then:
+      properties:
+        clocks:
+          minItems: 2
+          maxItems: 2
+
+        clock-names:
+          items:
+            - const: phy
+            - const: ref
+
+        reg:
+          maxItems: 1
+
+        reg-names:
+          maxItems: 1
+
+      required:
+        - avdd075_usb-supply
+        - avdd18_usb20-supply
+        - avdd33_usb20-supply

Neither was this entire diff hunk here.

This was part of other block for a reason.

Added regulators in driver. This block is added for regulators (other
block does not have "required" field for power supplies) if excluded
this block, "make ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/samsung,usb3-

drd

quoted

-phy.yaml" will fail as mentioned below

Nothing is explained in changelog/cover letter. You claim you only added Rb tag.
This is an entirely silent change while keeping the review.

Will add more explanations in cover letter/changelog why this block is added.

Combined with not even following DTS style!

Ok got it. Will change supplies name as below 
avdd075_usb => avdd075-usb
avdd18_usb20 => avdd18-usb20
avdd33_usb20 => avdd33-usb20
   
Confirm the above change that is meant in terms of DTS style.

It's not acceptable.

Best regards,
Krzysztof

Thank you.

Regards,
Pritam