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

[Krzysztof Kozlowski]

On 09/07/2025 10:46, Pritam Manohar Sutar wrote:
> Hi Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
>> Sent: 06 July 2025 03:11 PM
>> To: Pritam Manohar Sutar <pritam.sutar at samsung.com>
>> Cc: vkoul at kernel.org; kishon at kernel.org; robh at kernel.org;
>> krzk+dt at kernel.org; conor+dt at kernel.org; alim.akhtar at samsung.com;
>> andre.draszik at linaro.org; peter.griffin at linaro.org; neil.armstrong at linaro.org;
>> kauschluss at disroot.org; ivo.ivanov.ivanov1 at gmail.com;
>> m.szyprowski at samsung.com; s.nawrocki at samsung.com; linux-
>> phy at lists.infradead.org; devicetree at vger.kernel.org; linux-
>> kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-samsung-
>> soc at vger.kernel.org; rosa.pila at samsung.com; dev.tailor at samsung.com;
>> faraz.ata at samsung.com; muhammed.ali at samsung.com;
>> selvarasu.g at 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:
>>> 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 at linaro.org>
>>
>> No, really. Look:
>>
>>> Signed-off-by: Pritam Manohar Sutar <pritam.sutar at samsung.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.yaml
>>> @@ -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.
> 
>>
>>
>>>  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.
Combined with not even following DTS style!

It's not acceptable.

Best regards,
Krzysztof