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

[Marek Vasut]

On Thursday, July 02, 2015 at 08:12:32 AM, Marcel Ziswiler wrote:
> 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!

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!

No worries :)

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

Yeah, the code in certain areas of U-Boot isn't the pinacle of coding style
excellence for sure.

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

Yeah, that I can confirm this.

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

OK, thanks! :-)

[...]