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
