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