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.
