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

[Marcel Ziswiler]

On 2 July 2015 07:50:59 CEST, Marek Vasut <marex at denx.de> wrote:
>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.

I was hesitant at first but then decided to submit it anyway to get some feedback on the thematic. So thank you very much!

>The multiline comments should be like this according
>to kernel
>coding style (to my knowledge):
>
>/*
> * foo
> * bar
> * baz
> */

Yeah, sorry. My bad. I since got educated in doing this but stumble over it at times on older patches.

>> +#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?

AX88772B actually and yes there seem to be C variants of that same chip out now as well but we haven't gotten our hands on any such yet. I just do remember that ASIX does not take backwards compatibility too serious.

>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.

Understood.

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

Agreed.

>> +		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 {
>
>[...]