Re: [PATCHv3 2/6] mailbox: Introduce a new common API

[Jassi Brar]

On Tue, Feb 18, 2014 at 11:00 PM, Bjorn Andersson <bjorn@xxxxxxx> wrote:
> On Mon, Feb 17, 2014 at 11:06 PM, Jassi Brar <jaswinder.singh@xxxxxxxxxx> wrote:
>> Hi Courtney,
>>
>> On 18 February 2014 06:22, Courtney Cavin <courtney.cavin@xxxxxxxxxxxxxx> wrote:
>>> On Sat, Feb 15, 2014 at 07:25:27PM +0100, Jassi Brar wrote:
> [...]
>>>> +struct ipc_client {
>>>
>>> I'm not so sure about the naming scheme here.  This header is
>>> mailbox_client.h, and drivers are expected to go in drivers/mailbox, yet the
>>> structs and functions have an ipc_ prefix.  Please standardize this,
>>> preferably to mbox_ or mailbox_.
>>>
>> Yeah Loic already pointed that out.
>> The term 'mailbox' is specific to what ARM calls the IPC links in
>> their manuals. Perhaps we should rename the directory to drivers/ipc/.
>>  'Mailbox' is too symbolic. Though I am OK either way.
>
> In this world IPC means "Inter-process communication", so I'm afraid it's taken.
>
In this very world IPC also stands for "Inter-Processor-Communication" :)
But OK let's call it mailbox.

> [...]
>>>> +request_token_t ipc_send_message(void *channel, void *mssg);
>>>
>>> In the case of the blocking use-case, the function's return type is odd.
>>> Perhaps, instead of my recommendation above, this could be turned into
>>> two separate functions: ipc_send_message() and ipc_send_message_async()?
>>>
>> In blocking use case, the call will return success (a valid token).
>> The client sees a valid token and understands that the request was
>> accepted. And because the client asked it to be a blocking call, it
>> also understands the TX is successfully completed.  What's odd here?
>>
>
> I've tried to figure out why you return your magic type here, from
> what I can see it's just indicating to the consumer if the tx
> succeeded (and maybe was put on the fifo?). When constructing this
> number you have a comment that says "aid for debugging", which
> indicates that it really doesn't hold any value...
>
> There is already a convention for this in the kernel, return
> descriptive negative numbers on failures; 0 on success.
>
Courtney isn't talking about '0 as error code' here... which I said I
am ok making that a negative value.

>
> How do you plan to support probe deferral here?
>
What exactly do you mean? Aren't the drivers' probe supposed to do that?

> What is the plan to support references in Device Tree?
>
As I said right now channel assignment via DT isn't there. Nothing against that.

>>> Although I will admit that I didn't go looking at random code on the
>>> internet before submitting my series, I did take a look at the previous
>>> iterations of this submitted for upstream.
>>>
>> But you objections (except cosmetic ones) sound like you are not aware
>> of all use-cases that have been taken care of.
>
> Your argument in the discussion of Courtneys proposal was "I have code
> that depends on my code, let me show it to you first".
>
> So please enlighten us on what use cases it is that you do support;
> that are actually seen in real life.
>
Please read my last reply... I have mentioned many use-cases. All
those are supported and more. Courtney's asking and my giving
use-cases suggests Courtney wasn't even aware such cases existed.
Again, please read up the old threads.

  I, Suman Anna and Loic went through a lot of exchanges to cover
'all' variations in each step of inter-processor-communication. For
example some controllers have TX-Done irq, some have RX-Done irq, some
have both and some neither. Some clients want blocking and some async
xfer. Shared channel vs exclusive ownership support etc. No wonder the
last two platforms to join us found the api just worked for them.
  Please point me to your mailbox controller's manual, and I am sure I
can cook some code for you atleast as good as for your api. And I
haven't yet even got into reviewing Courtney's implementation. It is
sad it has come to yours-versus-mine ... Courtney should have picked
up our original implementation, which has the juice of long
discussions, and maybe 'fixed' it to taste and submitted.

-Jassi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/