[U-Boot-Users] Please pull u-boot-83xx.git (I2C rework)

[Stefan Roese <sr@denx.de>]

Hi Ben,

On Saturday 04 November 2006 03:11, Kim Phillips wrote:
> Please pull from 'master' branch of:
>
>   http://opensource.freescale.com/pub/scm/u-boot-83xx.git
>
> to receive the following updates (essentially MPC8349mITX and MPC8360EMDS
> support):
>
> Ben Warren:
>       Add support for multiple I2C buses
>       Multi-bus I2C implementation of MPC834x
>       Additional MPC8349 support for multibus i2c

I'm commenting on the I2C code you submitted, which is now included in the
git tree of Kim Phillips. Sorry for the late review, but I have some more
or less cosmetic comments:

> ------------------------------- common/cmd_i2c.c
> ------------------------------- index c543bb5..62378af 100644
> @@ -101,8 +101,31 @@ static uchar	i2c_mm_last_chip;
>  static uint	i2c_mm_last_addr;
>  static uint	i2c_mm_last_alen;
>
> +/* If only one I2C bus is present, the list of devices to ignore when
> + * the probe command is issued is represented by a 1D array of addresses.
> + * When multiple buses are present, the list is an array of bus-address
> + * pairs.  The following macros take care of this */
> +
>  #if defined(CFG_I2C_NOPROBES)
> +#if defined(CONFIG_I2C_MULTI_BUS)
> +static struct
> +{
> +	uchar	bus;
> +	uchar	addr;
> +} i2c_no_probes[] = CFG_I2C_NOPROBES;
> +#define GET_BUS_NUM	i2c_get_bus_num()
> +#define COMPARE_BUS(b,i)	(i2c_no_probes[(i)].bus == (b))
> +#define COMPARE_ADDR(a,i)	(i2c_no_probes[(i)].addr == (a))
> +#define NO_PROBE_ADDR(i)	i2c_no_probes[(i)].addr
> +#else		/* single bus */
>  static uchar i2c_no_probes[] = CFG_I2C_NOPROBES;
> +#define GET_BUS_NUM	0
> +#define COMPARE_BUS(b,i)	((b) == 0)	/* Make compiler happy */
> +#define COMPARE_ADDR(a,i)	(i2c_no_probes[(i)] == (a))
> +#define NO_PROBE_ADDR(i)	i2c_no_probes[(i)]
> +#endif	/* CONFIG_MULTI_BUS */
> +
> +#define NUM_ELEMENTS_NOPROBE
> (sizeof(i2c_no_probes)/sizeof(i2c_no_probes[0])) #endif
>
>  static int
> @@ -538,14 +561,17 @@ int do_i2c_probe (cmd_tbl_t *cmdtp, int
>  	int j;
>  #if defined(CFG_I2C_NOPROBES)
>  	int k, skip;
> -#endif
> +	uchar bus = GET_BUS_NUM;
> +#endif	/* NOPROBES */
>
>  	puts ("Valid chip addresses:");
>  	for(j = 0; j < 128; j++) {
>  #if defined(CFG_I2C_NOPROBES)
>  		skip = 0;
> -		for (k = 0; k < sizeof(i2c_no_probes); k++){
> -			if (j == i2c_no_probes[k]){
> +		for(k=0; k < NUM_ELEMENTS_NOPROBE; k++)
> +		{

Please move the "{" brace into the "for" loop line. And please also
insert a space between "for" and the "(" parenthesis. So the line above
should look like this:

		for (k=0; k < NUM_ELEMENTS_NOPROBE; k++) {

It seems that you use this "brace style" throughout the complete patch.
This is not according to the U-Boot coding styles (and Linux by the way).
So could you please rework your patch and merge it with Kim again? Or
perhaps Kim can rework the patch according to the U-Boot/Linux coding
style (Lindent?).

Thanks.

> +			if(COMPARE_BUS(bus, k) && COMPARE_ADDR(j, k))
> +			{
>  				skip = 1;
>  				break;
>  			}
> @@ -561,8 +587,11 @@ int do_i2c_probe (cmd_tbl_t *cmdtp, int
>
>  #if defined(CFG_I2C_NOPROBES)
>  	puts ("Excluded chip addresses:");
> -	for( k = 0; k < sizeof(i2c_no_probes); k++ )
> -		printf(" %02X", i2c_no_probes[k] );
> +	for(k=0; k < NUM_ELEMENTS_NOPROBE; k++)
> +	{
> +		if(COMPARE_BUS(bus,k))

Again, please insert a space after the "if".

> +			printf(" %02X", NO_PROBE_ADDR(k));
> +	}
>  	putc ('\n');
>  #endif
>
> @@ -873,6 +902,104 @@ int do_sdram  ( cmd_tbl_t *cmdtp, int fl
>  }
>  #endif	/* CFG_CMD_SDRAM */
>
> +#if defined(CONFIG_I2C_CMD_TREE)
> +#if defined(CONFIG_I2C_MULTI_BUS)
> +int do_i2c_bus_num(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
> +{
> +	int bus_idx, ret=0;
> +
> +	if (argc == 1)  /* querying current setting */
> +	{
> +		printf("Current bus is %d\n", i2c_get_bus_num());
> +	}
> +	else
> +	{

That should be

	} else {

> +		bus_idx = simple_strtoul(argv[1], NULL, 10);
> +		printf("Setting bus to %d\n", bus_idx);
> +		ret = i2c_set_bus_num(bus_idx);
> +		if(ret)
> +		{
> +			printf("Failure changing bus number (%d)\n", ret);
> +		}

Single line statements don't have braces.

And so on...

Please "talk" with Kim on how to clean up this patch.

Thanks.

Best regards,
Stefan