On Tue, Mar 18, 2014 at 4:26 PM, Joe Perches <joe@xxxxxxxxxxx> wrote: > On Tue, 2014-03-18 at 15:52 +0530, Girish KS wrote: >> On Mon, Mar 17, 2014 at 5:33 PM, Girish K S <ks.giri@xxxxxxxxxxx> wrote: > > Hi, just some trivial notes about the patch > >> > diff --git a/drivers/mailbox/mailbox-samsung.c b/drivers/mailbox/mailbox-samsung.c > [] >> > @@ -0,0 +1,354 @@ >> > +/* >> > + * Copyright 2013 Samsung Electronics Co. Ltd. >> > + * >> > + * This program is free software; you can redistribute it and/or modify it >> > + * under the terms and conditions of the GNU General Public License, >> > + * version 2, as published by the Free Software Foundation. >> > + * >> > + * This program is distributed in the hope it will be useful, but WITHOUT >> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> > + * more details. >> > + * >> > + * You should have received a copy of the GNU General Public License along with >> > + * this program. If not, see <http://www.gnu.org/licenses/>. >> > + */ > > Please add > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > before any #include so that error messages are > prefixed in the output with "samsung-mailbox: " > and the reader of the log has an idea what subsystem > is emitting messages. > Sure will do it >> > +#include <linux/err.h> >> > +#include <linux/module.h> >> > +#include <linux/slab.h> > [] >> > +static int samsung_mbox_send_data(struct ipc_link *link, void *msg) >> > +{ > [] >> > + /* Lock the mailbox for this transaction with the client id */ >> > + writel(val, ipc_base + MBOX_REG_TXLOCK); >> > + do { >> > + if (time_after(jiffies, timeout)) { >> > + pr_err("cannot aquire the lock\n"); > > This would be prefixed appropriately via pr_fmt. > Also, there's a typo of aquire here: acquire OK > >> > +static int samsung_mbox_startup(struct ipc_link *link, void *ignored) >> > +{ > [] >> > + ret = devm_request_irq(dev, samsung_link->irq, samsung_ipc_handler, >> > + IRQF_SHARED, link->link_name, >> > + samsung_link); >> > + if (unlikely(ret)) { >> > + pr_err("failed to register mailbox interrupt:%d\n", ret); > > Here too it would be prefixed ok > >> > +static int samsung_mbox_probe(struct platform_device *pdev) >> > +{ >> > + struct samsung_mbox *samsung_mbox; >> > + struct samsung_mlink *mbox_link; >> > + struct resource res; >> > + struct ipc_link **link; >> > + int loop, count, ret = 0; >> > + struct device_node *node = pdev->dev.of_node; >> > + >> > + if (!node) { >> > + dev_err(&pdev->dev, "driver doesnt support" >> > + "non-dt devices\n"); > > Please coalesce the format fragments. > > You would notice the missing space between > "support" and "non-dt" ok > > dev_err(&pdev->dev, "driver does not support non-dt devices\n"); > > It's OK to exceed 80 columns for these format strings. > >> > diff --git a/include/linux/mailbox-samsung.h b/include/linux/mailbox-samsung.h > [] >> > +static inline unsigned long get_alligned_buffer(void) > > Typo: There's one l in aligned will do it > >> > +{ >> > + return __get_free_pages(GFP_KERNEL, REQUEST_PAGE_ORDER); >> > +} >> > + >> > +static inline void put_alligned_buffer(unsigned long addr) >> > +{ > > here too > >> > + free_pages(addr, REQUEST_PAGE_ORDER); >> > +} >> > + > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
