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

[Wolfgang Denk]

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!


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.