Re: [PATCH] enhanced i2c driver for MPC8xx/MPC8260 CPM ...

On Fri, 31 May 2002 10:09:00 +1000, Rosen Jeremy [off-list ref] writes:

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

I'm glad someone has tried it...

I beleive you use the denx kernel, which is not 100% compatible with the
2.4.4-devel one.

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"

In particular, commproc.h is in asm/ in the devel kernel.

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.

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)

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).

you also need to include commproc.h in i2c-algo-cpm.h, because this is where
iic_t is defined

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.

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

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.

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

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: Murray.Jensen@csiro.au

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

On Fri, May 31, 2002 at 11:32:41AM +1000, Murray Jensen wrote:

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?

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.

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

Nah..

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

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