On 01/02/2021 16:42, Pierre-Louis Bossart wrote:
On 2/1/21 9:50 AM, Srinivas Kandagatla wrote:
On 29/01/2021 19:33, Pierre-Louis Bossart wrote:
On 1/29/21 11:32 AM, Srinivas Kandagatla wrote:
In the existing code every soundwire register read and register write
are kinda blocked. Each of these are using a special command id that
what does 'kinda blocked' mean?
I meant read/writes are waiting for completion interrupt!
generates interrupt after it successfully finishes. This is really
overhead, limiting and not really necessary unless we are doing
something special.
We can simply read/write the fifo that should also give exactly
what we need! This will also allow to read/write registers in
interrupt context, which was not possible with the special
command approach.
This is really unclear, sorry.
If read/writes are waiting for an interrupt, it becomes difficult to
read or write to any registers from same interrupt handler!
Well, yes, you need to handle the complete() at a lower level than the
code that initiates the transactions otherwise you self-deadlock.
IIRC in the Intel initial code, the complete was in the handler and the
register IOs in the thread.
Yes, we did the same in previous version of the code, however with this
patch reading/writing fifo directly without need of completion should
remove that need of another thread!
+ if (id != SWR_BROADCAST_CMD_ID) {
+ if (id < 14)
+ id += 1;
+ else
+ id = 0;
that is really odd. if id=13 (group2) then id becomes 14 (master
address). A comment is really needed here.
This is magic value for each fifo read or write, so that we can verify
that them by comparing with this magic value!
This has nothing to do with device number!
You should probably add a comment here then, or use a #define instead of
the 14 which threw me off.
I agree!
+ if (cmd_id == SWR_BROADCAST_CMD_ID) {
+ /*
+ * sleep for 10ms for MSM soundwire variant to allow broadcast
+ * command to complete.
that's also super-odd. There is nothing in SoundWire that makes any
difference between a regular and a broadcast command. they all
complete in the same time (a frame).
+ */
+ ret = wait_for_completion_timeout(&swrm->broadcast, (2 *
HZ/10));
is this 10ms really or dependent on CONFIG_HZ?
comment missed?
Not intentionally :-)
I should probably to use msecs_to_jiffies here to keep it inline with
the comment!
--srini
+ if (!ret)
+ ret = SDW_CMD_IGNORED;
+ else
+ ret = SDW_CMD_OK;
no CMD_FAILED support?
Qcom controllers does not provide that information if the command is
ignored or failed by any means!
That was the behavior from the starting of this driver.
ah yes, now I remember this.
