* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
@ 2005-11-02 14:24 ` Jean Delvare
2005-11-02 15:17 ` Roger Lucas
` (19 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Jean Delvare @ 2005-11-02 14:24 UTC (permalink / raw)
To: lm-sensors
Hi Roger,
On 2005-11-2, Roger Lucas wrote:
> The fan is handled correctly, but you have the nasty condition where if
> you set a minimum speed that is too low, it is set to zero. The Via
> speed sensor works on period rather than frequency, so there is a 1/x
> conversion done in the driver.
Note that this is pretty standard, almost all fan tachometers work that
way. The only drivers which do not have to invert the register value are
the FSC ones if I remember correctly.
> If you select a frequency that is too low, then the conversion to a
> period creates a period that is too large for the counters and the
> resulting illegal result is rejected.
It's not rejected, it's even worse than that: it's rounded to the
greatest possible value (255), which in turn is read back as 0 RPM.
> If you want very slow fan
> speeds then you must make sure the divider is set high enough so that
> the resulting period is within the limits of the device.
>
> Personally, I don't like this. It isn't really a massive amount of work
> to get the driver to automatically either:
>
> 1) Automatically select the appropriate divider for the minimum speed
> that you have requested (i.e. pick the highest divider ratio that can
> give the speed you want range)
>
> Or
> 2) Select the lowest possible minimum speed if the one you selected is
> still too low.
I think that most drivers implement solution 2. We have a (low) number of
drivers which implement automatic fan-div switching (pc87360, adm9240),
which is a subset of solution 1.
> A minimum speed of 0 should always disable the minimum speed detection.
We can't always do that, as it depends on what the chip itself does.
Some chips have a dedicated bit to disable the alarm. Some chips disable
the alarm if the limit register is set to 0. Some chips disable the
alarm if the limit register is set to 255. What does the VT8231 do?
> Resetting the minimum speed to zero when you select a non-zero minimum
> speed is just plain wrong, however, as it is not obvious exactly what is
> happening "behind the scenes". I spent quite a bit of time debugging the
> system to convince myself it was working "correctly".
> (...)
> Anyone else have thoughts on this?
I completely agree with you. Please fix the driver. We should port the
fix back to lm_sensors CVS too. In fact, I think the current behavior is
a bug in the original code. Consider the following code:
> /********* FAN RPM CONVERSIONS ********/
> /* But this chip saturates back at 0, not at 255 like all the other chips.
> So, 0 means 0 RPM */
> static inline u8 FAN_TO_REG(long rpm, int div)
> {
> if (rpm = 0)
> return 0;
> rpm = SENSORS_LIMIT(rpm, 1, 1000000);
> return SENSORS_LIMIT((1310720 + rpm * div / 2) / (rpm * div), 1,
> 255);
> }
>
> #define MIN_TO_REG(a, b) FAN_TO_REG(a, b)
> #define FAN_FROM_REG(val, div) \
> ((val)=0?0:(val)=255?0:1310720/((val)*(div)))
The comment clearly states that the register value will be 0 if the fan
speed is too low to be measured under the currently selected clock
divider. I guess this is true for stopped or missing fan as well (can
you confirm?) Thus there is no reason to consider the register value 255
any differently from other non-zero values, is there? The FAN_TO_REG
function does actually not, but the FAN_FROM_REG macro does, and I
believe this is where the bug is.
Try removing the (val)=255 case in FAN_TO_REG, and I think the driver
will behave like you described above (solution 2).
While you're there, please delete the MIN_TO_REG macro and use
FAN_TO_REG instead.
You could also get rid of the middle SENSORS_LIMIT call in FAN_TO_REG,
providing you use strtoul instead of strtol to read the input value (in
set_fan_min). The lower bound will be handled by strtoul and the = 0
check. The upper bound is redundant with the next SENSORS_LIMIT call.
If you are willing to implement the solution 1, fine with me. This isn't
a requirement for me to accept the driver in Linux 2.6 though.
I will try to review the code further later today. It would be great if
we could integrate this ported driver in Linux 2.6 soon, it has been
around for too long already.
Thanks a lot for your work,
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
2005-11-02 14:24 ` Jean Delvare
@ 2005-11-02 15:17 ` Roger Lucas
2005-11-02 19:47 ` Knut Petersen
` (18 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Roger Lucas @ 2005-11-02 15:17 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
Updated patch attached. I love it when you inherit loads of code that you
end up fixing... :-)
I am not so sure I am very keen on the intelligence for the auto-div
calculation moving into the driver. I think this code should be in the
sensors utility.
On the other hand, however, the way the DIV_TO_REG() function was coded, it
would be almost impossible for any application to determine the legal values
for DIV without knowing the details of the device. Any illegal value would
end up with DIV set at zero - not very helpful. As an alternative, this
attached patch includes a code revision that sets DIV to the smallest legal
DIV value that is the same as or larger than the requested DIV value.
Hence, the mapping is as below:
Requested DIV Set DIV
1 1
2 2
3 4
4 4
5 8
6 8
7 8
8 8
8+ 8
From this sequence, the DIV returned is predictable and an application can
very quickly determine the legal DIV values and make its calculations
accordingly.
With this revised code, if you ask for a low limit 1000 RPM with a divisor
of 4, you get a low limit of 1285 RPM returned. If you ask for a low limit
of 1000 RPM with a divisor of 5 then you get a low limit of 999 RPM returned
with a divisor of 8.
Consider the pseudo-code below:
set_div = 1;
set_rpm_div(set_div);
set_low_limit_rpm(requested_limit_rpm);
while( get_low_limit_rpm() > requested_limit_rpm )
{
true_div = get_rpm_div();
if (set_div > true_div)
{
break; // we have exceeded the DIV range
}
// Try next div value
set_div = true_div + 1;
set_rpm_div(set_div);
set_low_limit_rpm(requested_limit_rpm);
}
This will iterate through the valid DIV values and stop when it reaches the
best RPM match or the highest valid DIV setting.
I feel that this should be up in the application, however, and not in the
driver. IMHO, the drivers should be a simple as possible.
- Roger
-----Original Message-----
From: Jean Delvare [mailto:khali@linux-fr.org]
Sent: 02 November 2005 13:12
To: roger@planbit.co.uk
Cc: 'LM Sensors'; Knut Petersen
Subject: [lm-sensors] RE: vt8231.c
Hi Roger,
On 2005-11-2, Roger Lucas wrote:
> The fan is handled correctly, but you have the nasty condition where if
> you set a minimum speed that is too low, it is set to zero. The Via
> speed sensor works on period rather than frequency, so there is a 1/x
> conversion done in the driver.
Note that this is pretty standard, almost all fan tachometers work that
way. The only drivers which do not have to invert the register value are
the FSC ones if I remember correctly.
> If you select a frequency that is too low, then the conversion to a
> period creates a period that is too large for the counters and the
> resulting illegal result is rejected.
It's not rejected, it's even worse than that: it's rounded to the
greatest possible value (255), which in turn is read back as 0 RPM.
> If you want very slow fan
> speeds then you must make sure the divider is set high enough so that
> the resulting period is within the limits of the device.
>
> Personally, I don't like this. It isn't really a massive amount of work
> to get the driver to automatically either:
>
> 1) Automatically select the appropriate divider for the minimum speed
> that you have requested (i.e. pick the highest divider ratio that can
> give the speed you want range)
>
> Or
> 2) Select the lowest possible minimum speed if the one you selected is
> still too low.
I think that most drivers implement solution 2. We have a (low) number of
drivers which implement automatic fan-div switching (pc87360, adm9240),
which is a subset of solution 1.
> A minimum speed of 0 should always disable the minimum speed detection.
We can't always do that, as it depends on what the chip itself does.
Some chips have a dedicated bit to disable the alarm. Some chips disable
the alarm if the limit register is set to 0. Some chips disable the
alarm if the limit register is set to 255. What does the VT8231 do?
> Resetting the minimum speed to zero when you select a non-zero minimum
> speed is just plain wrong, however, as it is not obvious exactly what is
> happening "behind the scenes". I spent quite a bit of time debugging the
> system to convince myself it was working "correctly".
> (...)
> Anyone else have thoughts on this?
I completely agree with you. Please fix the driver. We should port the
fix back to lm_sensors CVS too. In fact, I think the current behavior is
a bug in the original code. Consider the following code:
> /********* FAN RPM CONVERSIONS ********/
> /* But this chip saturates back at 0, not at 255 like all the other chips.
> So, 0 means 0 RPM */
> static inline u8 FAN_TO_REG(long rpm, int div)
> {
> if (rpm = 0)
> return 0;
> rpm = SENSORS_LIMIT(rpm, 1, 1000000);
> return SENSORS_LIMIT((1310720 + rpm * div / 2) / (rpm * div), 1,
> 255);
> }
>
> #define MIN_TO_REG(a, b) FAN_TO_REG(a, b)
> #define FAN_FROM_REG(val, div) \
> ((val)=0?0:(val)=255?0:1310720/((val)*(div)))
The comment clearly states that the register value will be 0 if the fan
speed is too low to be measured under the currently selected clock
divider. I guess this is true for stopped or missing fan as well (can
you confirm?) Thus there is no reason to consider the register value 255
any differently from other non-zero values, is there? The FAN_TO_REG
function does actually not, but the FAN_FROM_REG macro does, and I
believe this is where the bug is.
Try removing the (val)=255 case in FAN_TO_REG, and I think the driver
will behave like you described above (solution 2).
While you're there, please delete the MIN_TO_REG macro and use
FAN_TO_REG instead.
You could also get rid of the middle SENSORS_LIMIT call in FAN_TO_REG,
providing you use strtoul instead of strtol to read the input value (in
set_fan_min). The lower bound will be handled by strtoul and the = 0
check. The upper bound is redundant with the next SENSORS_LIMIT call.
If you are willing to implement the solution 1, fine with me. This isn't
a requirement for me to accept the driver in Linux 2.6 though.
I will try to review the code further later today. It would be great if
we could integrate this ported driver in Linux 2.6 soon, it has been
around for too long already.
Thanks a lot for your work,
--
Jean Delvare
-------------- next part --------------
diff -Naur reference/drivers/hwmon/Kconfig linux-2.6.14/drivers/hwmon/Kconfig
--- reference/drivers/hwmon/Kconfig 2005-10-28 01:02:08.000000000 +0100
+++ linux-2.6.14/drivers/hwmon/Kconfig 2005-11-01 17:35:18.000000000 +0000
@@ -350,6 +350,18 @@
This driver can also be built as a module. If so, the module
will be called via686a.
+config SENSORS_VT8231
+ tristate "VT8231"
+ depends on HWMON && I2C && PCI && EXPERIMENTAL
+ select HWMON_VID
+ select I2C_ISA
+ help
+ If you say yes here then you get support for the integrated sensors
+ in the Via VT8231 device.
+
+ This driver can also be built as a module. If so, the module
+ will be called vt8231.
+
config SENSORS_W83781D
tristate "Winbond W83781D, W83782D, W83783S, W83627HF, Asus AS99127F"
depends on HWMON && I2C
diff -Naur reference/drivers/hwmon/Makefile linux-2.6.14/drivers/hwmon/Makefile
--- reference/drivers/hwmon/Makefile 2005-10-28 01:02:08.000000000 +0100
+++ linux-2.6.14/drivers/hwmon/Makefile 2005-11-01 10:59:10.000000000 +0000
@@ -40,6 +40,7 @@
obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
obj-$(CONFIG_SENSORS_VIA686A) += via686a.o
+obj-$(CONFIG_SENSORS_VT8231) += vt8231.o
obj-$(CONFIG_SENSORS_W83627EHF) += w83627ehf.o
obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o
diff -Naur reference/drivers/hwmon/vt8231.c linux-2.6.14/drivers/hwmon/vt8231.c
--- reference/drivers/hwmon/vt8231.c 1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.14/drivers/hwmon/vt8231.c 2005-11-02 13:50:57.000000000 +0000
@@ -0,0 +1,783 @@
+/*
+ vt8231.c - Part of lm_sensors, Linux kernel modules
+ for hardware monitoring
+
+ Copyright (c) 2002 Mark D. Studebaker <mdsxyz123@yahoo.com>
+ Aaron M. Marsh <amarsh@sdf.lonestar.org>
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program; if not, write to the Free Software
+ Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+*/
+
+/* Supports VIA VT8231 South Bridge embedded sensors
+*/
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/pci.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/i2c-isa.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-vid.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <asm/io.h>
+
+static int force_addr;
+module_param(force_addr, int,0644);
+MODULE_PARM_DESC(force_addr, "Initialize the base address of the sensors");
+
+/* Device address
+ Note that we can't determine the ISA address until we have initialized
+ our module */
+static unsigned short isa_address;
+
+#define VT8231_EXTENT 0x80
+#define VT8231_BASE_REG 0x70
+#define VT8231_ENABLE_REG 0x74
+
+/* pwm numbered 1-2 */
+#define VT8231_REG_PWM(nr) (0x5f + (nr))
+#define VT8231_REG_PWM_CTL 0x51
+
+/* The VT8231 registers */
+/* We define the sensors as follows. Somewhat convoluted to minimize
+ changes from via686a.
+ Sensor Voltage Mode Temp Mode
+ -------- ------------ ---------
+ Reading 1 temp3
+ Reading 3 temp1 not in vt8231
+ UCH1/Reading2 in0 temp2
+ UCH2 in1 temp4
+ UCH3 in2 temp5
+ UCH4 in3 temp6
+ UCH5 in4 temp7
+ 3.3V in5
+ -12V in6 not in vt8231
+*/
+
+/* ins numbered 0-6 */
+#define VT8231_REG_IN_MAX(nr) ((nr)=0 ? 0x3d : 0x29 + ((nr) * 2))
+#define VT8231_REG_IN_MIN(nr) ((nr)=0 ? 0x3e : 0x2a + ((nr) * 2))
+#define VT8231_REG_IN(nr) (0x21 + (nr))
+
+/* fans numbered 1-2 */
+#define VT8231_REG_FAN_MIN(nr) (0x3a + (nr))
+#define VT8231_REG_FAN(nr) (0x28 + (nr))
+
+static const u8 regtemp[] = { 0x20, 0x21, 0x1f, 0x22, 0x23, 0x24, 0x25 };
+static const u8 regover[] = { 0x39, 0x3d, 0x1d, 0x2b, 0x2d, 0x2f, 0x31 };
+static const u8 reghyst[] = { 0x3a, 0x3e, 0x1e, 0x2c, 0x2e, 0x30, 0x32 };
+
+/* temps numbered 1-7 */
+#define VT8231_REG_TEMP(nr) (regtemp[(nr) - 1])
+#define VT8231_REG_TEMP_OVER(nr) (regover[(nr) - 1])
+#define VT8231_REG_TEMP_HYST(nr) (reghyst[(nr) - 1])
+#define VT8231_REG_TEMP_LOW3 0x4b /* bits 7-6 */
+#define VT8231_REG_TEMP_LOW2 0x49 /* bits 5-4 */
+#define VT8231_REG_TEMP_LOW47 0x4d
+
+#define VT8231_REG_CONFIG 0x40
+#define VT8231_REG_ALARM1 0x41
+#define VT8231_REG_ALARM2 0x42
+#define VT8231_REG_VID 0x45
+#define VT8231_REG_FANDIV 0x47
+#define VT8231_REG_UCH_CONFIG 0x4a
+#define VT8231_REG_TEMP1_CONFIG 0x4b
+#define VT8231_REG_TEMP2_CONFIG 0x4c
+
+/* temps 1-7; voltages 0-6 */
+#define ISTEMP(i, ch_config) ((i) = 1 ? 1 : \
+ (i) = 3 ? 1 : \
+ (i) = 2 ? ((ch_config) >> 1) & 0x01 : \
+ ((ch_config) >> ((i)-1)) & 0x01)
+#define ISVOLT(i, ch_config) ((i) > 4 ? 1 : !(((ch_config) >> ((i)+2)) & 0x01))
+
+#define ALARMS_FROM_REG(val) (val)
+
+#define DIV_FROM_REG(val) (1 << (val))
+#define DIV_TO_REG(val) ((val)>4?3:(val)>2?2:(val)=2?1:0)
+#define PWM_FROM_REG(val) (val)
+#define PWM_TO_REG(val) SENSORS_LIMIT((val), 0, 255)
+
+#define TEMP_FROM_REG(val) (long)((val)*10)
+#define TEMP_FROM_REG10(val) (long)(((val)*10)/4)
+#define TEMP_TO_REG(val) (SENSORS_LIMIT(((val)<0?(((val)-5)/10):\
+ ((val)+5)/10), 0, 255))
+#define IN_FROM_REG(val) ((long)(val))
+#define IN_TO_REG(val) (SENSORS_LIMIT((((val) * 10 + 5)/10), 0, 255))
+
+
+/********* FAN RPM CONVERSIONS ********/
+/* But this chip saturates back at 0, not at 255 like all the other chips.
+ So, 0 means 0 RPM */
+static inline u8 FAN_TO_REG(long rpm, int div)
+{
+ if (rpm = 0)
+ return 0;
+ return SENSORS_LIMIT((1310720 + rpm * div / 2) / (rpm * div), 1, 255);
+}
+
+#define FAN_FROM_REG(val, div) ((val)=0?0:1310720/((val)*(div)))
+
+struct vt8231_data {
+ struct i2c_client client;
+ struct semaphore update_lock;
+ struct class_device *class_dev;
+ char valid; /* !=0 if following fields are valid */
+ unsigned long last_updated; /* In jiffies */
+
+ u8 in[7]; /* Register value */
+ u8 in_max[7]; /* Register value */
+ u8 in_min[7]; /* Register value */
+ u16 temp[7]; /* Register value 10 bit */
+ u8 temp_over[7]; /* Register value */
+ u8 temp_hyst[7]; /* Register value */
+ u8 fan[2]; /* Register value */
+ u8 fan_min[2]; /* Register value */
+ u8 fan_div[2]; /* Register encoding, shifted right */
+ u16 alarms; /* Register encoding */
+ u8 pwm[2]; /* Register value */
+ u8 pwm_ctl; /* Register value */
+ u8 cpu0_vid; /* Register encoding */
+ u8 vrm;
+ u8 uch_config;
+};
+
+static struct pci_dev *s_bridge;
+static int vt8231_detect(struct i2c_adapter *adapter);
+static int vt8231_detach_client(struct i2c_client *client);
+
+static inline int vt8231_read_value(struct i2c_client *client, u8 reg)
+{
+ return (inb_p(client->addr + reg));
+}
+
+static inline void vt8231_write_value(struct i2c_client *client, u8 reg,
+ u8 value)
+{
+ outb_p(value, client->addr + reg);
+}
+
+static struct vt8231_data *vt8231_update_device(struct device *dev);
+static void vt8231_init_client(struct i2c_client *client);
+
+/* following are the sysfs callback functions */
+static ssize_t show_in(struct device *dev, char *buf, int nr) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%ld\n", IN_FROM_REG(data->in[nr]));
+}
+
+static ssize_t show_in_min(struct device *dev, char *buf, int nr) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%ld\n", IN_FROM_REG(data->in_min[nr]));
+}
+
+static ssize_t show_in_max(struct device *dev, char *buf, int nr) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%ld\n", IN_FROM_REG(data->in_max[nr]));
+}
+
+static ssize_t set_in_min(struct device *dev, const char *buf,
+ size_t count, int nr) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
+ data->in_min[nr] = IN_TO_REG(val);
+ vt8231_write_value(client, VT8231_REG_IN_MIN(nr),
+ data->in_min[nr]);
+ return count;
+}
+static ssize_t set_in_max(struct device *dev, const char *buf,
+ size_t count, int nr) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
+ data->in_max[nr] = IN_TO_REG(val);
+ vt8231_write_value(client, VT8231_REG_IN_MAX(nr),
+ data->in_max[nr]);
+ return count;
+}
+
+#define show_in_offset(offset) \
+static ssize_t show_in##offset (struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ return show_in(dev, buf, offset); \
+} \
+static ssize_t show_in##offset##_min (struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ return show_in_min(dev, buf, offset); \
+} \
+static ssize_t show_in##offset##_max (struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ return show_in_max(dev, buf, offset); \
+} \
+static ssize_t set_in##offset##_min (struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_in_min(dev, buf, count, offset); \
+} \
+static ssize_t set_in##offset##_max (struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_in_max(dev, buf, count, offset); \
+} \
+static DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in##offset, NULL);\
+static DEVICE_ATTR(in##offset##_min, S_IRUGO | S_IWUSR, \
+ show_in##offset##_min, set_in##offset##_min); \
+static DEVICE_ATTR(in##offset##_max, S_IRUGO | S_IWUSR, \
+ show_in##offset##_max, set_in##offset##_max);
+
+show_in_offset(0);
+show_in_offset(1);
+show_in_offset(2);
+show_in_offset(3);
+show_in_offset(4);
+show_in_offset(5);
+
+/* 3 temperatures */
+static ssize_t show_temp(struct device *dev, char *buf, int nr) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%ld\n", TEMP_FROM_REG10(data->temp[nr]));
+}
+static ssize_t show_temp_over(struct device *dev, char *buf, int nr) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%ld\n", TEMP_FROM_REG(data->temp_over[nr]));
+}
+static ssize_t show_temp_hyst(struct device *dev, char *buf, int nr) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%ld\n", TEMP_FROM_REG(data->temp_hyst[nr]));
+}
+static ssize_t set_temp_over(struct device *dev, const char *buf,
+ size_t count, int nr) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+ data->temp_over[nr] = TEMP_TO_REG(val);
+ vt8231_write_value(client, VT8231_REG_TEMP_OVER(nr),
+ data->temp_over[nr]);
+ return count;
+}
+static ssize_t set_temp_hyst(struct device *dev, const char *buf,
+ size_t count, int nr) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+ data->temp_hyst[nr] = TEMP_TO_REG(val);
+ vt8231_write_value(client, VT8231_REG_TEMP_HYST(nr),
+ data->temp_hyst[nr]);
+ return count;
+}
+#define show_temp_offset(offset) \
+static ssize_t show_temp_##offset (struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ return show_temp(dev, buf, offset - 1); \
+} \
+static ssize_t show_temp_##offset##_over (struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ return show_temp_over(dev, buf, offset - 1); \
+} \
+static ssize_t show_temp_##offset##_hyst (struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ return show_temp_hyst(dev, buf, offset - 1); \
+} \
+static ssize_t set_temp_##offset##_over (struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_temp_over(dev, buf, count, offset - 1); \
+} \
+static ssize_t set_temp_##offset##_hyst (struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_temp_hyst(dev, buf, count, offset - 1); \
+} \
+static DEVICE_ATTR(temp##offset##_input, S_IRUGO, show_temp_##offset, NULL);\
+static DEVICE_ATTR(temp##offset##_max, S_IRUGO | S_IWUSR, \
+ show_temp_##offset##_over, set_temp_##offset##_over); \
+static DEVICE_ATTR(temp##offset##_max_hyst, S_IRUGO | S_IWUSR, \
+ show_temp_##offset##_hyst, set_temp_##offset##_hyst);
+
+
+/* show_temp_offset(1); */
+show_temp_offset(2);
+show_temp_offset(3);
+show_temp_offset(4);
+show_temp_offset(5);
+show_temp_offset(6);
+show_temp_offset(7);
+
+/* 2 Fans */
+static ssize_t show_fan(struct device *dev, char *buf, int nr) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[nr],
+ DIV_FROM_REG(data->fan_div[nr])));
+}
+static ssize_t show_fan_min(struct device *dev, char *buf, int nr) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n",
+ FAN_FROM_REG(data->fan_min[nr],
+ DIV_FROM_REG(data->fan_div[nr])));
+}
+static ssize_t show_fan_div(struct device *dev, char *buf, int nr) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n", DIV_FROM_REG(data->fan_div[nr]));
+}
+static ssize_t set_fan_min(struct device *dev, const char *buf,
+ size_t count, int nr) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int val = simple_strtoul(buf, NULL, 10);
+ data->fan_min[nr] = FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr]));
+ vt8231_write_value(client, VT8231_REG_FAN_MIN(nr+1), data->fan_min[nr]);
+ return count;
+}
+static ssize_t set_fan_div(struct device *dev, const char *buf,
+ size_t count, int nr) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int val = simple_strtoul(buf, NULL, 10);
+ int old = vt8231_read_value(client, VT8231_REG_FANDIV);
+ data->fan_div[nr] = DIV_TO_REG(val);
+ old = (old & 0x0f) | (data->fan_div[1] << 6) | (data->fan_div[0] << 4);
+ vt8231_write_value(client, VT8231_REG_FANDIV, old);
+ return count;
+}
+
+#define show_fan_offset(offset) \
+static ssize_t show_fan_##offset (struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ return show_fan(dev, buf, offset - 1); \
+} \
+static ssize_t show_fan_##offset##_min (struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ return show_fan_min(dev, buf, offset - 1); \
+} \
+static ssize_t show_fan_##offset##_div (struct device *dev, \
+ struct device_attribute *attr, char *buf) \
+{ \
+ return show_fan_div(dev, buf, offset - 1); \
+} \
+static ssize_t set_fan_##offset##_min (struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_fan_min(dev, buf, count, offset - 1); \
+} \
+static ssize_t set_fan_##offset##_div (struct device *dev, \
+ struct device_attribute *attr, \
+ const char *buf, size_t count) \
+{ \
+ return set_fan_div(dev, buf, count, offset - 1); \
+} \
+static DEVICE_ATTR(fan##offset##_input, S_IRUGO, show_fan_##offset, NULL);\
+static DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR, \
+ show_fan_##offset##_min, set_fan_##offset##_min); \
+static DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR, \
+ show_fan_##offset##_div, set_fan_##offset##_div);
+
+show_fan_offset(1);
+show_fan_offset(2);
+
+/* Alarms */
+static ssize_t show_alarms(struct device *dev, struct device_attribute *attr,
+ char *buf) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n", ALARMS_FROM_REG(data->alarms));
+}
+static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
+
+
+/**** VRM and VID *****/
+static ssize_t show_vid(struct device *dev, struct device_attribute *attr,
+ char *buf) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%ld\n", (long)vid_from_reg(data->cpu0_vid, data->vrm));
+}
+static DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid, NULL);
+
+static ssize_t show_vrm(struct device *dev, struct device_attribute *attr,
+ char *buf) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%ld\n", (long)data->vrm);
+}
+static ssize_t set_vrm(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+ printk("Setting VRM to \"%s\" = %d\n", buf, val);
+ data->vrm = val;
+ return count;
+}
+static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm, set_vrm);
+
+static struct i2c_driver vt8231_driver = {
+ .owner = THIS_MODULE,
+ .name = "vt8231",
+ .id = I2C_DRIVERID_VT8231,
+ .flags = I2C_DF_NOTIFY,
+ .attach_adapter = vt8231_detect,
+ .detach_client = vt8231_detach_client,
+};
+
+int vt8231_detect(struct i2c_adapter *adapter)
+{
+ struct i2c_client *new_client;
+ struct vt8231_data *data;
+ int err = 0;
+ const char type_name[] = "vt8231";
+ u16 val;
+
+ /* 8231 requires multiple of 256 */
+ if (force_addr)
+ isa_address = force_addr & 0xFF00;
+
+ if (force_addr) {
+ dev_warn(&adapter->dev, "forcing ISA address 0x%04X\n",
+ isa_address);
+ if (PCIBIOS_SUCCESSFUL != pci_write_config_word(s_bridge,
+ VT8231_BASE_REG, isa_address))
+ return -ENODEV;
+ }
+ if (PCIBIOS_SUCCESSFUL !+ pci_read_config_word(s_bridge, VT8231_ENABLE_REG, &val))
+ return -ENODEV;
+ if (!(val & 0x0001)) {
+ dev_warn(&adapter->dev, "enabling sensors\n");
+ if (PCIBIOS_SUCCESSFUL !+ pci_write_config_word(s_bridge, VT8231_ENABLE_REG,
+ val | 0x0001))
+ return -ENODEV;
+ }
+
+ /* Reserve the ISA region */
+ if (!request_region(isa_address, VT8231_EXTENT, type_name)) {
+ dev_err(&adapter->dev, "region 0x%x already in use!\n",
+ isa_address);
+ return -ENODEV;
+ }
+
+ if (!(data = kzalloc(sizeof(struct vt8231_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto exit_release;
+ }
+
+ new_client = &data->client;
+ i2c_set_clientdata(new_client, data);
+ new_client->addr = isa_address;
+ new_client->adapter = adapter;
+ new_client->driver = &vt8231_driver;
+ new_client->flags = 0;
+ new_client->dev.parent = &adapter->dev;
+
+ /* Fill in the remaining client fields and put into the global list */
+ strlcpy(new_client->name, type_name, I2C_NAME_SIZE);
+
+ data->valid = 0;
+ init_MUTEX(&data->update_lock);
+
+ /* Tell the I2C layer a new client has arrived */
+ if ((err = i2c_attach_client(new_client)))
+ goto exit_free;
+
+ vt8231_init_client(new_client);
+
+ /* Register sysfs hooks */
+ data->class_dev = hwmon_device_register(&new_client->dev);
+ if (IS_ERR(data->class_dev)) {
+ err = PTR_ERR(data->class_dev);
+ goto exit_detach;
+ }
+
+ device_create_file(&new_client->dev, &dev_attr_in0_input);
+ device_create_file(&new_client->dev, &dev_attr_in1_input);
+ device_create_file(&new_client->dev, &dev_attr_in2_input);
+ device_create_file(&new_client->dev, &dev_attr_in3_input);
+ device_create_file(&new_client->dev, &dev_attr_in4_input);
+ device_create_file(&new_client->dev, &dev_attr_in5_input);
+ device_create_file(&new_client->dev, &dev_attr_in0_min);
+ device_create_file(&new_client->dev, &dev_attr_in1_min);
+ device_create_file(&new_client->dev, &dev_attr_in2_min);
+ device_create_file(&new_client->dev, &dev_attr_in3_min);
+ device_create_file(&new_client->dev, &dev_attr_in4_min);
+ device_create_file(&new_client->dev, &dev_attr_in5_min);
+ device_create_file(&new_client->dev, &dev_attr_in0_max);
+ device_create_file(&new_client->dev, &dev_attr_in1_max);
+ device_create_file(&new_client->dev, &dev_attr_in2_max);
+ device_create_file(&new_client->dev, &dev_attr_in3_max);
+ device_create_file(&new_client->dev, &dev_attr_in4_max);
+ device_create_file(&new_client->dev, &dev_attr_in5_max);
+
+ device_create_file(&new_client->dev, &dev_attr_temp2_input);
+ device_create_file(&new_client->dev, &dev_attr_temp3_input);
+ device_create_file(&new_client->dev, &dev_attr_temp4_input);
+ device_create_file(&new_client->dev, &dev_attr_temp5_input);
+ device_create_file(&new_client->dev, &dev_attr_temp6_input);
+ device_create_file(&new_client->dev, &dev_attr_temp7_input);
+ device_create_file(&new_client->dev, &dev_attr_temp2_max);
+ device_create_file(&new_client->dev, &dev_attr_temp3_max);
+ device_create_file(&new_client->dev, &dev_attr_temp4_max);
+ device_create_file(&new_client->dev, &dev_attr_temp5_max);
+ device_create_file(&new_client->dev, &dev_attr_temp6_max);
+ device_create_file(&new_client->dev, &dev_attr_temp7_max);
+ device_create_file(&new_client->dev, &dev_attr_temp2_max_hyst);
+ device_create_file(&new_client->dev, &dev_attr_temp3_max_hyst);
+ device_create_file(&new_client->dev, &dev_attr_temp4_max_hyst);
+ device_create_file(&new_client->dev, &dev_attr_temp5_max_hyst);
+ device_create_file(&new_client->dev, &dev_attr_temp6_max_hyst);
+ device_create_file(&new_client->dev, &dev_attr_temp7_max_hyst);
+
+ device_create_file(&new_client->dev, &dev_attr_fan1_input);
+ device_create_file(&new_client->dev, &dev_attr_fan2_input);
+ device_create_file(&new_client->dev, &dev_attr_fan1_min);
+ device_create_file(&new_client->dev, &dev_attr_fan2_min);
+ device_create_file(&new_client->dev, &dev_attr_fan1_div);
+ device_create_file(&new_client->dev, &dev_attr_fan2_div);
+
+ device_create_file(&new_client->dev, &dev_attr_alarms);
+
+ device_create_file(&new_client->dev, &dev_attr_cpu0_vid);
+ device_create_file(&new_client->dev, &dev_attr_vrm);
+
+ return 0;
+
+exit_detach:
+ i2c_detach_client(new_client);
+exit_free:
+ kfree(data);
+exit_release:
+ release_region(isa_address, VT8231_EXTENT);
+ return err;
+}
+
+static int vt8231_detach_client(struct i2c_client *client)
+{
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int err;
+
+ hwmon_device_unregister(data->class_dev);
+
+ if ((err = i2c_detach_client(client))) {
+ dev_err(&client->dev,
+ "Client deregistration failed, client not detached.\n");
+ return err;
+ }
+
+ release_region(client->addr, VT8231_EXTENT);
+ kfree(i2c_get_clientdata(client));
+
+ return 0;
+}
+
+static void vt8231_init_client(struct i2c_client *client)
+{
+ struct vt8231_data *data = i2c_get_clientdata(client);
+
+ data->vrm = vid_which_vrm();
+
+ /* set "default" interrupt mode for alarms, which isn't the default */
+ vt8231_write_value(client, VT8231_REG_TEMP1_CONFIG, 0);
+ vt8231_write_value(client, VT8231_REG_TEMP2_CONFIG, 0);
+}
+
+static struct vt8231_data *vt8231_update_device(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int i, j;
+
+ down(&data->update_lock);
+
+ if ((jiffies - data->last_updated > HZ + HZ / 2) ||
+ (jiffies < data->last_updated) || !data->valid) {
+ data->uch_config = vt8231_read_value(client,
+ VT8231_REG_UCH_CONFIG);
+ for (i = 0; i <= 5; i++) {
+ if(ISVOLT(i, data->uch_config)) {
+ data->in[i] = vt8231_read_value(client,
+ VT8231_REG_IN(i));
+ data->in_min[i] = vt8231_read_value(client,
+ VT8231_REG_IN_MIN(i));
+ data->in_max[i] = vt8231_read_value(client,
+ VT8231_REG_IN_MAX(i));
+ } else {
+ data->in[i] = 0;
+ data->in_min[i] = 0;
+ data->in_max[i] = 0;
+ }
+ }
+ for (i = 1; i <= 2; i++) {
+ data->fan[i - 1] = vt8231_read_value(client,
+ VT8231_REG_FAN(i));
+ data->fan_min[i - 1] = vt8231_read_value(client,
+ VT8231_REG_FAN_MIN(i));
+ }
+ for (i = 2; i <= 7; i++) {
+ if(ISTEMP(i, data->uch_config)) {
+ data->temp[i - 1] = vt8231_read_value(client,
+ VT8231_REG_TEMP(i)) << 2;
+ switch(i) {
+ case 1:
+ /* ? */
+ j = 0;
+ break;
+ case 2:
+ j = (vt8231_read_value(client,
+ VT8231_REG_TEMP_LOW2) &
+ 0x30) >> 4;
+ break;
+ case 3:
+ j = (vt8231_read_value(client,
+ VT8231_REG_TEMP_LOW3) &
+ 0xc0) >> 6;
+ break;
+ case 4:
+ case 5:
+ case 6:
+ case 7:
+ default:
+ j = (vt8231_read_value(client,
+ VT8231_REG_TEMP_LOW47)
+ >> ((i-4)*2)) & 0x03;
+ break;
+
+ }
+ data->temp[i - 1] |= j;
+ data->temp_over[i - 1] =
+ vt8231_read_value(client,
+ VT8231_REG_TEMP_OVER(i));
+ data->temp_hyst[i - 1] =
+ vt8231_read_value(client,
+ VT8231_REG_TEMP_HYST(i));
+ } else {
+ data->temp[i - 1] = 0;
+ data->temp_over[i - 1] = 0;
+ data->temp_hyst[i - 1] = 0;
+ }
+ }
+
+ for (i = 1; i <= 2; i++) {
+ data->fan[i - 1] = vt8231_read_value(client,
+ VT8231_REG_FAN(i));
+ data->fan_min[i - 1] = vt8231_read_value(client,
+ VT8231_REG_FAN_MIN(i));
+ data->pwm[i - 1] = vt8231_read_value(client,
+ VT8231_REG_PWM(i));
+ }
+
+ data->pwm_ctl = vt8231_read_value(client, VT8231_REG_PWM_CTL);
+ i = vt8231_read_value(client, VT8231_REG_FANDIV);
+ data->fan_div[0] = (i >> 4) & 0x03;
+ data->fan_div[1] = i >> 6;
+ data->alarms = vt8231_read_value(client, VT8231_REG_ALARM1) |
+ (vt8231_read_value(client, VT8231_REG_ALARM2) << 8);
+ data->cpu0_vid= vt8231_read_value(client, VT8231_REG_VID)
+ & 0x1f;
+ data->last_updated = jiffies;
+ data->valid = 1;
+ }
+
+ up(&data->update_lock);
+
+ return data;
+}
+
+static struct pci_device_id vt8231_pci_ids[] = {
+ {
+ .vendor = PCI_VENDOR_ID_VIA,
+ .device = PCI_DEVICE_ID_VIA_8231_4,
+ .subvendor = PCI_ANY_ID,
+ .subdevice = PCI_ANY_ID,
+ },
+ { 0, }
+};
+
+static int __devinit vt8231_pci_probe(struct pci_dev *dev,
+ const struct pci_device_id *id)
+{
+ u16 val;
+
+ if (PCIBIOS_SUCCESSFUL != pci_read_config_word(dev, VT8231_BASE_REG,
+ &val))
+ return -ENODEV;
+
+ isa_address = val & ~(VT8231_EXTENT - 1);
+ if (isa_address = 0 && force_addr = 0) {
+ dev_err(&dev->dev, "base address not set -\
+ upgrade BIOS or use force_addr=0xaddr\n");
+ return -ENODEV;
+ }
+ if (force_addr)
+ isa_address = force_addr; /* so detect will get called */
+
+ if (!isa_address) {
+ dev_err(&dev->dev, "No Via 8231 sensors found.\n");
+ return -ENODEV;
+ }
+ s_bridge = pci_dev_get(dev);
+
+ if (i2c_isa_add_driver(&vt8231_driver)) {
+ pci_dev_put(s_bridge);
+ s_bridge = NULL;
+ }
+
+ /* Always return failure here. This is to allow other drivers to bind
+ * to this pci device. We don't really want to have control over the
+ * pci device, we only wanted to read as few register values from it.
+ */
+ return -ENODEV;
+}
+
+static struct pci_driver vt8231_pci_driver = {
+ .name = "vt8231",
+ .id_table = vt8231_pci_ids,
+ .probe = vt8231_pci_probe,
+};
+
+
+static int __init sm_vt8231_init(void)
+{
+ return pci_module_init(&vt8231_pci_driver);
+}
+
+static void __exit sm_vt8231_exit(void)
+{
+ pci_unregister_driver(&vt8231_pci_driver);
+ if (s_bridge != NULL) {
+ i2c_isa_del_driver(&vt8231_driver);
+ pci_dev_put(s_bridge);
+ s_bridge = NULL;
+ }
+}
+
+
+MODULE_AUTHOR("Mark D. Studebaker <mdsxyz123@yahoo.com>");
+MODULE_DESCRIPTION("VT8231 sensors");
+MODULE_LICENSE("GPL");
+
+module_init(sm_vt8231_init);
+module_exit(sm_vt8231_exit);
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
2005-11-02 14:24 ` Jean Delvare
2005-11-02 15:17 ` Roger Lucas
@ 2005-11-02 19:47 ` Knut Petersen
2005-11-02 20:15 ` Roger Lucas
` (17 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Knut Petersen @ 2005-11-02 19:47 UTC (permalink / raw)
To: lm-sensors
Roger Lucas schrieb:
>Hi Jean,
>
>Updated patch attached. I love it when you inherit loads of code that you
>end up fixing... :-)
>
>
One problem: As you know, I use a Verax fan with 9 pulses/revolution.
If i use
compute fan1 @/9 , @*9
set fan1_min 1
set fan1_div 1
in sensors.conf, the shortened sensors output is:
fan1: 1324 RPM (min = 571 RPM, div = 1)
and /sys/bus/i2c/devices/9191-6000/fan1_min contains 5140.
But if I use
compute fan1 @/9 , @*9
set fan1_min 0
set fan1_div 1
that results in a sensors output of
fan1: 1324 RPM (min = 0 RPM, div = 1) ALARM
and /sys/bus/i2c/devices/9191-6000/fan1_min contains 0.
The previous patch also behaved wrong for a "set fan1_min 0".
cu,
Knut
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (2 preceding siblings ...)
2005-11-02 19:47 ` Knut Petersen
@ 2005-11-02 20:15 ` Roger Lucas
2005-11-02 20:26 ` Jean Delvare
` (16 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Roger Lucas @ 2005-11-02 20:15 UTC (permalink / raw)
To: lm-sensors
Hi Knut,
I'm not sure exactly how the VT8251 generates its alarms. I'll have to look
into the device spec and see exactly what it does then get back to you.
- Roger
-----Original Message-----
From: Knut Petersen [mailto:Knut_Petersen@t-online.de]
Sent: 02 November 2005 18:48
To: Roger Lucas
Cc: 'Jean Delvare'; 'LM Sensors'
Subject: Re: [lm-sensors] RE: vt8231.c
Roger Lucas schrieb:
>Hi Jean,
>
>Updated patch attached. I love it when you inherit loads of code that you
>end up fixing... :-)
>
>
One problem: As you know, I use a Verax fan with 9 pulses/revolution.
If i use
compute fan1 @/9 , @*9
set fan1_min 1
set fan1_div 1
in sensors.conf, the shortened sensors output is:
fan1: 1324 RPM (min = 571 RPM, div = 1)
and /sys/bus/i2c/devices/9191-6000/fan1_min contains 5140.
But if I use
compute fan1 @/9 , @*9
set fan1_min 0
set fan1_div 1
that results in a sensors output of
fan1: 1324 RPM (min = 0 RPM, div = 1) ALARM
and /sys/bus/i2c/devices/9191-6000/fan1_min contains 0.
The previous patch also behaved wrong for a "set fan1_min 0".
cu,
Knut
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (3 preceding siblings ...)
2005-11-02 20:15 ` Roger Lucas
@ 2005-11-02 20:26 ` Jean Delvare
2005-11-02 21:50 ` Grant Coady
` (15 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Jean Delvare @ 2005-11-02 20:26 UTC (permalink / raw)
To: lm-sensors
BTW, Knut...
> Fans:
> ==
> I connected a fan for tests. It?s a Verax fan with 9 pulses / revolution.
>
> In sensors.conf I used the four dividers 1,2,4,9 instead of the *:
>
> set fan1_div *
> set fan1_min 1000
>
> Here are the results
>
> 1: fan1: 12024 RPM (min = 0 RPM, div = 1)
> 2: fan1: 11702 RPM (min = 0 RPM, div = 2)
> 4: fan1: 11702 RPM (min = 0 RPM, div = 4)
> 8: fan1: 12603 RPM (min = 999 RPM, div = 8)
> 9: fan1: 11915 RPM (min = 0 RPM, div = 2)
>
> Obviously the fan1_min are ignored most of the time and dividers are not
> handled correctly.
Dividers work the way they are supposed to. You do not seem to
understand what that way is. I invite you to read the following document:
http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/doc/fan-divisors
Hopefully this will help you understand how it works.
If you use a 9 pulse/rev fan (didn't know these existed...) instead of
standard 2 pulse/rev, you should add the following line to your
configuration file:
conpute fan1 @*2/9, @*9/2
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (4 preceding siblings ...)
2005-11-02 20:26 ` Jean Delvare
@ 2005-11-02 21:50 ` Grant Coady
2005-11-02 21:56 ` Jean Delvare
` (14 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Grant Coady @ 2005-11-02 21:50 UTC (permalink / raw)
To: lm-sensors
Jean Delvare wrote:
> BTW, Knut...
>
>
>>Fans:
>>==
>>I connected a fan for tests. It?s a Verax fan with 9 pulses / revolution.
>>
>>In sensors.conf I used the four dividers 1,2,4,9 instead of the *:
>>
>> set fan1_div *
>> set fan1_min 1000
>>
>>Here are the results
>>
>>1: fan1: 12024 RPM (min = 0 RPM, div = 1)
>>2: fan1: 11702 RPM (min = 0 RPM, div = 2)
>>4: fan1: 11702 RPM (min = 0 RPM, div = 4)
>>8: fan1: 12603 RPM (min = 999 RPM, div = 8)
>>9: fan1: 11915 RPM (min = 0 RPM, div = 2)
>>
>>Obviously the fan1_min are ignored most of the time and dividers are not
>>handled correctly.
>
>
> Dividers work the way they are supposed to. You do not seem to
> understand what that way is. I invite you to read the following document:
> http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/doc/fan-divisors
>
> Hopefully this will help you understand how it works.
>
> If you use a 9 pulse/rev fan (didn't know these existed...) instead of
> standard 2 pulse/rev, you should add the following line to your
> configuration file:
>
> conpute fan1 @*2/9, @*9/2
Possibly:
compute fan1 @*8/9, @*9/8
since fan1_div = 8 is only one gave a result for fan1_min?
Grant.
>
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (5 preceding siblings ...)
2005-11-02 21:50 ` Grant Coady
@ 2005-11-02 21:56 ` Jean Delvare
2005-11-03 0:28 ` Roger Lucas
` (13 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Jean Delvare @ 2005-11-02 21:56 UTC (permalink / raw)
To: lm-sensors
Hi Roger, Knut,
> > But if I use
> >
> > compute fan1 @/9 , @*9
> > set fan1_min 0
> > set fan1_div 1
> >
> > that results in a sensors output of
> >
> > fan1: 1324 RPM (min = 0 RPM, div = 1) ALARM
>
> I'm not sure exactly how the VT8231 generates its alarms. I'll have to look
> into the device spec and see exactly what it does then get back to you.
This means that my suggestion of changing FAN_FROM_REG earlier today
was probably not correct, and FAN_TO_REG should be changed instead:
static inline u8 FAN_TO_REG(long rpm, int div)
{
if (rpm = 0)
return 255;
return SENSORS_LIMIT(1310720 / (rpm * div), 1, 254);
}
static inline long FAN_FROM_REG(u8 reg, int div)
{
if (reg = 0 || reg = 255)
return 0;
return (1310720 / (reg * div));
}
Ideally we should probably handle the fan speed measurement registers
and the low limit registers differently, as 255 means no low limit (0
RPM) but is still a valid speed measurement. However, I am not certain
the benefit is worth the additional code.
Roger: I am in the process of reviewing all your code. This takes some
time so please be patient. I couldn't finish the review today,
hopefully it will be OK tomorrow.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (6 preceding siblings ...)
2005-11-02 21:56 ` Jean Delvare
@ 2005-11-03 0:28 ` Roger Lucas
2005-11-03 4:26 ` Mark M. Hoffman
` (12 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Roger Lucas @ 2005-11-03 0:28 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
No problem. I appreciate the effort you are spending on the code review.
- Roger
-----Original Message-----
From: Jean Delvare [mailto:khali@linux-fr.org]
Sent: 02 November 2005 20:56
To: Roger Lucas
Cc: Knut_Petersen@t-online.de; LM Sensors
Subject: Re: [lm-sensors] RE: vt8231.c
Hi Roger, Knut,
> > But if I use
> >
> > compute fan1 @/9 , @*9
> > set fan1_min 0
> > set fan1_div 1
> >
> > that results in a sensors output of
> >
> > fan1: 1324 RPM (min = 0 RPM, div = 1) ALARM
>
> I'm not sure exactly how the VT8231 generates its alarms. I'll have to
look
> into the device spec and see exactly what it does then get back to you.
This means that my suggestion of changing FAN_FROM_REG earlier today
was probably not correct, and FAN_TO_REG should be changed instead:
static inline u8 FAN_TO_REG(long rpm, int div)
{
if (rpm = 0)
return 255;
return SENSORS_LIMIT(1310720 / (rpm * div), 1, 254);
}
static inline long FAN_FROM_REG(u8 reg, int div)
{
if (reg = 0 || reg = 255)
return 0;
return (1310720 / (reg * div));
}
Ideally we should probably handle the fan speed measurement registers
and the low limit registers differently, as 255 means no low limit (0
RPM) but is still a valid speed measurement. However, I am not certain
the benefit is worth the additional code.
Roger: I am in the process of reviewing all your code. This takes some
time so please be patient. I couldn't finish the review today,
hopefully it will be OK tomorrow.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (7 preceding siblings ...)
2005-11-03 0:28 ` Roger Lucas
@ 2005-11-03 4:26 ` Mark M. Hoffman
2005-11-03 6:11 ` Grant Coady
` (11 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Mark M. Hoffman @ 2005-11-03 4:26 UTC (permalink / raw)
To: lm-sensors
Hi Grant:
> Jean Delvare wrote:
> > Dividers work the way they are supposed to. You do not seem to
> > understand what that way is. I invite you to read the following document:
> > http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/doc/fan-divisors
> >
> > Hopefully this will help you understand how it works.
> >
> > If you use a 9 pulse/rev fan (didn't know these existed...) instead of
> > standard 2 pulse/rev, you should add the following line to your
> > configuration file:
> >
> > conpute fan1 @*2/9, @*9/2
* Grant Coady <gcoady@gmail.com> [2005-11-03 07:49:19 +1100]:
> Possibly:
>
> compute fan1 @*8/9, @*9/8
>
> since fan1_div = 8 is only one gave a result for fan1_min?
No, no, no... fan divisors are *not* related to sensors.conf compute formulas!
You must have read the document Jean mentioned... didn't you work on automagic
fan divisor algorithms for a while?
Who are you? And what have you done with Grant? ;)
Regards,
--
Mark M. Hoffman
mhoffman@lightlink.com
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (8 preceding siblings ...)
2005-11-03 4:26 ` Mark M. Hoffman
@ 2005-11-03 6:11 ` Grant Coady
2005-11-06 17:00 ` Jean Delvare
` (10 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Grant Coady @ 2005-11-03 6:11 UTC (permalink / raw)
To: lm-sensors
Mark M. Hoffman wrote:
> Hi Grant:
...
> No, no, no... fan divisors are *not* related to sensors.conf compute formulas!
>
> You must have read the document Jean mentioned... didn't you work on automagic
> fan divisor algorithms for a while?
>
> Who are you? And what have you done with Grant? ;)
What can I say? It was before coffee this morning :o)
Cheers,
Grant.
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (9 preceding siblings ...)
2005-11-03 6:11 ` Grant Coady
@ 2005-11-06 17:00 ` Jean Delvare
2005-11-06 20:22 ` Roger Lucas
` (9 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Jean Delvare @ 2005-11-06 17:00 UTC (permalink / raw)
To: lm-sensors
Hi Roger,
> Updated patch attached. I love it when you inherit loads of code that you
> end up fixing... :-)
I finished reviewing your code (at last! sorry for the delay), here we go.
First of all, please delete all trailing whitespace in vt8231.c. There
are a lot of these.
And a generic comment: I *know* that some of the problems here are
not yours, they come from the original driver. Don't take it
personally ;) but we need to fix them all before the driver can
be accepted in Linux 2.6.
> diff -Naur reference/drivers/hwmon/Kconfig linux-2.6.14/drivers/hwmon/Kconfig
> --- reference/drivers/hwmon/Kconfig 2005-10-28 01:02:08.000000000 +0100
> +++ linux-2.6.14/drivers/hwmon/Kconfig 2005-11-01 17:35:18.000000000 +0000
> @@ -350,6 +350,18 @@
> This driver can also be built as a module. If so, the module
> will be called via686a.
>
> +config SENSORS_VT8231
> + tristate "VT8231"
> + depends on HWMON && I2C && PCI && EXPERIMENTAL
> + select HWMON_VID
> + select I2C_ISA
> + help
> + If you say yes here then you get support for the integrated sensors
> + in the Via VT8231 device.
VIA should never be spelled Via.
> diff -Naur reference/drivers/hwmon/vt8231.c linux-2.6.14/drivers/hwmon/vt8231.c
> --- reference/drivers/hwmon/vt8231.c 1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.14/drivers/hwmon/vt8231.c 2005-11-01 18:06:17.000000000 +0000
> @@ -0,0 +1,785 @@
> +static int force_addr;
> +module_param(force_addr, int,0644);
> +MODULE_PARM_DESC(force_addr, "Initialize the base address of the sensors");
The "0644" doesn't make sense. It means that a sysfs file will be created
for this parameter. Given that the parameter is only needed at
initialization time, the created file will be of no use. Use "0" to
not create a file.
Also, missing space before comma.
> +/* pwm numbered 1-2 */
> +#define VT8231_REG_PWM(nr) (0x5f + (nr))
> +#define VT8231_REG_PWM_CTL 0x51
> +
> +/* The VT8231 registers */
You could move the PWM register definitions below, else that comment
is somewhat confusing.
> +/* We define the sensors as follows. Somewhat convoluted to minimize
> + changes from via686a.
> + Sensor Voltage Mode Temp Mode
> + -------- ------------ ---------
> + Reading 1 temp3
> + Reading 3 temp1 not in vt8231
> + UCH1/Reading2 in0 temp2
> + UCH2 in1 temp4
> + UCH3 in2 temp5
> + UCH4 in3 temp6
> + UCH5 in4 temp7
> + 3.3V in5
> + -12V in6 not in vt8231
> +*/
What is the benefit in mentioning the inputs which the VT8231 does not
have?
It's a bit confusing (and non-standard) to have temperatures starting
at temp2 :(
> +static const u8 regtemp[] = { 0x20, 0x21, 0x1f, 0x22, 0x23, 0x24, 0x25 };
> +static const u8 regover[] = { 0x39, 0x3d, 0x1d, 0x2b, 0x2d, 0x2f, 0x31 };
> +static const u8 reghyst[] = { 0x3a, 0x3e, 0x1e, 0x2c, 0x2e, 0x30, 0x32 };
> +
> +/* temps numbered 1-7 */
So that would be 2-7. This suggests that the first "column" in the above
arrays is never used, so it should be deleted.
> +#define ALARMS_FROM_REG(val) (val)
Please discard that macro and use the value directly instead.
> +#define DIV_TO_REG(val) ((val)=8?3:(val)=4?2:(val)=1?0:1)
In Linux 2.6 we have changed to a different strategy. Instead of
defaulting to an arbitrary divider value when an unsupported value
is requested, we report the error to userspace. See the various other
Linux 2.6 drivers for examples.
> +#define PWM_FROM_REG(val) (val)
You can drop that one too.
> +#define PWM_TO_REG(val) SENSORS_LIMIT((val), 0, 255)
For this one too, it is advisable to return an error on invalid
values. See it87.c for an example.
> +#define TEMP_FROM_REG(val) (long)((val)*10)
> +#define TEMP_FROM_REG10(val) (long)(((val)*10)/4)
> +#define TEMP_TO_REG(val) (SENSORS_LIMIT(((val)<0?(((val)-5)/10):\
> + ((val)+5)/10), 0, 255))
These do not look correct to me. They suggest that you are exporting
the values to userspace with LSB = 0.1 degree C. The standard is 0.001
degree C, as documented in Documentation/hwmon/sysfs-interface. I am
surprised that you got correct temperature readings, unless you also
hacked libsensors.
Looking further in sensors.conf.eg, it looks like the VT8231 expresses
each temperature as a voltage, which libsensors then converts to a
temperature. This is a rare, but not unique, case. The pc87360
driver does something similar for temp4-6. In this case, the value
must be exported in sysfs with LSB = 1 mV, as voltages do.
Even then, the formulas in sensors.conf.eg look very complex to me.
We will have to look into that and make some sense out of them
> +#define IN_FROM_REG(val) ((long)(val))
> +#define IN_TO_REG(val) (SENSORS_LIMIT((((val) * 10 + 5)/10), 0, 255))
Voltage values must be exported to userspace with LSB = 1 mV, this
is a standard. This doesn't seem to be the case here. IN_TO_REG is also
nonsensical: you multiply by 10, then immediately divide the result by
10?
Seeing the conversion formulas in sensors.conf.eg, I think I understand
that the VT8231 differs much from the other chips. Most of the conversion
formula (the common part) should be moved inside the driver if we want
to respect the sysfs standard. This will break the compatibility between
the 2.4 and the 2.6 vt8231 drivers, but we don't seem to have the choice.
> +static inline int vt8231_read_value(struct i2c_client *client, u8 reg)
> +{
> + return (inb_p(client->addr + reg));
> +}
Please drop the redundant parentheses.
> +/* following are the sysfs callback functions */
Generic comment about sysfs callback functions:
A new possibility, known as "dynamic sysfs callbacks", was recently
introduced. Not all hardware monitoring drivers use it yet, but I would
like new drivers to do, so that we don't have to convert them later.
These dynamic callbacks let you get rid of a large part of the macros
above, by giving you the possibility to pass an integer parameter to
the callback functions, making it possible to use the same callback
function for several sysfs files.
See the it87 driver for an example. If would be very great if you could
convert the vt8231 driver to do the same. This greatly improves the code
readability, and makes the compiled object significantly smaller as
well.
> +static ssize_t set_in_min(struct device *dev, const char *buf,
> + size_t count, int nr) {
> + struct i2c_client *client = to_i2c_client(dev);
> + struct vt8231_data *data = i2c_get_clientdata(client);
> + unsigned long val = simple_strtoul(buf, NULL, 10);
> + data->in_min[nr] = IN_TO_REG(val);
> + vt8231_write_value(client, VT8231_REG_IN_MIN(nr),
> + data->in_min[nr]);
> + return count;
> +}
All "set" callback functions should hold data->update_lock while they
manipulate any data field. If not, you have a race condition if another
sysfs file is being read (triggering an update) while you write to this
one.
See any other hwmon files for an example, it's really easy.
> +/* show_temp_offset(1); */
Delete that line, no point in keeping comments about something which
doesn't exist.
> +/**** VRM and VID *****/
> +static ssize_t show_vid(struct device *dev, struct device_attribute *attr,
> + char *buf) {
> + struct vt8231_data *data = vt8231_update_device(dev);
> + return sprintf(buf, "%ld\n", (long)vid_from_reg(data->cpu0_vid, data->vrm));
> +}
The (long) cast is not needed, simply use %d instead of %ld.
> +static DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid, NULL);
> +
> +static ssize_t show_vrm(struct device *dev, struct device_attribute *attr,
> + char *buf) {
> + struct vt8231_data *data = vt8231_update_device(dev);
> + return sprintf(buf, "%ld\n", (long)data->vrm);
> +}
Same here.
> +static ssize_t set_vrm(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count) {
> + struct i2c_client *client = to_i2c_client(dev);
> + struct vt8231_data *data = i2c_get_clientdata(client);
> + int val = simple_strtol(buf, NULL, 10);
> + printk("Setting VRM to \"%s\" = %d\n", buf, val);
> + data->vrm = val;
> + return count;
> +}
Drop that printk, it's pretty useless.
> +int vt8231_detect(struct i2c_adapter *adapter)
> +{
> (...)
> + /* Reserve the ISA region */
> + if (!request_region(isa_address, VT8231_EXTENT, type_name)) {
> + dev_err(&adapter->dev, "region 0x%x already in use!\n",
> + isa_address);
> + return -ENODEV;
> + }
Please use vt8231_pci_driver.name to reserve the region (you'll need
to forward-declare it).
> + if (!(data = kzalloc(sizeof(struct vt8231_data), GFP_KERNEL))) {
> + err = -ENOMEM;
> + goto exit_release;
> + }
> +
> (...)
> + new_client->flags = 0;
I don't think you need to explicitely set it to 0, as kzalloc did it
for you. We'd need to fix all other drivers too.
> + /* Fill in the remaining client fields and put into the global list */
> + strlcpy(new_client->name, type_name, I2C_NAME_SIZE);
As this is now the only usage of type_name, you'd better put the string
verbatim here.
You forgot to create sysfs files for PWM?
> +static int vt8231_detach_client(struct i2c_client *client)
> +{
> (...)
> + if ((err = i2c_detach_client(client))) {
> + dev_err(&client->dev,
> + "Client deregistration failed, client not detached.\n");
> + return err;
> + }
Please drop the log message, it was refactored into i2c_detach_client
some times ago.
> + release_region(client->addr, VT8231_EXTENT);
> + kfree(i2c_get_clientdata(client));
You can user "data" instead.
> +static struct vt8231_data *vt8231_update_device(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct vt8231_data *data = i2c_get_clientdata(client);
> + int i, j;
> +
> + down(&data->update_lock);
> +
> + if ((jiffies - data->last_updated > HZ + HZ / 2) ||
> + (jiffies < data->last_updated) || !data->valid) {
Use time_after() instead of direct jiffies comparisons.
> + data->uch_config = vt8231_read_value(client,
> + VT8231_REG_UCH_CONFIG);
I now realize that there is no sysfs file to change this value.
I'm fine with that, as I believe this is the BIOS' job to properly
configure the chip. However, we should then be consistent and
modify the other parts of the driver accordingly. In particular,
the driver should not create the sysfs files for the channels which
are not in use. That way, the code below wouldn't have to repeatedly
set unused values to 0.
> + for (i = 0; i <= 5; i++) {
> + if(ISVOLT(i, data->uch_config)) {
> + data->in[i] = vt8231_read_value(client,
> + VT8231_REG_IN(i));
> + data->in_min[i] = vt8231_read_value(client,
> + VT8231_REG_IN_MIN(i));
> + data->in_max[i] = vt8231_read_value(client,
> + VT8231_REG_IN_MAX(i));
> + } else {
> + data->in[i] = 0;
> + data->in_min[i] = 0;
> + data->in_max[i] = 0;
> + }
> + }
I'd have a clear preference for "< 6" instead of <=5.
> + for (i = 2; i <= 7; i++) {
> + if(ISTEMP(i, data->uch_config)) {
> + data->temp[i - 1] = vt8231_read_value(client,
> + VT8231_REG_TEMP(i)) << 2;
> + switch(i) {
> + case 1:
> + /* ? */
> + j = 0;
> + break;
This case can obviously be dropped.
> + case 2:
> + j = (vt8231_read_value(client,
> + VT8231_REG_TEMP_LOW2) &
> + 0x30) >> 4;
> + break;
> + case 3:
> + j = (vt8231_read_value(client,
> + VT8231_REG_TEMP_LOW3) &
> + 0xc0) >> 6;
> + break;
> + case 4:
> + case 5:
> + case 6:
> + case 7:
> + default:
Either keep only default, or drop it.
> + j = (vt8231_read_value(client,
> + VT8231_REG_TEMP_LOW47)
> + >> ((i-4)*2)) & 0x03;
> + break;
> +
> + }
> (...)
> + for (i = 1; i <= 2; i++) {
> + data->fan[i - 1] = vt8231_read_value(client,
> + VT8231_REG_FAN(i));
> + data->fan_min[i - 1] = vt8231_read_value(client,
> + VT8231_REG_FAN_MIN(i));
> + data->pwm[i - 1] = vt8231_read_value(client,
> + VT8231_REG_PWM(i));
> + }
I don't get it. We already read the fan registers earlier in this
function!
> + data->cpu0_vid= vt8231_read_value(client, VT8231_REG_VID)
> + & 0x1f;
Missing space before "=" sign.
> +static struct pci_device_id vt8231_pci_ids[] = {
> + {
> + .vendor = PCI_VENDOR_ID_VIA,
> + .device = PCI_DEVICE_ID_VIA_8231_4,
> + .subvendor = PCI_ANY_ID,
> + .subdevice = PCI_ANY_ID,
> + },
> + { 0, }
> +};
Please use the PCI_DEVICE macro. Also add a call to
MODULE_DEVICE_TABLE (see via686a).
> +static int __devinit vt8231_pci_probe(struct pci_dev *dev,
> + const struct pci_device_id *id)
> +{
> (...)
> + if (force_addr)
> + isa_address = force_addr; /* so detect will get called */
No more needed, detect will get called anyway.
> + if (!isa_address) {
> + dev_err(&dev->dev, "No Via 8231 sensors found.\n");
> + return -ENODEV;
> + }
Drop that test, it was previously unneeded if you read the
code carefully, and now it is a bug.
> +static void __exit sm_vt8231_exit(void)
> +{
> + pci_unregister_driver(&vt8231_pci_driver);
> + if (s_bridge != NULL) {
> + i2c_isa_del_driver(&vt8231_driver);
> + pci_dev_put(s_bridge);
> + s_bridge = NULL;
> + }
> +}
> +
> +
Drop the second blank line.
"Other than that, it's OK" as I use to say ;)
Yeah, I know there are many many fixes needed. But nothing impossible
if you are motivated enough. And if you have questions we're here to
answer them.
I now answer your other questions:
> I am not so sure I am very keen on the intelligence for the auto-div
> calculation moving into the driver. I think this code should be in the
> sensors utility.
There was a debate long ago about this. Some people (at least Mark M.
Hoffman) advocated a user-space solution. I tend to agree in general
that putting policy into the kernel is bad, and also about the fact
that drivers must be as simple as possible. However, after checking the
facts, I came to the conclusion that doing this in the kernel was OK.
The main reason is that a user-space helper would need to know many
things which the drivers do not currently export: the list of divider
for each fan input, either the clock frequency, the register width.
Additionally, letting userspace change the divider means that the
driver needs additional code to preserve the min limit when this
happens. As I found I'd need more code to do all this than the
in-driver auto-fan-div takes, I decided for that second option.
Anyway, not all drivers do that. Those which do use slightly different
rules. It's not all settled yet. If anyone provides a userspace daemon
and modifies some drivers so that we can see how the userspace approach
would work, that's fine with me.
> On the other hand, however, the way the DIV_TO_REG() function was coded, it
> would be almost impossible for any application to determine the legal values
> for DIV without knowing the details of the device. Any illegal value would
> end up with DIV set at zero - not very helpful.
The old code defaulted to a divider of 2 actually. But see my comments
in the code above, 2.6 does things differently.
> As an alternative, this
> attached patch includes a code revision that sets DIV to the smallest legal
> DIV value that is the same as or larger than the requested DIV value.
> Hence, the mapping is as below:
>
> Requested DIV Set DIV
> 1 1
> 2 2
> 3 4
> 4 4
> 5 8
> 6 8
> 7 8
> 8 8
> 8+ 8
I prefer an explicit -EINVAL with no divider change on invalid value,
as all drivers in 2.6 now tend to do.
> From this sequence, the DIV returned is predictable and an application can
> very quickly determine the legal DIV values and make its calculations
> accordingly.
>
> With this revised code, if you ask for a low limit 1000 RPM with a divisor
> of 4, you get a low limit of 1285 RPM returned. If you ask for a low limit
> of 1000 RPM with a divisor of 5 then you get a low limit of 999 RPM returned
> with a divisor of 8.
The div returned is not predictable. Some chips have dividers up to
128. Some chips (it87) have different rules for the different channels
(fan3 only has possible dividers 2 and 8). Unless the chip explicitely
lists the possible dividers, there is no possible auto-div
implementation in user-space. We are *not* going to set random div
values and see if it works. This would generate actual register writes
and execute a lot of code. Let's not probe for something we already
have the knowledge of and simply needs to be exported.
Also, the only thing all chips share is the fact that the dividers are
powers of 2. So it's pretty pointless to try to support other inputs
from user-space. Nobody sane will try them.
> Consider the pseudo-code below:
>
> set_div = 1;
> set_rpm_div(set_div);
> set_low_limit_rpm(requested_limit_rpm);
>
> while( get_low_limit_rpm() > requested_limit_rpm )
> {
> true_div = get_rpm_div();
> if (set_div > true_div)
> {
> break; // we have exceeded the DIV range
> }
> // Try next div value
> set_div = true_div + 1;
> set_rpm_div(set_div);
> set_low_limit_rpm(requested_limit_rpm);
> }
>
> This will iterate through the valid DIV values and stop when it reaches the
> best RPM match or the highest valid DIV setting.
Sorry, I can't make any sense of this pseudo code. What part is in user
space? What part is in the driver? "set_low_limit_rpm" can mean about
anything. It can be in userspace (sensors, libsensors), it can be a
request to the driver, it can be a register write.
If you want to propose something, please explain it with words.
> I feel that this should be up in the application, however, and not in the
> driver. IMHO, the drivers should be a simple as possible.
As said above, I usually tend to agree, but in the facts, in this one
case, the code is much more simple and efficient when in the drivers.
It would probably make sense to have a helper module for all drivers
though, so as to not duplicate the code.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (10 preceding siblings ...)
2005-11-06 17:00 ` Jean Delvare
@ 2005-11-06 20:22 ` Roger Lucas
2005-11-07 10:37 ` Jean Delvare
` (8 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Roger Lucas @ 2005-11-06 20:22 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
Thanks for taking the time to review the code. There was a lot of code in
there that I did not understand and felt that I didn't need to understand as
it wasn't in areas that I was directly changing. Your comments have
clarified a lot, and any new knowledge is a good thing.
One question I have is regarding the "1 mV" calibration, and this returns me
to a general comment/question with the LM-Sensors system:
The LM-Sensors system appears to be based on what the sensors chips are on
the motherboard, rather than what the motherboard is. From the "probe and
find out what I have" concept, this is reasonable, but it does cause a LOT
of problems for users who are less educated. Many of the sensors devices
are wired up in different ways on different motherboards, with (for example)
a voltage input being used for a 12v rail monitor on one motherboard and a
5v rail monitor on another. The motherboard manufacturer is completely at
liberty to wire a device in any way they choose.
This therefore makes a sensors.conf file really a motherboard-specific
rather than a device specific file. If you use the configuration file
according to the device that you have, rather than the motherboard that you
have, then you run a very high risk of reporting nonsense to a user, or
(arguably worse) reporting information that looks correct but is actually
wrong.
Following this through, when you request that the driver report the voltage
detected in mV, I am _hoping_ that you are asking for the voltage at the pin
of the device, rather than the voltage that the device happens to be
ultimately connected to on this particular motherboard? Reporting the
voltage at the device pin is the only consistent behaviour that I can see
for a motherboard-independent device driver.
If the above makes sense, then would there be any interest in building some
kind of database of known good sensors.conf files for specific motherboards?
I would suggest that the output of "LSHW" or a similar utility is used to
obtain unique identifying details for the motherboard, and then this used to
determine the appropriate motherboard conf file. A simple web page with
known motherboards on it and their associated .conf would be a very good
start. New motherboards could then be added by people with their matching
.conf file.
Thanks,
Roger
-----Original Message-----
From: Jean Delvare [mailto:khali@linux-fr.org]
Sent: 06 November 2005 16:00
To: Roger Lucas
Cc: LM Sensors; Knut Petersen
Subject: Re: [lm-sensors] RE: vt8231.c
Hi Roger,
> Updated patch attached. I love it when you inherit loads of code that you
> end up fixing... :-)
I finished reviewing your code (at last! sorry for the delay), here we go.
First of all, please delete all trailing whitespace in vt8231.c. There
are a lot of these.
And a generic comment: I *know* that some of the problems here are
not yours, they come from the original driver. Don't take it
personally ;) but we need to fix them all before the driver can
be accepted in Linux 2.6.
> diff -Naur reference/drivers/hwmon/Kconfig
linux-2.6.14/drivers/hwmon/Kconfig
> --- reference/drivers/hwmon/Kconfig 2005-10-28 01:02:08.000000000 +0100
> +++ linux-2.6.14/drivers/hwmon/Kconfig 2005-11-01
17:35:18.000000000 +0000
> @@ -350,6 +350,18 @@
> This driver can also be built as a module. If so, the module
> will be called via686a.
>
> +config SENSORS_VT8231
> + tristate "VT8231"
> + depends on HWMON && I2C && PCI && EXPERIMENTAL
> + select HWMON_VID
> + select I2C_ISA
> + help
> + If you say yes here then you get support for the integrated
sensors
> + in the Via VT8231 device.
VIA should never be spelled Via.
> diff -Naur reference/drivers/hwmon/vt8231.c
linux-2.6.14/drivers/hwmon/vt8231.c
> --- reference/drivers/hwmon/vt8231.c 1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.14/drivers/hwmon/vt8231.c 2005-11-01
18:06:17.000000000 +0000
> @@ -0,0 +1,785 @@
> +static int force_addr;
> +module_param(force_addr, int,0644);
> +MODULE_PARM_DESC(force_addr, "Initialize the base address of the
sensors");
The "0644" doesn't make sense. It means that a sysfs file will be created
for this parameter. Given that the parameter is only needed at
initialization time, the created file will be of no use. Use "0" to
not create a file.
Also, missing space before comma.
> +/* pwm numbered 1-2 */
> +#define VT8231_REG_PWM(nr) (0x5f + (nr))
> +#define VT8231_REG_PWM_CTL 0x51
> +
> +/* The VT8231 registers */
You could move the PWM register definitions below, else that comment
is somewhat confusing.
> +/* We define the sensors as follows. Somewhat convoluted to minimize
> + changes from via686a.
> + Sensor Voltage Mode Temp Mode
> + -------- ------------ ---------
> + Reading 1 temp3
> + Reading 3 temp1 not in vt8231
> + UCH1/Reading2 in0 temp2
> + UCH2 in1 temp4
> + UCH3 in2 temp5
> + UCH4 in3 temp6
> + UCH5 in4 temp7
> + 3.3V in5
> + -12V in6 not in vt8231
> +*/
What is the benefit in mentioning the inputs which the VT8231 does not
have?
It's a bit confusing (and non-standard) to have temperatures starting
at temp2 :(
> +static const u8 regtemp[] = { 0x20, 0x21, 0x1f, 0x22, 0x23, 0x24, 0x25 };
> +static const u8 regover[] = { 0x39, 0x3d, 0x1d, 0x2b, 0x2d, 0x2f, 0x31 };
> +static const u8 reghyst[] = { 0x3a, 0x3e, 0x1e, 0x2c, 0x2e, 0x30, 0x32 };
> +
> +/* temps numbered 1-7 */
So that would be 2-7. This suggests that the first "column" in the above
arrays is never used, so it should be deleted.
> +#define ALARMS_FROM_REG(val) (val)
Please discard that macro and use the value directly instead.
> +#define DIV_TO_REG(val) ((val)=8?3:(val)=4?2:(val)=1?0:1)
In Linux 2.6 we have changed to a different strategy. Instead of
defaulting to an arbitrary divider value when an unsupported value
is requested, we report the error to userspace. See the various other
Linux 2.6 drivers for examples.
> +#define PWM_FROM_REG(val) (val)
You can drop that one too.
> +#define PWM_TO_REG(val) SENSORS_LIMIT((val), 0, 255)
For this one too, it is advisable to return an error on invalid
values. See it87.c for an example.
> +#define TEMP_FROM_REG(val) (long)((val)*10)
> +#define TEMP_FROM_REG10(val) (long)(((val)*10)/4)
> +#define TEMP_TO_REG(val) (SENSORS_LIMIT(((val)<0?(((val)-5)/10):\
> + ((val)+5)/10), 0, 255))
These do not look correct to me. They suggest that you are exporting
the values to userspace with LSB = 0.1 degree C. The standard is 0.001
degree C, as documented in Documentation/hwmon/sysfs-interface. I am
surprised that you got correct temperature readings, unless you also
hacked libsensors.
Looking further in sensors.conf.eg, it looks like the VT8231 expresses
each temperature as a voltage, which libsensors then converts to a
temperature. This is a rare, but not unique, case. The pc87360
driver does something similar for temp4-6. In this case, the value
must be exported in sysfs with LSB = 1 mV, as voltages do.
Even then, the formulas in sensors.conf.eg look very complex to me.
We will have to look into that and make some sense out of them
> +#define IN_FROM_REG(val) ((long)(val))
> +#define IN_TO_REG(val) (SENSORS_LIMIT((((val) * 10 + 5)/10), 0, 255))
Voltage values must be exported to userspace with LSB = 1 mV, this
is a standard. This doesn't seem to be the case here. IN_TO_REG is also
nonsensical: you multiply by 10, then immediately divide the result by
10?
Seeing the conversion formulas in sensors.conf.eg, I think I understand
that the VT8231 differs much from the other chips. Most of the conversion
formula (the common part) should be moved inside the driver if we want
to respect the sysfs standard. This will break the compatibility between
the 2.4 and the 2.6 vt8231 drivers, but we don't seem to have the choice.
> +static inline int vt8231_read_value(struct i2c_client *client, u8 reg)
> +{
> + return (inb_p(client->addr + reg));
> +}
Please drop the redundant parentheses.
> +/* following are the sysfs callback functions */
Generic comment about sysfs callback functions:
A new possibility, known as "dynamic sysfs callbacks", was recently
introduced. Not all hardware monitoring drivers use it yet, but I would
like new drivers to do, so that we don't have to convert them later.
These dynamic callbacks let you get rid of a large part of the macros
above, by giving you the possibility to pass an integer parameter to
the callback functions, making it possible to use the same callback
function for several sysfs files.
See the it87 driver for an example. If would be very great if you could
convert the vt8231 driver to do the same. This greatly improves the code
readability, and makes the compiled object significantly smaller as
well.
> +static ssize_t set_in_min(struct device *dev, const char *buf,
> + size_t count, int nr) {
> + struct i2c_client *client = to_i2c_client(dev);
> + struct vt8231_data *data = i2c_get_clientdata(client);
> + unsigned long val = simple_strtoul(buf, NULL, 10);
> + data->in_min[nr] = IN_TO_REG(val);
> + vt8231_write_value(client, VT8231_REG_IN_MIN(nr),
> + data->in_min[nr]);
> + return count;
> +}
All "set" callback functions should hold data->update_lock while they
manipulate any data field. If not, you have a race condition if another
sysfs file is being read (triggering an update) while you write to this
one.
See any other hwmon files for an example, it's really easy.
> +/* show_temp_offset(1); */
Delete that line, no point in keeping comments about something which
doesn't exist.
> +/**** VRM and VID *****/
> +static ssize_t show_vid(struct device *dev, struct device_attribute
*attr,
> + char *buf) {
> + struct vt8231_data *data = vt8231_update_device(dev);
> + return sprintf(buf, "%ld\n", (long)vid_from_reg(data->cpu0_vid,
data->vrm));
> +}
The (long) cast is not needed, simply use %d instead of %ld.
> +static DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid, NULL);
> +
> +static ssize_t show_vrm(struct device *dev, struct device_attribute
*attr,
> + char *buf) {
> + struct vt8231_data *data = vt8231_update_device(dev);
> + return sprintf(buf, "%ld\n", (long)data->vrm);
> +}
Same here.
> +static ssize_t set_vrm(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count) {
> + struct i2c_client *client = to_i2c_client(dev);
> + struct vt8231_data *data = i2c_get_clientdata(client);
> + int val = simple_strtol(buf, NULL, 10);
> + printk("Setting VRM to \"%s\" = %d\n", buf, val);
> + data->vrm = val;
> + return count;
> +}
Drop that printk, it's pretty useless.
> +int vt8231_detect(struct i2c_adapter *adapter)
> +{
> (...)
> + /* Reserve the ISA region */
> + if (!request_region(isa_address, VT8231_EXTENT, type_name)) {
> + dev_err(&adapter->dev, "region 0x%x already in use!\n",
> + isa_address);
> + return -ENODEV;
> + }
Please use vt8231_pci_driver.name to reserve the region (you'll need
to forward-declare it).
> + if (!(data = kzalloc(sizeof(struct vt8231_data), GFP_KERNEL))) {
> + err = -ENOMEM;
> + goto exit_release;
> + }
> +
> (...)
> + new_client->flags = 0;
I don't think you need to explicitely set it to 0, as kzalloc did it
for you. We'd need to fix all other drivers too.
> + /* Fill in the remaining client fields and put into the global list
*/
> + strlcpy(new_client->name, type_name, I2C_NAME_SIZE);
As this is now the only usage of type_name, you'd better put the string
verbatim here.
You forgot to create sysfs files for PWM?
> +static int vt8231_detach_client(struct i2c_client *client)
> +{
> (...)
> + if ((err = i2c_detach_client(client))) {
> + dev_err(&client->dev,
> + "Client deregistration failed, client not detached.\n");
> + return err;
> + }
Please drop the log message, it was refactored into i2c_detach_client
some times ago.
> + release_region(client->addr, VT8231_EXTENT);
> + kfree(i2c_get_clientdata(client));
You can user "data" instead.
> +static struct vt8231_data *vt8231_update_device(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct vt8231_data *data = i2c_get_clientdata(client);
> + int i, j;
> +
> + down(&data->update_lock);
> +
> + if ((jiffies - data->last_updated > HZ + HZ / 2) ||
> + (jiffies < data->last_updated) || !data->valid) {
Use time_after() instead of direct jiffies comparisons.
> + data->uch_config = vt8231_read_value(client,
> + VT8231_REG_UCH_CONFIG);
I now realize that there is no sysfs file to change this value.
I'm fine with that, as I believe this is the BIOS' job to properly
configure the chip. However, we should then be consistent and
modify the other parts of the driver accordingly. In particular,
the driver should not create the sysfs files for the channels which
are not in use. That way, the code below wouldn't have to repeatedly
set unused values to 0.
> + for (i = 0; i <= 5; i++) {
> + if(ISVOLT(i, data->uch_config)) {
> + data->in[i] = vt8231_read_value(client,
> + VT8231_REG_IN(i));
> + data->in_min[i] = vt8231_read_value(client,
> +
VT8231_REG_IN_MIN(i));
> + data->in_max[i] = vt8231_read_value(client,
> +
VT8231_REG_IN_MAX(i));
> + } else {
> + data->in[i] = 0;
> + data->in_min[i] = 0;
> + data->in_max[i] = 0;
> + }
> + }
I'd have a clear preference for "< 6" instead of <=5.
> + for (i = 2; i <= 7; i++) {
> + if(ISTEMP(i, data->uch_config)) {
> + data->temp[i - 1] vt8231_read_value(client,
> + VT8231_REG_TEMP(i))
<< 2;
> + switch(i) {
> + case 1:
> + /* ? */
> + j = 0;
> + break;
This case can obviously be dropped.
> + case 2:
> + j (vt8231_read_value(client,
> +
VT8231_REG_TEMP_LOW2) &
> + 0x30) >> 4;
> + break;
> + case 3:
> + j (vt8231_read_value(client,
> +
VT8231_REG_TEMP_LOW3) &
> + 0xc0) >> 6;
> + break;
> + case 4:
> + case 5:
> + case 6:
> + case 7:
> + default:
Either keep only default, or drop it.
> + j (vt8231_read_value(client,
> +
VT8231_REG_TEMP_LOW47)
> + >> ((i-4)*2)) &
0x03;
> + break;
> +
> + }
> (...)
> + for (i = 1; i <= 2; i++) {
> + data->fan[i - 1] = vt8231_read_value(client,
> + VT8231_REG_FAN(i));
> + data->fan_min[i - 1] = vt8231_read_value(client,
> + VT8231_REG_FAN_MIN(i));
> + data->pwm[i - 1] = vt8231_read_value(client,
> + VT8231_REG_PWM(i));
> + }
I don't get it. We already read the fan registers earlier in this
function!
> + data->cpu0_vid= vt8231_read_value(client, VT8231_REG_VID)
> + & 0x1f;
Missing space before "=" sign.
> +static struct pci_device_id vt8231_pci_ids[] = {
> + {
> + .vendor = PCI_VENDOR_ID_VIA,
> + .device = PCI_DEVICE_ID_VIA_8231_4,
> + .subvendor = PCI_ANY_ID,
> + .subdevice = PCI_ANY_ID,
> + },
> + { 0, }
> +};
Please use the PCI_DEVICE macro. Also add a call to
MODULE_DEVICE_TABLE (see via686a).
> +static int __devinit vt8231_pci_probe(struct pci_dev *dev,
> + const struct pci_device_id *id)
> +{
> (...)
> + if (force_addr)
> + isa_address = force_addr; /* so detect will get called */
No more needed, detect will get called anyway.
> + if (!isa_address) {
> + dev_err(&dev->dev, "No Via 8231 sensors found.\n");
> + return -ENODEV;
> + }
Drop that test, it was previously unneeded if you read the
code carefully, and now it is a bug.
> +static void __exit sm_vt8231_exit(void)
> +{
> + pci_unregister_driver(&vt8231_pci_driver);
> + if (s_bridge != NULL) {
> + i2c_isa_del_driver(&vt8231_driver);
> + pci_dev_put(s_bridge);
> + s_bridge = NULL;
> + }
> +}
> +
> +
Drop the second blank line.
"Other than that, it's OK" as I use to say ;)
Yeah, I know there are many many fixes needed. But nothing impossible
if you are motivated enough. And if you have questions we're here to
answer them.
I now answer your other questions:
> I am not so sure I am very keen on the intelligence for the auto-div
> calculation moving into the driver. I think this code should be in the
> sensors utility.
There was a debate long ago about this. Some people (at least Mark M.
Hoffman) advocated a user-space solution. I tend to agree in general
that putting policy into the kernel is bad, and also about the fact
that drivers must be as simple as possible. However, after checking the
facts, I came to the conclusion that doing this in the kernel was OK.
The main reason is that a user-space helper would need to know many
things which the drivers do not currently export: the list of divider
for each fan input, either the clock frequency, the register width.
Additionally, letting userspace change the divider means that the
driver needs additional code to preserve the min limit when this
happens. As I found I'd need more code to do all this than the
in-driver auto-fan-div takes, I decided for that second option.
Anyway, not all drivers do that. Those which do use slightly different
rules. It's not all settled yet. If anyone provides a userspace daemon
and modifies some drivers so that we can see how the userspace approach
would work, that's fine with me.
> On the other hand, however, the way the DIV_TO_REG() function was coded,
it
> would be almost impossible for any application to determine the legal
values
> for DIV without knowing the details of the device. Any illegal value
would
> end up with DIV set at zero - not very helpful.
The old code defaulted to a divider of 2 actually. But see my comments
in the code above, 2.6 does things differently.
> As an alternative, this
> attached patch includes a code revision that sets DIV to the smallest
legal
> DIV value that is the same as or larger than the requested DIV value.
> Hence, the mapping is as below:
>
> Requested DIV Set DIV
> 1 1
> 2 2
> 3 4
> 4 4
> 5 8
> 6 8
> 7 8
> 8 8
> 8+ 8
I prefer an explicit -EINVAL with no divider change on invalid value,
as all drivers in 2.6 now tend to do.
> From this sequence, the DIV returned is predictable and an application can
> very quickly determine the legal DIV values and make its calculations
> accordingly.
>
> With this revised code, if you ask for a low limit 1000 RPM with a divisor
> of 4, you get a low limit of 1285 RPM returned. If you ask for a low
limit
> of 1000 RPM with a divisor of 5 then you get a low limit of 999 RPM
returned
> with a divisor of 8.
The div returned is not predictable. Some chips have dividers up to
128. Some chips (it87) have different rules for the different channels
(fan3 only has possible dividers 2 and 8). Unless the chip explicitely
lists the possible dividers, there is no possible auto-div
implementation in user-space. We are *not* going to set random div
values and see if it works. This would generate actual register writes
and execute a lot of code. Let's not probe for something we already
have the knowledge of and simply needs to be exported.
Also, the only thing all chips share is the fact that the dividers are
powers of 2. So it's pretty pointless to try to support other inputs
from user-space. Nobody sane will try them.
> Consider the pseudo-code below:
>
> set_div = 1;
> set_rpm_div(set_div);
> set_low_limit_rpm(requested_limit_rpm);
>
> while( get_low_limit_rpm() > requested_limit_rpm )
> {
> true_div = get_rpm_div();
> if (set_div > true_div)
> {
> break; // we have exceeded the DIV range
> }
> // Try next div value
> set_div = true_div + 1;
> set_rpm_div(set_div);
> set_low_limit_rpm(requested_limit_rpm);
> }
>
> This will iterate through the valid DIV values and stop when it reaches
the
> best RPM match or the highest valid DIV setting.
Sorry, I can't make any sense of this pseudo code. What part is in user
space? What part is in the driver? "set_low_limit_rpm" can mean about
anything. It can be in userspace (sensors, libsensors), it can be a
request to the driver, it can be a register write.
If you want to propose something, please explain it with words.
> I feel that this should be up in the application, however, and not in the
> driver. IMHO, the drivers should be a simple as possible.
As said above, I usually tend to agree, but in the facts, in this one
case, the code is much more simple and efficient when in the drivers.
It would probably make sense to have a helper module for all drivers
though, so as to not duplicate the code.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (11 preceding siblings ...)
2005-11-06 20:22 ` Roger Lucas
@ 2005-11-07 10:37 ` Jean Delvare
2005-11-07 20:56 ` Roger Lucas
` (7 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Jean Delvare @ 2005-11-07 10:37 UTC (permalink / raw)
To: lm-sensors
Hi Roger,
On 2005-11-06, Roger Lucas wrote:
> One question I have is regarding the "1 mV" calibration, and this returns
> me to a general comment/question with the LM-Sensors system:
>
> The LM-Sensors system appears to be based on what the sensors chips are
> on the motherboard, rather than what the motherboard is. From the "probe
> and find out what I have" concept, this is reasonable, but it does cause
> a LOT of problems for users who are less educated. Many of the sensors
> devices are wired up in different ways on different motherboards, with
> (for example) a voltage input being used for a 12v rail monitor on one
> motherboard and a 5v rail monitor on another. The motherboard
> manufacturer is completely at liberty to wire a device in any way
> they choose.
Completely true.
> This therefore makes a sensors.conf file really a motherboard-specific
> rather than a device specific file. If you use the configuration file
> according to the device that you have, rather than the motherboard that
> you have, then you run a very high risk of reporting nonsense to a user,
> or (arguably worse) reporting information that looks correct but is
> actually wrong.
True again.
Keep in mind that the folks originally running the project were
passionates with good electronics knowledge. Back then, most users also
had the same profile. This explains a number of technical choices which
turned out not to be suitable for average users. In a way, the project
is the victim of its own success. It is also victim of the increase of
the number of hardware monitoring chips and the exponential growth of
the number of motherboard, each with its own specifics.
We're trying to fix these issues as time permits (e.g. the drivers do no
more set arbitrary limits, and they tend not to reset chips anymore) but
there's still a lot left to do.
> Following this through, when you request that the driver report the
> voltage detected in mV, I am _hoping_ that you are asking for the
> voltage at the pin of the device, rather than the voltage that the
> device happens to be ultimately connected to on this particular
> motherboard? Reporting the voltage at the device pin is the only
> consistent behaviour that I can see for a motherboard-independent device
> driver.
Yes, of course. Drivers are written for chips. All the rest belongs to
userspace (libsensors or whatever).
> If the above makes sense, then would there be any interest in building
> some kind of database of known good sensors.conf files for specific
> motherboards?
There have been 472 people complaining of the lack of such a database
already. A dozen of these said they would build one. Two of these
actually did. One is already dead, one is available at
http://lm-sensors-db.berlios.de/ but doesn't actually work.
We rejected the idea that we would include the configuration files one by
one in the lm_sensors project as people send them, because it would be
too much work. An external, web-based system where people could post and
retrieve their configuration files would be prefered. But we are still
in need of one which would work.
It might be discussed what to do with the sensors.conf.eg file. It
started as a small file when there were only a couple supported chips,
and motherboard manufacturers were following the chip maker's
recommendations on wiring. Now the file is quite large, and has lots of
conditionals (expressed as comments) to try and help users to get a
working configuration file. according to their hardware. I agree that it
isn't suitable for the average user.
Maybe we should stop installing this file to /etc/sensors.conf
automatically. As you say, It's probably better not to have a
configuration file than to have a bad one. However, this still a good
base for people to start tinkering, so we probably can't get rid of it
unless we actually have a database with configuration files for the most
popular boards.
> I would suggest that the output of "LSHW" or a similar utility is used
> to obtain unique identifying details for the motherboard, and then this
> used to determine the appropriate motherboard conf file. A simple web
> page with known motherboards on it and their associated .conf would be a
> very good start. New motherboards could then be added by people with
> their matching .conf file.
We would probably use the DMI fields for board identification (using
dmidecode, which is more standard than lshw I think). However, the
difficult part is to gather the data. What you call "a simple web
page" won't work. We want something the average user can freely
contribute to, with a minimum maintenance workload from us. Briefly:
* Any user can add a configuration file. He/she must pick from a list of
motherboard makers, or add one if his/her aint in the list. Then he/she
types the name of his/her motherboard and uploads his file. Ideally we
would do a brief content check to make sure nobody uploads crap.
* Any user can update (overwrite) a configuration file. We probably need
to backup older files just in case though.
* We need an administration interface to fix about anything (manufacturer
or board name, broken configuration statements) and even delete files if
needed.
* Users should have the possibility to alert us if they think something
is wrong.
* Not sure how to deal with duplicates. We probably want one single
configuration file per board, but it might be hard to enforce.
Note that there still is a problem with the fact that not all users of
the same board will use the same fans, so fan limits (and, to some
extent, temperature limits) depend on the user. The configuration files
are still templates. If we want to ultimately install them
automatically, we will probably need to enforce the fact that
configuration files should not set limits for fans and temperatures, nor
ignore statements for fans. These configuration files should essentially
contain label and compute statements.
This ain't simple, else this would have been done already. But if you
want to work on this (preferably *after* we are done with the vt8231
port ;)) you're very welcome.
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (12 preceding siblings ...)
2005-11-07 10:37 ` Jean Delvare
@ 2005-11-07 20:56 ` Roger Lucas
2005-11-09 13:03 ` Jean Delvare
` (6 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Roger Lucas @ 2005-11-07 20:56 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
Attached is revision 3 of the VT8231 driver. There are a LOT of changes to
implement your suggestions. Some of these break compatibility with older
vt8231 driver releases as follows:
tempX sequence
Temperatures are now measured temp0,temp1,temp2,etc. This causes the
"sensors" user-space program to fail with errors as it expects the older
temperature numbering sequence.
tempX scaling
Given that we have no idea what the temperature curves for a device are, the
driver now returns the 10-bit value directly from the register. Older
versions of the driver multiplied the register value by 2.5 (reason unknown
as it DEFINITELY didn't scale it to degrees Centigrade, Fahrenheit or
Kelvin). Older temperature scaling calculations are now completely wrong.
inX scaling
The driver now scales the measurement to mV. Inputs such as the 3.3v
internal reference are now completely correct. Inputs such as IN0-IN5 which
report an external pin now return the value at the pin. SENSORS.CONF still
has to scale these to compensate for external divider chains (if, for
example, the 12v rail is being measured).
tempX/inX presence
The driver only generates measurements for inputs that are genuinely
measured (according to the CONFIG register). This causes the "sensors"
program to fail as it expects to see all the tempX and inX sources
available, even if you add "ignore xxx" to the sensors.conf file. IMHO this
is a bug in the "sensors" program, as "ignore xxx" should cause it to fully
ignore that parameter, not still generate warnings if the parameter doesn't
exist.
PWM controls
The VT8231 doesn't have any PWM controls. All the code that was in the
driver was erroneous and has been removed (I suspect it was legacy from some
other driver used as the base for an earlier port).
If the newer vt8231 driver is accepted into the kernel tree, would you like
me to go through v2.9.2 of lm_sensors and try to see what can be done to
resolve the compatibilities issues with this newer vt8231 driver?
As far as the web site for the sensors.conf file is concerned, I have no
time to do this in the near-term. If I get some spare time in the future,
and no-one has done it by then, I will look into it further.
Best regards,
Roger
-----Original Message-----
From: Jean Delvare [mailto:khali@linux-fr.org]
Sent: 07 November 2005 09:24
To: roger@planbit.co.uk
Cc: Knut Petersen; LM Sensors
Subject: RE: [lm-sensors] RE: vt8231.c
Hi Roger,
On 2005-11-06, Roger Lucas wrote:
> One question I have is regarding the "1 mV" calibration, and this returns
> me to a general comment/question with the LM-Sensors system:
>
> The LM-Sensors system appears to be based on what the sensors chips are
> on the motherboard, rather than what the motherboard is. From the "probe
> and find out what I have" concept, this is reasonable, but it does cause
> a LOT of problems for users who are less educated. Many of the sensors
> devices are wired up in different ways on different motherboards, with
> (for example) a voltage input being used for a 12v rail monitor on one
> motherboard and a 5v rail monitor on another. The motherboard
> manufacturer is completely at liberty to wire a device in any way
> they choose.
Completely true.
> This therefore makes a sensors.conf file really a motherboard-specific
> rather than a device specific file. If you use the configuration file
> according to the device that you have, rather than the motherboard that
> you have, then you run a very high risk of reporting nonsense to a user,
> or (arguably worse) reporting information that looks correct but is
> actually wrong.
True again.
Keep in mind that the folks originally running the project were
passionates with good electronics knowledge. Back then, most users also
had the same profile. This explains a number of technical choices which
turned out not to be suitable for average users. In a way, the project
is the victim of its own success. It is also victim of the increase of
the number of hardware monitoring chips and the exponential growth of
the number of motherboard, each with its own specifics.
We're trying to fix these issues as time permits (e.g. the drivers do no
more set arbitrary limits, and they tend not to reset chips anymore) but
there's still a lot left to do.
> Following this through, when you request that the driver report the
> voltage detected in mV, I am _hoping_ that you are asking for the
> voltage at the pin of the device, rather than the voltage that the
> device happens to be ultimately connected to on this particular
> motherboard? Reporting the voltage at the device pin is the only
> consistent behaviour that I can see for a motherboard-independent device
> driver.
Yes, of course. Drivers are written for chips. All the rest belongs to
userspace (libsensors or whatever).
> If the above makes sense, then would there be any interest in building
> some kind of database of known good sensors.conf files for specific
> motherboards?
There have been 472 people complaining of the lack of such a database
already. A dozen of these said they would build one. Two of these
actually did. One is already dead, one is available at
http://lm-sensors-db.berlios.de/ but doesn't actually work.
We rejected the idea that we would include the configuration files one by
one in the lm_sensors project as people send them, because it would be
too much work. An external, web-based system where people could post and
retrieve their configuration files would be prefered. But we are still
in need of one which would work.
It might be discussed what to do with the sensors.conf.eg file. It
started as a small file when there were only a couple supported chips,
and motherboard manufacturers were following the chip maker's
recommendations on wiring. Now the file is quite large, and has lots of
conditionals (expressed as comments) to try and help users to get a
working configuration file. according to their hardware. I agree that it
isn't suitable for the average user.
Maybe we should stop installing this file to /etc/sensors.conf
automatically. As you say, It's probably better not to have a
configuration file than to have a bad one. However, this still a good
base for people to start tinkering, so we probably can't get rid of it
unless we actually have a database with configuration files for the most
popular boards.
> I would suggest that the output of "LSHW" or a similar utility is used
> to obtain unique identifying details for the motherboard, and then this
> used to determine the appropriate motherboard conf file. A simple web
> page with known motherboards on it and their associated .conf would be a
> very good start. New motherboards could then be added by people with
> their matching .conf file.
We would probably use the DMI fields for board identification (using
dmidecode, which is more standard than lshw I think). However, the
difficult part is to gather the data. What you call "a simple web
page" won't work. We want something the average user can freely
contribute to, with a minimum maintenance workload from us. Briefly:
* Any user can add a configuration file. He/she must pick from a list of
motherboard makers, or add one if his/her aint in the list. Then he/she
types the name of his/her motherboard and uploads his file. Ideally we
would do a brief content check to make sure nobody uploads crap.
* Any user can update (overwrite) a configuration file. We probably need
to backup older files just in case though.
* We need an administration interface to fix about anything (manufacturer
or board name, broken configuration statements) and even delete files if
needed.
* Users should have the possibility to alert us if they think something
is wrong.
* Not sure how to deal with duplicates. We probably want one single
configuration file per board, but it might be hard to enforce.
Note that there still is a problem with the fact that not all users of
the same board will use the same fans, so fan limits (and, to some
extent, temperature limits) depend on the user. The configuration files
are still templates. If we want to ultimately install them
automatically, we will probably need to enforce the fact that
configuration files should not set limits for fans and temperatures, nor
ignore statements for fans. These configuration files should essentially
contain label and compute statements.
This ain't simple, else this would have been done already. But if you
want to work on this (preferably *after* we are done with the vt8231
port ;)) you're very welcome.
--
Jean Delvare
-------------- next part --------------
diff -Naur reference/drivers/hwmon/Kconfig linux-2.6.14/drivers/hwmon/Kconfig
--- reference/drivers/hwmon/Kconfig 2005-10-28 01:02:08.000000000 +0100
+++ linux-2.6.14/drivers/hwmon/Kconfig 2005-11-07 12:44:21.000000000 +0000
@@ -350,6 +350,18 @@
This driver can also be built as a module. If so, the module
will be called via686a.
+config SENSORS_VT8231
+ tristate "VT8231"
+ depends on HWMON && I2C && PCI && EXPERIMENTAL
+ select HWMON_VID
+ select I2C_ISA
+ help
+ If you say yes here then you get support for the integrated sensors
+ in the VIA VT8231 device.
+
+ This driver can also be built as a module. If so, the module
+ will be called vt8231.
+
config SENSORS_W83781D
tristate "Winbond W83781D, W83782D, W83783S, W83627HF, Asus AS99127F"
depends on HWMON && I2C
diff -Naur reference/drivers/hwmon/Makefile linux-2.6.14/drivers/hwmon/Makefile
--- reference/drivers/hwmon/Makefile 2005-10-28 01:02:08.000000000 +0100
+++ linux-2.6.14/drivers/hwmon/Makefile 2005-11-01 10:59:10.000000000 +0000
@@ -40,6 +40,7 @@
obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
obj-$(CONFIG_SENSORS_VIA686A) += via686a.o
+obj-$(CONFIG_SENSORS_VT8231) += vt8231.o
obj-$(CONFIG_SENSORS_W83627EHF) += w83627ehf.o
obj-$(CONFIG_SENSORS_W83L785TS) += w83l785ts.o
diff -Naur reference/drivers/hwmon/vt8231.c linux-2.6.14/drivers/hwmon/vt8231.c
--- reference/drivers/hwmon/vt8231.c 1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.14/drivers/hwmon/vt8231.c 2005-11-07 19:41:45.000000000 +0000
@@ -0,0 +1,831 @@
+/*
+ vt8231.c - Part of lm_sensors, Linux kernel modules
+ for hardware monitoring
+
+ Copyright (c) 2002 Mark D. Studebaker <mdsxyz123@yahoo.com>
+ Aaron M. Marsh <amarsh@sdf.lonestar.org>
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program; if not, write to the Free Software
+ Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+*/
+
+/* Supports VIA VT8231 South Bridge embedded sensors
+*/
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/pci.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/i2c-isa.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon-vid.h>
+#include <linux/err.h>
+#include <asm/io.h>
+
+static int force_addr;
+module_param(force_addr, int, 0);
+MODULE_PARM_DESC(force_addr, "Initialize the base address of the sensors");
+
+/* Device address
+ Note that we can't determine the ISA address until we have initialized
+ our module */
+static unsigned short isa_address;
+
+#define VT8231_EXTENT 0x80
+#define VT8231_BASE_REG 0x70
+#define VT8231_ENABLE_REG 0x74
+
+/* The VT8231 registers
+
+ The default value for the input channel configuration is used (Reg 0x4A=0x07)
+ which sets the selected inputs marked with '*' below if multiple options are
+ possible:
+
+ Sensor Voltage Mode Temp Mode
+ -------- ------------ ---------
+ CPU Diode N/A temp0
+ UIC1 in0 temp1 *
+ UIC2 in1 * temp2
+ UIC3 in2 * temp3
+ UIC4 in3 * temp4
+ UIC5 in4 * temp5
+ 3.3V in5 N/A
+*/
+
+/* ins numbered 0-5 */
+/* fans numbered 1-2 */
+#define VT8231_REG_FAN_MIN(nr) (0x3a + (nr))
+#define VT8231_REG_FAN(nr) (0x28 + (nr))
+
+static const u8 regtemp[] = { 0x1f, 0x21, 0x22, 0x23, 0x24, 0x25 };
+static const u8 regtempmax[] = { 0x39, 0x3d, 0x2b, 0x2d, 0x2f, 0x31 };
+static const u8 regtempmin[] = { 0x3a, 0x3e, 0x2c, 0x2e, 0x30, 0x32 };
+
+static const u8 regvolt[] = { 0x21, 0x22, 0x23, 0x24, 0x25, 0x26 };
+static const u8 regvoltmax[] = { 0x3d, 0x2b, 0x2d, 0x2f, 0x31, 0x33 };
+static const u8 regvoltmin[] = { 0x3e, 0x2c, 0x2e, 0x30, 0x32, 0x34 };
+
+/* temps numbered 0-5 */
+#define VT8231_REG_TEMP_LOW01 0x49
+#define VT8231_REG_TEMP_LOW25 0x4d
+
+#define VT8231_REG_CONFIG 0x40
+#define VT8231_REG_ALARM1 0x41
+#define VT8231_REG_ALARM2 0x42
+#define VT8231_REG_VID 0x45
+#define VT8231_REG_FANDIV 0x47
+#define VT8231_REG_UCH_CONFIG 0x4a
+#define VT8231_REG_TEMP1_CONFIG 0x4b
+#define VT8231_REG_TEMP2_CONFIG 0x4c
+
+/* temps 0-5 */
+#define ISTEMP(i, ch_config) ((i) < 0 ? 0 : \
+ (i) = 0 ? 1 : \
+ (i) > 5 ? 0 : \
+ ((ch_config) >> ((i)+1)) & 0x01)
+/* voltages 0-6 */
+#define ISVOLT(i, ch_config) ((i) < 0 ? 0 : \
+ (i) > 5 ? 0 : \
+ (i) = 5 ? 1 : \
+ !(((ch_config) >> ((i)+2)) & 0x01))
+
+#define DIV_FROM_REG(val) (1 << (val))
+
+
+/* NB The values returned here are NOT temperatures. The calibration curves
+** for the thermistor curves are board-specific and must go in the
+** sensors.conf file. Temperature sensors 0 and 1 are ten bits, whilst
+** temperature sensors 2-5 are eight bits, so we scale all the values to be
+** equivalent to ten bits for consistency.
+** TEMP_TO_REG() sets the hardware comparisons and these are all use the 8
+** MSBs so we apply scaling in the macro to compensate.
+*/
+#define TEMP_TO_REG(val) (SENSORS_LIMIT((val)>>2, 0, 255))
+
+#define IN_TO_REG(val) (SENSORS_LIMIT((val), 0, 255))
+
+/********* FAN RPM CONVERSIONS ********/
+/* But this chip saturates back at 0, not at 255 like all the other chips.
+ So, 0 means 0 RPM */
+static inline u8 FAN_TO_REG(long rpm, int div)
+{
+ if (rpm = 0)
+ return 0;
+ return SENSORS_LIMIT((1310720 + rpm * div / 2) / (rpm * div), 1, 255);
+}
+
+#define FAN_FROM_REG(val, div) ((val)=0?0:1310720/((val)*(div)))
+
+struct vt8231_data {
+ struct i2c_client client;
+ struct semaphore update_lock;
+ struct class_device *class_dev;
+ char valid; /* !=0 if following fields are valid */
+ unsigned long last_updated; /* In jiffies */
+
+ u8 in[6]; /* Register value */
+ u8 in_max[6]; /* Register value */
+ u8 in_min[6]; /* Register value */
+ u16 temp[6]; /* Register value 10 bit */
+ u8 temp_max[6]; /* Register value */
+ u8 temp_min[6]; /* Register value */
+ u8 fan[2]; /* Register value */
+ u8 fan_min[2]; /* Register value */
+ u8 fan_div[2]; /* Register encoding, shifted right */
+ u16 alarms; /* Register encoding */
+ u8 cpu0_vid; /* Register encoding */
+ u8 vrm;
+ u8 uch_config;
+};
+
+static struct pci_dev *s_bridge;
+static int vt8231_detect(struct i2c_adapter *adapter);
+static int vt8231_detach_client(struct i2c_client *client);
+
+static inline int vt8231_read_value(struct i2c_client *client, u8 reg)
+{
+ return inb_p(client->addr + reg);
+}
+
+static inline void vt8231_write_value(struct i2c_client *client, u8 reg,
+ u8 value)
+{
+ outb_p(value, client->addr + reg);
+}
+
+static struct vt8231_data *vt8231_update_device(struct device *dev);
+static void vt8231_init_client(struct i2c_client *client);
+
+/* following are the sysfs callback functions */
+static ssize_t show_in(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+ struct vt8231_data *data = vt8231_update_device(dev);
+
+ switch(nr)
+ {
+ case 5: // in5 is scaled to 3.3v internally
+ // No floating point so 1/(0.0958*0.6296 ~= 1657/100
+ return sprintf(buf, "%d\n",
+ (int)(((data->in[nr] - 3) * 1657) / 100));
+
+ default:
+ // No floating point so 1/(0.0958 ~= 1044/100
+ return sprintf(buf, "%d\n",
+ (int)(((data->in[nr] - 3) * 1044) / 100));
+ };
+}
+
+static ssize_t show_in_min(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+ struct vt8231_data *data = vt8231_update_device(dev);
+ switch(nr)
+ {
+ case 5: // in5 is scaled to 3.3v internally
+ return sprintf(buf, "%d\n",
+ (int)(((data->in_min[nr] - 3) * 1657) / 100));
+
+ default:
+ return sprintf(buf, "%d\n",
+ (int)(((data->in_min[nr] - 3) * 1044) / 100));
+ };
+}
+
+static ssize_t show_in_max(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+ struct vt8231_data *data = vt8231_update_device(dev);
+ switch(nr)
+ {
+ case 5: // in5 is scaled to 3.3v internally
+ return sprintf(buf, "%d\n",
+ (int)(((data->in_max[nr] - 3) * 1657) / 100));
+
+ default:
+ return sprintf(buf, "%d\n",
+ (int)(((data->in_max[nr] - 3) * 1044) / 100));
+ };
+}
+
+static ssize_t set_in_min(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
+
+ down(&data->update_lock);
+ switch(nr)
+ {
+ case 5: // in5 is scaled to 3.3v internally
+ data->in_min[nr] = IN_TO_REG(((val * 100) / 1657) + 3);
+ break;
+ default:
+ data->in_min[nr] = IN_TO_REG(((val * 100) / 1044) + 3);
+ break;
+ };
+ vt8231_write_value(client, regvoltmin[nr], data->in_min[nr]);
+ up(&data->update_lock);
+ return count;
+}
+static ssize_t set_in_max(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
+ down(&data->update_lock);
+ switch(nr)
+ {
+ case 5: // in5 is scaled to 3.3v internally
+ data->in_max[nr] = IN_TO_REG(((val * 100) / 1657) + 3);
+ break;
+ default:
+ data->in_max[nr] = IN_TO_REG(((val * 100) / 1044) + 3);
+ break;
+ };
+ vt8231_write_value(client, regvoltmax[nr], data->in_max[nr]);
+ up(&data->update_lock);
+ return count;
+}
+
+#define show_in_offset(offset) \
+static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, \
+ show_in, NULL, offset);
+
+#define limit_in_offset(offset) \
+static SENSOR_DEVICE_ATTR(in##offset##_min, S_IRUGO | S_IWUSR, \
+ show_in_min, set_in_min, offset); \
+static SENSOR_DEVICE_ATTR(in##offset##_max, S_IRUGO | S_IWUSR, \
+ show_in_max, set_in_max, offset);
+
+show_in_offset(0);
+limit_in_offset(0);
+show_in_offset(1);
+limit_in_offset(1);
+show_in_offset(2);
+limit_in_offset(2);
+show_in_offset(3);
+limit_in_offset(3);
+show_in_offset(4);
+limit_in_offset(4);
+show_in_offset(5);
+limit_in_offset(5);
+
+/* 3 temperatures */
+static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n", data->temp[nr]);
+}
+static ssize_t show_temp_max(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n", data->temp_max[nr]);
+}
+static ssize_t show_temp_min(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n", data->temp_min[nr]);
+}
+static ssize_t set_temp_max(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+ down(&data->update_lock);
+ data->temp_max[nr] = TEMP_TO_REG(val);
+ vt8231_write_value(client, regtempmax[nr], data->temp_max[nr]);
+ up(&data->update_lock);
+ return count;
+}
+static ssize_t set_temp_min(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+ down(&data->update_lock);
+ data->temp_min[nr] = TEMP_TO_REG(val);
+ vt8231_write_value(client, regtempmin[nr],
+ data->temp_min[nr]);
+ up(&data->update_lock);
+ return count;
+}
+
+#define show_temp_offset(offset) \
+static SENSOR_DEVICE_ATTR(temp##offset##_input, S_IRUGO, \
+ show_temp, NULL, offset); \
+static SENSOR_DEVICE_ATTR(temp##offset##_max, S_IRUGO | S_IWUSR, \
+ show_temp_max, set_temp_max, offset); \
+static SENSOR_DEVICE_ATTR(temp##offset##_min, S_IRUGO | S_IWUSR, \
+ show_temp_min, set_temp_min, offset);
+
+show_temp_offset(0);
+show_temp_offset(1);
+show_temp_offset(2);
+show_temp_offset(3);
+show_temp_offset(4);
+show_temp_offset(5);
+
+/*
+** See the table at the top of this file for how the inputs and temperatures
+** map against each other.
+*/
+
+#define CFG_INFO_TEMP_ONLY(id) { -1, NULL, NULL, NULL, id, \
+ &sensor_dev_attr_temp##id##_input.dev_attr, \
+ &sensor_dev_attr_temp##id##_min.dev_attr, \
+ &sensor_dev_attr_temp##id##_max.dev_attr }
+#define CFG_INFO_VOLT_ONLY(id) { id, \
+ &sensor_dev_attr_in##id##_input.dev_attr, \
+ &sensor_dev_attr_in##id##_min.dev_attr, \
+ &sensor_dev_attr_in##id##_max.dev_attr, \
+ -1, NULL, NULL, NULL, }
+#define CFG_INFO_VOLT_TEMP(vid, tid) { vid, \
+ &sensor_dev_attr_in##vid##_input.dev_attr, \
+ &sensor_dev_attr_in##vid##_min.dev_attr, \
+ &sensor_dev_attr_in##vid##_max.dev_attr, \
+ tid, \
+ &sensor_dev_attr_temp##tid##_input.dev_attr, \
+ &sensor_dev_attr_temp##tid##_min.dev_attr, \
+ &sensor_dev_attr_temp##tid##_max.dev_attr }
+#define CFG_INFO_END { -1, NULL, NULL, NULL, -1, NULL, NULL, NULL }
+
+struct {
+ int in_id;
+ struct device_attribute *in;
+ struct device_attribute *in_min;
+ struct device_attribute *in_max;
+ int temp_id;
+ struct device_attribute *temp;
+ struct device_attribute *temp_min;
+ struct device_attribute *temp_max;
+} cfgInfoTempVolt[] = {
+ CFG_INFO_TEMP_ONLY(0),
+ CFG_INFO_VOLT_TEMP(0,1),
+ CFG_INFO_VOLT_TEMP(1,2),
+ CFG_INFO_VOLT_TEMP(2,3),
+ CFG_INFO_VOLT_TEMP(3,4),
+ CFG_INFO_VOLT_TEMP(4,5),
+ CFG_INFO_VOLT_ONLY(5),
+ CFG_INFO_END
+};
+
+/* 2 Fans */
+static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index - 1;
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[nr],
+ DIV_FROM_REG(data->fan_div[nr])));
+}
+
+static ssize_t show_fan_min(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index - 1;
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n",
+ FAN_FROM_REG(data->fan_min[nr],
+ DIV_FROM_REG(data->fan_div[nr])));
+}
+static ssize_t show_fan_div(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index - 1;
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n", DIV_FROM_REG(data->fan_div[nr]));
+}
+static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index - 1;
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int val = simple_strtoul(buf, NULL, 10);
+ down(&data->update_lock);
+ data->fan_min[nr] = FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr]));
+ vt8231_write_value(client, VT8231_REG_FAN_MIN(nr+1), data->fan_min[nr]);
+ up(&data->update_lock);
+ return count;
+}
+
+static ssize_t set_fan_div(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index - 1;
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ unsigned long val = simple_strtoul(buf, NULL, 10);
+ down(&data->update_lock);
+ int old = vt8231_read_value(client, VT8231_REG_FANDIV);
+
+ switch (val) {
+ case 1: data->fan_div[nr] = 0; break;
+ case 2: data->fan_div[nr] = 1; break;
+ case 4: data->fan_div[nr] = 2; break;
+ case 8: data->fan_div[nr] = 3; break;
+ default:
+ dev_err(&client->dev, "fan_div value %ld not "
+ "supported. Choose one of 1, 2, 4 or 8!\n", val);
+ up(&data->update_lock);
+ return -EINVAL;
+ }
+
+ old = (old & 0x0f) | (data->fan_div[1] << 6) | (data->fan_div[0] << 4);
+ vt8231_write_value(client, VT8231_REG_FANDIV, old);
+ up(&data->update_lock);
+ return count;
+}
+
+
+#define show_fan_offset(offset) \
+static SENSOR_DEVICE_ATTR(fan##offset##_input, S_IRUGO, \
+ show_fan, NULL, offset); \
+static SENSOR_DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR, \
+ show_fan_div, set_fan_div, offset); \
+static SENSOR_DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR, \
+ show_fan_min, set_fan_min, offset);
+
+show_fan_offset(1);
+show_fan_offset(2);
+
+/* Alarms */
+static ssize_t show_alarms(struct device *dev, struct device_attribute *attr,
+ char *buf) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n", data->alarms);
+}
+static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
+
+
+/**** VRM and VID *****/
+static ssize_t show_vid(struct device *dev, struct device_attribute *attr,
+ char *buf) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n", vid_from_reg(data->cpu0_vid, data->vrm));
+}
+static DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid, NULL);
+
+static ssize_t show_vrm(struct device *dev, struct device_attribute *attr,
+ char *buf) {
+ struct vt8231_data *data = vt8231_update_device(dev);
+ return sprintf(buf, "%d\n", (unsigned)data->vrm);
+}
+static ssize_t set_vrm(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int val = simple_strtol(buf, NULL, 10);
+ down(&data->update_lock);
+ data->vrm = val;
+ up(&data->update_lock);
+ return count;
+}
+static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm, set_vrm);
+
+static struct i2c_driver vt8231_driver = {
+ .owner = THIS_MODULE,
+ .name = "vt8231",
+ .id = I2C_DRIVERID_VT8231,
+ .flags = I2C_DF_NOTIFY,
+ .attach_adapter = vt8231_detect,
+ .detach_client = vt8231_detach_client,
+};
+
+static struct pci_device_id vt8231_pci_ids[] = {
+ { PCI_DEVICE( PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_8231_4) },
+ { 0, }
+};
+
+MODULE_DEVICE_TABLE(pci, vt8231_pci_ids);
+
+static int __devinit vt8231_pci_probe(struct pci_dev *dev,
+ const struct pci_device_id *id);
+
+static struct pci_driver vt8231_pci_driver = {
+ .name = "vt8231",
+ .id_table = vt8231_pci_ids,
+ .probe = vt8231_pci_probe,
+};
+
+int vt8231_detect(struct i2c_adapter *adapter)
+{
+ struct i2c_client *new_client;
+ struct vt8231_data *data;
+ int err = 0, i;
+ u16 val;
+
+ /* 8231 requires multiple of 256 */
+ if (force_addr)
+ isa_address = force_addr & 0xFF00;
+
+ if (force_addr) {
+ dev_warn(&adapter->dev, "forcing ISA address 0x%04X\n",
+ isa_address);
+ if (PCIBIOS_SUCCESSFUL != pci_write_config_word(s_bridge,
+ VT8231_BASE_REG, isa_address))
+ return -ENODEV;
+ }
+ if (PCIBIOS_SUCCESSFUL !+ pci_read_config_word(s_bridge, VT8231_ENABLE_REG, &val))
+ return -ENODEV;
+ if (!(val & 0x0001)) {
+ dev_warn(&adapter->dev, "enabling sensors\n");
+ if (PCIBIOS_SUCCESSFUL !+ pci_write_config_word(s_bridge, VT8231_ENABLE_REG,
+ val | 0x0001))
+ return -ENODEV;
+ }
+
+ /* Reserve the ISA region */
+ if (!request_region(isa_address, VT8231_EXTENT, vt8231_pci_driver.name)) {
+ dev_err(&adapter->dev, "region 0x%x already in use!\n",
+ isa_address);
+ return -ENODEV;
+ }
+
+ if (!(data = kzalloc(sizeof(struct vt8231_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto exit_release;
+ }
+
+ new_client = &data->client;
+ i2c_set_clientdata(new_client, data);
+ new_client->addr = isa_address;
+ new_client->adapter = adapter;
+ new_client->driver = &vt8231_driver;
+ new_client->dev.parent = &adapter->dev;
+
+ /* Fill in the remaining client fields and put into the global list */
+ strcpy(new_client->name, "vt8231");
+
+ data->valid = 0;
+ init_MUTEX(&data->update_lock);
+
+ /* Tell the I2C layer a new client has arrived */
+ if ((err = i2c_attach_client(new_client)))
+ goto exit_free;
+
+ vt8231_init_client(new_client);
+
+ /* Register sysfs hooks */
+ data->class_dev = hwmon_device_register(&new_client->dev);
+ if (IS_ERR(data->class_dev)) {
+ err = PTR_ERR(data->class_dev);
+ goto exit_detach;
+ }
+
+ /* Must update device information to find out the config field */
+ data->uch_config = vt8231_read_value(new_client, VT8231_REG_UCH_CONFIG);
+
+ for(i=0;
+ (cfgInfoTempVolt[i].in_id >= 0)||(cfgInfoTempVolt[i].temp_id >= 0);
+ i++)
+ {
+ if ( ISVOLT(cfgInfoTempVolt[i].in_id, data->uch_config) )
+ {
+ device_create_file(&new_client->dev,
+ cfgInfoTempVolt[i].in);
+ device_create_file(&new_client->dev,
+ cfgInfoTempVolt[i].in_max);
+ device_create_file(&new_client->dev,
+ cfgInfoTempVolt[i].in_min);
+ }
+ else if ( ISTEMP(cfgInfoTempVolt[i].temp_id, data->uch_config) )
+ {
+ device_create_file(&new_client->dev,
+ cfgInfoTempVolt[i].temp);
+ device_create_file(&new_client->dev,
+ cfgInfoTempVolt[i].temp_max);
+ device_create_file(&new_client->dev,
+ cfgInfoTempVolt[i].temp_min);
+ }
+ }
+
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan1_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan2_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan1_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan2_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan1_div.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan2_div.dev_attr);
+
+ device_create_file(&new_client->dev, &dev_attr_alarms);
+
+ device_create_file(&new_client->dev, &dev_attr_cpu0_vid);
+ device_create_file(&new_client->dev, &dev_attr_vrm);
+
+ return 0;
+
+exit_detach:
+ i2c_detach_client(new_client);
+exit_free:
+ kfree(data);
+exit_release:
+ release_region(isa_address, VT8231_EXTENT);
+ return err;
+}
+
+static int vt8231_detach_client(struct i2c_client *client)
+{
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int err;
+
+ hwmon_device_unregister(data->class_dev);
+
+ if ((err = i2c_detach_client(client))) {
+ return err;
+ }
+
+ release_region(client->addr, VT8231_EXTENT);
+ kfree(data);
+
+ return 0;
+}
+
+static void vt8231_init_client(struct i2c_client *client)
+{
+ struct vt8231_data *data = i2c_get_clientdata(client);
+
+ data->vrm = vid_which_vrm();
+
+ /* set "default" interrupt mode for alarms, which isn't the default */
+ vt8231_write_value(client, VT8231_REG_TEMP1_CONFIG, 0);
+ vt8231_write_value(client, VT8231_REG_TEMP2_CONFIG, 0);
+}
+
+static struct vt8231_data *vt8231_update_device(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct vt8231_data *data = i2c_get_clientdata(client);
+ int i, j;
+
+ down(&data->update_lock);
+
+ if (time_after(jiffies, data->last_updated + HZ + HZ / 2)
+ || !data->valid) {
+ data->uch_config = vt8231_read_value(client,
+ VT8231_REG_UCH_CONFIG);
+ for (i = 0; i < 6; i++) {
+ if(ISVOLT(i, data->uch_config)) {
+ data->in[i] = vt8231_read_value(client,
+ regvolt[i]);
+ data->in_min[i] = vt8231_read_value(client,
+ regvoltmin[i]);
+ data->in_max[i] = vt8231_read_value(client,
+ regvoltmax[i]);
+ }
+ }
+ for (i = 1; i < 3; i++) {
+ data->fan[i - 1] = vt8231_read_value(client,
+ VT8231_REG_FAN(i));
+ data->fan_min[i - 1] = vt8231_read_value(client,
+ VT8231_REG_FAN_MIN(i));
+ }
+ for (i = 0; i < 6; i++) {
+ if(ISTEMP(i, data->uch_config)) {
+ data->temp[i] = vt8231_read_value(client,
+ regtemp[i]) << 2;
+ switch(i) {
+ case 0: j = (vt8231_read_value(client,
+ VT8231_REG_TEMP_LOW01)
+ >> 6) & 0x03;
+ break;
+
+ case 1: j = (vt8231_read_value(client,
+ VT8231_REG_TEMP_LOW01)
+ >> 4) & 0x03;
+ break;
+ case 2:
+ case 3:
+ case 4:
+ case 5: j = (vt8231_read_value(client,
+ VT8231_REG_TEMP_LOW25)
+ >> ((i-2)*2)) & 0x03;
+ break;
+
+ default: j = 0;
+ break;
+ }
+ data->temp[i] |= j;
+ data->temp_max[i] = vt8231_read_value(client,
+ regtempmax[i]);
+ data->temp_min[i] = vt8231_read_value(client,
+ regtempmin[i]);
+ }
+ }
+
+ i = vt8231_read_value(client, VT8231_REG_FANDIV);
+ data->fan_div[0] = (i >> 4) & 0x03;
+ data->fan_div[1] = i >> 6;
+ data->alarms = vt8231_read_value(client, VT8231_REG_ALARM1) |
+ (vt8231_read_value(client, VT8231_REG_ALARM2) << 8);
+ data->cpu0_vid = vt8231_read_value(client, VT8231_REG_VID)
+ & 0x1f;
+ data->last_updated = jiffies;
+ data->valid = 1;
+ }
+
+ up(&data->update_lock);
+
+ return data;
+}
+
+static int __devinit vt8231_pci_probe(struct pci_dev *dev,
+ const struct pci_device_id *id)
+{
+ u16 val;
+
+ if (PCIBIOS_SUCCESSFUL != pci_read_config_word(dev, VT8231_BASE_REG,
+ &val))
+ return -ENODEV;
+
+ isa_address = val & ~(VT8231_EXTENT - 1);
+ if (isa_address = 0 && force_addr = 0) {
+ dev_err(&dev->dev, "base address not set -\
+ upgrade BIOS or use force_addr=0xaddr\n");
+ return -ENODEV;
+ }
+
+ s_bridge = pci_dev_get(dev);
+
+ if (i2c_isa_add_driver(&vt8231_driver)) {
+ pci_dev_put(s_bridge);
+ s_bridge = NULL;
+ }
+
+ /* Always return failure here. This is to allow other drivers to bind
+ * to this pci device. We don't really want to have control over the
+ * pci device, we only wanted to read as few register values from it.
+ */
+ return -ENODEV;
+}
+
+static int __init sm_vt8231_init(void)
+{
+ return pci_module_init(&vt8231_pci_driver);
+}
+
+static void __exit sm_vt8231_exit(void)
+{
+ pci_unregister_driver(&vt8231_pci_driver);
+ if (s_bridge != NULL) {
+ i2c_isa_del_driver(&vt8231_driver);
+ pci_dev_put(s_bridge);
+ s_bridge = NULL;
+ }
+}
+
+MODULE_AUTHOR("Mark D. Studebaker <mdsxyz123@yahoo.com>");
+MODULE_DESCRIPTION("VT8231 sensors");
+MODULE_LICENSE("GPL");
+
+module_init(sm_vt8231_init);
+module_exit(sm_vt8231_exit);
+
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (13 preceding siblings ...)
2005-11-07 20:56 ` Roger Lucas
@ 2005-11-09 13:03 ` Jean Delvare
2005-11-09 15:43 ` Roger Lucas
` (5 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Jean Delvare @ 2005-11-09 13:03 UTC (permalink / raw)
To: lm-sensors
Hi Roger,
On 2005-11-07, Roger Lucas wrote:
> Attached is revision 3 of the VT8231 driver. There are a LOT of changes
> to implement your suggestions. Some of these break compatibility with
> older vt8231 driver releases as follows:
I'll not review that code as you have posted r4 since, but I still would
like to answer the various points you make.
> tempX sequence
> Temperatures are now measured temp0,temp1,temp2,etc. This causes the
> "sensors" user-space program to fail with errors as it expects the older
> temperature numbering sequence.
No. As per Documentation/hwmon/sysfs-interface, temperature numbering
starts at 1. You can't use temp0.
I would usually object to renumbering because it breaks comptibility with
the 2.4 driver and libsensors, but it starts being obvious that we will
have to break compatibility anyway, and the 2.4 driver (and
libsensors/sensors) will need a significant rework, so it's probably no
more a problem.
> tempX scaling
> Given that we have no idea what the temperature curves for a device are,
> the driver now returns the 10-bit value directly from the register.
> Older versions of the driver multiplied the register value by 2.5 (reason
> unknown as it DEFINITELY didn't scale it to degrees Centigrade, Fahrenheit
> or Kelvin). Older temperature scaling calculations are now completely
> wrong.
We know how temperature scaling works, see the BIOS porting guide
document and sensors.conf.eg. This chip is quite different from other
ones and needs additional conputations.
temp3 is a bit special, it is diode-based. The conversion which is
currently done in userspace should be moved into the driver, so that the
exported value is a temperature rather than a random register value.
Other temps are thermistor based, a part of the conversion has to be
moved into the driver so that we export the voltage value at the chip
pin. The thermistor bridge computation (voltag -> temperature) belongs
to userspace because different thermistors can be used on different
motherboards. The same is done for the PC87366 temp4-6 already.
> inX scaling
> The driver now scales the measurement to mV. Inputs such as the 3.3v
> internal reference are now completely correct. Inputs such as IN0-IN5
> which report an external pin now return the value at the pin.
> SENSORS.CONF still has to scale these to compensate for external divider
> chains (if, for example, the 12v rail is being measured).
Correct, that's exactly what we want.
> tempX/inX presence
> The driver only generates measurements for inputs that are genuinely
> measured (according to the CONFIG register).
This sounds good to me. If anyone later complains that he/she needs to be
able to change the configuration, we'll see at that time. The BIOS
should configure it properly for us, so I would consider it a BIOS bug,
but...
> This causes the "sensors" program to fail as it expects to see all the
> tempX and inX sources available, even if you add "ignore xxx" to the
> sensors.conf file. IMHO this is a bug in the "sensors" program, as
> "ignore xxx" should cause it to fully ignore that parameter, not still
> generate warnings if the parameter doesn't exist.
Not sure if it is a bug or a design issue. I never took the time to
investigate this. If you have some time, please do. In the meantime, we
usually drop the printed complaint in sensors for all inputs which may
not exist in 2.6 drivers. It's kind of a hack (we could miss real
errors), but works.
> PWM controls
> The VT8231 doesn't have any PWM controls. All the code that was in the
> driver was erroneous and has been removed (I suspect it was legacy from
> some other driver used as the base for an earlier port).
Ugh. True, I don't see anything related to PWM in the VT8231 datasheet.
Mark, can you please comment on this? Undocumented registers which
worked for you? Or dead code inherited from the vt1211 or via686a
drivers? Let's clean up whatever can be.
> If the newer vt8231 driver is accepted into the kernel tree, would you
> like me to go through v2.9.2 of lm_sensors and try to see what can be
> done to resolve the compatibilities issues with this newer vt8231
> driver?
Yes, this would be very very welcome. I never feel confident when I have
to do that myself for drivers I can't test. If you can test the
changes, that's different, we can (and will have to) cleanup the 2.4
vt8231 driver.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (14 preceding siblings ...)
2005-11-09 13:03 ` Jean Delvare
@ 2005-11-09 15:43 ` Roger Lucas
2005-11-15 8:33 ` Jean Delvare
` (4 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Roger Lucas @ 2005-11-09 15:43 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
Please see my comments below.
- Roger
> -----Original Message-----
> From: Jean Delvare [mailto:khali@linux-fr.org]
> Sent: 09 November 2005 11:50
> To: roger@planbit.co.uk
> Cc: Knut Petersen; LM Sensors; Mark Studebaker
> Subject: RE: [lm-sensors] RE: vt8231.c
>
>
> Hi Roger,
>
> On 2005-11-07, Roger Lucas wrote:
> > Attached is revision 3 of the VT8231 driver. There are a LOT of changes
> > to implement your suggestions. Some of these break compatibility with
> > older vt8231 driver releases as follows:
>
> I'll not review that code as you have posted r4 since, but I still would
> like to answer the various points you make.
>
> > tempX sequence
> > Temperatures are now measured temp0,temp1,temp2,etc. This causes the
> > "sensors" user-space program to fail with errors as it expects the older
> > temperature numbering sequence.
>
> No. As per Documentation/hwmon/sysfs-interface, temperature numbering
> starts at 1. You can't use temp0.
No problem. Renumbering them is easy.
>
> I would usually object to renumbering because it breaks comptibility with
> the 2.4 driver and libsensors, but it starts being obvious that we will
> have to break compatibility anyway, and the 2.4 driver (and
> libsensors/sensors) will need a significant rework, so it's probably no
> more a problem.
>
> > tempX scaling
> > Given that we have no idea what the temperature curves for a device are,
> > the driver now returns the 10-bit value directly from the register.
> > Older versions of the driver multiplied the register value by 2.5
> (reason
> > unknown as it DEFINITELY didn't scale it to degrees Centigrade,
> Fahrenheit
> > or Kelvin). Older temperature scaling calculations are now completely
> > wrong.
>
> We know how temperature scaling works, see the BIOS porting guide
> document and sensors.conf.eg. This chip is quite different from other
> ones and needs additional conputations.
>
> temp3 is a bit special, it is diode-based. The conversion which is
> currently done in userspace should be moved into the driver, so that the
> exported value is a temperature rather than a random register value.
> Other temps are thermistor based, a part of the conversion has to be
> moved into the driver so that we export the voltage value at the chip
> pin. The thermistor bridge computation (voltag -> temperature) belongs
> to userspace because different thermistors can be used on different
> motherboards. The same is done for the PC87366 temp4-6 already.
We know what the datasheet and porting guide say about the temperatures, but
I am not sure that this matches reality. On my VIA EPIA 500 motherboard,
the CPU is a VIA EPIA 5000 and not an Intel CPU. I don't doubt that the
sensor scale is correct for an Intel CPU, but the VIA EPIA CPU is different.
If I use the scaling values in the porting guide then my temperatures are
over 30C off.
For reference,
VIA's equation: REG = TEMP * 0.9686 + 65
My EPIA Mobo: REG = TEMP * 0.7809 + 45
We can put the VIA official Intel scaling into the driver, but this would be
very misleading as the VT8231 is used on the VIA EPIA platform which is in
widespread use, and all users of that board would therefore need to know
that additional scaling was required. If they didn't then they would be
panicking when they see the CPU temperature (since the motherboard is only
passively cooled).
I'm happy putting in the appropriate voltage scaling for the other temps
into the driver, but I have no way to test it. We need to find someone else
who has a VT8231 on a motherboard that uses external temperature sensors who
can test it.
>
> > inX scaling
> > The driver now scales the measurement to mV. Inputs such as the 3.3v
> > internal reference are now completely correct. Inputs such as IN0-IN5
> > which report an external pin now return the value at the pin.
> > SENSORS.CONF still has to scale these to compensate for external divider
> > chains (if, for example, the 12v rail is being measured).
>
> Correct, that's exactly what we want.
>
> > tempX/inX presence
> > The driver only generates measurements for inputs that are genuinely
> > measured (according to the CONFIG register).
>
> This sounds good to me. If anyone later complains that he/she needs to be
> able to change the configuration, we'll see at that time. The BIOS
> should configure it properly for us, so I would consider it a BIOS bug,
> but...
>
> > This causes the "sensors" program to fail as it expects to see all the
> > tempX and inX sources available, even if you add "ignore xxx" to the
> > sensors.conf file. IMHO this is a bug in the "sensors" program, as
> > "ignore xxx" should cause it to fully ignore that parameter, not still
> > generate warnings if the parameter doesn't exist.
>
> Not sure if it is a bug or a design issue. I never took the time to
> investigate this. If you have some time, please do. In the meantime, we
> usually drop the printed complaint in sensors for all inputs which may
> not exist in 2.6 drivers. It's kind of a hack (we could miss real
> errors), but works.
>
> > PWM controls
> > The VT8231 doesn't have any PWM controls. All the code that was in the
> > driver was erroneous and has been removed (I suspect it was legacy from
> > some other driver used as the base for an earlier port).
>
> Ugh. True, I don't see anything related to PWM in the VT8231 datasheet.
> Mark, can you please comment on this? Undocumented registers which
> worked for you? Or dead code inherited from the vt1211 or via686a
> drivers? Let's clean up whatever can be.
>
> > If the newer vt8231 driver is accepted into the kernel tree, would you
> > like me to go through v2.9.2 of lm_sensors and try to see what can be
> > done to resolve the compatibilities issues with this newer vt8231
> > driver?
>
> Yes, this would be very very welcome. I never feel confident when I have
> to do that myself for drivers I can't test. If you can test the
> changes, that's different, we can (and will have to) cleanup the 2.4
> vt8231 driver.
I'll see what time I get for this, but I suspect I won't get much in the
immediate future.
>
> Thanks,
> --
> Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (15 preceding siblings ...)
2005-11-09 15:43 ` Roger Lucas
@ 2005-11-15 8:33 ` Jean Delvare
2005-11-15 12:14 ` Roger Lucas
` (3 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Jean Delvare @ 2005-11-15 8:33 UTC (permalink / raw)
To: lm-sensors
Hi Roger,
Once again, sorry for the delay... :(
> > temp3 is a bit special, it is diode-based. The conversion which is
> > currently done in userspace should be moved into the driver, so that the
> > exported value is a temperature rather than a random register value.
> > Other temps are thermistor based, a part of the conversion has to be
> > moved into the driver so that we export the voltage value at the chip
> > pin. The thermistor bridge computation (voltag -> temperature) belongs
> > to userspace because different thermistors can be used on different
> > motherboards. The same is done for the PC87366 temp4-6 already.
>
> We know what the datasheet and porting guide say about the temperatures, but
> I am not sure that this matches reality. On my VIA EPIA 500 motherboard,
> the CPU is a VIA EPIA 5000 and not an Intel CPU. I don't doubt that the
> sensor scale is correct for an Intel CPU, but the VIA EPIA CPU is different.
> If I use the scaling values in the porting guide then my temperatures are
> over 30C off.
>
> For reference,
> VIA's equation: REG = TEMP * 0.9686 + 65
> My EPIA Mobo: REG = TEMP * 0.7809 + 45
Good point. Do these constant have any kind of physical meaning? Is the
information publicly available? How did you come up with the correct
one for your VIA CPU?
What would the formula required for VIA CPUs look like if the Intel one
was integrated into the driver as I first proposed?
> We can put the VIA official Intel scaling into the driver, but this would be
> very misleading as the VT8231 is used on the VIA EPIA platform which is in
> widespread use, and all users of that board would therefore need to know
> that additional scaling was required. If they didn't then they would be
> panicking when they see the CPU temperature (since the motherboard is only
> passively cooled).
Well, whatever we do, the VIA CPU users will have an incorrect reading
at first and will have to edit some lines in their configuration file
to get the proper reading. So this point doesn't really weight in
either direction. All it really means is that we will have to improve
the vt8231's sensors.conf.eg section to make the point clear.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (16 preceding siblings ...)
2005-11-15 8:33 ` Jean Delvare
@ 2005-11-15 12:14 ` Roger Lucas
2005-11-15 13:15 ` Jean Delvare
` (2 subsequent siblings)
20 siblings, 0 replies; 22+ messages in thread
From: Roger Lucas @ 2005-11-15 12:14 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
> -----Original Message-----
> From: Jean Delvare [mailto:khali@linux-fr.org]
> Sent: 15 November 2005 07:13
> To: Roger Lucas
> Cc: Knut Petersen; LM Sensors; Mark Studebaker
> Subject: Re: [lm-sensors] RE: vt8231.c
>
> Hi Roger,
>
> Once again, sorry for the delay... :(
>
> > > temp3 is a bit special, it is diode-based. The conversion which is
> > > currently done in userspace should be moved into the driver, so that
> the
> > > exported value is a temperature rather than a random register value.
> > > Other temps are thermistor based, a part of the conversion has to be
> > > moved into the driver so that we export the voltage value at the chip
> > > pin. The thermistor bridge computation (voltag -> temperature) belongs
> > > to userspace because different thermistors can be used on different
> > > motherboards. The same is done for the PC87366 temp4-6 already.
> >
> > We know what the datasheet and porting guide say about the temperatures,
> but
> > I am not sure that this matches reality. On my VIA EPIA 500
> motherboard,
> > the CPU is a VIA EPIA 5000 and not an Intel CPU. I don't doubt that the
> > sensor scale is correct for an Intel CPU, but the VIA EPIA CPU is
> different.
> > If I use the scaling values in the porting guide then my temperatures
> are
> > over 30C off.
> >
> > For reference,
> > VIA's equation: REG = TEMP * 0.9686 + 65
> > My EPIA Mobo: REG = TEMP * 0.7809 + 45
>
> Good point. Do these constant have any kind of physical meaning? Is the
> information publicly available? How did you come up with the correct
> one for your VIA CPU?
We have been running some thermal tests on the VIA board and the values from
LM-Sensors were significantly off. We therefore ran our own thermal cycling
tests with a probe attached to the center of the heatsink directly above the
CPU. We were then able to obtain a set of measurements for the probe
temperature versus the register value. This gave us a very linear response
with the above equation. We did not get the curve information from VIA.
We have another sample of the board here now, so we will be repeating the
measurements to ensure that the VIA results are predictable from board to
board.
>
> What would the formula required for VIA CPUs look like if the Intel one
> was integrated into the driver as I first proposed?
>
It would still be a simple y=mx+c style graph but just with different
constants.
> > We can put the VIA official Intel scaling into the driver, but this
> would be
> > very misleading as the VT8231 is used on the VIA EPIA platform which is
> in
> > widespread use, and all users of that board would therefore need to know
> > that additional scaling was required. If they didn't then they would be
> > panicking when they see the CPU temperature (since the motherboard is
> only
> > passively cooled).
>
> Well, whatever we do, the VIA CPU users will have an incorrect reading
> at first and will have to edit some lines in their configuration file
> to get the proper reading. So this point doesn't really weight in
> either direction. All it really means is that we will have to improve
> the vt8231's sensors.conf.eg section to make the point clear.
I agree that whatever we do, the VIA users must apply some kind of equation
to get their correct temperature, but I feel that the safest approach is
"first do no wrong".
I am extremely uncomfortable with the situation where we release a driver
that we know will give incorrect information under some conditions. I feel
that it is much better to return the result from the driver as a register
value and put the text below:
# If you have an Intel CPU, then uncomment the line below
# temp0_input = (@-65)/0.9686, (@*0.9686)+65
# If you have an VIA EPIA CPU, then uncomment the line below
# temp0_input = (@-45)/0.7809, (@*0.7809)+45
This way, an incorrect value is never returned and the user must correctly
set the board configuration before a temperature is returned.
>
> Thanks,
> --
> Jean Delvare
Thanks for the work you are doing on reviewing this driver.
Best regards,
Roger
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (17 preceding siblings ...)
2005-11-15 12:14 ` Roger Lucas
@ 2005-11-15 13:15 ` Jean Delvare
2005-11-15 13:46 ` Roger Lucas
2005-11-15 20:17 ` Jean Delvare
20 siblings, 0 replies; 22+ messages in thread
From: Jean Delvare @ 2005-11-15 13:15 UTC (permalink / raw)
To: lm-sensors
Hi Roger,
On 2005-11-15, Roger Lucas wrote:
> We have another sample of the board here now, so we will be repeating the
> measurements to ensure that the VIA results are predictable from board to
> board.
OK, great. I've tried some mathematics on the formulae to find out if
anything obvious with regards to the physical meaning of the constants
would spring out, but did not find anything.
> I agree that whatever we do, the VIA users must apply some kind of
> equation to get their correct temperature, but I feel that the safest
> approach is "first do no wrong".
Fair enough.
> I am extremely uncomfortable with the situation where we release a
> driver that we know will give incorrect information under some
> conditions. I feel that it is much better to return the result from
> the driver as a register value and put the text below:
The problem is that the standard sysfs interface sets some contraints on
what we can or cannot do. In particular, it dictates that the magnitude
for temperature values is 3. This suggests that the value we would be
returning would be REG * 1000. As it seems that there are an additional
2 bits of resolution, that would be (REG << 2 + ADDREG) * 250.
> # If you have an Intel CPU, then uncomment the line below
> # temp0_input = (@-65)/0.9686, (@*0.9686)+65
>
> # If you have an VIA EPIA CPU, then uncomment the line below
> # temp0_input = (@-45)/0.7809, (@*0.7809)+45
s/temp0_input =/compute temp1/, but yes.
BTW, does this suggest that you decided that the diode temperature would
be temp1, and thermistor-based ones are temp2+? I have no objection a
priori, just curious.
> This way, an incorrect value is never returned and the user must
> correctly set the board configuration before a temperature is
> returned.
By commenting out both compute lines, I'd rather say that an incorrect
value will *always* be returned. I'd prefer to keep the Intel line by
default so as to preserve backward compatibility as much as we can.
I think you will end up with a dedicated configuration file for your
board anyway. The best we can do to help out VIA EPIA users is to make
this file publicly available.
Thanks,
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (18 preceding siblings ...)
2005-11-15 13:15 ` Jean Delvare
@ 2005-11-15 13:46 ` Roger Lucas
2005-11-15 20:17 ` Jean Delvare
20 siblings, 0 replies; 22+ messages in thread
From: Roger Lucas @ 2005-11-15 13:46 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
Comments below:
> -----Original Message-----
> From: Jean Delvare [mailto:khali@linux-fr.org]
> Sent: 15 November 2005 12:02
> To: roger@planbit.co.uk
> Cc: 'Mark Studebaker'; LM Sensors
> Subject: RE: [lm-sensors] RE: vt8231.c
>
>
> Hi Roger,
>
> On 2005-11-15, Roger Lucas wrote:
> > We have another sample of the board here now, so we will be repeating
> the
> > measurements to ensure that the VIA results are predictable from board
> to
> > board.
>
> OK, great. I've tried some mathematics on the formulae to find out if
> anything obvious with regards to the physical meaning of the constants
> would spring out, but did not find anything.
>
> > I agree that whatever we do, the VIA users must apply some kind of
> > equation to get their correct temperature, but I feel that the safest
> > approach is "first do no wrong".
>
> Fair enough.
>
> > I am extremely uncomfortable with the situation where we release a
> > driver that we know will give incorrect information under some
> > conditions. I feel that it is much better to return the result from
> > the driver as a register value and put the text below:
>
> The problem is that the standard sysfs interface sets some contraints on
> what we can or cannot do. In particular, it dictates that the magnitude
> for temperature values is 3. This suggests that the value we would be
> returning would be REG * 1000. As it seems that there are an additional
> 2 bits of resolution, that would be (REG << 2 + ADDREG) * 250.
>
From the above, if the driver returns the result ((REG << 2 + ADDREG) * 250)
to the sensors user-space application, then you are OK with this? The
SENSORS.CONF file would then apply one of the two lines below to give user
the temperature in degrees Centigrade.
> > # If you have an Intel CPU, then uncomment the line below
> > # temp0_input = (@-65)/0.9686, (@*0.9686)+65
> >
> > # If you have an VIA EPIA CPU, then uncomment the line below
> > # temp0_input = (@-45)/0.7809, (@*0.7809)+45
>
> s/temp0_input =/compute temp1/, but yes.
>
> BTW, does this suggest that you decided that the diode temperature would
> be temp1, and thermistor-based ones are temp2+? I have no objection a
> priori, just curious.
Nope. I don't care which is which. Really. If there is a general trend
for the CPU-0 temperature to be on a specific sensor then let me know and
I'll make the driver match.
>
> > This way, an incorrect value is never returned and the user must
> > correctly set the board configuration before a temperature is
> > returned.
>
> By commenting out both compute lines, I'd rather say that an incorrect
> value will *always* be returned. I'd prefer to keep the Intel line by
> default so as to preserve backward compatibility as much as we can.
>
> I think you will end up with a dedicated configuration file for your
> board anyway. The best we can do to help out VIA EPIA users is to make
> this file publicly available.
Fair enough.
>
> Thanks,
> --
> Jean Delvare
If you can send me the results for the code review of the driver then I'll
wrap these changes up into it and re-submit it. Hopefully then it is
complete.
Best regards,
Roger
^ permalink raw reply [flat|nested] 22+ messages in thread* [lm-sensors] RE: vt8231.c
2005-11-02 10:25 [lm-sensors] RE: vt8231.c Roger Lucas
` (19 preceding siblings ...)
2005-11-15 13:46 ` Roger Lucas
@ 2005-11-15 20:17 ` Jean Delvare
20 siblings, 0 replies; 22+ messages in thread
From: Jean Delvare @ 2005-11-15 20:17 UTC (permalink / raw)
To: lm-sensors
Hi Roger,
> From the above, if the driver returns the result ((REG << 2 + ADDREG) * 250)
> to the sensors user-space application, then you are OK with this? The
> SENSORS.CONF file would then apply one of the two lines below to give user
> the temperature in degrees Centigrade.
Yes, I'm fine with that. I don't think we can do anything better.
> > BTW, does this suggest that you decided that the diode temperature would
> > be temp1, and thermistor-based ones are temp2+? I have no objection a
> > priori, just curious.
>
> Nope. I don't care which is which. Really. If there is a general trend
> for the CPU-0 temperature to be on a specific sensor then let me know and
> I'll make the driver match.
No, the general trend is to use the same order the device itself does
because it usually allows for some code optimizations, and makes it
easier when comparing the code and the datasheet. In the case of the
VT8231, none of this really applies anyway.
> If you can send me the results for the code review of the driver then I'll
> wrap these changes up into it and re-submit it. Hopefully then it is
> complete.
Just done that :)
--
Jean Delvare
^ permalink raw reply [flat|nested] 22+ messages in thread