Re: [PATCH 2/3] (Resend part #1) Add the RapidIO support to powerpcarchitecture with memory mapping support.

[Stephen Rothwell]

Hi,

This is a very large patch.  It may be easier to review if it could be
split on some logical way, that is at all possible (I don't know either
way).  This is just a quick note about some more trivial things.

On Fri, 21 Dec 2007 17:58:43 +0800 Zhang Wei <wei.zhang@xxxxxxxxxxxxx> wrote:
>
> +struct rio_priv {
> +	volatile void __iomem *regs_win;
> +	volatile struct rio_atmu_regs __iomem *atmu_regs;
> +	volatile struct rio_atmu_regs __iomem *maint_atmu_regs;
> +	volatile struct rio_atmu_regs __iomem *dbell_atmu_regs;
> +	volatile void __iomem *dbell_win;
> +	volatile void __iomem *maint_win;
> +	volatile struct rio_msg_regs __iomem *msg_regs;

Paulus has said that any pointer marked __iomem does not need to be
volatile ...

> +static int of_cells_get(struct device_node *np, const char *str)
> +{
> +	struct device_node *tmp = NULL;
> +	const int *var = NULL;

These initializations are unnecessary.

> +	var = of_get_property(np, str, NULL);
> +	tmp = of_get_parent(np);
> +
> +	while (!var && tmp) {
> +		var = (int *)of_get_property(tmp, str, NULL);

While I applaud the number of casts remove by this patch, this one is an
unnecessary addition.

> +		of_node_put(tmp);
> +		tmp = of_get_parent(np);

You should do the above two line in the opposite order. Also do you
really want to keep getting the parent of the same node over and over
(i.e. you never change np)?

> +	}

You probably want a final of_node_put(tmp).


> +	INFO("Phy type: ");
> +	switch (phy_type) {
> +	case RIO_PHY_SERIAL:
> +		printk("serial\n");
> +		break;
> +	case RIO_PHY_PARALLEL:
> +		printk("parallel");

Missing \n

> +	port = kzalloc(sizeof(struct rio_mport), GFP_KERNEL);
> +	if (!port) {
> +		ERR("Can't alloc memory for 'port'\n");
> +		rc = -ENOMEM;
> +		goto err;
> +	}
>  	port->id = 0;
>  	port->index = 0;

These two could go as you just allocated zeroed memory.

-- 
Cheers,
Stephen Rothwell                    sfr@xxxxxxxxxxxxxxxx
http://www.canb.auug.org.au/~sfr/

Attachment: pgp00000.pgp
Description: PGP signature