[U-Boot] [EXT] Re: [PATCH 1/3] armv8: ls1088aqds: The ls1088aqds board supports the I2C driver model.

[Chuanhua Han]

> -----Original Message-----
> From: Wolfgang Denk <wd at denx.de>
> Sent: 2019年7月26日 14:24
> To: Chuanhua Han <chuanhua.han at nxp.com>
> Cc: albert.u.boot at aribaud.net; Prabhakar Kushwaha
> <prabhakar.kushwaha at nxp.com>; Ashish Kumar <ashish.kumar at nxp.com>;
> Rajesh Bhagat <rajesh.bhagat at nxp.com>; u-boot at lists.denx.de
> Subject: [EXT] Re: [U-Boot] [PATCH 1/3] armv8: ls1088aqds: The ls1088aqds
> board supports the I2C driver model.
> 
> Caution: EXT Email
> 
> Dear Chuanhua Han,
> 
> In message <20190725084400.4035-1-chuanhua.han at nxp.com> you wrote:
> > This patch is updating ls1088aqds board init code to support DM_I2C.
> >
> > Signed-off-by: Chuanhua Han <chuanhua.han at nxp.com>
> > ---
> > depends on:
> >     -
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwor
> k.ozlabs.org%2Fproject%2Fuboot%2Flist%2F%3Fseries%3D110856&data=
> 02%7C01%7Cchuanhua.han%40nxp.com%7Cf736cff780674f915ca208d71191db
> c7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636997190469571
> 724&sdata=LpVQJERj1Mc%2BEZpck8eYs69Lh43PBPqri%2FPl7%2BJsLks%3
> D&reserved=0
> >     -
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwor
> k.ozlabs.org%2Fproject%2Fuboot%2Flist%2F%3Fseries%3D109459&data=
> 02%7C01%7Cchuanhua.han%40nxp.com%7Cf736cff780674f915ca208d71191db
> c7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636997190469571
> 724&sdata=50UYLFRdy8NjujZ0GARa%2BlY6Qxt%2BORc8xwMI%2Fvw3Ao
> E%3D&reserved=0
> >       -
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> >
> work.ozlabs.org%2Fproject%2Fuboot%2Flist%2F%3Fseries%3D120936&da
> ta
> >
> =02%7C01%7Cchuanhua.han%40nxp.com%7Cf736cff780674f915ca208d71191d
> bc7%7
> >
> C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636997190469571724&
> amp;sda
> > ta=kdEbfqA0JJTeFf9bmnz7qj3E8T2ETAEXD3JmSkt0YF4%3D&reserved=0
> 
> Is this patch version 3? Or soemthign new?  You should explan what you
No,version 1
> changed...
> 
> >  board/freescale/ls1088a/eth_ls1088aqds.c | 74
> ++++++++++++++++++++++++++++++++
> >  include/configs/ls1088aqds.h             |  4 +-
> >  2 files changed, 76 insertions(+), 2 deletions(-)
> >
> > diff --git a/board/freescale/ls1088a/eth_ls1088aqds.c
> > b/board/freescale/ls1088a/eth_ls1088aqds.c
> > index f16b78c..3b83d5b 100644
> > --- a/board/freescale/ls1088a/eth_ls1088aqds.c
> > +++ b/board/freescale/ls1088a/eth_ls1088aqds.c
> > @@ -97,7 +97,16 @@ static void sgmii_configure_repeater(int dpmac)
> >       uint8_t ch_b_ctl2[] = {0x81, 0x82, 0x83, 0x84};
> >
> >       /* Set I2c to Slot 1 */
> > +#ifndef CONFIG_DM_I2C
> >       i2c_write(0x77, 0, 0, &a, 1);
> > +#else
> > +     struct udevice *udev;
> > +
> > +     ret = i2c_get_chip_for_busnum(0, 0x77, 1, &udev);
> > +     if (!ret)
> > +             dm_i2c_write(udev, 0, &a, 1); #endif
> 
> I asked you before to get rid of the declaration (struct udevice) in the middle of
> code (after the i2c_write()).
> 
> I also explained that you shoudl check error codes.  When you change this here,
> you could as well do it for i2c_write(), like this:
OK,I will modify it
> 
> #ifndef CONFIG_DM_I2C
>         ret = i2c_write(0x77, 0, 0, &a, 1); #else
>         ret = i2c_get_chip_for_busnum(0, 0x77, 1, &udev); #endif
>         if (!ret)
>                 ...
> 
> > +                     a = 0x18;
> > +                     dm_i2c_write(udev, 6, &a, 1);
> > +                     a = 0x38;
> > +                     dm_i2c_write(udev, 4, &a, 1);
> > +                     a = 0x4;
> > +                     dm_i2c_write(udev, 8, &a, 1);
> > +
> > +                     dm_i2c_write(udev, 0xf, &ch_a_eq[i], 1);
> > +                     dm_i2c_write(udev, 0x11, &ch_a_ctl2[j], 1);
> > +
> > +                     dm_i2c_write(udev, 0x16, &ch_b_eq[i], 1);
> > +                     dm_i2c_write(udev, 0x18, &ch_b_ctl2[j], 1);
> > +
> > +                     a = 0x14;
> > +                     dm_i2c_write(udev, 0x23, &a, 1);
> > +                     a = 0xb5;
> > +                     dm_i2c_write(udev, 0x2d, &a, 1);
> > +                     a = 0x20;
> > +                     dm_i2c_write(udev, 4, &a, 1);
> > +
> 
> Error checking missing.  And you really want to change this into a loop iterating
> over a set of data instead of repeting the same code again and again.
OK I will  Check,Thanks!
> 
> 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
> Contrary to popular belief, thinking does not cause brain damage.