On Thursday, July 02, 2015 at 08:12:32 AM, Marcel Ziswiler wrote:
On 2 July 2015 07:50:59 CEST, Marek Vasut marex@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-bitboundary */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! :-)
[...]