[PATCH 2/4 V5] MSI support on 83xx/85xx/86xx board

[Jin Zhengxiong]

Hi Stephen,

Thanks for the comments,  I'd like to clean up some of the
codes by patch after some updates(such as irq_alloc_host you
mentioned).

Jason 
> -----Original Message-----
> From: Stephen Rothwell [mailto:sfr at canb.auug.org.au] 
> Sent: Monday, May 19, 2008 11:34 AM
> To: Jin Zhengxiong
> Cc: galak at kernel.crashing.org; linuxppc-dev at ozlabs.org
> Subject: Re: [PATCH 2/4 V5] MSI support on 83xx/85xx/86xx board
> 
> Hi Jason,
> 
> Just a couple of comments.  All of which you may ignore. :-)
> 
> On Fri, 16 May 2008 17:50:45 +0800 Jason Jin 
> <Jason.jin at freescale.com> wrote:
> >
> > +static int fsl_msi_free_dt_hwirqs(struct fsl_msi *msi) {
> 
> > +	if ((len % 0x8) != 0) {
> 
> why not (len % (2 * sizeof(u32))) ?
> 
> > +	/* Format is: (<u32 start> <u32 count>)+ */
> > +	len /= sizeof(u32);
> > +	len /= 2;
> 
> len /= 2 * sizeof(u32);
> 
> > +	for (i = 0; i < len; i++, p += 2)
> 
> for (len /= 2 * sizeof(u32); len; len--, p += 2)
> 
> These are just style so you can ignore me if you like :-)
> 
> > +static int __devinit fsl_of_msi_probe(struct of_device *dev,
> > +				const struct of_device_id *match) {
> > +	struct fsl_msi *msi;
> > +	struct resource res;
> > +	int err, i, count;
> > +	int rc;
> > +	int virt_msir;
> > +	const u32 *p;
> > +	struct fsl_msi_feature *tmp_data;
> > +
> > +	printk(KERN_DEBUG "Setting up Freescale MSI support\n");
> > +
> > +	msi = kzalloc(sizeof(struct fsl_msi), GFP_KERNEL);
> > +	if (!msi) {
> > +		dev_err(&dev->dev, "No memory for MSI structure\n");
> > +		err = -ENOMEM;
> > +		goto error_out;
> > +	}
> > +
> > +	msi->of_node = dev->node;
> 
> You need to of_node_get dev->node [i.e. msi->of_node = 
> of_node_get(dev->node)] and if you delay this as far as 
> possible, you won't need of_node_put(msi->of_node) in the erro path.
> 
> > +	msi->irqhost = irq_alloc_host(of_node_get(dev->node),
> 
> irq_alloc_host should do the of_node_get if it needs to. 
> Which it doesn't (at the moment), so just leave it as you 
> have it and it will be cleaned up when irq_alloc_host is fixed.
> 
> --
> Cheers,
> Stephen Rothwell                    sfr at canb.auug.org.au
> http://www.canb.auug.org.au/~sfr/
>