Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	robh@kernel.org, vkoul@kernel.org
Cc: devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
	bgoswami@codeaurora.org, spapothi@codeaurora.org,
	lgirdwood@gmail.com, linux-kernel@vger.kernel.org,
	broonie@kernel.org
Subject: Re: [alsa-devel] [PATCH v3 2/2] soundwire: qcom: add support for SoundWire controller
Date: Mon, 14 Oct 2019 10:04:56 +0100
Message-ID: <d053a17e-3d6d-e3b6-f988-485e77c30e3b@linaro.org> (raw)
In-Reply-To: <9d00c94b-1bce-9fdf-55fe-ee681466a97a@linux.intel.com>

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
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-11 15:44 [alsa-devel] [PATCH v3 0/2] soundwire: Add support to Qualcomm SoundWire master Srinivas Kandagatla
2019-10-11 15:44 ` [alsa-devel] [PATCH v3 1/2] dt-bindings: soundwire: add bindings for Qcom controller Srinivas Kandagatla
2019-10-14 17:12   ` Rob Herring
2019-10-14 17:34     ` Srinivas Kandagatla
2019-10-15 11:35       ` Rob Herring
2019-10-15 12:22         ` Srinivas Kandagatla
2019-10-15 13:36           ` Rob Herring
2019-10-21  4:27   ` Vinod Koul
2019-10-11 15:44 ` [alsa-devel] [PATCH v3 2/2] soundwire: qcom: add support for SoundWire controller Srinivas Kandagatla
2019-10-11 17:50   ` Pierre-Louis Bossart
2019-10-14  9:04     ` Srinivas Kandagatla [this message]
2019-10-14 15:43       ` Pierre-Louis Bossart
2019-10-21  4:44   ` Vinod Koul
2019-10-30 14:56     ` Srinivas Kandagatla
2019-10-30 15:18       ` Pierre-Louis Bossart

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d053a17e-3d6d-e3b6-f988-485e77c30e3b@linaro.org \
    --to=srinivas.kandagatla@linaro.org \
    --cc=alsa-devel@alsa-project.org \
    --cc=bgoswami@codeaurora.org \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=robh@kernel.org \
    --cc=spapothi@codeaurora.org \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git