+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.
+err:
+ spin_lock_irqsave(&ctrl->comp_lock, flags);
+ ctrl->comp = NULL;
+ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+
+ return ret;
+}
+
+static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
+ u8 dev_addr, u16 reg_addr,
+ u32 len, u8 *rval)
+{
+ int i, ret;
+ u32 val;
+ DECLARE_COMPLETION_ONSTACK(comp);
+ unsigned long flags;
+
+ spin_lock_irqsave(&ctrl->comp_lock, flags);
+ ctrl->comp = ∁
+ spin_unlock_irqrestore(&ctrl->comp_lock, flags);
+
+ val = SWRM_REG_VAL_PACK(len, dev_addr, SWRM_SPECIAL_CMD_ID, reg_addr);
+ ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
+ if (ret)
+ goto err;
+
+ ret = wait_for_completion_timeout(ctrl->comp,
+ msecs_to_jiffies(TIMEOUT_MS));
+
+ if (!ret) {
+ ret = SDW_CMD_IGNORED;
+ goto err;
+ } else {
+ ret = SDW_CMD_OK;
+ }
same comment on reporting SDW_CMD_IGNORED on timeout, this is very odd.
+
+ 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?
[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?
+
+ 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.
+ 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.
+
+ 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
