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

[Jean Delvare]

Hi David,

On Sat, 3 Mar 2007 20:34:21 -0800, David Brownell wrote:
> On Saturday 03 March 2007 1:37 pm, Jean Delvare wrote:
> 
> > > @@ -163,6 +163,7 @@ struct i2c_client {
> > >  	int usage_count;		/* How many accesses currently  */
> > >  					/* to the client		*/
> > >  	struct device dev;		/* the device structure		*/
> > > +	int irq;			/* irq issued by device (or -1) */
> > >  	struct list_head list;
> > >  	char name[I2C_NAME_SIZE];
> > >  	struct completion released;
> > 
> > What makes the irq more important than any other device-specific
> > attribute? Given that the i2c core itself doesn't make any use of the
> > irq, shouldn't it be stored in the specific client data structure?
> >
> > I am asking because in my experience most I2C devices don't use an IRQ,
> 
> It's fairly common on the systems I've seen.  Lots of RTCs will provide
> an alarm IRQ; I2C GPIO expanders tend to do so (pin change irq); USB-OTG
> tranceivers do; power management chips do; various other multi-function
> chips do ...
> 
> Note that you seem to be arguing that chip type information (which not all
> I2C drivers need) should be part of the client struct, but not the IRQ
> (which likewise not all drivers need).  That's inconsistent.

I was not arguing that much. I was raising questions precisely because
myself am not sure what belongs where. I guess it's rather natural that
you see the attributes you use frequently as core attributes, and that
I see the attributes I use frequently as core attributes ;)

> > so I am reluctant to increase the structure size for something which
> > only some devices will use.
> 
> To the extent the space is an issue, remember that eliminating fields
> that the driver model handles lets us reclaim four pointers (adapter,
> driver, list_head) and one integer (usage_count) plus eventually also
> that "struct completion"; and the "flags" could as well be "short", to
> reclaim some pad bytes ... the i2c_client structure is hardly svelte.
> (Especially with *50* bytes allocated to a chip type name, which is
> rarely even 20 characters long ...)

Feel free to submit a patch making i2c_client.name smaller if you think
we don't need that much space. I tend to agree. At the moment, we have
the same max size define for adapter names and chip names, and adapter
names can be quite long, thus the 50.

Turning the flags to a short doesn't seem to spare enough in practice
to even care.

I do agree that the size is not a valid decision factor for the irq.
(See how I become more reasonable after a good night.)

> > > +struct i2c_board_info {
> > > +	char		driver[KOBJ_NAME_LEN];
> > 
> > I would add another string for the device type, but I guess you'll
> > argue that it belongs to the platform_data structure?
> 
> This is a tradeoff.  It _could_ go in platform_data, but see the other
> email where this was discussed; I pointed out that one thing you want
> (easy userspace creation of device nodes that include type annotation)
> would be easier if a device type string in i2c_board_info would set up
> the i2c_client.name field.

True, I think the code will be more simple if the chip type string is a
"core" attribute, even though some devices won't need it. More on that
below.

Is it OK to only store a pointer to the actual string in struct
i2c_board_info, or do we have to allocate space as you did for the
driver name? I sure don't want to waste 50 bytes to store it, but OTOH
there must be a reason why you used a real array for the driver name.
I guess you expect the original string to be __initdata and gone by the
time we need it?

If we have to store the string itself, then we definitely want to
execute your "make i2c_client.name shorter" plan first.

> > > +	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. 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. Just enable
CONFIG_PCI_MULTITHREAD_PROBE and watch everything break.

The only reason why we did not yet have to fix it, is that non-SMBus
hardware monitoring chips are becoming more popular, so there isn't too
much pressure. But once CONFIG_PCI_MULTITHREAD_PROBE is enabled in the
Linux distibutions, I expect the problem to become more visible and
we'll have to address it. No idea how, though.

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

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

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

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

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.

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

Here, done ;)

> > > +	unsigned short	dev_addr;
> > > +	void		*platform_data;
> > > +	int		irq;
> > 
> > I believe the irq should be just one member of the platform_data
> > structure for devices which need that.
> 
> But you believe chip type should not be in platform_data?  I'd say
> both or neither.  "Neither" seems simplest... that is, board_info
> records both the irq and the chip type.

Agreed :)

> > > +#define I2C_BOARD_INFO(driver_name,busnum,devaddr) \
> > > +	.driver = driver_name, .bus_num = busnum, .dev_addr = devaddr
> > 
> > This macro doesn't look terribly useful. It's not like you'll have
> > dozens of chips to declare on each bus, and it doesn't save that much.
> 
> What it does is group all the mandatory attributes together, so
> when arch/X/mach-Y/board-Z.c type files declare the devices it's
> harder to provide broken definitions.
> 
> FWIW the macro came because of Greg's feedback from the sample
> conversions I posted previously ... it's a lot easier this way
> to see what's going on.

