Re: [PATCH] MII bus API for PHY devices

From: Francois Romieu
Date: Thu Nov 11 2004 - 19:06:25 EST


Jason McMullan <jason.mcmullan@xxxxxxxxxxx> :
[...]
> --- /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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/