[i2c] Identifying I2C busses

[Jean Delvare]

Hi David,

On Tue, 6 Mar 2007 14:20:53 -0800, David Brownell wrote:
> On Tuesday 06 March 2007 6:53 am, Jean Delvare wrote:
> > * You mostly convinced me that your approach was correct. One thing
> > which worries me though: is there any other subsystem in the whole
> > Linux tree which works that way (mixing requested identification
> > numbers with dynamically assigned ones)? What makes I2C different?
> 
> SPI does it that way.  The board_info was essentially a clone of the
> mechanism used there ... although since it's so small, the SPI core
> is always statically linked.  That's not the best model for I2C, where
> the core is several times larger.  (And some folk even objected to
> that restriction for SPI ... not enough to supply a patch though!)

Yeah, now that I look at the spi core, I see the common parts with what
you proposed for i2c. I cannot find an equivalent of
__i2c_first_dynamic_bus_num though.

> At the time I did that SPI stuff I mentioned that I was looking for
> a model that would also work for I2C.  I suspect you didn't take
> that seriously at the time.  ;)

I suspect I didn't really have the time to look into SPI back then. I
was busy enough with I2C and hardware monitoring - and still am.

> I2C is not different in that respect:  neither serial bus supports
> a very useful level of autoconfiguration.  So both need a way to
> declare devices, including both "early" and "late" modes, etc.
> 
> 
> > * I'm very curious about how the stacked-board designs you mentioned
> > don't have problems with I2C address collisions if they work the way
> > you described.
> 
> They're designed to work together -- without collisions.  Stacking
> provides a lot of design, manufacturing and packaging flexibility.
> 
> Many of those CPU card designs seem to be geared, as I implied, to
> folk designing semicustom systems.  The skills involved in making a
> cpu card (6-8 layer PCB design, SMT manufacturing, board testing)
> are not the same as those involved in small runs of a custom I/O box
> to surround such a card.
> 
> (Related:  how do DRAM cards avoid collisions on their I2C EEPROMS?
> I'm guessing three pins in each slot get hooked up to the EEPROM
> address select pins, and they're wired on the baseboard so each slot
> has a unique number.  Similar approaches can work for other kinds
> of board stack, when an EEPROM is used to identify each card and
> maybe record factory-tested calibration data.)

You're right, 24C02 EEPROMs have 3 pins to select the address from 0x50
to 0x57 and each DRAM slot uses a different combination. Something
similar could work for your stack of cards, except that not all I2C
devices have a selectable address. And you may want to have more than
one chip of a kind on a given board. This is way more complex than DRAM
modules which are all the same I2C-bus-wise.

> > On a related topic, I am not fond of your "list of arrays of variable
> > size" approach to store the I2C device declarations. This seems to be
> > more complex than needed. Can't we simply go with a list of individual
> > records?
> 
> That could work.  I don't see much point to that change though...

Simplicity of the data structures.

> > Here's what I have in mind (beware, I changed the struct 
> > names):
> > 
> > struct i2c_device_info {
> > 	char		driver[KOBJ_NAME_LEN];
> > 	char		type[I2C_NAME_SIZE];
> > 	unsigned short	dev_addr;
> > 	void		*platform_data;
> > 	int		irq;
> > };
> > 
> > struct i2c_board_info {
> > 	struct list_head	list;
> > 	short			bus_num;
> > 	struct i2c_device_info	device_info;
> > };
> > 
> > And __i2c_board_list would be a list of struct i2c_board_info. This
> > should make your code somewhat more simple, at the price of additional
> > calls to i2c_register_board_info() and maybe some more memory
> > fragmentation, but I don't think this is a major problem. What do you
> > think?
> 
> I guess I still don't see a real advantage.  It hits the malloc heap more
> often at board init time; a handful of loop instructions move from scanning
> that list into setting it up.  If it bothers you deeply, I can change
> that to provide a list of singletons.

Now that we decided to move bus_num out of struct i2c_device_info, will
it make a big difference fragmentation-wise? I assumed not. I guess
that you'll have one or two I2C devices to declare on each card for
each bus, hardly more, right?

The code that sets the list up is __init anyway, if I followed your
explanations properly, so adding some more instructions there shouldn't
be worrisome.

So, yes, I believe that a list of singletons if prefered. If you really
care about memory fragmentation, then using a list wasn't the right
choice in the first place, and you want to switch to an array.

> > A different approach is one list per bus number, that would scale
> > better (no common pool), but that makes things a bit more complex
> > again, and as you underlined, we don't expect thousands of devices to
> > be declared anyway so scalability probably doesn't matter. I guess
> > real-world scenarii have a max of 2 I2C busses?
> 
> I've seen chips with three:  two lowspeed ones, and a highspeed one.
> Regardless of how they're factored, I personally have never happened
> across a production board with even a dozen I2C devices.

OK, not numbers were complexity matters.

> > * I am not happy with the idea of having one function named
> > i2c_add_adapter, and one named i2c_register_adapter (i.e. almost the
> > same name) doing something similar but different, without this
> > difference being visible in their name. We need to come up with
> > something better than that. Either we can have a single function, or we
> > need to rename i2c_register_adapter to something which expresses its
> > difference to i2c_add_adapter. i2c_add_numbered_adapter or something.
> 
> Your notion of using the device handle as the (relatively) stable
> bus ID would let there be only one function...

It's up to you, I'm not insisting. I _am_ fine with two functions, as
long as the name of the new one is meaningful.

> > * With regards to i2c-mainboard, wouldn't it be more simple to just
> > make it mandatory to build i2c-core into the kernel when you need the
> > early device declaration process? I seem to remember that this is a
> > common practice in the embedded world anyway, because you need I2C
> > early in the boot process.
> 
> Simpler ... yes.  It's common regardless of whether the board needs I2C
> early:  modular drivers cost more space than non-modular ones, and in any
> case one doesn't want the driver for a key system bus to be modular.
> But common doesn't mean universally desirable.
> 
> Also, I'm not sure that'd be a reasonable restriction.  It'd be no skin
> off my back, for sure, but I know some folk would object.  And how would
> you force static linkage of the i2c-core module in those cases?  It'd be
> tricky to guarantee "make allmodconfig" works on all the relevant boards...

Is there no Kconfig syntax to handle that case? Too bad :(

> So despite the minor mess, I don't see an easy way around that being
> a separate file.  Moving it out of i2c-core.c was a cleanup.  :)

This forces you to export some new symbols, and exports aren't free. I
wanted to avoid that. But if you see no other solution, so be it.

> > * Your implementation doesn't seem to support I2C bus number
> > reservation if there is no chip on that bus. Isn't it going to be
> > confusing? Don't we want a function in i2c-core that lets the platform
> > code explicitly bump __i2c_first_dynamic_bus_num, rather that deducing
> > it from the i2c devices definitions?
> 
> Good point, but a simpler fix is just to let i2c_register_boardinfo()
> do that bumping even when there are no devices to register.  That falls
> out easily from making the bus number be a parameter to that function
> rather than a member of the i2c_board_info structure.

I didn't expect people to call i2c_register_boardinfo() if there is no
device to register. But you can argue that they now have to and it's
their bug if they don't. So again, up to you.

Thanks,
-- 
Jean Delvare