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

[Krishna Kurapati PSSNV]

On 12/26/2023 3:03 PM, Krzysztof Kozlowski wrote:
> 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?"

HI Krzysztof,

 Thanks for the review.
 To answer the question,

"why ?" : The interrupts have been mis-interpreted on many platforms or 
many interrupts are missing.

Now, if I am adding the missing interrupts, I need to segregate targets 
also into respective buckets in the same patch and that is what making 
this patch a little complicated. Is it possible / acceptable to split 
this into two patches if this is the case. Can you help with suggestions 
from your end ? Or may be I am understanding your question wrong ? 😅

Regards,
Krishna,