[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[jim.cromie@gmail.com (Jim Cromie)]

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/20051214/83e80455/pc87360-use-sensor-attr-arrays-0001.pl