[i2c] [patch/rfc 2.6.19-rc6 1/3] I2C "new style" driver model type binding

Jean Delvare khali at linux-fr.org
Sun Nov 19 16:38:48 CET 2006


On Fri, 17 Nov 2006 13:31:43 -0800, David Brownell wrote:
> On Friday 17 November 2006 7:37 am, Jean Delvare wrote:
> 
> > >  - Update i2c_device_match() to handle new style driver binding by comparing
> > >    client and driver names (like platform and SPI busses).
> > 
> > This won't work in the general case, as i2c chip drivers can support up
> > to 8 different chip types.
> 
> Where did "8" come from?  My at24c eeprom driver handles 13 in its
> current incarnation, and I know there are others it could handle.

Because of the I2C_CLIENT_INSMOD mess, we needed one different macro for
each number of devices we wanted to support in a driver, and at 8 we
felt it was enough. It's an arbitrary number, I didn't mention it here
as being meaningful. And I misexpressed myself, it's not actually the
number of devices a driver can support, but rather the number of
devices the driver can handle differently. My point is simply that you
can't rely on driver.name == device.name in i2c_device_match(), it is
too restrictive for our needs.

> Chip type information is best suited to the platform_data field, in
> the common case where hardware probes are insufficient (or unsafe).

Granted, but in the case where hardware probes are sufficient, no
platform data will be provided. We just can't provide platform data
information for the 2000+ PC motherboards out there.

I don't know yet how this will be handled when we migrate such code to
the driver model-compliant code you're adding now, but one possibility
would be to have a dedicated kernel module probing the motherboard
SMBus, identifying the supported devices, and instanciating the
devices. Then the required drivers could be automatically loaded.
Another way would be to let the drivers probe and instanciate their own
device, old-style, but then we would not be able to load the needed
drivers automatically.

> > 	So we need to either list all supported 
> > types in the driver and lookup that list in i2c_device_match(), or
> > instanciate the i2c chips by {driver name, type number} pair rather
> > than chip name. The former sounds more flexible. And isn't exactly what
> > device_id tables are meant for?
> 
> The device id tables are for use with _managed_ device identifiers.
> 
> Like the PCI SIG assigns vendor codes, then vendors assign product
> codes; the USB-IF does the same for USB; Microsoft does for PNP;
> and so forth.  Those identifiers get associated with Linux drivers
> through those device id tables.
> 
> I2C doesn't have such a managaged ID namespace.  Which is why the
> simple solution seemed to be to reuse the approach Linux uses in
> similar cases.  It's not necessarily the _best_ approach, but it's
> one that works well and has precedents in Linux.  Plus it needs
> no design effort, and no special education/training.

I'm fine with using simple strings to identify I2C device types, but
then each i2c driver must provide a list of supported device types (as a
list of strings) and device.name must be matched against this list,
rather than driver.name.

> (Also, adding new device id table types gets really messy, fast,
> since it requires modutils updates.  That complicates deployment
> by quite a lot.)

But how do you automatically load the right drivers without these 
tables?

> > >  - Add device modalias attribute, for coldplug support; switch over to use
> > >    attribute groups ("name" and "modalias", contents are the same).
> > 
> > Why duplicate it if it's the same?
> 
> The convention is to call it "modalias", so that for example coldplug
> scripts (like the ones I use) can just scan /sys/devices after userspace
> is running and just "modprobe $(cat modalias)" for driverless devices,
> without needing to add bus-specific logic.

I see, it's a nice feature. Should we then (much) later delete the
"name" sysfs file?

> > >  - Add driver probe() method.  Update i2c_device_probe() to use it with
> > >    new style drivers.
> > >    
> > >  - Add driver remove() method.  Update i2c_device_remove() to use it with
> > >    new style drivers.  Call that from i2c_del_adapter() as needed.  Also
> > >    update i2c_del_adapter() to handle the new case of cleaning up I2C
> > >    devices without (new style) drivers.
> > > 
> > >  - Rename i2c_bus_{suspend,resume}() to reflect their per-device nature.
> > > 
> > >  - Update Documentation/i2c accordingly
> > 
> > Note that once again, these are quite a few changes for a single patch.
> > I'd appreciate smaller patches, they are easier to review and test.
> 
> I'll put the suspend/resume rename with the first "simpler" set of updates,
> and can split the doc updates out ... as for the rest, only the remove() is
> at all separable.  My usual rule is that patches should stand on their own;
> and probe() without remove(), or anything without its doc updates, doesn't
> IMO stand on its own.  But I'll split them out a bit more; it can also be
> OK to have a series of coupled patches.

Granted, splitting related things doesn't help. I just meant that 20+
kB patches aren't easy to review and test, and making them smaller
where possible will make it easier for me to review and merge your work.

