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@xxxxxxxxxx wrote:
> > > From: Sagar Dharia <sdharia@xxxxxxxxxxxxxx>
[...]
> > > +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
Attachment:
signature.asc
Description: PGP signature
