i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

All implementations of the functions i2c_reg_read() and i2c_reg_write() are identical. We can save space and simplify the code by converting these functions into inlines and putting them in i2c.h.

Signed-off-by Timur Tabi <[email protected]>

All implementations of the functions i2c_reg_read() and i2c_reg_write() are identical. We can save space and simplify the code by converting these functions into inlines and putting them in i2c.h.

Signed-off-by Timur Tabi <[email protected]>

---

v3: add 8xx-specific code, and include PRINTDs from blackfin and pxa

I'm posting this patch because I'm enhancing the I2C routines to support multiple I2C busses more easily, but I need to clean up the existing code first.

I'm going to be on vacation when the next merge window opens, so I'm posting this now in the hopes that it will be picked up when the window does open.

 cpu/arm920t/at91rm9200/i2c.c  |   14 ---------
 cpu/arm926ejs/davinci/i2c.c   |   17 -----------
 cpu/blackfin/i2c.c            |   16 ----------
 cpu/mpc512x/i2c.c             |   17 -----------
 cpu/mpc5xxx/i2c.c             |   16 ----------
 cpu/mpc8220/i2c.c             |   16 ----------
 cpu/mpc824x/drivers/i2c/i2c.c |   14 ---------

 cpu/mpc8260/i2c.c             |   16 ----------
 cpu/mpc8xx/i2c.c              |   33 ---------------------
 cpu/ppc4xx/i2c.c              |   20 -------------
 cpu/pxa/i2c.c                 |   15 ----------
 drivers/i2c/fsl_i2c.c         |   16 ----------
 drivers/i2c/soft_i2c.c        |   19 ------------
 include/i2c.h                 |   62 +++++++++++++++++++++++++++++++++++++++-
 14 files changed, 60 insertions(+), 231 deletions(-)

Dear Timur Tabi,

In message <[email protected]> you

Quoted 1 line · wrote:

All implementations of the functions i2c_reg_read() and

i2c_reg_write() are

Quoted 1 line

identical. We can save space and simplify the code by converting

these

Quoted 8 lines

functions into inlines and putting them in i2c.h.

Signed-off-by Timur Tabi <[email protected]>

---

v3: add 8xx-specific code, and include PRINTDs from blackfin and pxa

I'm posting this patch because I'm enhancing the I2C routines to

support

Quoted 1 line

multiple I2C busses more easily, but I need to clean up the existing

code

Quoted 3 lines

first.

I'm going to be on vacation when the next merge window opens, so I'm

posting

Quoted 1 line

this now in the hopes that it will be picked up when the window does

open.

We've been discussing this long enough when the previous window was open, so ...

Quoted 6 lines

 cpu/arm920t/at91rm9200/i2c.c  |   14 ---------
 cpu/arm926ejs/davinci/i2c.c   |   17 -----------
 cpu/blackfin/i2c.c            |   16 ----------
 cpu/mpc512x/i2c.c             |   17 -----------
 cpu/mpc5xxx/i2c.c             |   16 ----------
 cpu/mpc8220/i2c.c             |   16 ----------

Quoted 8 lines

 cpu/mpc824x/drivers/i2c/i2c.c |   14 ---------
 cpu/mpc8260/i2c.c             |   16 ----------
 cpu/mpc8xx/i2c.c              |   33 ---------------------
 cpu/ppc4xx/i2c.c              |   20 -------------
 cpu/pxa/i2c.c                 |   15 ----------
 drivers/i2c/fsl_i2c.c         |   16 ----------
 drivers/i2c/soft_i2c.c        |   19 ------------
 include/i2c.h                 |   62

+++++++++++++++++++++++++++++++++++++++-

Quoted 1 line

 14 files changed, 60 insertions(+), 231 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

Signature · 10 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
All these theories, diverse as they are, have two things  in  common:
they explain the observed facts, and they are completeley and utterly
wrong.                       - Terry Pratchett, _The Light Fantastic_
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

On Mon, 15 Dec 2008, Wolfgang Denk wrote:

Are you going to implement support for multiple I2C busses on some boards? I'm working on something like this now so it would be nice to coordinate our efforts somehow...

Quoted 3 lines · ksi at koi8.net wrote:

On Mon, 15 Dec 2008, Wolfgang Denk wrote:

