On 02/15/2013 09:20 PM, Guenter Roeck wrote: > On Fri, Feb 15, 2013 at 05:57:15PM +0100, Lars-Peter Clausen wrote: >> Currently each time the temperature register is read the driver also reads the >> threshold and hysteresis registers. This increases the amount of I2C traffic and >> time needed to read the temperature by a factor of ~5. Neither the threshold nor >> the hysteresis change on their own, so once we've read them, we should be able >> to just use the cached value of the registers. This patch modifies the code >> accordingly and only reads the threshold and hysteresis registers once during >> probe. >> >> Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx> >> --- >> drivers/hwmon/adt7410.c | 89 +++++++++++++++++++++++++++++++------------------ >> 1 file changed, 56 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/hwmon/adt7410.c b/drivers/hwmon/adt7410.c >> index 99a7290..5de0376 100644 >> --- a/drivers/hwmon/adt7410.c >> +++ b/drivers/hwmon/adt7410.c >> @@ -119,45 +119,31 @@ static int adt7410_temp_ready(struct i2c_client *client) >> return -ETIMEDOUT; >> } >> >> -static struct adt7410_data *adt7410_update_device(struct device *dev) >> +static int adt7410_update_temp(struct device *dev) >> { >> struct i2c_client *client = to_i2c_client(dev); >> struct adt7410_data *data = i2c_get_clientdata(client); >> - struct adt7410_data *ret = data; >> + int ret = 0; >> + >> mutex_lock(&data->update_lock); >> >> if (time_after(jiffies, data->last_updated + HZ + HZ / 2) >> || !data->valid) { >> - int i, status; >> >> dev_dbg(&client->dev, "Starting update\n"); >> >> - status = adt7410_temp_ready(client); /* check for new value */ >> - if (unlikely(status)) { >> - ret = ERR_PTR(status); >> + ret = adt7410_temp_ready(client); /* check for new value */ >> + if (ret) >> goto abort; >> - } >> - for (i = 0; i < ARRAY_SIZE(data->temp); i++) { >> - status = i2c_smbus_read_word_swapped(client, >> - ADT7410_REG_TEMP[i]); >> - if (unlikely(status < 0)) { >> - dev_dbg(dev, >> - "Failed to read value: reg %d, error %d\n", >> - ADT7410_REG_TEMP[i], status); >> - ret = ERR_PTR(status); >> - goto abort; >> - } >> - data->temp[i] = status; >> - } >> - status = i2c_smbus_read_byte_data(client, ADT7410_T_HYST); >> - if (unlikely(status < 0)) { >> - dev_dbg(dev, >> - "Failed to read value: reg %d, error %d\n", >> - ADT7410_T_HYST, status); >> - ret = ERR_PTR(status); >> + >> + ret = i2c_smbus_read_word_swapped(client, ADT7410_REG_TEMP[0]); >> + if (ret) { > > Should be > if (ret < 0) { > > or non-zero temperatures always create an error condition. > >> + dev_dbg(dev, "Failed to read value: reg %d, error %d\n", >> + ADT7410_REG_TEMP[0], ret); >> goto abort; >> } >> - data->hyst = status; >> + data->temp[0] = ret; >> + >> data->last_updated = jiffies; >> data->valid = true; >> } >> @@ -167,6 +153,35 @@ abort: >> return ret; > > ret can be > 0 as no-error return. So you'll either have to > change the no-error return case to always return 0, or change the > return value check when the function is called. >> } >> >> +static int adt7410_fill_cache(struct i2c_client *client) >> +{ >> + struct adt7410_data *data = i2c_get_clientdata(client); >> + int ret; >> + int i; >> + >> + for (i = 1; i < 3; i++) { > > I think using ARRAY_SIZE would be better here. With "i < 3" you don't read the > critical temperature, which proves the point (unless I am missing something). > >> + ret = i2c_smbus_read_word_swapped(client, ADT7410_REG_TEMP[i]); >> + if (ret) { > > ret < 0 > >> + dev_dbg(&client->dev, >> + "Failed to read value: reg %d, error %d\n", >> + ADT7410_REG_TEMP[0], ret); >> + return ret; >> + } >> + data->temp[i] = ret; >> + } >> + >> + ret = i2c_smbus_read_byte_data(client, ADT7410_T_HYST); >> + if (ret) { > > ret < 0 > >> + dev_dbg(&client->dev, >> + "Failed to read value: hyst reg, error %d\n", >> + ret); >> + return ret; >> + } >> + data->hyst = ret; >> + >> + return 0; >> +} >> + >> static s16 ADT7410_TEMP_TO_REG(long temp) >> { >> return DIV_ROUND_CLOSEST(clamp_val(temp, ADT7410_TEMP_MIN, >> @@ -193,10 +208,16 @@ static ssize_t adt7410_show_temp(struct device *dev, >> struct device_attribute *da, char *buf) >> { >> struct sensor_device_attribute *attr = to_sensor_dev_attr(da); >> - struct adt7410_data *data = adt7410_update_device(dev); >> + struct i2c_client *client = to_i2c_client(dev); >> + struct adt7410_data *data = i2c_get_clientdata(client); >> >> - if (IS_ERR(data)) >> - return PTR_ERR(data); >> + if (attr->index == 0) { >> + int ret; >> + >> + ret = adt7410_update_temp(dev); >> + if (ret) > > Problematic; see above. > Yep all correct, I somehow mixed up a few things between patch 6 and 7. Will fix it for v2. Thanks, - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html