Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH 2.6.12-rc4 15/15] drivers/i2c/chips/adm1026.c: use dynamic sysfs callbacks

0 views
Skip to first unread message

Yani Ioannou

unread,
May 17, 2005, 7:30:15 AM5/17/05
to
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 <yani.i...@gmail.com>

---

patch-linux-2.6.12-rc4-sysfsdyncallback-deviceattr-i2c-adm1026.diff.diffstat.txt
patch-linux-2.6.12-rc4-sysfsdyncallback-deviceattr-i2c-adm1026.diff

Yani Ioannou

unread,
May 17, 2005, 7:40:09 AM5/17/05
to
Hi Jean,

On 5/17/05, Jean Delvare <kh...@linux-fr.org> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Jean Delvare

unread,
May 17, 2005, 7:40:13 AM5/17/05
to

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

Grant Coady

unread,
May 17, 2005, 4:30:16 PM5/17/05
to
Hi Yani,
On Tue, 17 May 2005 07:18:51 -0400, Yani Ioannou <yani.i...@gmail.com> 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.

Yani Ioannou

unread,
May 17, 2005, 5:00:18 PM5/17/05
to
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

Yani Ioannou

unread,
May 17, 2005, 7:30:16 PM5/17/05
to
On 5/17/05, Grant Coady <grant...@dodo.com.au> wrote:
> Hi Yani,
>
> On Tue, 17 May 2005 16:56:04 -0400, Yani Ioannou <yani.i...@gmail.com> 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

Grant Coady

unread,
May 17, 2005, 7:30:16 PM5/17/05
to
Hi Yani,

On Tue, 17 May 2005 16:56:04 -0400, Yani Ioannou <yani.i...@gmail.com> 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 Coady

unread,
May 17, 2005, 10:10:08 PM5/17/05
to
Hi Yani,

On Tue, 17 May 2005 19:21:41 -0400, Yani Ioannou <yani.i...@gmail.com> 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.

Jean Delvare

unread,
May 19, 2005, 4:10:10 PM5/19/05
to
Hi Yanu

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

> Here is a patch against it87, since it seems to have many more device
> attributes than lm90. The size difference:
>
> before:
> Module Size Used by
> it87 28832 0
> after:
> Module Size Used by
> it87 22432 0
>
> That isn't bad, but the code cleanup is worth it alone in my opinion.

I finally gave a try to your patches, including the one for it87 which I
used for testing. It all works like a charm, pretty impressive
considering the overall complexity of the change. Congratulations :)

I'd like to add that the technical solution you came up with pleases me
much (which may or may not be relevant).

If we are into code refactoring and driver size shrinking, you may want
to take a look at the following patch, which makes it87 even smaller
(from 18976 bytes down to 16992 bytes on my system) and IMHO more
cleaner. I do voluntarily not sign it, it's not meant for immediate
inclusion, I'd rather see your changes in mainstream before I start
pushing my own ones. It's simply meant as a proof that your proposed big
change doesn't prevent further improvements, which is great :) Basically
the idea is to use arrays for the device sysfs attributes, so that we
then can use a loop to instantiate them instead of having to instantiate
each one individually. The same could be done in many other hardware
monitoring drivers as well, of course (which is why I defined the helper
macros in i2c-sysfs.h).

And once again... great great work Yani :)

Thanks.

drivers/i2c/chips/it87.c | 227 ++++++++++++++++++++++------------------------
include/linux/i2c-sysfs.h | 10 ++
2 files changed, 119 insertions(+), 118 deletions(-)

Index: linux-2.6.12-rc4/drivers/i2c/chips/it87.c
===================================================================
--- linux-2.6.12-rc4.orig/drivers/i2c/chips/it87.c 2005-05-19 19:13:52.000000000 +0200
+++ linux-2.6.12-rc4/drivers/i2c/chips/it87.c 2005-05-19 21:33:49.000000000 +0200
@@ -304,33 +304,40 @@
return count;
}

-#define show_in_offset(offset) \
-static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, \
- show_in, NULL, offset);
-
-#define limit_in_offset(offset) \
-static SENSOR_DEVICE_ATTR(in##offset##_min, S_IRUGO | S_IWUSR, \
- show_in_min, set_in_min, offset); \
-static SENSOR_DEVICE_ATTR(in##offset##_max, S_IRUGO | S_IWUSR, \
- show_in_max, set_in_max, offset);
-
-show_in_offset(0);
-limit_in_offset(0);
-show_in_offset(1);
-limit_in_offset(1);
-show_in_offset(2);
-limit_in_offset(2);
-show_in_offset(3);
-limit_in_offset(3);
-show_in_offset(4);
-limit_in_offset(4);
-show_in_offset(5);
-limit_in_offset(5);
-show_in_offset(6);
-limit_in_offset(6);
-show_in_offset(7);
-limit_in_offset(7);
-show_in_offset(8);
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(in_input, 9)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in0_input, S_IRUGO, show_in, NULL, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in1_input, S_IRUGO, show_in, NULL, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in2_input, S_IRUGO, show_in, NULL, 2)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in3_input, S_IRUGO, show_in, NULL, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in4_input, S_IRUGO, show_in, NULL, 4)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in5_input, S_IRUGO, show_in, NULL, 5)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in6_input, S_IRUGO, show_in, NULL, 6)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in7_input, S_IRUGO, show_in, NULL, 7)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in8_input, S_IRUGO, show_in, NULL, 8)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(in_min, 8)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in0_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in1_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in2_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 2)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in3_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in4_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 4)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in5_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 5)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in6_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 6)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in7_min, S_IRUGO | S_IWUSR, show_in_min, set_in_min, 7)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(in_max, 8)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in0_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in1_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in2_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 2)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in3_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in4_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 4)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in5_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 5)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in6_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 6)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(in7_max, S_IRUGO | S_IWUSR, show_in_max, set_in_max, 7)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+