Are you going to implement support for multiple I2C busses on some

boards?

Quoted 1 line

I'm working on something like this now so it would be nice to

coordinate our

Quoted 1 line

efforts somehow...

Yes, my goal is to add low-level i2c functions that take an i2c bus as a parameter. So instead of

	i2c_set_bus_num(1);
	i2c_read(...);

you can do

	i2c_read(1, ...)

or something like that. My goal is to eliminate i2c_set_bus_num() and everything related to it.

It sounds like we're working on something very similar. This is very low-priority for me, though.

That looks similar. But why do you want to remove i2c_set_bus_num()? I think it would be less work to keep it.

This way you can leave 90% or so of
existing I2C code unchanged by setting bus number to 0 at init.

My idea is to have global bus number variable in a single place and a single i2c_set_bus_num() that can be excluded for most boards with a single bus with #ifdef...

Then, we could use some kind of array of I2C structures each containing pointers to appropriate i2c-{read,write,probe,init}() functions with generic i2c functions just calling those pointers using bus number as index into that array.

That would allow for unlimited number of different adapters for any board.

Initial code for initializing such an array would have to go into each and every $(BOARD).c board specific file.

Quoted 1 line · ksi at koi8.net wrote:

That looks similar. But why do you want to remove i2c_set_bus_num()? I

think

Quoted 1 line

it would be less work to keep it.

Perhaps, but it would be even better to get rid of it. IMHO, it's a kludge. It
was a hack added to allow existing I2C routines to function while adding minimal
support for multiple buses on those platforms that needed it.

Quoted 2 lines

This way you can leave 90% or so of
existing I2C code unchanged by setting bus number to 0 at init.

I only intend on exporting the multiple-bus versions of the I2C function if
CONFIG_I2C_MULTI_BUS is defined.

Quoted 1 line

My idea is to have global bus number variable in a single place and a

single

Quoted 1 line

i2c_set_bus_num() that can be excluded for most boards with a single

bus

Quoted 1 line

with #ifdef...

We already have something like that. A global variable is inconvenient because
every time you want to access the bus, you need to do something like this:

	bus = i2c_get_bus_num();
	i2c_set_bus_num(x);
	i2c_write(...)
	i2c_set_bus_num(bus);

We need to save/restore the current bus number, because the I2C command-line has
the concept of a

Quoted 1 line

Then, we could use some kind of array of I2C structures each

containing

Quoted 1 line

pointers to appropriate i2c-{read,write,probe,init}() functions with

generic

Quoted 1 line

i2c functions just calling those pointers using bus number as index

into

Quoted 1 line

that array.

Sounds complicated.

Quoted 1 line

That would allow for unlimited number of different adapters for any

board.

Ah, now this is something else entirely. I don't think U-boot supports this at
all. I think you're being too ambitious. It's a noble idea, and I think U-boot
should support it, but I think we need to simplify the support for multiple
buses first.

Quoted 1 line

Initial code for initializing such an array would have to go into each

and

Quoted 1 line

every $(BOARD).c board specific file.

Ugh.

That looks messy... Why would we use two different versions if we can make everything uniform?

Eh, you can just set bus number every time you're gonna do i2c read/write...

That i2c_get_bus_num() doesn't make any sence at all. Just set bus number every time you access i2c device.

U-boot is single-task so there is no other
process that can change it from under you and you do not save anything with checking that bus number.

Quoted 1 line · ksi at koi8.net wrote:

That looks messy... Why would we use two different versions if we can

make

Quoted 1 line

everything uniform?

Because we already have something that makes it uniform, and it's broken. The
idea of having a "current i2c bus" that needs to be set before read/write
operations can be performed is the broken part!

Quoted 1 line

Eh, you can just set bus number every time you're gonna do i2c

read/write...

Not with the current i2c command line. We would need another global variable in
the i2c command line code to store what IT thinks is the current bus.

Quoted 1 line

That i2c_get_bus_num() doesn't make any sence at all. Just set bus

number

Quoted 1 line

every time you access i2c device.

That's risky. Sooner or later, you will want to know what the current bus
number is, at least for debugging or status purposes.

Quoted 2 lines

U-boot is single-task so there is no other
process that can change it from under you and you do not save anything

with

Quoted 1 line

checking that bus number.

Sounds to me like you haven't really looked at the U-Boot code. There are
plenty of places where one function does I2C operations, then calls another
function that does its own.

I think all this boils down to one core disagreement we have: I think the idea
of a "current" i2c bus is a bad one.

Quoted 10 lines · Timur Tabi

Quoted 1 line · ksi at koi8.net wrote:

That looks messy... Why would we use two different versions if we can

make

Quoted 1 line

everything uniform?

Because we already have something that makes it uniform, and it's broken. The
idea of having a "current i2c bus" that needs to be set before read/write
operations can be performed is the broken part!

Eh, we don't have any uniformity. That "uniformity" we do have is only for a trivial case of a single I2C bus. Everything else is a bunch of SoC specific hacks made differently fo different platforms. The fsl-i2c.c e.g. uses local bus number variable.

Quoted 3 lines

I think all this boils down to one core disagreement we have: I think the idea
of a "current" i2c bus is a bad one.

I can see nothing wrong with that but I also have nothing against changing all i2c_{read,write,probe}() functions to take bus number as an argument.

I offered 4 possible scenarios and additional parameter to i2c functions was one of them. Wolfgang said that current bus approach looks better than others and I agree with him. But it is not rocket science to use an additional parameter either. The only thing is it MUST be consistent, i.e. we should NOT have 2 different sets of functions based on CONFIG_MULTIBUS or whatever. If we are to make this change ALL boards must be multibus with majority of them having bus count of 1.

Does anybody else have something to say on this? I'm going to write code so let's make some decision. I don't want this to end up as a company hack and making it properly affects a lot of U-boot...

I offered 4 possible scenarios and additional parameter to i2c functions was one of them. Wolfgang said that current bus approach looks better than others and I agree with him. But it is not rocket science to use an

additional parameter either. The only thing is it MUST be consistent, i.e. we should NOT have 2 different sets of functions based on CONFIG_MULTIBUS or

whatever. If we are to make this change ALL boards must be multibus with majority of them having bus count of 1.

Does anybody else have something to say on this? I'm going to write code so let's make some decision. I don't want this to end up as a company hack and making it properly affects a lot of U-boot...

Dear ksi at koi8.net,

In message <[email protected]> you

Quoted 2 lines · wrote:

I offered 4 possible scenarios and additional parameter to i2c

functions was

Quoted 2 lines

one of them. Wolfgang said that current bus approach looks better than others and I agree with him. But it is not rocket science to use an

You and me agree on this.

Quoted 1 line

additional parameter either. The only thing is it MUST be consistent,

i.e.

Quoted 1 line

we should NOT have 2 different sets of functions based on

CONFIG_MULTIBUS or

We agree on this, too.

Quoted 1 line

whatever. If we are to make this change ALL boards must be multibus

with

Quoted 1 line

majority of them having bus count of 1.

I don't think this makes sense - it would just add complexity and memory footprint without added benefit.

Quoted 1 line

Does anybody else have something to say on this? I'm going to write

code so

Quoted 1 line

let's make some decision. I don't want this to end up as a company

hack and

Quoted 1 line

making it properly affects a lot of U-boot...

I support your position, not Timur's.

Quoted 2 lines · ksi at koi8.net wrote:

That looks messy... Why would we use two different versions if we can make everything uniform?

Because we already have something that makes it uniform, and it's broken. The idea of having a "current i2c bus" that needs to be set before read/write operations can be performed is the broken part!

Quoted 1 line

Eh, you can just set bus number every time you're gonna do i2c read/write...

Not with the current i2c command line. We would need another global variable in the i2c command line code to store what IT thinks is the current bus.

Quoted 2 lines

That i2c_get_bus_num() doesn't make any sence at all. Just set bus number every time you access i2c device.

That's risky. Sooner or later, you will want to know what the current bus number is, at least for debugging or status purposes.

Sounds to me like you haven't really looked at the U-Boot code. There are plenty of places where one function does I2C operations, then calls another function that does its own.

I think all this boils down to one core disagreement we have: I think the idea of a "current" i2c bus is a bad one.

Hm... what exactly is broken with the concept of having a "current device" or a "current bus"? We use it elasewhere, too (like for selection IDE or S-ATA devices and such), and so far I am not aware of fundamental issues because of that.

