[PATCH] hwmon: Add PEC support to the lm90 driver

Add PEC support to the lm90 driver. Only the ADM1032 chip supports it,
and in a rather tricky way, which is why this patch comes with
documentation reinforcements. At least, this demonstrates that the new
PEC support logic in i2c-core can properly deal with chips with partial
PEC support.

As enabling PEC causes a significant performance drop, it can be
disabled through a sysfs file (unsurprisingly named "pec").

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

diff --git a/Documentation/hwmon/lm90 b/Documentation/hwmon/lm90
index 2c4cf39..70abf93 100644 (file)
--- a/Documentation/hwmon/lm90
+++ b/Documentation/hwmon/lm90
@@ -71,8 +71,8 @@ increased resolution of the remote temperature measurement.
 
 The different chipsets of the family are not strictly identical, although
 very similar. This driver doesn't handle any specific feature for now,
-but could if there ever was a need for it. For reference, here comes a
-non-exhaustive list of specific features:
+with the exception of SMBus PEC. For reference, here comes a non-exhaustive
+list of specific features:
 
 LM90:
   * Filter and alert configuration register at 0xBF.
@@ -91,6 +91,7 @@ ADM1032:
   * Conversion averaging.
   * Up to 64 conversions/s.
   * ALERT is triggered by open remote sensor.
+  * SMBus PEC support for Write Byte and Receive Byte transactions.
 
 ADT7461
   * Extended temperature range (breaks compatibility)
@@ -119,3 +120,37 @@ The lm90 driver will not update its values more frequently than every
 other second; reading them more often will do no harm, but will return
 'old' values.
 
+PEC Support
+-----------
+
+The ADM1032 is the only chip of the family which supports PEC. It does
+not support PEC on all transactions though, so some care must be taken.
+
+When reading a register value, the PEC byte is computed and sent by the
+ADM1032 chip. However, in the case of a combined transaction (SMBus Read
+Byte), the ADM1032 computes the CRC value over only the second half of
+the message rather than its entirety, because it thinks the first half
+of the message belongs to a different transaction. As a result, the CRC
+value differs from what the SMBus master expects, and all reads fail.
+
+For this reason, the lm90 driver will enable PEC for the ADM1032 only if
+the bus supports the SMBus Send Byte and Receive Byte transaction types.
+These transactions will be used to read register values, instead of
+SMBus Read Byte, and PEC will work properly.
+
+Additionally, the ADM1032 doesn't support SMBus Send Byte with PEC.
+Instead, it will try to write the PEC value to the register (because the
+SMBus Send Byte transaction with PEC is similar to a Write Byte transaction
+without PEC), which is not what we want. Thus, PEC is explicitely disabled
+on SMBus Send Byte transactions in the lm90 driver.
+
+PEC on byte data transactions represents a significant increase in bandwidth
+usage (+33% for writes, +25% for reads) in normal conditions. With the need
+to use two SMBus transaction for reads, this overhead jumps to +50%. Worse,
+two transactions will typically mean twice as much delay waiting for
+transaction completion, effectively doubling the register cache refresh time.
+I guess reliability comes at a price, but it's quite expensive this time.
+
+So, as not everyone might enjoy the slowdown, PEC can be disabled through
+sysfs. Just write 0 to the "pec" file and PEC will be disabled. Write 1
+to that file to enable PEC again.

diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
index 3464005..764cdc5 100644 (file)
--- a/Documentation/hwmon/sysfs-interface
+++ b/Documentation/hwmon/sysfs-interface
@@ -272,3 +272,6 @@ beep_mask   Bitmask for beep.
 
 eeprom         Raw EEPROM data in binary form.
                Read only.
+
+pec            Enable or disable PEC (SMBus only)
+               Read/Write

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 5acc12a..fa0793e 100644 (file)
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -345,15 +345,63 @@ static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
 static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 4);
 static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
 
+/* pec used for ADM1032 only */
+static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
+                       char *buf)
+{
+       struct i2c_client *client = to_i2c_client(dev);
+       return sprintf(buf, "%d\n", !!(client->flags & I2C_CLIENT_PEC));
+}
+
+static ssize_t set_pec(struct device *dev, struct device_attribute *dummy,
+                      const char *buf, size_t count)
+{
+       struct i2c_client *client = to_i2c_client(dev);
+       long val = simple_strtol(buf, NULL, 10);
+
+       switch (val) {
+       case 0:
+               client->flags &= ~I2C_CLIENT_PEC;
+               break;
+       case 1:
+               client->flags |= I2C_CLIENT_PEC;
+               break;
+       default:
+               return -EINVAL;
+       }
+
+       return count;
+}
+
+static DEVICE_ATTR(pec, S_IWUSR | S_IRUGO, show_pec, set_pec);
+
 /*
  * Real code
  */
 
+/* The ADM1032 supports PEC but not on write byte transactions, so we need
+   to explicitely ask for a transaction without PEC. */
+static inline s32 adm1032_write_byte(struct i2c_client *client, u8 value)
+{
+       return i2c_smbus_xfer(client->adapter, client->addr,
+                             client->flags & ~I2C_CLIENT_PEC,
+                             I2C_SMBUS_WRITE, value, I2C_SMBUS_BYTE, NULL);
+}
+
+/* It is assumed that client->update_lock is held (unless we are in
+   detection or initialization steps). This matters when PEC is enabled,
+   because we don't want the address pointer to change between the write
+   byte and the read byte transactions. */
 static int lm90_read_reg(struct i2c_client* client, u8 reg, u8 *value)
 {
        int err;
 
-       err = i2c_smbus_read_byte_data(client, reg);
+       if (client->flags & I2C_CLIENT_PEC) {
+               err = adm1032_write_byte(client, reg);
+               if (err >= 0)
+                       err = i2c_smbus_read_byte(client);
+       } else
+               err = i2c_smbus_read_byte_data(client, reg);
 
        if (err < 0) {
                dev_warn(&client->dev, "Register %#02x read failed (%d)\n",
@@ -494,6 +542,10 @@ static int lm90_detect(struct i2c_adapter *adapter, int address, int kind)
                name = "lm90";
        } else if (kind == adm1032) {
                name = "adm1032";
+               /* The ADM1032 supports PEC, but only if combined
+                  transactions are not used. */
+               if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
+                       new_client->flags |= I2C_CLIENT_PEC;
        } else if (kind == lm99) {
                name = "lm99";
        } else if (kind == lm86) {
@@ -546,6 +598,9 @@ static int lm90_detect(struct i2c_adapter *adapter, int address, int kind)
                           &sensor_dev_attr_temp2_crit_hyst.dev_attr);
        device_create_file(&new_client->dev, &dev_attr_alarms);
 
+       if (new_client->flags & I2C_CLIENT_PEC)
+               device_create_file(&new_client->dev, &dev_attr_pec);
+
        return 0;
 
 exit_detach: