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

[David Brownell]

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.


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

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


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

This works fine for SOC designs, which are the primary customer of
the i2c_register_board_info() calls.


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


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

Since this uses the *current* bus identifier abstraction, I'm quite
surprised you have an issue with it...


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


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


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


> > +/* board_lock protects board_list and first_dynamic_bus_num.
> > + * nobody except i2c-core is allowed to use these symbols.
> > + */
> > +extern struct mutex	__i2c_board_lock;
> > +extern struct list_head	__i2c_board_list;
> > +extern int		__i2c_first_dynamic_bus_num;
> 
> If really nobody except i2c-core was allowed to use these symbols then
> you wouldn't have to export them. i2c-mainboard too is allowed to use
> them, obviously.

Read that as "the i2c core", not "the i2c-core.c file"; that i2c-mainboard
code is conceptually part of the core, although it needs to be statically
linked in order for the board-specific arch_initcall() code to call the
i2c_register_board_info() function to set up that state.



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


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


> And this is very architecture-dependent too. 

How?


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


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

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

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


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


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

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.


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

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

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


> There must be some reason why you came up with something much more
> complex than that,

See patch #5/5 and the previously posted examples of AT91, OMAP, and
OpenFirmware board updates to get more of the picture.

The sound-byte answer is that we need to declare i2c devices before
the I2C stack, or any I2C adapter driver, is available.


> while "my" proposal (actually I believe the original 
> proposal by Nathan Lutchansky was pretty much the same) only needs to
> introduce one new structure (i2c_board_info) and one new function
> (i2c_new_device),

ISTR none of the previous proposals used either of those names; or
adopted the driver model enough to let that comparison really work;
or accomodated tables of I2C devices that were provided as part of
system definition (thus keeping them out of drivers).


> and will work with multimedia adapters too. I guess 
> I'm missing something, but I can't see what. Care to enlighten me?

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.

- Dave



> Thanks,
> -- 
> Jean Delvare
>