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 <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?
>
>
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
