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

[Wolfgang Denk]

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: 
>     - http://patchwork.ozlabs.org/project/uboot/list/?series=110856
>     - http://patchwork.ozlabs.org/project/uboot/list/?series=109459
> 	- http://patchwork.ozlabs.org/project/uboot/list/?series=120936

Is this patch version 3? Or soemthign new?  You should explan what
you 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:

#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.

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.