Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c

From: Jarkko Nikula <jhnikula@gmail.com>
To: Kalle Jokiniemi <kalle.jokiniemi@digia.com>
Cc: khilman@deeprootsystems.com, linux-omap@vger.kernel.org
Subject: Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c
Date: Thu, 1 Oct 2009 09:10:40 +0300	[thread overview]
Message-ID: <20091001091040.dcc24d47.jhnikula@gmail.com> (raw)
In-Reply-To: <1253204923-21916-2-git-send-email-kalle.jokiniemi@digia.com>

Hi Kalle

Few minor comments below.

On Thu, 17 Sep 2009 19:28:43 +0300
Kalle Jokiniemi <kalle.jokiniemi@digia.com> 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