Jean Delvare wrote: >Hi Jim, > > > >>back in pre-14 days, I proposed a set of patches to hwmon/pc87360.c >>Id like to revisit the parts that didnt make it into 2.6.14, >>and see whether it might be consistent with some of Greg KHs >>longer term plans for sysfs rework LWN 12/1 >> >> > >I just read that article, it doesn't seem to really mention anything >concrete. I'm not sure if your work on the pc87360 attributes are >anyhow related to any change Greg has in mind. Grouping the attributes >registration has been long needed for many hardware monitoring drivers >anyway. Individual file registration is very costly in terms of driver >size when there are many attributes. > > > forgive a bit of hand-waving to justify the revisit ;-) evidently it wasnt needed. >>So, attached (sorry) is a patch (for discussion purposes), doing: >> >>hwmon-sysfs.h: >> >>gets a new __SENSOR_DEVICE_ATTR, which expands into an initialization, >>like so: >> >>+#define SENSOR_DEVICE_ATTR(_name,_mode,_show,_store,_index) \ >>+struct sensor_device_attribute sensor_dev_attr_##_name \ >>+ = __SENSOR_DEVICE_ATTR(_name,_mode,_show,_store,_index) >> >> > >On November 23rd, I sent a similar patch to you and this list, mostly >derived from yours: > >http://lists.lm-sensors.org/pipermail/lm-sensors/2005-November/014433.html > >So it's great that you come back to me on that topic now. My version >has shorter names and a few cosmetic changes. It's in my local stack : > >http://khali.linux-fr.org/devel/i2c/linux-2.6/hwmon-allow-sensor-attr-arrays.patch > >But I did not send it to Greg for integration yet. I was waiting for >some ack from you or others, and for a driver actually using the added >possibility. > > > I like yours, more 80-column friendly. >>pc87360.c: >> >>replaces many uses of SENSOR_DEVICE_ATTR with >>struct sensor_device_attribute array[] = { ..} >>declarations and initializations using the new macro, >>and rolls lists of device_create_file into loops over those arrays. >> >>Also includes a few macros that probly arent general enough for a .h, >>but are useful abbreviations here >> >> > >I am working on similar changes on a different driver myself. This >driver (f71805f) I have not released yet because it is implemented as a >platform driver and there are still some changes needed to the core >part for it to work properly. You may take a look at it here though: > >http://jdelvare.net2.nerim.net/sensors/hwmon-f71805f-new-driver.patch > >This initial version uses the old individual attribute file >registration approach. From there, there are two possibilities to get >the attributes registered as groups: > >1* Using sysfs_create_group(), as Greg KH suggested. This doesn't need >any preliminary patch. The patch is here: > >http://jdelvare.net2.nerim.net/sensors/hwmon-f71805f-sysfs-group.patch > >2* Using arrays of attributes and looping over them, as you proposed >for the pc87360 driver. This needs the preliminary hwmon-sysfs.h patch. >The patch is here: > >http://jdelvare.net2.nerim.net/sensors/hwmon-f71805f-use-attr-array.patch > >This second patch has my preference as it makes the code significantly >smaller. I don't much like the sysfs_create_group interface because it >forces you to create two additional data structures you otherwise have >no need for. > > > I too am partial to this way, less work for me ;-) Patch attached, done against 14.3 + your SENSOR_ATTR patch. >I am not totally sure what I want to do here, so hints in either >direction are welcome. > >As far as your pc87360 driver patch is concerned, the diffences with my >f71805f patch seem to be as follows: > >1* You introduce macros to make the attribute array definitions >shorter, I do not. I don't much like macros, they tend to make the code >more obscure to the reader, hide size costs and break grep and LXR. >They also increase the preprocessing time, which unfortunately distcc >cannot distribute. The point of Yani Ioannou's dynamic sysfs callbacks >was to remove a whole lot of macros, and we have a similar opportunity >here. > > > Ive dropped the macros - other reviewers may have stronger opinions, best not to bait them. >Now, I won't force my view on this to anyone. Just think about it and >compare your code with mine. If you still think using macros is better, >that's OK with me. > >2* You defined many individual arrays for each type of attribute. I did >group them as much as possible, so in the end I only have three arrays. >The pc87360 driver is unarguably more complex than the f71805f driver, >so you might not be able to reduce the array number that low even if >you wanted to. But that might still be an idea to consider. > > > I like the 1-to-1 correspondence between the arrays and the attribute names, and prefer to keep that symmetry at this point. Id like to see more array-ness, not less. ( for example: arrays of attr_sets, or attr_sets of attr_arrays ) I think theyre a natural fit with real hardware, . <more OT ramblings trimmed> >Here again, I'm not forcing my view. You may actually prefer to have >separate arrays, and that's OK with me as well. > >Others are invited to comment on both patches and compare between them >if they feel like to, as my judgment is certainly a bit biased by me >being the author of one of the patches and not the other ;) > >As for your patch itself, a few comments: > >* Use tabs for indentation, kill trailing whitespace. > >* Make everything (new code, at least) fit within 80 columns. > > Ack tabs. I saw no trailing ws. I shortened a couple variable-names to reduce the need for wrapping. >Lastly, would you want/accept to become the pc87360 driver maintainer? >If you do, just provide a patch to MAINTAINERS. You can look at other >hardware monitoring drivers entries for inspiration. > > > thanks for that vote of confidence. I guess I'd have the original author available for consult ;-) >Thanks, > > thank you. -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: pc87360-use-sensor-attr-arrays~ Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20051213/83e80455/attachment.pl