/* 3 temperatures */
static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
@@ -392,17 +399,25 @@
up(&data->update_lock);
return count;
}
-#define show_temp_offset(offset) \
-static SENSOR_DEVICE_ATTR(temp##offset##_input, S_IRUGO, \
- show_temp, NULL, offset - 1); \
-static SENSOR_DEVICE_ATTR(temp##offset##_max, S_IRUGO | S_IWUSR, \
- show_temp_max, set_temp_max, offset - 1); \
-static SENSOR_DEVICE_ATTR(temp##offset##_min, S_IRUGO | S_IWUSR, \
- show_temp_min, set_temp_min, offset - 1);
-
-show_temp_offset(1);
-show_temp_offset(2);
-show_temp_offset(3);
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(temp_input, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp1_input, S_IRUGO, show_temp, NULL, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp2_input, S_IRUGO, show_temp, NULL, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp3_input, S_IRUGO, show_temp, NULL, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(temp_min, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp1_min, S_IRUGO | S_IWUSR, show_temp_min, set_temp_min, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp2_min, S_IRUGO | S_IWUSR, show_temp_min, set_temp_min, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp3_min, S_IRUGO | S_IWUSR, show_temp_min, set_temp_min, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(temp_max, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp1_max, S_IRUGO | S_IWUSR, show_temp_max, set_temp_max, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp2_max, S_IRUGO | S_IWUSR, show_temp_max, set_temp_max, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp3_max, S_IRUGO | S_IWUSR, show_temp_max, set_temp_max, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+

static ssize_t show_sensor(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -451,13 +466,13 @@
up(&data->update_lock);
return count;
}
-#define show_sensor_offset(offset) \
-static SENSOR_DEVICE_ATTR(temp##offset##_type, S_IRUGO | S_IWUSR, \
- show_sensor, set_sensor, offset - 1);
-
-show_sensor_offset(1);
-show_sensor_offset(2);
-show_sensor_offset(3);
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(temp_type, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp1_type, S_IRUGO | S_IWUSR, show_sensor, set_sensor, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp2_type, S_IRUGO | S_IWUSR, show_sensor, set_sensor, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(temp3_type, S_IRUGO | S_IWUSR, show_sensor, set_sensor, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+

/* 3 Fans */
static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
@@ -619,27 +634,36 @@
return count;
}

-#define show_fan_offset(offset) \
-static SENSOR_DEVICE_ATTR(fan##offset##_input, S_IRUGO, \
- show_fan, NULL, offset - 1); \
-static SENSOR_DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR, \
- show_fan_min, set_fan_min, offset - 1); \
-static SENSOR_DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR, \
- show_fan_div, set_fan_div, offset - 1);
-
-show_fan_offset(1);
-show_fan_offset(2);
-show_fan_offset(3);
-
-#define show_pwm_offset(offset) \
-static SENSOR_DEVICE_ATTR(pwm##offset##_enable, S_IRUGO | S_IWUSR, \
- show_pwm_enable, set_pwm_enable, offset - 1); \
-static SENSOR_DEVICE_ATTR(pwm##offset, S_IRUGO | S_IWUSR, \
- show_pwm , set_pwm, offset - 1);
-
-show_pwm_offset(1);
-show_pwm_offset(2);
-show_pwm_offset(3);
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(fan_input, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan1_input, S_IRUGO, show_fan, NULL, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan2_input, S_IRUGO, show_fan, NULL, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan3_input, S_IRUGO, show_fan, NULL, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(fan_min, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan1_min, S_IRUGO | S_IWUSR, show_fan_min, set_fan_min, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan2_min, S_IRUGO | S_IWUSR, show_fan_min, set_fan_min, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan3_min, S_IRUGO | S_IWUSR, show_fan_min, set_fan_min, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(fan_div, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan1_div, S_IRUGO | S_IWUSR, show_fan_div, set_fan_div, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan2_div, S_IRUGO | S_IWUSR, show_fan_div, set_fan_div, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(fan3_div, S_IRUGO | S_IWUSR, show_fan_div, set_fan_div, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(pwm, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm2, S_IRUGO | S_IWUSR, show_pwm, set_pwm, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm3, S_IRUGO | S_IWUSR, show_pwm, set_pwm, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+
+static SENSOR_DEVICE_ATTR_ARRAY_HEAD(pwm_enable, 3)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm1_enable, S_IRUGO | S_IWUSR, show_pwm_enable, set_pwm_enable, 0)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm2_enable, S_IRUGO | S_IWUSR, show_pwm_enable, set_pwm_enable, 1)
+SENSOR_DEVICE_ATTR_ARRAY_ITEM(pwm3_enable, S_IRUGO | S_IWUSR, show_pwm_enable, set_pwm_enable, 2)
+SENSOR_DEVICE_ATTR_ARRAY_TAIL;
+

/* Alarms */
static ssize_t show_alarms(struct device *dev, struct device_attribute *attr, char *buf)
@@ -843,60 +867,27 @@
it87_init_client(new_client, data);

/* Register sysfs hooks */
- device_create_file(&new_client->dev, &sensor_dev_attr_in0_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in1_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in2_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in3_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in4_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in5_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in6_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in7_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in8_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in0_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in1_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in2_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in3_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in4_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in5_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in6_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in7_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in0_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in1_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in2_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in3_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in4_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in5_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in6_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_in7_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp1_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp2_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp3_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp1_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp2_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp3_max.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp1_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp2_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp3_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp1_type.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp2_type.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_temp3_type.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_fan1_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_fan2_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_fan3_input.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_fan1_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_fan2_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_fan3_min.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_fan1_div.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_fan2_div.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_fan3_div.dev_attr);
+ for (i=0; i<8; i++) {
+ device_create_file(&new_client->dev, &sensor_dev_attr_in_input[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in_min[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in_max[i].dev_attr);
+ }
+ device_create_file(&new_client->dev, &sensor_dev_attr_in_input[8].dev_attr);
+ for (i=0; i<3; i++) {
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp_input[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp_min[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp_max[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp_type[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan_input[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan_min[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan_div[i].dev_attr);
+ }
device_create_file(&new_client->dev, &dev_attr_alarms);
if (enable_pwm_interface) {
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm1_enable.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm2_enable.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm3_enable.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm1.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm2.dev_attr);
- device_create_file(&new_client->dev, &sensor_dev_attr_pwm3.dev_attr);
+ for (i=0; i<3; i++) {
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm_enable[i].dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_pwm[i].dev_attr);
+ }
}

if (data->type == it8712) {
Index: linux-2.6.12-rc4/include/linux/i2c-sysfs.h
===================================================================
--- linux-2.6.12-rc4.orig/include/linux/i2c-sysfs.h 2005-05-19 19:13:52.000000000 +0200
+++ linux-2.6.12-rc4/include/linux/i2c-sysfs.h 2005-05-19 20:40:21.000000000 +0200
@@ -34,4 +34,14 @@
.index=_index, \
}

+#define SENSOR_DEVICE_ATTR_ARRAY_HEAD(_name,_size) \
+struct sensor_device_attribute sensor_dev_attr_##_name[_size] = {
+
+#define SENSOR_DEVICE_ATTR_ARRAY_ITEM(_name,_mode,_show,_store,_index) \
+ { .dev_attr=__ATTR(_name,_mode,_show,_store), \
+ .index=_index, },
+
+#define SENSOR_DEVICE_ATTR_ARRAY_TAIL \
+}
+
#endif /* _LINUX_I2C_SYSFS_H */


--
Jean Delvare

Greg KH

unread,
May 19, 2005, 4:50:11 PM5/19/05
to
On Thu, May 19, 2005 at 10:02:35PM +0200, Jean Delvare wrote:
> If we are into code refactoring and driver size shrinking, you may want
> to take a look at the following patch, which makes it87 even smaller
> (from 18976 bytes down to 16992 bytes on my system) and IMHO more
> cleaner.

But this doesn't reduce the binary size of the module, right?

> /* Register sysfs hooks */
> - device_create_file(&new_client->dev, &sensor_dev_attr_in0_input.dev_attr);
> - device_create_file(&new_client->dev, &sensor_dev_attr_in1_input.dev_attr);
> - device_create_file(&new_client->dev, &sensor_dev_attr_in2_input.dev_attr);
> - device_create_file(&new_client->dev, &sensor_dev_attr_in3_input.dev_attr);

<snip>

You know, we do have arrays of attributes that can be registered with a
single call...

I'd recommend using that over this mess anyday :)

> +#define SENSOR_DEVICE_ATTR_ARRAY_HEAD(_name,_size) \
> +struct sensor_device_attribute sensor_dev_attr_##_name[_size] = {
> +
> +#define SENSOR_DEVICE_ATTR_ARRAY_ITEM(_name,_mode,_show,_store,_index) \
> + { .dev_attr=__ATTR(_name,_mode,_show,_store), \
> + .index=_index, },
> +
> +#define SENSOR_DEVICE_ATTR_ARRAY_TAIL \
> +}

No, I hate HEAD and TAIL macros. This really isn't buying you much code
savings, you could do it yourself with the __ATTR() macro yourself with
the same ammount of code I bet...

Or use the new macro that Yani created, that will make it even smaller
:)

thanks,

greg k-h

Jean Delvare

unread,
May 19, 2005, 5:10:07 PM5/19/05
to
Hi Greg,

> > If we are into code refactoring and driver size shrinking, you may
> > want to take a look at the following patch, which makes it87 even
> > smaller (from 18976 bytes down to 16992 bytes on my system) and IMHO
> > more cleaner.
>
> But this doesn't reduce the binary size of the module, right?

It does, as I just said. The benefit is probably mainly due to the
introduction of loops around device_create_file() calls. The patch
reduces the number of calls (in the binary) from 59 to 20.

> You know, we do have arrays of attributes that can be registered with
> a single call...
>
> I'd recommend using that over this mess anyday :)

Yeah, I'll take a look into this at some point. This should make the
code even more readable and efficient.

> No, I hate HEAD and TAIL macros. This really isn't buying you much
> code savings, you could do it yourself with the __ATTR() macro
> yourself with the same ammount of code I bet...
>
> Or use the new macro that Yani created, that will make it even smaller
> :)

Agreed. This was really a quick hack, not meant for inclusion. Maybe I
should have polished it a bit more before I dared sending it. I'll do so
next time, sorry for the noise.

--
Jean Delvare

Yani Ioannou

unread,
May 19, 2005, 5:20:09 PM5/19/05
to
On 5/19/05, Jean Delvare <kh...@linux-fr.org> wrote:
> I finally gave a try to your patches, including the one for it87 which I
> used for testing. It all works like a charm, pretty impressive
> considering the overall complexity of the change. Congratulations :)

I'm impressed too ;), and very happy to finally have confirmation it
works as planned without blowing up anything.

> I'd like to add that the technical solution you came up with pleases me
> much (which may or may not be relevant).

Well I guess a happy Jean doesn't hurt! I really can't accept full
credit for this though, I think the end solution was a good mix of
ideas/comments from a range of people especially Greg, Russell and
you.

I like the idea of aggregating the related sensor attributes into
arrays, I did things the same way in bmcsensors but of course that was
all dynamic, and would just use more memory if we tried to do that
here. As Greg points out there is probably a nicer way to create the
arrays and register the attributes, but the basic idea has merit I
think, and if we can standardize a method across all the sensors then
its even better.

Thanks,
Yani

Greg KH

unread,
May 19, 2005, 5:40:05 PM5/19/05
to
On Thu, May 19, 2005 at 10:57:12PM +0200, Jean Delvare wrote:
> Hi Greg,
>
> > > If we are into code refactoring and driver size shrinking, you may
> > > want to take a look at the following patch, which makes it87 even
> > > smaller (from 18976 bytes down to 16992 bytes on my system) and IMHO
> > > more cleaner.
> >
> > But this doesn't reduce the binary size of the module, right?
>
> It does, as I just said. The benefit is probably mainly due to the
> introduction of loops around device_create_file() calls. The patch
> reduces the number of calls (in the binary) from 59 to 20.

Ah, sorry, I mistook that for a code decrease and not binary decrease.

> > No, I hate HEAD and TAIL macros. This really isn't buying you much
> > code savings, you could do it yourself with the __ATTR() macro
> > yourself with the same ammount of code I bet...
> >
> > Or use the new macro that Yani created, that will make it even smaller
> > :)
>
> Agreed. This was really a quick hack, not meant for inclusion. Maybe I
> should have polished it a bit more before I dared sending it. I'll do so
> next time, sorry for the noise.

No, don't feel like this was noise at all, it wasn't. I was just
commenting on the patch, letting you know that it was a great place to
start, but it might be tweaked a bit in places.

Don't worry about polishing stuff up before sending it in, you have seen
some of my patches, right? :)

Also, there is a neat trick that you can do every once in a while if you
use it sparingly. Propose a patch that you know is wrong, just to get
someone else (usually the maintainer of the area) to do it correctly as
they don't like your way at all. It's very effective when used in small
doses.

Hm, which makes me want to go look at trying to convert those attributes
to an array right now...

thanks,

greg k-h

Jean Delvare

unread,
May 20, 2005, 4:00:15 AM5/20/05
to

Hi Greg,

> Hm, which makes me want to go look at trying to convert those attributes
> to an array right now...

I should probably develop my own thoughts and plans at this point then.

If you consider all the attributes of a given hardware monitoring driver,
you'll notice that, from a functional point of view, they can be
represented as two-dimension arrays rather than one-dimension ones. For
example, the temperature-related files of the it87 driver could be
represented this way:

+-------------+-------------+-------------+-------------+
| temp1_input | temp1_min | temp1_max | temp1_type |
+-------------+-------------+-------------+-------------+
| temp2_input | temp2_min | temp2_max | temp2_type |
+-------------+-------------+-------------+-------------+
| temp3_input | temp3_min | temp3_max | temp3_type |
+-------------+-------------+-------------+-------------+

In the patch I just proposed, I made the choice to consider the colums of
the array above to create arrays of attributes, each column becoming a
different array. While it may sound like a sane choice, espcially when
there are many similar channels (e.g. voltages), it might not be the
best choice in the long run, for the following reason.

All measurement channels of the IT8705F and IT8712F can be individually
disabled. While this feature was almost not used so far, I think we
should have the driver not create interface files for disabled inputs.
In the case of temperature channels which can be dynamically enabled and
disabled. it would even make sense to dynamically create and delete
related sysfs files. Doing so would allow for memory savings and would
also be less error-prone for the user (presenting an interface for
disabled features is quite confusing IMHO).

For this reason, considering the lines of the array above, rather than
its columns, in order to define arrays of attributes, may be more subtle
and flexible in the long run.

Thanks
--
Jean Delvare

Yani Ioannou

unread,
May 20, 2005, 5:00:21 AM5/20/05
to
On 5/20/05, Jean Delvare <kh...@linux-fr.org> wrote:
>
> Hi Greg,

> disabled. While this feature was almost not used so far, I think we
> should have the driver not create interface files for disabled inputs.
> In the case of temperature channels which can be dynamically enabled and
> disabled. it would even make sense to dynamically create and delete
> related sysfs files. Doing so would allow for memory savings and would
> also be less error-prone for the user (presenting an interface for
> disabled features is quite confusing IMHO).

If you think more than a few hwmon/chip drivers will benefit from
dynamically creating the attributes, then maybe we can create some
standard method for doing so, bmcsensors of course needs to
dynamically allocate things, so I'd be using them too.

An earlier idea was to create a standard sysfs function(s) for
dynamically creating device_attributes (and others), but now we will
have custom structures with an embedded device_attribute, it looks to
me like each attribute type would require it's own function.

Yani

Dmitry Torokhov

unread,
May 21, 2005, 10:00:16 PM5/21/05
to
On Friday 20 May 2005 03:53, Yani Ioannou wrote:
> On 5/20/05, Jean Delvare <kh...@linux-fr.org> wrote:
> >
> > Hi Greg,
> > disabled. While this feature was almost not used so far, I think we
> > should have the driver not create interface files for disabled inputs.
> > In the case of temperature channels which can be dynamically enabled and
> > disabled. it would even make sense to dynamically create and delete
> > related sysfs files. Doing so would allow for memory savings and would
> > also be less error-prone for the user (presenting an interface for
> > disabled features is quite confusing IMHO).
>
> If you think more than a few hwmon/chip drivers will benefit from
> dynamically creating the attributes, then maybe we can create some
> standard method for doing so, bmcsensors of course needs to
> dynamically allocate things, so I'd be using them too.
>
> An earlier idea was to create a standard sysfs function(s) for
> dynamically creating device_attributes (and others), but now we will
> have custom structures with an embedded device_attribute, it looks to
> me like each attribute type would require it's own function.
>

I really think that as far as I2C subsystem goes instead of creating
arrays of attributes we should move in direction of drivers registering
individual sensor class devices. So for example it87 would register
3 fans, 3 temp, sensors and 8 voltage sensors...

--
Dmitry

Jean Delvare

unread,
May 22, 2005, 3:00:17 AM5/22/05
to
Hi Dmitry,

> I really think that as far as I2C subsystem goes instead of creating
> arrays of attributes we should move in direction of drivers
> registering individual sensor class devices. So for example it87 would
> register 3 fans, 3 temp, sensors and 8 voltage sensors...

First, it's a matter of hardware monitoring drivers, not i2c subsystem
(both are tightly binded at the moment but I'd like this to change).

Second, not all devices have the same attributes for a temperature, fan
or voltage channel. Sure there are commonly found feature sets, but some
channels will lack some feature (e.g. it87's in8 has no min and max
limits), other chips will provide additional features (extra limits or
enhanced configurability). So I don't think you can have all devices
(and thus all drivers) fit into a single sensor class.

But of course I can be convinced your approach is better, with patches.
I don't know classes that well myself.

Thanks,
--
Jean Delvare

Dmitry Torokhov

unread,
May 22, 2005, 3:10:12 AM5/22/05
to
On Sunday 22 May 2005 01:50, Jean Delvare wrote:
> Hi Dmitry,
>
> > I really think that as far as I2C subsystem goes instead of creating
> > arrays of attributes we should move in direction of drivers
> > registering individual sensor class devices. So for example it87 would
> > register 3 fans, 3 temp, sensors and 8 voltage sensors...
>
> First, it's a matter of hardware monitoring drivers, not i2c subsystem
> (both are tightly binded at the moment but I'd like this to change).
>

Right, it's just i2c is pretty much the only supplier of these for now.



> Second, not all devices have the same attributes for a temperature, fan
> or voltage channel. Sure there are commonly found feature sets, but some
> channels will lack some feature (e.g. it87's in8 has no min and max
> limits), other chips will provide additional features (extra limits or
> enhanced configurability). So I don't think you can have all devices
> (and thus all drivers) fit into a single sensor class.
>

Well, userspace code manages it somehow, plus nothing stops driver from
adding some additional attributes to class devices.



> But of course I can be convinced your approach is better, with patches.

Heh, I was afraid you'd say so... Input sysfs conversion first and then
we'll see...

--
Dmitry

Yani Ioannou

unread,
May 22, 2005, 8:20:10 AM5/22/05
to
On 5/22/05, Dmitry Torokhov <dtor...@ameritech.net> wrote:
> On Sunday 22 May 2005 01:50, Jean Delvare wrote:
> > Hi Dmitry,
> >
> > > I really think that as far as I2C subsystem goes instead of creating
> > > arrays of attributes we should move in direction of drivers
> > > registering individual sensor class devices. So for example it87 would
> > > register 3 fans, 3 temp, sensors and 8 voltage sensors...

Well lets see some code ;-), but yeah I have to agree with Jean here,
I don't see a nice way to standardize this across all hwmon drivers,
things just differ too much.

> > First, it's a matter of hardware monitoring drivers, not i2c subsystem
> > (both are tightly binded at the moment but I'd like this to change).

How is that change going anyway? I could really do with something
finalized, but the last I heard about it was Mark Hoffman's patch and
that didn't seem to go anywhere.

Yani

Jean Delvare

unread,
May 22, 2005, 8:40:15 AM5/22/05
to
Hi Yani,

> > > First, it's a matter of hardware monitoring drivers, not i2c
> > > subsystem (both are tightly binded at the moment but I'd like this
> > > to change).
>
> How is that change going anyway? I could really do with something
> finalized, but the last I heard about it was Mark Hoffman's patch and
> that didn't seem to go anywhere.

My bad, I didn't take the time to review Mark's work yet :(

Anyway there's a long long way to go before there is a true separation
between i2c and hwmon. Mark introduced a hwmon class, which is needed
but not sufficient. The biggest part of the work will be to move all
drivers abusing the i2c subsystem to the subsystem where they really
belong (isa/platform or superio), and get rid of i2c-isa. The hybrid
drivers (w83781d, it87 and lm78) promise to be no fun to convert. We'll
split them if that's what it takes.

--
Jean Delvare

Yani Ioannou

unread,
May 22, 2005, 9:10:12 AM5/22/05
to
> Anyway there's a long long way to go before there is a true separation
> between i2c and hwmon. Mark introduced a hwmon class, which is needed
> but not sufficient. The biggest part of the work will be to move all
> drivers abusing the i2c subsystem to the subsystem where they really
> belong (isa/platform or superio), and get rid of i2c-isa.

And i2c-ipmi remember? :) Does that mean Mark's class is finalized and
I can work with it? Or is there some more work to be done on that
front too? If so I can probably help out.

Yani

Jean Delvare

unread,
May 22, 2005, 9:50:08 AM5/22/05
to
Hi Yani,

> And i2c-ipmi remember? :) Does that mean Mark's class is finalized and
> I can work with it? Or is there some more work to be done on that
> front too? If so I can probably help out.

I can't tell, I didn't even give his code a try :( If you have some
spare time, try applying the proposed patches, see how they work for you
and how you could use the new class in bmcsensors.

However, as I understand it, the hwmon class will be added to the
drivers, it isn't meant to replace anything. This will simply let us
search for hwmon stuff by class rather than assuming that all hwmon
drivers are devices on i2c busses, so it's more like a helper for
user-space tools. I don't think it solves the hwmon-is-not-i2c issue on
the kernel side at all.

--
Jean Delvare

Jean Delvare

unread,
Jun 5, 2005, 5:00:23 AM6/5/05
to
Hi Yani, all,

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

I just noticed that this patch has a repeated coding style error:

+ struct sensor_device_attribute *sensor_attr= to_sensor_dev_attr(attr);

should be:

+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);

Can we apply this modified version instead? Or is an incremental patch
preferred?

----------------------------------------------------------

Signed-off-by: Yani Ioannou <yani.i...@gmail.com>
Signed-off-by: Jean Delvare <kh...@linux-fr.org>

drivers/i2c/chips/adm1026.c | 524 ++++++++++++++++++++------------------------
1 files changed, 241 insertions(+), 283 deletions(-)

--- linux-2.6.12-rc5.orig/drivers/i2c/chips/adm1026.c 2005-05-25 20:55:47.000000000 +0200
+++ linux-2.6.12-rc5/drivers/i2c/chips/adm1026.c 2005-06-05 10:35:11.000000000 +0200
@@ -29,6 +29,7 @@
#include <linux/jiffies.h>
#include <linux/i2c.h>
#include <linux/i2c-sensor.h>
+#include <linux/i2c-sysfs.h>
#include <linux/i2c-vid.h>

/* Addresses to scan */
@@ -710,19 +711,27 @@
return data;
}

-static ssize_t show_in(struct device *dev, char *buf, int nr)
+static ssize_t show_in(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", INS_FROM_REG(nr, data->in[nr]));
}
-static ssize_t show_in_min(struct device *dev, char *buf, int nr)
+static ssize_t show_in_min(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", INS_FROM_REG(nr, data->in_min[nr]));
}
-static ssize_t set_in_min(struct device *dev, const char *buf,
- size_t count, int nr)
+static ssize_t set_in_min(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct i2c_client *client = to_i2c_client(dev);
struct adm1026_data *data = i2c_get_clientdata(client);
int val = simple_strtol(buf, NULL, 10);
@@ -733,14 +742,19 @@
up(&data->update_lock);
return count;
}
-static ssize_t show_in_max(struct device *dev, char *buf, int nr)
+static ssize_t show_in_max(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", INS_FROM_REG(nr, data->in_max[nr]));
}
-static ssize_t set_in_max(struct device *dev, const char *buf,
- size_t count, int nr)
+static ssize_t set_in_max(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct i2c_client *client = to_i2c_client(dev);
struct adm1026_data *data = i2c_get_clientdata(client);
int val = simple_strtol(buf, NULL, 10);
@@ -752,34 +766,13 @@
return count;
}

-#define in_reg(offset) \
-static ssize_t show_in##offset (struct device *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_in(dev, buf, offset); \
-} \
-static ssize_t show_in##offset##_min (struct device *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_in_min(dev, buf, offset); \
-} \
-static ssize_t set_in##offset##_min (struct device *dev, struct device_attribute *attr, \
- const char *buf, size_t count) \
-{ \
- return set_in_min(dev, buf, count, offset); \
-} \
-static ssize_t show_in##offset##_max (struct device *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_in_max(dev, buf, offset); \
-} \
-static ssize_t set_in##offset##_max (struct device *dev, struct device_attribute *attr, \
- const char *buf, size_t count) \
-{ \
- return set_in_max(dev, buf, count, offset); \
-} \
-static DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in##offset, NULL); \
-static DEVICE_ATTR(in##offset##_min, S_IRUGO | S_IWUSR, \
- show_in##offset##_min, set_in##offset##_min); \
-static DEVICE_ATTR(in##offset##_max, S_IRUGO | S_IWUSR, \
- show_in##offset##_max, set_in##offset##_max);
+#define in_reg(offset) \
+static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in, \
+ NULL, offset); \
+static SENSOR_DEVICE_ATTR(in##offset##_min, S_IRUGO | S_IWUSR, \
+ show_in_min, set_in_min, offset); \
+static SENSOR_DEVICE_ATTR(in##offset##_max, S_IRUGO | S_IWUSR, \
+ show_in_max, set_in_max, offset);


in_reg(0);
@@ -842,30 +835,38 @@
return count;
}

