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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 09/07/2025 10:46, Pritam Manohar Sutar wrote:
> Hi Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: 06 July 2025 03:11 PM
>> To: Pritam Manohar Sutar <pritam.sutar@samsung.com>
>> 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:
>>> 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@linaro.org>
>>
>> No, really. Look:
>>
>>> Signed-off-by: Pritam Manohar Sutar <pritam.sutar@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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy