Message-ID: <d053a17e-3d6d-e3b6-f988-485e77c30e3b@linaro.org>
Date:   Mon, 14 Oct 2019 10:04:56 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        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: [PATCH v3 2/2] soundwire: qcom: add support for SoundWire
 controller

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 = &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.

> 
>> +err:
>> +    spin_lock_irqsave(&ctrl->comp_lock, flags);
>> +    ctrl->comp = NULL;
>> +    spin_unlock_irqrestore(&ctrl->comp_lock, flags);
>> +
>> +    return ret;
>> +}
>> +
>> +
>> +    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;
>> +} > +
> 
> [snip]
> 
>> +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = dev_id;
>> +    u32 sts, value;
>> +    unsigned long flags;
>> +
>> +    ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS, &sts);
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
>> +        ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
>> +        dev_err_ratelimited(ctrl->dev,
>> +                    "CMD error, fifo status 0x%x\n",
>> +                     value);
>> +        ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
>> +    }
>> +
>> +    if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
>> +        sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS)
>> +        schedule_work(&ctrl->slave_work);
>> +
>> +    ctrl->reg_write(ctrl, SWRM_INTERRUPT_CLEAR, sts);
>> +
>> +    if (sts & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED) {
>> +        spin_lock_irqsave(&ctrl->comp_lock, flags);
>> +        if (ctrl->comp)
>> +            complete(ctrl->comp);
>> +        spin_unlock_irqrestore(&ctrl->comp_lock, flags);
> 
> 
> Wouldn't it be simpler if you declared the completion structure as part 
> of your controller definitions, as done for the Intel stuff?
> 
I can give that a go!
> [snip]
> 
>> +static void qcom_swrm_stream_free_ports(struct qcom_swrm_ctrl *ctrl,
>> +                    struct sdw_stream_runtime *stream)
>> +{
>> +    struct sdw_master_runtime *m_rt;
>> +    struct sdw_port_runtime *p_rt;
>> +    unsigned long *port_mask;
>> +
>> +    mutex_lock(&ctrl->port_lock);
> 
> is this lock to avoid races between alloc/free? if yes maybe document it?
> 

Yes, port allocation resource is protected across these calls here, I 
can add some notes as you suggested in next version.

>> +
>> +    list_for_each_entry(m_rt, &stream->master_list, stream_node) {
>> +        if (m_rt->direction == SDW_DATA_DIR_RX)
>> +            port_mask = &ctrl->dout_port_mask;
>> +        else
>> +            port_mask = &ctrl->din_port_mask;
>> +
>> +        list_for_each_entry(p_rt, &m_rt->port_list, port_node)
>> +            clear_bit(p_rt->num - 1, port_mask);
>> +    }
>> +
>> +    mutex_unlock(&ctrl->port_lock);
>> +}
>> +
>> +static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
>> +                    struct sdw_stream_runtime *stream,
>> +                       struct snd_pcm_hw_params *params,
>> +                       int direction)
>> +{
>> +    struct sdw_port_config pconfig[QCOM_SDW_MAX_PORTS];
>> +    struct sdw_stream_config sconfig;
>> +    struct sdw_master_runtime *m_rt;
>> +    struct sdw_slave_runtime *s_rt;
>> +    struct sdw_port_runtime *p_rt;
>> +    unsigned long *port_mask;
>> +    int i, maxport, pn, nports = 0, ret = 0;
>> +
>> +    mutex_lock(&ctrl->port_lock);
>> +    list_for_each_entry(m_rt, &stream->master_list, stream_node) {
>> +        if (m_rt->direction == SDW_DATA_DIR_RX) {
>> +            maxport = ctrl->num_dout_ports;
>> +            port_mask = &ctrl->dout_port_mask;
>> +        } else {
>> +            maxport = ctrl->num_din_ports;
>> +            port_mask = &ctrl->din_port_mask;
>> +        }
>> +
>> +        list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
>> +            list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
>> +                /* Port numbers start from 1 - 14*/
>> +                pn = find_first_zero_bit(port_mask, maxport);
>> +                if (pn > (maxport - 1)) {
>> +                    dev_err(ctrl->dev, "All ports busy\n");
>> +                    ret = -EBUSY;
>> +                    goto err;
>> +                }
>> +                set_bit(pn, port_mask);
>> +                pconfig[nports].num = pn + 1;
>> +                pconfig[nports].ch_mask = p_rt->ch_mask;
>> +                nports++;
>> +            }
>> +        }
>> +    }
>> +
>> +    if (direction == SNDRV_PCM_STREAM_CAPTURE)
>> +        sconfig.direction = SDW_DATA_DIR_TX;
>> +    else
>> +        sconfig.direction = SDW_DATA_DIR_RX;
>> +
>> +    sconfig.ch_count = 1;
>> +    sconfig.frame_rate = params_rate(params);
>> +    sconfig.type = stream->type;
>> +    sconfig.bps = 1;
> 
> Should probably add a note that hw_params is ignored since it's PDM 
> content, so only 1ch 1bit data.
> 

Okay Sure!
>> +    sdw_stream_add_master(&ctrl->bus, &sconfig, pconfig,
>> +                  nports, stream);
>> +err:
>> +    if (ret) {
>> +        for (i = 0; i < nports; i++)
>> +            clear_bit(pconfig[i].num - 1, port_mask);
>> +    }
>> +
>> +    mutex_unlock(&ctrl->port_lock);
>> +
>> +    return ret;
>> +}
>> +
> 
> [snip]
> 
>> +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_deprepare_stream(sruntime);
>> +    sdw_disable_stream(sruntime);
> 
> Should is be the reverse order? Removing ports/master before disabling 
> doesn't seem too good.

Good point!  Will fix it in next version.

> 
>> +
>> +    return 0;
>> +}
>> +
> 
>> +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.

Thanks,
srini
> 

Powered by blists - more mailing lists