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

[Vinod <vkoul@kernel.org>]

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