[i2c] [patch 4/5] i2c board_info and i2c_new_device()

[David Brownell]

This response primarily addresses issues of how I2C busses get identified.

> > > > +	short		bus_num;
> > > 
> > > I don't like this. This assumes you know in advance the numbers that
> > > will be assigned to at least some of the busses. This is true only in a
> > > limited number of cases,
> > 
> > This is the same addressing model already used by the I2C stack to
> > specify probing through module parameters!  Either that stack has
> > major problems in that area *today* or else this isn't a real issue.
> > IMO it's the latter situation.
> 
> You are right that this is the same addressing model already in use by
> our i2c module parameters. 

Busses without hardware autoconfiguration support need some
kind of stable identifiers to support configuration tables.
Assigning integers is a classic solution.


> This model didn't work too bad 5 years ago, 
> but the times they are a-changin', PC systems have many more i2c busses
> now, those order will quickly change depending on which drivers are
> loaded and the order in which they are loaded. 

Only for cases where those busses are dynamically created.
That may be common on PCs, but they are hardly the only relevant
system environment.

This is a fair example of "begging the question":  you are
using key parts of your proposal (no stable IDs) as a starting
assumption.  That's a good way to get your preferred answer (hey,
no such IDs!), but not a good way to explore otherwise.  :)


> Just enable 
> CONFIG_PCI_MULTITHREAD_PROBE and watch everything break.

That breakage is not related to the use of numbers for bus
identification (PCI certainly uses them).  It has more to do
with PCI drivers (and other infrastructure) assuming it's
single threaded during those init sequences.


> So I'd like to avoid repeating the mistake in your new code.

You'd need to identify a "mistake" however!  At this point all you
are doing is objecting to the reality of stable I2C bus identifiers
on some systems.  Which to me seems a self-evidently unsupportable
objection; the schematics for mainboard X are not going to change,
anything hooked up to hardware bus #1 won't jump to bus #2 absent
some kind of re-wiring (making it into board Y).


> > This works fine for SOC designs, which are the primary customer of
> > the i2c_register_board_info() calls.
> 
> Designing things for the sole purpose of their primary customers
> doesn't age well, as you should be well aware.

Not accounting for primary customers is called a "non-starter", as
I'm sure you are likewise aware.  :)

Likewise, false generalization is a big lose, unless you count
complexity and code bloat as a win.


> I want a generic 
> mechanism, which works for everyone. And complement it as needed to fill
> the additional needs of the SoC decigns, rather than the other way
> around.

Me too.  That's what I believe I've done here.  The generic mechanism
takes a descriptor (i2c_board_info) and an adapter, then instantiates
such a device as a child of that adapter node:  i2c_new_device().

The "complement" is a mechanism which builds on that, adding tables
which are needed needed with embedded and SOC designs:


> > > and this is quite fragile IMHO. This design 
> > > decision causes the whole mechanism to be only usable in this limited
> > > number of cases - typically embedded systems. Multimedia adapters won't
> > > be able to use it for example.
> > 
> > Those adapters will be using i2c_new_device() and pass both the adapter
> > and the i2c_board_info.  In that context the bus_num is ignored.
> 
> This is frustrating. I would like media adapter drivers (or for that
> matter, any other driver with creates one or more i2c adapters) to be
> able to provide a table of "static" clients and have i2c core
> instantiate them.

Frustrating why?  That works *just fine* with the current scheme:

  (1) create the adapter(s)
  (2) for each device you know is connected to those adapters:
	  call i2c_new_device(adapter, descriptor)

Perhaps you're saying you'd like to have the loop handled by i2c-core?
That's a trivial change, which doesn't affect the basic model.

Note also the memory management implication:  i2c_register_board_info()
needs private copies of data, available to the I2C stack, to apply later.

But such a loop (even if it becomes an i2c core function) doesn't need
to keep a copy of the data.  That's the right model for e.g. the Nth
instance of a given media adapter card, which might vanish because of
a PCI hotplug mechanism along with its I2C adapter and devices (while
other instances of the driver, and their devices, stick around).


> i2c_new_device() is convenient to have for the more 
> complex cases, but the basic cases should be handled by the boardinfo
> mechanism IMHO, and currently aren't (because in general you don't know
> the i2c bus numbers beforehand.)

Erm, it ** IS ALREADY HANDLED USING I2C_BOARD_INFO ** as shown above.

