Re: [Patch v6 2/7] slimbus: Add messaging APIs to slimbus framework

["Jonathan Neuschäfer" <j.neuschaefer@gmx.net>]

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]