On 10/24/2023 12:40 PM, Johan Hovold wrote:
This comment no longer makes sense with your current implementation.
Can you help elaborate on your comment ? Do you mean that this API
doesn't get speed on all ports, but this has to be called in a loop to
get all the port speeds ? In that sense, I agree, I can change the
comments here.
It does not make sense to keep only half the comment after your update
as it is a suggestion for how one could go about and generalise this for
multiport, which is what you are now doing.
Thanks for explanation. Will update the comments.
But perhaps this should be done using usb_hub_for_each_child() instead
as that may be more efficient. Then you use this function to read out
the speed for all the ports in go (and store it in the port structures I
mentioned). Please determine which alternative is best.
Either ways is fine. We would have qcom->num_ports to determine how many
speeds we can read.
That's not the point. I'm referring to which alternative is less
computationally expensive and allows for a clean implementation.
Please do try to figure it out yourself.
I don't think its much of a difference:
while (loop over num_ports) {
read_usb2_speed()
}
read_usb2_speed() {
while (loop over num_ports) {
hub api to read speed.
}
}
The second one would avoid calling read_usb2_speed multiple times. Will
take that path.
[ I realise that the confusion around hs_phy_irq may be partly to blame
for this but since that one is also a per-port interrupt, that's no
longer an issue. ]
I don't want to add support for this right away [1]. I would like to
keep hs_phy_irq outside the loop for now.
No. Stop trying to take shortcuts. Again, this is upstream, not
Qualcomm's vendor kernel.
I don't think it is a shortcut.
The reason I said I would keep it out of loop is I know why we need
DP/DM/SS IRQ's during wakeup. The wakeup signals come in as
rising/falling edges in high speed on DP/DM lines and LFPS terminations
come on SS lines.
So we need these 3 interrupts for sure in wakeup context.
hs_phy_irq is not mandatory for wakeup. Any particular reason why it is
needed to add driver support for hs_phy_irq's of multiport now ? May be
I am missing something. If there is any reason why we need to add it
now, I would try to learn and see if it has any side effects (like
generating spurious wakeup's) and if nothing, I would add it back to
port structure.
Regards,
Krishna,
