[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1a1a4ac0-388c-8999-47a2-6d6f7471ab93@linux.intel.com>
Date: Mon, 14 Oct 2019 10:43:54 -0500
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
robh@...nel.org, vkoul@...nel.org
Cc: broonie@...nel.org, bgoswami@...eaurora.org,
devicetree@...r.kernel.org, lgirdwood@...il.com,
alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
spapothi@...eaurora.org
Subject: Re: [alsa-devel] [PATCH v3 2/2] soundwire: qcom: add support for
SoundWire controller
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
Powered by blists - more mailing lists