Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965527AbbEMTpk (ORCPT ); Wed, 13 May 2015 15:45:40 -0400 Received: from mail-pa0-f53.google.com ([209.85.220.53]:35340 "EHLO mail-pa0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965380AbbEMTpg (ORCPT ); Wed, 13 May 2015 15:45:36 -0400 Date: Wed, 13 May 2015 12:45:21 -0700 From: Brian Norris To: Arnd Bergmann Cc: linux-mtd@lists.infradead.org, Dmitry Torokhov , Anatol Pomazao , Ray Jui , Corneliu Doban , Jonathan Richardson , Scott Branden , Florian Fainelli , =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , bcm-kernel-feedback-list@broadcom.com, Dan Ehrenberg , Gregory Fong , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Kevin Cernekee Subject: Re: [PATCH v4 08/11] mtd: brcmnand: add BCM63138 support Message-ID: <20150513194521.GF11598@ld-irv-0074> References: <1431478424-29230-1-git-send-email-computersforpeace@gmail.com> <1431478424-29230-9-git-send-email-computersforpeace@gmail.com> <7133995.DH7aRmliCR@wuerfel> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7133995.DH7aRmliCR@wuerfel> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3148 Lines: 100 On Wed, May 13, 2015 at 12:49:01PM +0200, Arnd Bergmann wrote: > On Tuesday 12 May 2015 17:53:41 Brian Norris wrote: > > +static bool bcm63138_nand_intc_ack(struct brcmnand_soc *soc) > > +{ > > + struct bcm63138_nand_soc_priv *priv = soc->priv; > > + void __iomem *mmio = priv->base + BCM63138_NAND_INT_STATUS; > > + u32 val = brcmnand_readl(mmio); > > + > > + if (val & BCM63138_CTLRDY) { > > + brcmnand_writel(val & ~BCM63138_CTLRDY, mmio); > > + return true; > > + } > > + > > + return false; > > +} > ... > > +static int bcm63138_nand_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct bcm63138_nand_soc_priv *priv; > > + struct brcmnand_soc *soc; > > + struct resource *res; > > + > > + soc = devm_kzalloc(dev, sizeof(*soc), GFP_KERNEL); > > + if (!soc) > > + return -ENOMEM; > > + > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > This is a slightly unconventional method of doing the abstraction. > For consistency with a lot of other drivers, I'd do it like this: > > struct bcm63138_controller { > void __iomem *base; > brcmnand_controller parent; > }; Does it really make sense to publicize all of the brcmnand_controller details to each of the constituent drivers? I was intentionally keeping them private, with a very small and well-defined interface provided for shim SoC drivers. This is kind of a problem that has plagued the wider MTD (and esp. NAND) subsystem in general; we expose a ton of details to low-level drivers, and they're free to muck with things however they want, as long as it ends up working. I'd rather be more intentional in what I expose. > static bool bcm63138_nand_intc_ack(struct brcmnand_controller *parent) > { > struct bcm63138_controller *controller; > controller = container_of(parent, struct brcmnand_controller, parent); > > ... > } > > static int bcm63138_nand_probe(...) > { > struct bcm63138_controller *controller; > > controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL); > ... > return brcmnand_probe(pdev, &controller->parent); > } > > This also simplifies the probe() functions and means less pointer chasing. I could still avoid one pointer chase and one extra memory allocation by embedding 'struct brcmnand_soc' in a 'struct bcm63138_nand_soc'. e.g.: struct bcm63138_nand_soc { void __iomem *base; struct brcmnand_soc soc; }; static bool bcm63138_nand_intc_ack(struct brcmnand_soc *soc) { struct bcm63138_nand_soc *priv; priv = container_of(soc, struct bcm63138_nand_soc, soc); ... } static int bcm63138_nand_probe(...) { struct bcm63138_nand_soc *priv; priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); ... return brcmnand_probe(pdev, &priv->soc); } Brian -- 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/