Hi Suman, [Resending with mailing-list CCed this time. For some reason LKML doesn't appear in reply-to despite this message being fwd'ed from there]
On Wed, Mar 13, 2013 at 8:53 AM, Suman Anna <s-a...@ti.com> wrote: > Hi, > Please find the updated mailbox patch series for pulling into linux-next. > The series is rebased on top of 3.9-rc2, and includes one new patch to > rename an existing mailbox.h added as part of the highbank cpufreq > support for 3.9 merge window [1]. > I am to implement IPC protocol and client drivers on a platform that has a master processor having control over critical h/w resources, like clock and power management, to be used by the MPCore running linux. And there are 2 such IPC controllers - one for communication within the MPCore and another for between MPCore and the 'Master'. I have a few concerns about this api as follows... a) No documentation. Usually the header would have proper documentation of data structures and info for users of both side of the api. b) The api is not scalable. The assumption of just one IPC controller in the platform is hardcoded. c) There seems to be no consistent design - mailbox_ops has 12 callback hooks. Some, save_ctx, restore_ctx, enable_irq, disable_irq are exported for no apparent reason (legacy of a legacy - OMAP), while other hooks are kept private to the api. I believe OMAP had valid reasons to make IPC clients save/restore context of the IPC controller, but I don't think that is the case in general. I2C client drivers don't save/restore context of I2C controller's, why should IPC's? Similarly enable/disable_irq of the controller seem too intrusive for a client driver. mailbox_ops.mailbox_type_t makes no sense to me. At least not without documentation, though I doubt any amount of documentation could help it - mailbox.c doesn't even read the member. Btw, isn't every mailbox a MBOX_SHARED_MEM_TYPE? A part of data passed via FIFO/REG could point to location in shared memory that might have full command/payload for the message. Or did I get the meaning of MBOX_SHARED_MEM_TYPE wrong in the absence of documentation? The api seems to worry too much about the low-level details of the IPC controller (save/restore context, enable/disable_irq and ack_irq), while it fails to provide a tight well-defined interface to client drivers. There are also some code issues, which might come as replies to respective patches. Regards, -Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
