Re: [PATCH 1/2] port ndfc driver to of platform

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 14 August 2008, Sean MacLennan wrote:
> On Thu, 14 Aug 2008 11:53:07 +0200
> "Arnd Bergmann" <arnd@arndb.de> wrote:
> 
> > > +	ndfc->ndfcbase = ioremap(reg[1], reg[2]);
> > 
> > This could be better expressed as of_iomap().
> 
> I tried of_iomap(), but it doesn't seem to like the 3 value reg. i.e.
> It doesn't skip the chip select. And since I need to read the reg
> property to get the chip select any way, I just used the value directly.

If of_iomap and of_address_to_resource don't work properly, there
is probably something wrong in your device tree, maybe an incorrect
#size-cells or #address-cells or ranges property in one of
the parents. You need to fix this anyway.

> > > -	platform_set_drvdata(pdev, ndfc);
> > > +	__raw_writel(ccr, ndfc->ndfcbase + NDFC_CCR);
> > >  
> > > -	printk("NDFC NAND Driver initialized. Chip-Rev: 0x%08x\n",
> > > -	       __raw_readl(ndfc->ndfcbase + NDFC_REVID));
> > > +	/* Set the bank settings */
> > > +	reg = of_get_property(ofdev->node, "bank_settings", NULL);
> > > +	bank_settings = reg ? *reg : 0x80002222;
> > 
> > Your device tree does have a bank_setting, so why not assume that
> > all others will have it as well? I would remove the default.
> 
> I am thinking of making the bank settings an optional value. I assume
> most people with 44x chips with NAND will be using u-boot. If you
> enable NAND in u-boot, it should configure the bank settings for you.
> 
> I put the bank setting in my dts just to show a "complete"
> configuration.

Ok, I guess that in the absence of the property, you should not do anything
with this value then, rather than assuming a default.

	Arnd <><