Merge remote-tracking branch 'lsk/v3.10/topic/mailbox' into linux-linaro-lsklsk-v3.10-14.08

5 files changed, 235 insertions, 115 deletions

diff --git a/Documentation/devicetree/bindings/mailbox/mailbox.txt b/Documentation/devicetree/bindings/mailbox/mailbox.txt
index 3f009555f392..1a2cd3d266db 100644
--- a/Documentation/devicetree/bindings/mailbox/mailbox.txt
+++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt
@@ -19,15 +19,20 @@ Example:
 * Mailbox Client
 
 Required property:
-- mbox: List of phandle and mailbox channel specifier.
+- mboxes: List of phandle and mailbox channel specifiers.
 
+Optional property:
 - mbox-names: List of identifier strings for each mailbox channel
-		required by the client.
+		required by the client. The use of this property
+		is discouraged in favor of using index in list of
+		'mboxes' while requesting a mailbox. Instead the
+		platforms may define channel indices, in DT headers,
+		to something legible.
 
 Example:
 	pwr_cntrl: power {
 		...
 		mbox-names = "pwr-ctrl", "rpc";
-		mbox = <&mailbox 0
+		mboxes = <&mailbox 0
 			&mailbox 1>;
 	};
diff --git a/Documentation/mailbox.txt b/Documentation/mailbox.txt
new file mode 100644
index 000000000000..60f43ff629aa
--- /dev/null
+++ b/Documentation/mailbox.txt
@@ -0,0 +1,122 @@
+		The Common Mailbox Framework
+		Jassi Brar <jaswinder.singh@linaro.org>
+
+ This document aims to help developers write client and controller
+drivers for the API. But before we start, let us note that the
+client (especially) and controller drivers are likely going to be
+very platform specific because the remote firmware is likely to be
+proprietary and implement non-standard protocol. So even if two
+platforms employ, say, PL320 controller, the client drivers can't
+be shared across them. Even the PL320 driver might need to accommodate
+some platform specific quirks. So the API is meant mainly to avoid
+similar copies of code written for each platform. Having said that,
+nothing prevents the remote f/w to also be Linux based and use the
+same api there. However none of that helps us locally because we only
+ever deal at client's protocol level.
+ Some of the choices made during implementation are the result of this
+peculiarity of this "common" framework.
+
+
+
+	Part 1 - Controller Driver (See include/linux/mailbox_controller.h)
+
+ Allocate mbox_controller and the array of mbox_chan.
+Populate mbox_chan_ops, except peek_data() all are mandatory.
+The controller driver might know a message has been consumed
+by the remote by getting an IRQ or polling some hardware flag
+or it can never know (the client knows by way of the protocol).
+The method in order of preference is IRQ -> Poll -> None, which
+the controller driver should set via 'txdone_irq' or 'txdone_poll'
+or neither.
+
+
+	Part 2 - Client Driver (See include/linux/mailbox_client.h)
+
+ The client might want to operate in blocking mode (synchronously
+send a message through before returning) or non-blocking/async mode (submit
+a message and a callback function to the API and return immediately).
+
+
+struct demo_client {
+	struct mbox_client cl;
+	struct mbox_chan *mbox;
+	struct completion c;
+	bool async;
+	/* ... */
+};
+
+/*
+ * This is the handler for data received from remote. The behaviour is purely
+ * dependent upon the protocol. This is just an example.
+ */
+static void message_from_remote(struct mbox_client *cl, void *mssg)
+{
+	struct demo_client *dc = container_of(mbox_client,
+						struct demo_client, cl);
+	if (dc->aysnc) {
+		if (is_an_ack(mssg)) {
+			/* An ACK to our last sample sent */
+			return; /* Or do something else here */
+		} else { /* A new message from remote */
+			queue_req(mssg);
+		}
+	} else {
+		/* Remote f/w sends only ACK packets on this channel */
+		return;
+	}
+}
+
+static void sample_sent(struct mbox_client *cl, void *mssg, int r)
+{
+	struct demo_client *dc = container_of(mbox_client,
+						struct demo_client, cl);
+	complete(&dc->c);
+}
+
+static void client_demo(struct platform_device *pdev)
+{
+	struct demo_client *dc_sync, *dc_async;
+	/* The controller already knows async_pkt and sync_pkt */
+	struct async_pkt ap;
+	struct sync_pkt sp;
+
+	dc_sync = kzalloc(sizeof(*dc_sync), GFP_KERNEL);
+	dc_async = kzalloc(sizeof(*dc_async), GFP_KERNEL);
+
+	/* Populate non-blocking mode client */
+	dc_async->cl.dev = &pdev->dev;
+	dc_async->cl.rx_callback = message_from_remote;
+	dc_async->cl.tx_done = sample_sent;
+	dc_async->cl.tx_block = false;
+	dc_async->cl.tx_tout = 0; /* doesn't matter here */
+	dc_async->cl.knows_txdone = false; /* depending upon protocol */
+	dc_async->async = true;
+	init_completion(&dc_async->c);
+
+	/* Populate blocking mode client */
+	dc_sync->cl.dev = &pdev->dev;
+	dc_sync->cl.rx_callback = message_from_remote;
+	dc_sync->cl.tx_done = NULL; /* operate in blocking mode */
+	dc_sync->cl.tx_block = true;
+	dc_sync->cl.tx_tout = 500; /* by half a second */
+	dc_sync->cl.knows_txdone = false; /* depending upon protocol */
+	dc_sync->async = false;
+
+	/* ASync mailbox is listed second in 'mboxes' property */
+	dc_async->mbox = mbox_request_channel(&dc_async->cl, 1);
+	/* Populate data packet */
+	/* ap.xxx = 123; etc */
+	/* Send async message to remote */
+	mbox_send_message(dc_async->mbox, &ap);
+
+	/* Sync mailbox is listed first in 'mboxes' property */
+	dc_sync->mbox = mbox_request_channel(&dc_sync->cl, 0);
+	/* Populate data packet */
+	/* sp.abc = 123; etc */
+	/* Send message to remote in blocking mode */
+	mbox_send_message(dc_sync->mbox, &sp);
+	/* At this point 'sp' has been sent */
+
+	/* Now wait for async chan to be done */
+	wait_for_completion(&dc_async->c);
+}
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 607dd91e87a1..9a937ef35068 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -1,7 +1,7 @@
 /*
  * Mailbox: Common code for Mailbox controllers and users
  *
- * Copyright (C) 2014 Linaro Ltd.
+ * Copyright (C) 2013-2014 Linaro Ltd.
  * Author: Jassi Brar <jassisinghbrar@gmail.com>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -17,17 +17,18 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/bitops.h>
 #include <linux/mailbox_client.h>
 #include <linux/mailbox_controller.h>
 
-#define TXDONE_BY_IRQ	(1 << 0) /* controller has remote RTR irq */
-#define TXDONE_BY_POLL	(1 << 1) /* controller can read status of last TX */
-#define TXDONE_BY_ACK	(1 << 2) /* S/W ACK recevied by Client ticks the TX */
+#define TXDONE_BY_IRQ	BIT(0) /* controller has remote RTR irq */
+#define TXDONE_BY_POLL	BIT(1) /* controller can read status of last TX */
+#define TXDONE_BY_ACK	BIT(2) /* S/W ACK recevied by Client ticks the TX */
 
 static LIST_HEAD(mbox_cons);
 static DEFINE_MUTEX(con_mutex);
 
