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