OK, sold.

> > > @@ -0,0 +1,13 @@
> > > +struct boardinfo {
> > > +	struct list_head	list;
> > > +	unsigned		n_board_info;
> > > +	struct i2c_board_info	board_info[0];
> > > +};
> > 
> > I don't like having an i2c-specific structure with a name as generic as
> > "boardinfo".
> 
> It's internal to the I2C stack, no other code has any business seeing it.
> But the name doesn't much matter; feel free to suggest a better one.

But this name could collide with an external one someday. Maybe
i2c_board_info_list would do. It's a detail anyway, first we need to
agree on the core design choices.

> > > +i2c_register_board_info(struct i2c_board_info const *info, unsigned len)
> > > +{
> > > +	struct boardinfo	*bi;
> > 
> > No two-letter variables please, they are a nightmare to grep for.
> 
> Easily changed; done.  But it's not like one needs to grep for that,
> a function's local variables aren't going to escape!  They'll stay
> neatly contained on that part of the screen.

That's the theory. In practice I was very happy that people have been
naming their i2c adapters "adap" or "adapter" in most drivers, rather
than just "a" or "ad", when we were working on the dev/class_dev removal
patches. It made it possible to catch all drivers which needed to be
updated. So I believe that in general, we should be friendly with
people who will be grepping our source code in the future for reasons
we can't even think of today.

> > > +	int			i;
> > > +
> > > +	if (len == 0)
> > > +		return 0;
> > > +
> > > +	bi = kmalloc(sizeof(*bi) + len * sizeof *info, GFP_KERNEL);
> > 
> > I don't like this kind of construct at all, because they are very
> > fragile. You assume that alignment will be OK, which might be true
> > right now, but might no longer be if the structures are modified at
> > some later point.
> 
> That's a pretty standard idiom ... it seems that using the C99 "flexible
> array member" syntax (omit length of that array at the end) should be
> an always-portable solution, so I'll switch to that.  The "length 0"
> syntax is the traditional GCC solution. 

But is the C99-style accepted? Are there other use cases of either
syntax in the kernel at the moment, so we can just do the same?

> > And this is very architecture-dependent too. 
> 
> How?

Sorry, my bad, it isn't. It was a bit late when I replied on Saturday
evening, and I missed the fact that the padding, if needed, would be
added automatically thanks to the empty board_info member of struct
boardinfo. I'm not used to these constructs.

> > > @@ -215,6 +263,7 @@ static void i2c_unregister_device(struct
> > >  		put_device(&client->dev);
> > >  
> > >  }
> > > +EXPORT_SYMBOL_GPL(i2c_unregister_device);
> > 
> > All the other exports are at the end of i2c-core.c, so for the sake of
> > consistency please do the same for the new ones.
> 
> The current style is to keep exports adjacent to the function.  That's
> the way new code should be written... arguably, those current exports
> should be moved.

I'm fine with moving the exports right after the functions, but you
know the rule: I want a separate patch doing only that. In theory such a
patch should come first, but I am fine with a patch going after your
current patchset if it makes things easier for you. I'll be waiting for
it before I push the whole patchset to -mm, though. So the path is up
to you, but the end result has to be consistent.

> > > +static void i2c_scan_static_board_info(struct i2c_adapter *adap)
> > > +{
> > > +	struct boardinfo	*bi;
> > > +
> > > +	mutex_lock(&__i2c_board_lock);
> > > +	list_for_each_entry(bi, &__i2c_board_list, list) {
> > > +		struct i2c_board_info	*chip = bi->board_info;
> > > +		unsigned		n;
> > > +
> > > +		for (n = bi->n_board_info; n > 0; n--, chip++) {
> > > +			if (chip->bus_num != adap->nr)
> > > +				continue;
> > > +			(void) i2c_new_device(adap, chip);
> > > +		}
> > > +	}
> > > +	mutex_unlock(&__i2c_board_lock);
> > > +}
> > 
> > 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.
> 
>  - Droup 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 ;)

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

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

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.

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

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

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

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

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

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

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.

We agree that we also want to declare the I2C devices connected to
these busses at this point, not later. My approach is that this
information is no different from the rest of the platform-specific data
needed to get the I2C bus running. 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.

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? 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(). 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.

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.
* No need to put all the I2C device declarations in a common pool and
sort them out afterwards.
* Generic mechanism which can be used for every i2c_adapter, including
multimedia adapters.

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

Thanks,
-- 
Jean Delvare