-static DEVICE_ATTR(in16_input, S_IRUGO, show_in16, NULL);
-static DEVICE_ATTR(in16_min, S_IRUGO | S_IWUSR, show_in16_min, set_in16_min);
-static DEVICE_ATTR(in16_max, S_IRUGO | S_IWUSR, show_in16_max, set_in16_max);
+static SENSOR_DEVICE_ATTR(in16_input, S_IRUGO, show_in16, NULL, 16);
+static SENSOR_DEVICE_ATTR(in16_min, S_IRUGO | S_IWUSR, show_in16_min, set_in16_min, 16);
+static SENSOR_DEVICE_ATTR(in16_max, S_IRUGO | S_IWUSR, show_in16_max, set_in16_max, 16);




/* Now add fan read/write functions */

-static ssize_t show_fan(struct device *dev, char *buf, int nr)
+static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", FAN_FROM_REG(data->fan[nr],
data->fan_div[nr]));
}
-static ssize_t show_fan_min(struct device *dev, char *buf, int nr)
+static ssize_t show_fan_min(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", FAN_FROM_REG(data->fan_min[nr],
data->fan_div[nr]));
}
-static ssize_t set_fan_min(struct device *dev, const char *buf,
- size_t count, int nr)
+static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct i2c_client *client = to_i2c_client(dev);
struct adm1026_data *data = i2c_get_clientdata(client);
int val = simple_strtol(buf, NULL, 10);
@@ -878,23 +879,11 @@
return count;
}