-static int _add_to_rbuf(struct mbox_chan *chan, void *mssg)
+static int add_to_rbuf(struct mbox_chan *chan, void *mssg)
 {
 	int idx;
 	unsigned long flags;
@@ -37,7 +38,7 @@ static int _add_to_rbuf(struct mbox_chan *chan, void *mssg)
 	/* See if there is any space left */
 	if (chan->msg_count == MBOX_TX_QUEUE_LEN) {
 		spin_unlock_irqrestore(&chan->lock, flags);
-		return -ENOMEM;
+		return -ENOBUFS;
 	}
 
 	idx = chan->msg_free;
@@ -54,7 +55,7 @@ static int _add_to_rbuf(struct mbox_chan *chan, void *mssg)
 	return idx;
 }
 
-static void _msg_submit(struct mbox_chan *chan)
+static void msg_submit(struct mbox_chan *chan)
 {
 	unsigned count, idx;
 	unsigned long flags;
@@ -63,10 +64,8 @@ static void _msg_submit(struct mbox_chan *chan)
 
 	spin_lock_irqsave(&chan->lock, flags);
 
-	if (!chan->msg_count || chan->active_req) {
-		spin_unlock_irqrestore(&chan->lock, flags);
-		return;
-	}
+	if (!chan->msg_count || chan->active_req)
+		goto exit;
 
 	count = chan->msg_count;
 	idx = chan->msg_free;
@@ -83,7 +82,7 @@ static void _msg_submit(struct mbox_chan *chan)
 		chan->active_req = data;
 		chan->msg_count--;
 	}
