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.