On Sat, Oct 07, 2017 at 11:24:33AM +0100, Srinivas Kandagatla wrote:
Thanks for the comments.
On 07/10/17 07:42, Jonathan Neuschäfer wrote:
Hi,
On Fri, Oct 06, 2017 at 05:51:31PM +0200, srinivas.kandagatla@linaro.org wrote:
From: Sagar Dharia sdharia@codeaurora.org
[...]
+int slim_xfer_msg(struct slim_controller *ctrl,
struct slim_device *sbdev, struct slim_val_inf *msg,u8 mc)+{
- DEFINE_SLIM_LDEST_TXN(txn_stack, mc, 6, sbdev->laddr, msg);
- struct slim_msg_txn *txn = &txn_stack;
- int ret;
- u16 sl, cur;
- ret = slim_val_inf_sanity(ctrl, msg, mc);
- if (ret)
return ret;- sl = slim_slicesize(msg->num_bytes);
- dev_dbg(&ctrl->dev, "SB xfer msg:os:%x, len:%d, MC:%x, sl:%x\n",
msg->start_offset, msg->num_bytes, mc, sl);- cur = slim_slicecodefromsize(sl);
- txn->ec = ((sl | (1 << 3)) | ((msg->start_offset & 0xFFF) << 4));
Shouldn't this be (cur | (1 << 3)?
I misread the code here: I thought that cur was assigned the "compressed" message length (in the range 0..7) here, but that's not true, as slim_slicecodefromsize returns an "uncompressed" number. Thus cur is a "quantized"[1] version of msg->num_bytes.
cur seems to be redundant TBH, the only difference between cur and sl is that the slim_slicesize() can give slice size to program for any lengths between 1-16 bytes. However the slim_slicecodefromsize() can only handle 1,2,3,4, 6,8,12,16 byte sizes.
In any case, cur is only assigned and not used, as the code currently is.
So we can delete slim_slicecodefromsize() call and function together. looks like it was a leftover from downstream.
I agree. I don't know how it *might* be used, because I haven't read the SLIMbus spec, but it is unused here.
(Also, what does cur mean? Cursor? Current?)
No Idea!! :-) it is supposed to return slice size as per number of bytes.
Another problem solved by deleting slim_slicecodefromsize :-)
(As a small side-note, I think slim_slicesize and slim_slicecodefromsize are named backwards: I would call sl, as used above, a "slice code", because it encodes the message length)
+/*
- slim_request_val_element: change and request a given value element
name should be fixed here..
Good catch.
- @sb: client handle requesting elemental message reads, writes.
- @msg: Input structure for start-offset, number of bytes to write.
- context: can sleep
- Returns:
- -EINVAL: Invalid parameters
- -ETIMEDOUT: If transmission of this message timed out (e.g. due to bus lines
- not being clocked or driven by controller)
- -ENOTCONN: If the transmitted message was not ACKed by destination device.
Does rbuf contain the old value after this function finishes?
Yep, device should send a reply value with the old value with matching tid.
I think you should document this in the comment to help readers.
Thanks, Jonathan Neuschäfer