On Mon, Apr 05, 2010 at 09:46:54AM +0200, Jean Delvare wrote:
> Hi Ira,
>
> On Tue, 30 Mar 2010 15:56:28 -0700, Ira W. Snyder wrote:
> > The adm1031 chip is capable of an 8 Hz update rate, configurable at runtime
> > with the fan filter register. Increase the update rate to get faster
> > temperature readings.
> >
> > Signed-off-by: Ira W. Snyder <iws@xxxxxxxxxxxxxxxx>
> > ---
> >
> > This may be a bit controversial. The ADM1031 chip may use more power at a
> > higher update rate. In my (embedded) application, this doesn't matter. The
> > faster update rate is more important.
>
> If you use the automatic fan speed control capabilities, then indeed a
> faster update rate can be benefical. But as you suspected, it is
> somewhat controversial, due to the higher power consumption possibly
> for no good reason. Also, the BIOS/firmware might have already
> configured the chip and traditionally the hwmon drivers preserve these
> settings as much as possible.
>
Yep, that makes perfect sense.
> > Does anyone have thoughts on this? It is easy enough for me to carry
> > myself, but I'd much rather have it upstream.
>
> I wouldn't want it upstream as is. However I can think of two
> alternative implementations which I would be happy with:
>
> 1* Let the adm1031 driver accept platform data, and the update rate be
> part of that platform data. Then you can instantiate your device
> explicitly and pass the desired update rate.
>
> 2* Expose the update frequency through sysfs, and make it writable.
> After all, so many monitoring devices have a configurable update rate
> that it would make sense to have a standard attribute for it. If you go
> that route, we have to standardize on it first, and add it to
> Documentation/hwmon/sysfs-interface. Then implement it in the adm1031
> driver and possibly other drivers.
>
> Note that both options aren't mutually exclusive, although one of them
> should be enough to satisfy your need.
>
I like the idea of a sysfs node. Does "update_rate" (in milliseconds)
sound ok, or do you have another name you'd prefer?
Thanks,
Ira
> > drivers/hwmon/adm1031.c | 6 +++++-
> > 1 files changed, 5 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
> > index 1644b92..c7195cc 100644
> > --- a/drivers/hwmon/adm1031.c
> > +++ b/drivers/hwmon/adm1031.c
> > @@ -36,6 +36,7 @@
> > #define ADM1031_REG_FAN_DIV(nr) (0x20 + (nr))
> > #define ADM1031_REG_PWM (0x22)
> > #define ADM1031_REG_FAN_MIN(nr) (0x10 + (nr))
> > +#define ADM1031_REG_FAN_FILTER (0x23)
> >
> > #define ADM1031_REG_TEMP_OFFSET(nr) (0x0d + (nr))
> > #define ADM1031_REG_TEMP_MAX(nr) (0x14 + 4 * (nr))
> > @@ -919,6 +920,9 @@ static void adm1031_init_client(struct i2c_client *client)
> > ADM1031_CONF1_MONITOR_ENABLE);
> > }
> >
> > + /* Increase the ADC sampling rate -- temperatures update at 8 Hz */
> > + read_val = adm1031_read_value(client, ADM1031_REG_FAN_FILTER);
> > + adm1031_write_value(client, ADM1031_REG_FAN_FILTER, read_val | 0x1C);
> > }
> >
> > static struct adm1031_data *adm1031_update_device(struct device *dev)
> > @@ -929,7 +933,7 @@ static struct adm1031_data *adm1031_update_device(struct device *dev)
> >
> > mutex_lock(&data->update_lock);
> >
> > - if (time_after(jiffies, data->last_updated + HZ + HZ / 2)
> > + if (time_after(jiffies, data->last_updated + HZ / 8)
> > || !data->valid) {
> >
> > dev_dbg(&client->dev, "Starting adm1031 update\n");
>
>
> --
> Jean Delvare
>
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
