[PATCH 1/3 v4] P4080/eLBC: Make Freescale elbc interrupt common to elbc devices

[Zang Roy-R61911]

> -----Original Message-----
> From: Anton Vorontsov [mailto:cbouatmailru at gmail.com]
> Sent: Monday, September 20, 2010 23:37 PM
> To: Zang Roy-R61911
> Cc: linux-mtd at lists.infradead.org; dwmw2 at infradead.org; dedekind1 at gmail.com;
> akpm at linux-foundation.org; Lan Chunhe-B25806; Wood Scott-B07421; Gala Kumar-
> B11780; linuxppc-dev at ozlabs.org
> Subject: Re: [PATCH 1/3 v4] P4080/eLBC: Make Freescale elbc interrupt common
> to elbc devices
> 
> On Fri, Sep 17, 2010 at 03:01:07PM +0800, Roy Zang wrote:

[snip]

> >  int fsl_upm_run_pattern(struct fsl_upm *upm, void __iomem *io_base, u32 mar)
> >  {
> >  	int ret = 0;
> >  	unsigned long flags;
> [...]
> > +static int __devinit fsl_lbc_ctrl_probe(struct platform_device *dev)
> > +{
> > +	int ret;
> > +
> > +	if (!dev->dev.of_node) {
> > +		dev_err(&dev->dev, "Device OF-Node is NULL");
> > +		return -EFAULT;
> > +	}
> > +	fsl_lbc_ctrl_dev = kzalloc(sizeof(*fsl_lbc_ctrl_dev), GFP_KERNEL);
> 
> Btw, the code in the NAND driver checks for !fsl_lbc_ctrl_dev.
> 
> So it might make sense to assign the global variable after the
> struct is fully initialized.
What struct?
assign the global variable in nand driver? 
I'd prefer init it in the lbc code.
There may be other lbc device, who will use this  global variable.

> 
> > +	if (!fsl_lbc_ctrl_dev)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(&dev->dev, fsl_lbc_ctrl_dev);
> > +
> > +	spin_lock_init(&fsl_lbc_ctrl_dev->lock);
> > +	init_waitqueue_head(&fsl_lbc_ctrl_dev->irq_wait);
> > +
> > +	fsl_lbc_ctrl_dev->regs = of_iomap(dev->dev.of_node, 0);
> > +	if (!fsl_lbc_ctrl_dev->regs) {
> > +		dev_err(&dev->dev, "failed to get memory region\n");
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +
> > +	fsl_lbc_ctrl_dev->irq = irq_of_parse_and_map(dev->dev.of_node, 0);
> > +	if (fsl_lbc_ctrl_dev->irq == NO_IRQ) {
> > +		dev_err(&dev->dev, "failed to get irq resource\n");
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +
> > +	fsl_lbc_ctrl_dev->dev = &dev->dev;
> > +
> > +	ret = fsl_lbc_ctrl_init(fsl_lbc_ctrl_dev);
> > +	if (ret < 0)
> > +		goto err;
> > +
> > +	ret = request_irq(fsl_lbc_ctrl_dev->irq, fsl_lbc_ctrl_irq, 0,
> > +				"fsl-lbc", fsl_lbc_ctrl_dev);
> > +	if (ret != 0) {
> > +		dev_err(&dev->dev, "failed to install irq (%d)\n",
> > +			fsl_lbc_ctrl_dev->irq);
> > +		ret = fsl_lbc_ctrl_dev->irq;
> > +		goto err;
> > +	}
> > +
> > +	return 0;
> > +
> > +err:
> > +	iounmap(fsl_lbc_ctrl_dev->regs);
> > +	kfree(fsl_lbc_ctrl_dev);
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id fsl_lbc_match[] = {
> 
> #include <linux/mod_devicetable.h> is needed for this.
> 
> 
> Plus, I think the patch is not runtime bisectable (i.e. you
> now do request_irq() here, but not removing it from the nand
> driver, so nand will fail to probe).
Nand driver does not need to request irq. It will use the irq requested by lbc.
remember, other lbc device may also need to use this registered irq. 
It should not be removed in nand driver.

Thanks.
Roy