Messages in this thread | | | Subject | Re: [PATCH v7 11/13] slimbus: qcom: Add Qualcomm Slimbus controller driver | From | Srinivas Kandagatla <> | Date | Fri, 24 Nov 2017 14:39:59 +0000 |
| |
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.
| |