public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Timur Tabi <timur@freescale.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions
Date: Tue, 16 Dec 2008 17:46:31 -0600	[thread overview]
Message-ID: <49483DD7.5080908@freescale.com> (raw)
In-Reply-To: <20081216204954.CF8DA832E8A1@gemini.denx.de>

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

  reply	other threads:[~2008-12-16 23:46 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-12-03 17:28 [U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions Timur Tabi
2008-12-06 17:49 ` Jean-Christophe PLAGNIOL-VILLARD
2008-12-15 22:47 ` Wolfgang Denk
2008-12-15 23:37   ` ksi at koi8.net
2008-12-15 23:42     ` Timur Tabi
2008-12-16  0:24       ` ksi at koi8.net
2008-12-16 15:19         ` Timur Tabi
2008-12-16 17:58           ` ksi at koi8.net
2008-12-16 18:51             ` Timur Tabi
2008-12-16 19:40               ` ksi at koi8.net
2008-12-16 20:35                 ` Jerry Van Baren
2008-12-16 20:58                 ` Wolfgang Denk
2008-12-17  3:55                   ` ksi at koi8.net
2008-12-16 20:49               ` Wolfgang Denk
2008-12-16 23:46                 ` Timur Tabi [this message]
2008-12-17  1:00                   ` ksi at koi8.net
2008-12-17  1:28                   ` Wolfgang Denk
2008-12-16 17:47         ` Scott Wood
2008-12-16 18:07           ` ksi at koi8.net

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=49483DD7.5080908@freescale.com \
    --to=timur@freescale.com \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox