How to read 2’s complement value from two registers into an int

Question

I am trying to read values from the STC3100 battery monitor IC, but the values I am getting are not correct. What the datasheet says:

The temperature value is coded in 2’s complement format, and the LSB value is 0.125° C.

REG_TEMPERATURE_LOW, address 10, temperature value, bits 0-7
REG_TEMPERATURE_HIGH, address 11, temperature value, bits 8-15

This is the datasheet: http://www.st.com/internet/com/TECHNICAL_RESOURCES/TECHNICAL_LITERATURE/DATASHEET/CD00219947.pdf

What I have in my code:

__u8 regaddr = 0x0a; /* Device register to access */
__s32 res_l, res_h;

int temp_value;
float temperature;

res_l = i2c_smbus_read_word_data(myfile, regaddr);
regaddr++;
res_h = i2c_smbus_read_word_data(myfile, regaddr);
if (res_l < 0) {
  /* ERROR HANDLING: i2c transaction failed */
} else {
  temp_value = (res_h << 8)+res_l;
  temperature = (float)temp_value * 0.125;
  printf("Temperature: %4.2f C\n", temperature);
}

What am I doing wrong? Is this not how I should copy a 2's complement value into an int?

Answer 1

i2c_smbus_read_word_data() will read 16 bits starting from your specified register on the device, so a single i2c_smbus_read_word_data() will read both registers that you're interested in using a single i2c transaction.

i2c_smbus_read_word_data() returns the 16 bits read from the device as an unsigned quantity - if there's an error, the return from i2c_smbus_read_word_data() will be negative. You should be able to read the temperature sensor like so:

__u8 regaddr = 0x0a; /* Device register to access */
__s32 res;

int temp_value;
float temperature;

res = i2c_smbus_read_word_data(myfile, regaddr);

if (res < 0) {
  /* ERROR HANDLING: i2c transaction failed */
} else {
  temp_value = (__s16) res;
  temperature = (float)temp_value * 0.125;
  printf("Temperature: %4.2f C\n", temperature);
}

To address questions from the comments:

The i2c_smbus_read_word_data() function returns the 16 bits of data obtained from the i2c bus as an unsigned 16-bit value if there's no error. A 16-bit unsigned value can easily be represented in the 32-bit int returned by the function, so by definition the 16-bits of data cannot be negative. res will be negative if and only if there's an error.

Interpreting the 16 bit value as a (possibly negative) two's complement value is handled by the (__s16) cast of res. This takes that value that's in res and converts it to a signed 16-bit int representation. Strictly speaking, it's implementation-defined regarding how negative numbers will be dealt with by this cast. I believe that on Linux implementations, this will always simply treat the lower 16 bits of res as a two's complement number.

If you're concerned about the implementation defined aspect of the (__s16) cast, you can avoid it by using arithmetic instead of a cast as in caf's answer:

temp_value = (res > 0x7fff) ? res - (0xffff + 1) : res;

Which will perform the correct conversion to a negative value even if you happen to be running on a one's complement machine (does Linux even support running on such a thing?).

Also note that the above posted code assumes you're running on a little-endian machine - you'll need to swap the bytes appropriately on a big-endian machine before converting the data to a negative value, The following should do the trick however the target CPU represents integer values (big/little, one' or two's):

__u16 data = __le16_to_cpu( (__u16) res);

// convert negative two's complement values to native negative value:
int temp_value = (data > 0x7fff) ? data - (0xffff + 1) : data;