-#define fan_offset(offset) \
-static ssize_t show_fan_##offset (struct device *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_fan(dev, buf, offset - 1); \
-} \
-static ssize_t show_fan_##offset##_min (struct device *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_fan_min(dev, buf, offset - 1); \
-} \
-static ssize_t set_fan_##offset##_min (struct device *dev, struct device_attribute *attr, \
- const char *buf, size_t count) \
-{ \
- return set_fan_min(dev, buf, count, offset - 1); \
-} \
-static DEVICE_ATTR(fan##offset##_input, S_IRUGO, show_fan_##offset, NULL); \
-static DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR, \
- show_fan_##offset##_min, set_fan_##offset##_min);
+#define fan_offset(offset) \
+static SENSOR_DEVICE_ATTR(fan##offset##_input, S_IRUGO, show_fan, NULL, \
+ offset - 1); \
+static SENSOR_DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR, \


+ show_fan_min, set_fan_min, offset - 1);

fan_offset(1);
fan_offset(2);
@@ -925,14 +914,19 @@
}

/* Now add fan_div read/write functions */
-static ssize_t show_fan_div(struct device *dev, char *buf, int nr)
+static ssize_t show_fan_div(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", data->fan_div[nr]);
}
-static ssize_t set_fan_div(struct device *dev, const char *buf,
- size_t count, int nr)
+static ssize_t set_fan_div(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct i2c_client *client = to_i2c_client(dev);
struct adm1026_data *data = i2c_get_clientdata(client);
int val,orig_div,new_div,shift;
@@ -966,17 +960,8 @@
}

