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.
> @@ -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.
> + 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;
> + }
--
Jarkko
--
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
