[U-Boot] [PATCH 2/8] net: asix: fix operation without eeprom

[Marek Vasut]

On Thursday, July 02, 2015 at 01:04:47 AM, Marcel Ziswiler wrote:

Hi!

[...]

> @@ -64,8 +67,14 @@
>  	 AX_MEDIUM_AC | AX_MEDIUM_RE)
> 
>  /* AX88772 & AX88178 RX_CTL values */
> +#define AX_RX_CTL_RH2M			0x0200	/* Enable IP header in 
receive
> +						   buffer aligned on 32-bit
> +						   boundary */

The comments need a bit of polishing, though it is not the main problem I have
with this patch. The multiline comments should be like this according to kernel
coding style (to my knowledge):

/*
 * foo
 * bar
 * baz
 */

> +#define AX_RX_CTL_RH1M			0x0100	/* Enable RX-Header mode 
0 */
>  #define AX_RX_CTL_SO			0x0080
>  #define AX_RX_CTL_AB			0x0008
> +#define AX_RX_HEADER_DEFAULT		(AX_RX_CTL_RH1M | \
> +					 AX_RX_CTL_RH2M)
> 
>  #define AX_DEFAULT_RX_CTL	\
>  	(AX_RX_CTL_SO | AX_RX_CTL_AB)
> @@ -426,7 +435,15 @@ static int asix_init(struct eth_device *eth, bd_t *bd)
> 
>  	debug("** %s()\n", __func__);
> 
> -	if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0)
> +	if ((dev->pusb_dev->descriptor.idVendor == 0x0b95) &&
> +	    (dev->pusb_dev->descriptor.idProduct == 0x772b)) {

I don't like hardcoding these constants here (and further down).
I understand that those are AX88792B chips (or whatever the number
is, there's a B at the end and they're not exactly compatible with
the original AX88792), but what about making this a bit more generic?

What I expect is that when AX88792C comes, we'd just add another
if (idVendor == ... ) into this code here with another magic number
and it will become an unmaintainable horror.

Maybe add a function which handles quirks of each revision (B, C, ...)
of the ASIX chip and definitelly define those magic numbers as macros.

> +		if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL |
> +					   AX_RX_HEADER_DEFAULT) < 0)
> +			goto out_err;
> +	} else if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0)
> +		goto out_err;
> +
> +	if (asix_write_hwaddr(eth) < 0)
>  		goto out_err;
> 
>  	do {

[...]