On 10/30/19 10:31 AM, Srinivas Kandagatla wrote:
Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
either integrated as part of WCD audio codecs via slimbus or
as part of SOC I/O.
This patchset adds support to a very basic controller which has been
tested with WCD934x SoundWire controller connected to WSA881x smart
speaker amplifiers.
Sorry for the delay in reviewing this patch.
I have a set of comments mostly on error handling and mapping between ASoC callbacks and stream states (which took a lot of work on our side and required an updated state machine in the patches shared last week).
[snip]
+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;
+err:
+ÂÂÂ spin_lock_irqsave(&ctrl->comp_lock, flags);
+ÂÂÂ ctrl->comp = NULL;
+ÂÂÂ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+
+ÂÂÂ return ret;
+}
+
+static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u8 dev_addr, u16 reg_addr,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u32 len, u8 *rval)
+{
+ÂÂÂ int i, ret;
+ÂÂÂ u32 val;
+ÂÂÂ DECLARE_COMPLETION_ONSTACK(comp);
+ÂÂÂ unsigned long flags;
+
+ÂÂÂ spin_lock_irqsave(&ctrl->comp_lock, flags);
+ÂÂÂ ctrl->comp = ∁
+ÂÂÂ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+
+ÂÂÂ val = SWRM_REG_VAL_PACK(len, dev_addr, SWRM_SPECIAL_CMD_ID, reg_addr);
+ÂÂÂ ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ goto err;
+
+ÂÂÂ ret = wait_for_completion_timeout(ctrl->comp,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ msecs_to_jiffies(TIMEOUT_MS));
+
+ÂÂÂ if (!ret) {
+ÂÂÂÂÂÂÂ ret = SDW_CMD_IGNORED;
+ÂÂÂÂÂÂÂ goto err;
+ÂÂÂ } else {
+ÂÂÂÂÂÂÂ ret = SDW_CMD_OK;
+ÂÂÂ }
+
+ÂÂÂ for (i = 0; i < len; i++) {
+ÂÂÂÂÂÂÂ ret = ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR, &val);
+ÂÂÂÂÂÂÂ if (ret)
+ÂÂÂÂÂÂÂÂÂÂÂ return ret;
+
+ÂÂÂÂÂÂÂ rval[i] = val & 0xFF;
+ÂÂÂ }
+
+err:
+ÂÂÂ spin_lock_irqsave(&ctrl->comp_lock, flags);
+ÂÂÂ ctrl->comp = NULL;
+ÂÂÂ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+
+ÂÂÂ return ret;
+}
+
+static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
+{
+ÂÂÂ u32 val;
+ÂÂÂ int i;
+
+ÂÂÂ ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val);
Sometimes you test the return value of reg_read(), and sometimes you don't? same for read_write()?
For the Intel stuff, we typically don't check the read/writes to controller registers, but anything that goes out on the bus is checked.
+
+static int qcom_swrm_prepare(struct snd_pcm_substream *substream,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_soc_dai *dai)
+{
+ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
+
+ÂÂÂ if (!ctrl->sruntime[dai->id])
+ÂÂÂÂÂÂÂ return -EINVAL;
+
+ÂÂÂ return sdw_enable_stream(ctrl->sruntime[dai->id]);
So in hw_params you call sdw_prepare_stream() and in _prepare you call sdw_enable_stream()?
Shouldn't this be handled in a .trigger operation as per the documentation "From ASoC DPCM framework, this stream state is linked to
.trigger() start operation."
It's also my understanding that .prepare will be called multiples times,
including for underflows and resume if you don't support INFO_RESUME.
the sdw_disable_stream() is in .hw_free, which is not necessarily called by the core, so you may have a risk of not being able to recover?
+}
+
+static int qcom_swrm_hw_free(struct snd_pcm_substream *substream,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_soc_dai *dai)
+{
+ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
+ÂÂÂ struct sdw_stream_runtime *sruntime = ctrl->sruntime[dai->id];
+
+ÂÂÂ qcom_swrm_stream_free_ports(ctrl, sruntime);
+ÂÂÂ sdw_stream_remove_master(&ctrl->bus, sruntime);
+ÂÂÂ sdw_disable_stream(sruntime);
+ÂÂÂ sdw_deprepare_stream(sruntime);
is the order correct here?
On the Intel side we do
trigger_stop:
sdw_disable_stream(sruntime);
hw_free
sdw_deprepare_stream(sruntime);
sdw_stream_remove_master(&ctrl->bus, sruntime);
sdw_release_stream(dma->stream);
+
+ÂÂÂ return 0;
+}
+
+static int qcom_swrm_set_sdw_stream(struct snd_soc_dai *dai,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void *stream, int direction)
+{
+ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
+
+ÂÂÂ ctrl->sruntime[dai->id] = stream;
+
+ÂÂÂ return 0;
+}
+
+static int qcom_swrm_startup(struct snd_pcm_substream *substream,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_soc_dai *dai)
+{
+ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
+ÂÂÂ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ÂÂÂ struct sdw_stream_runtime *sruntime;
+ÂÂÂ int ret, i;
if you supported pm_runtime, that's where you'd want to take a reference?
yes, we need this for dailink parsing code in machine driver which lookup the name from device tree node. If we do not add name at this point machine driver will fail to probe as missing dai dependencies.+ÂÂÂ sruntime = sdw_alloc_stream(dai->name);
+ÂÂÂ if (!sruntime)
+ÂÂÂÂÂÂÂ return -ENOMEM;
+
+ÂÂÂ ctrl->sruntime[dai->id] = sruntime;
+
+ÂÂÂ for (i = 0; i < rtd->num_codecs; i++) {
+ÂÂÂÂÂÂÂ ret = snd_soc_dai_set_sdw_stream(rtd->codec_dais[i], sruntime,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ substream->stream);
+ÂÂÂÂÂÂÂ if (ret < 0 && ret != -ENOTSUPP) {
+ÂÂÂÂÂÂÂÂÂÂÂ dev_err(dai->dev, "Failed to set sdw stream on %s",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ rtd->codec_dais[i]->name);
+ÂÂÂÂÂÂÂÂÂÂÂ sdw_release_stream(sruntime);
+ÂÂÂÂÂÂÂÂÂÂÂ return ret;
+ÂÂÂÂÂÂÂ }
+ÂÂÂ }
+
+ÂÂÂ return 0;
+}
+
+static void qcom_swrm_shutdown(struct snd_pcm_substream *substream,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct snd_soc_dai *dai)
+{
+ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
and that's where you'd want to call pm_runtime_put()
+ÂÂÂ sdw_release_stream(ctrl->sruntime[dai->id]);
+ÂÂÂ ctrl->sruntime[dai->id] = NULL;
+}
+
+static const struct snd_soc_dai_ops qcom_swrm_pdm_dai_ops = {
+ÂÂÂ .hw_params = qcom_swrm_hw_params,
+ÂÂÂ .prepare = qcom_swrm_prepare,
+ÂÂÂ .hw_free = qcom_swrm_hw_free,
+ÂÂÂ .startup = qcom_swrm_startup,
+ÂÂÂ .shutdown = qcom_swrm_shutdown,
+ÂÂÂ .set_sdw_stream = qcom_swrm_set_sdw_stream,
no .trigger?
+};
+
+static const struct snd_soc_component_driver qcom_swrm_dai_component = {
+ÂÂÂ .name = "soundwire",
+};
+
+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;
I think I mentioned we don't do this in the Intel stuff since it conflicts with topology? is this required on the QCOM side?
+
+ÂÂÂÂÂÂÂ if (i < ctrl->num_dout_ports)
+ÂÂÂÂÂÂÂÂÂÂÂ stream = &dais[i].playback;
+ÂÂÂÂÂÂÂ else
+ÂÂÂÂÂÂÂÂÂÂÂ stream = &dais[i].capture;
+
+ÂÂÂÂÂÂÂ stream->channels_min = 1;
+ÂÂÂÂÂÂÂ stream->channels_max = 1;
+ÂÂÂÂÂÂÂ stream->rates = SNDRV_PCM_RATE_48000;
+ÂÂÂÂÂÂÂ stream->formats = SNDRV_PCM_FMTBIT_S16_LE;
+
+ÂÂÂÂÂÂÂ dais[i].ops = &qcom_swrm_pdm_dai_ops;
+ÂÂÂÂÂÂÂ dais[i].id = i;
+ÂÂÂ }
+
+ÂÂÂ return devm_snd_soc_register_component(ctrl->dev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &qcom_swrm_dai_component,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dais, num_dais);
+}
+
Trying to keep things simple for the first patchset! added this dummies to keep the soundwire core happy!+static int qcom_swrm_runtime_suspend(struct device *device)
+{
+ÂÂÂ /* TBD */
+ÂÂÂ return 0;
+}
+
+static int qcom_swrm_runtime_resume(struct device *device)
+{
+ÂÂÂ /* TBD */
+ÂÂÂ return 0;
+}
+
+static const struct dev_pm_ops qcom_swrm_dev_pm_ops = {
+ÂÂÂ SET_RUNTIME_PM_OPS(qcom_swrm_runtime_suspend,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ qcom_swrm_runtime_resume,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ NULL
+ÂÂÂ )
+};
Maybe define pm_runtime at a later time then? We've had a lot of race conditions to deal with, and it's odd that you don't support plain vanilla suspend first?
+static const struct of_device_id qcom_swrm_of_match[] = {
+ÂÂÂ { .compatible = "qcom,soundwire-v1.3.0", },
+ÂÂÂ {/* sentinel */},
+};
+
+MODULE_DEVICE_TABLE(of, qcom_swrm_of_match);
+
+static struct platform_driver qcom_swrm_driver = {
+ÂÂÂ .probeÂÂÂ = &qcom_swrm_probe,
+ÂÂÂ .remove = &qcom_swrm_remove,
+ÂÂÂ .driver = {
+ÂÂÂÂÂÂÂ .nameÂÂÂ = "qcom-soundwire",
+ÂÂÂÂÂÂÂ .of_match_table = qcom_swrm_of_match,
+ÂÂÂÂÂÂÂ .pm = &qcom_swrm_dev_pm_ops,
+ÂÂÂ }
+};
+module_platform_driver(qcom_swrm_driver);
+
+MODULE_DESCRIPTION("Qualcomm soundwire driver");
+MODULE_LICENSE("GPL v2");