> > > --- o26.orig/drivers/i2c/i2c-core.c	2006-11-16 09:44:40.000000000 -0800
> > > +++ o26/drivers/i2c/i2c-core.c	2006-11-16 11:04:35.000000000 -0800
> > > @@ -44,13 +44,46 @@ static DEFINE_IDR(i2c_adapter_idr);
> > >  
> > >  static int i2c_device_match(struct device *dev, struct device_driver *drv)
> > >  {
> > > -	/* (for now) bypass driver model probing entirely; drivers
> > > -	 * scan each i2c adapter/bus themselves.
> > > +	struct i2c_client	*client = to_i2c_client(dev);
> > > +	struct i2c_driver	*driver = to_i2c_driver(drv);
> > 
> > BTW, did I mention how much I dislike this coding style?
> 
> Which coding style is that?

Aligning variables names with tabs.

> > > -/* 
> > > - * We can't use the DEVICE_ATTR() macro here as we want the same filename for a
> > > - * different type of a device.  So beware if the DEVICE_ATTR() macro ever
> > > +/*
> > > + * We can't use the DEVICE_ATTR() macro here; we used the same name for
> > > + * an i2c adapter attribute (above).  So if the DEVICE_ATTR() macro ever
> > >   * changes, this definition will also have to change.
> > >   */
> > >  static struct device_attribute dev_attr_client_name = {
> > 
> > This change doesn't belong to this patch. BTW, it looks to me like this
> > issue could be solved by using the __ATTR macro, then the comment would
> > no longer be needed.
> 
> OK by me ... I'll expect you to merge that cleanup.  I found that
> particular comment confusing-to-wrong.  :)

Yes, it's merged already.

> > > +static const struct attribute *i2c_dev_attrs[] = {
> > > +	&dev_attr_client_name.attr,
> > > +	&dev_attr_modalias.attr,
> > > +	NULL,
> > > +};
> > > +
> > > +static struct attribute_group i2c_dev_attr_group = {
> > > +	.attrs = (struct attribute **)i2c_dev_attrs,
> > 
> > Don't declare the struct attribute array const, and you won't have to
> > cast. The struct attribute_group itself, on the other hand, can be
> > declared const.
> 
> Trying to save initialized ".data" space here, on general principles
> (to make XIP work better, etc) ... BOTH should ideally be const/readonly.
> But you're right that it's simpler not to fight that battle here.

Indeed. I know for a fact that Greg would take a patch cleaning it all
up, but that's not the matter here.

> > > -	/* (maybe) tell drivers about this removal */
> > > +	/* (maybe) tell legacy drivers about this removal */
> > 
> > Why "maybe", BTW?
> 
> Because they don't all have the method that's called.

Ah. Maybe you should reword it to "if they care" or something similar.
This "maybe" is a but confusing, it sounds more like the author isn't
sure if this piece of code it legitimate or not.

> (And in fact it should be superfluous ... if a per-adapter
> method is needed, the per-device one isn't, and vice versa.)

Both proved to be useful, the media/video drivers in particular need
this. But I bet the need will disappear with the new model.

> "i2c master" vs "i2c slave".  Most of the I2C controllers I see lately
> have support for both modes.  Linux doesn't.  The docs say "SMBus host"
> in several places already ...

Ah, yes. SMBus uses the term "host" where I2C uses "master". Nevermind
then.

> > > +which provides an inexpensive (two pins!) bus for connecting many types of
> > > +devices with infrequent or low bandwidth communications requirements.  It
> > > +is widely used with embedded systems.
> > > +
> > > +SMBus (System Management Bus) is based on the I2C protocol, and is mostly
> > > +a subset of I2C protocols and signaling.  Many I2C devices will work on an
> > > +SMBUS, but some SMBUS protocols are I2C-incompatible.  Modern PC mainboards
> > 
> > Oh, really? Which ones?
> 
> Other than SMBUS_QUICK which you highlighted?  ARP, according to one
> doc describing parts of SMBUS that a particular I2C controller could
> not implement.

An I2C controller could implement any part of the SMBus specification
if they wish. As a matter of fact, the software implementation in
Linux, i2c-algo-bit, implements PEC, which is an SMBus extension as
well. And on the other hand, an SMBus "host" driver may not implement
all the SMBus features. So there isn't that much practical difference
between SMBus and I2C masters. SMBus adds names to particular
transactions, which were perfectly valid under I2C, but were just
random transactions amongst a variety of other ones.

> > I don't see anything in the I2C specification which would make the
> > SMBus Quick command invalid. It's a standard I2C transaction. What
> > makes you think otherwise?
> 
> That's what you said a while back ... ISTR one point being that even
> if the I2C transaction were defined, the behavior of chips faced with
> such messages is _not_ defined except for SMBus-compliant chips.

This is mostly the idea, yes, but here too there's some difference
between theory and practice.

For example, the theory is that the SMBus Quick command writes one bit
of data to the target SMBus chip. The practice is that no chip was ever
found to do anything upon receiving such a command (which is why we use
it for probing purposes). And most I2C chips behave the same way (i.e.
they ack but don't do anything useful otherwise.)

Another example: the theory is that the SMBus Read command reads a
value from a given register of a given SMBus chip. The practice is
that, in most cases, it also changes the internal address pointer of
the chip as a side effect, in a way which may differ depending on the
chip and register.

So, SMBus adds some semantics on top of I2C, but there's still some
margin for chips to behave differently. And many I2C chips use the
SMBus semantics too. So there again, the boundary between I2C chips and
SMBus chips is a fuzzy one.

-- 
Jean Delvare



More information about the i2c mailing list