i2c: new sysfs class "hwmon"
cover i2c: new sysfs class "hwmon"
Hi Khali, Greg, et. al:
After a brief chat w/ Khali last week (and some reading LDDv3) I did these patches. The result for me looks like this:
/sys/class/hwmon/1-0290/device/ -> /sys/devices/platform/i2c-1/1-0290
Notes/Questions:
1. I don't like "1-0290" for a class ID, but it was the easiest thing to use at the time. A better name might be w83627thf-0.
2. The kernel complains if I don't provide a class::release function, but I can't really see a need for it in this case. If I hold a file open through the /sys/class/hwmon/X/device/Y link and then try to rmmod the driver, the rmmod will sleep until I close the file just as I expect. Any hints here Greg?
3. I added the struct class_device to struct i2c_client. This makes it slightly easier to modify the sensors drivers... but not all i2c_clients will need it (bloat). It could be added to each struct <driver>_data (e.g. w83627hf_data) instead.
4. I didn't modify all the chips, just w83627hf.c and lm78.c as examples. (IIRC Khali experiments a lot on lm78).
Comments?
Regards,
Signature · 2 lines
Mark M. Hoffman [email protected]
cover i2c: new sysfs class "hwmon"
Quoted 8 lines · Mark M. Hoffman
Hi Khali, Greg, et. al:
After a brief chat w/ Khali last week (and some reading LDDv3) I did these patches. The result for me looks like this:
/sys/class/hwmon/1-0290/device/->/sys/devices/platform/i2c-1/1-0290Notes/Questions:
First a question from me, why? What are you trying to do here? Who will be using the hwmon class? And what for? Will all "sensor-like" sysfs files move to this new class, or is it just to enable userspace to be able to find the hwmon devices easier?
Quoted 2 lines
1. I don't like "1-0290" for a class ID, but it was the easiest thing to use at the time. A better name might be w83627thf-0.
As long as it's unique, that's the only requirement.
Quoted 5 lines
2. The kernel complains if I don't provide a
class::releasefunction, but I can't really see a need for it in this case. If I hold a file open through the/sys/class/hwmon/X/device/Ylink and then try to rmmod the driver, the rmmod will sleep until I close the file just as I expect. Any hints here Greg?
Ick, yes, you _must_ have a release function. _Please_ listen to the kernel, it isn't saying those things for no reason... If you add a file to the class device, and do not have a release function, then you can have a memory leak. The way that you use the class device owned by a different device is not safe, nor a good idea. I recommend using the "class_device_create()" function that has been added to the -mm tree (or the class_simple stuff that's in mainline, if you want an idea of what it is. Hm, no that will not quite work, that's why I wrote the class_device_create() call...use that.)
Quoted 4 lines
3. I added the struct
class_deviceto structi2c_client. This makes it slightly easier to modify the sensors drivers... but not alli2c_clientswill need it (bloat). It could be added to each struct <driver>_data (e.g.w83627hf_data) instead.
Use a pointer instead, and the class_device_create function.
thanks,
greg k-h
cover i2c: new sysfs class "hwmon"
In reply to Mark M. HoffmanHi Greg, thanks for your advice:
Quoted 6 lines · Greg KH
Quoted 5 lines · Mark M. Hoffman
Hi Khali, Greg, et. al:
After a brief chat w/ Khali last week (and some reading LDDv3) I did these patches. The result for me looks like this:
Quoted 8 lines
Quoted 3 lines
/sys/class/hwmon/1-0290/device/->/sys/devices/platform/i2c-1/1-0290Notes/Questions:
First a question from me, why? What are you trying to do here? Who will be using the hwmon class? And what for? Will all "sensor-like" sysfs files move to this new class, or is it just to enable userspace to be able to find the hwmon devices easier?
Khali is working on patches to get rid of the i2c-isa 'pseudo-adapter'. Since any sensor chip attached to the ISA bus will no longer appear in the /sys/bus/i2c tree, a new way is needed. Userspace tools can be taught to look in /sys/class/hwmon first, and then /sys/bus/i2c for backwards-compatibility.
Quoted 4 lines
Quoted 2 lines
1. I don't like "1-0290" for a class ID, but it was the easiest thing to use at the time. A better name might be w83627thf-0.
As long as it's unique, that's the only requirement.
I just strlcpy'd the '1-0290' ID from the /sys/bus/i2c tree because it is unique. My concern is that it's not very descriptive.
Quoted 9 lines
Quoted 5 lines
2. The kernel complains if I don't provide a
class::releasefunction, but I can't really see a need for it in this case. If I hold a file open through the/sys/class/hwmon/X/device/Ylink and then try to rmmod the driver, the rmmod will sleep until I close the file just as I expect. Any hints here Greg?Ick, yes, you _must_ have a release function. _Please_ listen to the kernel, it isn't saying those things for no reason... If you add a file to the class device, and do not have a release function, then you can
But... I didn't add any files to the class device. ;) I'm not planning to move any existing files (temp1_input, etc) - I just want the link back to the device. It didn't look like that (by itself) should require a release function, but I'll go back and read it again.
Quoted 2 lines
have a memory leak. The way that you use the class device owned by a different device is not safe, nor a good idea. I recommend using the
Err... what about struct i2c_adapter?
Quoted 4 lines
"
class_device_create()" function that has been added to the -mm tree (or theclass_simplestuff that's in mainline, if you want an idea of what it is. Hm, no that will not quite work, that's why I wrote theclass_device_create()call...use that.)
OK, I'll try it again that way.
Quoted 6 lines
Quoted 4 lines
3. I added the struct
class_deviceto structi2c_client. This makes it slightly easier to modify the sensors drivers... but not alli2c_clientswill need it (bloat). It could be added to each struct <driver>_data (e.g.w83627hf_data) instead.Use a pointer instead, and the
class_device_createfunction.
OK.
Thanks and regards,
Signature · 2 lines
Mark M. Hoffman [email protected]
cover i2c: new sysfs class "hwmon"
In reply to Mark M. HoffmanQuoted 18 lines · Mark M. Hoffman
Hi Greg, thanks for your advice:
Quoted 6 lines · Greg KH
Quoted 5 lines · Mark M. Hoffman
Hi Khali, Greg, et. al:
After a brief chat w/ Khali last week (and some reading LDDv3) I did these patches. The result for me looks like this:
Quoted 8 lines
Quoted 3 lines
/sys/class/hwmon/1-0290/device/->/sys/devices/platform/i2c-1/1-0290Notes/Questions:
First a question from me, why? What are you trying to do here? Who will be using the hwmon class? And what for? Will all "sensor-like" sysfs files move to this new class, or is it just to enable userspace to be able to find the hwmon devices easier?
Quoted 5 lines
Khali is working on patches to get rid of the i2c-isa 'pseudo-adapter'. Since any sensor chip attached to the ISA bus will no longer appear in the
/sys/bus/i2ctree, a new way is needed. Userspace tools can be taught to look in/sys/class/hwmonfirst, and then/sys/bus/i2cfor backwards-compatibility.
Ok, that sounds acceptable.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
1. I don't like "1-0290" for a class ID, but it was the easiest thing to use at the time. A better name might be w83627thf-0.
As long as it's unique, that's the only requirement.
I just strlcpy'd the '1-0290' ID from the
/sys/bus/i2ctree because it is unique. My concern is that it's not very descriptive.
True, pick something else if you don't like that :)
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
2. The kernel complains if I don't provide a
class::releasefunction, but I can't really see a need for it in this case. If I hold a file open through the/sys/class/hwmon/X/device/Ylink and then try to rmmod the driver, the rmmod will sleep until I close the file just as I expect. Any hints here Greg?
Quoted 8 lines
Quoted 3 lines
Ick, yes, you _must_ have a release function. _Please_ listen to the kernel, it isn't saying those things for no reason... If you add a file to the class device, and do not have a release function, then you can
But... I didn't add any files to the class device. ;) I'm not planning to move any existing files (
temp1_input, etc) - I just want the link back to the device. It didn't look like that (by itself) should require a release function, but I'll go back and read it again.
Ok, if you have _no_ sysfs files, and you have no where anyone else could ever grab ahold of a reference to you, then no, you don't need a release function. But that's really not how a class_device structure should be used.
Quoted 4 lines
Quoted 2 lines
have a memory leak. The way that you use the class device owned by a different device is not safe, nor a good idea. I recommend using the
Err... what about struct
i2c_adapter?
Yeah, that's a hack, I wouldn't look at how I intregrated the i2c code into the driver core as _any_ model that should be copied :)
Quoted 6 lines
Quoted 4 lines
"
class_device_create()" function that has been added to the -mm tree (or theclass_simplestuff that's in mainline, if you want an idea of what it is. Hm, no that will not quite work, that's why I wrote theclass_device_create()call...use that.)OK, I'll try it again that way.
Thanks, I think you'll find it a lot easier.
greg k-h
cover i2c: new sysfs class "hwmon"
In reply to Mark M. HoffmanQuoted 22 lines · Jean Delvare
Quoted 7 lines
Quoted 5 lines
Khali is working on patches to get rid of the i2c-isa 'pseudo-adapter'. Since any sensor chip attached to the ISA bus will no longer appear in the
/sys/bus/i2ctree, a new way is needed. Userspace tools can be taught to look in/sys/class/hwmonfirst, and then/sys/bus/i2cfor backwards-compatibility.Ok, that sounds acceptable.
As Mark underlined, it's not only acceptable. It's needed. Newer Super-I/O hardware monitoring logical devices just don't fit in the i2c subsystem (for a reason: they are not I2C devices at all). These drivers are currently very ugly and their registration is inefficient and overall broken.
The key here is that, from the early days, it was assumed that i2c and hardware monitoring were the same thing. That's simply not true, which is the reason why I want us to clean up the mess now, and have drivers really attach to where they belong instead of an arbitrary bus. The embedded folks will thank us when they won't need to load i2c-core for their ISA hardware monitoring devices anymore (let alone the shrink in the size of the drivers themselves).
Quoted 4 lines
That being said, if your "acceptable" means that Mark's approach is not the prefered way to do it, I'd certainly like to hear about the proper way. We will change the hardware monitoring device drivers in a major way, so we better get it right.
Sorry for the vague wording, no this approach sounds very good, and correct. In fact, someone on lkml yesterday was wanting just this kind of class to handle their driver, which provided hwmon type support, yet it is not a i2c driver.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
1. I don't like "1-0290" for a class ID, but it was the easiest thing to use at the time. A better name might be w83627thf-0.
As long as it's unique, that's the only requirement.
I just strlcpy'd the '1-0290' ID from the
/sys/bus/i2ctree because it is unique. My concern is that it's not very descriptive.
Quoted 8 lines
Quoted 1 line
True, pick something else if you don't like that :)
Once the ISA hardware monitoring drivers will have moved outside of the i2c subsystem, their old client ID will make no more sense (the bus part, that is), and will be changed to something that makes sense for an ISA device. I don't know how other ISA device drivers do, but using the base address alone would make sense. This means that 1-0290 would become 0290.
That sounds good.
Quoted 6 lines
The reason why I mention that at this point is that it means that the various driver types will have different ID schemes, so using the same ID for the device and for its class sounds dangerous, as we wouldn't be able to guarantee the uniqueness. (This is all new to me, I didn't know that class entries needed an ID on their own.) Maybe we can take a look at what other classes chose as an ID scheme?
They just use a unique value, as an example, look at the i2c-adapter class. We can just have the hwmon class create a unique id itself, not based on anything else, which would be simple and easy, and probably confusing to users :)
Quoted 8 lines
Quoted 4 lines
Ok, if you have _no_ sysfs files, and you have no where anyone else could ever grab ahold of a reference to you, then no, you don't need a release function. But that's really not how a
class_devicestructure should be used.How should it be used then? Do you suggest that all sysfs files should be "physically" moved from the i2c device to the hwmon class? This would break compatibility with the current user-space stuff :(
No, I'm not saying that.
Quoted 2 lines
I thought that classes were really only links to where the devices were in the sysfs tree. Looks like I was wrong.
Look at all of the files in the /sys/class/ tree that are not symlinks.
Anyway, the class_device_create() function should solve this issue.
thanks,
greg k-h
cover i2c: new sysfs class "hwmon"
In reply to Mark M. HoffmanJust a note that this is also very important for moving away from i2c-ipmi so the bmcsensors driver can be included in 2.6, so I'll be working with Jean and Mark on this (there is a thread from last week where Jean and I were discussing the problem). Unfortunately "real" work has been keeping me busy since that discussion, but I hope to get around to it this weekend.
Jean, in reply to your message last week, separating i2c-ipmi is much easier than the task you have of separating i2c-isa since there is no overlap at all for ipmi as there seems to be for isa accessible chips. When I get home I'll send my preliminary patch for dynamic sysfs callbacks to you and get your opinion on things :-).
Thanks,
Yani
Quoted 81 lines · Jean Delvare
Hi Mark, hi Greg,
Sorry for joining the thread late. Don't misinterpret this, I am *very* interested by what Mark proposes.
Greg, I am working at one end of the problem (making it possible to separate ISA hardware monitoring drivers from the i2c-core), while Mark works at the other end (preparing a hwmon class which is independant from i2c so all hardware monitoring device drivers can use it regardless of the bus they are connected to). Hopefully we'll meet at some point, and the job will be done.
Quoted 7 lines
Quoted 5 lines
Khali is working on patches to get rid of the i2c-isa 'pseudo-adapter'. Since any sensor chip attached to the ISA bus will no longer appear in the
/sys/bus/i2ctree, a new way is needed. Userspace tools can be taught to look in/sys/class/hwmonfirst, and then/sys/bus/i2cfor backwards-compatibility.Ok, that sounds acceptable.
As Mark underlined, it's not only acceptable. It's needed. Newer Super-I/O hardware monitoring logical devices just don't fit in the i2c subsystem (for a reason: they are not I2C devices at all). These drivers are currently very ugly and their registration is inefficient and overall broken.
The key here is that, from the early days, it was assumed that i2c and hardware monitoring were the same thing. That's simply not true, which is the reason why I want us to clean up the mess now, and have drivers really attach to where they belong instead of an arbitrary bus. The embedded folks will thank us when they won't need to load i2c-core for their ISA hardware monitoring devices anymore (let alone the shrink in the size of the drivers themselves).
That being said, if your "acceptable" means that Mark's approach is not the prefered way to do it, I'd certainly like to hear about the proper way. We will change the hardware monitoring device drivers in a major way, so we better get it right.
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
1. I don't like "1-0290" for a class ID, but it was the easiest thing to use at the time. A better name might be w83627thf-0.
As long as it's unique, that's the only requirement.
I just strlcpy'd the '1-0290' ID from the
/sys/bus/i2ctree because it is unique. My concern is that it's not very descriptive.True, pick something else if you don't like that :)
Once the ISA hardware monitoring drivers will have moved outside of the i2c subsystem, their old client ID will make no more sense (the bus part, that is), and will be changed to something that makes sense for an ISA device. I don't know how other ISA device drivers do, but using the base address alone would make sense. This means that 1-0290 would become 0290.
The reason why I mention that at this point is that it means that the various driver types will have different ID schemes, so using the same ID for the device and for its class sounds dangerous, as we wouldn't be able to guarantee the uniqueness. (This is all new to me, I didn't know that class entries needed an ID on their own.) Maybe we can take a look at what other classes chose as an ID scheme?
Quoted 4 lines
Ok, if you have _no_ sysfs files, and you have no where anyone else could ever grab ahold of a reference to you, then no, you don't need a release function. But that's really not how a
class_devicestructure should be used.How should it be used then? Do you suggest that all sysfs files should be "physically" moved from the i2c device to the hwmon class? This would break compatibility with the current user-space stuff :(
I thought that classes were really only links to where the devices were in the sysfs tree. Looks like I was wrong.
Thanks,
--
Jean Delvare
cover i2c: new sysfs class "hwmon"
In reply to Mark M. HoffmanHi,
Sorry for breaking the thread (I was not subscribed to this list)...
I think that just simply linking current i2c device attributes intoclass structure is not the best thing to do. Since userspace needs tobe changed anyway (to a certain extent) we should evaluate whether itwould be beneficial to change layout too. I would like to see something like this:
sys/class/sensors---fan---0---(min,in,div) | | | --1---(min,in,div) | |-temp---0---(type,min,max,hyst?) | | | --1---(type,min,max,hyst..) | --curr----0---(min,max,in)
This of course depends on Greg adding support for nested classes tothe driver code (how is it going, Greg?). Alternatively we couldflatten a tree a bit doingfan%d, temp%d...
We would have: sensors_register_fan, sensors_register_temp,sensors_register_curr, sensors_register_generic...
struct sensor_fan_dev { char name[MAX_SENSOR_NAME] int min, div; struct class_device class_dev;
int (*read)(struct sensor_temp_dev *); int (*start)(struct sensor_temp_dev *, int speed)};
struct sensor_temp_dev { char name[MAX_SENSOR_NAME] int type, min, max, hyst; struct class_device class_dev;
int (*read)(struct sensor_temp_dev *);};This way every sensor input is registered as an independent classdevice, all their attributes are kept together, names are muchsimplier and there are less chances of clashes and I believe they areeasier to maintain. Other subsystems should also be able to add to thehierarchy without knowing anything about i2c internals. What do you think?
-- Dmitry
cover i2c: new sysfs class "hwmon"
In reply to Mark M. HoffmanHi Mark, hi Greg,
Sorry for joining the thread late. Don't misinterpret this, I am *very* interested by what Mark proposes.
Greg, I am working at one end of the problem (making it possible to separate ISA hardware monitoring drivers from the i2c-core), while Mark works at the other end (preparing a hwmon class which is independant from i2c so all hardware monitoring device drivers can use it regardless of the bus they are connected to). Hopefully we'll meet at some point, and the job will be done.
Quoted 7 lines
Quoted 5 lines
Khali is working on patches to get rid of the i2c-isa 'pseudo-adapter'. Since any sensor chip attached to the ISA bus will no longer appear in the
/sys/bus/i2ctree, a new way is needed. Userspace tools can be taught to look in/sys/class/hwmonfirst, and then/sys/bus/i2cfor backwards-compatibility.Ok, that sounds acceptable.
As Mark underlined, it's not only acceptable. It's needed. Newer Super-I/O hardware monitoring logical devices just don't fit in the i2c subsystem (for a reason: they are not I2C devices at all). These drivers are currently very ugly and their registration is inefficient and overall broken.
The key here is that, from the early days, it was assumed that i2c and hardware monitoring were the same thing. That's simply not true, which is the reason why I want us to clean up the mess now, and have drivers really attach to where they belong instead of an arbitrary bus. The embedded folks will thank us when they won't need to load i2c-core for their ISA hardware monitoring devices anymore (let alone the shrink in the size of the drivers themselves).
That being said, if your "acceptable" means that Mark's approach is not the prefered way to do it, I'd certainly like to hear about the proper way. We will change the hardware monitoring device drivers in a major way, so we better get it right.
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
1. I don't like "1-0290" for a class ID, but it was the easiest thing to use at the time. A better name might be w83627thf-0.
As long as it's unique, that's the only requirement.
I just strlcpy'd the '1-0290' ID from the
/sys/bus/i2ctree because it is unique. My concern is that it's not very descriptive.True, pick something else if you don't like that :)
Once the ISA hardware monitoring drivers will have moved outside of the i2c subsystem, their old client ID will make no more sense (the bus part, that is), and will be changed to something that makes sense for an ISA device. I don't know how other ISA device drivers do, but using the base address alone would make sense. This means that 1-0290 would become 0290.
The reason why I mention that at this point is that it means that the various driver types will have different ID schemes, so using the same ID for the device and for its class sounds dangerous, as we wouldn't be able to guarantee the uniqueness. (This is all new to me, I didn't know that class entries needed an ID on their own.) Maybe we can take a look at what other classes chose as an ID scheme?
Quoted 4 lines
Ok, if you have _no_ sysfs files, and you have no where anyone else could ever grab ahold of a reference to you, then no, you don't need a release function. But that's really not how a
class_devicestructure should be used.
How should it be used then? Do you suggest that all sysfs files should be "physically" moved from the i2c device to the hwmon class? This would break compatibility with the current user-space stuff :(
I thought that classes were really only links to where the devices were in the sysfs tree. Looks like I was wrong.
Thanks,
--
Jean Delvare
cover i2c: new sysfs class "hwmon"
In reply to Mark M. HoffmanQuoted 3 lines · Dmitry Torokhov
This of course depends on Greg adding support for nested classes to the driver code (how is it going, Greg?).
The locking code is still stewing in the -mm tree. Once all of those changes hit mainline, we can start adding new stuff like nested classes.
thanks,
greg k-h