[i2c] linux-i2c and smbalert

[Hendrik Sattler]

Hi,

Am Dienstag 19 September 2006 21:59 schrieb Jean Delvare:
> > 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?

The benefit is that the ARA cannot be take by an i2c_client if the host 
decided to support smbus alerts, even before actually doing and read on that 
address. Additionally, I think that it is a cleaner approach, especially when 
doing polling.

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

I'll do that. However, currently I can only use the polling-only mode that you 
find inacceptable. I'll do it differently in the driver if that will not go 
in, then (just as you said, polling with the client driver).

> > * 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?

Well, the chip answers on the ARA, so it somewhat is supported. When using 
pre-build systems without wiring documentation, it is difficult to say that 
it is not supported. Maybe it needs a setting that I did not find, yet.
But yes, the smbus alert hardware pin does not get asserted (according to that 
driver implementation, cannot test this in hardware).

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

One, or two or three. At what point does it get more sufficient to poll the 
ARA instead?
But thinking about that more and more, smbus alert isn't a well-designed thing 
(the "one missing driver can mess it all up" thing).

> > 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 attached it. It should show the way an i2c_client can use it.
Original fjkey stuff is from apanel.sf.net.
I only have (well, kind of) chip documentation for the OZ99x, but not for the 
OZ007S. And no, I didn't figure out all the wiring details of this system, 
yet.
Pressing the button down will make the OZ007S (on 0x57) respond to the ARA. 
However, no bit in its registers changes (maybe I have to turn that on but 
it's kind of impossible without documentation).
However, the buttons are _also_ connected to the OZ992S and at least the 
button released event is latched there.
FSC kind of messed it up, though :-(

> * 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?

A count for i2c_clients with smbus_alert function that registered with the 
i2c_adapter. It is used to decide when the smbus_alert function of 
i2c_adapter gets called to enable/disable ARA handling (no need to handle 
that if no driver supports it).

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

Ok.

> * 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?

That is the incomplete answer to my question about that. It is needed to do 
something outside the interrupt context if that action may sleep (like 
actually using the ARA).
There are two major options:
* use a preselected workqueue_struct somewhere:
  - global one, or
  - one for all of i2c, or
  - one per i2c_adapter (like now), or
  - one for i2c_smbus_alert
* let the i2c_adapter define the workqueue_struct to use (additional parameter 
for a function, only)

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

Many drivers have an integrated changelog (without too many details) but if 
you don't like that, I'll drop it (I don't really care).
External history logs are often lost when changing source control system (not 
that that ever happens ;)) and are not shipped with the kernel. However, they 
are probably easier to handle from a git users POV.

HS
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fjkey.c
Type: text/x-csrc
Size: 2993 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/i2c/attachments/20060919/49d02aef/attachment-0004.bin 
-------------- next part --------------
obj-m	+= fsc_btns.o

fsc_btns-objs := fjkey.o oz.o
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oz.c
Type: text/x-csrc
Size: 11315 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/i2c/attachments/20060919/49d02aef/attachment-0005.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Makefile
Type: text/x-makefile
Size: 195 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/i2c/attachments/20060919/49d02aef/attachment-0006.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fjkey.h
Type: text/x-objchdr
Size: 316 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/i2c/attachments/20060919/49d02aef/attachment-0007.bin