Re: [PATCH 2/2] slimbus: ngd: add stream support

From: Vinod
Date: Mon Jun 25 2018 - 00:43:29 EST


On 21-06-18, 14:40, Srinivas Kandagatla wrote:

> + if (txn->mt == SLIM_MSG_MT_CORE &&
> + (txn->mc == SLIM_MSG_MC_CONNECT_SOURCE ||
> + txn->mc == SLIM_MSG_MC_CONNECT_SINK ||
> + txn->mc == SLIM_MSG_MC_DISCONNECT_PORT)) {
> +
> + txn->mt = SLIM_MSG_MT_DEST_REFERRED_USER;
> + if (txn->mc == SLIM_MSG_MC_CONNECT_SOURCE)
> + txn->mc = SLIM_USR_MC_CONNECT_SRC;
> + else if (txn->mc == SLIM_MSG_MC_CONNECT_SINK)
> + txn->mc = SLIM_USR_MC_CONNECT_SINK;
> + else if (txn->mc == SLIM_MSG_MC_DISCONNECT_PORT)
> + txn->mc = SLIM_USR_MC_DISCONNECT_PORT;

How about a switch case for this

> + i = 0;
> + wbuf[i++] = txn->la;
> + la = SLIM_LA_MGR;
> + wbuf[i++] = txn->msg->wbuf[0];
> + if (txn->mc != SLIM_USR_MC_DISCONNECT_PORT)
> + wbuf[i++] = txn->msg->wbuf[1];
> +
> + txn->comp = &done;
> + ret = slim_alloc_txn_tid(sctrl, txn);
> + if (ret) {
> + dev_err(ctrl->dev, "Unable to allocate TID\n");
> + return ret;
> + }
> +
> + wbuf[i++] = txn->tid;
> +
> + txn->msg->num_bytes = i;
> + txn->msg->wbuf = wbuf;
> + txn->msg->rbuf = rbuf;
> + txn->rl = txn->msg->num_bytes + 4;
> + }
> +
> /* HW expects length field to be excluded */
> txn->rl--;
> puc = (u8 *)pbuf;
> @@ -830,6 +869,19 @@ static int qcom_slim_ngd_xfer_msg(struct slim_controller *sctrl,
> return -ETIMEDOUT;
> }
>
> + if (txn->mt == SLIM_MSG_MT_DEST_REFERRED_USER &&
> + (txn->mc == SLIM_USR_MC_CONNECT_SRC ||
> + txn->mc == SLIM_USR_MC_CONNECT_SINK ||
> + txn->mc == SLIM_USR_MC_DISCONNECT_PORT)) {

how about precalculate this check and use:
bool something = txn->mt == SLIM_MSG_MT_DEST_REFERRED_USER &&
txn->mc == SLIM_USR_MC_CONNECT_SRC ||
txn->mc == SLIM_USR_MC_CONNECT_SINK ||
txn->mc == SLIM_USR_MC_DISCONNECT_PORT;

and then use in this case and previous one, make code better to read

if (something) {
> + timeout = wait_for_completion_timeout(&done, HZ);
> + if (!timeout) {
> + dev_err(sctrl->dev, "TX timed out:MC:0x%x,mt:0x%x",
> + txn->mc, txn->mt);
> + return -ETIMEDOUT;
> + }
> +
> + }
> +

[...]

> + struct slim_port *port = &rt->ports[i];
> +
> + if (txn.msg->num_bytes == 0) {
> + int seg_interval = SLIM_SLOTS_PER_SUPERFRAME/rt->ratem;
> + int exp;
> +
> + wbuf[txn.msg->num_bytes++] = sdev->laddr;
> + wbuf[txn.msg->num_bytes] = rt->bps >> 2 |
> + (port->ch.aux_fmt << 6);
> +
> + /* Data channel segment interval not multiple of 3 */
> + exp = seg_interval % 3;
> + if (exp)
> + wbuf[txn.msg->num_bytes] |= BIT(5);
> +
> + txn.msg->num_bytes++;
> + wbuf[txn.msg->num_bytes++] = exp << 4 | rt->prot;
> +
> + if (rt->prot == SLIM_PROTO_ISO)
> + wbuf[txn.msg->num_bytes++] =
> + port->ch.prrate |
> + SLIM_CHANNEL_CONTENT_FL;
> + else
> + wbuf[txn.msg->num_bytes++] = port->ch.prrate;
> +
> + ret = slim_alloc_txn_tid(ctrl, &txn);
> + if (ret) {
> + dev_err(&sdev->dev, "Fail to allocate TID\n");
> + return -ENXIO;
> + }
> + wbuf[txn.msg->num_bytes++] = txn.tid;
> + }
> + wbuf[txn.msg->num_bytes++] = port->ch.id;
> + }
> +
> + txn.mc = SLIM_USR_MC_DEF_ACT_CHAN;
> + txn.rl = txn.msg->num_bytes + 4;
> + ret = qcom_slim_ngd_xfer_msg_sync(ctrl, &txn);
> + if (ret) {
> + slim_free_txn_tid(ctrl, &txn);
> + dev_err(&sdev->dev, "TX timed out:MC:0x%x,mt:0x%x", txn.mc,
> + txn.mt);
> + return ret;
> + }
> +
> + txn.mc = SLIM_USR_MC_RECONFIG_NOW;
> + txn.msg->num_bytes = 2;
> + wbuf[1] = sdev->laddr;
> + txn.rl = txn.msg->num_bytes + 4;
> +
> + ret = slim_alloc_txn_tid(ctrl, &txn);
> + if (ret) {

what about tid allocated in previous loop.. they are not freed here on
error and seems to be overwritten by this allocation.
--
~Vinod