Re: [PATCH v5 1/2] dt-bindings: usb: dwc3: Clean up hs_phy_irq in binding

[Krzysztof Kozlowski]

On 26/12/2023 06:37, Krishna Kurapati PSSNV wrote:
> 
> 
> On 12/25/2023 6:35 PM, Krzysztof Kozlowski wrote:
>> On 22/12/2023 07:36, Krishna Kurapati wrote:
>>> The high speed related interrupts present on QC targets are as follows:
>>>
>>
>>
>>>   
>>>     interrupt-names:
>>> -    minItems: 1
>>> -    maxItems: 4
>>> +    minItems: 2
>>> +    maxItems: 5
>>>   
>>>     qcom,select-utmi-as-pipe-clk:
>>>       description:
>>> @@ -361,60 +378,21 @@ allOf:
>>>           compatible:
>>>             contains:
>>>               enum:
>>> -              - qcom,ipq4019-dwc3
>>
>> Why do you remove it, without adding it somewhere else. Nothing in the
>> commit msg explains it.
>>
> 
> Apologies, Will check and add it back.

Please check your patch for other entries. I just took first compatible
which turns out to be gone. I did not check the reset and I don't want
to keep checking.

...

>>> -    then:
>>> -      properties:
>>> -        interrupts:
>>> -          minItems: 1
>>> -          maxItems: 2
>>> -        interrupt-names:
>>> -          minItems: 1
>>> -          items:
>>> -            - const: hs_phy_irq
>>> -            - const: ss_phy_irq
>>> -
>>> -  - if:
>>> -      properties:
>>> -        compatible:
>>> -          contains:
>>> -            enum:
>>> -              - qcom,sc7280-dwc3
>>> +              - qcom,sm6115-dwc3
>>> +              - qcom,sm6125-dwc3
>>>       then:
>>>         properties:
>>>           interrupts:
>>>             minItems: 3
>>>             maxItems: 4
>>>           interrupt-names:
>>> -          minItems: 3
>>>             items:
>>> +            - const: pwr_event
>>>               - const: hs_phy_irq
>>> -            - const: dp_hs_phy_irq
>>> -            - const: dm_hs_phy_irq
>>> +            - const: qusb2_phy
>>
>> Why qusb2_phy is after hs_phy_irq? In the earlier if:then: it is the
>> second one.
>>
> 
> In v3 as well, the hs_phy_irq is before qusb2_phy interrupt:
> https://lore.kernel.org/all/20231211121124.4194-2-quic_kriskura@xxxxxxxxxxx/

? How v3 matters?

> 
>>
>>>               - const: ss_phy_irq
>>>   
>>>     - if:
>>> @@ -460,11 +422,13 @@ allOf:
>>>           compatible:
>>>             contains:
>>>               enum:
>>> +              - qcom,ipq5332-dwc3
>>>                 - qcom,sc8280xp-dwc3
>>>                 - qcom,x1e80100-dwc3
>>>       then:
>>>         properties:
>>>           interrupts:
>>> +          minItems: 3
>>
>> Hm, why? This commit is unmanageable. Your commit msg is already huge
>> but still does not explain this. Are you sure you are fixing only one
>> logical thing per patch? Does not look like.
>>
> 
> This is reordering the targets based on interrupts they have. I put it 
> in one commit because splitting this into multiple patches breaks one 
> thing or other. Also once I am defining permutations, I have to group 
> targets into these combinations in the same patch. I know this is a big 
> commit but it solves the interrupt cleanup and defines a way for future 
> targets.


This does not answer why, you sc8280xp and x1e80100 not get one optional
interrupt. I asked "why" you are doing this change. Why do you need it?
What is the rationale?

Then I grunted about unmanageable commit, because all my troubles to
review it are the effect of it: it is very difficult to read. It is also
difficult for you, because you keep making here mistakes. So if you
cannot write this commit properly and I cannot review it, then it is way
over-complicated, don't you think? But this is still second problem
here, don't ignore the fist - "why?"

Best regards,
Krzysztof