[U-Boot] [PATCH] Marvell 88EXXXX Switch/PHY init support

[Wolfgang Denk]

Dear Prafulla Wadaskar,

In message <1238798370-9245-4-git-send-email-prafulla at marvell.com> you wrote:
> From: prafulla_wadaskar <prafulla at marvell.com>
> 
> 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 <yotam at marvell.com>
> Michael Blostein <michaelbl at marvell.com
> 
> Signed-off-by: prafulla_wadaskar <prafulla at marvell.com>
> Reviewed by: Ronen Shitrit <rshitrit at 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);
> +		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: wd at denx.de
Eeeek!
'eval' on strings should have been named 'evil'.    -- Tom Phoenix in
        <Pine.GSO.3.96.980526121813.27437N-100000 at user2.teleport.com>