[U-Boot] [PATCH v4 2/4] armv8: ls2088aqds: The ls2088aqds board supports the I2C driver model.

[Wolfgang Denk]

Dear Chuanhua Han,

In message <20190726112403.32842-2-chuanhua.han at nxp.com> you wrote:
> This patch is updating ls2088aqds board init code to support DM_I2C.
...
> +struct reg_pair {
> +	uint addr;
> +	u8 *val;
> +};
> +
>  static void sgmii_configure_repeater(int serdes_port)
>  {
>  	struct mii_dev *bus;
>  	uint8_t a = 0xf;
> -	int i, j, ret;
> +	int i, j, k, ret;
>  	int dpmac_id = 0, dpmac, mii_bus = 0;
>  	unsigned short value;
>  	char dev[2][20] = {"LS2080A_QDS_MDIO0", "LS2080A_QDS_MDIO3"};
> @@ -104,10 +109,30 @@ static void sgmii_configure_repeater(int serdes_port)
>  	uint8_t ch_b_eq[] = {0x1, 0x2, 0x3, 0x7};
>  	uint8_t ch_b_ctl2[] = {0x81, 0x82, 0x83, 0x84};
>  
> +	u8 reg_val[6] = {0x18, 0x38, 0x4, 0x14, 0xb5, 0x20};
> +	struct reg_pair reg_pair[10] = {
> +			{6, &reg_val[0]}, {4, &reg_val[1]},
> +			{8, &reg_val[2]}, {0xf, NULL},
> +			{0x11, NULL}, {0x16, NULL},
> +			{0x18, NULL}, {0x23, &reg_val[3]},
> +			{0x2d, &reg_val[4]}, {4, &reg_val[5]},
> +	};

Argh... this is pretty much unreadable.  Why do you use a pointer
for "val" in your struct reg_pair?

Would it not be much simpler to use something like this:

struct reg_pair {
	uint	addr;
	u8	val;
}
...
	struct reg_pair reg_pair[] = {
		{0x06, 0x18},
		{0x04, 0x38},
		{0x08, 0x04},
		{0x0F, 0},
		{0x11, 0},
		{0x16, 0},
		{0x18, 0},
		{0x23, 0x14},
		{0x2D, 0xB5},
		{0x04, 0x20},
	}}
?

Also, you should add some comments what all these magic numbers
mean. As is, nobody is able to understand what you are doing here.
This must be explained!

Also, a comment should be added that (and why) some of the values get
inserted dynamically later down in the code.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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
The easiest way to figure the cost of living is to take  your  income
and add ten percent.