linux-kernel - Re: [PATCH 1/2] mailbox: samsung: added support for samsung mailbox

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKrE-KcMMtFT-aL8wFeKiFkr1LCnsvcewHGK1WYSP+HnynO61A@mail.gmail.com>
Date:	Wed, 19 Mar 2014 09:19:36 +0530
From:	Girish KS <girishks2000@...il.com>
To:	Joe Perches <joe@...ches.com>
Cc:	Girish K S <ks.giri@...sung.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>, devicetree@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Jassi Singh <jassisinghbrar@...il.com>,
	"Anna, Suman" <s-anna@...com>, Ilho Lee <ilho215.lee@...sung.com>
Subject: Re: [PATCH 1/2] mailbox: samsung: added support for samsung mailbox

On Tue, Mar 18, 2014 at 4:26 PM, Joe Perches <joe@...ches.com> 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@...sung.com> 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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/