From mboxrd@z Thu Jan  1 00:00:00 1970
From: Richard Retanubun <RichardRetanubun@RuggedCom.com>
Date: Mon, 18 Jan 2010 10:32:02 -0500
Subject: [U-Boot] [RFC] 83xx: uec: miiphybb: Added support for bitBang
 SMI and uec SMI enabled at the same time.
In-Reply-To: <4B53F8B7.1060609@gmail.com>
References: <4A394E90.6080600@RuggedCom.com>
	<20091122202133.BAD70E34A27@gemini.denx.de>
	<4B50912F.1060103@RuggedCom.com> <4B53F8B7.1060609@gmail.com>
Message-ID: <4B547EF2.9010408@RuggedCom.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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