From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Dooks Subject: Re: [PATCH][resend] iMX/MXC support for I2C Date: Wed, 3 Dec 2008 23:45:04 +0000 Message-ID: <20081203234504.GB4256@fluff.org.uk> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-i2c-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Guennadi Liakhovetski Cc: Darius , linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW@public.gmane.org List-Id: linux-i2c@vger.kernel.org On Wed, Dec 03, 2008 at 10:19:32PM +0100, Guennadi Liakhovetski wrote: > On Wed, 3 Dec 2008, Darius wrote: > > > Guennadi Liakhovetski wrote: > > > > + > > > > +static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx) > > > > +{ > > > > + int result; > > > > + > > > > + result = wait_event_interruptible_timeout(i2c_imx->queue, > > > > + i2c_imx->i2csr & I2SR_IIF, I2C_IMX_TIME_TRX * HZ); > > > > > > 5s is much too long! > > > > how much? 600us? > > mxc uses 1 jiffie. > > > > > + /* write slave address */ > > > > + writeb(msgs->addr, i2c_imx->base + IMX_I2C_I2DR); > > > > > > This is wrong! You have to double the address before writing to the > > > register. > > > > strange! there are I2c board data in my MXLADS code: > > > > struct i2c_board_info __initdata mx1ads_i2c_devices[] = { > > { > > I2C_BOARD_INFO("ov7xxx", 0x42), > > .platform_data = &iclink[0], > > }, { > > I2C_BOARD_INFO("mt9v111", 0x90), > > .platform_data = &iclink[0], > > } > > } > > > > slave addresses are exactly 0x42 and 0x90 (from datasheets). > > my driver works with these devices with address not doubled. > > I saw this in other I2C drivers, but If I double address in my driver, it > > works wrong. > > I tested this with oscilloscope - now it works ok, with all devices I have > > tryed. > > As Mark explained - Linux uses i2c addresses without the read/write bit, > i.e., shifted one bit right. > > > > > +module_param(clkfreq, uint, S_IRUGO); > > > > +MODULE_PARM_DESC(clkfreq, "desired IMX I2C Clock Rate in Hz"); > > > > > > Making clkfreq a module parameter you force the same frequency on all i2c > > > busses. On my i.MX31 system 2 busses are internal and one goes to a > > > connector, to which a camera is connected. This third bus can only handle a > > > lower frequency, which, however, doesn't mean we also have to throttle the > > > other two busses. Can we put this into platform data? > > > > We can do that, but now there is possibility to change bitrate when re-loading > > module. > > What is better? > > But is it really necessary to be able to override this at load-time? At > least not as one single parameter. If you absolutely need this > possibility, maybe an array of frequencies? But then you don't know how > many busses you are going to have. Having an array of 8 ints will probably > be enough for a while:-) Platform data seems a good way of getting a board to declare a bus rate. Otherwise a standard i2c command line option for all busses to use may be the best thing to do. > > > > +struct imxi2c_platform_data { > > > > + int (*init)(struct device *dev); > > > > + int (*exit)(struct device *dev); > > > > > > What are you going to use .exit() for? Is it really needed? Even if it is, > > > it can easily return void I guess? > > > > .init is used to request and setup gpio pins, .exit used to free gpio. > > yes, .exit can return void - I will fix it. > > You mean in your .init() you not only configure iomux pins for i2c, you > also gpio_request(pin_to_gpio(), "i2c")? Now that I think about this, > maybe this is indeed correct, and then you gpio_free() in .exit()... Is > this what you mean? What does David Brownell think of that? -- Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/) 'a smiley only costs 4 bytes'