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

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

[Murray Jensen]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

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

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


[-- Attachment #2: i2c.patch.gz --]
[-- Type: application/x-gzip , Size: 12203 bytes --]

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

[Dan Malek]

Murray Jensen wrote:

> enhanced i2c driver for MPC8xx/MPC8260 CPM ...

These have to be submitted through the i2c project on sourceforge (or
where ever it is).  The i2c isn't part of the kernel source tree, it is
applied on top from their repository from time to time.

Thanks.


	-- Dan


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

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

[Murray Jensen]

On Tue, 28 May 2002 07:01:25 -0400, Dan Malek <dan@embeddededge.com> writes:
>Murray Jensen wrote:
>
>> enhanced i2c driver for MPC8xx/MPC8260 CPM ...
>
>These have to be submitted through the i2c project on sourceforge (or
>where ever it is).  The i2c isn't part of the kernel source tree, it is
>applied on top from their repository from time to time.

Tom said this to me as well, and I agree in principle, but I thought it should
first be reviewed by the Linux/PPC Embedded community. It's actually quite a
hacky driver, but in my defence, I could claim the original driver was hacky
to start with - I really didn't change it a lot, conceptually (what I mean by
this is that it could be much improved in the way it does things - more
correct, more efficient, etc, but hey it works reliably for me, so there isn't
a lot of incentive to fix it - its only i2c after all :-).

Also, the i2c people probably wouldn't have any hope of testing this driver,
since nothing else is going to have a CPM, and in fact I haven't even tested
it myself on the 8xx platform yet. I propose we get it into linuxppc_2_4_devel
and have it hammered on by others, and when/if it gets the thumbs up from the
Linux/PPC Embedded community, we/I contribute it back to the i2c project -
really just for the sake of completeness. Comments? 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/


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

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

[Matt Porter]

On Tue, May 28, 2002 at 09:39:31PM +1000, Murray Jensen wrote:
>
> On Tue, 28 May 2002 07:01:25 -0400, Dan Malek <dan@embeddededge.com> writes:
> >Murray Jensen wrote:
> >
> >> enhanced i2c driver for MPC8xx/MPC8260 CPM ...
> >
> >These have to be submitted through the i2c project on sourceforge (or
> >where ever it is).  The i2c isn't part of the kernel source tree, it is
> >applied on top from their repository from time to time.
>
> Tom said this to me as well, and I agree in principle, but I thought it should
> first be reviewed by the Linux/PPC Embedded community. It's actually quite a
> hacky driver, but in my defence, I could claim the original driver was hacky
> to start with - I really didn't change it a lot, conceptually (what I mean by
> this is that it could be much improved in the way it does things - more
> correct, more efficient, etc, but hey it works reliably for me, so there isn't
> a lot of incentive to fix it - its only i2c after all :-).
>
> Also, the i2c people probably wouldn't have any hope of testing this driver,
> since nothing else is going to have a CPM, and in fact I haven't even tested
> it myself on the 8xx platform yet. I propose we get it into linuxppc_2_4_devel
> and have it hammered on by others, and when/if it gets the thumbs up from the
> Linux/PPC Embedded community, we/I contribute it back to the i2c project -
> really just for the sake of completeness. Comments? Cheers!

FWIW, that's why I pushed the 4xx IIC driver to _devel recently.  It
certainly allows a wider audience to easily test and submit changes
to the maintainer, allowing for faster turnaround in improvements.

Regards,
--
Matt Porter
porter@cox.net
This is Linux Country. On a quiet night, you can hear Windows reboot.

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

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

[Jeremy Rosen]

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

>>> Murray Jensen 05/28/02 03:43pm >>>
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: Murray.Jensen@csiro.au

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

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

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

[Murray Jensen]

[-- Attachment #1: Type: text/plain, Size: 2906 bytes --]

On Fri, 31 May 2002 10:09:00 +1000, Rosen Jeremy <Rosen.Jeremy@tms-pty.com> 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/


[-- Attachment #2: i2c.patch.fix.gz --]
[-- Type: application/x-gzip , Size: 418 bytes --]

[-- Attachment #3: i2c.patch.gz --]
[-- Type: application/x-gzip , Size: 12178 bytes --]

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

[Tom Rini]

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/

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

[Murray Jensen]

On Fri, 31 May 2002 07:47:25 -0700, Tom Rini <trini@kernel.crashing.org> writes:
>> 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>

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

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

Agreed.

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

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/

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

[Tom Rini]

On Sat, Jun 01, 2002 at 12:28:34PM +1000, Murray Jensen wrote:
>
> On Fri, 31 May 2002 07:47:25 -0700, Tom Rini <trini@kernel.crashing.org> writes:
> >> 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>
>
> 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".

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/