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

[Pierre-Louis Bossart]

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