On Tue, Dec 09, 2008 at 05:29:49PM -0800, David Brownell wrote:
> On Tuesday 09 December 2008, Felipe Balbi wrote:
> > +struct davinci_nand_info {
> > + /* device lock */
> > + spinlock_t lock;
> > + struct nand_chip chip;
> > + struct mtd_info mtd;
> >
> > -/*
> > - * MTD structure for DaVinici board
> > - */
> > -static struct mtd_info *nand_davinci_mtd;
> > + struct device *dev;
> > + struct clk *clk;
> > +
> > + void __iomem *base;
> > + void __iomem *vaddr;
> > +};
> > +
> > +#define to_davinci_nand(m) container_of(m, struct davinci_nand_info, mtd)
>
> Minor nit: put "mtd" first and this becomes a NOP by the
> time code generation completes, so code is a bit smaller
> and register pressure is a bit less.
didn't quite get it. You mean:
#define to_davinci_nand(mtd) container_of(mtd, struct davinci_nand_info, mtd)
or something else ?!?
> Could you briefly explain why the spinlock is needed?
> Just for the HW ECC support, or is there more to it?
>
> Most NAND drivers don't use them internally ... if
> this really needs a spinlock, say why. I suspect
> that you don't need one either, or at least you don't
> need one everywhere it's now used.
Yeah, think it's not necessary. Will try it out without it and see how
it behaves.
> > @@ -287,7 +319,7 @@ static int nand_davinci_memory_bbt(struct mtd_info
> > *mtd, from += blocksize;
> > }
> >
> > - printk(KERN_NOTICE "Bad block scan: %d out of %d blocks are bad.\n",
> > + dev_dbg(info->dev, "Bad block scan: %d out of %d blocks are bad.\n",
> > mtd->ecc_stats.badblocks, numblocks>>1);
> > return 0;
>
> I'd think that should be dev_notice(). And ... dev_*() changes do NOT
> relate to $SUBJECT. Such messaging patches would better be split out
> from this patch... which is already kind of large.
Sure, will split a that out ;-)
> > @@ -408,10 +451,13 @@ static void nand_davinci_set_eccbytes(struct
> > nand_chip *chip)
> > #endif
> > }
> >
> > -static void __devinit nand_davinci_flash_init(void)
> > +static void __devinit nand_davinci_flash_init(struct davinci_nand_info
> > *info) {
> > + unsigned long flags;
> > u32 regval, tmp;
> >
> > + spin_lock_irqsave(&info->lock, flags);
> > +
> > /* Check for correct pin mux, reconfigure if necessary */
> > tmp = davinci_readl(DAVINCI_SYSTEM_MODULE_BASE + PINMUX0);
>
> Didn't you say you were wondering why this driver misbhaves on dm355?
>
> That's part of the clue! PINMUX0 on dm355 is for video stuff; I
> think it's PINMUX2 that's for AEMIF. This driver is assuming that
> it's on a dm6446, I bet. (Not that it should ever do pinmuxing
> directly, in any case...)
>
> There's work afoot to generalize the pinmux stuff; not sure when
> it'll arrive with dm355 support.
Great catch :-)
I think muxing should be done by board support, not driver. Didn't
remove not to break what was already there. Hah, might be that running
this driver, breaks video then :-p
That's fun.
--
balbi
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
