On Thursday, July 02, 2015 at 08:12:32 AM, Marcel Ziswiler wrote: > On 2 July 2015 07:50:59 CEST, Marek Vasut <ma...@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! :-) [...] _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot