[lm-sensors] [patch] pc87360 de-macro and code shrink

[lm-sensors] [patch] pc87360 de-macro and code shrink

[Jim Cromie]

attached is a patch which:

1. adds an __ATTR_N macro to device.h,
which takes an extra _index arg, and initializes device_attibute's new 
.index member/

2. replaces lots of nested-macro-uses of these:

static DEVICE_ATTR(temp##offset##_input, S_IRUGO, \
    show_temp##offset##_input, NULL); \

with array initializers. like these:

static struct device_attribute dev_attr_temp_input[]   {
    __ATTR_N(temp1_input, S_IRUGO, show_temp_input, NULL, 0),
    __ATTR_N(temp2_input, S_IRUGO, show_temp_input, NULL, 1),
    __ATTR_N(temp3_input, S_IRUGO, show_temp_input, NULL, 2),
    __ATTR_N(temp4_input, S_IRUGO, show_temp_input, NULL, 3),
    __ATTR_N(temp5_input, S_IRUGO, show_temp_input, NULL, 4),
    __ATTR_N(temp6_input, S_IRUGO, show_temp_input, NULL, 5),
  };

3.  de-macros the functions called by the individual items.
the functions use devattr->index to determine which item theyre working on.


this is an interim version, and has ifdefs to support back and forth 
debugging:

grep define pc87360.c |grep ATTR_LOOP
#define FAN_ATTR_LOOP 0
#define PWM_ATTR_LOOP 0
#define VOLT_ATTR_LOOP 1
#define TEMP_ATTR_LOOP 0

using loops results in a large savings in obj code.

baseline:           -rw-rw-r--  1 jimc jimc 168326 Jul 27 23:10 
drivers/hwmon/pc87360.ko
VOLT_ATTR_LOOP      -rw-rw-r--  1 jimc jimc 135940 Jul 27 23:11 
drivers/hwmon/pc87360.ko
+ TEMP_ATTR_LOOP    -rw-rw-r--  1 jimc jimc 127441 Jul 27 23:16 
drivers/hwmon/pc87360.ko
+ FAN/PWM_ATTR_LOOP -rw-rw-r--  1 jimc jimc 121062 Jul 27 23:55 
drivers/hwmon/pc87360.ko


I have a couple of questions that Im hoping you can clarify for me;
its late, and I could sleep now, and see your responses when I wake.

line 887 starts the show_and_set_temp(offset)  macro,
which looks to be a duplicate of
#define show_and_set_therm(offset) on line 651,

or if theyre not duplicates (I havent looked carefully), its an 
unfortunate naming,
creating functions which are disambiguated only by the offset.

OTOH, they could be different cuz the physical units on the chip have 
different capabilities.
ATM code has some unused data, but probly not another 10k

Using loops to call device_create_file also simplifies some of the logic 
wrt data->innr, tempnr, fannr;
the if-variations are hidden/abstracted by 0..configd_max.   Of course, 
I could have oversimplified.


Ive done cursory testing: new code does this.

root@soekris:~# sensors
pc87366-isa-6620
Adapter: ISA adapter
avi0:      +3.01 V  (min =  +0.00 V, max =  +3.01 V)
VCORE:     +2.02 V  (min =  +0.00 V, max =  +3.01 V)
VCC:       +5.01 V  (min =  +0.00 V, max =  +6.03 V)
VPWR:     +14.01 V  (min =  +0.00 V, max = +60.56 V)
+12V:     +11.91 V  (min =  +0.00 V, max = +14.46 V)
-12V:     -12.28 V  (min = -60.61 V, max =  -2.76 V)
GND:       +0.00 V  (min =  +0.00 V, max =  +3.01 V)
Vsb:       +3.33 V  (min =  +0.00 V, max =  +6.03 V)
Vdd:       +2.91 V  (min =  +0.00 V, max =  +6.03 V)
Vbat:      +3.01 V  (min =  +0.00 V, max =  +3.01 V)
AVdd:      +3.31 V  (min =  +0.00 V, max =  +6.03 V)
Temp:       +107 C  (low  =   -55 C, high =  +127 C)
Critical:   +127 C

root@soekris:~#
root@soekris:~# cd /sys/bus/i2c/devices/0-6620/
alarms_temp   in2_max       in6_max       temp1_crit    temp4_input
bus           in2_min       in6_min       temp1_input   temp4_max
driver        in2_status    in6_status    temp1_max     temp4_min
in0_input     in3_input     in7_input     temp1_min     temp4_status
in0_max       in3_max       in7_max       temp1_status  temp5_crit
in0_min       in3_min       in7_min       temp2_crit    temp5_input
in0_status    in3_status    in7_status    temp2_input   temp5_max
in10_input    in4_input     in8_input     temp2_max     temp5_min
in10_max      in4_max       in8_max       temp2_min     temp5_status
in10_min      in4_min       in8_min       temp2_status  temp6_crit
in10_status   in4_status    in8_status    temp3_crit    temp6_input
in1_input     in5_input     in9_input     temp3_input   temp6_max
in1_max       in5_max       in9_max       temp3_max     temp6_min
in1_min       in5_min       in9_min       temp3_min     temp6_status
in1_status    in5_status    in9_status    temp3_status
in2_input     in6_input     name          temp4_crit

I have no fans on this board, and have never gotten temp readings I trust.
But the voltages look right :-)

thanks
jimc


-------------- next part --------------
diff -ruN -X exclude-diffs ../linux-2.6.13-rc3-mm1/drivers/hwmon/pc87360.c linux/drivers/hwmon/pc87360.c
--- ../linux-2.6.13-rc3-mm1/drivers/hwmon/pc87360.c	2005-07-18 12:36:02.000000000 -0600
+++ linux/drivers/hwmon/pc87360.c	2005-07-27 23:55:13.000000000 -0600
@@ -46,7 +46,7 @@
 static unsigned int normal_isa[] = { 0, I2C_CLIENT_ISA_END };
 static struct i2c_force_data forces[] = {{ NULL }};
 static u8 devid;
-static unsigned int extra_isa[3];
+static unsigned int extra_isa[3];	/* isa bus addresses of the 3 logical devices */
 static u8 confreg[4];
 
 enum chips { any_chip, pc87360, pc87363, pc87364, pc87365, pc87366 };
@@ -251,7 +251,7 @@
  * Sysfs stuff
  */
 
-static ssize_t set_fan_min(struct device *dev, const char *buf,
+static ssize_t _set_fan_min(struct device *dev, const char *buf,
 	size_t count, int nr)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -280,6 +280,9 @@
 	return count;
 }
 
+#define FAN_ATTR_LOOP 1
+#if ! FAN_ATTR_LOOP
+
 #define show_and_set_fan(offset) \
 static ssize_t show_fan##offset##_input(struct device *dev, struct device_attribute *attr, char *buf) \
 { \
@@ -308,7 +311,7 @@
 static ssize_t set_fan##offset##_min(struct device *dev, struct device_attribute *attr, const char *buf, \
 	size_t count) \
 { \
-	return set_fan_min(dev, buf, count, offset-1); \
+	return _set_fan_min(dev, buf, count, offset-1); \
 } \
 static DEVICE_ATTR(fan##offset##_input, S_IRUGO, \
 	show_fan##offset##_input, NULL); \
@@ -322,6 +325,68 @@
 show_and_set_fan(2)
 show_and_set_fan(3)
 
+#else
+
+static ssize_t show_fan_input(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", FAN_FROM_REG(data->fan[attr->index-1],
+		       FAN_DIV_FROM_REG(data->fan_status[attr->index-1])));
+}
+static ssize_t show_fan_min(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", FAN_FROM_REG(data->fan_min[attr->index-1],
+		       FAN_DIV_FROM_REG(data->fan_status[attr->index-1])));
+}
+static ssize_t show_fan_div(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n",
+		       FAN_DIV_FROM_REG(data->fan_status[attr->index-1]));
+}
+static ssize_t show_fan_status(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n",
+		       FAN_STATUS_FROM_REG(data->fan_status[attr->index-1]));
+}
+static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr, const char *buf,
+	size_t count)
+{
+	return _set_fan_min(dev, buf, count, attr->index-1);
+}
+
+static struct device_attribute dev_attr_fan_input[] +  {
+    __ATTR_N(fan1_input, S_IRUGO, show_fan_input, NULL, 0),
+    __ATTR_N(fan2_input, S_IRUGO, show_fan_input, NULL, 1),
+    __ATTR_N(fan3_input, S_IRUGO, show_fan_input, NULL, 2),
+  };
+static struct device_attribute dev_attr_fan_status[] +  {
+    __ATTR_N(fan_status, S_IRUGO, show_fan_status, NULL, 0),
+    __ATTR_N(fan_status, S_IRUGO, show_fan_status, NULL, 1),
+    __ATTR_N(fan_status, S_IRUGO, show_fan_status, NULL, 2),
+  };
+static struct device_attribute dev_attr_fan_div[] +  {
+    __ATTR_N(fan_div, S_IRUGO, show_fan_div, NULL, 0),
+    __ATTR_N(fan_div, S_IRUGO, show_fan_div, NULL, 1),
+    __ATTR_N(fan_div, S_IRUGO, show_fan_div, NULL, 2),
+  };
+static struct device_attribute dev_attr_fan_min[] +  {
+    __ATTR_N(fan_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 0),
+    __ATTR_N(fan_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 1),
+    __ATTR_N(fan_min, S_IWUSR | S_IRUGO, show_fan_min, set_fan_min, 2),
+  };
+
+#endif
+
+#define PWM_ATTR_LOOP 1
+#if ! PWM_ATTR_LOOP
+
 #define show_and_set_pwm(offset) \
 static ssize_t show_pwm##offset(struct device *dev, struct device_attribute *attr, char *buf) \
 { \
@@ -352,6 +417,44 @@
 show_and_set_pwm(2)
 show_and_set_pwm(3)
 
+#else
+
+static ssize_t show_pwm(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n",
+		       PWM_FROM_REG(data->pwm[attr->index-1],
+				    FAN_CONFIG_INVERT(data->fan_conf,
+						      attr->index-1)));
+}
+static ssize_t set_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
+	size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pc87360_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+
+	down(&data->update_lock);
+	data->pwm[attr->index-1] = PWM_TO_REG(val,
+			      FAN_CONFIG_INVERT(data->fan_conf, attr->index-1));
+	pc87360_write_value(data, LD_FAN, NO_BANK, PC87360_REG_PWM(attr->index-1),
+			    data->pwm[attr->index-1]);
+	up(&data->update_lock);
+	return count;
+}
+
+static struct device_attribute dev_attr_pwm[] +  {
+    __ATTR_N(pwm1, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 0),
+    __ATTR_N(pwm2, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 1),
+    __ATTR_N(pwm3, S_IWUSR | S_IRUGO, show_pwm, set_pwm, 2),
+  };
+
+#endif
+
+#define VOLT_ATTR_LOOP 1
+#if ! VOLT_ATTR_LOOP
+
 #define show_and_set_in(offset) \
 static ssize_t show_in##offset##_input(struct device *dev, struct device_attribute *attr, char *buf) \
 { \
@@ -425,6 +528,125 @@
 show_and_set_in(9)
 show_and_set_in(10)
 
+#else
+
+static ssize_t show_v_input(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", IN_FROM_REG(data->in[attr->index],
+		       data->in_vref));
+}
+static ssize_t show_v_min(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", IN_FROM_REG(data->in_min[attr->index],
+		       data->in_vref));
+}
+static ssize_t show_v_max(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", IN_FROM_REG(data->in_max[attr->index],
+		       data->in_vref));
+}
+static ssize_t show_v_status(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", data->in_status[attr->index]);
+}
+static ssize_t set_v_min(struct device *dev, struct device_attribute *attr, const char *buf,
+	size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pc87360_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+
+	down(&data->update_lock);
+	data->in_min[attr->index] = IN_TO_REG(val, data->in_vref);
+	pc87360_write_value(data, LD_IN, attr->index, PC87365_REG_IN_MIN,
+			    data->in_min[attr->index]);
+	up(&data->update_lock);
+	return count;
+}
+static ssize_t set_v_max(struct device *dev, struct device_attribute *attr, const char *buf,
+	size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pc87360_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+
+	down(&data->update_lock);
+	data->in_max[attr->index] = IN_TO_REG(val,
+			       data->in_vref);
+	pc87360_write_value(data, LD_IN, attr->index, PC87365_REG_IN_MAX,
+			    data->in_max[attr->index]);
+	up(&data->update_lock);
+	return count;
+}
+
+static struct device_attribute dev_attr_v_input[] +  {
+    __ATTR_N(in0_input, S_IRUGO, show_v_input, NULL, 0),
+    __ATTR_N(in1_input, S_IRUGO, show_v_input, NULL, 1),
+    __ATTR_N(in2_input, S_IRUGO, show_v_input, NULL, 2),
+    __ATTR_N(in3_input, S_IRUGO, show_v_input, NULL, 3),
+    __ATTR_N(in4_input, S_IRUGO, show_v_input, NULL, 4),
+    __ATTR_N(in5_input, S_IRUGO, show_v_input, NULL, 5),
+    __ATTR_N(in6_input, S_IRUGO, show_v_input, NULL, 6),
+    __ATTR_N(in7_input, S_IRUGO, show_v_input, NULL, 7),
+    __ATTR_N(in8_input, S_IRUGO, show_v_input, NULL, 8),
+    __ATTR_N(in9_input, S_IRUGO, show_v_input, NULL, 9),
+    __ATTR_N(in10_input, S_IRUGO, show_v_input, NULL, 10),
+  };
+static struct device_attribute dev_attr_v_status[] +  {
+    __ATTR_N(in0_status, S_IRUGO, show_v_status, NULL, 0),
+    __ATTR_N(in1_status, S_IRUGO, show_v_status, NULL, 1),
+    __ATTR_N(in2_status, S_IRUGO, show_v_status, NULL, 2),
+    __ATTR_N(in3_status, S_IRUGO, show_v_status, NULL, 3),
+    __ATTR_N(in4_status, S_IRUGO, show_v_status, NULL, 4),
+    __ATTR_N(in5_status, S_IRUGO, show_v_status, NULL, 5),
+    __ATTR_N(in6_status, S_IRUGO, show_v_status, NULL, 6),
+    __ATTR_N(in7_status, S_IRUGO, show_v_status, NULL, 7),
+    __ATTR_N(in8_status, S_IRUGO, show_v_status, NULL, 8),
+    __ATTR_N(in9_status, S_IRUGO, show_v_status, NULL, 9),
+    __ATTR_N(in10_status, S_IRUGO, show_v_status, NULL, 10),
+  };
+static struct device_attribute dev_attr_v_min[] = 
+  {
+    __ATTR_N(in0_min, S_IWUSR | S_IRUGO, show_v_min, set_v_min, 0),
+    __ATTR_N(in1_min, S_IWUSR | S_IRUGO, show_v_min, set_v_min, 1),
+    __ATTR_N(in2_min, S_IWUSR | S_IRUGO, show_v_min, set_v_min, 2),
+    __ATTR_N(in3_min, S_IWUSR | S_IRUGO, show_v_min, set_v_min, 3),
+    __ATTR_N(in4_min, S_IWUSR | S_IRUGO, show_v_min, set_v_min, 4),
+    __ATTR_N(in5_min, S_IWUSR | S_IRUGO, show_v_min, set_v_min, 5),
+    __ATTR_N(in6_min, S_IWUSR | S_IRUGO, show_v_min, set_v_min, 6),
+    __ATTR_N(in7_min, S_IWUSR | S_IRUGO, show_v_min, set_v_min, 7),
+    __ATTR_N(in8_min, S_IWUSR | S_IRUGO, show_v_min, set_v_min, 8),
+    __ATTR_N(in9_min, S_IWUSR | S_IRUGO, show_v_min, set_v_min, 9),
+    __ATTR_N(in10_min, S_IWUSR | S_IRUGO, show_v_min, set_v_min, 10),
+
+  };
+static struct device_attribute dev_attr_v_max[] +  {
+    __ATTR_N(in0_max, S_IWUSR | S_IRUGO, show_v_max, set_v_max, 0),
+    __ATTR_N(in1_max, S_IWUSR | S_IRUGO, show_v_max, set_v_max, 1),
+    __ATTR_N(in2_max, S_IWUSR | S_IRUGO, show_v_max, set_v_max, 2),
+    __ATTR_N(in3_max, S_IWUSR | S_IRUGO, show_v_max, set_v_max, 3),
+    __ATTR_N(in4_max, S_IWUSR | S_IRUGO, show_v_max, set_v_max, 4),
+    __ATTR_N(in5_max, S_IWUSR | S_IRUGO, show_v_max, set_v_max, 5),
+    __ATTR_N(in6_max, S_IWUSR | S_IRUGO, show_v_max, set_v_max, 6),
+    __ATTR_N(in7_max, S_IWUSR | S_IRUGO, show_v_max, set_v_max, 7),
+    __ATTR_N(in8_max, S_IWUSR | S_IRUGO, show_v_max, set_v_max, 8),
+    __ATTR_N(in9_max, S_IWUSR | S_IRUGO, show_v_max, set_v_max, 9),
+    __ATTR_N(in10_max, S_IWUSR | S_IRUGO, show_v_max, set_v_max, 10),
+  };
+
+#endif
+
+#define TEMP_ATTR_LOOP 1
+
+#if ! TEMP_ATTR_LOOP
+
 #define show_and_set_therm(offset) \
 static ssize_t show_temp##offset##_input(struct device *dev, struct device_attribute *attr, char *buf) \
 { \
@@ -511,6 +733,129 @@
 show_and_set_therm(5)
 show_and_set_therm(6)
 
+#else 
+
+static ssize_t show_temp_input(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", IN_FROM_REG(data->in[attr->index+7],
+		       data->in_vref));
+}
+static ssize_t show_temp_min(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", IN_FROM_REG(data->in_min[attr->index+7],
+		       data->in_vref));
+}
+static ssize_t show_temp_max(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", IN_FROM_REG(data->in_max[attr->index+7],
+		       data->in_vref));
+}
+static ssize_t show_temp_crit(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", IN_FROM_REG(data->in_crit[attr->index-4],
+		       data->in_vref));
+}
+static ssize_t show_temp_status(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct pc87360_data *data = pc87360_update_device(dev);
+	return sprintf(buf, "%u\n", data->in_status[attr->index+7]);
+}
+static ssize_t set_temp_min(struct device *dev, struct device_attribute *attr, const char *buf,
+	size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pc87360_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+
+	down(&data->update_lock);
+	data->in_min[attr->index+7] = IN_TO_REG(val, data->in_vref);
+	pc87360_write_value(data, LD_IN, attr->index+7, PC87365_REG_TEMP_MIN,
+			    data->in_min[attr->index+7]);
+	up(&data->update_lock);
+	return count;
+}
+static ssize_t set_temp_max(struct device *dev, struct device_attribute *attr, const char *buf,
+	size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pc87360_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+
+	down(&data->update_lock);
+	data->in_max[attr->index+7] = IN_TO_REG(val, data->in_vref);
+	pc87360_write_value(data, LD_IN, attr->index+7, PC87365_REG_TEMP_MAX,
+			    data->in_max[attr->index+7]);
+	up(&data->update_lock);
+	return count;
+}
+static ssize_t set_temp_crit(struct device *dev, struct device_attribute *attr, const char *buf,
+	size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pc87360_data *data = i2c_get_clientdata(client);
+	long val = simple_strtol(buf, NULL, 10);
+
+	down(&data->update_lock);
+	data->in_crit[attr->index-4] = IN_TO_REG(val, data->in_vref);
+	pc87360_write_value(data, LD_IN, attr->index+7, PC87365_REG_TEMP_CRIT,
+			    data->in_crit[attr->index-4]);
+	up(&data->update_lock);
+	return count;
+}
+
+static struct device_attribute dev_attr_temp_input[] +  {
+    __ATTR_N(temp1_input, S_IRUGO, show_temp_input, NULL, 0),
+    __ATTR_N(temp2_input, S_IRUGO, show_temp_input, NULL, 1),
+    __ATTR_N(temp3_input, S_IRUGO, show_temp_input, NULL, 2),
+    __ATTR_N(temp4_input, S_IRUGO, show_temp_input, NULL, 3),
+    __ATTR_N(temp5_input, S_IRUGO, show_temp_input, NULL, 4),
+    __ATTR_N(temp6_input, S_IRUGO, show_temp_input, NULL, 5),
+  };
+
+static struct device_attribute dev_attr_temp_status[] +  {
+    __ATTR_N(temp1_status, S_IRUGO, show_temp_status, NULL, 0),
+    __ATTR_N(temp2_status, S_IRUGO, show_temp_status, NULL, 1),
+    __ATTR_N(temp3_status, S_IRUGO, show_temp_status, NULL, 2),
+    __ATTR_N(temp4_status, S_IRUGO, show_temp_status, NULL, 3),
+    __ATTR_N(temp5_status, S_IRUGO, show_temp_status, NULL, 4),
+    __ATTR_N(temp6_status, S_IRUGO, show_temp_status, NULL, 5),
+  };
+static struct device_attribute dev_attr_temp_min[] +  {
+    __ATTR_N(temp1_min, S_IWUSR | S_IWUSR, show_temp_min, set_temp_min, 0),
+    __ATTR_N(temp2_min, S_IWUSR | S_IWUSR, show_temp_min, set_temp_min, 1),
+    __ATTR_N(temp3_min, S_IWUSR | S_IWUSR, show_temp_min, set_temp_min, 2),
+    __ATTR_N(temp4_min, S_IWUSR | S_IWUSR, show_temp_min, set_temp_min, 3),
+    __ATTR_N(temp5_min, S_IWUSR | S_IWUSR, show_temp_min, set_temp_min, 4),
+    __ATTR_N(temp6_min, S_IWUSR | S_IWUSR, show_temp_min, set_temp_min, 5),
+  };
+static struct device_attribute dev_attr_temp_max[] +  {
+    __ATTR_N(temp1_max, S_IWUSR | S_IWUSR, show_temp_max, set_temp_max, 0),
+    __ATTR_N(temp2_max, S_IWUSR | S_IWUSR, show_temp_max, set_temp_max, 1),
+    __ATTR_N(temp3_max, S_IWUSR | S_IWUSR, show_temp_max, set_temp_max, 2),
+    __ATTR_N(temp4_max, S_IWUSR | S_IWUSR, show_temp_max, set_temp_max, 3),
+    __ATTR_N(temp5_max, S_IWUSR | S_IWUSR, show_temp_max, set_temp_max, 4),
+    __ATTR_N(temp6_max, S_IWUSR | S_IWUSR, show_temp_max, set_temp_max, 5),
+  };
+static struct device_attribute dev_attr_temp_crit[] +  {
+    __ATTR_N(temp1_crit, S_IWUSR | S_IWUSR, show_temp_crit, set_temp_crit, 0),
+    __ATTR_N(temp2_crit, S_IWUSR | S_IWUSR, show_temp_crit, set_temp_crit, 1),
+    __ATTR_N(temp3_crit, S_IWUSR | S_IWUSR, show_temp_crit, set_temp_crit, 2),
+    __ATTR_N(temp4_crit, S_IWUSR | S_IWUSR, show_temp_crit, set_temp_crit, 3),
+    __ATTR_N(temp5_crit, S_IWUSR | S_IWUSR, show_temp_crit, set_temp_crit, 4),
+    __ATTR_N(temp6_crit, S_IWUSR | S_IWUSR, show_temp_crit, set_temp_crit, 5),
+  };
+
+#endif
+
 static ssize_t show_vid(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct pc87360_data *data = pc87360_update_device(dev);
@@ -683,6 +1028,7 @@
 			continue;
 		}
 
