[U-Boot] [EXT] Re: [PATCH v3 2/4] armv8: ls2088aqds: The ls2088aqds board supports the I2C driver model.
Chuanhua Han
chuanhua.han at nxp.com
Fri Jul 26 06:36:48 UTC 2019
Hi, Wolfgang Denk
> -----Original Message-----
> From: Wolfgang Denk <wd at denx.de>
> Sent: 2019年7月26日 14:34
> To: Chuanhua Han <chuanhua.han at nxp.com>
> Cc: albert.u.boot at aribaud.net; Prabhakar Kushwaha
> <prabhakar.kushwaha at nxp.com>; Rajesh Bhagat <rajesh.bhagat at nxp.com>;
> Priyanka Jain <priyanka.jain at nxp.com>; u-boot at lists.denx.de;
> lukma at denx.de; trini at konsulko.com
> Subject: [EXT] Re: [PATCH v3 2/4] armv8: ls2088aqds: The ls2088aqds board
> supports the I2C driver model.
>
> Caution: EXT Email
>
> Dear Chuanhua Han,
>
> In message <20190726032729.14381-2-chuanhua.han at nxp.com> you wrote:
> >
> > /* Set I2c to Slot 1 */
> > - i2c_write(0x77, 0, 0, &a, 1);
> > +#ifndef CONFIG_DM_I2C
> > + ret = i2c_write(0x77, 0, 0, &a, 1); #else
> > + ret = i2c_get_chip_for_busnum(0, 0x77, 1, &udev);
> > + if (!ret)
> > + ret = dm_i2c_write(udev, 0, &a, 1); #endif
>
> See my other comment. You should add error checking for the i2c_write(),
> too.
>
> > + 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;
> > + }
> > +
>
> NAK. You really want to change this ugly code, please.
>
> > /* Set I2c to Slot 1 */
> > - i2c_write(0x77, 0, 0, &a, 1);
> > +#ifndef CONFIG_DM_I2C
> > + ret = i2c_write(0x77, 0, 0, &a, 1); #else
> > + ret = i2c_get_chip_for_busnum(0, 0x77, 1, &udev);
> > + if (!ret)
> > + ret = dm_i2c_write(udev, 0, &a, 1); #endif
>
> Why do you do it here, but not above? You need to work more consisntently!
>
> > diff --git a/board/freescale/ls2080aqds/ls2080aqds.c
> > b/board/freescale/ls2080aqds/ls2080aqds.c
> > index a0a3301..ec22cf8 100644
> > --- a/board/freescale/ls2080aqds/ls2080aqds.c
> > +++ b/board/freescale/ls2080aqds/ls2080aqds.c
> > @@ -161,7 +161,15 @@ int select_i2c_ch_pca9547(u8 ch) {
> > int ret;
> >
> > +#ifndef CONFIG_DM_I2C
> > ret = i2c_write(I2C_MUX_PCA_ADDR_PRI, 0, 1, &ch, 1);
> > +#else
> > + struct udevice *dev;
>
> No declarations after code, please!
All these I will check and modify, thank you!
>
>
> 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
> Single tasking: Just Say No.
More information about the U-Boot
mailing list