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

[Stephen Rothwell]

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/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20080519/5a480695/attachment.pgp>