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

[Suman Anna <s-anna@ti.com>]

On 05/27/2014 11:20 PM, Jassi Brar wrote:
> On Wed, May 21, 2014 at 10:57 PM, Mark Brown <broonie@kernel.org> wrote:
>> 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.
>>
> The client wants the call to be blocking. Out of 'zero', 'infinity'
> and some 'valid' delay, it makes better sense to have 'infinity' than
> zero or another value that might be valid for some platform. I assume
> 1hr to be 'infinity', though I am open to better suggestions. Maybe
> put a WARN() ?
> 
> 
>>> +     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.
>>
> OK.
> 
> 
> BTW, I have not converted Highbank's PL320 and OMAP's controller and
> client drivers. I believe Highbank's can't be converted to DT now and
> Suman would want to convert the OMAP himself.

Yes, I will get to this next week, especially as there are new SoCs like
DRA7 and AM437x that need special handling.

regards,
Suman




> 
> Also, maybe mailbox patches could be upstreamed via, say, arm-soc tree?
> 
> Regards,
> Jassi
>