Re: [PATCH 5/6] soundwire: qcom: update register read/write routine

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux