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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
