lkml.org 
[lkml]   [2017]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 11/13] slimbus: qcom: Add Qualcomm Slimbus controller driver
From
Date
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.

\
 
 \ /
  Last update: 2017-11-24 15:41    [W:0.062 / U:0.468 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site