> Done.
My comments inline:
> @@ -126,7 +126,7 @@
> if (tmp < 0) return (-EIO);
>
> /* convert the data to little endian format */
> - *value = swab16((u16) tmp);
> + *value = ((u16) tmp >> 8) | (u16) ((u16) tmp << 8);
>
> return (0);
> }
> @@ -134,10 +134,8 @@
> static inline int lm92_write16 (struct i2c_client *client,u8 reg,u16 value)
> {
> /* convert the data to big endian format */
> - if (i2c_smbus_write_word_data(client, reg, swab16(value)) < 0)
> - return -EIO;
> -
> - return 0;
> + u16 be = (value >> 8) | (u16) (value << 8);
> + return (i2c_smbus_write_word_data (client,reg,be) < 0 ? -EIO : 0);
> }
These are two changes I just commited to the CVS repository. Please don't revert them.
> +static int max6635_check(struct i2c_client *client)
> +{
> + lm92_t *data = (lm92_t *) client->data;
> + int i, cn = 8;
I hardly can see any reason to define cn as a variable.
> + u16 temp_low, temp_high, temp_hyst, temp_max;
> +
> + temp_low = i2c_smbus_read_word_data(client, LM92_REG_TRIP_LOW);
> + temp_high = i2c_smbus_read_word_data(client, LM92_REG_TRIP_HIGH);
> + temp_hyst = i2c_smbus_read_word_data(client, LM92_REG_TRIP_HYSTERESIS);
> + temp_max = i2c_smbus_read_word_data(client, LM92_REG_TRIP_CRITICAL);
> +
> + if (!(!(temp_low & 0x7f00) && !(temp_high & 0x7f00) && !(temp_hyst & 0x7f00) && !(temp_max & 0x7f00)))
> + return 0;
You can spare negations by just using ||'s instead of &&'s (De Morgan's
law, http://en.wikipedia.org/wiki/De_Morgan's_law).
> + for (i=0; i<32; ++i)
You can skip i=0, it cannot fail.
> + {
> + if ((i % 2 == 0) && (i < 16))
> + {
> + if ((i2c_smbus_read_word_data(client, LM92_REG_TRIP_LOW + i*cn) != temp_low) ||
> + (temp_high != i2c_smbus_read_word_data(client, LM92_REG_TRIP_HIGH + i*cn)) ||
> + (temp_hyst != i2c_smbus_read_word_data(client, LM92_REG_TRIP_HYSTERESIS + i*cn)) ||
> + (temp_max != i2c_smbus_read_word_data(client, LM92_REG_TRIP_CRITICAL + i*cn)))
> + return 0;
> + }
> + else
> + {
> + if ((i2c_smbus_read_word_data(client, LM92_REG_TRIP_LOW + i*cn) != 0) ||
> + (0 != i2c_smbus_read_word_data(client, LM92_REG_TRIP_HIGH + i*cn)) ||
> + (0 != i2c_smbus_read_word_data(client, LM92_REG_TRIP_HYSTERESIS + i*cn)) ||
> + (0 != i2c_smbus_read_word_data(client, LM92_REG_TRIP_CRITICAL + i*cn)))
> + return 0;
I don't think you can rely of these zeroes. Maybe other chips would return random values or the limits.
I would simply do "for (i=2; i<16; i+=2)" (or even "for (i=1; i<8; ++i)"
with cn=16) and discard that second test.
> + data->temp.low = swab16(temp_low);
> + data->temp.high = swab16(temp_high);
> + data->temp.crit = swab16(temp_max);
> + data->temp.hyst = swab16(temp_hyst);
> + data->temp.low = swab16(i2c_smbus_read_word_data(client, LM92_REG_TEMPERATURE));
Bug here.
Anyway I don't see the point in filling the variables since you don't
update data->timestamp accordingly (so everything will be read again
anyway). And you cannot update data->timestamp if you don't read the
alarms too. All in all you would have to duplicate lm92_read here. So
either don't store the values at all (recommended) or call lm92_read
instead or rewriting it.
> @@ -294,20 +335,25 @@
> return (-ERESTARTSYS);
> }
>
> - if ((kind < 0 && lm92_read16 (client,LM92_REG_MANUFACTURER,&manufacturer) < 0) ||
> - manufacturer != LM92_MANUFACTURER_ID) {
> - kfree (client);
> - up (&mutex);
> - return (-ENODEV);
> - }
> -
> if ((result = i2c_attach_client (client))) {
> kfree (client);
> up (&mutex);
> return (result);
> }
>
> - if ((result = i2c_register_entry (client,client->name,lm92_dir_table)) < 0) {
> + if ((kind < 0 && lm92_read16 (client,LM92_REG_MANUFACTURER,&manufacturer) < 0) ||
> + manufacturer != LM92_MANUFACTURER_ID) {
> +
> + if (!max6635_check(client))
> + {
> + i2c_detach_client (client);
> + kfree (client);
> + up (&mutex);
> + return (-ENODEV);
> + }
> + }
> +
> + if ((result = i2c_register_entry (client,client->name,lm92_dir_table, THIS_MODULE)) < 0) {
> i2c_detach_client (client);
> kfree (client);
> up (&mutex);
Why did you move swapped the detection and the client attachement?
Detecting before we attempt to attach the client is actually the correct
order.
The additional parameter to i2c_register_entry breaks compilation. I
suspect that you use an outdated version of i2c. Get at least version
2.8.2.
> static struct i2c_driver lm92_driver = {
> - .owner = THIS_MODULE,
> .name = "lm92",
> - .id = I2C_DRIVERID_LM92,
> .flags = I2C_DF_NOTIFY,
> .attach_adapter = lm92_attach_adapter,
> .detach_client = lm92_detach_client,
No, don't do that.
--
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