Quoted 3 lines

Sounds to me like you haven't really looked at the U-Boot code. There are plenty of places where one function does I2C operations, then calls another function that does its own.

Really? Where exactly does such a thing happen?

I tend to call this a bug that needs to be fixed, if there is ssuch code.

This statement is probably correct: I don't share your point of view here, either.

Quoted 4 lines · Wolfgang Denk

Hm... what exactly is broken with the concept of having a "current device" or a "current bus"? We use it elasewhere, too (like for selection IDE or S-ATA devices and such), and so far I am not aware of fundamental issues because of that.

I think it's a kludge because you have to set the current device before you
can access it. It seems ridiculous that you have to do this:

	i2c_set_bus_num(x)
	i2c_write(...)

when you could do this:

	i2c_write(x, ...)

Quoted 1 line

Quoted 1 line

Sounds to me like you haven't really looked at the U-Boot code.

There are

Quoted 5 lines

Quoted 3 lines

plenty of places where one function does I2C operations, then calls another
function that does its own.

Really? Where exactly does such a thing happen?

Well, I can't find a real example, but here's something close. Take a look
at function pib_init() in mpc8568mds.c. This function needs to talk to the
2nd I2C bus. It has no idea who called it, so it has no idea what the current I2C bus is. Therefore, it has to save and restore it.

Now consider the code that calls pib_init(). Technically, this code cannot
know that pib_init() does I2C operations. So it doesn't know that pib_init()
could change the current I2C bus. So it wouldn't know to save and restore
what it needs.

This situation could happen anywhere.

Quoted 2 lines

I tend to call this a bug that needs to be fixed, if there is ssuch code.

It's not a bug. Function X does I2C operations, and function Y also does I2C
operations, but on a different device. If function X calls function Y, then
Y needs to save and restore the current bus number. You will never get rid
of this requirement as long as you have the concept of a current I2C bus number.

So you're still going to need i2c_get_bus_num() and i2c_set_bus_num(), and
you're still going to have code like this:

	bus = i2c_get_bus_num();
	i2c_set_bus_num(x);
	i2c_write(...)
	i2c_set_bus_num(bus);

Quoted 2 lines

This statement is probably correct: I don't share your point of view here, either.

In that case, I have no interest in working on this new I2C design. You guys
obviously have everything under control, so good luck.

Signature · 2 lines

Timur Tabi
Linux Kernel Developer @ Freescale

I think it's a kludge because you have to set the current device before you can access it. It seems ridiculous that you have to do this:

	i2c_set_bus_num(x)
	i2c_write(...)

when you could do this:

	i2c_write(x, ...)

Quoted 1 line

Really? Where exactly does such a thing happen?

Well, I can't find a real example, but here's something close. Take a look at function pib_init() in mpc8568mds.c. This function needs to talk to the 2nd I2C bus. It has no idea who called it, so it has no idea what the current I2C bus is. Therefore, it has to save and restore it.

This situation could happen anywhere.

It's not a bug. Function X does I2C operations, and function Y also does I2C operations, but on a different device. If function X calls function Y, then Y needs to save and restore the current bus number. You will never get rid of this requirement as long as you have the concept of a current I2C bus number.

So you're still going to need i2c_get_bus_num() and i2c_set_bus_num(), and you're still going to have code like this:

	bus = i2c_get_bus_num();
	i2c_set_bus_num(x);
	i2c_write(...)
	i2c_set_bus_num(bus);

In that case, I have no interest in working on this new I2C design. You guys obviously have everything under control, so good luck.

Then, we could use some kind of array of I2C structures each containing pointers to appropriate i2c-{read,write,probe,init}() functions with generic i2c functions just calling those pointers using bus number as index into that array.

Quoted 1 line · On Mon, Dec 15, 2008 at 04:24:26PM -0800, ksi at koi8.net wrote:

Then, we could use some kind of array of I2C structures each

containing

Quoted 1 line

pointers to appropriate i2c-{read,write,probe,init}() functions with

generic

Quoted 1 line

i2c functions just calling those pointers using bus number as index

into

Quoted 1 line

that array.

Why not just pass a pointer to the i2c device, rather than an index?