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