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

[ksi at koi8.net]

On Tue, 16 Dec 2008, Timur Tabi wrote:

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

Only if you have more than one I2C bus that 95% of supported boards don't
have (95% is a wild guess; in reality, methinks, it is even closer to 99%.)
That means you should only do this for less than 5% of existing boards.

Adding a parameter would require you to make changes for 100% of boards.

common/cmd_i2c.c is a single file, common for all platforms so it is a
special case.

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

No, that is not an issue. That pib_init() does not happen out of a blue, it
is YOU who call it in $(BOARD).c file so you MUST know what you are doing.

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

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************