Re: [PATCH] drivers/net: spidernet driver on Celleb

[Christoph Hellwig <hch@infradead.org>]

On Tue, Dec 12, 2006 at 02:25:50PM +0900, Ishizaki Kou wrote:
> 
> Following are the changes.
> -This patch enables auto-negotiation.
> -Loading firmware is done when spidernet_open() is called.
> -And this patch adds other several small changes for Celleb. 

This should be split into three separate patches, sent as a patch
series.

> -/* outside loopback mode: ETOMOD signal dont matter, not connected */
> -#define SPIDER_NET_OPMODE_VALUE		0x00000063
> +/* ETOMOD signal is brought to PHY reset. bit2 must be 1 in Celleb */
> +#define SPIDER_NET_OPMODE_VALUE		0x00000067

Is it okay to simple change this value for the ibm blades?

> +static int is1000 = 1;

This should be in struct spider_net_card instead of a global flag.

>  	case SPIDER_NET_GTMFLLINT:
> -		if (netif_msg_intr(card) && net_ratelimit())
> -			pr_err("Spider TX RAM full\n");
> +		/* if (netif_msg_intr(card) && net_ratelimit())
> +			pr_err("Spider TX RAM full\n"); */

Either this should be kept or removed entirely.  In the latter case you
need a good description why it's removed in the patch header.

> +
> +	spider_net_write_reg(card, SPIDER_NET_GMACOPEMD,
> +	                     spider_net_read_reg(card, SPIDER_NET_GMACOPEMD) | 0x4);

Please make sure this doesn't overflow the 80 characters per line limit.

> +static int spider_net_init_firmware(struct spider_net_card *);

Random forward declarations in the middle of the file aren't very nice.
If you really need them put them at the beggining of the file, but it would
be even better if you moved spider_net_init_firmware further up in the
file so we wouldn't need the forward-declaration at all.

> +	if (card->phy.def->phy_id)
> +		mod_timer(&card->aneg_timer, jiffies + SPIDER_NET_ANEG_TIMER);
> +	else
> +		pr_err("No phy is available\n");

What is this idiom about?  Is not having a phy a fatal error in which case
we should abort here, or is it tolerable in which case pr_err is too much.

> +static void spider_net_init_card(struct spider_net_card *);

Same comment above forward declarations as above.