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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
