On Wed, Nov 15, 2017 at 02:10:36PM +0000, srinivas.kandagatla@linaro.org wrote:
+void slim_msg_response(struct slim_controller *ctrl, u8 *reply, u8 tid, u8 len) +{
- struct slim_msg_txn *txn;
- struct slim_val_inf *msg;
- unsigned long flags;
- spin_lock_irqsave(&ctrl->txn_lock, flags);
Do you need this to be a _irqsave variant? What is the context of io transfers in this case
+/**
- slim_do_transfer() - Process a slimbus-messaging transaction
- @ctrl: Controller handle
- @txn: Transaction to be sent over SLIMbus
- Called by controller to transmit messaging transactions not dealing with
- Interface/Value elements. (e.g. transmittting a message to assign logical
- address to a slave device
- Return: -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.
I am preferring ENODATA in SDW for this case, as Slaves didnt respond or ACK.
ENOTCONN is defined as /* Transport endpoint is not connected */ which is not the case here, connected yes but not responded.
+static int slim_val_inf_sanity(struct slim_controller *ctrl,
struct slim_val_inf *msg, u8 mc)+{
- if (!msg || msg->num_bytes > 16 ||
(msg->start_offset + msg->num_bytes) > 0xC00)goto reterr;- switch (mc) {
- case SLIM_MSG_MC_REQUEST_VALUE:
- case SLIM_MSG_MC_REQUEST_INFORMATION:
if (msg->rbuf != NULL)return 0;break;
empty line here and after each break make it look better
- case SLIM_MSG_MC_CHANGE_VALUE:
- case SLIM_MSG_MC_CLEAR_INFORMATION:
if (msg->wbuf != NULL)return 0;break;- case SLIM_MSG_MC_REQUEST_CHANGE_VALUE:
- case SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION:
if (msg->rbuf != NULL && msg->wbuf != NULL)return 0;break;- default:
break;
this seems superflous and we can just fall thru to error below.
- }
+reterr:
- dev_err(ctrl->dev, "Sanity check failed:msg:offset:0x%x, mc:%d\n",
msg->start_offset, mc);- return -EINVAL;
...
+static 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;
- 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);
better to add tracing support for these debug prints
- txn->ec = ((sl | (1 << 3)) | ((msg->start_offset & 0xFFF) << 4));
- 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_do_transfer(ctrl, txn);
+}
+/**
- slim_request_val_element() - request value element
- @sb: client handle requesting elemental message reads, writes.
- @msg: Input structure for start-offset, number of bytes to read.
- context: can sleep
- Return: -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 the device.
- */
+int slim_request_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_VALUE);
+} +EXPORT_SYMBOL_GPL(slim_request_val_element);
+/**
- slim_request_inf_element() - Request a info element
- @sb: client handle requesting elemental message reads.
- @msg: Input structure for start-offset, number of bytes to read
- wbuf will contain information element(s) bit masks to be cleared.
- rbuf will return what the information element value was
- */
+int slim_request_inf_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_INFORMATION);
+} +EXPORT_SYMBOL_GPL(slim_request_inf_element);
+/**
- slim_change_val_element: change a given value element
- @sb: client handle requesting elemental message reads, writes.
- @msg: Input structure for start-offset, number of bytes to write.
- context: can sleep
- Return:
- -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 the device.
- */
+int slim_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_CHANGE_VALUE);
+} +EXPORT_SYMBOL_GPL(slim_change_val_element);
+/**
- slim_clear_inf_element() - Clear info element
- @sb: client handle requesting elemental message reads, writes.
- @msg: Input structure for start-offset, number of bytes to read
- wbuf will contain information element(s) bit masks to be cleared.
- rbuf will return what the information element value was
- */
+int slim_clear_inf_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_CLEAR_INFORMATION);
+} +EXPORT_SYMBOL_GPL(slim_clear_inf_element);
+/**
- slim_request_val_element() - change and request a given value element
- @sb: client handle requesting elemental message reads, writes.
- @msg: Input structure for start-offset, number of bytes to write.
- context: can sleep
- Return:
- -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 the device.
- */
+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);
looking at this, does it really help to have different APIs for SLIM_MSG_XXX why not slim_xfer_msg() be an exported one..
+int slim_write(struct slim_device *sdev, u32 addr, size_t count, u8 *val) +{
- struct slim_val_inf msg;
- int ret;
- slim_fill_msg(&msg, addr, count, val, NULL);
- ret = slim_change_val_element(sdev, &msg);
return slim_change_val_element()
- return ret;
+}
...
+/* Destination type Values */ +#define SLIM_MSG_DEST_LOGICALADDR 0 +#define SLIM_MSG_DEST_ENUMADDR 1 +#define SLIM_MSG_DEST_BROADCAST 3
^^^ why tab here