Finally (phew!) this patch demonstrates how to adapt the adm1026 to
take advantage of the new callbacks, and the i2c-sysfs.h defined
structure/macros. Most of the other sensor/hwmon drivers could be
updated in the same way. The odd few exceptions (bmcsensors for
example) however might be better off with their own custom attribute
structure.
Again I'd prefer someone test this particular patch before it be
applied, because I haven't got an adm1026 to test it with :-).
Signed-off-by: Yani Ioannou <[email protected]>
---
Hi Yani,
> Again I'd prefer someone test this particular patch before it be
> applied, because I haven't got an adm1026 to test it with :-).
If you could come up with an additional demonstration patch for either
the lm90 driver or the it87 driver, I would happily give it a try. The
adm1026 has very few users AFAIK so it might not be the best choice to
find testers, although I agree that it was a convenient way to
demonstrate how drivers could benefit from the proposed change.
And, once again, thanks *a lot* for looking into this, this is very much
appreciated.
--
Jean Delvare
Hi Jean,
On 5/17/05, Jean Delvare <[email protected]> wrote:
> If you could come up with an additional demonstration patch for either
> the lm90 driver or the it87 driver, I would happily give it a try. The
> adm1026 has very few users AFAIK so it might not be the best choice to
> find testers, although I agree that it was a convenient way to
> demonstrate how drivers could benefit from the proposed change.
No problem, I'll give this a shot later today/tomorrow when I get some
time, it shouldn't be very hard.
>
> And, once again, thanks *a lot* for looking into this, this is very much
> appreciated.
>
The concept benefits bmcsensors more than anything else so it isn't
exactly unselfish but thanks :-), its great to know its appreciated.
Yani
Hi Yani,
On Tue, 17 May 2005 07:18:51 -0400, Yani Ioannou <[email protected]> wrote:
Following is derived from gcc -E adm1026.c with your patch (the
script strips all but base number from numbered names):
adm1026.c SENSOR_fan1_div RW
adm1026.c SENSOR_fan1_input R
adm1026.c SENSOR_fan1_min RW
adm1026.c SENSOR_in0_input R
adm1026.c SENSOR_in0_max RW
adm1026.c SENSOR_in0_min RW
adm1026.c SENSOR_temp1_auto_point1_temp RW
adm1026.c SENSOR_temp1_auto_point1_temp_hyst R
adm1026.c SENSOR_temp1_crit RW
adm1026.c SENSOR_temp1_input R
adm1026.c SENSOR_temp1_max RW
adm1026.c SENSOR_temp1_min RW
adm1026.c SENSOR_temp1_offset RW
adm1026.c alarm_mask RW
adm1026.c alarms R
adm1026.c analog_out RW
adm1026.c gpio RW
adm1026.c gpio_mask RW
adm1026.c pwm1 RW << these also should also
adm1026.c pwm1_enable RW << have SENSOR_ in front
adm1026.c temp1_auto_point1_pwm RW << of them, seems you
adm1026.c temp1_crit_enable RW << missed a group?
adm1026.c vid R
adm1026.c vrm RW
I'm assuming some magic elsewhere sees that 'SENSOR_' and removes
it before it gets to sysfs. I can test adm9240, w83627hf and it87
drivers as well as watch the overall patterns as above.
--Grant.
Hi Grant,
Those are the sysfs names? If so something looks wrong with the
SENSOR_ ones..maybe an unintended effect of the new
sensor_device_attribute macro. I can't seem to find anything like that
in "gcc -I ../../../include/ -E adm1026.c" though, would you mind
sending me your script? Also make sure you are including the new
i2c-sysfs.h header file.
The group of attributes you've highlighted below don't use
sensor_device_attribute on purpose because they don't benefit from the
dynamic sysfs callbacks, mainly because they are singletons. Well its
possible you could roll all the attribute callbacks into one if you
wanted but that seems a bit extreme, and you'd have a large nasty
if-else going on in it :-).
BTW looks like a useful script :-), I'm always worried when doing
these changes I might accidently change a sysfs attribute permission.
Thanks,
Yani
On 5/17/05, Grant Coady <[email protected]> wrote:
> Hi Yani,
> On Tue, 17 May 2005 07:18:51 -0400, Yani Ioannou <[email protected]> wrote:
>
> Following is derived from gcc -E adm1026.c with your patch (the
> script strips all but base number from numbered names):
>
> adm1026.c SENSOR_fan1_div RW
> adm1026.c SENSOR_fan1_input R
> adm1026.c SENSOR_fan1_min RW
> adm1026.c SENSOR_in0_input R
> adm1026.c SENSOR_in0_max RW
> adm1026.c SENSOR_in0_min RW
> adm1026.c SENSOR_temp1_auto_point1_temp RW
> adm1026.c SENSOR_temp1_auto_point1_temp_hyst R
> adm1026.c SENSOR_temp1_crit RW
> adm1026.c SENSOR_temp1_input R
> adm1026.c SENSOR_temp1_max RW
> adm1026.c SENSOR_temp1_min RW
> adm1026.c SENSOR_temp1_offset RW
> adm1026.c alarm_mask RW
> adm1026.c alarms R
> adm1026.c analog_out RW
> adm1026.c gpio RW
> adm1026.c gpio_mask RW
> adm1026.c pwm1 RW << these also should also
> adm1026.c pwm1_enable RW << have SENSOR_ in front
> adm1026.c temp1_auto_point1_pwm RW << of them, seems you
> adm1026.c temp1_crit_enable RW << missed a group?
> adm1026.c vid R
> adm1026.c vrm RW
>
> I'm assuming some magic elsewhere sees that 'SENSOR_' and removes
> it before it gets to sysfs. I can test adm9240, w83627hf and it87
> drivers as well as watch the overall patterns as above.
>
> --Grant.
>
>
Hi Yani,
On Tue, 17 May 2005 16:56:04 -0400, Yani Ioannou <[email protected]> wrote:
>Those are the sysfs names? If so something looks wrong with the
Not the final ones, just from first macro expansion of driver
source, that's why I'd like to see changes on w83627hf driver
as I can test it right through.
I haven't looked at your patched source yet to see about applying
it to other drivers, 'cos I'm happy to do that part for you if I
can follow the changes :)
>SENSOR_ ones..maybe an unintended effect of the new
>sensor_device_attribute macro. I can't seem to find anything like that
>in "gcc -I ../../../include/ -E adm1026.c" though, would you mind
>sending me your script? Also make sure you are including the new
>i2c-sysfs.h header file.
No, I'm not doing a proper compile, I'm intentionally doing partial
compile of driver.c and _not_ including headers, ignoring errors due
to missing headers.
This technique seems valid for the type of testing I'm doing, which
is simply to pull out the first level macro expansion.
Script is work in progress, updated to current version up at:
http://scatter.mine.nu/hwmon/sysfs-names/
>The group of attributes you've highlighted below don't use
>sensor_device_attribute on purpose because they don't benefit from the
>dynamic sysfs callbacks, mainly because they are singletons. Well its
Not singletons, 3 of each (from an intermediate file):
adm1026.c temp1_crit_enable S_IRUGO S_IWUSR
adm1026.c temp2_crit_enable S_IRUGO S_IWUSR
adm1026.c temp3_crit_enable S_IRUGO S_IWUSR
adm1026.c pwm1 S_IRUGO S_IWUSR
adm1026.c pwm2 S_IRUGO S_IWUSR
adm1026.c pwm3 S_IRUGO S_IWUSR
adm1026.c temp1_auto_point1_pwm S_IRUGO S_IWUSR
adm1026.c temp2_auto_point1_pwm S_IRUGO S_IWUSR
adm1026.c temp3_auto_point1_pwm S_IRUGO S_IWUSR
adm1026.c temp1_auto_point2_pwm S_IRUGO
adm1026.c temp2_auto_point2_pwm S_IRUGO
adm1026.c temp3_auto_point2_pwm S_IRUGO
Yet, in related groups of three you have:
adm1026.c SENSOR_temp1_crit S_IRUGO S_IWUSR
adm1026.c SENSOR_temp2_crit S_IRUGO S_IWUSR
adm1026.c SENSOR_temp3_crit S_IRUGO S_IWUSR
>BTW looks like a useful script :-), I'm always worried when doing
>these changes I might accidently change a sysfs attribute permission.
Thank you, what I will do is remove the "SENSOR_" and flag attribute
column with an asterisk to indicate dynamic attribute, something
like that. And flag singletons too.
Thanks,
--Grant.
On 5/17/05, Grant Coady <[email protected]> wrote:
> Hi Yani,
>
> On Tue, 17 May 2005 16:56:04 -0400, Yani Ioannou <[email protected]> wrote:
> >Those are the sysfs names? If so something looks wrong with the
>
> Not the final ones, just from first macro expansion of driver
> source, that's why I'd like to see changes on w83627hf driver
> as I can test it right through.
>
> No, I'm not doing a proper compile, I'm intentionally doing partial
> compile of driver.c and _not_ including headers, ignoring errors due
> to missing headers.
Ah..OK, that is probably why, I've put the macros which would be
expanded in the first level in a separate header because it will
probably be shared amongst many drivers. Although I still don't see
where SENSOR_blah is coming from at all at the moment, if you can
track that down I'd be interested to know if its just something to do
with the script or a problem with the patch.
> Script is work in progress, updated to current version up at:
>
> http://scatter.mine.nu/hwmon/sysfs-names/
Ok, I'll have a look at it later.
>
> >The group of attributes you've highlighted below don't use
> >sensor_device_attribute on purpose because they don't benefit from the
> >dynamic sysfs callbacks, mainly because they are singletons. Well its
>
> Not singletons, 3 of each (from an intermediate file):
>
> adm1026.c temp1_crit_enable S_IRUGO S_IWUSR
> adm1026.c temp2_crit_enable S_IRUGO S_IWUSR
> adm1026.c temp3_crit_enable S_IRUGO S_IWUSR
> adm1026.c pwm1 S_IRUGO S_IWUSR
> adm1026.c pwm2 S_IRUGO S_IWUSR
> adm1026.c pwm3 S_IRUGO S_IWUSR
> adm1026.c temp1_auto_point1_pwm S_IRUGO S_IWUSR
> adm1026.c temp2_auto_point1_pwm S_IRUGO S_IWUSR
> adm1026.c temp3_auto_point1_pwm S_IRUGO S_IWUSR
> adm1026.c temp1_auto_point2_pwm S_IRUGO
> adm1026.c temp2_auto_point2_pwm S_IRUGO
> adm1026.c temp3_auto_point2_pwm S_IRUGO
>
> Yet, in related groups of three you have:
>
> adm1026.c SENSOR_temp1_crit S_IRUGO S_IWUSR
> adm1026.c SENSOR_temp2_crit S_IRUGO S_IWUSR
> adm1026.c SENSOR_temp3_crit S_IRUGO S_IWUSR
Well I said mainly singletons :-), some of the attributes don't
benefit from the dynamic sysfs callbacks simply because they already
only use one callback for a few different attributes, I believe that's
the case with the non-singletons in this case.
Thanks,
Yani
Hi Yani,
On Tue, 17 May 2005 19:21:41 -0400, Yani Ioannou <[email protected]> wrote:
>Ah..OK, that is probably why, I've put the macros which would be
>expanded in the first level in a separate header because it will
>probably be shared amongst many drivers. Although I still don't see
>where SENSOR_blah is coming from at all at the moment, if you can
>track that down I'd be interested to know if its just something to do
>with the script or a problem with the patch.
Oops, my script, sorry. I'll fix that.
>> Not singletons, 3 of each (from an intermediate file):
. . .
>
>Well I said mainly singletons :-), some of the attributes don't
>benefit from the dynamic sysfs callbacks simply because they already
>only use one callback for a few different attributes, I believe that's
>the case with the non-singletons in this case.
Not quite that, one sysfs name, one value. The multiple sysfs names
that were 'missed' by your changes don't use the usual macro. Three
instances of each attribute in the source, instead.
--Grant.