There are two coupled mechanisms.  The core one uses boardinfo to create
a single device, and may be used as soon as the relevant adapter exists.
The other records boardinfo for *later* use, before adapters exist.

 
> > > I don't like the idea that we're introducing a new API which will only
> > > work in some cases. This is exactly what you are blaming the current
> > > API for - that it was too hardware-monitoring centric. I don't want to
> > > reproduce the same mistake. More about that below.
> > 
> > As I noted, this is not a new abstraction in the API.  It's the same one
> > that's currently in use.  And it works ... solving a key problem: how to
> > declare devices in platform-specific code, before their driver model parent
> > nodes could possibly exist.
> 
> Granted, this is a problem which needs to be addressed, but there must
> be another way.

These are digital computers.  It's going to boil down to some kind of
bit string, a.k.a. number...


> It is new in a sense: so far, only user-space assumed bus number
> values to be predictable. With your proposal, the kernel code starts
> making a similar assumption. I didn't like user-space doing it, I don't
> like the kernel code starting to do it.

The only kernel code that would depend on it would be kernel code which
has a right to do so.  For example, it was written by someone who has
board schematics in hand.  Note that such a person may well need to work
from userspace as well as within the kernel ... don't conflate "user-space"
with "has no business knowing the actual hardware configuration" which
will normally be expressed using bus numbers.  (I must have dozens of
board schematics sitting around, all using bus numbers when there's more
than a single instance of a given bus -- I2C or otherwise.)


> > Since this uses the *current* bus identifier abstraction, I'm quite
> > surprised you have an issue with it...
> 
> Maybe I should have been more voicy about how much I dislike it. I've
> been discussing it privately with Greg KH and with Mark M. Hoffman a
> number of times, but maybe never on a mailing list, so you couldn't
> guess, sorry. So let me state it officially here for the records:
> 
> The module parameters declared by i2c chip drivers suck, they are fat
> and fragile, and I want to get rid of them someday.

Good!  And:  the sooner, the better!


> Here, done ;)

Sure, but "nasty module parameters" are distinct from "stable identifiers
for each hardware bus".

You're conflating two unrelated issues.  The fact that the module params suck
painful kidney stones doesn't mean there's anything at all wrong with using
integers as stable bus identifiers.   The fact that board designs use such
numbers, and have done so for many years, is proof to the contrary.


> > > This looks quite inefficient. I really don't buy the whole logic. First
> > > you declare the devices using bus numbers, then you group all the
> > > devices regardless of the bus number, then you create the busses and
> > > they receive their number, and finally you must check all the devices
> > > against all the adapters to find the matches.
> > 
> > We need a solution that supports device addresing in arch_initcall() code,
> > before i2c-core is loaded and before any i2c_adapter is created.  It needs
> > to accomodate the reality that systems can be built from stacks of boards,
> > with each board including its own I2C busses and bus segments.  So
> > 
> >  - Declare devices using bus numbers ... basically matching chip specs,
> >    which map to board schematics, and working in the absence of software.
> >    There'd usually be one call for the mainboard devices; sometimes there
> >    would be a call for devices on other boards.
> > 
> >  - Group all the devices ignoring busnum ... no need for any complex
> >    data structure, there usually aren't many devices anyway;
> 
> We obviously don't agree on what's complex and what's not ;)

It's news to me that anyone could consider a linked list, or an array,
or a trivial combination of the two (like that), as complex.  Even Linus'
classic "bright high school student" can handle that.


> >  - Create the busses and they receive their number ... I'd say it's more
> >    like they "claim" their number, as assigned on board/chip schematics;
> 
> This is something definitely new in your design, it never worked that
> way so far.

True.  And that statement is isomorphic with saying that the system never
had a good way to pre-define configurations of boards with I2C devices.
As you said above, "fragile".

 
> >  - Finally check ... yes, that can't happen before the i2c_adapter driver
> >    for the relevant bus has been registered.
> > 
> > Correctness is a primary goal here.  It doesn't happen enough for there
> > to be an issue with efficiency.
> > 
> > You don't seem to be looking at patch #5/5 in these comments, or in fact
> > any of the platform conversions -- I posted AT91 and OMAP code earlier,
> > and there was a PPC/OpenFirmware conversion too, it's all in the list
> > archives -- which use this stuff.  It's easier to see these answers if
> > you look at the whole context.
> 
> You're right that I had not read these former posts, neither patch #5/5
> of this series. This is done now, so I think my understanding of what
> you did and why you did it is complete.

We'll see about that!  I'll agree on "better" for sure.  :)


> On the other hand, one post from December caught my attention:
> http://lists.lm-sensors.org/pipermail/i2c/2006-December/000586.html
> 
> Basically Scott Wood had more or less the same objection I am having,
> and proposed the exact same solution: that an array of I2C device
> descriptions could be attached to i2c adapters before we register them.

