[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[Jim Cromie]

Jean, everyone

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

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)


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


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: diff.SDA-1-okcomp
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20051210/0b08065c/diff-0001.pl

[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[Jean Delvare]

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.

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

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

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.

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.

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,
-- 
Jean Delvare

[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[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

[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[Mark M. Hoffman]

Hi Jean, Jim:

* Jean Delvare <khali at linux-fr.org> [2005-12-10 11:21:34 +0100]:
> 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.

Although I haven't been involved in the macro cleanup and conversion work
up to this point... a long time ago I complained that the sysfs inits make
the hwmon drivers much bigger than they need to be.  Anyway, thanks for
working on this stuff; and that patch looks OK to me.

[...]

> 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 agree, but I wonder if sysfs_create_group() has benefits on the back
end.  It's possible that looping over the attributes uses more memory
after all. [checks code]  No, your second patch has the exact same
effect, less one big array and one small one.

> I am not totally sure what I want to do here, so hints in either
> direction are welcome.

Also, in the sysfs-group patch, there was this line...

>>> static struct attribute_group attr_group_fan[3] __initdata = {

"array of structs containing a pointer to array of pointers to struct attribute"

My own preference is to avoid that kind of stuff like the plague.

Regards,

-- 
Mark M. Hoffman
mhoffman at lightlink.com

[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[Jim Cromie]

Mark M. Hoffman wrote:

>Hi Jean, Jim:
>
>
>  
>
>Although I haven't been involved in the macro cleanup and conversion work
>up to this point... a long time ago I complained that the sysfs inits make
>the hwmon drivers much bigger than they need to be.  Anyway, thanks for
>working on this stuff; and that patch looks OK to me.
>
>[...]
>
>  
>
cool.  Ive started converting adm1026 to use this array init,
it compiles ok, but I cannot test it, so Im certain it has latent
bugs.  Im attaching in case anyone wants to pick it up and run
with it.

>>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 agree, but I wonder if sysfs_create_group() has benefits on the back
>end.  It's possible that looping over the attributes uses more memory
>after all. [checks code]  No, your second patch has the exact same
>effect, less one big array and one small one.
>
>  
>
>>I am not totally sure what I want to do here, so hints in either
>>direction are welcome.
>>    
>>
>
>Also, in the sysfs-group patch, there was this line...
>
>  
>
>>>>static struct attribute_group attr_group_fan[3] __initdata = {
>>>>        
>>>>
>
>"array of structs containing a pointer to array of pointers to struct attribute"
>
>My own preference is to avoid that kind of stuff like the plague.
>
>Regards,
>
>  
>

<conjecture>

I noticed that, if a 2nd param was passed to the show/store callbacks,
then the type-safe cast to_sensor_dev() woundnt be needed
( since its done to fetch the index out of the struct sensor_devicee_attr,
 which is now passed directly. )
With that done, we would no longer need all the  individual 
sensor_device_attrs,
since the 2nd arg can carry that info.

then, posit a pair of group-layering constructs:
struct attr_set  would hold  the attrs for each of the attrubutes,
ex:  a voltage device attr_set would have min, max, crit

now, posit that every attr be elevated to a 1 element array,
allowing for full converson later .  (useful for vin0..vin10 )

Now suppose user reads in0_max from sysfs.
sysfs invokes show_in_input(),   passes a 'sysfs-devno'
as the 2nd arg.

then show_in_input, uses the 2nd arg as the index.

Ive left out the attr-selection process - it requires a bit more setup;
when the (say voltage) attr-set-array is 1st registered, it names the 
attributes
its working for (inpu, min, max, crit), and the array lengths.  

The registration routine will, in essence, create a major device number 
for the
voltage unit, then split the minor-numbers so that the array range is 
covered,
and to select from amongst the available attrs.  Because this is done 
per-dev
at registration time, the split is made independently for each device.

so callbaks get pointer to the voltage 'sub-group', and the 2nd arg
will allow it to get the right unit/element number and attr-function.

UNFORTUNATELY, this all falls apart because there is nowhere
in the sysfs infrastructure to carry the 2nd param so its available
to pass into the callbacks.  (the conjecture is kinda handwavey cuz
Id concluded it cant work due to lack of space for the 2nd param)

Q.  are there any fields in sysfs infrastructure that we can
commandeer to carry a 2nd arg ?   For pc87360, I could imagine
a consolidation:
50 vin devices            ->  1 composite attr descriptor
14*5 temp devices      -> 1 composite attr descriptor.

IOW, the 50 individual voltage related sensor-dev-attrs, carrying;
 2 fn-ptrs, mode, name, index
are replaced by 1 common structure + 1 integer (devno)

I hope for one of these answers:
    HUH ?
    nope, youve missed some important deatails....
    it looks like it would work, but for the missing param.
    we can steal field X for this.

</>



for the attached patch,

Start converting adm1026 to use arrays of sensor device attributes.
This is compile-tested only, and needs someone with hardware to
clean it up, fix the overlooked bits, and test it.

Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: diff.adm1026.only
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20051216/2d16e135/diff.adm1026-0001.pl

[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[Jean Delvare]

Hi Jim,

> I too am partial to this way, less work for me ;-)
> Patch attached, done against 14.3 + your SENSOR_ATTR patch.

Are you certain this was the last version of your patch? I'm asking
because of the name: pc87360-use-sensor-attr-arrays~. Trailing tilde
usually means backup copy...

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

It took me several seconds to figure out that was me ;) You probably
know the device as least as good as I do by now, and you have one
physical device to test on, while I don't. I'm awaiting for your
MAINTAINERS patch :) But of course you are right, I'm still available
for advice if needed.

Thanks,
-- 
Jean Delvare

[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[Jean Delvare]

Hi Jim,

> Start converting adm1026 to use arrays of sensor device attributes.
> This is compile-tested only, and needs someone with hardware to
> clean it up, fix the overlooked bits, and test it.
> 
> Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>

The compiler complains about unused functions, and is right:

> +static struct sensor_device_attribute sda_in_min[] = {
> +	SENSOR_ATTR(in0_min, S_IRUGO, show_in, set_in_min, 0),

show_in should be show_in_min...

> +static struct sensor_device_attribute sda_in_max[] = {
> +	SENSOR_ATTR(in0_max, S_IRUGO, show_in, set_in_max, 0),

... and show_in_max.

> +static struct sensor_device_attribute sda_fan_min[] = {
> +	SENSOR_ATTR(fan1_min, S_IRUGO, show_fan, set_fan_min, 0),

show_fan should be show_fan_min...

> +static struct sensor_device_attribute sda_fan_div[] = {
> +	SENSOR_ATTR(fan1_div, S_IRUGO, show_fan, set_fan_div, 0),

... and show_fan_div.

I also had to edit the patch in order to apply it, due to recent
patches to the adm1026 driver.

Can you respin it based on 2.6.15-rc5-mm1 or later, fixing the bugs
above?

I'll need someone to test this patch on a real ADM1026 device before I
can accept it.

Thanks,
-- 
Jean Delvare

[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[Jim Cromie]

Jean Delvare wrote:

>Hi Jim,
>
>  
>
>>I too am partial to this way, less work for me ;-)
>>Patch attached, done against 14.3 + your SENSOR_ATTR patch.
>>    
>>
>
>Are you certain this was the last version of your patch? I'm asking
>because of the name: pc87360-use-sensor-attr-arrays~. Trailing tilde
>usually means backup copy...
>
>  
>
yes, thanks for catching this fat-finger double-tap.
new patch attached.  tested against 14.3, then respun
for rc5-mm3  (kzzalloc)
(my build wont boot - switchroot prob, so I cant test newest yet)

>It took me several seconds to figure out that was me ;) You probably
>know the device as least as good as I do by now, and you have one
>physical device to test on, while I don't. 
>
I thought of all those reasons, (esp the last one)
 but wasnt quite ready to acknowledge them yet.

>I'm awaiting for your
>MAINTAINERS patch :) 
>
now included.

>But of course you are right, I'm still available
>for advice if needed.
>
>Thanks,
>  
>
thank you

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pc87360-use-sensor-attr-arrays.patch
Type: text/x-patch
Size: 24185 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20051219/5a723585/pc87360-use-sensor-attr-arrays.bin

[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[Jim Cromie]

Jean Delvare wrote:

>Hi Jim,
>
>  
>
>>Start converting adm1026 to use arrays of sensor device attributes.
>>This is compile-tested only, and needs someone with hardware to
>>clean it up, fix the overlooked bits, and test it.
>>
>>Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>
>>    
>>
>
>The compiler complains about unused functions, and is right:
>  
>
>show_in should be show_in_min...
>  
>
>... and show_in_max.
>show_fan should be show_fan_min...
>  
>
>... and show_fan_div.
>
>I also had to edit the patch in order to apply it, due to recent
>patches to the adm1026 driver.
>
>Can you respin it based on 2.6.15-rc5-mm1 or later, fixing the bugs
>above?
>
>  
>
Thanks for the review, heres the respin against rc5-mm3
with corrections to the above.

>I'll need someone to test this patch on a real ADM1026 device before I
>can accept it.
>
>Thanks,
>  
>
hmm,  it has no maintainer listed.  I hope someone can test it.
Im CCg the authors named in the file, in case theyre not on lm-sensors.

also, it looks like the following could also benefit some (I havent 
looked closely)
adm1025,
adm1031.
adm9240 ,
asb100,
gl520sm.c
lm{78,85}               several *_fan_##ofset##_{min,max,div,input}

A sligthly sloppy grep for DEVICE_ATTR or  '##' shows 905 lines.
in hwmon/*, some of them are junk, but others are legitimate opportunities
to lose weight.

Also.  I noticed some repeated uses like this:
../../linux-2.6.14.3/drivers/hwmon/gl518sm.c:327:static 
DEVICE_ATTR(in3_input, S_IRUGO, show_in_input3, NULL);

It suggests that DEVICE_ATTR needs a helper initialization macro
just like SENSOR_ATTR, thoughts ?


thanks.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: adm1026-use-sensor-attr-arrays.patch
Type: text/x-patch
Size: 23846 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20051219/7cbbc854/adm1026-use-sensor-attr-arrays-0001.bin

[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[Jean Delvare]

Hi Jim,

> > I'm awaiting for your MAINTAINERS patch :) 
>
> now included.

Thanks, however, this is a bit confusing as you are moving an entry
around while adding your own. Please send a separate patch (to Linus or
Andrew) reordering "PERSONALITY HANDLING" where it belongs. Then send a
separate patch to me adding your own entry for PC87360.

About the pc87360 patch itself:

> -       struct i2c_client *new_client;
> +       struct i2c_client *cli;

I wholeheartedly agree that "new_client" was a uselessly long
identifier (inherited from the first Linux hardware monitoring drivers
6 years ago!) However, "cli" might be a bit short now and possibly
confusing (makes one think of the "clear interrupt" assembly operand).
"client" would probably be a reasonable compromise.

Now I understand that one of the reasons beyind the choice of the new
variable name is to keep lines short and avoid repeated line breaks.
There are a few tricks you could use to achieve the same, while using
"client" instead of "cli":

1* Define the following inline function:

static inline int client_create_file(i2c_client *client, struct device_attribute *attr)
{
        return device_create_file(&client->dev, attr):
}

Then you can create the files that way:

client_create_file(client, &sensor_dev_attr_in0_input.dev_attr);

2* Keep a pointer to client->dev handy, and use it everywhere is
possible:

struct device *dev = &client->dev;

device_create_file(dev, &sensor_dev_attr_in0_input.dev_attr);

This second option is probably better as it avoids repeated dereferences
of the same pointer, so I think it should be more efficient
performance-wise - although such a change doesn't seem to affect the
binary size; maybe gcc optimizes it on its own already.

This change might be a separate, preliminary patch to the SENSOR_ATTR()
one, at your option.

> -       strcpy(new_client->name, name);
> +       strcpy(cli->name, name);

While you're at it, strlcpy is prefered in this case.

> +               for (i=0; i<11; i++) {

Please respect the coding style that was used for this file WRT spaces
around assignment and comparison symbols.

Thanks,
-- 
Jean Delvare

[lm-sensors] revisiting __SENSOR_DEVICE_ATTR() and array

[Jean Delvare]

Hi Jim,

> Thanks for the review, heres the respin against rc5-mm3
> with corrections to the above.

Looks OK, except:

> diff -ruNp -X exclude-diffs ../linux-2.6.15-rc5-mm3-sk/include/linux/hwmon-sysfs.h adm/include/linux/hwmon-sysfs.h
> --- ../linux-2.6.15-rc5-mm3-sk/include/linux/hwmon-sysfs.h	2005-10-28 15:32:08.000000000 -0600
> +++ adm/include/linux/hwmon-sysfs.h	2005-12-19 05:12:15.000000000 -0700
> @@ -27,11 +27,13 @@ struct sensor_device_attribute{
>  #define to_sensor_dev_attr(_dev_attr) \
>  	container_of(_dev_attr, struct sensor_device_attribute, dev_attr)
>  
> -#define SENSOR_DEVICE_ATTR(_name,_mode,_show,_store,_index)	\
> -struct sensor_device_attribute sensor_dev_attr_##_name = {	\
> -	.dev_attr =	__ATTR(_name,_mode,_show,_store),	\
> -	.index =	_index,					\
> -}
> +#define SENSOR_ATTR(_name, _mode, _show, _store, _index)	\
> +	{ .dev_attr = __ATTR(_name, _mode, _show, _store),	\
> +	  .index = _index }
> +
> +#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store, _index)	\
> +struct sensor_device_attribute sensor_dev_attr_##_name		\
> +	= SENSOR_ATTR(_name, _mode, _show, _store, _index)
>  
>  struct sensor_device_attribute_2 {
>  	struct device_attribute dev_attr;

This change is already provided by an earlier patch of mine - so you
shouldn't include it here.

> +       for (i=0; i<16; i++) {

Same comment here as for pc87360.c: please try to respect the coding
style around.

> +        /* vid deprecated in favour of cpu0_vid, remove after 2005-11-11 */

You are reintroducing a comment which was removed some times ago.

> hmm,  it has no maintainer listed.  I hope someone can test it.
> Im CCg the authors named in the file, in case theyre not on lm-sensors.

I know they are, but they might not be paying attention to everthing so
an explicit Cc couldn't hurt.

> also, it looks like the following could also benefit some (I havent 
> looked closely)
> adm1025,
> adm1031.
> adm9240 ,
> asb100,
> gl520sm.c
> lm{78,85}               several *_fan_##ofset##_{min,max,div,input}
> 
> A sligthly sloppy grep for DEVICE_ATTR or  '##' shows 905 lines.
> in hwmon/*, some of them are junk, but others are legitimate opportunities
> to lose weight.

Yes, virtually every driver can benefit. Just like with Yani Ioannou's
"dynamic sysfs callbacks", the larger the driver, the more it benefits.

> Also.  I noticed some repeated uses like this:
> ../../linux-2.6.14.3/drivers/hwmon/gl518sm.c:327:static 
> DEVICE_ATTR(in3_input, S_IRUGO, show_in_input3, NULL);
> 
> It suggests that DEVICE_ATTR needs a helper initialization macro
> just like SENSOR_ATTR, thoughts ?

It already exists, it's named __ATTR. I'm using it in my (not yet
published) f71805f driver.

Thanks,
-- 
Jean Delvare