Re: [PATCH v3 2/2] soundwire: qcom: add support for SoundWire controller

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

 





On 10/14/19 4:04 AM, Srinivas Kandagatla wrote:
Thanks Pierre for taking time to review the patch.

On 11/10/2019 18:50, Pierre-Louis Bossart wrote:

+static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
+                     u8 dev_addr, u16 reg_addr)
+{
+    DECLARE_COMPLETION_ONSTACK(comp);
+    unsigned long flags;
+    u32 val;
+    int ret;
+
+    spin_lock_irqsave(&ctrl->comp_lock, flags);
+    ctrl->comp = ∁
+    spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+    val = SWRM_REG_VAL_PACK(cmd_data, dev_addr,
+                SWRM_SPECIAL_CMD_ID, reg_addr);
+    ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
+    if (ret)
+        goto err;
+
+    ret = wait_for_completion_timeout(ctrl->comp,
+                      msecs_to_jiffies(TIMEOUT_MS));
+
+    if (!ret)
+        ret = SDW_CMD_IGNORED;
+    else
+        ret = SDW_CMD_OK;

It's odd to report CMD_IGNORED on a timeout. CMD_IGNORED is a valid answer that should be retrieved immediately. You probably need to translate the soundwire errors into -ETIMEOUT or something.

In this controller we have no way to know if the command is ignored or timedout, so All the commands that did not receive response either due to ignore or timeout are currently detected with out any response interrupt in given timeout.

ok. It's still very odd. a CMD_IGNORED is a required answer e.g. when there is no device0 left to enumerate, when a device has fallen off the bus or when accessing a non-implemented register.

+static int qcom_swrm_register_dais(struct qcom_swrm_ctrl *ctrl)
+{
+    int num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
+    struct snd_soc_dai_driver *dais;
+    struct snd_soc_pcm_stream *stream;
+    struct device *dev = ctrl->dev;
+    int i;
+
+    /* PDM dais are only tested for now */
+    dais = devm_kcalloc(dev, num_dais, sizeof(*dais), GFP_KERNEL);
+    if (!dais)
+        return -ENOMEM;
+
+    for (i = 0; i < num_dais; i++) {
+        dais[i].name = devm_kasprintf(dev, GFP_KERNEL, "SDW Pin%d", i);
+        if (!dais[i].name)
+            return -ENOMEM;
+
+        if (i < ctrl->num_dout_ports) {
+            stream = &dais[i].playback;
+            stream->stream_name = devm_kasprintf(dev, GFP_KERNEL,
+                                 "SDW Tx%d", i);
+        } else {
+            stream = &dais[i].capture;
+            stream->stream_name = devm_kasprintf(dev, GFP_KERNEL,
+                                 "SDW Rx%d", i);
+        }

For the Intel stuff, we removed the stream_name assignment since it conflicted with the DAI widgets added by the topology. Since the code looks inspired by the Intel DAI handling, you should look into this.

Yes, this code was inspired by Intel's DAI handling, I will take a look a look at latest code and update accordingly.


FWIW, the stream handling seems to have issues as well, specifically the 'deprepare' part, we are currently working around errors with suspend-resume, see e.g. experimental branch at https://github.com/thesofproject/linux/pull/1314

_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel




[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