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

[Luigi Mantellini]

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 at gmail.com> wrote:
> Hi Luigi,
>
> Luigi 'Comio' Mantellini wrote:
>>
>> From: Luigi 'Comio' Mantellini <luigi.mantellini at 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?

>>
>> +       {
>> +               .name = BB_MII_DEVNAME,

BB_MII_DEVNAME will be "bb_mii". is it ok?

>> +               .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.

>> +
>> +       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


-- 
Luigi 'Comio' Mantellini
R&D - Software
Industrie Dial Face S.p.A.
Via Canzo, 4
20068 Peschiera Borromeo (MI), Italy

Tel.: +39 02 5167 2813
Fax: +39 02 5167 2459
web: www.idf-hit.com
mail: luigi.mantellini at idf-hit.com