Re: [PATCHv5 2/4] mailbox: Introduce framework for mailbox

[Mark Brown <broonie@kernel.org>]

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

On Thu, May 15, 2014 at 11:41:00AM +0530, Jassi Brar wrote:
> Introduce common framework for client/protocol drivers and
> controller drivers of Inter-Processor-Communication (IPC).

This looks pretty nice, though I do have a few *very* small nits beyond
those Arnd had.

> +	if (chan->cl->tx_block && chan->active_req) {
> +		int ret;
> +		init_completion(&chan->tx_complete);

reinit_completion().

> +	if (!cl->tx_tout) /* wait for ever */
> +		cl->tx_tout = msecs_to_jiffies(3600000);
> +	else
> +		cl->tx_tout = msecs_to_jiffies(cl->tx_tout);

Is the default wait for ever the best timeout - I'm not sure it's best
from a defensiveness point of view.  It should be fine either way,
it's just a matter of taste.

> +	ret = chan->mbox->ops->startup(chan);
> +	if (ret) {
> +		pr_err("Unable to startup the chan\n");

Perhaps print the error codes?  Might be helpful to users.

> +	/* The queued TX requests are simply aborted, no callbacks are made */
> +	spin_lock_irqsave(&chan->lock, flags);
> +	chan->cl = NULL;
> +	chan->active_req = NULL;
> +	if (chan->txdone_method == (TXDONE_BY_POLL | TXDONE_BY_ACK))
> +		chan->txdone_method = TXDONE_BY_POLL;
> +
> +	module_put(chan->mbox->dev->driver->owner);
> +	spin_unlock_irqrestore(&chan->lock, flags);

Is the module_put() safe in atomic context?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]