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 ([EMAIL PROTECTED], http://www.fluff.org/)
'a smiley only costs 4 bytes'
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