In both cases, what I see is hand-waving.

Because neither you nor he showed another way (much less a better one!)
to identify those adapters before they get registered.  And that's the
fundamental question.

Remember also that adapter drivers must not in general know what they're
hooking up to ... this is a basic driver modularity goal.  If they tried
to know such things, then the adapter driver would need to change for
every board.  The notion that adding support for a simple variant of one
board must always involve patching drivers all over the kernel tree, rather
than just adding one arch/.../.../../board-XXX.c file and maybe tweaking
a few minor bits elsewhere, seems deeply wrong to me.  (A driver for a
specific DVB card is a different case.)

 
> > > I really fail to see why you need all this complexity (list, mutex,
> > > concept of static i2c busses vs. dynamically allocated ones, separate
> > > source file for the device declaration function...) I would prefer
> > > something less embedded-centric, that would work for everyone, without
> > > introducing new concepts and files.
> > 
> > I'll note that the OpenFirmware tables of I2C devices have very similar
> > issues to the embedded systems.  In both cases, the problem is neatly
> > phrased as the way the current I2C stack omits a *fundamental* concept:
> > that of a pre-defined table of devices, meaningful before any I2C stack
> > is active because it just describes the hardware.  (Not unlike PNPACPI
> > does for ISA/LPC devices, which likewise can't really autoconfigure by
> > using a hardware probing scheme.)
> 
> My approach should support that too as far as I can see, just in a
> different way.

At best, you're still hand-waving.  To actually take off, you need wings
built on a foundation of "here's an alternate scheme for IDs matched to
the hardware".


> > > What is wrong with this more simple approach:
> > > 
> > > i2c_adapter gets an extra optional member, a pointer to an array of
> > > struct i2c_board_info. As each struct i2c_board_info is attached to a
> > > given i2c_adapter,
> > 
> > That presumes that the i2c_adapter has already been created.  See above;
> > the board-specific initialization normally runs a long time before the
> > i2c_adapters get registered.
> 
> No, it doesn't. It only presumes that we associate the i2c_board_info
> with the i2c_adapter as we create it. Said i2c_board_info can be (and
> should have been) created by the board-specific initialization long
> before that, same as in your model.

But exactly how would that association be done?

Note by the way that you're assuming that the system is constructed
in a way that having a single array makes sense.  One family of
counterexamples uses cpu cards plugging into an I/O chassis:

 - CPU card has SOC (with I2C controller), flash, DRAM, I2C RTC; with
   many I/O signals, including I2C, routed to card edge connectors.
   I went to find an example, and the first card I checked (!) was
   an exact match:  www.cogcomp.com, csb735, ARM i.MX21 SOC.

 - System integrator takes that CPU card and sticks it into a box which
   adds various I/O devices ... including some on the I2C bus.  Plus
   of course they add lots of custom software, e.g. to monitor some
   industrial process, the weather, etc.  (Box may have multiple cards,
   or not.)

Obviously, the clean way to package something like that splits out
the CPU card support from the rest of the system, so the core will
declare everything found on the CPU card and then additionsl logic
will hook up the I2C devices on external bus segments.  Both of those
use i2c_register_board_info() calls, for the same I2C bus.

That kind of scenario is easily handled by patches #4/#5.  But not
by any model constrained to a *single* array of I2C devices, without
ugly #ifdeffery.  The responsibility for declaring those devices
can't naturally be centralized, since the devices can be split between
two or more boards.  The best way to build systems like that is to
have one module to set up each board, connecting them later.


> > However, note that you're sketching exactly what i2c_new_device() does.
> > That function can be used for certain kinds of plug-in scenarios, but
> > it can't address the canonical example of seeding the I2C stack with
> > device tables *early* in system boot.
> 
> Except that i2c_new_device can only create one device at once, and it
> must be called explicitely for every device. I agree it works, but I
> believe that such "manual" calls should be the exception, not the rule.

