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:-)
> > > +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?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
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
