[i2c] linux-i2c and smbalert

[Hendrik Sattler]

Am Montag 28 August 2006 20:15 schrieb Jean Delvare:
> > > 1* As you noticed already, most SMBus bus drivers used in regular PCs
> > > are poll-based rather than interrupt-driven (i2c-i801, i2c-viapro,
> > > i2c-nforce2, i2c-amd756...) That being said, the interrupt you would
> > > get from an SMBus alert would be of a different nature than the
> > > interrupt the driver would get on transaction completion. In theory
> > > they could even be different interrupt lines as I understand it. But
> > > converting the drivers to be interrupt-driven would be very welcome
> > > anyway, and there might be common code.
> >
> > Still, polling a memory-mapped register is far cheaper that polling via
> > i2c commands. Note that for i801, this can be done via polling the
> > HST_STA bit 5.
>
> Yes, you are completely right. In fact it never occured to me that we
> could poll the ARA, it would be too ugly ;)

ugly <-> easy
Many things have trade-offs.

> This register is named "HST_STS" in the datasheets I have here.

Well, in mine (downloaded just recently from the link in my first post, date 
says June 1999) it is called HST_STA (chapter 11.2.1).

> The bit 
> in question seems to only exist starting with the ICH2 (82801BA). I
> guess the earlier models don't support SMBus Alert.

That's the model that I have.

> > That also means that finding the right i2c_client struct can be fully
> > internal.
>
> Internal to what?

Internal to i2c-core.

> > The patch for i2c_i801 will follow based on this one. It is a bit tricky
> > as the polled read/write from the register from a timer should be secured
> > by a mutex to not interfere with the main transaction function.
>
> True. Each i2c_adapter has a mutex already (bus_lock), which we use to
> serialize the transactions on the bus. Maybe you can use it for that
> purpose too?

Would be a big lock though, so better have a second one for the read/write 
action of that register.

> > +unsigned short i2c_smbus_read_ara(struct i2c_adapter *adapter)
> > +{
> > +	struct i2c_client ara = {
> > +	  .addr = I2C_SMBUS_ADDR_ARA,
> > +	  .adapter = adapter,
> > +	  .flags = 0,
> > +	};
>
> Use tabs for indentation, not spaces.

Any standard configuration for emacs?

> > +
> > +	/* TODO: does not work with 10bit addresses
> > +	 * SMBus-1.0:
> > +	 *   In case of 10bit addresses, two bytes are returned:
> > +	 *     1111 0HH0 LLLL LLLL (H mark the high bits, L the low bits)
> > +	 *   In case of 7bit addresses, one byte is returned:
> > +	 *     AAAA AAAX (A is the 7bit address, X is 0 or 1).
> > +	 */
> > +	s32 addr = i2c_smbus_read_byte(&ara);
> > +	if (addr != -1)
>
> You should test for < 0 rather than explicit -1.

Ok.

> > +	        return ((addr >> 1) & 0x3F);
>
> Broken mask, should be 0x7F...

Ups, should have been 0x3FF for 10bits. But since that's not supported, 
anyway, 0x7F is fine.

> but you don't actually need any masking 
> than I can see, as you aleady have a 7-bit value at this point.

Good point.

> > +	else
> > +	        return 0;
> > +}
>
> Returning 0 on error, interesting convention ;)

Not really:
 addr = i2c_smbus_read_ara(me);
 if (addr) i2c_smbus_alert_notify(me,addr);
But...

> Better make the return type an int and let the error go through.

...this may be more like other places in kernel.

> One thing which may need to be discussed is that the ARA could have
> been requested by a driver already. We should either make sure this
> address can never be requested, or test that it is still free before
> calling i2c_smbus_read_byte.c

Well, this would restrict a SMBus over an I²C bus. There may be I²C chips out 
there that use this address. OTOH, it is an illegal address for a chip 
according to SMBus. However, in I²C 2.1 it is allowed.
So either restrict I²C or omit small parts SMBus specs. As devices from both 
can be on the same bus, I'd for restricting I²C.
And while we are at it, the following addresses should not be allowed to 
register:
 <= 0x07 (0x04-0x07 are for HS-mode(I²C) or reserved(SMBus))
 >= 0x70 (10bit or reserverd (I²C and SMBus))
    0x08 (Host(SMBus))
    0x0C (alert response address(SMBus))
    0x61 (device default address(SMBus)), are there known ARP devices?

Probably optional:
    0x28 (ACCESS bus host(SMBus))
    0x37 (ACCESS bus default address(SMBus))

So it's more than just the ARA to think about.

> Please rework your patch, then add support for at least one i2c bus
> driver and one i2c chip driver, so that we can see what it looks like.
> And please test it!

Yes, may take a while, though. When done, I'll resend it as a patch series.
I'll just attach the patch, revised according to your comments. Just in case, 
someone else is also interested...

HS

PS: anyone got I²C HighSpeed mode as i2-algo-* or as other solution?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smbalert.patch
Type: text/x-diff
Size: 2939 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/i2c/attachments/20060828/79c5789e/attachment.bin