[i2c] linux-i2c and smbalert

Jean Delvare khali at linux-fr.org
Tue Sep 19 21:59:04 CEST 2006


Hi Hendrik,

> I reqrote the most part of it. i2c-smbalert now registers as i2c_driver and 
> has to be manually attached to the bus by the i2c_adapter driver that want to 
> support it.

What is the benefit of doing this over your original, more simple
approach?

> I also attached the implementation for i2c-i801 (also contains some other 
> cleanups) that runs fine on my system (Intel Corporation 82801BA/BAM SMBus)

Please submit cleanups as a seperate patch. But first please make sure
to base your patch on the latest version of the i2c-i801 driver - I made
lots of cleanups to this driver in 2.6.18-rc1. It appears to me that
most of (all?) the cleanups in your patch are already done upstream.

> Problems:
> * If reading the ARA contains an I2C address that is not registered by a 
> driver with an smbus_alert function, an asserted SMBALERT# might never get 
> deasserted. If it has the highest address, the ARA will always contain that 
> address. This is a problem of the system (SMBALERTs should only be issued 
> after explicit configuration), not resolvable.

I seem to remember that the _lowest_ address wins the arbitration. Not
that it changes anything, I agree there's not much we can do, except
printing a big fat warning message in the logs.

> * Some host adapters have an unconnected SMBALERT pin (e.g. my system). I 
> solved this for i2c-i801 with a Kconfig option that only enables conditional 
> poll based on that choice. This could also be a module option, instead.

You mean that the only system where someone (you) wants SMBus Alert
support for, doesn't actually support it?

Indeed a module option - or even better, a sysfs file, would be
magnitudes better than a Kconfig option. Kconfig choices are a pain for
distributions and their users - no single option fits every need.

But the truth is that I don't want to see poll-based SMBus Alert
support in the kernel, ever. It simply makes no sense. The only benefit
of SMBus Alert is that it is interrupt driven. If you are going to poll
the ARA, you might as well poll the I2C chip directly.

> How can I make a loose dependency with Kconfig: i2c_i801 must not be compiled 
> built-in if i2c_smbalert is configured as module (linking error). However, it 
> also shall not depend on it (after all, it's optional).
> 
> Comments (especially on better variable and function names) are welcome.
> 
> I also have an i2c_client driver but this is only useful for a Lifebook C-6637 
> notebook.

We still want to see it.

I don't have much time for a complete review of your code at the
moment, so just some general and random comments:

* You need to provide detailed explanations of what your
implementation. Your original implementation was simple enough that I
could (mostly) get it just by reading it once. This new implementation
is much more complex and tricky. What is adapter->smbus_alert_count
for? What are the 6 new functions declared in <linux/i2c.h> for? Etc.
etc. I have very little time for code review these days, so you really
need to make it easy for me.

* Documentation/CodingStyle is real, please clean up your code.

* Please use the -p option when generating patches, so that we can
quickly see to which functions and structures you are adding code.

* I don't think we want a workqueue pointer in struct i2c_adapter. Not
all adapters will need it, and the i2c-core doesn't use it. I would let
each driver implement its workqueue as its author sees fit. Unless it
creates a problem I missed?

* Please do not abbreviate SMBus into "smb". This is confusing, as
"smb" can mean a dozen different things (Session Message Block [Samba]
and Systems Management Board, amnonst others.)

Additionally, the drivers those name begin with "i2c-" are typically
I2C/SMBus bus drivers. For chip drivers we don't use the "i2c-" prefix.
So your i2c-smbalert driver should rather be named smbusalert (or
smbus-alert.)

> +void i2c_smbus_alert_notify (struct i2c_adapter *adapter, unsigned short addr)
> +{
> +	struct i2c_client  *client = i2c_smbus_alert_find_client(adapter,addr);
> +
> +	if (!client)
> +		return;
> +	if (i2c_use_client(client))
> +		return;
> +	if (client->driver->smbus_alert)
> +		client->driver->smbus_alert(client);
> +	i2c_release_client(client);
> +}

This is racy. i2c_smbus_alert_find_client() needs to call i2c_use_client()
by itself while it holds the lock on the client list.

> +/* Sept. 2006 (Hendrik Sattler):
> + * - added SMBALERT handling
> + * - replaced in-code SMBHSTSTS values with bit defines
> + */

Changelog belongs to the patch header comment, and from there it'll go
to the source control system logs. It does not belong to the source
code.

-- 
Jean Delvare



More information about the i2c mailing list