So having a loop in e.g. the code for a particular DVB card, rather than
in the I2C code, bothers you?

 
> > > we don't need the bus_num field, and we also don't 
> > > need to test for matching bus numbers afterwards.
> > 
> > Consider six different boards, using the same SOC i2c_adapter driver.
> > Two use controller #1 (bus_num == 1); two use controller #2 (bus_num == 2);
> > two use both controllers.  Each has a different set of I2C devices connected
> > to those busses.  That i2c_adapter driver may be configured as a module.
> > 
> > Now, the board-specific init logic wants to run early, then get scrubbed
> > from memory before starting init ... which means it's got to finish well
> > before the I2C adapter module would be modprobed.  It will register one
> > or two platform_device nodes for those adapters.  It also does something
> > for the I2C devices on those busses.
> 
> Agreed.
> 
> > Using the scheme in this patch (and patch #5/5) this is easily done;
> > the arch_init() code registers the board info, the i2c_adapter driver
> > module calls i2c_register_adapter(), device nodes get created, hotplug
> > events get issued, the drivers for those devices get loaded, all is fine.
> > 
> > Using that "more simple" approach, some board-specific code needs to run
> > after the adapter driver is loaded.  That's a significant change not just
> > to the I2C stack, but also to the init sequence of all architectures...
> 
> I don't think we need to do that. Either I am overlooking something, or
> you didn't understood exactly what my plan is (i.e. I didn't express
> myself properly.)

You actually didn't include the key details, like what would
distinguish one adapter from another *before* the i2c_adapter
was created by its driver.  (Which is why I kept asking about
that particular detail ...)

 
> > > When an i2c_adapter is registered (in i2c_add_adapter), i2c-core looks
> > > at the associated i2c_board_info data and calls i2c_new_device for all
> > > entries. Likewise, when an i2c_adapter is unregistered, it first calls
> > > i2c_unregister_device for all the device listed in the i2c_board_info
> > > array.
> > 
> > So how are you expecting that i2c-core would identify an adapter?

Notice how often I was asking that question, and that you still
did not answer it ...


> > If the answer is "use a 'struct i2c_adapter *' that's exactly what
> > the i2c_new_device() in this patch does ... but that can NOT work
> > for the six board scenario I sketched above, and it does not handle
> > the cases i2c_register_board_info() and i2c_register_adapter() handle.
> 
> I don't think i2c-core needs to "identify an adapter".

See above.  If there's no way to do that, then how could you possibly
make sure the devices get added to the correct bus?

How to identify adapters is a fundamental question.


> I do not want to 
> include an i2c_adapter identifier of any kind in i2c_board_info, but
> the other way around: I want to include i2c_board_info records in the
> i2c_adapter definition.

That seems to contradict what you said earlier, about how the board info
can be recorded before the adapter is defined... you couldn't record
that until the i2c_adapter is handed to the i2c core!


> In other words, I want i2c busses to know which 
> devices are connected to them, rather than i2c devices to know which
> busses they are connected to, as your model does.

They know by virtue of the driver model.  Busses don't need to have
device descriptions (i2c_board_info) once the i2c_client exists.

Let me re-emphasize that.  The "DVB example" (above) has no need
to record i2c_board_info before the DVB card gets set up, or after
it's removed.  The DVB driver sets itself up, adds an i2c_adapter,
adds the i2c devices.  When that module is unloaded (including driver
modules both for i2c adapter and for "real" media), all those records
should vanish.  For the board info, the simplest way to ensure that
is never to record them outside of the DVB driver.


> > The solution in patches #4 and #5 addresses at least two kinds of
> > configuration table:  static mainboard tables use register_board_info,
> > and more dynamic ones for plug-in adapters (e.g. DVB/multimedia) use
> > the i2c_new_device() code.  You are overlooking the former case.
> 
> Not really overlooking, I see it's there but I find it impractical.

That word does not seem to mean what you think it means!  It's highly
practical.  It's worked in these I2C patches since before November,
and it uses notions that have worked on numerous systems both before
and after Linux came into existence.


> Let me reformulate my propsal again in greater detail and hopefully
> address your objections.
> 
> Your board-init code will register one or more platform devices for the
> I2C busses. We agree there are no i2c_adapter instantiated yet, just
> enough platform-specific data (I/O ports, IRQs etc...) to do so when
> their driver (say i2c-mpc or i2c-at91) is loaded later.

Fair enough.  This is usually done indirectly, by the way, calling a
utility routine that's shared between all boards using that same chip
and which keeps the device node private (only visible through driver
model calls).

 
> We agree that we also want to declare the I2C devices connected to
> these busses at this point, not later.

Good.


> My approach is that this 
> information is no different from the rest of the platform-specific data
> needed to get the I2C bus running.

I disagree.  There's a fundamental difference between information
used to operate a platform's I2C adapter (platform_data) ... and
information about what devices a given board stack hooks up to
that adapter.

