On Fri, Nov 28, 2008 at 01:52:27PM +0530, ext Pillai, Manikandan wrote:
> Hi Balbi,
> 
> Thanks for your comments. Pls find my response inlined.

please, keep linux-omap in the loop otherwise we loose the thread ;-)

> > diff --git a/arch/arm/mach-omap2/board-omap3evm.c 
> > b/arch/arm/mach-omap2/board-omap3evm.c
> > index bc44cb5..f5f9ea5 100644
> > --- a/arch/arm/mach-omap2/board-omap3evm.c
> > +++ b/arch/arm/mach-omap2/board-omap3evm.c
> > @@ -42,6 +42,9 @@
> >  #include "twl4030-generic-scripts.h"
> >  #include "mmc-twl4030.h"
> >  
> > +#define    OMAP3_I2C_STD   100
> > +#define    OMAP3_I2C_FAST  400
> > +#define    OMAP3_I2C_HS    2600
> 
> Why ?? <see below>
> >>>>Mani: I got an earlier comment to remove the numbers. I guess I will go 
> >>>>back to how it was.

>From who ? Every other boards use the number themselves ;-)

> No way. Create separated versions of the omap3evm_i2c_boardinfo[] for
> the "conflicting" devices and register one or the other version based on
> if (machine_is_XXX()) or something similar.
> 
> You can register as many i2c_board_infos as you want per bus.
> 
> >>>>>>Mani: Not clear on what the comment is. Do I need to remove the
> #if defined(CONFIG_PR785). 

Yes... you should for example:

static struct i2c_board_info omap3evm_i2c_board_info[] __initdata = {
... (all devices but tps and twl) ...
};

static struct i2c_board_info pr785_i2c_board_info[] __initdata = {
        {
                I2C_BOARD_INFO("tps62352_core_pwr", 0x4A),
                .flags = I2C_CLIENT_WAKE,
        }, {
                I2C_BOARD_INFO("tps62353_mpu_pwr", 0x48),
                .flags = I2C_CLIENT_WAKE,
        },
};

static struct i2c_board_info twl4030_i2c_board_info[] __initdata = {
        {
                I2C_BOARD_INFO("twl4030", 0x48),
        },
};

Then on init:

...

omap_register_i2c_bus(1, omap3_evm_i2c_board_info,
                ARRAY_SIZE(omap3evm_i2c_board_info);

if (machine_is_pr785())
        i2c_register_board_info(1, pr785_i2c_board_info,
                ARRAY_SIZE(pr785_i2c_board_info));

if (machine_is_xxxx())
        i2c_register_board_info(1, twl4030_i2c_board_info,
                ARRAY_SIZE(twl4030_i2c_board_info));


> > diff --git a/drivers/video/omap/lcd_omap3evm.c 
> > b/drivers/video/omap/lcd_omap3evm.c
> > index 90aa015..ac51dd0 100644
> > --- a/drivers/video/omap/lcd_omap3evm.c
> > +++ b/drivers/video/omap/lcd_omap3evm.c
> > @@ -66,9 +66,11 @@ static int omap3evm_panel_init(struct lcd_panel *panel,
> >     gpio_direction_output(LCD_PANEL_LR, 1);
> >     gpio_direction_output(LCD_PANEL_UD, 1);
> >  
> > +#if !defined(CONFIG_PR785)
> >     twl4030_i2c_write_u8(TWL4030_MODULE_LED, 0x11, TWL_LED_LEDEN);
> >     twl4030_i2c_write_u8(TWL4030_MODULE_PWMA, 0x01, TWL_PWMA_PWMAON);
> >     twl4030_i2c_write_u8(TWL4030_MODULE_PWMA, 0x02, TWL_PWMA_PWMAOFF);
> > +#endif
> >     bklight_level = 100;
> >  
> >     return 0;
> > @@ -97,12 +99,14 @@ static unsigned long omap3evm_panel_get_caps(struct 
> > lcd_panel *panel)
> >  static int omap3evm_bklight_setlevel(struct lcd_panel *panel,
> >                                             unsigned int level)
> >  {
> > +#if !defined(CONFIG_PR785)
> >     u8 c;
> >     if ((level >= 0) && (level <= 100)) {
> >             c = (125 * (100 - level)) / 100 + 2;
> >             twl4030_i2c_write_u8(TWL4030_MODULE_PWMA, c, TWL_PWMA_PWMAOFF);
> >             bklight_level = level;
> >     }
> > +#endif
> 
> No!!
> 
> NAK to all ifdefs. It'll break multiomap support.
> >>>>>>Mani: Will use a positive #if defined. 

also not.. you need to use platform_data to tell you how to set stuff
up for different boards, don't use pre-processor conditionals.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to