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.
> + return ret;
> + }
>
> return sprintf(buf, "%d\n", ADT7410_REG_TO_TEMP(data,
> data->temp[attr->index]));
> @@ -232,13 +253,11 @@ static ssize_t adt7410_show_t_hyst(struct device *dev,
> char *buf)
> {
> struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> - struct adt7410_data *data;
> + struct i2c_client *client = to_i2c_client(dev);
> + struct adt7410_data *data = i2c_get_clientdata(client);
> int nr = attr->index;
> int hyst;
>
> - data = adt7410_update_device(dev);
> - if (IS_ERR(data))
> - return PTR_ERR(data);
> hyst = (data->hyst & ADT7410_T_HYST_MASK) * 1000;
>
> /*
> @@ -371,6 +390,10 @@ static int adt7410_probe(struct i2c_client *client,
> }
> dev_dbg(&client->dev, "Config %02x\n", data->config);
>
> + ret = adt7410_fill_cache(client);
> + if (ret)
> + goto exit_restore;
> +
> /* Register sysfs hooks */
> ret = sysfs_create_group(&client->dev.kobj, &adt7410_group);
> if (ret)
> --
> 1.8.0
>
>
--
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
