Dear Prafulla Wadaskar,
In message <[email protected]> you wrote:
> From: prafulla_wadaskar <[email protected]>
>
> Chips supprted:-
> 1. 88E61XX 6 port gbe swtich with 5 integrated PHYs
> 2. 88E6061 6 port fe swtich with 5 integrated PHYs
> 3. 88E1116 gbe transceiver
>
> Contributors:
> Yotam Admon <[email protected]>
> Michael Blostein <[email protected]
>
> Signed-off-by: prafulla_wadaskar <[email protected]>
> Reviewed by: Ronen Shitrit <[email protected]>
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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [email protected]
Eeeek!
'eval' on strings should have been named 'evil'. -- Tom Phoenix in
<[email protected]>
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
