linux-kernel - Re: [PATCH v3 1/3] mailbox: Add support for APM X-Gene platform mailbox driver

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]

Date:	Fri, 20 Nov 2015 17:17:16 +0530
From:	Jassi Brar <jassisinghbrar@...il.com>
To:	Duc Dang <dhdang@....com>
Cc:	Paul Bolle <pebolle@...cali.nl>,
	Devicetree List <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	patches <patches@....com>, Feng Kan <fkan@....com>
Subject: Re: [PATCH v3 1/3] mailbox: Add support for APM X-Gene platform
 mailbox driver

On Mon, Nov 9, 2015 at 11:35 PM, Duc Dang <dhdang@....com> wrote:
> X-Gene mailbox controller provides 8 mailbox channels, with
> each channel has a dedicated interrupt line.
>
> [dhdang: rebase over 4.3-rc5, some minor changes to
> address comment in v2 patch set]
>
Do you want this to go into git logs?

> Signed-off-by: Feng Kan <fkan@....com>
> Signed-off-by: Duc Dang <dhdang@....com>
> ---
here is the right place for any history. And it is good practice to
specify whatever changes were made from last revision.


> +
> +struct slimpro_mbox_chan {
> +       struct device *dev;
> +       struct mbox_chan *chan;
> +       void __iomem *reg;
> +       int id;
>
You don't seem to really need 'id'.


> +
> +static void mb_chan_enable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
> +{
> +       u32 val = readl(mb_chan->reg + REG_DB_STATMASK);
> +
> +       val &= ~mask;
> +       writel(val, mb_chan->reg + REG_DB_STATMASK);
> +}
> +
> +static void mb_chan_disable_int(struct slimpro_mbox_chan *mb_chan, u32 mask)
> +{
> +       u32 val = readl(mb_chan->reg + REG_DB_STATMASK);
> +
> +       val |= mask;
> +       writel(val, mb_chan->reg + REG_DB_STATMASK);
> +}
> +
These 2 functions are called just once. And do what other drivers
usually inline. Wouldn't it be better to directly set & clear the bit
when needed?

> +static int slimpro_mbox_send_data(struct mbox_chan *chan, void *msg)
> +{
> +       struct slimpro_mbox_chan *mb_chan =
> +                               (struct slimpro_mbox_chan *)chan->con_priv;
> +
You don't need to typecast a void pointer. Here and elsewhere.


> +
> +       /* Setup mailbox links */
> +       for (i = 0; i < MBOX_CNT; i++) {
> +               ctx->mc[i].irq = platform_get_irq(pdev, i);
>
You expect every platform to provide exactly 'MBOX_CNT' irqs and they
must be different (because you don't 'SHARE' when you request).
Usually developers relax either of the conditions.


> +static struct platform_driver slimpro_mbox_driver = {
> +       .probe  = slimpro_mbox_probe,
> +       .remove = slimpro_mbox_remove,
> +       .driver = {
> +               .name = "xgene-slimpro-mbox",
> +               .owner = THIS_MODULE,
>
No need to set .owner.

Cheers.
--
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/