+		/* address points to extra_isa */
 		address[i] = val;
 
 		if (i=0) { /* Fans */
@@ -838,6 +1184,19 @@
 	}
 
 	/* Register sysfs hooks */
+
+	#if VOLT_ATTR_LOOP
+
+	if (data->innr)
+	  for (i=0; i<11; i++) { // data->innr; i++) {
+		device_create_file(&new_client->dev, &dev_attr_v_input[i]);
+		device_create_file(&new_client->dev, &dev_attr_v_min[i]);
+		device_create_file(&new_client->dev, &dev_attr_v_max[i]);
+		device_create_file(&new_client->dev, &dev_attr_v_status[i]);
+	}
+
+	#else
+
 	if (data->innr) {
 		device_create_file(&new_client->dev, &dev_attr_in0_input);
 		device_create_file(&new_client->dev, &dev_attr_in1_input);
@@ -888,6 +1247,19 @@
 		device_create_file(&new_client->dev, &dev_attr_vrm);
 		device_create_file(&new_client->dev, &dev_attr_alarms_in);
 	}
+	#endif
+
+	#if TEMP_ATTR_LOOP
+
+	for (i=0; i<data->tempnr; i++) {
+		device_create_file(&new_client->dev, &dev_attr_temp_input[i]);
+		device_create_file(&new_client->dev, &dev_attr_temp_min[i]);
+		device_create_file(&new_client->dev, &dev_attr_temp_max[i]);
+		device_create_file(&new_client->dev, &dev_attr_temp_status[i]);
+		device_create_file(&new_client->dev, &dev_attr_temp_crit[i]);
+	}
+
+	#else
 
 	if (data->tempnr) {
 		device_create_file(&new_client->dev, &dev_attr_temp1_input);
@@ -910,6 +1282,7 @@
 		device_create_file(&new_client->dev, &dev_attr_temp3_crit);
 		device_create_file(&new_client->dev, &dev_attr_temp3_status);
 	}
