RE: [PATCH 2/3] (Resend part #1) Add the RapidIO support to powerpcarchitecture with memory mapping support.
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
> <[EMAIL PROTECTED]> 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[EMAIL PROTECTED]
> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/3] (Resend part #1) Add the RapidIO support to powerpcarchitecture with memory mapping support.
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
[EMAIL PROTECTED] 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[EMAIL PROTECTED]
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
