Messages in this thread | | | Date | Fri, 12 Nov 2004 00:54:39 +0100 | From | Francois Romieu <> | Subject | Re: [PATCH] MII bus API for PHY devices |
| |
Jason McMullan <jason.mcmullan@timesys.com> : [...] > --- /dev/null > +++ linux/drivers/net/mii_bus.c [...] > +struct phy_cmd { > + u32 mii_reg; > + u32 mii_data; > + u16 (*funct) (u16 mii_reg, int bus, int id); ^^^^
-> spaces
[...] > + /* Local state information */ > + struct { > + int irq; > + unsigned long msecs; > + void (*func)(void *data); > + void *data; > + struct work_struct tq; > + struct timer_list timer; ^^^^^
-> tab...
> + } delta; ^^^^
-> ...spaces
[...] > +/* Write value to the PHY for this device to the register at regnum, */ > +/* waiting until the write is done before it returns. All PHY */ > +/* configuration has to be done through the TSEC1 MIIM regs */ > +EXPORT_SYMBOL(mii_bus_write); > +int mii_bus_write(int bus, int id, int regnum, uint16_t value) ^^^^^^^^ -> the code of this file has already used some u16/u32 before this point.
> +{ > + if (!VALID_BUS(bus)) > + return -EINVAL; > + > + mii_bus[bus]->write(mii_bus[bus]->priv,id,regnum,value); ^^^^^^^^^^^^^
-> spaces instead of tab -> please add a space after a colon
-> AFAIKS, the code guarantees that VALID_BUS will not fail.
> + return 0; > +} > + > +/* Reads from register regnum in the PHY for device dev, */ > +/* returning the value. Clears miimcom first. All PHY */ > +/* configuration has to be done through the TSEC1 MIIM regs */ > +EXPORT_SYMBOL(mii_bus_read); > +int mii_bus_read(int bus, int id, int regnum) > +{ > + if (!VALID_BUS(bus)) > + return -EINVAL; > + > + return mii_bus[bus]->read(mii_bus[bus]->priv,id,regnum);
-> see above.
[...] > +#define BRIEF_MII_ERRORS > +/* Wait for auto-negotiation to complete */ > +u16 mii_parse_sr(u16 mii_reg, int bus, int id) > +{ > + unsigned int timeout = MII_TIMEOUT; > + struct phy_state *pstate; > + > + if (!VALID(bus, id)) return 0xffff;
-> the return on a separate line only costs an extra character and makes the style more consistent (see the code above).
[...] > +u16 mii_parse_cis8201(u16 mii_reg, int bus, int id) > +{ > + unsigned int speed; > + struct phy_state *pstate; > + > + if (!VALID(bus, id)) return 0xffff; > + pstate = &mii_bus[bus]->phy[id]->state; > + > + if (pstate->link) { > + if (mii_reg & MIIM_CIS8201_AUXCONSTAT_DUPLEX) > + pstate->duplex = 1; > + else > + pstate->duplex = 0;
-> If you are really concerned about the size of the source code: pstate->duplex = (mii_reg & MIIM_CIS8201_AUXCONSTAT_DUPLEX) ? 1 : 0; > + NULL > +}; > + > +/* Use the PHY ID registers to determine what type of PHY is attached > + * to device dev. return a struct phy_info structure describing that PHY > + */ > +struct phy_info *mii_phy_get_info(int bus, int id) > +{ > + u16 phy_reg; > + u32 phy_ID; > + int i; > + struct phy_info *theInfo = NULL;
-> s/theInfo/info/ ?
> + if (mii_bus[bus] == NULL) > + return NULL;
-> This function is not exported and the caller has already issued a VALID_BUS.
[...] > + /* loop through all the known PHY types, and find one that */ > + /* matches the ID we read from the PHY. */ > + for (i = 0; phy_info[i]; i++) > + if (phy_info[i]->id == (phy_ID >> phy_info[i]->shift)) > + theInfo = phy_info[i];
-> add braces around the for block + break ?
> + > + if (theInfo == NULL) { > + printk("phy %d.%d: PHY id 0x%x is not supported!\n", bus, id, phy_ID); > + return NULL;
-> no need to return here.
> + } else { > + printk("phy %d.%d: PHY is %s (%x)\n", bus, id, theInfo->name, > + phy_ID); > + } > + > + return theInfo; > +} [...] > +int mii_phy_attach(struct mii_if_info *mii, struct net_device *dev, int bus, int id) > +{ > + struct phy_info *phy,*info; > + > + if (mii_bus[bus] == NULL) { > + printk(KERN_ERR "mii_phy_attach: Can't attach %s, no MII bus %d present\n",dev->name,bus); > + return -ENODEV; > + } > + > + if (VALID(bus,id)) { > + printk(KERN_ERR "mii_phy_attach: PHY %d.%d is already attached to %s\n",bus,id,dev->name); > + return -EBUSY; > + } > + > + info = mii_phy_get_info(bus, id); > + if (info == NULL) > + return -ENODEV; > + > + phy = kmalloc(sizeof(*phy), GFP_KERNEL); > + memcpy(phy,info,sizeof(*phy));
-> kmalloc can fail.
[...] > +int mii_bus_register(struct mii_bus *bus) > +{ > + int bus_id; > + > + if (bus == NULL || bus->name == NULL || bus->read == NULL || > + bus->write == NULL) > + return -EINVAL; [...] > + mii_bus[bus_id] = bus; > + > + if (mii_bus[bus_id] == NULL) {
-> bus is not NULL, neither is mii_bus[bus_id] (see above).
> + up(&mii_bus_lock); > + return -EINVAL; > + } > + > + if (mii_bus[bus_id]->reset) > + mii_bus[bus_id]->reset(mii_bus[bus_id]->priv);
if (bus->reset) bus->reset(bus->priv);
Please, pretty please, more polish (I did not say that it could be done instantly :o) ).
-- Ueimor - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |