[i2c] [PATCH 2of3] change i2c-i801 driver to use internal 32-byte buffer on ICH4+
Jean Delvare
khali at linux-fr.org
Fri Jun 15 17:15:46 CEST 2007
Hi Oleg,
On Mon, 11 Jun 2007 04:05:33 -0700, Oleg Ryjkov wrote:
> Second part. The main change is here. See i2c mailing list for the
> description of all the changes. ( Thread subject: "[i2c] [PATCH] change
> to i2c-i801 driver to use internal 32-byte buffer on ICH4+".
>
> Signed-off-by: Oleg Ryjkov <olegr at google.com>
A couple comments:
> --- linux-2.6-untouched/drivers/i2c/busses/i2c-i801.c 2007-06-11 03:43:18.000000000 -0700
> +++ linux-2.6/drivers/i2c/busses/i2c-i801.c 2007-06-11 03:43:25.000000000 -0700
> @@ -26,8 +26,8 @@
> 82801AB 2423
> 82801BA 2443
> 82801CA/CAM 2483
> - 82801DB 24C3 (HW PEC supported, 32 byte buffer not supported)
> - 82801EB 24D3 (HW PEC supported, 32 byte buffer not supported)
> + 82801DB 24C3 (HW PEC supported)
> + 82801EB 24D3 (HW PEC supported)
> 6300ESB 25A4
> ICH6 266A
> ICH7 27DA
> @@ -114,7 +114,7 @@
> static struct pci_dev *I801_dev;
> static int isich4;
>
> -static int i801_transaction(void)
> +static int i801_transaction(int xact)
> {
> int temp;
> int result = 0;
> @@ -139,7 +139,9 @@
> }
> }
>
> - outb_p(inb(SMBHSTCNT) | I801_START, SMBHSTCNT);
> + /* the current contents of SMBHSTCNT can be overwritten, since PEC,
> + * INTREN, SMBSCMD are passed in xact */
> + outb_p( xact | I801_START, SMBHSTCNT);
>
> /* We will always wait for a fraction of a second! */
> do {
This could have been part of the previous cleanup patch, right?
> @@ -207,44 +209,53 @@
> outb_p(temp, SMBHSTSTS);
> }
>
> -/* All-inclusive block transaction function */
> -static int i801_block_transaction(union i2c_smbus_data *data, char read_write,
> - int command, int hwpec)
> +static int i801_block_transaction_by_block(union i2c_smbus_data *data,
> + char read_write, int hwpec)
> {
> int i, len;
> - int smbcmd;
> +
> + inb_p(SMBHSTCNT); /* reset the data buffer index */
> +
> + /* Use 32-byte buffer to process this transaction */
> + if (read_write == I2C_SMBUS_WRITE) {
> + len = data->block[0];
> + outb_p(len, SMBHSTDAT0);
> + for (i = 0; i < len; i++)
> + outb_p(data->block[i+1], SMBBLKDAT);
> + }
> +
> + if (i801_transaction(I801_BLOCK_DATA | ENABLE_INT9 | hwpec))
The "hwpec" isn't correct, as it is a boolean value and not a bit
constant. The PEC_EN flag in register HST_CNT is bit 7, and here you're
instead setting bit 0 which is INTREN aka ENABLE_INT9.
> + return -1;
> +
> + if (read_write == I2C_SMBUS_READ) {
> + len = inb_p(SMBHSTDAT0);
> + if (len < 1 || len > 32)
I2C_SMBUS_BLOCK_MAX
> + return -1;
> +
> + data->block[0] = len;
> + for (i = 0; i < len; i++)
> + data->block[i + 1] = inb_p(SMBBLKDAT);
> + }
> + return 0;
> +}
> +
> +static int i801_block_transaction_byte_by_byte(union i2c_smbus_data *data,
> + char read_write, int hwpec)
> +{
> +
> int temp;
> + int i;
> int result = 0;
> + int smbcmd;
> int timeout;
> - unsigned char hostc, errmask;
> + int len;
Moving the declarations of i, len and smbcmd at the top as:
int i, len;
int smbcmd;
minimizes the differences compared to the original code, making the
patch slightly smaller and more readable.
> + unsigned char errmask;
>
> - if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
> - if (read_write == I2C_SMBUS_WRITE) {
> - /* set I2C_EN bit in configuration register */
> - pci_read_config_byte(I801_dev, SMBHSTCFG, &hostc);
> - pci_write_config_byte(I801_dev, SMBHSTCFG,
> - hostc | SMBHSTCFG_I2C_EN);
> - } else {
> - dev_err(&I801_dev->dev,
> - "I2C_SMBUS_I2C_BLOCK_READ not DB!\n");
> - return -1;
> - }
> - }
> + len = data->block[0];
>
> if (read_write == I2C_SMBUS_WRITE) {
> - len = data->block[0];
> - if (len < 1)
> - len = 1;
> - if (len > 32)
> - len = 32;
> outb_p(len, SMBHSTDAT0);
> outb_p(data->block[1], SMBBLKDAT);
> - } else {
> - len = 32; /* max for reads */
> - }
> -
> - if(isich4 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
> - /* set 32 byte buffer */
> }
>
> for (i = 1; i <= len; i++) {
> @@ -277,14 +288,11 @@
> if (((temp = inb_p(SMBHSTSTS)) & errmask) != 0x00) {
> dev_err(&I801_dev->dev,
> "Reset failed! (%02x)\n", temp);
> - result = -1;
> - goto END;
> + return -1;
> }
> - if (i != 1) {
> + if (i != 1)
> /* if die in middle of block transaction, fail */
> - result = -1;
> - goto END;
> - }
> + return -1;
> }
>
> if (i == 1)
> @@ -326,10 +334,8 @@
>
> if (i == 1 && read_write == I2C_SMBUS_READ) {
> len = inb_p(SMBHSTDAT0);
> - if (len < 1 || len > 32) {
> - result = -1;
> - goto END;
> - }
> + if (len < 1 || len > 32)
> + return -1;
> data->block[0] = len;
> }
>
> @@ -352,14 +358,65 @@
> inb_p(SMBHSTDAT0), inb_p(SMBBLKDAT));
>
> if (result < 0)
> - goto END;
> + return result;
> }
> + return result;
> +}
>
> - if (hwpec)
> +static int i801_set_block_buffer_mode(void)
> +{
> + int temp;
> + temp = inb_p(SMBAUXCTL);
> + if (temp & SMBAUXCTL_E32B)
> + return 0;
> + dev_info(&I801_dev->dev, "Enabling 32-byte data buffer\n");
> + outb_p(inb_p(SMBAUXCTL) | SMBAUXCTL_E32B, SMBAUXCTL);
Why read SMBAUXCTL again when you have it in temp?
> + if ((inb_p(SMBAUXCTL) & SMBAUXCTL_E32B) == 0) {
> + dev_err(&I801_dev->dev, "Failed to set E32B bit\n");
> + return -1;
> + }
> + return 0;
> +}
This implementation isn't compatible with the fact that you now clear
the E32B bit after each transaction. You will flood the logs with
"Enabling 32-byte data buffer" messages. And there's no point in
optimizing the "nothing to do" case, as it will virtually never happen.
> +
> +/* Block transaction function */
> +static int i801_block_transaction(union i2c_smbus_data *data, char read_write,
> + int command, int hwpec)
> +{
> + int result = 0;
> + unsigned char hostc;
> +
> + if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
> + if (read_write == I2C_SMBUS_WRITE) {
> + /* set I2C_EN bit in configuration register */
> + pci_read_config_byte(I801_dev, SMBHSTCFG, &hostc);
> + pci_write_config_byte(I801_dev, SMBHSTCFG,
> + hostc | SMBHSTCFG_I2C_EN);
> + } else {
> + dev_err(&I801_dev->dev,
> + "I2C_SMBUS_I2C_BLOCK_READ not DB!\n");
> + return -1;
> + }
> + }
> +
> + if (read_write == I2C_SMBUS_WRITE) {
> + if (data->block[0] < 1)
> + data->block[0] = 1;
> + if (data->block[0] > 32)
> + data->block[0] = 32;
I2C_SMBUS_BLOCK_MAX
> + } else {
> + data->block[0] = 32; /* max for reads */
> + }
> +
> + if (isich4 && i801_set_block_buffer_mode() == 0 )
> + result = i801_block_transaction_by_block(data, read_write,
> + hwpec);
> + else
> + result = i801_block_transaction_byte_by_byte(data, read_write,
> + hwpec);
> +
> + if (result == 0 && hwpec)
> i801_wait_hwpec();
>
> - result = 0;
> -END:
> if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
> /* restore saved configuration register value */
> pci_write_config_byte(I801_dev, SMBHSTCFG, hostc);
> @@ -430,15 +487,15 @@
>
> if(block)
> ret = i801_block_transaction(data, read_write, size, hwpec);
> - else {
> - outb_p(xact | ENABLE_INT9, SMBHSTCNT);
> - ret = i801_transaction();
> - }
> + else
> + ret = i801_transaction(xact | ENABLE_INT9);
>
> /* Some BIOSes don't like it when PEC is enabled at reboot or resume
> - time, so we forcibly disable it after every transaction. */
> + time, so we forcibly disable it after every transaction. Turn off
> + E32B for the same reason. */
> if (hwpec)
> - outb_p(inb_p(SMBAUXCTL) & (~SMBAUXCTL_CRC), SMBAUXCTL);
> + outb_p(inb_p(SMBAUXCTL) & ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B),
> + SMBAUXCTL);
>
> if(block)
> return ret;
Care to resend a patch addressing these last few issues?
Thanks,
--
Jean Delvare
More information about the i2c
mailing list