pc87360 de-macro and code shrink

PATCH In review Jim Cromie 3 participants 6 messages Jul 28, 2005 – Aug 4, 2005

Subsystems (3)

Driver core Hardware monitoring PC87360 hardware monitoring driver

lore

Share & tools

lore

Jim Cromie

author

2005-07-28 08:27 UTC #0 lore raw

PATCH 1/1 pc87360 de-macro and code shrink

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 --------------

drivers/hwmon/pc87360.c renamed from linux-2.6.13-rc3-mm1/drivers/hwmon/pc87360.c +382 -3

diff -ruN -X exclude-diffs ../linux-2.6.13-rc3-mm1/drivers/hwmon/pc87360.c linux/drivers/hwmon/pc87360.c
@@ -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;

include/linux/device.h renamed from linux-2.6.13-rc3-mm1/include/linux/device.h +14 -0

diff -ruN -X exclude-diffs ../linux-2.6.13-rc3-mm1/include/linux/device.h linux/include/linux/device.h
@@ -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);

Jean Delvare

2005-07-28 10:16 UTC #1 lore raw

Hi Jim,

Quoted 5 lines · 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.

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.

Quoted 7 lines

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:

Quoted 8 lines

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.

Quoted 8 lines

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/

Review

Quoted 6 lines

using loops results in a large savings in obj code.
baseline: (...) 168326 (...) drivers/hwmon/pc87360.ko
(quoted diff) +3 -0
+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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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

Yup, that's it.

Quoted 1 line

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).

Quoted 4 lines

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.

Quoted 2 lines

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

Yani Ioannou

2005-07-28 18:37 UTC #2 lore raw

In reply to Jim Cromie

Quoted 4 lines · Jean Delvare

Hi Jim,
On 2005-07-28, Jim Cromie wrote:

Quoted 8 lines

Quoted 5 lines
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.

Quoted 16 lines

Quoted 7 lines

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:

Quoted 8 lines

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),

};

Quoted 5 lines

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

Jim Cromie

author

2005-08-04 03:25 UTC #3 lore raw

In reply to Jim Cromie

Quoted 29 lines · Yani Ioannou

Quoted 7 lines · Jean Delvare
Hi Jim,
Quoted 2 lines · Jim Cromie
attached is a patch which:
Quoted 9 lines
Quoted 5 lines
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.

Quoted 9 lines

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.

Yani Ioannou

2005-08-04 05:32 UTC #4 lore raw

In reply to Jim Cromie

Quoted 6 lines · Jim Cromie

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.

Quoted 11 lines

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...

Quoted 1 line

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.

Quoted 3 lines

.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

Jean Delvare

2005-08-04 22:03 UTC #5 lore raw

In reply to Jim Cromie

Hi Yani,

Quoted 12 lines

Quoted 11 lines

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;
};

Quoted 6 lines

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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 7 lines

Quoted 3 lines
.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.

Signature · 1 line

Jean Delvare