Re: [PATCH] MII bus API for PHY devices

[Francois Romieu <romieu@fr.zoreil.com>]

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