From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kalle Jokiniemi Subject: Re: [PATCH 1/1] OMAP: I2C: Add mpu wake up latency constraint in i2c Date: Thu, 1 Oct 2009 10:56:39 +0300 Message-ID: <1254383799.22468.140.camel@ubuntu> References: <1253204923-21916-1-git-send-email-kalle.jokiniemi@digia.com> <1253204923-21916-2-git-send-email-kalle.jokiniemi@digia.com> <20091001091040.dcc24d47.jhnikula@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Return-path: Received: from smtp1.digia.com ([82.118.214.156]:57887 "EHLO smtp1.digia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755711AbZJAH4K (ORCPT ); Thu, 1 Oct 2009 03:56:10 -0400 In-Reply-To: <20091001091040.dcc24d47.jhnikula@gmail.com> Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Jarkko Nikula Cc: "khilman@deeprootsystems.com" , "linux-omap@vger.kernel.org" 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 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; > > + } > >