#define fan_offset_div(offset) \
-static ssize_t show_fan_##offset##_div (struct device *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_fan_div(dev, buf, offset - 1); \
-} \
-static ssize_t set_fan_##offset##_div (struct device *dev, struct device_attribute *attr, \
- const char *buf, size_t count) \
-{ \
- return set_fan_div(dev, buf, count, offset - 1); \
-} \
-static DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR, \
- show_fan_##offset##_div, set_fan_##offset##_div);
+static SENSOR_DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR, \
+ show_fan_div, set_fan_div, offset - 1);

fan_offset_div(1);
fan_offset_div(2);
@@ -988,19 +973,27 @@
fan_offset_div(8);

/* Temps */
-static ssize_t show_temp(struct device *dev, char *buf, int nr)
+static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", TEMP_FROM_REG(data->temp[nr]));
}
-static ssize_t show_temp_min(struct device *dev, char *buf, int nr)
+static ssize_t show_temp_min(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", TEMP_FROM_REG(data->temp_min[nr]));
}
-static ssize_t set_temp_min(struct device *dev, const char *buf,
- size_t count, int nr)
+static ssize_t set_temp_min(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct i2c_client *client = to_i2c_client(dev);
struct adm1026_data *data = i2c_get_clientdata(client);
int val = simple_strtol(buf, NULL, 10);
@@ -1012,14 +1005,19 @@
up(&data->update_lock);
return count;
}
-static ssize_t show_temp_max(struct device *dev, char *buf, int nr)
+static ssize_t show_temp_max(struct device *dev, struct device_attribute *attr,
+ char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", TEMP_FROM_REG(data->temp_max[nr]));
}
-static ssize_t set_temp_max(struct device *dev, const char *buf,
- size_t count, int nr)
+static ssize_t set_temp_max(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct i2c_client *client = to_i2c_client(dev);
struct adm1026_data *data = i2c_get_clientdata(client);
int val = simple_strtol(buf, NULL, 10);
@@ -1031,48 +1029,34 @@
up(&data->update_lock);
return count;
}
-#define temp_reg(offset) \
-static ssize_t show_temp_##offset (struct device *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_temp(dev, buf, offset - 1); \
-} \
-static ssize_t show_temp_##offset##_min (struct device *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_temp_min(dev, buf, offset - 1); \
-} \
-static ssize_t show_temp_##offset##_max (struct device *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_temp_max(dev, buf, offset - 1); \
-} \
-static ssize_t set_temp_##offset##_min (struct device *dev, struct device_attribute *attr, \
- const char *buf, size_t count) \
-{ \
- return set_temp_min(dev, buf, count, offset - 1); \
-} \
-static ssize_t set_temp_##offset##_max (struct device *dev, struct device_attribute *attr, \
- const char *buf, size_t count) \
-{ \
- return set_temp_max(dev, buf, count, offset - 1); \
-} \
-static DEVICE_ATTR(temp##offset##_input, S_IRUGO, show_temp_##offset, NULL); \
-static DEVICE_ATTR(temp##offset##_min, S_IRUGO | S_IWUSR, \
- show_temp_##offset##_min, set_temp_##offset##_min); \
-static DEVICE_ATTR(temp##offset##_max, S_IRUGO | S_IWUSR, \
- show_temp_##offset##_max, set_temp_##offset##_max);
+
+#define temp_reg(offset) \
+static SENSOR_DEVICE_ATTR(temp##offset##_input, S_IRUGO, show_temp, \
+ NULL, offset - 1); \
+static SENSOR_DEVICE_ATTR(temp##offset##_min, S_IRUGO | S_IWUSR, \
+ show_temp_min, set_temp_min, offset - 1); \
+static SENSOR_DEVICE_ATTR(temp##offset##_max, S_IRUGO | S_IWUSR, \
+ show_temp_max, set_temp_max, offset - 1);


temp_reg(1);
temp_reg(2);
temp_reg(3);

-static ssize_t show_temp_offset(struct device *dev, char *buf, int nr)
+static ssize_t show_temp_offset(struct device *dev,
+ struct device_attribute *attr, char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", TEMP_FROM_REG(data->temp_offset[nr]));
}
-static ssize_t set_temp_offset(struct device *dev, const char *buf,
- size_t count, int nr)
+static ssize_t set_temp_offset(struct device *dev,
+ struct device_attribute *attr, const char *buf,
+ size_t count)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct i2c_client *client = to_i2c_client(dev);
struct adm1026_data *data = i2c_get_clientdata(client);
int val = simple_strtol(buf, NULL, 10);
@@ -1085,46 +1069,45 @@
return count;
}

-#define temp_offset_reg(offset) \
-static ssize_t show_temp_##offset##_offset (struct device *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_temp_offset(dev, buf, offset - 1); \
-} \
-static ssize_t set_temp_##offset##_offset (struct device *dev, struct device_attribute *attr, \
- const char *buf, size_t count) \
-{ \
- return set_temp_offset(dev, buf, count, offset - 1); \
-} \
-static DEVICE_ATTR(temp##offset##_offset, S_IRUGO | S_IWUSR, \
- show_temp_##offset##_offset, set_temp_##offset##_offset);
+#define temp_offset_reg(offset) \
+static SENSOR_DEVICE_ATTR(temp##offset##_offset, S_IRUGO | S_IWUSR, \
+ show_temp_offset, set_temp_offset, offset - 1);

temp_offset_reg(1);
temp_offset_reg(2);
temp_offset_reg(3);

-static ssize_t show_temp_auto_point1_temp_hyst(struct device *dev, char *buf,
- int nr)
+static ssize_t show_temp_auto_point1_temp_hyst(struct device *dev,
+ struct device_attribute *attr, char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", TEMP_FROM_REG(
ADM1026_FAN_ACTIVATION_TEMP_HYST + data->temp_tmin[nr]));
}
-static ssize_t show_temp_auto_point2_temp(struct device *dev, char *buf,
- int nr)
+static ssize_t show_temp_auto_point2_temp(struct device *dev,
+ struct device_attribute *attr, char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", TEMP_FROM_REG(data->temp_tmin[nr] +
ADM1026_FAN_CONTROL_TEMP_RANGE));
}
-static ssize_t show_temp_auto_point1_temp(struct device *dev, char *buf,
- int nr)
+static ssize_t show_temp_auto_point1_temp(struct device *dev,
+ struct device_attribute *attr, char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", TEMP_FROM_REG(data->temp_tmin[nr]));
}
-static ssize_t set_temp_auto_point1_temp(struct device *dev, const char *buf,
- size_t count, int nr)
+static ssize_t set_temp_auto_point1_temp(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct i2c_client *client = to_i2c_client(dev);
struct adm1026_data *data = i2c_get_clientdata(client);
int val = simple_strtol(buf, NULL, 10);
@@ -1137,46 +1120,27 @@
return count;
}

