[U-Boot] [RFC] 83xx: uec: miiphybb: Added support for bitBang SMI and uec SMI enabled at the same time.
Richard Retanubun
RichardRetanubun at RuggedCom.com
Mon Jan 18 16:32:02 CET 2010
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
More information about the U-Boot
mailing list