[U-Boot] [PATCH 1/3 v3] Bit-banged MII driver with multi-bus support.

[Ben Warren <biggerbadderben@gmail.com>]

Hi Luigi,

Luigi Mantellini wrote:
> Hi Ben,
>
> thank for your review. Find my inline comments.
>
> Thanks and best regards,
>
> luigi
>
> On Mon, Oct 5, 2009 at 8:27 AM, Ben Warren <biggerbadderben@gmail.com> wrote:
>   
>> Hi Luigi,
>>
>> Luigi 'Comio' Mantellini wrote:
>>     
>>> From: Luigi 'Comio' Mantellini <luigi.mantellini@idf-hit.com>
>>>
>>>
>>>       
>> Please add some descriptive information here.  This is a big patch and
>> deserves a changelong
>>     
>>> +
>>> +#define BBMII_RELOATE(v,off) (v += (v?off:0))
>>>
>>>       
>> s/RELOATE/RELOCATE/
>>
>> Please apply globally
>>     
>
> Ok.
>
>   
>>> +
>>> +struct bbmiibus bbmiibusses[] = {
>>>
>>>       
>> Elsewhere, you name things 'bb_mii', but here it's 'bbmii'.  I personally
>> prefer adding the underscores, but please be consistent.
>>     
>
> What do you mean? change bbmiibus -> bb_mii_bus and bbmiibuses -> bb_mii_buses?
>
>   
Sure, that would be fine.  Consistency is the key.
>>> +       {
>>> +               .name = BB_MII_DEVNAME,
>>>       
>
> BB_MII_DEVNAME will be "bb_mii". is it ok?
>
>   
Yes.
>>> +               .init = bb_mii_init_wrap,
>>> +               .mdio_active = bb_mdio_active_wrap,
>>> +               .mdio_tristate = bb_mdio_tristate_wrap,
>>> +               .set_mdio = bb_set_mdio_wrap,
>>> +               .get_mdio = bb_get_mdio_wrap,
>>> +               .set_mdc = bb_set_mdc_wrap,
>>> +               .delay = bb_delay_wrap,
>>> +       }
>>> +};
>>>       
>
>   
>>> +#if !defined(CONFIG_RELOC_FIXUP_WORKS)
>>> +               /* Reloate the hooks pointers*/
>>>
>>>       
>> s/Reloate/Relocate/
>> s/hooks/hook/
>>     
>
> ok
>
>
>   
>>>  */
>>> -static void miiphy_pre (char read, unsigned char addr, unsigned char reg)
>>> +static void miiphy_pre (struct bbmiibus *bus, char read, unsigned char
>>> addr, unsigned char reg)
>>>
>>>       
>> This line's too long (please keep < 78 characters).  Apply globally
>>     
>
> ok
>
>   
>>> */
>>> -       MDC (0);
>>> -       MDIO (0);
>>> -       MIIDELAY;
>>>       
> ...
>   
>>> +       bus->set_mdc (bus, 0);
>>> +       bus->set_mdio (bus, 0);
>>> +       bus->delay(bus);
>>>
>>>       
>> Please be consistent with whitespace.  I prefer no space before the opening
>> paren, although I think Wolfgang likes that.   He's funny that way.  Note
>> that this only relates to function calls.  Control logic (if, for, while
>> etc.) should always have a space before the opening paren.
>>     
>
> I just replaced MD{C,IO} with bus->set_md{c,io}.
> Ok.
>
>   
I assumed that's what happened, but now's a good time to clean it up.  
Thanks.
>>> +
>>> +       bus = bb_miiphy_getbus(devname);
>>> +       if (bus == NULL) {
>>> +               /* Bus not found! */
>>>
>>>       
>> Comment's kinda superfluous...
>>     
>
> I know that the source code is always sufficiently clear regard what it does. :)
> ok
>
>
>   
>> Thanks for this submission.  It's a really big improvement.
>>
>> regards,
>> Ben
>>
>>     
>
> I use GPL code on my devices... It obvious that my improvements will
> be shred with community.
>
> thanks again and best regards,
>
> luigi
>
>   
regards,
Ben