Dear Prafulla Wadaskar,
In message 1238798370-9245-4-git-send-email-prafulla@marvell.com you wrote:
From: prafulla_wadaskar prafulla@marvell.com
Chips supprted:-
- 88E61XX 6 port gbe swtich with 5 integrated PHYs
- 88E6061 6 port fe swtich with 5 integrated PHYs
- 88E1116 gbe transceiver
Contributors: Yotam Admon yotam@marvell.com Michael Blostein <michaelbl@marvell.com
Signed-off-by: prafulla_wadaskar prafulla@marvell.com Reviewed by: Ronen Shitrit rshitrit@marvell.com
ben has already commented on the incorrect location of this code in the directory hierarchy. I will restrict my review to formal issues.
diff --git a/board/Marvell/common/mv88e1116.c b/board/Marvell/common/mv88e1116.c new file mode 100644 index 0000000..87ec550 --- /dev/null +++ b/board/Marvell/common/mv88e1116.c
...
+#ifndef MV88E1116_DEBUG +#define MV88E1116_DEBUG 0 +#endif +#define DEBUG_PRINT MV88E1116_DEBUG
+#include <common.h> +#include <debug_prints.h> +#include "../common/ppc_error_no.h"
See before (i. e. get rid of this stuff, here and everywhere else; it will make the code jkust simpler).
diff --git a/board/Marvell/common/mv88e60xx.c b/board/Marvell/common/mv88e60xx.c new file mode 100644 index 0000000..6034f7b --- /dev/null +++ b/board/Marvell/common/mv88e60xx.c
...
+static void mv_switch_88e60xx_vlan_init(u32 eth_port_num,
u32 switch_cpu_port,u32 switch_max_ports_num,u32 switch_ports_ofs,u32 switch_enabled_ports_mask)+{
- u32 prt;
- u16 reg;
- debug_print_ftrace();
- /* be sure all ports are disabled */
- for (prt = 0; prt < switch_max_ports_num; prt++) {
mv_sw_eth_phy_reg_read(eth_port_num, (switch_ports_ofs + prt),MV88E60XX_PORT_CONTROL_REG, ®);reg &= ~0x3;mv_sw_eth_phy_reg_write(eth_port_num, (switch_ports_ofs + prt),MV88E60XX_PORT_CONTROL_REG, reg);
Please read the CodingStyle document about resonable choice oif namess for functions, variables, etc. Names like mv_switch_88e60xx_vlan_init(), mv_sw_eth_phy_reg_read(), switch_max_ports_num, etc. are a pain to write, and a bigger pain to read. Please use SIMPLE, readable names.
...
+U_BOOT_CMD(smi, CONFIG_SYS_MAXARGS, 1, do_smi,
"smi - isues read/write command on smi for switch registers\n","smi read [smiaddr] [regaddr] [page]\n"" - read smi register command\n""smi write [smiaddr] [regaddr] [value] [page]\n"" - write <value> to <regaddr> register command\n"" - run the commands in the environment variable(s) 'var'\n");
The definition of the U_BOOT_CMD macro has changed, please fix your
...
+U_BOOT_CMD(dump60xxphy, CONFIG_SYS_MAXARGS, 1, do_dump60xxphy,
"dump60xxphy - dump 88360xx registers\n","var [...]\n"" - run the commands in the environment variable(s) 'var'\n");+#endif /* CONFIG_CMD_DUMP60XXPHY */
Ditto.
Hm... if we the function of this is to "dump 88360xx registers", then why is the command name "dump60xxphy" ?
60xx != 88360xx ??
Ditto for the rest of the file.
Best regards,
Wolfgang Denk