Olof Johansson wrote at Monday, March 07, 2011 12:30 PM: > [oops, missed reply-all] > > Hi, > > On Mon, Mar 7, 2011 at 9:24 AM, Stephen Warren <swarren@xxxxxxxxxx> wrote: > > Olof Johansson wrote at Monday, March 07, 2011 1:27 AM: > >> Register the base i2c devices on seaboard. A few more are pending, > >> but it's a start. > >> > >> Signed-off-by: Olof Johansson <olof@xxxxxxxxx> > >> --- > >> arch/arm/mach-tegra/board-seaboard-pinmux.c | 1 + > >> arch/arm/mach-tegra/board-seaboard.c | 83 > >> +++++++++++++++++++++++++++ > >> 2 files changed, 84 insertions(+), 0 deletions(-) >... > >> + i2c_register_board_info(0, &isl29018_device, 1); > >> + > >> + i2c_register_board_info(4, &adt7461_device, 1); > >> + > >> + common_i2c_init(); > >> +} > >> + > >> +static void __init kaen_i2c_init(void) > >> +{ > > > > {seaboard,kaen,wario}_i2c_init seem identical. Should this be a single > > shared function, and only the board-specific bits in the non-common > > functions? Also, see below. > > They are now -- they weren't when all i2c devices were registered. I > can re-join them and split them when devices are re-introduced later. Perhaps move the common code into common_i2c_init, then put only the differences in ${boardname}_i2c_init(), which would currently be empty except to call the common function? > >> + gpio_request(TEGRA_GPIO_ISL29018_IRQ, "isl29018"); > >> + gpio_direction_input(TEGRA_GPIO_ISL29018_IRQ); > >> + > >> + i2c_register_board_info(0, &isl29018_device, 1); > >> + > >> + i2c_register_board_info(4, &adt7461_device, 1); > >> + > >> + common_i2c_init(); > >> +} > >> + > >> +static void __init wario_i2c_init(void) > >> +{ > >> + gpio_request(TEGRA_GPIO_ISL29018_IRQ, "isl29018"); > >> + gpio_direction_input(TEGRA_GPIO_ISL29018_IRQ); > >> + > >> + i2c_register_board_info(0, &isl29018_device, 1); > >> + > >> + i2c_register_board_info(4, &adt7461_device, 1); > >> + > >> + common_i2c_init(); > >> +} > >> + > >> static void __init __tegra_seaboard_init(void) > >> { > >> seaboard_pinmux_init(); > >> @@ -145,6 +222,8 @@ static void __init tegra_seaboard_init(void) > >> debug_uart_platform_data[0].irq = INT_UARTD; > >> > >> __tegra_seaboard_init(); > >> + > >> + seaboard_i2c_init(); > >> } > >> > >> static void __init tegra_kaen_init(void) > >> @@ -155,6 +234,8 @@ static void __init tegra_kaen_init(void) > >> debug_uart_platform_data[0].irq = INT_UARTB; > >> > >> __tegra_seaboard_init(); > > > > __tegra_seaboard_init calls seaboard_i2c_init. > > It shouldn't be (note __tegra* vs tegra*). Maybe I should rename > __tegra_seaboard_init to something less alike. Oh right. I double-checked that, but made the same reading mistake both times; the __ version appeared right above the diff, so I must have skipped reading the @@ line. > > > >> + > >> + kaen_i2c_init(); > > > > kaen_i2c_init is the same as seaboard_i2c_init. > > > > So, all the registration happens twice? > > Shouldn't be. The code I am looking at doesn't, so please check your side > again? Yes, it looks functionally OK when read correctly. If you want, I can ack this, but perhaps reworking the code duplication and making that __tegra_seaboard_init rename would make sense; common_seaboard_init or common_board_init? -- nvpublic -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html