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

[Zhang Wei]

Hi,  

Thanks!

Maybe I should make a clean and split them into small patches.

Cheers!
Wei.

> 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/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/