Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1162562AbbKTLrT (ORCPT ); Fri, 20 Nov 2015 06:47:19 -0500 Received: from mail-ig0-f180.google.com ([209.85.213.180]:32964 "EHLO mail-ig0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934126AbbKTLrQ (ORCPT ); Fri, 20 Nov 2015 06:47:16 -0500 MIME-Version: 1.0 In-Reply-To: References: <1429863912.2927.43.camel@x220> Date: Fri, 20 Nov 2015 17:17:16 +0530 Message-ID: Subject: Re: [PATCH v3 1/3] mailbox: Add support for APM X-Gene platform mailbox driver From: Jassi Brar To: Duc Dang Cc: Paul Bolle , Devicetree List , "linux-arm-kernel@lists.infradead.org" , Linux Kernel Mailing List , patches , Feng Kan Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2462 Lines: 80 On Mon, Nov 9, 2015 at 11:35 PM, Duc Dang 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 > Signed-off-by: Duc Dang > --- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/