Hi
I am interested in this, but I only have LKML at work. Mind
sending me all the changes to 2.5.66 as one bulk patch?
Also, I did some initial patch to get w83781d support in, but
it very rough. I will try to get to doing it according to your
'How to convert i2c adapter drivers into good kernel code' ...
Who do I sent it to if completed ?
Thanks,
--
Martin Schlemmer
On Tue, Mar 25, 2003 at 10:53:14AM +0200, Martin Schlemmer wrote:
> Hi
>
> I am interested in this, but I only have LKML at work. Mind
> sending me all the changes to 2.5.66 as one bulk patch?
They should be all in the patch-2.5.66-bk1 file on kernel.org right now.
> Also, I did some initial patch to get w83781d support in, but
> it very rough. I will try to get to doing it according to your
> 'How to convert i2c adapter drivers into good kernel code' ...
> Who do I sent it to if completed ?
I have a port of this driver in my kernel tree, if you really want to
use it right now. But I'd recommend waiting on converting the chip
drivers until we get the sysfs interface conversion done, otherwise we
will have to go back and modify all files again :)
See my other post about the eeprom.c driver for more info on the sysfs
changes needed for i2c chip drivers, it isn't as simple.
But if you really want to do it, great. Send it on to lkml, and
probably the sensors list. You can cc me if you want to.
thanks,
greg k-h
Hi
Ok, this is the w83781d driver updated for 2.5.66bk2. It works
over here.
I did look at the changes needed for sysfs, but this beast have
about 6 ctl_tables, and is hairy in general. I am not sure what
is the best way to do it for the different chips, so here is what
I have until I or somebody else can do the sysfs stuff.
Regards,
--
Martin Schlemmer
Martin Schlemmer wrote:
>
> I did look at the changes needed for sysfs, but this beast have
> about 6 ctl_tables, and is hairy in general. I am not sure what
> is the best way to do it for the different chips, so here is what
> I have until I or somebody else can do the sysfs stuff.
>
I've just done this with the via686a driver. Saves about 100 lines of code.
Comments?
Jan
--- c/drivers/i2c/chips/via686a.c 2003-03-26 10:35:04.000000000 +0100
+++ b/drivers/i2c/chips/via686a.c 2003-03-26 19:57:19.000000000 +0100
@@ -394,25 +394,185 @@
unsigned short flags, int kind);
static int via686a_detach_client(struct i2c_client *client);
-static int via686a_read_value(struct i2c_client *client, u8 register);
-static void via686a_write_value(struct i2c_client *client, u8 register,
- u8 value);
+static inline int via686a_read_value(struct i2c_client *client, u8 reg)
+{
+ return (inb_p(client->addr + reg));
+}
+
+static inline void via686a_write_value(struct i2c_client *client, u8 reg,
+ u8 value)
+{
+ outb_p(value, client->addr + reg);
+}
+
static void via686a_update_client(struct i2c_client *client);
static void via686a_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 i2c_client *client = to_i2c_client(dev);
+ struct via686a_data *data = i2c_get_clientdata(client);
+ via686a_update_client(client);
+
+ return sprintf(buf,"%ld %ld %ld\n",
+ IN_FROM_REG(data->in_min[nr], nr),
+ IN_FROM_REG(data->in_max[nr], nr),
+ IN_FROM_REG(data->in[nr], nr) );
+}
+
+static ssize_t store_in(struct device *dev, const char *buf, size_t
count, int nr) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct via686a_data *data = i2c_get_clientdata(client);
+ int in_min, in_max, ret;
+ ret = sscanf(buf, "%d %d", &in_min, &in_max);
+ if (ret == -1) return -EINVAL;
+ if (ret >= 1) {
+ data->in_min[nr] = IN_TO_REG(in_min, nr);
+ via686a_write_value(client, VIA686A_REG_IN_MIN(nr), data->in_min[nr]);
+ }
+ if (ret >= 2) {
+ data->in_max[nr] = IN_TO_REG(in_max, nr);
+ via686a_write_value(client, VIA686A_REG_IN_MAX(nr), data->in_max[nr]);
+ }
+ return count;
+}
+
+#define show_in_offset(offset) \
+static ssize_t \
+show_in_##offset (struct device *dev, char *buf) \
+{ \
+ return show_in(dev, buf, 0x##offset); \
+} \
+static ssize_t store_in_##offset (struct device *dev, const char *buf,
size_t count) \
+{ \
+ return store_in(dev, buf, count, 0x##offset); \
+} \
+static DEVICE_ATTR(in##offset, S_IRUGO| S_IWUSR, show_in_##offset,
store_in_##offset)
+
+show_in_offset(0);
+show_in_offset(1);
+show_in_offset(2);
+show_in_offset(3);
+show_in_offset(4);
+
+static ssize_t show_temp(struct device *dev, char *buf, int nr) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct via686a_data *data = i2c_get_clientdata(client);
+ via686a_update_client(client);
+
+ return sprintf(buf,"%ld %ld %ld\n",
+ TEMP_FROM_REG(data->temp_over[nr]),
+ TEMP_FROM_REG(data->temp_hyst[nr]),
+ TEMP_FROM_REG10(data->temp[nr]) );
+}
+static ssize_t store_temp(struct device *dev, const char *buf, size_t
count, int nr) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct via686a_data *data = i2c_get_clientdata(client);
+ int temp_over, temp_hyst, ret;
+ printk(buf);
+ ret = sscanf(buf, "%d %d", &temp_over, &temp_hyst);
+ if (ret == -1) return -EINVAL;
+ if (ret >= 1) {
+ data->temp_over[nr] = TEMP_TO_REG(temp_over);
+ via686a_write_value(client, VIA686A_REG_TEMP_OVER(nr + 1),
data->temp_over[nr]);
+ }
+ if (ret >= 2) {
+ data->temp_hyst[nr] = TEMP_TO_REG(temp_hyst);
+ via686a_write_value(client, VIA686A_REG_TEMP_HYST(nr + 1),
data->temp_hyst[nr]);
+ }
+ return count;
+}
+#define show_temp_offset(offset) \
+static ssize_t \
+show_temp_##offset (struct device *dev, char *buf) \
+{ \
+ return show_temp(dev, buf, 0x##offset); \
+} \
+static ssize_t store_temp_##offset (struct device *dev, const char
*buf, size_t count) \
+{ \
+ return store_temp(dev, buf, count, 0x##offset); \
+} \
+static DEVICE_ATTR(temp##offset, S_IRUGO | S_IWUSR, show_temp_##offset,
store_temp_##offset)
+
+show_temp_offset(0);
+show_temp_offset(1);
+show_temp_offset(2);
+
+static ssize_t show_fan(struct device *dev, char *buf, int nr) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct via686a_data *data = i2c_get_clientdata(client);
+ via686a_update_client(client);
+
+ return sprintf(buf,"%d %d\n",
+ FAN_FROM_REG(data->fan_min[nr - 1], DIV_FROM_REG(data->fan_div[nr - 1])),
+ FAN_FROM_REG(data->fan[nr - 1], DIV_FROM_REG(data->fan_div[nr - 1])) );
+}
+static ssize_t store_fan(struct device *dev, const char *buf, size_t
count, int nr) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct via686a_data *data = i2c_get_clientdata(client);
+ int fan_min, ret;
+ ret = sscanf(buf, "%d", &fan_min);
+ if (ret == -1) return -EINVAL;
+ if (ret >= 1) {
+ data->fan_min[nr] = FAN_TO_REG(fan_min, DIV_FROM_REG(data->fan_div[nr]));
+ via686a_write_value(client, VIA686A_REG_FAN_MIN(nr+1),
data->fan_min[nr]);
+ }
+ return count;
+}
+#define show_fan_offset(offset) \
+static ssize_t \
+show_fan_##offset (struct device *dev, char *buf) \
+{ \
+ return show_fan(dev, buf, 0x##offset); \
+} \
+static ssize_t store_fan_##offset (struct device *dev, const char *buf,
size_t count) \
+{ \
+ return store_fan(dev, buf, count, 0x##offset); \
+} \
+static DEVICE_ATTR(fan##offset, S_IRUGO | S_IWUSR, show_fan_##offset,
store_fan_##offset)
+
+show_fan_offset(1);
+show_fan_offset(2);
+
+static ssize_t show_alarm(struct device *dev, char *buf) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct via686a_data *data = i2c_get_clientdata(client);
+ via686a_update_client(client);
+
+ return sprintf(buf,"%d\n", ALARMS_FROM_REG(data->alarms));
+}
+static DEVICE_ATTR(alarm, S_IRUGO | S_IWUSR, show_alarm, NULL);
+
+static ssize_t show_fan_div(struct device *dev, char *buf) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct via686a_data *data = i2c_get_clientdata(client);
+ via686a_update_client(client);
+
+ return sprintf(buf,"%d %d\n",
+ DIV_FROM_REG(data->fan_div[0]),
+ DIV_FROM_REG(data->fan_div[1]) );
+}
+static ssize_t store_fan_div(struct device *dev, const char *buf,
size_t count) {
+ struct i2c_client *client = to_i2c_client(dev);
+ struct via686a_data *data = i2c_get_clientdata(client);
+ int fan_div[2], ret, old;
-static void via686a_in(struct i2c_client *client, int operation,
- int ctl_name, int *nrels_mag, long *results);
-static void via686a_fan(struct i2c_client *client, int operation,
- int ctl_name, int *nrels_mag, long *results);
-static void via686a_temp(struct i2c_client *client, int operation,
- int ctl_name, int *nrels_mag, long *results);
-static void via686a_alarms(struct i2c_client *client, int operation,
- int ctl_name, int *nrels_mag, long *results);
-static void via686a_fan_div(struct i2c_client *client, int operation,
- int ctl_name, int *nrels_mag, long *results);
+ ret = sscanf(buf, "%d %d", &fan_div[0], &fan_div[1]);
+ if (ret == -1) return -EINVAL;
+ old = via686a_read_value(client, VIA686A_REG_FANDIV);
+ if (ret >= 2) {
+ data->fan_min[1] = DIV_TO_REG(fan_div[1]);
+ old = (old & 0x3f) | (data->fan_div[1] << 6);
+ }
+ if (ret >= 1) {
+ data->fan_min[0] = DIV_TO_REG(fan_div[0]);
+ old = (old & 0xcf) | (data->fan_div[0] << 4);
+ via686a_write_value(client, VIA686A_REG_FANDIV, old);
+ }
+ return count;
+}
+static DEVICE_ATTR(fan_div, S_IRUGO | S_IWUSR, show_fan_div,
store_fan_div);
-static int via686a_id = 0;
/* The driver. I choose to use type i2c_driver, as at is identical to both
smbus_driver and isa_driver, and clients could be of either kind */
@@ -426,95 +586,18 @@
};
-
-/* The /proc/sys entries */
-
-/* -- SENSORS SYSCTL START -- */
-#define VIA686A_SYSCTL_IN0 1000
-#define VIA686A_SYSCTL_IN1 1001
-#define VIA686A_SYSCTL_IN2 1002
-#define VIA686A_SYSCTL_IN3 1003
-#define VIA686A_SYSCTL_IN4 1004
-#define VIA686A_SYSCTL_FAN1 1101
-#define VIA686A_SYSCTL_FAN2 1102
-#define VIA686A_SYSCTL_TEMP 1200
-#define VIA686A_SYSCTL_TEMP2 1201
-#define VIA686A_SYSCTL_TEMP3 1202
-#define VIA686A_SYSCTL_FAN_DIV 2000
-#define VIA686A_SYSCTL_ALARMS 2001
-
-#define VIA686A_ALARM_IN0 0x01
-#define VIA686A_ALARM_IN1 0x02
-#define VIA686A_ALARM_IN2 0x04
-#define VIA686A_ALARM_IN3 0x08
-#define VIA686A_ALARM_TEMP 0x10
-#define VIA686A_ALARM_FAN1 0x40
-#define VIA686A_ALARM_FAN2 0x80
-#define VIA686A_ALARM_IN4 0x100
-#define VIA686A_ALARM_TEMP2 0x800
-#define VIA686A_ALARM_CHAS 0x1000
-#define VIA686A_ALARM_TEMP3 0x8000
-
-/* -- SENSORS SYSCTL END -- */
-
-/* These files are created for each detected VIA686A. This is just a
template;
- though at first sight, you might think we could use a statically
- allocated list, we need some way to get back to the parent - which
- is done through one of the 'extra' fields which are initialized
- when a new copy is allocated. */
-static ctl_table via686a_dir_table_template[] = {
- {VIA686A_SYSCTL_IN0, "in0", NULL, 0, 0644, NULL, &i2c_proc_real,
- &i2c_sysctl_real, NULL, &via686a_in},
- {VIA686A_SYSCTL_IN1, "in1", NULL, 0, 0644, NULL, &i2c_proc_real,
- &i2c_sysctl_real, NULL, &via686a_in},
- {VIA686A_SYSCTL_IN2, "in2", NULL, 0, 0644, NULL, &i2c_proc_real,
- &i2c_sysctl_real, NULL, &via686a_in},
- {VIA686A_SYSCTL_IN3, "in3", NULL, 0, 0644, NULL, &i2c_proc_real,
- &i2c_sysctl_real, NULL, &via686a_in},
- {VIA686A_SYSCTL_IN4, "in4", NULL, 0, 0644, NULL, &i2c_proc_real,
- &i2c_sysctl_real, NULL, &via686a_in},
- {VIA686A_SYSCTL_FAN1, "fan1", NULL, 0, 0644, NULL, &i2c_proc_real,
- &i2c_sysctl_real, NULL, &via686a_fan},
- {VIA686A_SYSCTL_FAN2, "fan2", NULL, 0, 0644, NULL, &i2c_proc_real,
- &i2c_sysctl_real, NULL, &via686a_fan},
- {VIA686A_SYSCTL_TEMP, "temp1", NULL, 0, 0644, NULL, &i2c_proc_real,
- &i2c_sysctl_real, NULL, &via686a_temp},
- {VIA686A_SYSCTL_TEMP2, "temp2", NULL, 0, 0644, NULL,
- &i2c_proc_real, &i2c_sysctl_real, NULL, &via686a_temp},
- {VIA686A_SYSCTL_TEMP3, "temp3", NULL, 0, 0644, NULL,
- &i2c_proc_real, &i2c_sysctl_real, NULL, &via686a_temp},
- {VIA686A_SYSCTL_FAN_DIV, "fan_div", NULL, 0, 0644, NULL,
- &i2c_proc_real, &i2c_sysctl_real, NULL, &via686a_fan_div},
- {VIA686A_SYSCTL_ALARMS, "alarms", NULL, 0, 0444, NULL,
- &i2c_proc_real, &i2c_sysctl_real, NULL, &via686a_alarms},
- {0}
-};
-
-static inline int via686a_read_value(struct i2c_client *client, u8 reg)
-{
- return (inb_p(client->addr + reg));
-}
-
-static inline void via686a_write_value(struct i2c_client *client, u8 reg,
- u8 value)
-{
- outb_p(value, client->addr + reg);
-}
-
/* This is called when the module is loaded */
static int via686a_attach_adapter(struct i2c_adapter *adapter)
{
return i2c_detect(adapter, &addr_data, via686a_detect);
}
-int via686a_detect(struct i2c_adapter *adapter, int address,
+static int via686a_detect(struct i2c_adapter *adapter, int address,
unsigned short flags, int kind)
{
- int i;
struct i2c_client *new_client;
struct via686a_data *data;
int err = 0;
- const char *type_name = "via686a";
const char client_name[] = "via686a chip";
u16 val;
@@ -573,28 +656,31 @@
/* Fill in the remaining client fields and put into the global list */
snprintf(new_client->dev.name, DEVICE_NAME_SIZE, client_name);
- new_client->id = via686a_id++;
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 ERROR3;
-
- /* Register a new directory entry with module sensors */
- if ((i = i2c_register_entry((struct i2c_client *) new_client,
- type_name,
- via686a_dir_table_template)) < 0) {
- err = i;
- goto ERROR4;
- }
- data->sysctl_id = i;
+
+ device_create_file(&new_client->dev, &dev_attr_in0);
+ device_create_file(&new_client->dev, &dev_attr_in1);
+ device_create_file(&new_client->dev, &dev_attr_in2);
+ device_create_file(&new_client->dev, &dev_attr_in3);
+ device_create_file(&new_client->dev, &dev_attr_in4);
+ device_create_file(&new_client->dev, &dev_attr_temp0);
+ device_create_file(&new_client->dev, &dev_attr_temp1);
+ device_create_file(&new_client->dev, &dev_attr_temp2);
+ device_create_file(&new_client->dev, &dev_attr_fan1);
+ device_create_file(&new_client->dev, &dev_attr_fan2);
+ device_create_file(&new_client->dev, &dev_attr_alarm);
+ device_create_file(&new_client->dev, &dev_attr_fan_div);
/* Initialize the VIA686A chip */
via686a_init_client(new_client);
return 0;
- ERROR4:
- i2c_detach_client(new_client);
+// ERROR4:
+// i2c_detach_client(new_client);
ERROR3:
release_region(address, VIA686A_EXTENT);
kfree(new_client);
@@ -605,8 +691,8 @@
static int via686a_detach_client(struct i2c_client *client)
{
int err;
- struct via686a_data *data = i2c_get_clientdata(client);
- i2c_deregister_entry(data->sysctl_id);
+// struct via686a_data *data = i2c_get_clientdata(client);
+// i2c_deregister_entry(data->sysctl_id);
if ((err = i2c_detach_client(client))) {
dev_err(&client->dev,
@@ -739,157 +825,12 @@
up(&data->update_lock);
}
-
-/* The next few functions are the call-back functions of the /proc/sys and
- sysctl files. Which function is used is defined in the ctl_table in
- the extra1 field.
- Each function must return the magnitude (power of 10 to divide the date
- with) if it is called with operation==SENSORS_PROC_REAL_INFO. It must
- put a maximum of *nrels elements in results reflecting the data of this
- file, and set *nrels to the number it actually put in it, if operation==
- SENSORS_PROC_REAL_READ. Finally, it must get upto *nrels elements from
- results and write them to the chip, if
operations==SENSORS_PROC_REAL_WRITE.
- Note that on SENSORS_PROC_REAL_READ, I do not check whether results is
- large enough (by checking the incoming value of *nrels). This is not
very
- good practice, but as long as you put less than about 5 values in
results,
- you can assume it is large enough. */
-static void via686a_in(struct i2c_client *client, int operation, int
ctl_name,
- int *nrels_mag, long *results)
-{
- struct via686a_data *data = i2c_get_clientdata(client);
- int nr = ctl_name - VIA686A_SYSCTL_IN0;
-
- if (operation == SENSORS_PROC_REAL_INFO)
- *nrels_mag = 2;
- else if (operation == SENSORS_PROC_REAL_READ) {
- via686a_update_client(client);
- results[0] = IN_FROM_REG(data->in_min[nr], nr);
- results[1] = IN_FROM_REG(data->in_max[nr], nr);
- results[2] = IN_FROM_REG(data->in[nr], nr);
- *nrels_mag = 3;
- } else if (operation == SENSORS_PROC_REAL_WRITE) {
- if (*nrels_mag >= 1) {
- data->in_min[nr] = IN_TO_REG(results[0], nr);
- via686a_write_value(client, VIA686A_REG_IN_MIN(nr),
- data->in_min[nr]);
- }
- if (*nrels_mag >= 2) {
- data->in_max[nr] = IN_TO_REG(results[1], nr);
- via686a_write_value(client, VIA686A_REG_IN_MAX(nr),
- data->in_max[nr]);
- }
- }
-}
-
-void via686a_fan(struct i2c_client *client, int operation, int ctl_name,
- int *nrels_mag, long *results)
-{
- struct via686a_data *data = i2c_get_clientdata(client);
- int nr = ctl_name - VIA686A_SYSCTL_FAN1 + 1;
-
- if (operation == SENSORS_PROC_REAL_INFO)
- *nrels_mag = 0;
- else if (operation == SENSORS_PROC_REAL_READ) {
- via686a_update_client(client);
- results[0] = FAN_FROM_REG(data->fan_min[nr - 1],
- DIV_FROM_REG(data->fan_div
- [nr - 1]));
- results[1] = FAN_FROM_REG(data->fan[nr - 1],
- DIV_FROM_REG(data->fan_div[nr - 1]));
- *nrels_mag = 2;
- } else if (operation == SENSORS_PROC_REAL_WRITE) {
- if (*nrels_mag >= 1) {
- data->fan_min[nr - 1] = FAN_TO_REG(results[0],
- DIV_FROM_REG(data->
- fan_div[nr -1]));
- via686a_write_value(client,
- VIA686A_REG_FAN_MIN(nr),
- data->fan_min[nr - 1]);
- }
- }
-}
-
-void via686a_temp(struct i2c_client *client, int operation, int ctl_name,
- int *nrels_mag, long *results)
-{
- struct via686a_data *data = i2c_get_clientdata(client);
- int nr = ctl_name - VIA686A_SYSCTL_TEMP;
-
- if (operation == SENSORS_PROC_REAL_INFO)
- *nrels_mag = 1;
- else if (operation == SENSORS_PROC_REAL_READ) {
- via686a_update_client(client);
- results[0] = TEMP_FROM_REG(data->temp_over[nr]);
- results[1] = TEMP_FROM_REG(data->temp_hyst[nr]);
- results[2] = TEMP_FROM_REG10(data->temp[nr]);
- *nrels_mag = 3;
- } else if (operation == SENSORS_PROC_REAL_WRITE) {
- if (*nrels_mag >= 1) {
- data->temp_over[nr] = TEMP_TO_REG(results[0]);
- via686a_write_value(client,
- VIA686A_REG_TEMP_OVER(nr + 1),
- data->temp_over[nr]);
- }
- if (*nrels_mag >= 2) {
- data->temp_hyst[nr] = TEMP_TO_REG(results[1]);
- via686a_write_value(client,
- VIA686A_REG_TEMP_HYST(nr + 1),
- data->temp_hyst[nr]);
- }
- }
-}
-
-void via686a_alarms(struct i2c_client *client, int operation, int ctl_name,
- int *nrels_mag, long *results)
-{
- struct via686a_data *data = i2c_get_clientdata(client);
- if (operation == SENSORS_PROC_REAL_INFO)
- *nrels_mag = 0;
- else if (operation == SENSORS_PROC_REAL_READ) {
- via686a_update_client(client);
- results[0] = ALARMS_FROM_REG(data->alarms);
- *nrels_mag = 1;
- }
-}
-
-void via686a_fan_div(struct i2c_client *client, int operation,
- int ctl_name, int *nrels_mag, long *results)
-{
- struct via686a_data *data = i2c_get_clientdata(client);
- int old;
-
- if (operation == SENSORS_PROC_REAL_INFO)
- *nrels_mag = 0;
- else if (operation == SENSORS_PROC_REAL_READ) {
- via686a_update_client(client);
- results[0] = DIV_FROM_REG(data->fan_div[0]);
- results[1] = DIV_FROM_REG(data->fan_div[1]);
- *nrels_mag = 2;
- } else if (operation == SENSORS_PROC_REAL_WRITE) {
- old = via686a_read_value(client, VIA686A_REG_FANDIV);
- if (*nrels_mag >= 2) {
- data->fan_div[1] = DIV_TO_REG(results[1]);
- old = (old & 0x3f) | (data->fan_div[1] << 6);
- }
- if (*nrels_mag >= 1) {
- data->fan_div[0] = DIV_TO_REG(results[0]);
- old = (old & 0xcf) | (data->fan_div[0] << 4);
- via686a_write_value(client, VIA686A_REG_FANDIV,
- old);
- }
- }
-}
-
-
static struct pci_device_id via686a_pci_ids[] __devinitdata = {
{
.vendor = PCI_VENDOR_ID_VIA,
.device = PCI_DEVICE_ID_VIA_82C686_4,
.subvendor = PCI_ANY_ID,
.subdevice = PCI_ANY_ID,
- .class = 0,
- .class_mask = 0,
- .driver_data = 0,
},
{ 0, }
};
On Wed, 2003-03-26 at 21:40, Jan Dittmer wrote:
> Martin Schlemmer wrote:
> >
> > I did look at the changes needed for sysfs, but this beast have
> > about 6 ctl_tables, and is hairy in general. I am not sure what
> > is the best way to do it for the different chips, so here is what
> > I have until I or somebody else can do the sysfs stuff.
> >
> I've just done this with the via686a driver. Saves about 100 lines of code.
>
> Comments?
>
Nice example, thanks =) The w83781d does things in similar fashion,
except that it support 6 different 'models', that have more or less
the same readouts except for here and there a few more or less.
Regards,
--
Martin Schlemmer
On Wed, Mar 26, 2003 at 08:40:58PM +0100, Jan Dittmer wrote:
> Martin Schlemmer wrote:
> >
> >I did look at the changes needed for sysfs, but this beast have
> >about 6 ctl_tables, and is hairy in general. I am not sure what
> >is the best way to do it for the different chips, so here is what
> >I have until I or somebody else can do the sysfs stuff.
> >
> I've just done this with the via686a driver. Saves about 100 lines of code.
>
> Comments?
<snip>
> +/* following are the sysfs callback functions */
> +static ssize_t show_in(struct device *dev, char *buf, int nr) {
> + struct i2c_client *client = to_i2c_client(dev);
> + struct via686a_data *data = i2c_get_clientdata(client);
> + via686a_update_client(client);
> +
> + return sprintf(buf,"%ld %ld %ld\n",
> + IN_FROM_REG(data->in_min[nr], nr),
> + IN_FROM_REG(data->in_max[nr], nr),
> + IN_FROM_REG(data->in[nr], nr) );
> +}
We should really split these multivalue files up into individual files,
as sysfs is for single value files. Makes parsing easier too.
Here's a patch for the lm75.c driver that does this. As we are going to
need a "generic" read and write for the "real" values that the i2c
drivers use, I added these files to the i2c-proc.c file.
Yes, i2c_read_real() doesn't really work just yet :)
Anyway, I think this is the direction we should be moving to. And what
is "temp_os" and "temp_hyst"? Should these files be named something a
bit more descriptive?
thanks,
greg k-h
diff -Nru a/drivers/i2c/chips/lm75.c b/drivers/i2c/chips/lm75.c
--- a/drivers/i2c/chips/lm75.c Wed Mar 26 12:28:45 2003
+++ b/drivers/i2c/chips/lm75.c Wed Mar 26 12:28:45 2003
@@ -102,6 +102,49 @@
static int lm75_id = 0;
+#define show(value) \
+static ssize_t show_##value(struct device *dev, char *buf) \
+{ \
+ struct i2c_client *client = to_i2c_client(dev); \
+ struct lm75_data *data = i2c_get_clientdata(client); \
+ int temp; \
+ \
+ lm75_update_client(client); \
+ temp = TEMP_FROM_REG(data->value); \
+ return i2c_write_real(buf, temp, 1); \
+}
+show(temp);
+show(temp_os);
+show(temp_hyst);
+
+static ssize_t set_temp_os(struct device *dev, const char *buf, size_t count)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct lm75_data *data = i2c_get_clientdata(client);
+ int temp;
+
+ i2c_read_real(buf, &temp, 1);
+ data->temp_os = TEMP_TO_REG(temp);
+ lm75_write_value(client, LM75_REG_TEMP_OS, data->temp_os);
+ return count;
+}
+
+static ssize_t set_temp_hyst(struct device *dev, const char *buf, size_t count)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct lm75_data *data = i2c_get_clientdata(client);
+ int temp;
+
+ i2c_read_real(buf, &temp, 1);
+ data->temp_hyst = TEMP_TO_REG(temp);
+ lm75_write_value(client, LM75_REG_TEMP_HYST, data->temp_hyst);
+ return count;
+}
+
+static DEVICE_ATTR(temp, S_IRUGO, show_temp, NULL);
+static DEVICE_ATTR(temp_os, 0644, show_temp_os, set_temp_os);
+static DEVICE_ATTR(temp_hyst, 0644, show_temp_hyst, set_temp_hyst);
+
static int lm75_attach_adapter(struct i2c_adapter *adapter)
{
return i2c_detect(adapter, &addr_data, lm75_detect);
@@ -192,6 +235,10 @@
if ((err = i2c_attach_client(new_client)))
goto error3;
+ device_create_file(&new_client->dev, &dev_attr_temp);
+ device_create_file(&new_client->dev, &dev_attr_temp_os);
+ device_create_file(&new_client->dev, &dev_attr_temp_hyst);
+
/* Register a new directory entry with module sensors */
i = i2c_register_entry(new_client, type_name, lm75_dir_table_template);
if (i < 0) {
diff -Nru a/drivers/i2c/i2c-proc.c b/drivers/i2c/i2c-proc.c
--- a/drivers/i2c/i2c-proc.c Wed Mar 26 12:28:45 2003
+++ b/drivers/i2c/i2c-proc.c Wed Mar 26 12:28:45 2003
@@ -381,6 +381,14 @@
return ret;
}
+int i2c_read_real(const char *buf, int *real, int magnitude)
+{
+ char *temp;
+
+ *real = simple_strtoul(buf, &temp, 10);
+
+ return 0;
+}
/* nrels contains initially the maximum number of elements which can be
put in results, and finally the number of elements actually put there.
@@ -499,6 +507,29 @@
return 0;
}
+int i2c_write_real(char *buf, int real, int magnitude)
+{
+ char printfstr[12];
+ int mag;
+ int times;
+ int field_location;
+
+ mag = magnitude;
+ for (times = 1; mag-- > 0; times *= 10)
+ ;
+
+ if (real < 0) {
+ strcpy(printfstr, "-%ld.%0Xld");
+ field_location = 7;
+ } else {
+ strcpy(printfstr, "%ld.%0Xld");
+ field_location = 6;
+ }
+ printfstr[field_location] = magnitude + '0';
+ real = abs(real);
+ return sprintf(buf, printfstr, real / times, real % times);
+}
+
static int i2c_write_reals(int nrels, void *buffer, size_t *bufsize,
long *results, int magnitude)
{
@@ -724,6 +755,8 @@
EXPORT_SYMBOL(i2c_proc_real);
EXPORT_SYMBOL(i2c_sysctl_real);
EXPORT_SYMBOL(i2c_detect);
+EXPORT_SYMBOL(i2c_write_real);
+EXPORT_SYMBOL(i2c_read_real);
MODULE_AUTHOR("Frodo Looijaard <[email protected]>");
MODULE_DESCRIPTION("i2c-proc driver");
diff -Nru a/include/linux/i2c-proc.h b/include/linux/i2c-proc.h
--- a/include/linux/i2c-proc.h Wed Mar 26 12:28:45 2003
+++ b/include/linux/i2c-proc.h Wed Mar 26 12:28:45 2003
@@ -410,5 +410,8 @@
char name[SENSORS_PREFIX_MAX + 13];
};
+extern int i2c_write_real(char *buf, int real, int magnitude);
+extern int i2c_read_real(const char *buf, int *real, int magnitude);
+
#endif /* def _LINUX_I2C_PROC_H */
On Wed, Mar 26, 2003 at 12:26:23PM -0800, Greg KH wrote:
> We should really split these multivalue files up into individual files,
> as sysfs is for single value files. Makes parsing easier too.
>
> Here's a patch for the lm75.c driver that does this. As we are going to
> need a "generic" read and write for the "real" values that the i2c
> drivers use, I added these files to the i2c-proc.c file.
i2c-proc.c is the wrong place. Please add a i2c-sensor.c file with
helper code for hardware sensors driver (i2c_detect should move over to
there from i2c-proc.c aswell)
On Wed, Mar 26, 2003 at 09:04:33PM +0200, Martin Schlemmer wrote:
> Hi
>
> Ok, this is the w83781d driver updated for 2.5.66bk2. It works
> over here.
Looks nice, thanks.
Some of the nasty casts should be fixed up though. Stuff like:
> + ERROR7:
> + if (!is_isa)
> + i2c_detach_client(&
> + (((struct w83781d_data
> + *) (i2c_get_clientdata(new_client)))->
> + lm75[1]));
> + ERROR6:
> + if (!is_isa)
> + i2c_detach_client(&
> + (((struct w83781d_data
> + *) (i2c_get_clientdata(new_client)))->
> + lm75[0]));
> + ERROR5:
> + if (!is_isa)
> + kfree(((struct w83781d_data *) (i2c_get_clientdata(new_client)))->
> + lm75);
Is just obnoxious :)
I'll hold off sending this driver to Linus until it gets cleaned up with
sysfs entries, as I'd rather not pollute /proc and sysctls anymore.
thanks,
greg k-h
On Wed, Mar 26, 2003 at 08:43:43PM +0000, Christoph Hellwig wrote:
> On Wed, Mar 26, 2003 at 12:26:23PM -0800, Greg KH wrote:
> > We should really split these multivalue files up into individual files,
> > as sysfs is for single value files. Makes parsing easier too.
> >
> > Here's a patch for the lm75.c driver that does this. As we are going to
> > need a "generic" read and write for the "real" values that the i2c
> > drivers use, I added these files to the i2c-proc.c file.
>
> i2c-proc.c is the wrong place. Please add a i2c-sensor.c file with
> helper code for hardware sensors driver (i2c_detect should move over to
> there from i2c-proc.c aswell)
Oh, I agree. I just threw it there as I was matching up with other
functions already in that file. Eventually i2c-proc.c should be
deleted, as all of the proc stuff will be gone. I like the idea of
i2c-sensor.c for the remaining i2c_detect() function.
thanks,
greg k-h
If you rename the files and/or split multivalue files into separate
single value files, or change the format of the contents,
and continue these changes across the 30 or so "chip" drivers of ours,
you will completely blow up our libsensors library, and userspace programs.
If all the patches do is move all the files unchanged from
/proc/sys/dev/sensors/... to /sysfs/... then that change is much much easier
to incorporate in our programs.
While not all drivers conform perfectly to the our standard (link below)
(lm75 temp_os and temp_hyst don't), this is the naming and
data format standard we attempt to follow. This has evolved
over the years. If the chip drivers in the kernel
diverge from this, or even worse diverge from each other haphazardly,
we're going to end up with a mess and no usable userspace tools
for a long long time.
Please consider keeping the file names and contents unchanged.
thanks
mds
------------
lm_sensors /proc naming standars for sensors chip drivers:
http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/doc/developers/proc
Greg KH wrote:
>
> On Wed, Mar 26, 2003 at 08:40:58PM +0100, Jan Dittmer wrote:
> > Martin Schlemmer wrote:
> > >
> > >I did look at the changes needed for sysfs, but this beast have
> > >about 6 ctl_tables, and is hairy in general. I am not sure what
> > >is the best way to do it for the different chips, so here is what
> > >I have until I or somebody else can do the sysfs stuff.
> > >
> > I've just done this with the via686a driver. Saves about 100 lines of code.
> >
> > Comments?
>
> <snip>
>
> > +/* following are the sysfs callback functions */
> > +static ssize_t show_in(struct device *dev, char *buf, int nr) {
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct via686a_data *data = i2c_get_clientdata(client);
> > + via686a_update_client(client);
> > +
> > + return sprintf(buf,"%ld %ld %ld\n",
> > + IN_FROM_REG(data->in_min[nr], nr),
> > + IN_FROM_REG(data->in_max[nr], nr),
> > + IN_FROM_REG(data->in[nr], nr) );
> > +}
>
> We should really split these multivalue files up into individual files,
> as sysfs is for single value files. Makes parsing easier too.
>
> Here's a patch for the lm75.c driver that does this.
Subject changed to reflect change in topic, and to call attention to
others who might be interested.
On Wed, Mar 26, 2003 at 05:26:54PM -0500, Mark Studebaker wrote:
> If you rename the files and/or split multivalue files into separate
> single value files, or change the format of the contents,
> and continue these changes across the 30 or so "chip" drivers of ours,
> you will completely blow up our libsensors library, and userspace programs.
Well ideally libsensors can change and then userspace programs will
never know the difference :)
I need to start digging into the libsensors code to be able to back this
up, but at first glance I think it's feasible.
> If all the patches do is move all the files unchanged from
> /proc/sys/dev/sensors/... to /sysfs/... then that change is much much easier
> to incorporate in our programs.
True, but multi-valued files are not allowed in sysfs. It's especially
obnoxious that you have 3 value files when you read them, but only
expect 2 values for writing. The way I split up the values in the
lm75.c driver shows a user exactly which values are writable, and
enforces this on the file system level.
I don't want to detract from all of the work you, and the lm_sensors
group has done in the past, it's quite nice. I'm just trying to fit the
drivers into current kernel policies in the best way possible.
Remember, I want this to work, and for everyone to come to an
understanding.
> lm_sensors /proc naming standars for sensors chip drivers:
>
> http://www2.lm-sensors.nu/~lm78/cvs/lm_sensors2/doc/developers/proc
Thanks for the link. I don't really think this is a problem. As an
example, the temp files, which are currently defined as a proc file with
this description:
Entry Values Function
----- ------ --------
temp,
temp[1-3] 3 Temperature max, min or hysteresis, and input value.
Floating point values XXX.X or XXX.XX in degrees Celcius.
'temp' is used if there is only one temperature sensor on the
chip; for multiple temps. start with 'temp1'.
Temp1 is generally the sensor inside the chip itself,
generally reported as "motherboard temperature".
Temp2 and temp3 are generally sensors external to the chip
itself, for example the thermal diode inside the CPU or a
termistor nearby. The second value is preferably a hysteresis
value, reported as a absolute temperature, NOT a delta from
the max value.
First two values are read/write and third is read only.
Could be rewritten to say:
Entry Function
----- --------
temp_max[1-3] Temperature max value.
Fixed point value in form XXXXX and should be divided by
100 to get degrees Celsius.
Read/Write value.
temp_min[1-3] Temperature min or hysteresis value.
Fixed point value in form XXXXX and should be divided by
100 to get degrees Celsius. This is preferably a
hysteresis value, reported as a absolute temperature, NOT
a delta from the max value.
Read/Write value.
temp_input[1-3] Temperature input value.
Read only value.
If there are multiple temperature sensors, temp_*1 is
generally the sensor inside the chip itself, generally
reported as "motherboard temperature". temp_*2 and
temp_*3 are generally sensors external to the chip
itself, for example the thermal diode inside the CPU or a
thermistor nearby.
That would give us one value per file, use no floating point in the
kernel (fake or not) and generally make things a whole lot more orderly.
Also, if a sensor does not have a max value (for example, I don't really
know if this is true), instead of having to fake a value, it can just
not create the file. Then userspace can easily detect this is not
supported, and is not a placeholder value.
Does that sound reasonable?
thanks,
greg k-h
On Wed, 2003-03-26 at 22:29, Greg KH wrote:
> Some of the nasty casts should be fixed up though. Stuff like:
>
> > + ERROR7:
> > + if (!is_isa)
> > + i2c_detach_client(&
> > + (((struct w83781d_data
> > + *) (i2c_get_clientdata(new_client)))->
> > + lm75[1]));
> > + ERROR6:
> > + if (!is_isa)
> > + i2c_detach_client(&
> > + (((struct w83781d_data
> > + *) (i2c_get_clientdata(new_client)))->
> > + lm75[0]));
> > + ERROR5:
> > + if (!is_isa)
> > + kfree(((struct w83781d_data *) (i2c_get_clientdata(new_client)))->
> > + lm75);
>
> Is just obnoxious :)
>
Quick question .... With sysfs, is it not needed to call
i2c_detach_client ? I am asking this as it seems from patches
that you remove all these calls ...
Regards,
--
Martin Schlemmer
On Thu, Mar 27, 2003 at 01:34:32AM +0200, Martin Schlemmer wrote:
>
> Quick question .... With sysfs, is it not needed to call
> i2c_detach_client ? I am asking this as it seems from patches
> that you remove all these calls ...
No, that's still required. I didn't delete that in my lm75.c patch, did
I?
You just don't have to call i2c_deregister_entry, as we don't call
i2c_register_entry anymore.
thanks,
greg k-h
Greg KH wrote:
> True, but multi-valued files are not allowed in sysfs. It's especially
> obnoxious that you have 3 value files when you read them, but only
> expect 2 values for writing. The way I split up the values in the
> lm75.c driver shows a user exactly which values are writable, and
> enforces this on the file system level.
Agreed, I never knew which of the three values had which functionality.
For via686a this would be inX, inX_min, inX_max, tempX, tempX_overshoot
(over = overshoot = os ?), tempX_hyst, and so on.
> Entry Values Function
> ----- ------ --------
> temp,
> temp[1-3] 3 Temperature max, min or hysteresis, and input value.
> Floating point values XXX.X or XXX.XX in degrees Celcius.
If we're restructuring it, I think we should also agree on _one_ common
denominator for all values ie. mVolt and milli-Degree Celsius, so that
no userspace program ever again has know how to convert them to
user-readable values and every user can just cat the values and doesn't
have to wonder if it's centi-Volt, milli-Volt, centi-Degree, dezi-Degree
or whatever.
Thanks,
Jan
On Thu, 2003-03-27 at 12:46, Jan Dittmer wrote:
> Greg KH wrote:
> > True, but multi-valued files are not allowed in sysfs. It's especially
> > obnoxious that you have 3 value files when you read them, but only
> > expect 2 values for writing. The way I split up the values in the
> > lm75.c driver shows a user exactly which values are writable, and
> > enforces this on the file system level.
>
> Agreed, I never knew which of the three values had which functionality.
> For via686a this would be inX, inX_min, inX_max, tempX, tempX_overshoot
> (over = overshoot = os ?), tempX_hyst, and so on.
>
> > Entry Values Function
> > ----- ------ --------
> > temp,
> > temp[1-3] 3 Temperature max, min or hysteresis, and input value.
> > Floating point values XXX.X or XXX.XX in degrees Celcius.
>
> If we're restructuring it, I think we should also agree on _one_ common
> denominator for all values ie. mVolt and milli-Degree Celsius, so that
> no userspace program ever again has know how to convert them to
> user-readable values and every user can just cat the values and doesn't
> have to wonder if it's centi-Volt, milli-Volt, centi-Degree, dezi-Degree
> or whatever.
>
Right, can we just get this finalised soon ? Not much fun in redoing
something 2 times already ;)
Regards,
--
Martin Schlemmer
Martin Schlemmer wrote:
> On Thu, 2003-03-27 at 12:46, Jan Dittmer wrote:
>
>>Greg KH wrote:
>>
>>>True, but multi-valued files are not allowed in sysfs. It's especially
>>>obnoxious that you have 3 value files when you read them, but only
>>>expect 2 values for writing. The way I split up the values in the
>>>lm75.c driver shows a user exactly which values are writable, and
>>>enforces this on the file system level.
>>
>
> Right, can we just get this finalised soon ? Not much fun in redoing
> something 2 times already ;)
>
Btw., why is temperature conversion done twice? First time in kernel
space and then with the help of sensors.conf again in user space?
Wouldn't it be much nicer to move this to the drivers? So there would be
no need anymore to do this in userspace and one could rely on the values
found in sysfs?!
Thanks,
Jan
On Thu, 2003-03-27 at 14:27, Jan Dittmer wrote:
> Btw., why is temperature conversion done twice? First time in kernel
> space and then with the help of sensors.conf again in user space?
> Wouldn't it be much nicer to move this to the drivers? So there would be
> no need anymore to do this in userspace and one could rely on the values
> found in sysfs?!
>
I guess for stuff like fan_div, etc at least, is that not all the
chips are on spec, the sensors are located on different places,
and the why the manufacturer of the board did things could all
influence things. Thus you need to be able to tweak fan_dev,
etc for each board individually to get the most precise reading.
For instance, what my bios say, and what the raw reading from
/proc is, is two different things. I also had to slightly adjust
things between some models of Asus boards I had.
Regards,
--
Martin Schlemmer
Martin Schlemmer wrote:
> For instance, what my bios say, and what the raw reading from
> /proc is, is two different things. I also had to slightly adjust
> things between some models of Asus boards I had.
>
But why not return always the raw data then and don't do any conversion
at all? I mean, if we already try to guess the real value in the driver,
we should try to get it right.
Ah, and I don't want to do small corrections like 1 or 2 percent up or
down, but things like this for lm80:
compute in0 (24/14.7 + 1) * @ , @ / (24/14.7 + 1)
compute in2 (22.1/30 + 1) * @ , @ / (22.1/30 + 1)
compute in3 (2.8/1.9) * @, @ * 1.9/2.8
compute in4 (160/30.1 + 1) * @, @ / (160/30.1 + 1)
These differ a lot. And as stated in the sensors.conf seem to be from
the datasheet of the lm80 and not mainboard specific.
So that the sysfs values at least are near reality.
Thanks,
Jan
> Ah, and I don't want to do small corrections like 1 or 2 percent up or
> or down, but things like this for lm80:
> compute in0 (24/14.7 + 1) * @ , @ / (24/14.7 + 1)
> compute in2 (22.1/30 + 1) * @ , @ / (22.1/30 + 1)
> compute in3 (2.8/1.9) * @, @ * 1.9/2.8
> compute in4 (160/30.1 + 1) * @, @ / (160/30.1 + 1)
>
> These differ a lot. And as stated in the sensors.conf seem to be from
> the datasheet of the lm80 and not mainboard specific.
They may be mainboard specific. The formulae rely on *suggested*
resistor values, which the integrators may or may not follow. There
definitely is a need for userspace conversion. Libsensors needs to be
tunable by the user.
I don't know about the kernel conversion Jan is talking about (never
coded a sensor driver myself), so I can't talk about this half, but I'm
pretty sure you shouldn't even think of moving all conversions into
kernel space.
Mark, Phil, maybe you could explain the reasons better than I would?
--
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
On Thu, Mar 27, 2003 at 11:46:16AM +0100, Jan Dittmer wrote:
> >Entry Values Function
> >----- ------ --------
> >temp,
> >temp[1-3] 3 Temperature max, min or hysteresis, and input value.
> > Floating point values XXX.X or XXX.XX in degrees Celcius.
>
> If we're restructuring it, I think we should also agree on _one_ common
> denominator for all values ie. mVolt and milli-Degree Celsius, so that
> no userspace program ever again has know how to convert them to
> user-readable values and every user can just cat the values and doesn't
> have to wonder if it's centi-Volt, milli-Volt, centi-Degree, dezi-Degree
> or whatever.
Um, that's what my proposal stated. Do you not agree with it? (You're
quoting the existing document above, not my proposed changes.)
thanks,
greg k-h
* Jean Delvare <[email protected]> [2003-03-27 14:31:40 +0100]:
> > Ah, and I don't want to do small corrections like 1 or 2 percent up or
> > or down, but things like this for lm80:
> > compute in0 (24/14.7 + 1) * @ , @ / (24/14.7 + 1)
> > compute in2 (22.1/30 + 1) * @ , @ / (22.1/30 + 1)
> > compute in3 (2.8/1.9) * @, @ * 1.9/2.8
> > compute in4 (160/30.1 + 1) * @, @ / (160/30.1 + 1)
> >
> > These differ a lot. And as stated in the sensors.conf seem to be from
> > the datasheet of the lm80 and not mainboard specific.
>
> They may be mainboard specific. The formulae rely on *suggested*
> resistor values, which the integrators may or may not follow. There
> definitely is a need for userspace conversion. Libsensors needs to be
> tunable by the user.
>
> I don't know about the kernel conversion Jan is talking about (never
> coded a sensor driver myself), so I can't talk about this half, but I'm
> pretty sure you shouldn't even think of moving all conversions into
> kernel space.
>
> Mark, Phil, maybe you could explain the reasons better than I would?
I'm not the Mark to which Jean refers, but anyway... he is correct.
The sensor chip cannot read temperatures directly, only voltage. The
conversion from degress C to V is dependent on the mainboard for the
reasons Jean mentions.
But also, the chip driver cannot read voltage directly, only bits in a
register. *That* conversion is *not* mainboard dependent; it is
sensor chip specific.
Even when the subject in question is voltage (e.g. power supply +12V),
there is mainboard dependent nonsense between the "real value" and what
is presented at the pin of the sensor chip.
So there is a legitimate need for two conversions. IMHO, the
mainboard dependent one *must* be done in userspace (as Jean says).
But the conversion of raw register values to volts should happen in
the kernel; fixed point maths are sufficient for that.
(Yes I'm simplifying; external sensors can present data to sensors
chips in other electrical formats, e.g. PWM. Same argument applies.)
Regards,
--
Mark M. Hoffman
[email protected]
Greg KH wrote:
>>If we're restructuring it, I think we should also agree on _one_ common
>>denominator for all values ie. mVolt and milli-Degree Celsius, so that
>>no userspace program ever again has know how to convert them to
>>user-readable values and every user can just cat the values and doesn't
>>have to wonder if it's centi-Volt, milli-Volt, centi-Degree, dezi-Degree
>>or whatever.
>
>
> Um, that's what my proposal stated. Do you not agree with it? (You're
> quoting the existing document above, not my proposed changes.)
I just wanted to emphasis that _all_ units should be milli oder centi.
Not mixing centiDegrees and milliVolts or one driver using milliVolt and
another centiVolt.
From your description it could well be, that one driver uses centi's
and another milli's, both for voltage or one driver uses milliVolt but
centi-degree.
Thanks,
Jan
On Thu, Mar 27, 2003 at 07:06:08PM +0100, Jan Dittmer wrote:
> Greg KH wrote:
> >>If we're restructuring it, I think we should also agree on _one_ common
> >>denominator for all values ie. mVolt and milli-Degree Celsius, so that
> >>no userspace program ever again has know how to convert them to
> >>user-readable values and every user can just cat the values and doesn't
> >>have to wonder if it's centi-Volt, milli-Volt, centi-Degree, dezi-Degree
> >>or whatever.
> >
> >Um, that's what my proposal stated. Do you not agree with it? (You're
> >quoting the existing document above, not my proposed changes.)
>
> I just wanted to emphasis that _all_ units should be milli oder centi.
> Not mixing centiDegrees and milliVolts or one driver using milliVolt and
> another centiVolt.
I agree.
> From your description it could well be, that one driver uses centi's
> and another milli's, both for voltage or one driver uses milliVolt but
> centi-degree.
Huh? I said:
temp_max[1-3] Temperature max value.
Fixed point value in form XXXXX and should be divided by
100 to get degrees Celsius.
Read/Write value.
Where is the ability to use a different scale from different drivers in
that?
Anyway, it sounds like we are agreeing here, so I guess I'll go and
write up the whole document in the new style and post it for comments.
thanks,
greg k-h
Greg KH wrote:
> That would give us one value per file, use no floating point in the
> kernel (fake or not) and generally make things a whole lot more orderly.
> Also, if a sensor does not have a max value (for example, I don't really
> know if this is true), instead of having to fake a value, it can just
> not create the file. Then userspace can easily detect this is not
> supported, and is not a placeholder value.
>
Is this the way you want to go? Just an example for the voltages.
Btw, is it indended behaviour of sysfs, that after writing to a file,
the size is zero?
ds666:/sys/devices/legacy/i2c-0/i2c_dev_0# echo 100 > in4_min
ds666:/sys/devices/legacy/i2c-0/i2c_dev_0# ls -l in4_min
-rw-r--r-- 1 root root 0 Mar 27 19:18 in4_min
-r--r--r-- 1 root root 4096 Mar 27 19:18 in0_input
-rw-r--r-- 1 root root 4096 Mar 27 19:18 in0_max
-rw-r--r-- 1 root root 4096 Mar 27 19:18 in0_min
-r--r--r-- 1 root root 4096 Mar 27 19:18 in1_input
-rw-r--r-- 1 root root 4096 Mar 27 19:18 in1_max
-rw-r--r-- 1 root root 4096 Mar 27 19:18 in1_min
-r--r--r-- 1 root root 4096 Mar 27 19:18 in2_input
-rw-r--r-- 1 root root 4096 Mar 27 19:18 in2_max
-rw-r--r-- 1 root root 4096 Mar 27 19:18 in2_min
-r--r--r-- 1 root root 4096 Mar 27 19:18 in3_input
-rw-r--r-- 1 root root 4096 Mar 27 19:18 in3_max
-rw-r--r-- 1 root root 4096 Mar 27 19:18 in3_min
-r--r--r-- 1 root root 4096 Mar 27 19:18 in4_input
-rw-r--r-- 1 root root 4096 Mar 27 19:18 in4_max
-rw-r--r-- 1 root root 4096 Mar 27 19:18 in4_min
/* 7 voltage sensors */
static ssize_t show_in(struct device *dev, char *buf, int nr) {
struct i2c_client *client = to_i2c_client(dev);
struct via686a_data *data = i2c_get_clientdata(client);
via686a_update_client(client);
return sprintf(buf,"%ld\n", IN_FROM_REG(data->in[nr], nr) );
}
static ssize_t show_in_min(struct device *dev, char *buf, int nr) {
struct i2c_client *client = to_i2c_client(dev);
struct via686a_data *data = i2c_get_clientdata(client);
via686a_update_client(client);
return sprintf(buf,"%ld\n", IN_FROM_REG(data->in_min[nr], nr) );
}
static ssize_t show_in_max(struct device *dev, char *buf, int nr) {
struct i2c_client *client = to_i2c_client(dev);
struct via686a_data *data = i2c_get_clientdata(client);
via686a_update_client(client);
return sprintf(buf,"%ld\n", IN_FROM_REG(data->in_max[nr], 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 via686a_data *data = i2c_get_clientdata(client);
unsigned long val = simple_strtoul(buf, NULL, 10);
data->in_min[nr] = IN_TO_REG(val,nr);
via686a_write_value(client, VIA686A_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 via686a_data *data = i2c_get_clientdata(client);
unsigned long val = simple_strtoul(buf, NULL, 10);
data->in_max[nr] = IN_TO_REG(val,nr);
via686a_write_value(client, VIA686A_REG_IN_MAX(nr),
data->in_max[nr]);
return count;
}
#define show_in_offset(offset) \
static ssize_t \
show_in##offset (struct device *dev, char *buf) \
{ \
return show_in(dev, buf, 0x##offset); \
} \
static ssize_t \
show_in##offset##_min (struct device *dev, char *buf) \
{ \
return show_in_min(dev, buf, 0x##offset); \
} \
static ssize_t \
show_in##offset##_max (struct device *dev, char *buf) \
{ \
return show_in_max(dev, buf, 0x##offset); \
} \
static ssize_t set_in##offset##_min (struct device *dev, \
const char *buf, size_t count) \
{ \
return set_in_min(dev, buf, count, 0x##offset); \
} \
static ssize_t set_in##offset##_max (struct device *dev, \
const char *buf, size_t count) \
{ \
return set_in_max(dev, buf, count, 0x##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);
On Thu, Mar 27, 2003 at 07:40:26PM +0100, Jan Dittmer wrote:
> Greg KH wrote:
> >That would give us one value per file, use no floating point in the
> >kernel (fake or not) and generally make things a whole lot more orderly.
> >Also, if a sensor does not have a max value (for example, I don't really
> >know if this is true), instead of having to fake a value, it can just
> >not create the file. Then userspace can easily detect this is not
> >supported, and is not a placeholder value.
> >
>
> Is this the way you want to go? Just an example for the voltages.
That looks very good to me, nice job.
Sensors developers, does this look sane?
> Btw, is it indended behaviour of sysfs, that after writing to a file,
> the size is zero?
Hm, don't know about that, I haven't seen that before. If you cat the
file after writing it, does the file size change?
thanks,
greg k-h
[ stripped some cc's ]
Greg KH wrote:
> On Thu, Mar 27, 2003 at 07:40:26PM +0100, Jan Dittmer wrote:
>
>>Btw, is it indended behaviour of sysfs, that after writing to a file,
>>the size is zero?
>
>
> Hm, don't know about that, I haven't seen that before. If you cat the
> file after writing it, does the file size change?
>
No it stays 0. Happens also with other files in sysfs.
Jan
> > Btw, is it indended behaviour of sysfs, that after writing to a file,
> > the size is zero?
>
> Hm, don't know about that, I haven't seen that before. If you cat the
> file after writing it, does the file size change?
It's a known problem that I haven't been able to track down.
The file size is hardwired to PAGE_SIZE when it is created. For some
reason, it's reset to 0 after it's opened. I haven't had a chance to track
down why, or how to reset it..
-pat
On Thu, 2003-03-27 at 20:52, Greg KH wrote:
> > Is this the way you want to go? Just an example for the voltages.
>
> That looks very good to me, nice
While we are at it, some form question. The w83781d have a
magnitude of files in sysfs if you split them like this, so
I went for the shorter (easier?) way.
This ok, or should I split it up a bit more. Note that I
have not done much for indentation yet.
------------------------------------------------------
#define show_in_reg(reg) \
static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
{ \
struct i2c_client *client = to_i2c_client(dev); \
struct w83781d_data *data = i2c_get_clientdata(client); \
w83781d_update_client(client); \
\
return sprintf(buf,"%ld\n", \
IN_FROM_REG(data->reg[nr])); \
}
show_in_reg(in);
show_in_reg(in_min);
show_in_reg(in_max);
#define store_in_reg(REG,reg) \
static ssize_t store_##reg (struct device *dev, const char *buf, size_t
count, int nr) \
{ \
struct i2c_client *client = to_i2c_client(dev); \
struct w83781d_data *data = i2c_get_clientdata(client); \
int reg, ret; \
\
ret = sscanf(buf, "%d", ®); \
if (ret == -1) return -EINVAL; \
if (ret >= 1) { \
data->reg[nr] = IN_TO_REG(reg); \
w83781d_write_value(client, W83781D_REG_IN_##REG(nr),
data->reg[nr]); \
} \
return count; \
}
store_in_reg(MIN, in_min);
store_in_reg(MAX, in_max);
#define show_in_offset(offset) \
static ssize_t \
show_in_##offset (struct device *dev, char *buf) \
{ \
return show_in(dev, buf, 0x##offset); \
} \
static DEVICE_ATTR(in_input##offset, S_IRUGO | S_IWUSR,
show_in_##offset, NULL)
#define show_in_reg_offset(reg,offset) \
static ssize_t show_##reg##offset (struct device *dev, char *buf) \
{ \
return show_##reg (dev, buf, 0x##offset); \
} \
static ssize_t store_##reg##offset (struct device *dev, const char *buf,
size_t count) \
{ \
return store_##reg (dev, buf, count, 0x##offset); \
} \
static DEVICE_ATTR(##reg##offset, S_IRUGO| S_IWUSR, show_##reg##offset,
store_##reg##offset)
#define show_in_offsets(offset) \
show_in_offset(offset); \
show_in_reg_offset(in_min, offset); \
show_in_reg_offset(in_max, offset);
show_in_offsets(0);
show_in_offsets(1);
show_in_offsets(2);
show_in_offsets(3);
show_in_offsets(4);
show_in_offsets(5);
show_in_offsets(6);
show_in_offsets(7);
show_in_offsets(8);
--
Martin Schlemmer
On Thu, Mar 27, 2003 at 09:15:23PM +0200, Martin Schlemmer wrote:
> On Thu, 2003-03-27 at 20:52, Greg KH wrote:
>
> > > Is this the way you want to go? Just an example for the voltages.
> >
> > That looks very good to me, nice
>
> While we are at it, some form question. The w83781d have a
> magnitude of files in sysfs if you split them like this, so
> I went for the shorter (easier?) way.
>
> This ok, or should I split it up a bit more. Note that I
> have not done much for indentation yet.
This is fine with me, whatever works for you. Either way, we are
abusing macros a bunch :)
thanks,
greg k-h
Ok, I've modified the /proc file document to reflect the proposed sysfs
file changes and included it below.
Any comments? Any objections?
thanks,
greg k-h
Greg KH wrote:
> fan_min[1-3] Fan minimum value
> in_min[0-8] Voltage min value.
> pwn[1-3] Pulse width modulation fan control.
> temp_input[1-3] Temperature input value.
Why not start all these counts from 0? Is there any reason to start from
1? Historical reasons or does the datasheet start the counting from 1?
Jan
On Thu, Mar 27, 2003 at 09:32:46PM +0100, Jan Dittmer wrote:
> Greg KH wrote:
>
> >fan_min[1-3] Fan minimum value
> >in_min[0-8] Voltage min value.
> >pwn[1-3] Pulse width modulation fan control.
> >temp_input[1-3] Temperature input value.
> Why not start all these counts from 0? Is there any reason to start from
> 1? Historical reasons or does the datasheet start the counting from 1?
Hm, good point. It looks like most of the values started at 1, with the
one exception being the voltage files "in".
I don't know why it's this way, history? Perhaps someone from the
sensors project can tell us.
thanks,
greg k-h
* Greg KH <[email protected]> [2003-03-27 13:53:44 -0800]:
> On Thu, Mar 27, 2003 at 09:32:46PM +0100, Jan Dittmer wrote:
> > Greg KH wrote:
> >
> > >fan_min[1-3] Fan minimum value
> > >in_min[0-8] Voltage min value.
> > >pwn[1-3] Pulse width modulation fan control.
> > >temp_input[1-3] Temperature input value.
> > Why not start all these counts from 0? Is there any reason to start from
> > 1? Historical reasons or does the datasheet start the counting from 1?
>
> Hm, good point. It looks like most of the values started at 1, with the
> one exception being the voltage files "in".
>
> I don't know why it's this way, history? Perhaps someone from the
> sensors project can tell us.
Hardware designers tend to count devices starting with "1". Silly them. :)
Regards,
--
Mark M. Hoffman
[email protected]
Greg KH writes:
> temp_max[1-3] Temperature max value.
> Fixed point value in form XXXXX and
> should be divided by
> 100 to get degrees Celsius.
> Read/Write value.
Celsius can go negative, which may be yucky
and hard to test. Kelvin generally doesn't
suffer this problem. (yeah, yeah, quantum stuff...)
Getting temperature display into "top" would sure
be nice, but not if that means requiring a library
that almost nobody has installed. It's good to give
apps a simple way to get CPU temperature, including
per-CPU data for SMP systems when available.
Info about sensor quality would be good. For example,
my CPU measures temperature in 4-degree increments
and is not calibrated.
On Thu, Mar 27, 2003 at 06:00:51PM -0500, Albert Cahalan wrote:
> Greg KH writes:
>
> > temp_max[1-3] Temperature max value.
> > Fixed point value in form XXXXX and
> > should be divided by
> > 100 to get degrees Celsius.
> > Read/Write value.
>
> Celsius can go negative, which may be yucky
> and hard to test. Kelvin generally doesn't
> suffer this problem. (yeah, yeah, quantum stuff...)
Wow, only 4 hours before someone mentioned Kelvin, I think I lost a bet
with someone :)
Seriously, let the value go negative, no problem. As long as it isn't
floating point input which has to be parsed by the kernel. That's all I
care about.
> Getting temperature display into "top" would sure
> be nice, but not if that means requiring a library
> that almost nobody has installed. It's good to give
> apps a simple way to get CPU temperature, including
> per-CPU data for SMP systems when available.
libsensors is installed on almost all distros these days.
> Info about sensor quality would be good. For example,
> my CPU measures temperature in 4-degree increments
> and is not calibrated.
I doubt the kernel driver knows this information.
thanks,
greg k-h
On Thu, 2003-03-27 at 23:53, Greg KH wrote:
> On Thu, Mar 27, 2003 at 09:32:46PM +0100, Jan Dittmer wrote:
> > Greg KH wrote:
> >
> > >fan_min[1-3] Fan minimum value
> > >in_min[0-8] Voltage min value.
> > >pwn[1-3] Pulse width modulation fan control.
> > >temp_input[1-3] Temperature input value.
> > Why not start all these counts from 0? Is there any reason to start from
> > 1? Historical reasons or does the datasheet start the counting from 1?
>
> Hm, good point. It looks like most of the values started at 1, with the
> one exception being the voltage files "in".
>
> I don't know why it's this way, history? Perhaps someone from the
> sensors project can tell us.
>
Might be because you usually have marked on the boards fan1, fan2, etc.
Thus it should be less confusing for the user, as it is a more 1-1
mapping to the real locations. As the voltage inputs just have to be
mapped correctly, it should not bother him ?
Regards,
--
Martin Schlemmer
On Fri, 2003-03-28 at 01:10, Greg KH wrote:
> On Thu, Mar 27, 2003 at 06:00:51PM -0500, Albert Cahalan wrote:
> > Greg KH writes:
> >
> > > temp_max[1-3] Temperature max value.
> > > Fixed point value in form XXXXX and
> > > should be divided by
> > > 100 to get degrees Celsius.
> > > Read/Write value.
> >
> > Celsius can go negative, which may be yucky
> > and hard to test. Kelvin generally doesn't
> > suffer this problem. (yeah, yeah, quantum stuff...)
>
> Wow, only 4 hours before someone mentioned Kelvin, I think I lost a bet
> with someone :)
>
> Seriously, let the value go negative, no problem. As long as it isn't
> floating point input which has to be parsed by the kernel. That's all I
> care about.
>
Silly w83781d again. temp1 is a u8, and temp2 and temp3 is u16
(if they are supported on the specific model.
Should we do any bounds checking on input via sysfs ?
Regards,
--
Martin Schlemmer
On Fri, Mar 28, 2003 at 09:21:48AM +0200, Martin Schlemmer wrote:
>
> Silly w83781d again. temp1 is a u8, and temp2 and temp3 is u16
> (if they are supported on the specific model.
>
> Should we do any bounds checking on input via sysfs ?
So that you can't hurt your hardware or crash the os, yes.
I think the write ability is limited to root, so that's a good first
step too.
thanks,
greg k-h
Hi
This should have a working w83781d driver updated for 2.5.66-bk4.
Currently sysfs support is working, and are according to the spec
(sensors-sysfs) in the 'lm sensors sysfs file structure' thread.
Thus I used 'temp_input[1-3]', as there was not final decision
on having them temp_input[0-2] as well, for example.
This patch is against a 'vanilla' 2.5.66-bk4 (do not have w83781d.c
from lm_sensors2 cvs present). Retry, as previous did not go through
due to size.
Regards,
--
Martin Schlemmer
Hi!
> > > Floating point values XXX.X or XXX.XX in degrees Celcius.
> >
> > If we're restructuring it, I think we should also agree on _one_ common
> > denominator for all values ie. mVolt and milli-Degree Celsius, so that
> > no userspace program ever again has know how to convert them to
> > user-readable values and every user can just cat the values and doesn't
> > have to wonder if it's centi-Volt, milli-Volt, centi-Degree, dezi-Degree
> > or whatever.
>
> Um, that's what my proposal stated. Do you not agree with it? (You're
> quoting the existing document above, not my proposed changes.)
Well, you had cV for PSU voltages and
mV for cpu core voltage. I guess mV
and mili-deg-C everywhere would be
nicer.
Pavel
--
Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...
Hi!
> temp_min[1-3] Temperature min or hysteresis value.
> Fixed point value in form XXXXX and should be divided by
> 100 to get degrees Celsius. This is preferably a
I believe floating point is nicer... It is
improbable to have miliKelvins around,
still I don'n see why fixedpoint...
Pavel
--
Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...