enhanced i2c driver for MPC8xx/MPC8260 CPM ...

Hello, I have just applied your patch, and would like to add a few comments

I beleive you use the denx kernel, which is not 100% compatible with the 2.4.4-devel one.
In particular, commproc.h is in asm/ in the devel kernel. The patch did not apply properly, and I had to do some trivial editing to make it work (note that my last pull from 2.4-devel is quite old) you also need to include commproc.h in i2c-algo-cpm.h, because this is where iic_t is defined

in arch/ppc/8xx_io/commproc.c you reference a cp variable that oesn't exist, I changed it to cpmp, which is probably what you meant

you include the same header twice in i2c-algo-cpm : asm/mpc8xx.h

once compiled, the driver didn't work. I have a very simple test program using /dev/i2c/0 (set the client address, read two bytes, it worked with the old driver),
and it hangs after printing

cpm_iic_read(abyte=0x91
(no closing brace, the 0x91 value is correct)
I inserted
cpm_iic_print_params(cpm);
return 0;

after the blocking printk, and this allowed the printk to finish properly and returned the following info

i2c-algo-cpm.o: #0 addr=0x48 flags=0x1 len=2
cpm_iic_read(abyte=0x91)
i2c regs @ 0xff000860:
mod 00 add fe brg 07 com 01 cer 00 cmr 00
iic params @ 0xff002400:

        rbase 0838 tbase 0828 rfcr 10 tfcr 10 mrblr 0080
        rstate 00000000 rdp 00000000 rbptr 0000 rbc 0000 rxtmp
00000000
        tstate 00000000 tdp 00000000 tbptr 0000 tbc 0000 txtmp
00000000
        res dfcff956 rpbase 0060 res2 2400

Using relocation patch!
i2c-algo-cpm.o: read 0
10,0

I am trying to find out what's going wrong, but any clue would be welcome..

(FYI I have a 860T FADS board with a Maxim DS1775R digital thermometer attached on the IIC bus)
I only tested access through /dev/i2c/0, no sensor drivers.

Thanks Jeremy

enhanced i2c driver for MPC8xx/MPC8260 CPM ...

- replace the 8xx i2c driver with a combined 8xx/8260 driver (or in future
anything else that has a CPM)
- also enhances the code so that the driver can be used as a loadable module
- fixes the i2c-rpx driver to use the combined algo driver, but it really should just be removed, since I'm sure that the generic
i2c-cpm
driver would work on rpx (untested - I don't have an RPX)

Cheers!

								Murray...

--
Murray Jensen, CSIRO Manufacturing Sci & Tech, Phone: +61 3
9662 7763
Locked Bag No. 9, Preston, Vic, 3072, Australia. Fax: +61 3
9662 7853
Internet: [email protected]

Hymod project: http://www.msa.cmst.csiro.au/projects/Hymod/

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

I'm glad someone has tried it...

We've hit a snag straight away - I use linuxppc_2_4_devel grabbed via bitkeeper from "bk://ppc.bkbits.net/linuxppc_2_4_devel"

That's where it is in mine also. Actually, its in "include/asm-ppc" - "include/asm" is just a symlink.

Oh, I see the problem - I didn't update the #include in "drivers/i2c/i2c-algo-cpm.c" - *sigh* I thought I had done this - the others in "drivers/i2c/i2c-cpm.c" and "drivers/i2c/i2c-rpx.c" were done.

My last pull from "bk://ppc.bkbits.net/linuxppc_2_4_devel" was a couple of weeks ago. If yours is very old, you will probably have trouble with at least "arch/ppc/kernel/ppc_ksyms.c" (but it should be obvious what is required in there).

I think the file that includes <linux/i2c-algo-cpm.h> is expected to include commproc.h, or cpm_8260.h, as required. What is the policy on these things? Should every include file include all headers it needs? Anyone?

The original <linux/i2c-algo-8xx.h> file only included <linux/i2c.h>, yet referenced the "i2c8xx_t" and "cpm8xx_t" types. I simply did the same.

Yes, indeed - you are correct (when I made those additions, there was a cp local variable - it got removed later).

OK, as I said in my message, I haven't tested this on the 8xx platform. Obviously, it needed it - but I think we can work together to get it going.

It is only included once in my source.

I have attached two patches - the first is a further patch to the previously posted patch, the second is the entire thing in one patch. These fix the obvious compile problems you point out.

Note that I have a separate repository which has the vanilla linuxppc_2_4_devel as it's parent, and only contains these i2c patches. The last time I pulled was a couple of weeks ago - there shouldn't be any of my other local stuff interfering with this.

Should we take this discussion off the list now? Cheers!

								Murray...

--
Murray Jensen, CSIRO Manufacturing Sci & Tech, Phone: +61 3 9662 7763
Locked Bag No. 9, Preston, Vic, 3072, Australia. Fax: +61 3 9662 7853
Internet: [email protected]

Hymod project: http://www.msa.cmst.csiro.au/projects/Hymod/

IMHO, a header file should only include other things which the header itself needs. eg if you do 'u8 foo;' in foo.h, add #include <asm/types.h>

Anything the C file needs itself, it should include, and for the sake of being explicit (and it's good for multi-arch drivers) if the C code does 'u8 bar;' it should do #include <asm/types.h> too.

Nah..

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

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

OK, but should it include *all* headers it needs? If so, then the existing <linux/i2c-algo-8xx.h> should include both <asm/8xx_immap.h> and <asm/commproc.h> since it uses the types "i2c8xx_t" and "cpm8xx_t".

Agreed.

I was referring to the discussion about the combined 8xx/8260 i2c driver I posted. I agree - the above should stay on the list. Cheers!

								Murray...

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

It a header should include all headers that it needs. If linux/i2c-algo-8xx.h has 'cpm8xx_t foo;' then it should have been including <asm/8xx_immap.h> :)

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

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