Jean Delvare wrote:
> Hi Jim, Mark,
>
>
hi guys
> Sorry for the late answer, I have a hard time catching up with e-mail
> since I returned from vacation.
>
>
>>> - changing strategy from completely-unchecked to
>>> undo-everything-and-bailout
>>> is a rather long step, and makes driver possibly unusable in some
>>> corner cases
>>> (none of which we know and can repeat)
>>>
>> True enough, but I imagine that any other solution will be frowned upon
>> as "wallpapering over bugs". Jean: any opinion on this?
>>
>
> Yup, I don't much like Jim's approach, because it's meant to be
> temporary. Mark's approach seems better, especially since it is less
> difficult than I first thought. Now let's see how it scales to more
> complex drivers. Jim, would you like to try implementing something
> similar for the pc87360 driver (or any other complex driver of your
> choice, as long as you can test it) and see how it goes?
>
>
Im completely impressed by how clean Mark's patch is.
In the end, the elegance seduced me, patch follows.
in the start, I cut-pasted my own remove-bunch/group.
its if-0'd now, to be yanked..
In contrast with Mark's declarative grouping, Im doing it at runtime:
pc87360_detect()
calls add_tothe_group() to add it to one_big_group[] for
each sensor-attr wanted,
then calls sysfs_create_group()
NB - the var and fn names are chosen to convey the implicit context,
and some sheepishness on my part for the obvious laziness ;-)
OTOH - all the implicit-ness is in the 1st 30 lines of the patch.
I thought about trying to generalize the fn a bit better :
sysfs_addto_group( attr_group, new_attr);
but that seemed to imply promises about a dynamically allocated (and
resized) group,
which probly should be done as a real LIST or something, but I wanted
static allocation
(ENOUGH_ATTRS) since the driver has *only* statically declared attributes.
Its probly worth noting that pc87360 now does: hwmon_register b4
sysfs_create_group
whereas Mark's patch does the opposite. Does this matter ??
Ive left a few other comments in the code...
diff -ruNp -X dontdiff -X exclude-diffs ../linux-2.6.18-rc3-sk/drivers/hwmon/pc87360.c sysdev/drivers/hwmon/pc87360.c
--- ../linux-2.6.18-rc3-sk/drivers/hwmon/pc87360.c 2006-06-17 19:49:35.000000000 -0600
+++ sysdev/drivers/hwmon/pc87360.c 2006-08-03 12:58:29.000000000 -0600
@@ -829,6 +829,83 @@ static int __init pc87360_find(int sioad
return 0;
}
+/* Declare and use an attribute-group for simplicity.
+ This driver declares all attributes statically, so we count uses of
+ SENSOR_ATTR, DEVICE_ATTR, and add 1 space for trailing NULL (+0 fudge)
+*/
+#define ENOUGH_ATTRS 89 + 4 + 1 + 0
+static struct attribute *one_big_group[ENOUGH_ATTRS];
+static int next_member;
+
+static struct attribute_group my_group = {
+ .attrs = one_big_group
+};
+
+static void add_tothe_group(struct device *dev,
+ struct device_attribute *devattr)
+{
+ /* add attr to the group for later sysfs_create_group */
+ if (next_member < ENOUGH_ATTRS)
+ one_big_group[next_member++] = &devattr->attr;
+ else {
+ BUG_ON("too many in group for static alloc!\n");
+ dev_err(dev, "too many in group for static alloc!\n");
+ }
+}
+
+#if 0
+static void remove_all_devattr_files(struct device *dev)
+{
+ int i;
+ /* Mark Hoffman used sysfs_remove_group here, it nicely tracks
+ group membership, at cost of array of pointers. For now,
+ stick w non-group approach - cost is cut-paste in-elegance
+ & maint if sensor set changes */
+
+ dev_info(dev, "created %d attr-files, w errs %d. now destroying\n",
+ next_member, devattr_file_create_errs);
+
+ /* tiny cheat - rely on size equivalence of {type}_{attr}[]
+ arrays across all attrs for each type (declared that way)
+ */
+ for (i = 0; i < ARRAY_SIZE(in_input); i++) {
+ device_remove_file(dev, &in_input[i].dev_attr);
+ device_remove_file(dev, &in_min[i].dev_attr);
+ device_remove_file(dev, &in_max[i].dev_attr);
+ device_remove_file(dev, &in_status[i].dev_attr);
+ }
+ device_remove_file(dev, &dev_attr_cpu0_vid);
+ device_remove_file(dev, &dev_attr_vrm);
+ device_remove_file(dev, &dev_attr_alarms_in);
+
+ for (i = 0; i < ARRAY_SIZE(temp_input); i++) {
+ device_remove_file(dev, &temp_input[i].dev_attr);
+ device_remove_file(dev, &temp_min[i].dev_attr);
+ device_remove_file(dev, &temp_max[i].dev_attr);
+ device_remove_file(dev, &temp_crit[i].dev_attr);
+ device_remove_file(dev, &temp_status[i].dev_attr);
+ }
+ device_remove_file(dev, &dev_attr_alarms_temp);
+
+ for (i = 0; i < ARRAY_SIZE(therm_input); i++) {
+ device_remove_file(dev, &therm_input[i].dev_attr);
+ device_remove_file(dev, &therm_min[i].dev_attr);
+ device_remove_file(dev, &therm_max[i].dev_attr);
+ device_remove_file(dev, &therm_crit[i].dev_attr);
+ device_remove_file(dev, &therm_status[i].dev_attr);
+ }
+ for (i = 0; i < ARRAY_SIZE(fan_input); i++) {
+ device_remove_file(dev, &fan_input[i].dev_attr);
+ device_remove_file(dev, &fan_min[i].dev_attr);
+ device_remove_file(dev, &fan_div[i].dev_attr);
+ device_remove_file(dev, &fan_status[i].dev_attr);
+ device_remove_file(dev, &pwm[i].dev_attr);
+ }
+
+ dev_info(dev, "remaining attr-files %d\n", next_member);
+}
+#endif
+
static int pc87360_detect(struct i2c_adapter *adapter)
{
int i;
@@ -944,50 +1021,61 @@ static int pc87360_detect(struct i2c_ada
if (data->innr) {
for (i = 0; i < 11; i++) {
- device_create_file(dev, &in_input[i].dev_attr);
- device_create_file(dev, &in_min[i].dev_attr);
- device_create_file(dev, &in_max[i].dev_attr);
- device_create_file(dev, &in_status[i].dev_attr);
- }
- device_create_file(dev, &dev_attr_cpu0_vid);
- device_create_file(dev, &dev_attr_vrm);
- device_create_file(dev, &dev_attr_alarms_in);
+ add_tothe_group(dev, &in_input[i].dev_attr);
+ add_tothe_group(dev, &in_min[i].dev_attr);
+ add_tothe_group(dev, &in_max[i].dev_attr);
+ add_tothe_group(dev, &in_status[i].dev_attr);
+ }
+ add_tothe_group(dev, &dev_attr_cpu0_vid);
+ add_tothe_group(dev, &dev_attr_vrm);
+ add_tothe_group(dev, &dev_attr_alarms_in);
}
if (data->tempnr) {
for (i = 0; i < data->tempnr; i++) {
- device_create_file(dev, &temp_input[i].dev_attr);
- device_create_file(dev, &temp_min[i].dev_attr);
- device_create_file(dev, &temp_max[i].dev_attr);
- device_create_file(dev, &temp_crit[i].dev_attr);
- device_create_file(dev, &temp_status[i].dev_attr);
+ add_tothe_group(dev, &temp_input[i].dev_attr);
+ add_tothe_group(dev, &temp_min[i].dev_attr);
+ add_tothe_group(dev, &temp_max[i].dev_attr);
+ add_tothe_group(dev, &temp_crit[i].dev_attr);
+ add_tothe_group(dev, &temp_status[i].dev_attr);
}
- device_create_file(dev, &dev_attr_alarms_temp);
+ add_tothe_group(dev, &dev_attr_alarms_temp);
}
if (data->innr == 14) {
for (i = 0; i < 3; i++) {
- device_create_file(dev, &therm_input[i].dev_attr);
- device_create_file(dev, &therm_min[i].dev_attr);
- device_create_file(dev, &therm_max[i].dev_attr);
- device_create_file(dev, &therm_crit[i].dev_attr);
- device_create_file(dev, &therm_status[i].dev_attr);
+ add_tothe_group(dev, &therm_input[i].dev_attr);
+ add_tothe_group(dev, &therm_min[i].dev_attr);
+ add_tothe_group(dev, &therm_max[i].dev_attr);
+ add_tothe_group(dev, &therm_crit[i].dev_attr);
+ add_tothe_group(dev, &therm_status[i].dev_attr);
}
}
for (i = 0; i < data->fannr; i++) {
if (FAN_CONFIG_MONITOR(data->fan_conf, i)) {
- device_create_file(dev, &fan_input[i].dev_attr);
- device_create_file(dev, &fan_min[i].dev_attr);
- device_create_file(dev, &fan_div[i].dev_attr);
- device_create_file(dev, &fan_status[i].dev_attr);
+ add_tothe_group(dev, &fan_input[i].dev_attr);
+ add_tothe_group(dev, &fan_min[i].dev_attr);
+ add_tothe_group(dev, &fan_div[i].dev_attr);
+ add_tothe_group(dev, &fan_status[i].dev_attr);
}
if (FAN_CONFIG_CONTROL(data->fan_conf, i))
- device_create_file(dev, &pwm[i].dev_attr);
+ add_tothe_group(dev, &pwm[i].dev_attr);
}
+ /* Register sysfs hooks for the group */
+ err = sysfs_create_group(&client->dev.kobj, &my_group);
+ if (err) {
+ dev_err(&client->dev, "group-create failed: %d, %d members\n",
+ err, next_member);
+ goto ERROR3;
+ }
+
return 0;
+ /* ERROR4:
+ sysfs_remove_group(&client->dev.kobj, &my_group);
+ */
ERROR3:
i2c_detach_client(client);
ERROR2:
@@ -1006,6 +1094,7 @@ static int pc87360_detach_client(struct
struct pc87360_data *data = i2c_get_clientdata(client);
int i;
+ sysfs_remove_group(&client->dev.kobj, &my_group);
hwmon_device_unregister(data->class_dev);
if ((i = i2c_detach_client(client)))
