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

[Prabhakar Kushwaha]

++ Xiaowei Bao (he is taking over from Chuanhua)

> -----Original Message-----
> From: Wolfgang Denk <wd at denx.de>
> Sent: Friday, August 23, 2019 3:05 PM
> To: Chuanhua Han <chuanhua.han at nxp.com>
> Cc: albert.u.boot at aribaud.net; Prabhakar Kushwaha
> <prabhakar.kushwaha at nxp.com>; Priyanka Jain <priyanka.jain at nxp.com>;
> Rajesh Bhagat <rajesh.bhagat at nxp.com>; u-boot at lists.denx.de;
> lukma at denx.de; trini at konsulko.com
> Subject: Re: [PATCH v4 2/4] armv8: ls2088aqds: The ls2088aqds board supports
> the I2C driver model.
> 
> 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.
> 
> 

Dear Wolfgang, 

Unfortunately this patch has been merged in main-line. 

But I will ask (Xiaowei Bao) to send patch to fix it.  I will make sure to get fix in RC3 or RC4. 

--pk