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

[Bjorn Andersson]

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.

[...]
>>> +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.


How do you plan to support probe deferral here?

What is the plan to support references in Device Tree?

[...]
>> 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.

Regards,
Bjorn
--
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/