Another way to see the difference is to do a little "roles and
responsibilities" style design analysis.

 - The responsibility of the I2C adapter driver is talking to
   its controller.  Its platform_data might be "use these two
   GPIO lines"; I/O resources like an IRQ and controller
   addresses likely don't go that route.

   It's easy to assign one chunk of platform code the role of
   passing that data to the adapter driver as it creates the
   platform device and configures I/O pins.

 - Creating driver model nodes is the responsibility of the
   I2C core.  (Just as it is for the core of any other driver
   framework.)  So that's the audience of i2c_board_info.

   Several different chunks of platform code may collaborate in
   roles which say what devices exist.  For example, one may
   be on the mainboard, and two different add-on boards may also
   need to declare additional devices for their segments of that
   particular I2C bus.

Adapter drivers have no more business performing device enumeration
than the i2c-core has business acking IRQs.  Different roles,
different responsibilities.


> To me there isn't much difference 
> between the I/O port number that will be used to create an i2c_adapter
> and the list of I2C devices we want to instantiate on this bus. Both
> are static data, expressed as bare numbers and strings with no relation
> with the device driver model, and specific to a given future
> i2c_adapter.

One could as well argue that males and females are the same, since
together they can create future males or females!

They are both necessary; neither is sufficient; both are different;
there's even limited interchangeability.


> When the i2c bus driver is later loaded, it will find the platform
> devices that were created by your board-init code, and will call its
> probe() function on them. At this point, it will read the platform data
> to use the correct resources (I/O ports etc.) before calling
> i2c_add_adapter() to create the I2C busses, right?

Yes, though obviously it's not always a platform_device.


> Likewise, it could 
> read the I2C devices declaration that was also stored as platform data,
> and add it to the i2c_adapter structure before calling
> i2c_add_adapter().

Again, this doesn't accomodate significant real-world scenarios,
like the cpu-card example I gave above ... where several different
components must collaborate to list the devices that exist, and
which will eventually trigger i2c_new_device() calls.

The current i2c_register_board_info() handles such multi-segement
declarations just fine.  I could see passing the stable bus ID
as a parameter there, and removing it from "struct i2c_board_info";
that seems almost like a cleanup, though it does imply extra tables
that wouldn't otherwise be needed.  (E.g. one board might use two
I2C busses; current patches only require one table, that change
would require two.  No big deal.)


> Then all we need to do is let i2c-core read this 
> declaration in (i2c_add_adapter()) and automatically call
> i2c_new_device() for each declared i2c device.

Sounds not dissimilar to what patch #5 does for i2c_register_adapter(),
except that association is not directly coupled to a "struct device".


> Advantages:
> * No need to assume anything about i2c_adapter numbering, so no need to
> introduce a difference between statically and dynamically assigned
> i2c_adapter numbers.

The assumption is being shifted more or less from "platform_device.id
matches bus_num" into "platform_device.dev is the device ID".

(Yeah, that's kind of loose and I'm having to read things into what you
said, but that seems to be your answer to my question about how you're
identifying the adapters.)


> * No need to put all the I2C device declarations in a common pool and
> sort them out afterwards.

There needs to be a pool in any case, because the declarations need to
be provided to by multiple components.   And since this does not belong
in platform data I don't see any issue with having a "common" pool.
If that's an advantage, it's because of two disadvantages...


> * Generic mechanism which can be used for every i2c_adapter, including
> multimedia adapters.

Not an advantage, since the current patchset already handles that just fine.


And disadvantages:

 * Doesn't handle cases like that systems-integrator example I gave,
   since it doesn't allow for declaring bus content segment-by-segment.

 * Places new and unusual requirements on platform_data ... for an issue
   which is not in the least bit platform-specific.

 * Memory management is also unclear, e.g. when does that platform data
   get allocated and freed?  The normal answer is "alloc in board setup
   code, never free", but in that PCI DVB card example that can't work.

 * Design violates basic roles/responsibilities analysis, which means
   that it won't evolve very well.

 
> What do you think? Why couldn't we implement things that way? I would
> like it much more than your current proposal.

See above.

The notion of using a device handle instead of a bus number may have
potential, something like

  int i2c_register_board_info(struct device *dev,
	struct i2c_board_info const *info, unsigned n);

instead of the current signature, moving bus_num out of board_info, but
otherwise still acting like the current code.  (That is, storing a copy
of the data, which is inappropriate for those DVB card scenarios.)

One problem though is those device nodes are normally not available except
through the driver model.  Mainboard init could change to pass board_info
down to the code registering that device, but for add-on cards it looks
like lookup-by-name, maybe with bus_find_device(), would be needed.

For the "DVB card" scenarios, a similar function could be used ... but
it would take a "struct i2c_adapter *" not a "struct device *", and
would not store the data.  That way you get your "loop in i2c-core".

- Dave