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

[Srinivas Kandagatla]

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>
> >
> > Slimbus devices use value-element, and information elements to
> > control device parameters (e.g. value element is used to represent
> > gain for codec, information element is used to represent interrupt
> > status for codec when codec interrupt fires).
> > Messaging APIs are used to set/get these value and information
> > elements. Slimbus specification uses 8-bit "transaction IDs" for
> > messages where a read-value is anticipated. Framework uses a table
> > of pointers to store those TIDs and responds back to the caller in
> > O(1).
> > Caller can opt to do synchronous, or asynchronous reads/writes. For
> > asynchronous operations, the callback will be called from atomic
> > context.
> > TX and RX circular rings are used to allow queuing of multiple
> > transfers per controller. Controller can choose size of these rings
> > based of controller HW implementation. The buffers are coerently
>
> s/based of/based on/
> s/coerently/coherently/
Yep.. will fix this in next version.
> > mapped so that controller can utilize DMA operations for the
> > transactions without remapping every transaction buffer.
> > Statically allocated rings help to improve performance by avoiding
> > overhead of dynamically allocating transactions on need basis.
> >
> > Signed-off-by: Sagar Dharia <sdharia@xxxxxxxxxxxxxx>
> > Tested-by: Naveen Kaje <nkaje@xxxxxxxxxxxxxx>
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > ---
> [...]
> > +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)?
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.

So we can delete slim_slicecodefromsize() call and function together.
looks like it was a leftover from downstream.

> (Also, what does cur mean? Cursor? Current?)
No Idea!! :-) it is supposed to return slice size as per number of bytes.

> > +
> > +	switch (mc) {
> > +	case SLIM_MSG_MC_REQUEST_CHANGE_VALUE:
> > +	case SLIM_MSG_MC_CHANGE_VALUE:
> > +	case SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION:
> > +	case SLIM_MSG_MC_CLEAR_INFORMATION:
> > +		txn->rl += msg->num_bytes;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	if (slim_tid_txn(txn->mt, txn->mc))
> > +		txn->rl++;
> > +
> > +	return slim_processtxn(ctrl, txn);
> > +}
> > +EXPORT_SYMBOL_GPL(slim_xfer_msg);
> [...]
> > +/*
> > + * slim_request_val_element: change and request a given value element

name should be fixed here..
> > + * @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.

> > + */
> > +int slim_request_change_val_element(struct slim_device *sb,
> > +					struct slim_val_inf *msg)
> > +{
> > +	struct slim_controller *ctrl = sb->ctrl;
> > +
> > +	if (!ctrl)
> > +		return -EINVAL;
> > +
> > +	return slim_xfer_msg(ctrl, sb, msg, SLIM_MSG_MC_REQUEST_CHANGE_VALUE);
> > +}
> > +EXPORT_SYMBOL_GPL(slim_request_change_val_element);
> [...]
> > +/**
> > + * struct slim_pending: context of pending transfers
> > + * @cb: callback for this transfer
> > + * @ctx: contex for the callback function
>
> s/contex/context/
Will fix all these instances.
> > + * @need_tid: True if this transfer need Transaction ID
> > + */
> > +struct slim_pending {
> > +	void (*cb)(void *ctx, int err);
> > +	void *ctx;
> > +	bool need_tid;
> > +};
>
>
> Thanks,
> Jonathan NeuschÃfer