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 <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