+
 	if (data->innr = 14) {
 		device_create_file(&new_client->dev, &dev_attr_temp4_input);
 		device_create_file(&new_client->dev, &dev_attr_temp5_input);
@@ -928,6 +1301,24 @@
 		device_create_file(&new_client->dev, &dev_attr_temp6_status);
 	}
 
+	#endif
+
+	#if FAN_ATTR_LOOP
+
+	for (i=0; i<data->fannr; i++) {
+
+		if (FAN_CONFIG_MONITOR(data->fan_conf, i)) {
+			device_create_file(&new_client->dev, &dev_attr_fan_input[i]);
+			device_create_file(&new_client->dev, &dev_attr_fan_min[i]);
+			device_create_file(&new_client->dev, &dev_attr_fan_div[i]);
+			device_create_file(&new_client->dev, &dev_attr_fan_status[i]);
+		}
+		if (FAN_CONFIG_CONTROL(data->fan_conf, i))
+			device_create_file(&new_client->dev, &dev_attr_pwm[i]);
+	}
+
+	#else
+
 	if (data->fannr) {
 		if (FAN_CONFIG_MONITOR(data->fan_conf, 0)) {
 			device_create_file(&new_client->dev,
@@ -971,6 +1362,7 @@
 		if (FAN_CONFIG_CONTROL(data->fan_conf, 2))
 			device_create_file(&new_client->dev, &dev_attr_pwm3);
 	}
+	#endif
 
 	return 0;
 
diff -ruN -X exclude-diffs ../linux-2.6.13-rc3-mm1/include/linux/device.h linux/include/linux/device.h
--- ../linux-2.6.13-rc3-mm1/include/linux/device.h	2005-07-18 12:36:48.000000000 -0600
+++ linux/include/linux/device.h	2005-07-27 21:21:40.000000000 -0600
@@ -342,15 +342,29 @@
 
 struct device_attribute {
 	struct attribute	attr;
+
 	ssize_t (*show)(struct device *dev, struct device_attribute *attr,
 			char *buf);
 	ssize_t (*store)(struct device *dev, struct device_attribute *attr,
 			 const char *buf, size_t count);
+	int index;
 };
 
 #define DEVICE_ATTR(_name,_mode,_show,_store) \
 struct device_attribute dev_attr_##_name = __ATTR(_name,_mode,_show,_store)
 
+#define __ATTR_N(_name,_mode,_show,_store,_index) { \
+	.attr = {.name = __stringify(_name),	\
+		 .mode = _mode,			\
+		 .owner = THIS_MODULE },	\
+	.show	= _show,			\
+	.store	= _store,			\
+	.index	= _index,			\
+}
+
+#define DEVICE_ATTR_N(_name,_mode,_show,_store, _index) \
+struct device_attribute dev_attr_##_name = __ATTR_N(_name,_mode,_show,_store,_index)
+
 
 extern int device_create_file(struct device *device, struct device_attribute * entry);
 extern void device_remove_file(struct device * dev, struct device_attribute * attr);

[lm-sensors] Re: [patch] pc87360 de-macro and code shrink

[Jean Delvare]

Hi Jim,

On 2005-07-28, Jim Cromie wrote:
> attached is a patch which:
>
> 1. adds an __ATTR_N macro to device.h,
> which takes an extra _index arg, and initializes device_attibute's new
> .index member.

This seems to be duplicating the SENSOR_DEVICE_ATTR macro as defined in
include/linux/hwmon-sysfs.h.

I like the cleanup you are proposing, but I'd like you to do it using
the mechanism that was introduced recently by Yani Ioannou rather than
reimplementing your own. Only a few drivers have been ported to use it
at the moment: adm1026, it87, lm63, lm83, lm90. See how that was done,
and do the same for pc87360.

> 2. replaces lots of nested-macro-uses of these:
>
> static DEVICE_ATTR(temp##offset##_input, S_IRUGO, \
>     show_temp##offset##_input, NULL); \
>
> with array initializers. like these:
>
> static struct device_attribute dev_attr_temp_input[] >   {
>     __ATTR_N(temp1_input, S_IRUGO, show_temp_input, NULL, 0),
>     __ATTR_N(temp2_input, S_IRUGO, show_temp_input, NULL, 1),
>     __ATTR_N(temp3_input, S_IRUGO, show_temp_input, NULL, 2),
>     __ATTR_N(temp4_input, S_IRUGO, show_temp_input, NULL, 3),
>     __ATTR_N(temp5_input, S_IRUGO, show_temp_input, NULL, 4),
>     __ATTR_N(temp6_input, S_IRUGO, show_temp_input, NULL, 5),
>   };

This is a different cleanup. Getting rid of macros is one thing, using
arrays of attributes is another. As I said before, I'd like a separate
patch (or even several) for each step. This makes reviewing much easier.
Macro killing should be easy and unquestionable, while attribute arrays
is something that still needs discussing.

> this is an interim version, and has ifdefs to support back and forth
> debugging:
>
> grep define pc87360.c |grep ATTR_LOOP
> #define FAN_ATTR_LOOP 0
> #define PWM_ATTR_LOOP 0
> #define VOLT_ATTR_LOOP 1
> #define TEMP_ATTR_LOOP 0

This might be convenient for debugging but it's not so for reviewing. A
reviewer needs to be able to compare the code you remove with the code
you add, both in terms of contents and size, just by looking at the
patch.

You may consider using tools like quilt [1] to divide your work into
individual patches. This will still give you the ability to test your
changes individually, while also making it easy to review.

[1] http://savannah.nongnu.org/projects/quilt/

> using loops results in a large savings in obj code.
>
> baseline:           (...) 168326  (...) drivers/hwmon/pc87360.ko
> +VOLT_ATTR_LOOP     (...) 135940  (...) drivers/hwmon/pc87360.ko
> +TEMP_ATTR_LOOP     (...) 127441  (...) drivers/hwmon/pc87360.ko
> +FAN/PWM_ATTR_LOOP  (...) 121062  (...) drivers/hwmon/pc87360.ko

Sure it does, it won't be hard to convince me the idea has some value.
However, you should disable debugging before doing binary size
comparisons. The size of the drivers in debug mode is not exactly
meaningful. Also, some of the gain is due to the macro killing, not the
loops. Having separate patches for this would let you evaluate what
change exactly produces what gain.

> line 887 starts the show_and_set_temp(offset)  macro,
> which looks to be a duplicate of
> #define show_and_set_therm(offset) on line 651,

They are not duplicate. _temp is for diode-based temperature measurements
of the TMS logical device. _therm is for thermistor-based temperature
measurements of the VLM logical device. I remember there are a few
differences between them (let alone the fact that they point to
different data arrays, and are expressed in different units), such as
the limits being swapped or the critical limit being useless for certain
types of thermistors.

> or if theyre not duplicates (I havent looked carefully), its an
> unfortunate naming,
> creating functions which are disambiguated only by the offset.

Blame it to the complex design of the chip, the driver mostly follows it
both in construct and namings.

> OTOH, they could be different cuz the physical units on the chip have
> different capabilities.

Yup, that's it.

> ATM code has some unused data, but probly not another 10k

Probably way less than that and at the cost of some maintainability. I
wouldn't even try. _therm is certainly more similar to _in than _temp
(but I wouldn't try merging these either).

> Using loops to call device_create_file also simplifies some of the logic
> wrt data->innr, tempnr, fannr;
> the if-variations are hidden/abstracted by 0..configd_max.   Of course,
> I could have oversimplified.

I'm not sure I get what you mean. Again, this would probably be a
separate patch so that these changes are not hidden in much bigger
changes, and we can evaluate them properly.

> I have no fans on this board, and have never gotten temp readings I trust.
> But the voltages look right :-)

Known problem. Only temp3, which is internal, seems to report anything
valueable on the net4801, and even this value is usually too high to be
likely. But at least you should get the same value for temp3 before and
after your patches, and you should be able to change the limits as well.

Thanks,
--
Jean Delvare

[lm-sensors] Re: [patch] pc87360 de-macro and code shrink

[Yani Ioannou]

On 7/28/05, Jean Delvare <khali@linux-fr.org> wrote:
> 
> Hi Jim,
> 
> On 2005-07-28, Jim Cromie wrote:
> > attached is a patch which:
> >
> > 1. adds an __ATTR_N macro to device.h,
> > which takes an extra _index arg, and initializes device_attibute's new
> > .index member.
> 
> This seems to be duplicating the SENSOR_DEVICE_ATTR macro as defined in
> include/linux/hwmon-sysfs.h.

Just as a further to Jean's point, I'd like to highlight that
device_attribute doesn't need new members, rather it now passes an
extra parameter (a device_attribute *) to it's callbacks. What that
extra parameter does allow you to do is to embed a device_attribute in
another type (e.g. sensor_device_attribute) and access anything
defined in that new type (e.g. index). This is exactly what is defined
in include/hwmon-sysfs.h. See any of Jean's or my patches to various
sensor drivers that show how to use them.

With that said I'm glad you are trying to clean these things up :-),
you might want to look through the archives for "dynamic sysfs
callbacks" and the ensuing discussions if you are interested. At one
point early on adding a .index attribute to the device_attribute was
considered a possibility, but I feel what we ended up with is much,
much better.

> > 2. replaces lots of nested-macro-uses of these:
> >
> > static DEVICE_ATTR(temp##offset##_input, S_IRUGO, \
> >     show_temp##offset##_input, NULL); \
> >
> > with array initializers. like these:
> >
> > static struct device_attribute dev_attr_temp_input[] > >   {
> >     __ATTR_N(temp1_input, S_IRUGO, show_temp_input, NULL, 0),
> >     __ATTR_N(temp2_input, S_IRUGO, show_temp_input, NULL, 1),
> >     __ATTR_N(temp3_input, S_IRUGO, show_temp_input, NULL, 2),
> >     __ATTR_N(temp4_input, S_IRUGO, show_temp_input, NULL, 3),
> >     __ATTR_N(temp5_input, S_IRUGO, show_temp_input, NULL, 4),
> >     __ATTR_N(temp6_input, S_IRUGO, show_temp_input, NULL, 5),
> >   };
> 
> This is a different cleanup. Getting rid of macros is one thing, using
> arrays of attributes is another. As I said before, I'd like a separate
> patch (or even several) for each step. This makes reviewing much easier.
> Macro killing should be easy and unquestionable, while attribute arrays
> is something that still needs discussing.

Its a hard thing to see how to do nicely. We need to send Greg more
rough patches so we can pique his interest in solving the problem
again ;-).

Yani

[lm-sensors] Re: [patch] pc87360 de-macro and code shrink

[Jim Cromie]

Yani Ioannou wrote:

>On 7/28/05, Jean Delvare <khali@linux-fr.org> wrote:
>  
>
>>Hi Jim,
>>
>>On 2005-07-28, Jim Cromie wrote:
>>    
>>
>>>attached is a patch which:
>>>
>>>1. adds an __ATTR_N macro to device.h,
>>>which takes an extra _index arg, and initializes device_attibute's new
>>>.index member.
>>>      
>>>
>>This seems to be duplicating the SENSOR_DEVICE_ATTR macro as defined in
>>include/linux/hwmon-sysfs.h.
>>    
>>
>
>Just as a further to Jean's point, I'd like to highlight that
>device_attribute doesn't need new members, rather it now passes an
>extra parameter (a device_attribute *) to it's callbacks. What that
>extra parameter does allow you to do is to embed a device_attribute in
>another type (e.g. sensor_device_attribute) and access anything
>defined in that new type (e.g. index). This is exactly what is defined
>in include/hwmon-sysfs.h. See any of Jean's or my patches to various
>sensor drivers that show how to use them.
>
>With that said I'm glad you are trying to clean these things up :-),
>you might want to look through the archives for "dynamic sysfs
>callbacks" and the ensuing discussions if you are interested. At one
>point early on adding a .index attribute to the device_attribute was
>considered a possibility, but I feel what we ended up with is much,
>much better.
>
>  
>
IIUC, the 'much better' is passing a *dev-attr,
doing to_sensor_dev_attr() on it to get the full SDA
in a typesafe manner, then using whatever new members
you have added (ex index, and for sda2, nr also)

or have I missed something ?

just to follow up,  rc4-mm1 has

struct sensor_device_attribute{
        struct device_attribute dev_attr;
        int index;
};
struct sensor_device_attribute_2 {
        struct device_attribute dev_attr;
        u8 index;
        u8 nr;
};


Ive used the 1st sda in my latest patch-set, are these structs 'stable' ?
.index is still there, with more room than is useful for file-entries in 
a sysfs dir-node.
Any reason not to shorten the index, to 'reserve' another short ?

I also note there is room for another short in the latter, in dark 
(padding) memory.

[lm-sensors] Re: [patch] pc87360 de-macro and code shrink

[Yani Ioannou]

On 8/3/05, Jim Cromie <jcromie@divsol.com> wrote:
> IIUC, the 'much better' is passing a *dev-attr,
> doing to_sensor_dev_attr() on it to get the full SDA
> in a typesafe manner, then using whatever new members
> you have added (ex index, and for sda2, nr also)
> 
> or have I missed something ?

Well the change affects more than sensors of course, and there is a
lot of code other places in the kernel with the same problem (and not
just for device_attribute), so I expect to see other structs out there
like sensor_device_attribute, but yes basically :-). Its much less
error prone than a void * as you say because of it's typesafe manner,
and doesn't require us to add anything to the sysfs attribute struct
itself.

> just to follow up,  rc4-mm1 has
> 
> struct sensor_device_attribute{
>         struct device_attribute dev_attr;
>         int index;
> };
> struct sensor_device_attribute_2 {
>         struct device_attribute dev_attr;
>         u8 index;
>         u8 nr;
> };

I never noticed that, looks like it originated in this patch:
http://lists.lm-sensors.org/pipermail/lm-sensors/2005-July/013104.html.

Is sensor_device_attribute_2 useful to enough drivers (>1) to justify
putting it in hwmon-sysfs? Otherwise maybe its better off just being
defined by that driver itself...
 
> Ive used the 1st sda in my latest patch-set, are these structs 'stable' ?
Is anything stable in the kernel? I don't expect it to disappear
anytime soon though.

> .index is still there, with more room than is useful for file-entries in
> a sysfs dir-node.
> Any reason not to shorten the index, to 'reserve' another short ?

Hmm..that's an interesting proposition, what is the limit for the
number of sysfs attributes in a directory? It certainly doesn't make
any sense to use an int if a short will suffice for even the maximum.

Yani

[lm-sensors] Re: [patch] pc87360 de-macro and code shrink

[Jean Delvare]

Hi Yani,

> > just to follow up,  rc4-mm1 has
> > 
> > struct sensor_device_attribute{
> >         struct device_attribute dev_attr;
> >         int index;
> > };
> > struct sensor_device_attribute_2 {
> >         struct device_attribute dev_attr;
> >         u8 index;
> >         u8 nr;
> > };
> 
> I never noticed that, looks like it originated in this patch:
> http://lists.lm-sensors.org/pipermail/lm-sensors/2005-July/013104.html.
> 
> Is sensor_device_attribute_2 useful to enough drivers (>1) to justify
> putting it in hwmon-sysfs? Otherwise maybe its better off just being
> defined by that driver itself...

It is virtually useful to any hardware monitoring driver having several
limits for one kind of sensors, and several sensors that kind. For
example, if you have a device having:

in0_input in0_min in0_max
in1_input in1_min in1_max
in2_input in2_min in2_max
in3_input in3_min in3_max

then you can easily use a two-dimension array to store the register
values, and use nr=0 for input, nr=1 for min and nr=2 for max.

You could argue that this can also be done using one single index and
some arithmetics. That's true, and we might end up doing that, however
it might make the drivers' code less clear. At any rate, this structure
doesn't cost anything to drivers not using it, so I have no problem with
it being in a common header. Let's see if people converting drivers want
to use it. I think we need so some more drivers converted (if possible
by different persons) before we can decide which structure is the best -
if it happens that one single structure can fulfill every drivers'
needs.

> > Ive used the 1st sda in my latest patch-set, are these structs
> > 'stable' ?
>
> Is anything stable in the kernel? I don't expect it to disappear
> anytime soon though.

And if it does, it'll be replaced by something else, and the person
doing that will have to convert all the structure users.

> > .index is still there, with more room than is useful for
> > file-entries in a sysfs dir-node.
> > Any reason not to shorten the index, to 'reserve' another short ?
> 
> Hmm..that's an interesting proposition, what is the limit for the
> number of sysfs attributes in a directory? It certainly doesn't make
> any sense to use an int if a short will suffice for even the maximum.

Agreed, I had that in mind when the structure was introduced. I was just
(and am still) waiting to see more drivers converted before the
structures are changed to fit the needs better.

-- 
Jean Delvare