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

[Chuanhua Han]

> -----Original Message-----
> From: Wolfgang Denk <wd at denx.de>
> Sent: 2019年7月25日 15:40
> 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: [EXT] Re: [PATCH v2 2/4] armv8: ls2088aqds: The ls2088aqds board
> supports the I2C driver model.
> 
> Caution: EXT Email
> 
> Dear Chuanhua Han,
> 
> In message <20190725043934.30236-2-chuanhua.han at nxp.com> you wrote:
> > This patch is updating ls2088aqds board init code to support DM_I2C.
> 
> >                               a = 0x18;
> > -                             i2c_write(i2c_addr[dpmac], 6, 1, &a, 1);
> > +                             ret = i2c_write(i2c_addr[dpmac], 6, 1,
> &a, 1);
> > +                             if (ret)
> > +                                     goto error;
> >                               a = 0x38;
> > -                             i2c_write(i2c_addr[dpmac], 4, 1, &a, 1);
> > +                             ret = i2c_write(i2c_addr[dpmac], 4, 1,
> &a, 1);
> > +                             if (ret)
> > +                                     goto error;
> >                               a = 0x4;
> > -                             i2c_write(i2c_addr[dpmac], 8, 1, &a, 1);
> > +                             ret = i2c_write(i2c_addr[dpmac], 8, 1,
> &a, 1);
> > +                             if (ret)
> > +                                     goto error;
> >
> > -                             i2c_write(i2c_addr[dpmac], 0xf, 1,
> > -                                       &ch_a_eq[i], 1);
> > -                             i2c_write(i2c_addr[dpmac], 0x11, 1,
> > -                                       &ch_a_ctl2[j], 1);
> > +                             ret = i2c_write(i2c_addr[dpmac], 0xf,
> > +                                             1, &ch_a_eq[i], 1);
> > +                             if (ret)
> > +                                     goto error;
> > +                             ret = i2c_write(i2c_addr[dpmac], 0x11,
> > +                                             1, &ch_a_ctl2[j], 1);
> > +                             if (ret)
> > +                                     goto error;
> >
> > -                             i2c_write(i2c_addr[dpmac], 0x16, 1,
> > -                                       &ch_b_eq[i], 1);
> > -                             i2c_write(i2c_addr[dpmac], 0x18, 1,
> > -                                       &ch_b_ctl2[j], 1);
> > +                             ret = i2c_write(i2c_addr[dpmac], 0x16,
> > +                                             1, &ch_b_eq[i], 1);
> > +                             if (ret)
> > +                                     goto error;
> > +                             ret = i2c_write(i2c_addr[dpmac], 0x18,
> > +                                             1, &ch_b_ctl2[j], 1);
> > +                             if (ret)
> > +                                     goto error;
> >
> >                               a = 0x14;
> > -                             i2c_write(i2c_addr[dpmac], 0x23, 1, &a,
> 1);
> > +                             ret = i2c_write(i2c_addr[dpmac],
> > +                                             0x23, 1, &a, 1);
> > +                             if (ret)
> > +                                     goto error;
> >                               a = 0xb5;
> > -                             i2c_write(i2c_addr[dpmac], 0x2d, 1, &a,
> 1);
> > +                             ret = i2c_write(i2c_addr[dpmac],
> > +                                             0x2d, 1, &a, 1);
> > +                             if (ret)
> > +                                     goto error;
> >                               a = 0x20;
> > -                             i2c_write(i2c_addr[dpmac], 4, 1, &a, 1);
> > +                             ret = i2c_write(i2c_addr[dpmac], 4, 1,
> &a, 1);
> > +                             if (ret)
> > +                                     goto error; #else
> > +                             ret = i2c_get_chip_for_busnum(0,
> > +
> i2c_addr[dpmac],
> > +                                                           1,
> &udev);
> > +                             if (!ret) {
> > +                                     a = 0x18;
> > +                                     ret = dm_i2c_write(udev, 6,
> &a, 1);
> > +                                     if (ret)
> > +                                             goto error;
> > +                                     a = 0x38;
> > +                                     ret = dm_i2c_write(udev, 4,
> &a, 1);
> > +                                     if (ret)
> > +                                             goto error;
> > +                                     a = 0x4;
> > +                                     ret = dm_i2c_write(udev, 8,
> &a, 1);
> > +                                     if (ret)
> > +                                             goto error;
> > +
> > +                                     ret = dm_i2c_write(udev, 0xf,
> > +
> &ch_a_eq[i], 1);
> > +                                     if (ret)
> > +                                             goto error;
> > +                                     ret = dm_i2c_write(udev,
> 0x11,
> > +
> &ch_a_ctl2[j], 1);
> > +                                     if (ret)
> > +                                             goto error;
> > +
> > +                                     ret = dm_i2c_write(udev,
> 0x16,
> > +
> &ch_b_eq[i], 1);
> > +                                     if (ret)
> > +                                             goto error;
> > +                                     ret = dm_i2c_write(udev,
> 0x18,
> > +
> &ch_b_ctl2[j], 1);
> > +                                     if (ret)
> > +                                             goto error;
> > +                                     a = 0x14;
> > +                                     ret = dm_i2c_write(udev, 0x23,
> &a, 1);
> > +                                     if (ret)
> > +                                             goto error;
> > +                                     a = 0xb5;
> > +                                     ret = dm_i2c_write(udev, 0x2d,
> &a, 1);
> > +                                     if (ret)
> > +                                             goto error;
> > +                                     a = 0x20;
> > +                                     ret = dm_i2c_write(udev, 4,
> &a, 1);
> > +                                     if (ret)
> > +                                             goto error;
> 
> This is a really long list where you repeat the very same code again and again
> and again.  Would it not make sense to declare a data array (holding pairs of
> <offset>, <data> entries), and then iterrate over the loop?  The could would be
> much easier to read and also much smaller...
It seems feasible to use the array you mentioned, but some of the values to be set by i2c are obtained by index I /j in other arrays.
 So we keep setting the i2c register in the same way as before.
> 
> 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 Any
> sufficiently advanced  technology  is  indistinguishable  from  a
> rigged demo.                              - Andy Finkel, computer guy