Hi Ben,
Thanks for the feedback, my comments are inline. I'll hold off on a patch-V2 until you bless the #define CONFIG_SYS_* name and my use of (void) casting function call.
- Richard
<snip>
+int bb_miiphy_read (char *devname, unsigned char addr,
unsigned char reg, unsigned short *value);+int bb_miiphy_write (char *devname, unsigned char addr,
unsigned char reg, unsigned short value);There's already BB-related stuff in include/miiphy.h. Why not put the prototypes there?
Indeed there is, I think Luigi added them and I missed it; sorry about that my patch-V2 will have this file removed.
<snip>
@@ -579,8 +579,7 @@ static void phy_change(struct eth_device *dev) adjust_link(dev); }
-#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
- && !defined(BITBANGMII)
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /*&& !defined(BITBANGMII)*/
What's the point of this comment?
I think at the time I was unsure of what/how BITBANGMII was being used (it is some legacy thing, no?). I was trying to (gently) signal its removal by commenting it out from the #ifdef and forgot to clean it up. will be removed in patch-V2.
<snip>
@@ -1372,8 +1371,7 @@ int uec_initialize(bd_t *bis, uec_info_t *uec_info) return err; }
-#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
- && !defined(BITBANGMII)
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /* && !defined(CONFIG_BITBANGMII) */ miiphy_register(dev->name, uec_miiphy_read, uec_miiphy_write);
Same comment-related comment as above.
Same explanation, will remove in patch-V2
<snip>
#endif
diff --git a/drivers/qe/uec_phy.c b/drivers/qe/uec_phy.c index 9715183..6fb3b4d 100644 --- a/drivers/qe/uec_phy.c +++ b/drivers/qe/uec_phy.c @@ -25,6 +25,7 @@ #include "uec.h" #include "uec_phy.h" #include "miiphy.h" +#include "../net/phy/miiphybb.h"
Please don't use relative includes. As I'm sure you've noticed, people around here are keen on massive file reorganization, and this sort of thing makes that more work. If you put your prototypes in miiphy.h it won't be a problem
Understood, will do in patch-V2
<snip>
- #define CONFIG_SYS_BITBANG_PHY_PORT(name) name,
- #define CONFIG_SYS_BITBANG_PHY_PORTS
CONFIG_SYS_BITBANG_PHY_PORT("FSL UEC0")
Maybe you could come up with a shorter name?
I thought this name lines up well with CONFIG_SYS_FIXED_PHY_PORTS; but I can go with CONFIG_SYS_BB_PHY_PORT(S) (5 less chars) if you like. I think _BITBANG_ is easier to read though.
Let me know and I'll make it so in patch-V2
+#if defined(CONFIG_BITBANGMII)
- u8 i = 0;
Why is this a u8? I don't believe you save any space by making it less than an int, and probably invite compiler warnings with the comparison to ARRAY_SIZE()
Good point, I'll make it a u32 then.
- for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
if (strncmp(dev->name, bitbang_phy_port[i],strlen(dev->name)) == 0) {It's probably better to do sizeof(dev->name)
Understood, will do in patch-V2.
(void)bb_miiphy_write(NULL, mii_id, regnum, value);What's the point of this cast?
I was thought that it is a good practice to do when you call a function that have a return code, but you don't care/check for it now.
With the casting, if you choose to do check return codes in the future, you know which functions have return code.
- for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
if (strncmp(dev->name, bitbang_phy_port[i],strlen(dev->name)) == 0) {(void)bb_miiphy_read(NULL, mii_id, regnum, &value);return (value);}- }
Same comments as above.
Will do the same things as above (u32 and sizeof).
Thanks a lot for your time.
- Richard