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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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