Re: [PATCH v7 11/13] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

Thanks for your review,

On 23/11/17 10:07, Charles Keepax wrote:
>> +static irqreturn_t qcom_slim_handle_rx_irq(struct qcom_slim_ctrl *ctrl,
>> +					   u32 stat)
>> +{
>> +	u32 *rx_buf, pkt[10];
>> +	bool q_rx = false;
>> +	u8 la, *buf, mc, mt, len, *b = (u8 *)&pkt[0];
>> +	u16 ele;
>> +
> 
> This function feels pretty funky, we basically have rx_buf, pkt,
> b and buf all of which basically point to the same thing. Can we
> simplify it a little?
I will give that a try before I send next version.
> 
>> +	pkt[0] = readl_relaxed(ctrl->base + MGR_RX_MSG);
>> +	mt = SLIM_HEADER_GET_MT(b[0]);
>> +	len = SLIM_HEADER_GET_RL(b[0]);
>> +	mc = SLIM_HEADER_GET_MC(b[1]);
>> +
>> +	/*
...

>> +
>> +	puc = (u8 *)pbuf;
>> +	head = (u32 *)pbuf;
>> +
>> +	if (txn->dt == SLIM_MSG_DEST_LOGICALADDR)
>> +		*head = SLIM_MSG_ASM_FIRST_WORD(txn->rl, txn->mt, txn->mc, 0,
>> +						la);
>> +	else
>> +		*head = SLIM_MSG_ASM_FIRST_WORD(txn->rl, txn->mt, txn->mc, 1,
>> +						la);
>> +
>> +	if (txn->dt == SLIM_MSG_DEST_LOGICALADDR)
>> +		puc += 3;
>> +	else
>> +		puc += 2;
> 
> Combine these two if statements, makes it much clearer the actions
> are related.
I agree!!

> 
>> +
>> +	if (txn->mt == SLIM_MSG_MT_CORE && slim_tid_txn(txn->mt, txn->mc))
> 
> slim_tid_txn checks for SLIM_MSG_MT_CORE so the check here should
> be redundant.
> 
Yep, will remove this in next version.

>> +		*(puc++) = txn->tid;
>> +
>> +	if ((txn->mt == SLIM_MSG_MT_CORE) &&
>> +		((txn->mc >= SLIM_MSG_MC_REQUEST_INFORMATION &&
>> +		txn->mc <= SLIM_MSG_MC_REPORT_INFORMATION) ||
>> +		(txn->mc >= SLIM_MSG_MC_REQUEST_VALUE &&
>> +		 txn->mc <= SLIM_MSG_MC_CHANGE_VALUE))) {
>> +		*(puc++) = (txn->ec & 0xFF);
>> +		*(puc++) = (txn->ec >> 8) & 0xFF;
>> +	}
> 
> As you already have slim_tid_txn, would it be worth adding
> something like slim_ec_txn? 
I will give it a go and see how it looks like..

To state if an element code is
> required, feels like other controls will probably want to do a
> similar thing and would make the code a little more readable
> here.
> 
>> +
>> +	if (txn->msg && txn->msg->wbuf)
>> +		memcpy(puc, txn->msg->wbuf, txn->msg->num_bytes);
>> +
>> +	qcom_slim_queue_tx(ctrl, head, txn->rl, MGR_TX_MSG);
>> +	timeout = wait_for_completion_timeout(&done, msecs_to_jiffies(ms));
>> +
>> +	if (!timeout) {
>> +		dev_err(ctrl->dev, "TX timed out:MC:0x%x,mt:0x%x", txn->mc,
>> +					txn->mt);
>> +		ret = -ETIMEDOUT;
>> +	}
>> +
>> +	return ret;
>> +
>> +}
>> +
>> +static void qcom_slim_rxwq(struct work_struct *work)
>> +{
>> +	u8 buf[SLIM_MSGQ_BUF_LEN];
>> +	u8 mc, mt, len;
>> +	int i, ret;
>> +	struct qcom_slim_ctrl *ctrl = container_of(work, struct qcom_slim_ctrl,
>> +						 wd);
>> +
>> +	while ((slim_get_current_rxbuf(ctrl, buf)) != -ENODATA) {
>> +		len = SLIM_HEADER_GET_RL(buf[0]);
>> +		mt = SLIM_HEADER_GET_MT(buf[0]);
>> +		mc = SLIM_HEADER_GET_MC(buf[1]);
>> +		if (mt == SLIM_MSG_MT_CORE &&
>> +			mc == SLIM_MSG_MC_REPORT_PRESENT) {
>> +			u8 laddr;
>> +			struct slim_eaddr ea;
>> +			u8 e_addr[6];
>> +
>> +			for (i = 0; i < 6; i++)
>> +				e_addr[i] = buf[7-i];
>> +
>> +			ea.manf_id = (u16)(e_addr[5] << 8) | e_addr[4];
>> +			ea.prod_code = (u16)(e_addr[3] << 8) | e_addr[2];
>> +			ea.dev_index = e_addr[1];
>> +			ea.instance = e_addr[0];
> 
> If we are just bitshifting this out of the bytes does it really
> make it much more clear to reverse the byte order first? Feels
> like you might as well shift it out of buf directly.
> 
> Also we didn't bother to reverse the bytes for the element code
> above, so feels more consistent.
I will try Jonathan Neuschäfer Suggestion to simplify this area of code.