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,
