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 <biggerbadder...@gmail.com> wrote:
> Hi Luigi,
>
> Luigi 'Comio' Mantellini wrote:
>>
>> From: Luigi 'Comio' Mantellini <luigi.mantell...@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.mantell...@idf-hit.com
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
