linux-kernel - RE: [PATCH v4 1/6] dt-bindings: phy: samsung,usb3-drd-phy: add ExynosAutov920 HS phy compatible

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <000c01dbf70b$ccdbf630$6693e290$@samsung.com>
Date: Thu, 17 Jul 2025 16:43:14 +0530
From: "Pritam Manohar Sutar" <pritam.sutar@...sung.com>
To: "'Krzysztof Kozlowski'" <krzk@...nel.org>, "'Krzysztof Kozlowski'"
	<krzysztof.kozlowski@...aro.org>
Cc: <vkoul@...nel.org>, <kishon@...nel.org>, <robh@...nel.org>,
	<krzk+dt@...nel.org>, <conor+dt@...nel.org>, <alim.akhtar@...sung.com>,
	<andre.draszik@...aro.org>, <peter.griffin@...aro.org>,
	<neil.armstrong@...aro.org>, <kauschluss@...root.org>,
	<ivo.ivanov.ivanov1@...il.com>, <m.szyprowski@...sung.com>,
	<s.nawrocki@...sung.com>, <linux-phy@...ts.infradead.org>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-samsung-soc@...r.kernel.org>,
	<rosa.pila@...sung.com>, <dev.tailor@...sung.com>, <faraz.ata@...sung.com>,
	<muhammed.ali@...sung.com>, <selvarasu.g@...sung.com>
Subject: RE: [PATCH v4 1/6] dt-bindings: phy: samsung,usb3-drd-phy: add
 ExynosAutov920 HS phy compatible

Hi Krzysztof, 

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@...nel.org>
> Sent: 12 July 2025 01:44 PM
> To: Pritam Manohar Sutar <pritam.sutar@...sung.com>; 'Krzysztof Kozlowski'
> <krzysztof.kozlowski@...aro.org>
> Cc: vkoul@...nel.org; kishon@...nel.org; robh@...nel.org;
> krzk+dt@...nel.org; conor+dt@...nel.org; alim.akhtar@...sung.com;
> andre.draszik@...aro.org; peter.griffin@...aro.org; neil.armstrong@...aro.org;
> kauschluss@...root.org; ivo.ivanov.ivanov1@...il.com;
> m.szyprowski@...sung.com; s.nawrocki@...sung.com; linux-
> phy@...ts.infradead.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-samsung-
> soc@...r.kernel.org; rosa.pila@...sung.com; dev.tailor@...sung.com;
> faraz.ata@...sung.com; muhammed.ali@...sung.com;
> selvarasu.g@...sung.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:
> > Hi Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> >> Sent: 06 July 2025 03:11 PM
> >> To: Pritam Manohar Sutar <pritam.sutar@...sung.com>
> >> Cc: vkoul@...nel.org; kishon@...nel.org; robh@...nel.org;
> >> krzk+dt@...nel.org; conor+dt@...nel.org; alim.akhtar@...sung.com;
> >> andre.draszik@...aro.org; peter.griffin@...aro.org;
> >> neil.armstrong@...aro.org; kauschluss@...root.org;
> >> ivo.ivanov.ivanov1@...il.com; m.szyprowski@...sung.com;
> >> s.nawrocki@...sung.com; linux- phy@...ts.infradead.org;
> >> devicetree@...r.kernel.org; linux- kernel@...r.kernel.org;
> >> linux-arm-kernel@...ts.infradead.org; linux-samsung-
> >> soc@...r.kernel.org; rosa.pila@...sung.com; dev.tailor@...sung.com;
> >> faraz.ata@...sung.com; muhammed.ali@...sung.com;
> >> selvarasu.g@...sung.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:
> >>> 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 <krzysztof.kozlowski@...aro.org>
> >>
> >> No, really. Look:
> >>
> >>> Signed-off-by: Pritam Manohar Sutar <pritam.sutar@...sung.com>
> >>> ---
> >>>  .../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.

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