Messages in this thread | | | Date | Sat, 7 Oct 2017 14:29:01 +0200 | From | Jonathan Neuschäfer <> | Subject | Re: [Patch v6 2/7] slimbus: Add messaging APIs to slimbus framework |
| |
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
[1]: https://en.wikipedia.org/wiki/Quantization [unhandled content-type:application/pgp-signature] | |