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

Dear Chuanhua Han,

In message 20190726112403.32842-2-chuanhua.han@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