On 09/05/2022 15:24, Pierre-Louis Bossart wrote:
You could say why do we need wait itself in the first place.
The reason we need wait in first place is because, there is a danger of codec accessing registers even before enumeration is finished. Because most of the ASoC codec registration happens as part of codec/component driver probe function rather than status callback.
I hope this answers your questions.
Humm, not really.
First, you're using this TIMEOUT_MS value in 3 unrelated places, and using 2 different struct completion, which means there are side effects beyond autoenumeration.
2 of these 3 are autoenum ones, one is in probe path and other in bus reset path during pm.
The final one is broadcast timeout, new timeout value should be okay for that too, given that we need 10ms timeout.
probably better to make things explicit with a different timeout value for the broadcast case.
I agree, we should define a dedicated 10ms timeout for this to avoid confusion.
100ms for a broadcast is really eons, it's supposed to be immediate really.
And then I don't get what you do on a timeout. in the enumeration part,
We can't do much on the timeout.
the timeout value is not checked for, so I guess enumeration proceeds without the hardware being available? That seems to contradict the assertion that you don't want to access registers before the hardware is properly initialized.
Even if enumeration timeout, we will not access the registers because the ASoC codec is not registered yet from WCD938x component master.
What happens when the codec is registered then? the autoenumeration
Codec is only registered after reset and when both TX and RX components are probed.
still didn't complete, so what prevents the read/writes from failing then?
If codec is reset and registered and for some reason autoenum took more than 100ms which will be hw bug then :-). In this case register read/writes will fail.
--srini
And then on broadcast you have this error handling:
ret = wait_for_completion_timeout(&swrm->broadcast, msecs_to_jiffies(TIMEOUT_MS)); if (!ret) ret = SDW_CMD_IGNORED; else ret = SDW_CMD_OK;
Which is equally confusing since SDW_CMD_IGNORED is really an error, and the bus layer does not really handle it well - not to mention that I vaguely recall the qcom hardware having its own definition of IGNORED?
QCom hardware alteast the version based on which this driver was written did not have any support to report errors type back on register read/writes.
In this case a broad cast read or write did not generate a complete interrupt its assumed that its ignored, as there is no way to say if its error or ignored.
ok, that should be fine.
The code in bus.c mostly ignores -ENODATA for the suspend cases. For the bank switch, I forgot that we also ignore it, Bard added a patch in 2021. The only case where we abort a transfer is on a real error.