Re: [PATCH][RFC]Updated MPC I2C driver

Hi

Adrian Cox wrote:

On Fri, 2004-07-02 at 12:01, Sylvain Munaut wrote:

quoted

#include <asm/ppcboot.h>
extern bd_t __res;
u32 ipbfreq = __res.bi_ipbfreq;

But this field will only exists when :
- CONFIG_PPC_MPC52xx symbol is defined.
- When the MPC52xx patch is applied to the kernel

Maybe we should define two more fields in the ocp_fs_i2c_data structure:
one for base clock, and one for i2c clock. Then platform code could fill
in the clocks as necessary.

Yes, that's a good idea. It would need a flag to tell which clock
computation to take though.
Maybe instead of passing the baseclock, giving a pointer to a u32 that
contains the clock would be more appropriate. Since all board have
probably an int somewhere holding that value, you can put de definition
in the ocp_def without any further modifications.

quoted

Also, there is no DFSRR register on the 5200.

I noticed that. I don't think anybody ever used anything but the default
value on the other chips.

Yes. I've seen that the DFSRR is not as the same place. So maybe we
would need a two bits flag
like
#define FS_I2C_NO_DFSRR 0x00
#define FS_I2C_PACKED_DFSRR  0x02
#define FS_I2C_SEPARATE_DFSRR 0x04

quoted

Are you sure ? If I don't set the BNBE (Bus Not Busy Enable) bit, I just
get timeouts.

quoted

From the manual it looks as if setting BNBE might cause extra

interrupts, which the driver has no way to handle.  Could you try
enabling the interrupts, and see if this happens?

quoted

Yes sure, that's the easiest way. It's just that I'd like to avoid it.
Especially when it's content is dependent on if the user has choosed to
use irq or not.
But It's sure is a pity that the register is shared between the two I2C
... Because even with a flag, the driver should be passed the address of
this register, and what bits to use.

How about putting a function pointer for platform interrupt enabling and
disabling into the ocp_fs_i2c_data?

Well, you're right setting the BNBE is not right, it just hang because
interrupts are fired and not handled.
Now, here is the interesting part :
If I set the ocp_def to use both I2C controller, defining the I2C1
before I2C2, it works fine. Now if I just invert the entry, it does not.
Interrupts for I2C2 are never fired.
If I only put I2C1, it seems to work ( no timout). With only I2C2,
interrupts are never fired ...

I have no clue why ! The best option is just to set IRQ to OCP_IRQ_NA,
and it works fine, whatever the value of the shared interrupt registers is.

It looks to me that supporting the 5200 will require a lot of small
changes.

Well, for the interrupt yes that a quirk. The solution of dropping
interrupt completly for it is the best option for now.
For the DFSRR register, the solution mentionned above is clean, the 3
chips requires different implementation anyway.
For the clock thing, I think it's good to be able to set the I2C clock.


Sylvain Munaut


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/