Hi Yanu
> > If you could come up with an additional demonstration patch for
> > either the lm90 driver or the it87 driver, I would happily give it
> > a try. The adm1026 has very few users AFAIK so it might not be the
> > best choice to find testers, although I agree that it was a
> > convenient way to demonstrate how drivers could benefit from the
> > proposed change.
>
> Here is a patch against it87, since it seems to have many more device
> attributes than lm90. The size difference:
>
> before:
> Module Size Used by
> it87 28832 0
> after:
> Module Size Used by
> it87 22432 0
>
> That isn't bad, but the code cleanup is worth it alone in my opinion.
I finally gave a try to your patches, including the one for it87 which I
used for testing. It all works like a charm, pretty impressive
considering the overall complexity of the change. Congratulations :)
I'd like to add that the technical solution you came up with pleases me
much (which may or may not be relevant).
If we are into code refactoring and driver size shrinking, you may want
to take a look at the following patch, which makes it87 even smaller
(from 18976 bytes down to 16992 bytes on my system) and IMHO more
cleaner. I do voluntarily not sign it, it's not meant for immediate
inclusion, I'd rather see your changes in mainstream before I start
pushing my own ones. It's simply meant as a proof that your proposed big
change doesn't prevent further improvements, which is great :) Basically
the idea is to use arrays for the device sysfs attributes, so that we
then can use a loop to instantiate them instead of having to instantiate
each one individually. The same could be done in many other hardware
monitoring drivers as well, of course (which is why I defined the helper
macros in i2c-sysfs.h).
And once again... great great work Yani :)
Thanks.
drivers/i2c/chips/it87.c | 227 ++++++++++++++++++++++------------------------
include/linux/i2c-sysfs.h | 10 ++
2 files changed, 119 insertions(+), 118 deletions(-)
Index: linux-2.6.12-rc4/drivers/i2c/chips/it87.c
===================================================================
--- linux-2.6.12-rc4.orig/drivers/i2c/chips/it87.c 2005-05-19 19:13:52.000000000 +0200
+++ linux-2.6.12-rc4/drivers/i2c/chips/it87.c 2005-05-19 21:33:49.000000000 +0200
@@ -304,33 +304,40 @@
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);
-show_in_offset(6);
-limit_in_offset(6);
-show_in_offset(7);
-limit_in_offset(7);
-show_in_offset(8);
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(in_input, 9)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in0_input, S_IRUGO, show_in, NULL, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in1_input, S_IRUGO, show_in, NULL, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in2_input, S_IRUGO, show_in, NULL, 2)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in3_input, S_IRUGO, show_in, NULL, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in4_input, S_IRUGO, show_in, NULL, 4)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in5_input, S_IRUGO, show_in, NULL, 5)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in6_input, S_IRUGO, show_in, NULL, 6)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in7_input, S_IRUGO, show_in, NULL, 7)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in8_input, S_IRUGO, show_in, NULL, 8)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(in_min, 8)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in0_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in1_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in2_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 2)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in3_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in4_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 4)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in5_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 5)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in6_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 6)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in7_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 7)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(in_max, 8)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in0_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in1_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in2_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 2)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in3_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in4_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 4)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in5_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 5)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in6_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 6)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in7_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 7)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
/* 3 temperatures */
static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
@@ -392,17 +399,25 @@
up(&data->update_lock);
return count;
}
-#define show_temp_offset(offset) \
-static SENSOR_DEVICE_ATTR(temp##offset##_input, S_IRUGO, \
- show_temp, NULL, offset - 1); \
-static SENSOR_DEVICE_ATTR(temp##offset##_max, S_IRUGO | S_IWUSR, \
- show_temp_max, set_temp_max, offset - 1); \
-static SENSOR_DEVICE_ATTR(temp##offset##_min, S_IRUGO | S_IWUSR, \
- show_temp_min, set_temp_min, offset - 1);
-
-show_temp_offset(1);
-show_temp_offset(2);
-show_temp_offset(3);
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(temp_input, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp1_input, S_IRUGO, show_temp, NULL, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp2_input, S_IRUGO, show_temp, NULL, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp3_input, S_IRUGO, show_temp, NULL, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(temp_min, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp1_min, S_IRUGO | S_IWUSR, show_temp_min, set_temp_min, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp2_min, S_IRUGO | S_IWUSR, show_temp_min, set_temp_min, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp3_min, S_IRUGO | S_IWUSR, show_temp_min, set_temp_min, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(temp_max, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp1_max, S_IRUGO | S_IWUSR, show_temp_max, set_temp_max, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp2_max, S_IRUGO | S_IWUSR, show_temp_max, set_temp_max, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp3_max, S_IRUGO | S_IWUSR, show_temp_max, set_temp_max, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
static ssize_t show_sensor(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -451,13 +466,13 @@
up(&data->update_lock);
return count;
}
-#define show_sensor_offset(offset) \
-static SENSOR_DEVICE_ATTR(temp##offset##_type, S_IRUGO | S_IWUSR, \
- show_sensor, set_sensor, offset - 1);
-
-show_sensor_offset(1);
-show_sensor_offset(2);
-show_sensor_offset(3);
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(temp_type, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp1_type, S_IRUGO | S_IWUSR, show_sensor, set_sensor, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp2_type, S_IRUGO | S_IWUSR, show_sensor, set_sensor, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp3_type, S_IRUGO | S_IWUSR, show_sensor, set_sensor, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
/* 3 Fans */
static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
@@ -619,27 +634,36 @@
return count;
}
-#define show_fan_offset(offset) \
-static SENSOR_DEVICE_ATTR(fan##offset##_input, S_IRUGO, \
- show_fan, NULL, offset - 1); \
-static SENSOR_DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR, \
- show_fan_min, set_fan_min, offset - 1); \
-static SENSOR_DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR, \
- show_fan_div, set_fan_div, offset - 1);
-
-show_fan_offset(1);
-show_fan_offset(2);
-show_fan_offset(3);
-
-#define show_pwm_offset(offset) \
-static SENSOR_DEVICE_ATTR(pwm##offset##_enable, S_IRUGO | S_IWUSR, \
- show_pwm_enable, set_pwm_enable, offset - 1); \
-static SENSOR_DEVICE_ATTR(pwm##offset, S_IRUGO | S_IWUSR, \
- show_pwm , set_pwm, offset - 1);
-
-show_pwm_offset(1);
-show_pwm_offset(2);
-show_pwm_offset(3);
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(fan_input, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan1_input, S_IRUGO, show_fan, NULL, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan2_input, S_IRUGO, show_fan, NULL, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan3_input, S_IRUGO, show_fan, NULL, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(fan_min, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan1_min, S_IRUGO | S_IWUSR, show_fan_min, set_fan_min, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan2_min, S_IRUGO | S_IWUSR, show_fan_min, set_fan_min, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan3_min, S_IRUGO | S_IWUSR, show_fan_min, set_fan_min, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(fan_div, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan1_div, S_IRUGO | S_IWUSR, show_fan_div, set_fan_div, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan2_div, S_IRUGO | S_IWUSR, show_fan_div, set_fan_div, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan3_div, S_IRUGO | S_IWUSR, show_fan_div, set_fan_div, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(pwm, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm2, S_IRUGO | S_IWUSR, show_pwm, set_pwm, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm3, S_IRUGO | S_IWUSR, show_pwm, set_pwm, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(pwm_enable, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm1_enable, S_IRUGO | S_IWUSR, show_pwm_enable, set_pwm_enable, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm2_enable, S_IRUGO | S_IWUSR, show_pwm_enable, set_pwm_enable, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm3_enable, S_IRUGO | S_IWUSR, show_pwm_enable, set_pwm_enable, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
/* Alarms */
static ssize_t show_alarms(struct device *dev, struct device_attribute *attr, char *buf)
@@ -843,60 +867,27 @@
it87_init_client(new_client, data);
/* Register sysfs hooks */
- device_create_file(&new_client->dev, &sensor_dev_attr_in0_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in1_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in2_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in3_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in4_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in5_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in6_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in7_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in8_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in0_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in1_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in2_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in3_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in4_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in5_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in6_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in7_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in0_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in1_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in2_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in3_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in4_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in5_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in6_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in7_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp1_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp2_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp3_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp1_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp2_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp3_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp1_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp2_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp3_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp1_type.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp2_type.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp3_type.dev_attr);
- 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_fan3_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_fan3_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, &sensor_dev_attr_fan3_div.dev_attr);
+ for (i=0; i<8; i++) {
+ device_create_file(&new_client->dev, &sensor_dev_attr_in_input[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in_min[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in_max[i].dev_attr);
+ }
+ device_create_file(&new_client->dev, &sensor_dev_attr_in_input[8].dev_attr);
+ for (i=0; i<3; i++) {
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp_input[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp_min[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp_max[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp_type[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan_input[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan_min[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan_div[i].dev_attr);
+ }
device_create_file(&new_client->dev, &dev_attr_alarms);
if (enable_pwm_interface) {
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm1_enable.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm2_enable.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm3_enable.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm1.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm2.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm3.dev_attr);
+ for (i=0; i<3; i++) {
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm_enable[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm[i].dev_attr);
+ }
}
if (data->type == it8712) {
Index: linux-2.6.12-rc4/include/linux/i2c-sysfs.h
===================================================================
--- linux-2.6.12-rc4.orig/include/linux/i2c-sysfs.h 2005-05-19 19:13:52.000000000 +0200
+++ linux-2.6.12-rc4/include/linux/i2c-sysfs.h 2005-05-19 20:40:21.000000000 +0200
@@ -34,4 +34,14 @@
.index=_index, \
}
+#define SENSOR_DEVICE_ATTR_ARRAY_HEAD(_name,_size) \
+struct sensor_device_attribute sensor_dev_attr_##_name[_size] = {
+
+#define SENSOR_DEVICE_ATTR_ARRAY_ITEM(_name,_mode,_show,_store,_index) \
+ { .dev_attr=__ATTR(_name,_mode,_show,_store), \
+ .index=_index, },
+
+#define SENSOR_DEVICE_ATTR_ARRAY_TAIL \
+}
+
#endif /* _LINUX_I2C_SYSFS_H */
--
Jean Delvare
On Thu, May 19, 2005 at 10:02:35PM +0200, Jean Delvare wrote:
> If we are into code refactoring and driver size shrinking, you may want
> to take a look at the following patch, which makes it87 even smaller
> (from 18976 bytes down to 16992 bytes on my system) and IMHO more
> cleaner.
But this doesn't reduce the binary size of the module, right?
> /* Register sysfs hooks */
> - device_create_file(&new_client->dev, &sensor_dev_attr_in0_input.dev_attr);
> - device_create_file(&new_client->dev, &sensor_dev_attr_in1_input.dev_attr);
> - device_create_file(&new_client->dev, &sensor_dev_attr_in2_input.dev_attr);
> - device_create_file(&new_client->dev, &sensor_dev_attr_in3_input.dev_attr);
<snip>
You know, we do have arrays of attributes that can be registered with a
single call...
I'd recommend using that over this mess anyday :)
> +#define SENSOR_DEVICE_ATTR_ARRAY_HEAD(_name,_size) \
> +struct sensor_device_attribute sensor_dev_attr_##_name[_size] = {
> +
> +#define SENSOR_DEVICE_ATTR_ARRAY_ITEM(_name,_mode,_show,_store,_index) \
> + { .dev_attr=__ATTR(_name,_mode,_show,_store), \
> + .index=_index, },
> +
> +#define SENSOR_DEVICE_ATTR_ARRAY_TAIL \
> +}
No, I hate HEAD and TAIL macros. This really isn't buying you much code
savings, you could do it yourself with the __ATTR() macro yourself with
the same ammount of code I bet...
Or use the new macro that Yani created, that will make it even smaller
:)
thanks,
greg k-h
Hi Greg,
> > If we are into code refactoring and driver size shrinking, you may
> > want to take a look at the following patch, which makes it87 even
> > smaller (from 18976 bytes down to 16992 bytes on my system) and IMHO
> > more cleaner.
>
> But this doesn't reduce the binary size of the module, right?
It does, as I just said. The benefit is probably mainly due to the
introduction of loops around device_create_file() calls. The patch
reduces the number of calls (in the binary) from 59 to 20.
> You know, we do have arrays of attributes that can be registered with
> a single call...
>
> I'd recommend using that over this mess anyday :)
Yeah, I'll take a look into this at some point. This should make the
code even more readable and efficient.
> No, I hate HEAD and TAIL macros. This really isn't buying you much
> code savings, you could do it yourself with the __ATTR() macro
> yourself with the same ammount of code I bet...
>
> Or use the new macro that Yani created, that will make it even smaller
> :)
Agreed. This was really a quick hack, not meant for inclusion. Maybe I
should have polished it a bit more before I dared sending it. I'll do so
next time, sorry for the noise.
--
Jean Delvare
On 5/19/05, Jean Delvare <[email protected]> wrote:
> I finally gave a try to your patches, including the one for it87 which I
> used for testing. It all works like a charm, pretty impressive
> considering the overall complexity of the change. Congratulations :)
I'm impressed too ;), and very happy to finally have confirmation it
works as planned without blowing up anything.
> I'd like to add that the technical solution you came up with pleases me
> much (which may or may not be relevant).
Well I guess a happy Jean doesn't hurt! I really can't accept full
credit for this though, I think the end solution was a good mix of
ideas/comments from a range of people especially Greg, Russell and
you.
I like the idea of aggregating the related sensor attributes into
arrays, I did things the same way in bmcsensors but of course that was
all dynamic, and would just use more memory if we tried to do that
here. As Greg points out there is probably a nicer way to create the
arrays and register the attributes, but the basic idea has merit I
think, and if we can standardize a method across all the sensors then
its even better.
Thanks,
Yani
On Thu, May 19, 2005 at 10:57:12PM +0200, Jean Delvare wrote:
> Hi Greg,
>
> > > If we are into code refactoring and driver size shrinking, you may
> > > want to take a look at the following patch, which makes it87 even
> > > smaller (from 18976 bytes down to 16992 bytes on my system) and IMHO
> > > more cleaner.
> >
> > But this doesn't reduce the binary size of the module, right?
>
> It does, as I just said. The benefit is probably mainly due to the
> introduction of loops around device_create_file() calls. The patch
> reduces the number of calls (in the binary) from 59 to 20.
Ah, sorry, I mistook that for a code decrease and not binary decrease.
> > No, I hate HEAD and TAIL macros. This really isn't buying you much
> > code savings, you could do it yourself with the __ATTR() macro
> > yourself with the same ammount of code I bet...
> >
> > Or use the new macro that Yani created, that will make it even smaller
> > :)
>
> Agreed. This was really a quick hack, not meant for inclusion. Maybe I
> should have polished it a bit more before I dared sending it. I'll do so
> next time, sorry for the noise.
No, don't feel like this was noise at all, it wasn't. I was just
commenting on the patch, letting you know that it was a great place to
start, but it might be tweaked a bit in places.
Don't worry about polishing stuff up before sending it in, you have seen
some of my patches, right? :)
Also, there is a neat trick that you can do every once in a while if you
use it sparingly. Propose a patch that you know is wrong, just to get
someone else (usually the maintainer of the area) to do it correctly as
they don't like your way at all. It's very effective when used in small
doses.
Hm, which makes me want to go look at trying to convert those attributes
to an array right now...
thanks,
greg k-h
Hi Greg,
> Hm, which makes me want to go look at trying to convert those attributes
> to an array right now...
I should probably develop my own thoughts and plans at this point then.
If you consider all the attributes of a given hardware monitoring driver,
you'll notice that, from a functional point of view, they can be
represented as two-dimension arrays rather than one-dimension ones. For
example, the temperature-related files of the it87 driver could be
represented this way:
+-------------+-------------+-------------+-------------+
| temp1_input | temp1_min | temp1_max | temp1_type |
+-------------+-------------+-------------+-------------+
| temp2_input | temp2_min | temp2_max | temp2_type |
+-------------+-------------+-------------+-------------+
| temp3_input | temp3_min | temp3_max | temp3_type |
+-------------+-------------+-------------+-------------+
In the patch I just proposed, I made the choice to consider the colums of
the array above to create arrays of attributes, each column becoming a
different array. While it may sound like a sane choice, espcially when
there are many similar channels (e.g. voltages), it might not be the
best choice in the long run, for the following reason.
All measurement channels of the IT8705F and IT8712F can be individually
disabled. While this feature was almost not used so far, I think we
should have the driver not create interface files for disabled inputs.
In the case of temperature channels which can be dynamically enabled and
disabled. it would even make sense to dynamically create and delete
related sysfs files. Doing so would allow for memory savings and would
also be less error-prone for the user (presenting an interface for
disabled features is quite confusing IMHO).
For this reason, considering the lines of the array above, rather than
its columns, in order to define arrays of attributes, may be more subtle
and flexible in the long run.
Thanks
--
Jean Delvare
On 5/20/05, Jean Delvare <[email protected]> wrote:
>
> Hi Greg,
> disabled. While this feature was almost not used so far, I think we
> should have the driver not create interface files for disabled inputs.
> In the case of temperature channels which can be dynamically enabled and
> disabled. it would even make sense to dynamically create and delete
> related sysfs files. Doing so would allow for memory savings and would
> also be less error-prone for the user (presenting an interface for
> disabled features is quite confusing IMHO).
If you think more than a few hwmon/chip drivers will benefit from
dynamically creating the attributes, then maybe we can create some
standard method for doing so, bmcsensors of course needs to
dynamically allocate things, so I'd be using them too.
An earlier idea was to create a standard sysfs function(s) for
dynamically creating device_attributes (and others), but now we will
have custom structures with an embedded device_attribute, it looks to
me like each attribute type would require it's own function.
Yani
On Friday 20 May 2005 03:53, Yani Ioannou wrote:
> On 5/20/05, Jean Delvare <[email protected]> wrote:
> >
> > Hi Greg,
> > disabled. While this feature was almost not used so far, I think we
> > should have the driver not create interface files for disabled inputs.
> > In the case of temperature channels which can be dynamically enabled and
> > disabled. it would even make sense to dynamically create and delete
> > related sysfs files. Doing so would allow for memory savings and would
> > also be less error-prone for the user (presenting an interface for
> > disabled features is quite confusing IMHO).
>
> If you think more than a few hwmon/chip drivers will benefit from
> dynamically creating the attributes, then maybe we can create some
> standard method for doing so, bmcsensors of course needs to
> dynamically allocate things, so I'd be using them too.
>
> An earlier idea was to create a standard sysfs function(s) for
> dynamically creating device_attributes (and others), but now we will
> have custom structures with an embedded device_attribute, it looks to
> me like each attribute type would require it's own function.
>
I really think that as far as I2C subsystem goes instead of creating
arrays of attributes we should move in direction of drivers registering
individual sensor class devices. So for example it87 would register
3 fans, 3 temp, sensors and 8 voltage sensors...
--
Dmitry
Hi Dmitry,
> I really think that as far as I2C subsystem goes instead of creating
> arrays of attributes we should move in direction of drivers
> registering individual sensor class devices. So for example it87 would
> register 3 fans, 3 temp, sensors and 8 voltage sensors...
First, it's a matter of hardware monitoring drivers, not i2c subsystem
(both are tightly binded at the moment but I'd like this to change).
Second, not all devices have the same attributes for a temperature, fan
or voltage channel. Sure there are commonly found feature sets, but some
channels will lack some feature (e.g. it87's in8 has no min and max
limits), other chips will provide additional features (extra limits or
enhanced configurability). So I don't think you can have all devices
(and thus all drivers) fit into a single sensor class.
But of course I can be convinced your approach is better, with patches.
I don't know classes that well myself.
Thanks,
--
Jean Delvare
On Sunday 22 May 2005 01:50, Jean Delvare wrote:
> Hi Dmitry,
>
> > I really think that as far as I2C subsystem goes instead of creating
> > arrays of attributes we should move in direction of drivers
> > registering individual sensor class devices. So for example it87 would
> > register 3 fans, 3 temp, sensors and 8 voltage sensors...
>
> First, it's a matter of hardware monitoring drivers, not i2c subsystem
> (both are tightly binded at the moment but I'd like this to change).
>
Right, it's just i2c is pretty much the only supplier of these for now.
> Second, not all devices have the same attributes for a temperature, fan
> or voltage channel. Sure there are commonly found feature sets, but some
> channels will lack some feature (e.g. it87's in8 has no min and max
> limits), other chips will provide additional features (extra limits or
> enhanced configurability). So I don't think you can have all devices
> (and thus all drivers) fit into a single sensor class.
>
Well, userspace code manages it somehow, plus nothing stops driver from
adding some additional attributes to class devices.
> But of course I can be convinced your approach is better, with patches.
Heh, I was afraid you'd say so... Input sysfs conversion first and then
we'll see...
--
Dmitry
On 5/22/05, Dmitry Torokhov <[email protected]> wrote:
> On Sunday 22 May 2005 01:50, Jean Delvare wrote:
> > Hi Dmitry,
> >
> > > I really think that as far as I2C subsystem goes instead of creating
> > > arrays of attributes we should move in direction of drivers
> > > registering individual sensor class devices. So for example it87 would
> > > register 3 fans, 3 temp, sensors and 8 voltage sensors...
Well lets see some code ;-), but yeah I have to agree with Jean here,
I don't see a nice way to standardize this across all hwmon drivers,
things just differ too much.
> > First, it's a matter of hardware monitoring drivers, not i2c subsystem
> > (both are tightly binded at the moment but I'd like this to change).
How is that change going anyway? I could really do with something
finalized, but the last I heard about it was Mark Hoffman's patch and
that didn't seem to go anywhere.
Yani
Hi Yani,
> > > First, it's a matter of hardware monitoring drivers, not i2c
> > > subsystem (both are tightly binded at the moment but I'd like this
> > > to change).
>
> How is that change going anyway? I could really do with something
> finalized, but the last I heard about it was Mark Hoffman's patch and
> that didn't seem to go anywhere.
My bad, I didn't take the time to review Mark's work yet :(
Anyway there's a long long way to go before there is a true separation
between i2c and hwmon. Mark introduced a hwmon class, which is needed
but not sufficient. The biggest part of the work will be to move all
drivers abusing the i2c subsystem to the subsystem where they really
belong (isa/platform or superio), and get rid of i2c-isa. The hybrid
drivers (w83781d, it87 and lm78) promise to be no fun to convert. We'll
split them if that's what it takes.
--
Jean Delvare
> Anyway there's a long long way to go before there is a true separation
> between i2c and hwmon. Mark introduced a hwmon class, which is needed
> but not sufficient. The biggest part of the work will be to move all
> drivers abusing the i2c subsystem to the subsystem where they really
> belong (isa/platform or superio), and get rid of i2c-isa.
And i2c-ipmi remember? :) Does that mean Mark's class is finalized and
I can work with it? Or is there some more work to be done on that
front too? If so I can probably help out.
Yani
Hi Yani,
> And i2c-ipmi remember? :) Does that mean Mark's class is finalized and
> I can work with it? Or is there some more work to be done on that
> front too? If so I can probably help out.
I can't tell, I didn't even give his code a try :( If you have some
spare time, try applying the proposed patches, see how they work for you
and how you could use the new class in bmcsensors.
However, as I understand it, the hwmon class will be added to the
drivers, it isn't meant to replace anything. This will simply let us
search for hwmon stuff by class rather than assuming that all hwmon
drivers are devices on i2c busses, so it's more like a helper for
user-space tools. I don't think it solves the hwmon-is-not-i2c issue on
the kernel side at all.
--
Jean Delvare