Re: [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 <w...@denx.de>
> Sent: Friday, August 23, 2019 3:05 PM
> To: Chuanhua Han <chuanhua....@nxp.com>
> Cc: albert.u.b...@aribaud.net; Prabhakar Kushwaha
> <prabhakar.kushw...@nxp.com>; Priyanka Jain <priyanka.j...@nxp.com>;
> Rajesh Bhagat <rajesh.bha...@nxp.com>; u-boot@lists.denx.de;
> lu...@denx.de; tr...@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....@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot