thanks for the comments,
On 17/11/17 07:48, Vinod Koul wrote:
On Wed, Nov 15, 2017 at 02:10:36PM +0000, srinivas.kandagatla@xxxxxxxxxx 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
Yes, On Qcom controller driver it is called in Interrupt context, but it
depends on caller (controller driver) which could be in process context too.
+/**
+ * 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.
Isn't that a timeout error then.
ENODATA is for "No data available", reporting ENODATA would be misleading.
ENOTCONN is defined as /* Transport endpoint is not connected */ which is
not the case here, connected yes but not responded.
Code as it is would not return this, so i will be deleting ENOTCONN from
kernel doc.
+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
Yep, will remove this in next version.
+ 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.
Agree..
will fix it in next version.
+ }
+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
Will explore tracing side of it..
+
+ 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:
+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..
I did think about this cleanup, and it makes sense.
I will have a go at removing this and just leaving slim_readb/writeb()
slim_read/write() and slim_xfer_msg() API's in next version.
We can discuss to add this in future incase its required.
+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()
Makes sense.
+
+ 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
Will fix it in next version.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
