On Thu, 2009-10-01 at 09:10 +0300, Jarkko Nikula wrote: > Hi Kalle > > Few minor comments below. > > On Thu, 17 Sep 2009 19:28:43 +0300 > Kalle Jokiniemi <kalle.jokiniemi@xxxxxxxxx> wrote: > > > -static u32 i2c_rate[ARRAY_SIZE(i2c_resources)]; > > +static struct omap_i2c_bus_platform_data i2c_pdata[ARRAY_SIZE(i2c_resources)]; > > static struct platform_device omap_i2c_devices[] = { > > - I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_rate[0]), > > + I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]), > > #if defined(CONFIG_ARCH_OMAP24XX) || defined(CONFIG_ARCH_OMAP34XX) > > - I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_rate[1]), > > + I2C_DEV_BUILDER(2, i2c_resources[1], &i2c_pdata[1]), > > #endif > > #if defined(CONFIG_ARCH_OMAP34XX) > > - I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_rate[2]), > > + I2C_DEV_BUILDER(3, i2c_resources[2], &i2c_pdata[2]), > > #endif > > }; > This is good idea. IRCC, e.g. the I2C_SCL t_high and t_low timing values > can be board dependent values so passing just the i2c_rate as a > platform_data seems to be not enough in the future. I gather that this t_high/t_low passing change can be added later ;) > > > @@ -526,8 +529,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap, > > * REVISIT: We should abort the transfer on signals, but the bus goes > > * into arbitration and we're currently unable to recover from it. > > */ > > + dev->set_mpu_wkup_lat(dev->dev, 1); > > r = wait_for_completion_timeout(&dev->cmd_complete, > > OMAP_I2C_TIMEOUT); > > + dev->set_mpu_wkup_lat(dev->dev, 0); > > These lines here and lines below in omap_i2c_probe looks suspicious. I > would rather allow callback to be NULL (and then you would not need > empty function in arch/arm/plat-omap/i2c.c) and just test it here > before executing. Looks much more safe code then. Yes, this needs to be fixed. Otherwise we could get null pointer calls. Nice catch. - Kalle > > > + if (pdata != NULL) { > > + speed = pdata->clkrate; > > + dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat; > > + } else { > > + speed = 100; /* Default speed */ > > + dev->set_mpu_wkup_lat = NULL; > > + } > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html