[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

Timur Tabi timur at freescale.com
Wed Dec 17 00:46:31 CET 2008


Wolfgang Denk wrote:

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

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

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.

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

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

-- 
Timur Tabi
Linux Kernel Developer @ Freescale


More information about the U-Boot mailing list