-
+exit:
 	spin_unlock_irqrestore(&chan->lock, flags);
 }
 
@@ -98,13 +97,14 @@ static void tx_tick(struct mbox_chan *chan, int r)
 	spin_unlock_irqrestore(&chan->lock, flags);
 
 	/* Submit next message */
-	_msg_submit(chan);
+	msg_submit(chan);
 
 	/* Notify the client */
+	if (mssg && chan->cl->tx_done)
+		chan->cl->tx_done(chan->cl, mssg, r);
+
 	if (chan->cl->tx_block)
 		complete(&chan->tx_complete);
-	else if (mssg && chan->cl->tx_done)
-		chan->cl->tx_done(chan->cl, mssg, r);
 }
 
 static void poll_txdone(unsigned long data)
@@ -125,15 +125,15 @@ static void poll_txdone(unsigned long data)
 	}
 
 	if (resched)
-		mod_timer(&mbox->poll,
-			jiffies + msecs_to_jiffies(mbox->period));
+		mod_timer(&mbox->poll, jiffies +
+				msecs_to_jiffies(mbox->period));
 }
 
 /**
  * mbox_chan_received_data - A way for controller driver to push data
  *				received from remote to the upper layer.
  * @chan: Pointer to the mailbox channel on which RX happened.
- * @data: Client specific message typecasted as void *
+ * @mssg: Client specific message typecasted as void *
  *
  * After startup and before shutdown any data received on the chan
  * is passed on to the API via atomic mbox_chan_received_data().
@@ -160,7 +160,8 @@ EXPORT_SYMBOL_GPL(mbox_chan_received_data);
 void mbox_chan_txdone(struct mbox_chan *chan, int r)
 {
 	if (unlikely(!(chan->txdone_method & TXDONE_BY_IRQ))) {
-		pr_err("Controller can't run the TX ticker\n");
+		dev_err(chan->mbox->dev,
+		       "Controller can't run the TX ticker\n");
 		return;
 	}
 
@@ -180,7 +181,7 @@ EXPORT_SYMBOL_GPL(mbox_chan_txdone);
 void mbox_client_txdone(struct mbox_chan *chan, int r)
 {
 	if (unlikely(!(chan->txdone_method & TXDONE_BY_ACK))) {
-		pr_err("Client can't run the TX ticker\n");
+		dev_err(chan->mbox->dev, "Client can't run the TX ticker\n");
 		return;
 	}
 
@@ -227,8 +228,6 @@ EXPORT_SYMBOL_GPL(mbox_client_peek_data);
  * is not queued, a negative token is returned. Upon failure or successful
  * TX, the API calls 'tx_done' from atomic context, from which the client
  * could submit yet another request.
- *  In blocking mode, 'tx_done' is not called, effectively making the
- * queue length 1.
  * The pointer to message should be preserved until it is sent
  * over the chan, i.e, tx_done() is made.
  * This function could be called from atomic context as it simply
@@ -245,15 +244,15 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 	if (!chan || !chan->cl)
 		return -EINVAL;
 
-	t = _add_to_rbuf(chan, mssg);
+	t = add_to_rbuf(chan, mssg);
 	if (t < 0) {
-		pr_err("Try increasing MBOX_TX_QUEUE_LEN\n");
+		dev_err(chan->mbox->dev, "Try increasing MBOX_TX_QUEUE_LEN\n");
 		return t;
 	}
 
-	_msg_submit(chan);
+	msg_submit(chan);
 
-	init_completion(&chan->tx_complete);
+	INIT_COMPLETION(chan->tx_complete);
 
 	if (chan->txdone_method	== TXDONE_BY_POLL)
 		poll_txdone((unsigned long)chan->mbox);
@@ -262,7 +261,7 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 		unsigned long wait;
 		int ret;
 
-		if (!chan->cl->tx_tout) /* wait for ever */
+		if (!chan->cl->tx_tout) /* wait forever */
 			wait = msecs_to_jiffies(3600000);
 		else
 			wait = msecs_to_jiffies(chan->cl->tx_tout);
