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