-#define temp_auto_point(offset) \
-static ssize_t show_temp##offset##_auto_point1_temp (struct device *dev, struct device_attribute *attr, \
- char *buf) \
-{ \
- return show_temp_auto_point1_temp(dev, buf, offset - 1); \
-} \
-static ssize_t set_temp##offset##_auto_point1_temp (struct device *dev, struct device_attribute *attr, \
- const char *buf, size_t count) \
-{ \
- return set_temp_auto_point1_temp(dev, buf, count, offset - 1); \
-} \
-static ssize_t show_temp##offset##_auto_point1_temp_hyst (struct device \
- *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_temp_auto_point1_temp_hyst(dev, buf, offset - 1); \
-} \
-static ssize_t show_temp##offset##_auto_point2_temp (struct device *dev, struct device_attribute *attr, \
- char *buf) \
-{ \
- return show_temp_auto_point2_temp(dev, buf, offset - 1); \
-} \
-static DEVICE_ATTR(temp##offset##_auto_point1_temp, S_IRUGO | S_IWUSR, \
- show_temp##offset##_auto_point1_temp, \
- set_temp##offset##_auto_point1_temp); \
-static DEVICE_ATTR(temp##offset##_auto_point1_temp_hyst, S_IRUGO, \
- show_temp##offset##_auto_point1_temp_hyst, NULL); \
-static DEVICE_ATTR(temp##offset##_auto_point2_temp, S_IRUGO, \
- show_temp##offset##_auto_point2_temp, NULL);
+#define temp_auto_point(offset) \
+static SENSOR_DEVICE_ATTR(temp##offset##_auto_point1_temp, S_IRUGO | S_IWUSR, \
+ show_temp_auto_point1_temp, set_temp_auto_point1_temp, \
+ offset - 1); \
+static SENSOR_DEVICE_ATTR(temp##offset##_auto_point1_temp_hyst, S_IRUGO, \
+ show_temp_auto_point1_temp_hyst, NULL, offset - 1); \
+static SENSOR_DEVICE_ATTR(temp##offset##_auto_point2_temp, S_IRUGO, \
+ show_temp_auto_point2_temp, NULL, offset - 1);

temp_auto_point(1);
temp_auto_point(2);
temp_auto_point(3);

-static ssize_t show_temp_crit_enable(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t show_temp_crit_enable(struct device *dev,
+ struct device_attribute *attr, char *buf)
{
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", (data->config1 & CFG1_THERM_HOT) >> 4);
}
-static ssize_t set_temp_crit_enable(struct device *dev, struct device_attribute *attr, const char *buf,
- size_t count)
+static ssize_t set_temp_crit_enable(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
{
struct i2c_client *client = to_i2c_client(dev);
struct adm1026_data *data = i2c_get_clientdata(client);
@@ -1192,24 +1156,27 @@
return count;
}

-static DEVICE_ATTR(temp1_crit_enable, S_IRUGO | S_IWUSR,
- show_temp_crit_enable, set_temp_crit_enable);
-
-static DEVICE_ATTR(temp2_crit_enable, S_IRUGO | S_IWUSR,
- show_temp_crit_enable, set_temp_crit_enable);
-
-static DEVICE_ATTR(temp3_crit_enable, S_IRUGO | S_IWUSR,
+#define temp_crit_enable(offset) \
+static DEVICE_ATTR(temp##offset##_crit_enable, S_IRUGO | S_IWUSR, \
show_temp_crit_enable, set_temp_crit_enable);

+temp_crit_enable(1);
+temp_crit_enable(2);
+temp_crit_enable(3);

-static ssize_t show_temp_crit(struct device *dev, char *buf, int nr)
+static ssize_t show_temp_crit(struct device *dev,
+ struct device_attribute *attr, char *buf)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct adm1026_data *data = adm1026_update_device(dev);
return sprintf(buf,"%d\n", TEMP_FROM_REG(data->temp_crit[nr]));
}
-static ssize_t set_temp_crit(struct device *dev, const char *buf,
- size_t count, int nr)
+static ssize_t set_temp_crit(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
{
+ struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+ int nr = sensor_attr->index;
struct i2c_client *client = to_i2c_client(dev);
struct adm1026_data *data = i2c_get_clientdata(client);
int val = simple_strtol(buf, NULL, 10);
@@ -1222,18 +1189,9 @@
return count;
}

-#define temp_crit_reg(offset) \
-static ssize_t show_temp_##offset##_crit (struct device *dev, struct device_attribute *attr, char *buf) \
-{ \
- return show_temp_crit(dev, buf, offset - 1); \
-} \
-static ssize_t set_temp_##offset##_crit (struct device *dev, struct device_attribute *attr, \
- const char *buf, size_t count) \
-{ \
- return set_temp_crit(dev, buf, count, offset - 1); \
-} \
-static DEVICE_ATTR(temp##offset##_crit, S_IRUGO | S_IWUSR, \
- show_temp_##offset##_crit, set_temp_##offset##_crit);
+#define temp_crit_reg(offset) \
+static SENSOR_DEVICE_ATTR(temp##offset##_crit, S_IRUGO | S_IWUSR, \
+ show_temp_crit, set_temp_crit, offset - 1);

temp_crit_reg(1);
temp_crit_reg(2);
@@ -1597,114 +1555,114 @@
adm1026_init_client(new_client);



/* Register sysfs hooks */

- device_create_file(&new_client->dev, &dev_attr_in0_input);
- device_create_file(&new_client->dev, &dev_attr_in0_max);
- device_create_file(&new_client->dev, &dev_attr_in0_min);
- device_create_file(&new_client->dev, &dev_attr_in1_input);
- device_create_file(&new_client->dev, &dev_attr_in1_max);
- device_create_file(&new_client->dev, &dev_attr_in1_min);
- device_create_file(&new_client->dev, &dev_attr_in2_input);
- device_create_file(&new_client->dev, &dev_attr_in2_max);
- device_create_file(&new_client->dev, &dev_attr_in2_min);
- device_create_file(&new_client->dev, &dev_attr_in3_input);
- device_create_file(&new_client->dev, &dev_attr_in3_max);
- device_create_file(&new_client->dev, &dev_attr_in3_min);
- device_create_file(&new_client->dev, &dev_attr_in4_input);
- device_create_file(&new_client->dev, &dev_attr_in4_max);
- device_create_file(&new_client->dev, &dev_attr_in4_min);
- device_create_file(&new_client->dev, &dev_attr_in5_input);
- device_create_file(&new_client->dev, &dev_attr_in5_max);
- device_create_file(&new_client->dev, &dev_attr_in5_min);
- device_create_file(&new_client->dev, &dev_attr_in6_input);
- device_create_file(&new_client->dev, &dev_attr_in6_max);
- device_create_file(&new_client->dev, &dev_attr_in6_min);
- device_create_file(&new_client->dev, &dev_attr_in7_input);
- device_create_file(&new_client->dev, &dev_attr_in7_max);
- device_create_file(&new_client->dev, &dev_attr_in7_min);
- device_create_file(&new_client->dev, &dev_attr_in8_input);
- device_create_file(&new_client->dev, &dev_attr_in8_max);
- device_create_file(&new_client->dev, &dev_attr_in8_min);
- device_create_file(&new_client->dev, &dev_attr_in9_input);
- device_create_file(&new_client->dev, &dev_attr_in9_max);
- device_create_file(&new_client->dev, &dev_attr_in9_min);
- device_create_file(&new_client->dev, &dev_attr_in10_input);
- device_create_file(&new_client->dev, &dev_attr_in10_max);
- device_create_file(&new_client->dev, &dev_attr_in10_min);
- device_create_file(&new_client->dev, &dev_attr_in11_input);
- device_create_file(&new_client->dev, &dev_attr_in11_max);
- device_create_file(&new_client->dev, &dev_attr_in11_min);
- device_create_file(&new_client->dev, &dev_attr_in12_input);
- device_create_file(&new_client->dev, &dev_attr_in12_max);
- device_create_file(&new_client->dev, &dev_attr_in12_min);
- device_create_file(&new_client->dev, &dev_attr_in13_input);
- device_create_file(&new_client->dev, &dev_attr_in13_max);
- device_create_file(&new_client->dev, &dev_attr_in13_min);
- device_create_file(&new_client->dev, &dev_attr_in14_input);
- device_create_file(&new_client->dev, &dev_attr_in14_max);
- device_create_file(&new_client->dev, &dev_attr_in14_min);
- device_create_file(&new_client->dev, &dev_attr_in15_input);
- device_create_file(&new_client->dev, &dev_attr_in15_max);
- device_create_file(&new_client->dev, &dev_attr_in15_min);
- device_create_file(&new_client->dev, &dev_attr_in16_input);
- device_create_file(&new_client->dev, &dev_attr_in16_max);
- device_create_file(&new_client->dev, &dev_attr_in16_min);
- device_create_file(&new_client->dev, &dev_attr_fan1_input);
- device_create_file(&new_client->dev, &dev_attr_fan1_div);
- device_create_file(&new_client->dev, &dev_attr_fan1_min);
- device_create_file(&new_client->dev, &dev_attr_fan2_input);
- device_create_file(&new_client->dev, &dev_attr_fan2_div);
- device_create_file(&new_client->dev, &dev_attr_fan2_min);
- device_create_file(&new_client->dev, &dev_attr_fan3_input);
- device_create_file(&new_client->dev, &dev_attr_fan3_div);
- device_create_file(&new_client->dev, &dev_attr_fan3_min);
- device_create_file(&new_client->dev, &dev_attr_fan4_input);
- device_create_file(&new_client->dev, &dev_attr_fan4_div);
- device_create_file(&new_client->dev, &dev_attr_fan4_min);
- device_create_file(&new_client->dev, &dev_attr_fan5_input);
- device_create_file(&new_client->dev, &dev_attr_fan5_div);
- device_create_file(&new_client->dev, &dev_attr_fan5_min);
- device_create_file(&new_client->dev, &dev_attr_fan6_input);
- device_create_file(&new_client->dev, &dev_attr_fan6_div);
- device_create_file(&new_client->dev, &dev_attr_fan6_min);
- device_create_file(&new_client->dev, &dev_attr_fan7_input);
- device_create_file(&new_client->dev, &dev_attr_fan7_div);
- device_create_file(&new_client->dev, &dev_attr_fan7_min);
- device_create_file(&new_client->dev, &dev_attr_fan8_input);
- device_create_file(&new_client->dev, &dev_attr_fan8_div);
- device_create_file(&new_client->dev, &dev_attr_fan8_min);
- device_create_file(&new_client->dev, &dev_attr_temp1_input);
- device_create_file(&new_client->dev, &dev_attr_temp1_max);
- device_create_file(&new_client->dev, &dev_attr_temp1_min);
- device_create_file(&new_client->dev, &dev_attr_temp2_input);
- device_create_file(&new_client->dev, &dev_attr_temp2_max);
- device_create_file(&new_client->dev, &dev_attr_temp2_min);
- device_create_file(&new_client->dev, &dev_attr_temp3_input);
- device_create_file(&new_client->dev, &dev_attr_temp3_max);
- device_create_file(&new_client->dev, &dev_attr_temp3_min);
- device_create_file(&new_client->dev, &dev_attr_temp1_offset);
- device_create_file(&new_client->dev, &dev_attr_temp2_offset);
- device_create_file(&new_client->dev, &dev_attr_temp3_offset);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in0_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in0_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in0_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in1_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in1_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in1_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in2_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in2_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in2_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in3_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in3_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in3_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in4_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in4_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in4_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in5_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in5_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in5_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in6_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in6_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in6_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in7_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in7_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in7_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in8_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in8_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in8_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in9_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in9_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in9_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in10_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in10_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in10_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in11_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in11_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in11_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in12_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in12_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in12_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in13_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in13_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in13_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in14_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in14_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in14_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in15_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in15_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in15_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in16_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in16_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_in16_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan1_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan1_div.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan1_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan2_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan2_div.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan2_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan3_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan3_div.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan3_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan4_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan4_div.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan4_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan5_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan5_div.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan5_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan6_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan6_div.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan6_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan7_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan7_div.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan7_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan8_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan8_div.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_fan8_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp1_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp1_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp1_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp2_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp2_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp2_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp3_input.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp3_max.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp3_min.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp1_offset.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp2_offset.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp3_offset.dev_attr);
device_create_file(&new_client->dev,
- &dev_attr_temp1_auto_point1_temp);
+ &sensor_dev_attr_temp1_auto_point1_temp.dev_attr);
device_create_file(&new_client->dev,
- &dev_attr_temp2_auto_point1_temp);
+ &sensor_dev_attr_temp2_auto_point1_temp.dev_attr);
device_create_file(&new_client->dev,
- &dev_attr_temp3_auto_point1_temp);
+ &sensor_dev_attr_temp3_auto_point1_temp.dev_attr);
device_create_file(&new_client->dev,
- &dev_attr_temp1_auto_point1_temp_hyst);
+ &sensor_dev_attr_temp1_auto_point1_temp_hyst.dev_attr);
device_create_file(&new_client->dev,
- &dev_attr_temp2_auto_point1_temp_hyst);
+ &sensor_dev_attr_temp2_auto_point1_temp_hyst.dev_attr);
device_create_file(&new_client->dev,
- &dev_attr_temp3_auto_point1_temp_hyst);
+ &sensor_dev_attr_temp3_auto_point1_temp_hyst.dev_attr);
device_create_file(&new_client->dev,
- &dev_attr_temp1_auto_point2_temp);
+ &sensor_dev_attr_temp1_auto_point2_temp.dev_attr);
device_create_file(&new_client->dev,
- &dev_attr_temp2_auto_point2_temp);
+ &sensor_dev_attr_temp2_auto_point2_temp.dev_attr);
device_create_file(&new_client->dev,
- &dev_attr_temp3_auto_point2_temp);
- device_create_file(&new_client->dev, &dev_attr_temp1_crit);
- device_create_file(&new_client->dev, &dev_attr_temp2_crit);
- device_create_file(&new_client->dev, &dev_attr_temp3_crit);
+ &sensor_dev_attr_temp3_auto_point2_temp.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp1_crit.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp2_crit.dev_attr);
+ device_create_file(&new_client->dev, &sensor_dev_attr_temp3_crit.dev_attr);
device_create_file(&new_client->dev, &dev_attr_temp1_crit_enable);
device_create_file(&new_client->dev, &dev_attr_temp2_crit_enable);
device_create_file(&new_client->dev, &dev_attr_temp3_crit_enable);

Greg KH

unread,
Jun 6, 2005, 3:30:19 AM6/6/05
to
On Sun, Jun 05, 2005 at 10:51:46AM +0200, Jean Delvare wrote:
> Hi Yani, all,
>
> > 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.
>
> I just noticed that this patch has a repeated coding style error:
>
> + struct sensor_device_attribute *sensor_attr= to_sensor_dev_attr(attr);
>
> should be:
>
> + struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>
> Can we apply this modified version instead? Or is an incremental patch
> preferred?

I've replaced the version in my tree with this one, thanks.

greg k-h

0 new messages