@@ -281,6 +280,7 @@ EXPORT_SYMBOL_GPL(mbox_send_message);
 /**
  * mbox_request_channel - Request a mailbox channel.
  * @cl: Identity of the client requesting the channel.
+ * @index: Index of mailbox specifier in 'mboxes' property.
  *
  * The Client specifies its requirements and capabilities while asking for
  * a mailbox channel. It can't be called from atomic context.
@@ -294,64 +294,42 @@ EXPORT_SYMBOL_GPL(mbox_send_message);
  * Return: Pointer to the channel assigned to the client if successful.
  *		ERR_PTR for request failure.
  */
-struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index)
 {
 	struct device *dev = cl->dev;
 	struct mbox_controller *mbox;
 	struct of_phandle_args spec;
 	struct mbox_chan *chan;
 	unsigned long flags;
-	int count, i, ret;
+	int ret;
 
 	if (!dev || !dev->of_node) {
-		pr_err("%s: No owner device node\n", __func__);
-		return ERR_PTR(-ENODEV);
-	}
-
-	count = of_property_count_strings(dev->of_node, "mbox-names");
-	if (count < 0) {
-		pr_err("%s: mbox-names property of node '%s' missing\n",
-			__func__, dev->of_node->full_name);
+		pr_debug("%s: No owner device node\n", __func__);
 		return ERR_PTR(-ENODEV);
 	}
 
 	mutex_lock(&con_mutex);
 
-	ret = -ENODEV;
-	for (i = 0; i < count; i++) {
-		const char *s;
-
-		if (of_property_read_string_index(dev->of_node,
-						"mbox-names", i, &s))
-			continue;
-
-		if (strcmp(cl->chan_name, s))
-			continue;
-
-		if (of_parse_phandle_with_args(dev->of_node,
-					 "mbox", "#mbox-cells",	i, &spec))
-			continue;
-
-		chan = NULL;
-		list_for_each_entry(mbox, &mbox_cons, node)
-			if (mbox->dev->of_node == spec.np) {
-				chan = mbox->of_xlate(mbox, &spec);
-				break;
-			}
-
-		of_node_put(spec.np);
-
-		if (!chan)
-			continue;
+	if (of_parse_phandle_with_args(dev->of_node, "mboxes",
+				       "#mbox-cells", index, &spec)) {
+		dev_dbg(dev, "%s: can't parse \"mboxes\" property\n", __func__);
+		mutex_unlock(&con_mutex);
+		return ERR_PTR(-ENODEV);
+	}
 
-		ret = -EBUSY;
-		if (!chan->cl && try_module_get(mbox->dev->driver->owner))
+	chan = NULL;
+	list_for_each_entry(mbox, &mbox_cons, node)
+		if (mbox->dev->of_node == spec.np) {
+			chan = mbox->of_xlate(mbox, &spec);
 			break;
-	}
+		}
 
-	if (i == count) {
+	of_node_put(spec.np);
+
+	if (!chan || chan->cl || !try_module_get(mbox->dev->driver->owner)) {
+		dev_dbg(dev, "%s: mailbox not free\n", __func__);
 		mutex_unlock(&con_mutex);
-		return ERR_PTR(ret);
+		return ERR_PTR(-EBUSY);
 	}
 
 	spin_lock_irqsave(&chan->lock, flags);
@@ -361,14 +339,14 @@ struct mbox_chan *mbox_request_channel(struct mbox_client *cl)
 	chan->cl = cl;
 	init_completion(&chan->tx_complete);
 
-	if (chan->txdone_method	== TXDONE_BY_POLL
-			&& cl->knows_txdone)
+	if (chan->txdone_method	== TXDONE_BY_POLL && cl->knows_txdone)
 		chan->txdone_method |= TXDONE_BY_ACK;
+
 	spin_unlock_irqrestore(&chan->lock, flags);
 
 	ret = chan->mbox->ops->startup(chan);
 	if (ret) {
-		pr_err("Unable to startup the chan (%d)\n", ret);
+		dev_err(dev, "Unable to startup the chan (%d)\n", ret);
 		mbox_free_channel(chan);
 		chan = ERR_PTR(ret);
 	}
@@ -406,7 +384,7 @@ EXPORT_SYMBOL_GPL(mbox_free_channel);
 
 static struct mbox_chan *
 of_mbox_index_xlate(struct mbox_controller *mbox,
-				const struct of_phandle_args *sp)
+		    const struct of_phandle_args *sp)
 {
 	int ind = sp->args[0];
 
@@ -420,7 +398,7 @@ of_mbox_index_xlate(struct mbox_controller *mbox,
  * mbox_controller_register - Register the mailbox controller
  * @mbox:	Pointer to the mailbox controller.
  *
- * The controller driver registers its communication chans
+ * The controller driver registers its communication channels
  */
 int mbox_controller_register(struct mbox_controller *mbox)
 {
@@ -445,6 +423,7 @@ int mbox_controller_register(struct mbox_controller *mbox)
 
 	for (i = 0; i < mbox->num_chans; i++) {
 		struct mbox_chan *chan = &mbox->chans[i];
+
 		chan->cl = NULL;
 		chan->mbox = mbox;
 		chan->txdone_method = txdone;
@@ -463,7 +442,7 @@ int mbox_controller_register(struct mbox_controller *mbox)
 EXPORT_SYMBOL_GPL(mbox_controller_register);
 
 /**
- * mbox_controller_unregister - UnRegister the mailbox controller
+ * mbox_controller_unregister - Unregister the mailbox controller
  * @mbox:	Pointer to the mailbox controller.
  */
 void mbox_controller_unregister(struct mbox_controller *mbox)
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index 955f3d7641e8..307d9cab2026 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014 Linaro Ltd.
+ * Copyright (C) 2013-2014 Linaro Ltd.
  * Author: Jassi Brar <jassisinghbrar@gmail.com>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -11,36 +11,36 @@
 #define __MAILBOX_CLIENT_H
 
 #include <linux/of.h>
+#include <linux/device.h>
 
 struct mbox_chan;
 
 /**
  * struct mbox_client - User of a mailbox
  * @dev:		The client device
- * @chan_name:		The "controller:channel" this client wants
- * @rx_callback:	Atomic callback to provide client the data received
- * @tx_done:		Atomic callback to tell client of data transmission
  * @tx_block:		If the mbox_send_message should block until data is
  *			transmitted.
  * @tx_tout:		Max block period in ms before TX is assumed failure
- * @knows_txdone:	if the client could run the TX state machine. Usually
+ * @knows_txdone:	If the client could run the TX state machine. Usually
  *			if the client receives some ACK packet for transmission.
  *			Unused if the controller already has TX_Done/RTR IRQ.
+ * @rx_callback:	Atomic callback to provide client the data received
+ * @tx_done:		Atomic callback to tell client of data transmission
  */
 struct mbox_client {
 	struct device *dev;
-	const char *chan_name;
-	void (*rx_callback)(struct mbox_client *cl, void *mssg);
-	void (*tx_done)(struct mbox_client *cl, void *mssg, int r);
 	bool tx_block;
 	unsigned long tx_tout;
 	bool knows_txdone;
+
+	void (*rx_callback)(struct mbox_client *cl, void *mssg);
+	void (*tx_done)(struct mbox_client *cl, void *mssg, int r);
 };
 
-struct mbox_chan *mbox_request_channel(struct mbox_client *cl);
+struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index);
 int mbox_send_message(struct mbox_chan *chan, void *mssg);
-void mbox_client_txdone(struct mbox_chan *chan, int r);
-bool mbox_client_peek_data(struct mbox_chan *chan);
-void mbox_free_channel(struct mbox_chan *chan);
+void mbox_client_txdone(struct mbox_chan *chan, int r); /* atomic */
+bool mbox_client_peek_data(struct mbox_chan *chan); /* atomic */
+void mbox_free_channel(struct mbox_chan *chan); /* may sleep */
 
 #endif /* __MAILBOX_CLIENT_H */
diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
index 5d1915b9af60..9ee195b02444 100644
--- a/include/linux/mailbox_controller.h
+++ b/include/linux/mailbox_controller.h
@@ -8,31 +8,38 @@
 #define __MAILBOX_CONTROLLER_H
 
 #include <linux/of.h>
+#include <linux/types.h>
+#include <linux/timer.h>
+#include <linux/device.h>
+#include <linux/completion.h>
 
 struct mbox_chan;
 
 /**
- * struct mbox_chan_ops - s/w representation of a communication chan
+ * struct mbox_chan_ops - methods to control mailbox channels
  * @send_data:	The API asks the MBOX controller driver, in atomic
  *		context try to transmit a message on the bus. Returns 0 if
  *		data is accepted for transmission, -EBUSY while rejecting
  *		if the remote hasn't yet read the last data sent. Actual
  *		transmission of data is reported by the controller via
  *		mbox_chan_txdone (if it has some TX ACK irq). It must not
- *		block.
+ *		sleep.
  * @startup:	Called when a client requests the chan. The controller
  *		could ask clients for additional parameters of communication
  *		to be provided via client's chan_data. This call may
  *		block. After this call the Controller must forward any
  *		data received on the chan by calling mbox_chan_received_data.
+ *		The controller may do stuff that need to sleep.
  * @shutdown:	Called when a client relinquishes control of a chan.
- *		This call may block too. The controller must not forwared
+ *		This call may block too. The controller must not forward
  *		any received data anymore.
+ *		The controller may do stuff that need to sleep.
  * @last_tx_done: If the controller sets 'txdone_poll', the API calls
  *		  this to poll status of last TX. The controller must
  *		  give priority to IRQ method over polling and never
  *		  set both txdone_poll and txdone_irq. Only in polling
  *		  mode 'send_data' is expected to return -EBUSY.
+ *		  The controller may do stuff that need to sleep/block.
  *		  Used only if txdone_poll:=true && txdone_irq:=false
  * @peek_data: Atomic check for any received data. Return true if controller
  *		  has some data to push to the client. False otherwise.
@@ -46,11 +53,11 @@ struct mbox_chan_ops {
 };
 
 /**
- * struct mbox_controller - Controller of a class of communication chans
+ * struct mbox_controller - Controller of a class of communication channels
  * @dev:		Device backing this controller
- * @controller_name:	Literal name of the controller.
  * @ops:		Operators that work on each communication chan
- * @chans:		Null terminated array of chans.
+ * @chans:		Array of channels
+ * @num_chans:		Number of channels in the 'chans' array.
  * @txdone_irq:		Indicates if the controller can report to API when
  *			the last transmitted data was read by the remote.
  *			Eg, if it has some TX ACK irq.
@@ -59,6 +66,10 @@ struct mbox_chan_ops {
  *			no interrupt rises. Ignored if 'txdone_irq' is set.
  * @txpoll_period:	If 'txdone_poll' is in effect, the API polls for
  *			last TX's status after these many millisecs
+ * @of_xlate:		Controller driver specific mapping of channel via DT
+ * @poll:		API private. Used to poll for TXDONE on all channels.
+ * @period:		API private. Polling period.
+ * @node:		API private. To hook into list of controllers.
  */
 struct mbox_controller {
 	struct device *dev;
@@ -69,14 +80,10 @@ struct mbox_controller {
 	bool txdone_poll;
 	unsigned txpoll_period;
 	struct mbox_chan *(*of_xlate)(struct mbox_controller *mbox,
-					const struct of_phandle_args *sp);
-	/*
-	 * If the controller supports only TXDONE_BY_POLL,
-	 * this timer polls all the links for txdone.
-	 */
+				      const struct of_phandle_args *sp);
+	/* Internal to API */
 	struct timer_list poll;
 	unsigned period;
-	/* Hook to add to the global controller list */
 	struct list_head node;
 };
 
@@ -84,38 +91,45 @@ struct mbox_controller {
  * The length of circular buffer for queuing messages from a client.
  * 'msg_count' tracks the number of buffered messages while 'msg_free'
  * is the index where the next message would be buffered.
- * We shouldn't need it too big because every transferr is interrupt
+ * We shouldn't need it too big because every transfer is interrupt
  * triggered and if we have lots of data to transfer, the interrupt
  * latencies are going to be the bottleneck, not the buffer length.
  * Besides, mbox_send_message could be called from atomic context and
  * the client could also queue another message from the notifier 'tx_done'
  * of the last transfer done.
- * REVIST: If too many platforms see the "Try increasing MBOX_TX_QUEUE_LEN"
+ * REVISIT: If too many platforms see the "Try increasing MBOX_TX_QUEUE_LEN"
  * print, it needs to be taken from config option or somesuch.
  */
 #define MBOX_TX_QUEUE_LEN	20
 
+/**
+ * struct mbox_chan - s/w representation of a communication chan
+ * @mbox:		Pointer to the parent/provider of this channel
+ * @txdone_method:	Way to detect TXDone chosen by the API
+ * @cl:			Pointer to the current owner of this channel
+ * @tx_complete:	Transmission completion
+ * @active_req:		Currently active request hook
+ * @msg_count:		No. of mssg currently queued
+ * @msg_free:		Index of next available mssg slot
+ * @msg_data:		Hook for data packet
+ * @lock:		Serialise access to the channel
+ * @con_priv:		Hook for controller driver to attach private data
+ */
 struct mbox_chan {
-	struct mbox_controller *mbox; /* Parent Controller */
+	struct mbox_controller *mbox;
 	unsigned txdone_method;
-
-	/* client */
 	struct mbox_client *cl;
 	struct completion tx_complete;
-
 	void *active_req;
 	unsigned msg_count, msg_free;
 	void *msg_data[MBOX_TX_QUEUE_LEN];
-	/* Access to the channel */
-	spinlock_t lock;
-
-	/* Private data for controller */
+	spinlock_t lock; /* Serialise access to the channel */
 	void *con_priv;
 };
 
-int mbox_controller_register(struct mbox_controller *mbox);
-void mbox_chan_received_data(struct mbox_chan *chan, void *data);
-void mbox_chan_txdone(struct mbox_chan *chan, int r);
-void mbox_controller_unregister(struct mbox_controller *mbox);
+int mbox_controller_register(struct mbox_controller *mbox); /* can sleep */
+void mbox_controller_unregister(struct mbox_controller *mbox); /* can sleep */
+void mbox_chan_received_data(struct mbox_chan *chan, void *data); /* atomic */
+void mbox_chan_txdone(struct mbox_chan *chan, int r); /* atomic */
 
 #endif /* __MAILBOX_CONTROLLER_H */