[i2c] linux-i2c and smbalert

[Jean Delvare]

Hi Hendrik,

> > 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 ;)

This register is named "HST_STS" in the datasheets I have here. The bit
in question seems to only exist starting with the ICH2 (82801BA). I
guess the earlier models don't support SMBus Alert.

> I had hope that it would be the natural goal of drivers to be interrupt 
> driven. After all, polling is the worst case for a driver, isn't it?

It certainly is a natural goal, but we seem to be out of natural
resources :( All these drivers were originally written using polling,
for simplicity I guess, and now that they work well enough and are
widely used, there seems to be little interest in converting them. If
we do convert them, we'll need a very good testing so as to make sure
we did not break anything.

> > 2* As I understand it, ARA support would require some cooperation from
> > the i2c core. Once your receive an ARA's interrupt in your bus driver
> > you can easily get the address of the chip which sent it, but from
> > there, you have currently no way to callback the driver for that chip,
> > if any.
> 
> Well, receiving the address should be done by i2c/smbus core to reduce code 
> duplication. The bus driver only has to call that function. If a controller 
> does this internally, it simply does not call that function but only the 
> notifier.

Yes, good point.

> That also means that finding the right i2c_client struct can be fully 
> internal.

Internal to what?

> > You would need to add a callback function in struct i2c_driver, I guess.
> > It's probably not too difficult but we need to agree on what exactly is
> > needed, so that we get it right at once.
> 
> calling it
>  void (*smbus_alert) (struct i2c_client*)
> seems the most natural thing. It would make clear that it is for smbus devices 
> only.

Yes, looks OK to me.

> I attached a simple patch that should work (well, it compiles). I did not 
> implemented it for any bus or chip driver, thus it is untested. But maybe you 
> can take a look at it. 10bit adress reading is not supported (only the 1.0 
> spec tells about it), not sure how to read 2 or 1 byte from smbus (block 
> read?).

I wouldn't bother with 10-bit addressing. I've never seen a 10-bit
address chip in 3 years. And the SMBus 2.0 specification even says:

"All 10-bit slave addresses are reserved for future use and are outside
the scope of this specification."

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

> Index: linux-2.6.17/drivers/i2c/i2c-core.c
> ===================================================================
> --- linux-2.6.17.orig/drivers/i2c/i2c-core.c	2006-08-26 17:08:26.234925000 +0200
> +++ linux-2.6.17/drivers/i2c/i2c-core.c	2006-08-26 21:21:45.696832000 +0200
> @@ -1126,6 +1126,47 @@
>  	return res;
>  }
>  
> +void i2c_smbus_alert_notify(struct i2c_adapter *adapter, unsigned short addr)
> +{
> +	struct list_head   *item;
> +	struct i2c_client  *client = NULL;
> +
> +	mutex_lock(&adapter->clist_lock);
> +	list_for_each(item,&adapter->clients) {
> +		client = list_entry(item, struct i2c_client, list);
> +		if (client->addr == addr)
> +		        break;
> +	}
> +	mutex_unlock(&adapter->clist_lock);
> +	if (client && client->driver->smbus_alert)
> +	        client->driver->smbus_alert(client);
> +}

If no client with the given address is found (which could happen if a
device without a driver generates an alert) you end up running
smbus_alert() on the last client of the list. This isn't correct.

> +
> +/* read from the alert-reponse-address (ARA)
> + * must NOT be called from interrupt context
> + */
> +#define I2C_SMBUS_ADDR_ARA 0xC0

I would make that I2C_ADDR_SMBUS_ARA to be inline with a number of
other constants defined in the kernel tree. And the value is 0x0C, not
0xC0!

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

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

> +	        return ((addr >> 1) & 0x3F);

Broken mask, should be 0x7F... but you don't actually need any masking
than I can see, as you aleady have a 7-bit value at this point.

> +	else
> +	        return 0;
> +}

Returning 0 on error, interesting convention ;)

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

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.

>  
>  /* Next four are needed by i2c-isa */
>  EXPORT_SYMBOL_GPL(i2c_adapter_dev_release);
> @@ -1163,6 +1204,9 @@
>  EXPORT_SYMBOL(i2c_smbus_read_i2c_block_data);
>  EXPORT_SYMBOL(i2c_smbus_write_i2c_block_data);
>  
> +EXPORT_SYMBOL(i2c_smbus_alert_notify);
> +EXPORT_SYMBOL(i2c_smbus_read_ara);
> +
>  MODULE_AUTHOR("Simon G. Vogl <simon at tk.uni-linz.ac.at>");
>  MODULE_DESCRIPTION("I2C-Bus main module");
>  MODULE_LICENSE("GPL");
> Index: linux-2.6.17/include/linux/i2c.h
> ===================================================================
> --- linux-2.6.17.orig/include/linux/i2c.h	2006-08-26 17:43:51.963774750 +0200
> +++ linux-2.6.17/include/linux/i2c.h	2006-08-26 21:15:15.220428750 +0200
> @@ -104,6 +104,11 @@
>  					  u8 command, u8 length,
>  					  u8 *values);
>  
> +/* read the device address from the ARA */
> +unsigned short i2c_smbus_read_ara(struct i2c_adapter *adapter);
> +/* call the smbus_alert function for an I2C address */
> +void i2c_smbus_alert_notify(struct i2c_adapter *adapter, unsigned short addr);
> +
>  /*
>   * A driver is capable of handling one or more physical devices present on
>   * I2C adapters. This information is used to inform the driver of adapter
> @@ -138,6 +143,10 @@
>  	 */
>  	int (*command)(struct i2c_client *client,unsigned int cmd, void *arg);
>  
> +        /* handle incoming SMBALERTs from the device
> +	 */

Make that comment a single line. And use tabulations for indentation,
please!

> +        void (*smbus_alert) (struct i2c_client *client);
> +
>  	struct device_driver driver;
>  	struct list_head list;
>  };
> 

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!

It happens that I do have hardware which supports SMBus alert (an
ADM1032 evaluation board on parallel port), so once you have something
on your side, I should be able to give it a try locally.

Thanks,
-- 
Jean Delvare