Using hwmon in-kernel
Subsystems (23)
I'm possibly missing something here, but. I'm working on improving the thermal management of nvidia cards in nouveau. I've got I2C up and running and I can bind appropriate hwmon drivers to the chips and read temperatures. I've got a couple of remaining problems:
1) I know the offsets that need to be applied to the lm99 sensors. How do I do that in-kernel?
2) How do I read the temperature in-kernel? This only seems to be exported via sysfs, which is obviously not ideal (I'm not willing to leave the thermal management up to userspace).
Are these possible?
Thanks,
Signature · 6 lines
Matthew Garrett | [email protected] _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Hi Matthew,
Quoted 7 lines · Matthew Garrett
I'm possibly missing something here, but. I'm working on improving the thermal management of nvidia cards in nouveau. I've got I2C up and running and I can bind appropriate hwmon drivers to the chips and read temperatures. I've got a couple of remaining problems:
1) I know the offsets that need to be applied to the lm99 sensors. How do I do that in-kernel?
Depends on what you're speaking of exactly. If you refer to the +16 degrees offset that must be applied to all LM99 readings per hardware design, the problem is that the LM89 can't be differentiated from the LM99 by probing (they have the same die revision code register value.) The LM99 needs the +16 degrees offset while the LM89 doesn't, so it isn't possible to get both right when the devices are auto-detected. For this reason, I decided to not handle the offset in the lm90 driver. Instead, it is handled in user-space using a compute statement in /etc/sensors.conf, assuming that users know whether they have an LM89 or LM99 chip.
As the lm90 driver has been converted to a new-style i2c device driver, it is now possible to explicitly instantiate an LM89 or LM99 chip, without relying on auto-detection. So I admit that it would make sense to handle the +16 degrees offset in the driver for the LM99. This will cause some compatibility issues during the transition period though, as using a recent lm90 driver with an old sensors.conf file will report a temperature 16 degrees higher than it really is.
If you instead refer to a board-specific offset that should be applied to compensate for the distance between the thermal sensor and the graphics core, or for a non-standard thermal diode, the lm90 driver exposes attribute temp2_offset so user-space can set and read the temperature offset.
Quoted 3 lines
2) How do I read the temperature in-kernel? This only seems to be exported via sysfs, which is obviously not ideal (I'm not willing to leave the thermal management up to userspace).
You are right, there is no in-kernel interface to read temperature values, only a sysfs interface. The reason for that is that in most cases, the kernel doesn't have enough information about the wirings to make the information useful. The labelling and voltage scaling is all done in user-space.
Why do you want to retrieve the temperature value from the kernel? Please explain your use case.
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 5 lines · Jean Delvare
If you instead refer to a board-specific offset that should be applied to compensate for the distance between the thermal sensor and the graphics core, or for a non-standard thermal diode, the lm90 driver exposes attribute
temp2_offsetso user-space can set and read the temperature offset.
Right. My kernel driver is in the privileged position of knowing precisely what offset should be applied to the lm90 readings, so doing this in-kernel would be advantageous :)
Quoted 2 lines
Why do you want to retrieve the temperature value from the kernel? Please explain your use case.
I'm implementing power management for GPUs. These typically have several different performance constraints, but one of them is chip temperature. The maximum supported temperature is generally exported via tables in the graphics card BIOS, so it's necesssary for the kernel driver to be aware of the current temperature in order to limit the available performance modes to ensure the GPU stays within its thermal envelope.
Signature · 6 lines
Matthew Garrett | [email protected] _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Hi Matthew,
Quoted 11 lines · Matthew Garrett
Quoted 5 lines · Jean Delvare
If you instead refer to a board-specific offset that should be applied to compensate for the distance between the thermal sensor and the graphics core, or for a non-standard thermal diode, the lm90 driver exposes attribute
temp2_offsetso user-space can set and read the temperature offset.Right. My kernel driver is in the privileged position of knowing precisely what offset should be applied to the lm90 readings, so doing this in-kernel would be advantageous :)
There's nothing preventing you from accessing the LM99's registers directly and retrieve the temperature that way. Alternatively, we could add an internal interface to access some of the hwmon device features. It would take some time to define something everybody agrees on. If you have an interest in this, please make a proposal and we can discuss it.
Quoted 9 lines
Quoted 2 lines
Why do you want to retrieve the temperature value from the kernel? Please explain your use case.
I'm implementing power management for GPUs. These typically have several different performance constraints, but one of them is chip temperature. The maximum supported temperature is generally exported via tables in the graphics card BIOS, so it's necesssary for the kernel driver to be aware of the current temperature in order to limit the available performance modes to ensure the GPU stays within its thermal envelope.
OK, I see. Then indeed it makes sense to deviate from the traditional hwmon model. You could prevent auto-detection of the hwmon device (by dropping I2C_CLASS_HWMON from i2c_adapter.class) and instantiate the lm99 device manually instead (using i2c_new_device()). This gives you two things: a handle on the created device (so that you can access the chip registers directly if needed, and its private data too) and the possibility to pass platform data to the driver for specific initialization purposes. The lm90 driver doesn't implement the later yet, but we have another driver doing that (lm87) so it could be added if needed.
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 10 lines · Jean Delvare
Hi Matthew,
Quoted 7 lines · Matthew Garrett
Quoted 5 lines · Jean Delvare
If you instead refer to a board-specific offset that should be applied to compensate for the distance between the thermal sensor and the graphics core, or for a non-standard thermal diode, the lm90 driver exposes attribute
temp2_offsetso user-space can set and read the temperature offset.
Quoted 7 lines
Quoted 3 lines
Right. My kernel driver is in the privileged position of knowing precisely what offset should be applied to the lm90 readings, so doing this in-kernel would be advantageous :)
There's nothing preventing you from accessing the LM99's registers directly and retrieve the temperature that way. Alternatively, we could add an internal interface to access some of the hwmon device features.
I think that we really should be thinking about adding an internal interface, I think other parts of the kernel poking at IC registers where the IC is managed by another driver is a *bad* idea.
With that said, I have no experience in this field and no idea where to start.
Quoted 9 lines
Quoted 8 lines
Quoted 2 lines
Why do you want to retrieve the temperature value from the kernel? Please explain your use case.
I'm implementing power management for GPUs. These typically have several different performance constraints, but one of them is chip temperature. The maximum supported temperature is generally exported via tables in the graphics card BIOS, so it's necesssary for the kernel driver to be aware of the current temperature in order to limit the available performance modes to ensure the GPU stays within its thermal envelope.
Quoted 11 lines
OK, I see. Then indeed it makes sense to deviate from the traditional hwmon model. You could prevent auto-detection of the hwmon device (by dropping
I2C_CLASS_HWMONfrom i2c_adapter.class) and instantiate the lm99 device manually instead (usingi2c_new_device()). This gives you two things: a handle on the created device (so that you can access the chip registers directly if needed, and its private data too) and the possibility to pass platform data to the driver for specific initialization purposes. The lm90 driver doesn't implement the later yet, but we have another driver doing that (lm87) so it could be added if needed.
+1
Regards,
Hans
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 5 lines · Jean Delvare
There's nothing preventing you from accessing the LM99's registers directly and retrieve the temperature that way. Alternatively, we could add an internal interface to access some of the hwmon device features. It would take some time to define something everybody agrees on. If you have an interest in this, please make a proposal and we can discuss it.
Using the registers directly seems like duplication of existing code, so I'd prefer the access interface. I've fleshed out a rough interface that simply changes the hwmon device to its own struct containing the device structure, and then added an ops structure to that for getting and setting parameters. Accessor functions in the generic hwmon code then let other drivers use these. The advantage of this approach is that porting the drivers over to the new interface is trivial and there's no need to actually add the functions to specific drivers until they're needed. I'll tidy up the patch and send it.
Signature · 6 lines
Matthew Garrett | [email protected] _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Here's the patch I mentioned. Most of it is noise to update all the drivers, but the substantive portion is in the hunks modifying hwmon.c and hwmon.h. lm90.c has been updated to add the callbacks. This only scratches my own itch, so doesn't add voltage or fanspeed handling (or whatever) - adding those would be trivial if anyone needs them. Like I said, individual drivers can be updated piecemeal.
drivers/hwmon/abituguru.c +1 -1
@@ -176,7 +176,7 @@ MODULE_PARM_DESC(verbose, "How verbose should the driver be? (0-3):\n"The structure is dynamically allocated, at the same time when a new abituguru device is allocated. */ struct abituguru_data { -struct device *hwmon_dev;/* hwmon registered device */ +struct hwmon_device *hwmon_dev;/* hwmon registered device */ struct mutex update_lock;/* protect access to data and uGuru */ unsigned long last_updated;/* In jiffies */ unsigned short addr;/* uguru base address */
drivers/hwmon/abituguru3.c +1 -1
@@ -128,7 +128,7 @@ struct abituguru3_motherboard_info {The structure is dynamically allocated, at the same time when a new abituguru3 device is allocated. */ struct abituguru3_data { -struct device *hwmon_dev;/* hwmon registered device */ +struct hwmon_device *hwmon_dev;/* hwmon registered device */ struct mutex update_lock;/* protect access to data and uGuru */ unsigned short addr;/* uguru base address */ char valid;/* !=0 if following fields are valid */
drivers/hwmon/ad7414.c +1 -1
@@ -38,7 +38,7 @@static u8 AD7414_REG_LIMIT [] = {AD7414_REG_T_HIGH ,AD7414_REG_T_LOW };struct ad7414_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock;/* atomic read data updates */ char valid;/* !=0 if following fields are valid */ unsigned long next_update;/* In jiffies */
drivers/hwmon/ad7418.c +1 -1
@@ -43,7 +43,7 @@ static const u8 AD7418_REG_TEMP[] = { AD7418_REG_TEMP_IN, AD7418_REG_TEMP_OS };struct ad7418_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct attribute_group attrs;enum chips type;struct mutex lock;
drivers/hwmon/adcxx.c +1 -1
@@ -48,7 +48,7 @@#define DRVNAME "adcxx"struct adcxx { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock;u32 channels;u32 reference;/* in millivolts */
drivers/hwmon/adm1021.c +1 -1
@@ -78,7 +78,7 @@ clearing it. Weird, ey? --Phil *//* Each client has this additional data */ struct adm1021_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;enum chips type;struct mutex update_lock;
drivers/hwmon/adm1025.c +1 -1
@@ -145,7 +145,7 @@ static struct i2c_driver adm1025_driver = {*/ struct adm1025_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* zero until following fields are valid */ unsigned long last_updated;/* in jiffies */
drivers/hwmon/adm1026.c +1 -1
@@ -259,7 +259,7 @@ struct pwm_data { };struct adm1026_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;int valid;/* !=0 if following fields are valid */
drivers/hwmon/adm1029.c +1 -1
@@ -150,7 +150,7 @@ static struct i2c_driver adm1029_driver = {*/ struct adm1029_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* zero until following fields are valid */ unsigned long last_updated;/* in jiffies */
drivers/hwmon/adm1031.c +1 -1
@@ -70,7 +70,7 @@ typedef u8 auto_chan_table_t[8][2];/* Each client has this additional data */ struct adm1031_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;int chip_type;char valid;/* !=0 if following fields are valid */
drivers/hwmon/adm9240.c +1 -1
@@ -161,7 +161,7 @@ static struct i2c_driver adm9240_driver = {/* per client data */ struct adm9240_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;unsigned long last_updated_measure;
drivers/hwmon/ads7828.c +1 -1
@@ -64,7 +64,7 @@ static unsigned int ads7828_lsb_resol; /* resolution of the ADC sample lsb *//* Each client has this additional data */ struct ads7828_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;/* mutex protect updates */ char valid;/* !=0 if following fields are valid */ unsigned long last_updated;/* In jiffies */
drivers/hwmon/adt7470.c +1 -1
@@ -138,7 +138,7 @@ I2C_CLIENT_INSMOD_1(adt7470);#define FAN_DATA_VALID (x) ((x) && (x) != FAN_PERIOD_INVALID)struct adt7470_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct attribute_group attrs;struct mutex lock;char sensors_valid;
drivers/hwmon/adt7473.c +1 -1
@@ -130,7 +130,7 @@ I2C_CLIENT_INSMOD_1(adt7473);#define FAN_DATA_VALID (x) ((x) && (x) != FAN_PERIOD_INVALID)struct adt7473_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct attribute_group attrs;struct mutex lock;char sensors_valid;
drivers/hwmon/applesmc.c +1 -1
@@ -140,7 +140,7 @@ static const int debug;static struct platform_device *pdev;static s16 rest_x;static s16 rest_y; -static struct device *hwmon_dev; +static struct hwmon_device *hwmon_dev;static struct input_polled_dev *applesmc_idev;/* Indicates whether this computer has an accelerometer. */
drivers/hwmon/asb100.c +1 -1
@@ -176,7 +176,7 @@ static u8 DIV_TO_REG(long val)data is pointed to by client->data. The structure itself is dynamically allocated, at the same time the client itself is allocated. */ struct asb100_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock;struct mutex update_lock;
drivers/hwmon/atxp1.c +1 -1
@@ -72,7 +72,7 @@ static struct i2c_driver atxp1_driver = { };struct atxp1_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;unsigned long last_updated;u8 valid;
drivers/hwmon/coretemp.c +1 -1
@@ -48,7 +48,7 @@ typedef enum { SHOW_TEMP, SHOW_TJMAX, SHOW_TTARGET, SHOW_LABEL,static struct coretemp_data *coretemp_update_device (struct device *dev);struct coretemp_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;const char *name;u32 id;
drivers/hwmon/dme1737.c +1 -1
@@ -177,7 +177,7 @@ static const u8 DME1737_BIT_ALARM_FAN[] = {10, 11, 12, 13, 22, 23};struct dme1737_data {struct i2c_client *client;/* for I2C devices only */ -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;const char *name;unsigned int addr;/* for ISA devices only */
drivers/hwmon/ds1621.c +1 -1
@@ -72,7 +72,7 @@ static const u8 DS1621_REG_TEMP[3] = {/* Each client has this additional data */ struct ds1621_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* !=0 if following fields are valid */ unsigned long last_updated;/* In jiffies */
drivers/hwmon/f71805f.c +1 -1
@@ -166,7 +166,7 @@ struct f71805f_auto_point {struct f71805f_data {unsigned short addr;const char *name; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* !=0 if following fields are valid */
drivers/hwmon/f71882fg.c +1 -1
@@ -89,7 +89,7 @@ static inline void superio_exit(int base);struct f71882fg_data {unsigned short addr; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* !=0 if following fields are valid */
drivers/hwmon/f75375s.c +1 -1
@@ -87,7 +87,7 @@ I2C_CLIENT_INSMOD_2(f75373, f75375);struct f75375_data {unsigned short addr; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;const char *name;int kind;
drivers/hwmon/fscher.c +1 -1
@@ -143,7 +143,7 @@ static struct i2c_driver fscher_driver = {*/ struct fscher_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* zero until following fields are valid */ unsigned long last_updated;/* in jiffies */
drivers/hwmon/fschmd.c +1 -1
@@ -209,7 +209,7 @@ static struct i2c_driver fschmd_driver = {*/ struct fschmd_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;int kind;char valid;/* zero until following fields are valid */
drivers/hwmon/fscpos.c +1 -1
@@ -124,7 +124,7 @@ static struct i2c_driver fscpos_driver = {* Client data (each client gets its own) */ struct fscpos_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* 0 until following fields are valid */ unsigned long last_updated;/* In jiffies */
drivers/hwmon/gl518sm.c +1 -1
@@ -114,7 +114,7 @@ static inline u8 FAN_TO_REG(long rpm, int div)/* Each client has this additional data */ struct gl518_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;enum chips type;struct mutex update_lock;
drivers/hwmon/gl520sm.c +1 -1
@@ -110,7 +110,7 @@ static struct i2c_driver gl520_driver = {/* Client data */ struct gl520_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* zero until the following fields are valid */ unsigned long last_updated;/* in jiffies */
drivers/hwmon/hwmon.c +74 -3
118 lines not on this page · raw message
drivers/hwmon/i5k_amb.c +1 -1
@@ -105,7 +105,7 @@ struct i5k_device_attribute { };struct i5k_amb_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;unsigned long amb_base;unsigned long amb_len;
drivers/hwmon/ibmaem.c +1 -1
@@ -132,7 +132,7 @@ struct aem_rw_sensor_template {struct aem_data {struct list_head list; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct platform_device *pdev;struct mutex lock;char valid;
drivers/hwmon/ibmpex.c +1 -1
@@ -79,7 +79,7 @@ struct ibmpex_sensor_data {struct ibmpex_bmc_data {struct list_head list; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct device *bmc_device;struct mutex lock;char valid;
drivers/hwmon/it87.c +1 -1
@@ -245,7 +245,7 @@ struct it87_sio_data {/* For each registered chip, we need to keep some data in memory. The structure is dynamically allocated. */ struct it87_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;enum chips type;u8 revision;
drivers/hwmon/k8temp.c +1 -1
@@ -38,7 +38,7 @@#define SEL_CORE 0x04struct k8temp_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;const char *name;char valid;/* zero until following fields are valid */
drivers/hwmon/lm63.c +1 -1
@@ -165,7 +165,7 @@ static struct i2c_driver lm63_driver = {*/ struct lm63_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* zero until following fields are valid */ unsigned long last_updated;/* in jiffies */
drivers/hwmon/lm70.c +1 -1
@@ -38,7 +38,7 @@#define DRVNAME "lm70"struct lm70 { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock; };
drivers/hwmon/lm75.c +2 -2
@@ -72,7 +72,7 @@ static const u8 LM75_REG_TEMP[3] = {/* Each client has this additional data */ struct lm75_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;u8 orig_conf;char valid;/* !=0 if registers are valid */
@@ -190,7 +190,7 @@ lm75_probe(struct i2c_client *client, const struct i2c_device_id *id) }dev_info (&client->dev,"%s: sensor '%s'\n" , - data->hwmon_dev->bus_id, client->name); + data->hwmon_dev->dev->bus_id, client->name);return 0 ;
drivers/hwmon/lm77.c +1 -1
@@ -52,7 +52,7 @@ I2C_CLIENT_INSMOD_1(lm77);/* Each client has this additional data */ struct lm77_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;unsigned long last_updated;/* In jiffies */
drivers/hwmon/lm78.c +1 -1
@@ -129,7 +129,7 @@ static inline int TEMP_FROM_REG(s8 val)the driver field to differentiate between I2C and ISA chips. */ struct lm78_data {struct i2c_client client; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock;enum chips type;
drivers/hwmon/lm80.c +1 -1
@@ -108,7 +108,7 @@ static inline long TEMP_FROM_REG(u16 temp)*/ struct lm80_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* !=0 if following fields are valid */ unsigned long last_updated;/* In jiffies */
drivers/hwmon/lm83.c +1 -1
@@ -153,7 +153,7 @@ static struct i2c_driver lm83_driver = {*/ struct lm83_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* zero until following fields are valid */ unsigned long last_updated;/* in jiffies */
drivers/hwmon/lm85.c +1 -1
@@ -284,7 +284,7 @@ struct lm85_autofan {The structure is dynamically allocated. */ struct lm85_data {struct i2c_client client; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;enum chips type;struct mutex update_lock;
drivers/hwmon/lm87.c +1 -1
@@ -193,7 +193,7 @@ static struct i2c_driver lm87_driver = {*/ struct lm87_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* zero until following fields are valid */ unsigned long last_updated;/* In jiffies */
drivers/hwmon/lm90.c +80 -44
192 lines not on this page · raw message
drivers/hwmon/lm92.c +1 -1
@@ -96,7 +96,7 @@ static struct i2c_driver lm92_driver;/* Client data (each client gets its own) */ struct lm92_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* zero until following fields are valid */ unsigned long last_updated;/* in jiffies */
drivers/hwmon/lm93.c +1 -1
@@ -200,7 +200,7 @@ struct block1_t {* Client-specific data */ struct lm93_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;unsigned long last_updated;/* In jiffies */
drivers/hwmon/max1619.c +1 -1
@@ -114,7 +114,7 @@ static struct i2c_driver max1619_driver = {*/ struct max1619_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* zero until following fields are valid */ unsigned long last_updated;/* in jiffies */
drivers/hwmon/max6650.c +1 -1
@@ -140,7 +140,7 @@ static struct i2c_driver max6650_driver = {struct max6650_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* zero until following fields are valid */ unsigned long last_updated;/* in jiffies */
drivers/hwmon/pc87360.c +1 -1
@@ -184,7 +184,7 @@ static inline u8 PWM_TO_REG(int val, int inv)struct pc87360_data {const char *name; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock;struct mutex update_lock;char valid;/* !=0 if following fields are valid */
drivers/hwmon/pc87427.c +1 -1
@@ -46,7 +46,7 @@ static struct platform_device *pdev;device is using banked registers) and the register cache (needed to keep the data in the registers and the cache in sync at any time). */ struct pc87427_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock;int address[2 ];const char *name;
drivers/hwmon/sis5595.c +1 -1
@@ -163,7 +163,7 @@ static inline u8 DIV_TO_REG(int val)struct sis5595_data {unsigned short addr;const char *name; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock;struct mutex update_lock;
drivers/hwmon/smsc47b397.c +1 -1
@@ -98,7 +98,7 @@ static u8 smsc47b397_reg_temp[] = {0x25, 0x26, 0x27, 0x80};struct smsc47b397_data {unsigned short addr;const char *name; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock;struct mutex update_lock;
drivers/hwmon/smsc47m1.c +1 -1
@@ -120,7 +120,7 @@ struct smsc47m1_data {unsigned short addr;const char *name;enum chips type; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;unsigned long last_updated;/* In jiffies */
drivers/hwmon/smsc47m192.c +1 -1
@@ -96,7 +96,7 @@ static inline int TEMP_FROM_REG(s8 val) }struct smsc47m192_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* !=0 if following fields are valid */ unsigned long last_updated;/* In jiffies */
drivers/hwmon/thmc50.c +1 -1
@@ -63,7 +63,7 @@ static const u8 THMC50_REG_TEMP_DEFAULT[] = { 0x17, 0x18, 0x18 };/* Each client has this additional data */ struct thmc50_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;enum chips type;
drivers/hwmon/via686a.c +1 -1
@@ -294,7 +294,7 @@ static inline long TEMP_FROM_REG10(u16 val)struct via686a_data {unsigned short addr;const char *name; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* !=0 if following fields are valid */ unsigned long last_updated;/* In jiffies */
drivers/hwmon/vt1211.c +1 -1
@@ -112,7 +112,7 @@ static const u8 bitalarmfan[] = {6, 7};struct vt1211_data {unsigned short addr;const char *name; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* !=0 if following fields are valid */
drivers/hwmon/vt8231.c +1 -1
@@ -148,7 +148,7 @@ struct vt8231_data {const char *name;struct mutex update_lock; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;char valid;/* !=0 if following fields are valid */ unsigned long last_updated;/* In jiffies */
drivers/hwmon/w83627ehf.c +1 -1
@@ -260,7 +260,7 @@ struct w83627ehf_data {int addr;/* IO base of hw monitor block */ const char *name; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock;struct mutex update_lock;
drivers/hwmon/w83627hf.c +1 -1
@@ -348,7 +348,7 @@ static inline u8 DIV_TO_REG(long val)struct w83627hf_data {unsigned short addr;const char *name; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock;enum chips type;
drivers/hwmon/w83781d.c +1 -1
@@ -214,7 +214,7 @@ DIV_TO_REG(long val, enum chips type)the driver field to differentiate between I2C and ISA chips. */ struct w83781d_data {struct i2c_client client; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex lock;enum chips type;
drivers/hwmon/w83791d.c +1 -1
@@ -245,7 +245,7 @@ static u8 div_to_reg(int nr, long val) }struct w83791d_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* !=0 if following fields are valid */
drivers/hwmon/w83792d.c +1 -1
@@ -267,7 +267,7 @@ DIV_TO_REG(long val) }struct w83792d_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* !=0 if following fields are valid */
drivers/hwmon/w83793.c +1 -1
@@ -180,7 +180,7 @@ static inline s8 TEMP_TO_REG(long val, s8 min, s8 max)struct w83793_data {struct i2c_client *lm75[2 ]; -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* !=0 if following fields are valid */ unsigned long last_updated;/* In jiffies */
drivers/hwmon/w83l785ts.c +1 -1
@@ -116,7 +116,7 @@ static struct i2c_driver w83l785ts_driver = {*/ struct w83l785ts_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* zero until following fields are valid */ unsigned long last_updated;/* in jiffies */
drivers/hwmon/w83l786ng.c +1 -1
@@ -121,7 +121,7 @@ DIV_TO_REG(long val) }struct w83l786ng_data { -struct device *hwmon_dev; +struct hwmon_device *hwmon_dev;struct mutex update_lock;char valid;/* !=0 if following fields are valid */ unsigned long last_updated;/* In jiffies */
include/linux/hwmon.h +20 -2
@@ -16,9 +16,27 @@#include <linux/device.h> -struct device *hwmon_device_register (struct device *dev); +struct hwmon_device ; -void hwmon_device_unregister (struct device *dev); +struct hwmon_device_ops { +int (*get_temp) (struct hwmon_device *,int ,int *); +int (*set_temp) (struct hwmon_device *,int ,int ); +}; + +struct hwmon_device { +struct device *dev; +struct list_head node; +struct hwmon_device_ops ops; +}; + +struct hwmon_device *hwmon_device_register (struct device *dev); + +void hwmon_device_unregister (struct hwmon_device *dev); + +struct hwmon_device *hwmon_get_device (struct device *dev); + +int hwmon_get_temp (struct hwmon_device *,int ,int *); +int hwmon_set_temp (struct hwmon_device *,int ,int );/* Scale user input to sensible values */ static inline int SENSORS_LIMIT (long value,long low,long high)
Signature · 6 lines
Matthew Garrett | [email protected] _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 6 lines · Matthew Garrett
Here's the patch I mentioned. Most of it is noise to update all the drivers, but the substantive portion is in the hunks modifying
hwmon.candhwmon.h.lm90.chas been updated to add the callbacks. This only scratches my own itch, so doesn't add voltage or fanspeed handling (or whatever) - adding those would be trivial if anyone needs them. Like I said, individual drivers can be updated piecemeal.
I wonder if there is a way to do this that makes more use of the existing sysfs interface?
Maybe something like:
/* @dev: device with attribute
* @attr: attribute name
* @buf: buffer where result is placed
*
* Return value is the length of the result or an error code, i.e.
* the same as the attribute's show() method returns.
*/
int sysfs_show_attribute(struct device *dev, const char *attr, char *buf);
or maybe use a struct device_attribute * as an argument.
Then you could just call sysfs_show_attribute(hwmondev, "temp2_input", buf) or whatever and get the value in the kernel. It would would with all drivers without adding a code to them.
Of course there is the conversion to and from acsii. That's certainly ugly. It could be wrapped inside a function that just returns an int and deals with finding a buffer and converting from ascii, but the conversion would still be there. Compared to rate sensor values change, it's not very expensive.
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 3 lines · Trent Piepho
Then you could just call
sysfs_show_attribute(hwmondev, "temp2_input", buf)or whatever and get the value in the kernel. It would would with all drivers without adding a code to them.
Mnf. I'm not convinced general kernel upstream would be enthusiastic.
Signature · 6 lines
Matthew Garrett | [email protected] _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 7 lines · Matthew Garrett
Quoted 3 lines · Trent Piepho
Then you could just call
sysfs_show_attribute(hwmondev, "temp2_input", buf)or whatever and get the value in the kernel. It would would with all drivers without adding a code to them.Mnf. I'm not convinced general kernel upstream would be enthusiastic.
Any reason for that?
It would solve a lot of problems with trying to use sysfs interfaces from inside the kernel. It's not just hwmon where this is an issue, but other systems have this problem too. For instance the led subsystem only has a userspace sysfs interface.
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 4 lines · Trent Piepho
Quoted 1 line · Matthew Garrett
Mnf. I'm not convinced general kernel upstream would be enthusiastic.
Any reason for that?
Because any interface that basically enforces integer->string->integer conversion is an indication that you're already doing it wrong.
Signature · 6 lines
Matthew Garrett | [email protected] _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 8 lines · Matthew Garrett
Quoted 4 lines · Trent Piepho
Quoted 1 line · Matthew Garrett
Mnf. I'm not convinced general kernel upstream would be enthusiastic.
Any reason for that?
Because any interface that basically enforces
integer->string->integerconversion is an indication that you're already doing it wrong.
Many if not most users of the sysfs interface from userspace are doing int->string->int. Yeah, 'cat' and 'echo' aren't, but most apps more sophisticated than that do. So it's no more wrong than sysfs already is.
For what it used for, the sysfs int->string->int cost isn't very significant. It's less than the kernel to userspace cost of sysfs.
If the overhead of the sysfs interface to userspace is not enough to be a problem, and a kernel interface to sysfs has less overhead than from userspace, then how can the kernel based overhead be too much?
Though maybe sysfs or hwmon could provide int<->string wrappers. Virtually every hwmon store does a sprintf and virtually every show does a strtol. It could quite possibly be more efficient to move all that code to common set/show wrappers.
Then the hwmon driver would provide set/show methods use/produce ints instead of strings. The wrappers would take care of converting those to/from strings.
From this:
static SENSOR_DEVICE_ATTR(foo, S_IWUSR|S_IRUGO, show_foo, set_foo, 1);
static ssize_t show_foo(..., char *buf)
{ return sprintf(buf, "%d\n", value); }
static ssize_t set_foo(..., const char *buf, size_t count)
{ unsigned long val = simple_strtol(buf, NULL, 10); }
To this:
static SENSOR_DEVICE_ATTR_INT(foo, S_IWUSR | S_IRUGO, show_foo, set_foo, 1);
static ssize_t show_foo(..., signed long *output)
{ *output = value; return 0; }
static ssize_t set_foo(..., signed long val)
{ ... }
The actual sysfs store and set would be a wrapper that calls the methods registered with SENSOR_DEVICE_ATTR_INT and does the int<->string stuff.
hwmon could then provide this function, which would call show_foo() directly.
int hwmon_show_attribute_int(const struct device *dev, const char *attr, long *out)
Instead of hwmon providing this stuff, sysfs could always do it. There are a lot of non-hwmon attributes that could make use of it.
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 7 lines · Trent Piepho
Quoted 2 lines · Matthew Garrett
Because any interface that basically enforces
integer->string->integerconversion is an indication that you're already doing it wrong.Many if not most users of the sysfs interface from userspace are doing
int->string->int. Yeah, 'cat' and 'echo' aren't, but most apps more sophisticated than that do. So it's no more wrong than sysfs already is.
That's fine. It's an unavoidable result of one of the sysfs design goals being for as much as possible to be human readable. That doesn't hold when you're looking at kernel subsystems that are perfectly capable of exporting their functionality to the rest of the kernel.
Quoted 2 lines
Instead of hwmon providing this stuff, sysfs could always do it. There are a lot of non-hwmon attributes that could make use of it.
If you want to press for that then please feel free to do so, but it's not an argument I'm interested in having with upstream.
Signature · 6 lines
Matthew Garrett | [email protected] _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Hi Trent, Matthew,
Quoted 9 lines · Trent Piepho
Quoted 8 lines · Matthew Garrett
Quoted 4 lines · Trent Piepho
Quoted 1 line · Matthew Garrett
Mnf. I'm not convinced general kernel upstream would be enthusiastic.
Any reason for that?
Because any interface that basically enforces
integer->string->integerconversion is an indication that you're already doing it wrong.
I wholeheartedly second Matthew here. Making the string-based sysfs interface available to the kernel would certainly be easy, but that would be damn inefficient. If we have to do that then let's do it right.
Quoted 10 lines
Many if not most users of the sysfs interface from userspace are doing
int->string->int. Yeah, 'cat' and 'echo' aren't, but most apps more sophisticated than that do. So it's no more wrong than sysfs already is.For what it used for, the sysfs
int->string->intcost isn't very significant. It's less than the kernel to userspace cost of sysfs.If the overhead of the sysfs interface to userspace is not enough to be a problem, and a kernel interface to sysfs has less overhead than from userspace, then how can the kernel based overhead be too much?
You wrote it yourself: as far as user-space is concerned, the int->string->int cost is less than the kernel-to-userspace cost. That's the very reason why the sysfs interface concept was deemed acceptable. For kernel-space interfaces though, there is no huge kernel-to-kernel cost which means that the overall cost _can_ be low. And if it can be low, is should be, which means: no useless int->string->int conversion.
Quoted 2 lines
Though maybe sysfs or hwmon could provide int<->string wrappers. Virtually every hwmon store does a sprintf and virtually every show does a strtol. It
Actually, the other way around.
Quoted 26 lines
could quite possibly be more efficient to move all that code to common set/show wrappers.
Then the hwmon driver would provide set/show methods use/produce ints instead of strings. The wrappers would take care of converting those to/from strings.
From this:
static
SENSOR_DEVICE_ATTR(foo, S_IWUSR|S_IRUGO, show_foo, set_foo, 1);static
ssize_tshow_foo(..., char *buf)
{ returnsprintf(buf, "%d\n", value); }static
ssize_tset_foo(..., const char *buf, size_t count)
{ unsigned long val =simple_strtol(buf, NULL, 10); }To this:
static
SENSOR_DEVICE_ATTR_INT(foo, S_IWUSR | S_IRUGO, show_foo, set_foo, 1);static
ssize_tshow_foo(..., signed long *output)
{ *output = value; return 0; }static
ssize_tset_foo(..., signed long val)
{ ... }
Quoted 6 lines
The actual sysfs store and set would be a wrapper that calls the methods registered with
SENSOR_DEVICE_ATTR_INTand does the int<->string stuff.hwmon could then provide this function, which would call
show_foo()directly.int
hwmon_show_attribute_int(const struct device *dev, const char *attr, long *out)
This is indeed the way things should be implemented if we want to generalize kernel access to monitored values.
Note however that, as far as hwmon drivers are concerned, there's more than just the access interface to consider. Almost all hwmon drivers read all registers at once and cache the results for one or two seconds, because this made sense for the user-space users. An in-kernel user such as Matthew's driver would instead need to read only _one_ sensor value, and would probably like to avoid caching it, so that his driver can react faster to temperature changes. So, drivers which are needed for in-kernel access will need to be reworked a bit anyway, we can't just point to the sysfs callback functions, not even if we manage to get them to return an integer rather than its string form.
Quoted 2 lines
Instead of hwmon providing this stuff, sysfs could always do it. There are a lot of non-hwmon attributes that could make use of it.
If sysfs ever offers a facility for this then yes, the hwmon subsystem should be converted to make use of it.
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 6 lines · Jean Delvare
Quoted 2 lines
Quoted 2 lines
Because any interface that basically enforces
integer->string->integerconversion is an indication that you're already doing it wrong.I wholeheartedly second Matthew here. Making the string-based sysfs interface available to the kernel would certainly be easy, but that would be damn inefficient. If we have to do that then let's do it right.
Does it really make that much of a difference? My guess would be that the most significant cost of using the sysfs interface from the kernel would be the attribute lookup and the locking sysfs does. The int/string stuff shouldn't be much compared to that.
Quoted 7 lines
Quoted 6 lines
For what it used for, the sysfs
int->string->intcost isn't very significant. It's less than the kernel to userspace cost of sysfs.If the overhead of the sysfs interface to userspace is not enough to be a problem, and a kernel interface to sysfs has less overhead than from userspace, then how can the kernel based overhead be too much?
Quoted 6 lines
You wrote it yourself: as far as user-space is concerned, the
int->string->intcost is less than the kernel-to-userspace cost. That's the very reason why the sysfs interface concept was deemed acceptable. For kernel-space interfaces though, there is no huge kernel-to-kernel cost which means that the overall cost _can_ be low. And if it can be low, is should be, which means: no uselessint->string->intconversion.
The sysfs concept isn't acceptable for everything, only low volume interfaces where the overhead isn't significant. You can make a faster kernel interface, but you can make a faster userspace interface too. But some things don't need it.
If userspace sysfs is fast enough for a hwmon chip, why would a faster kernel sysfs interface not be fast enough? The rate at which temperatures change, hwmon chips update their registers, and the speed of I2C haven't changed.
Quoted 10 lines
Note however that, as far as hwmon drivers are concerned, there's more than just the access interface to consider. Almost all hwmon drivers read all registers at once and cache the results for one or two seconds, because this made sense for the user-space users. An in-kernel user such as Matthew's driver would instead need to read only _one_ sensor value, and would probably like to avoid caching it, so that his driver can react faster to temperature changes. So, drivers which are needed for in-kernel access will need to be reworked a bit anyway, we can't just point to the sysfs callback functions, not even if we manage to get them to return an integer rather than its string form.
That's a good point. I've never really liked the one second rate limiting. It would be nice if the cache time could be adjusted at run time, or if the user the request was made under, root/kernel vs non-root, could be taken into account.
Quoted 5 lines
Quoted 2 lines
Instead of hwmon providing this stuff, sysfs could always do it. There are a lot of non-hwmon attributes that could make use of it.
If sysfs ever offers a facility for this then yes, the hwmon subsystem should be converted to make use of it.
It already does offer something very much like this, see SYSDEV_ULONG_ATTR and SYSDEV_INT_ATTR in sysdev.h and the various sysdev functions like sysdev_show_ulong().
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Quoted 6 lines · Jean Delvare
Quoted 5 lines · Trent Piepho
Quoted 4 lines · Matthew Garrett
Quoted 3 lines · Trent Piepho
Quoted 1 line · Matthew Garrett
Mnf. I'm not convinced general kernel upstream would be enthusiastic.
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Any reason for that?
Because any interface that basically enforces
integer->string->integerconversion is an indication that you're already doing it wrong.I wholeheartedly second Matthew here. Making the string-based sysfs interface available to the kernel would certainly be easy, but that would be damn inefficient. If we have to do that then let's do it right.
I third it, and it is extremely bad taste, so I would hate to see the Linus special on that one if he noticed it before/while pulling.
ACPI almost introduced some integer->string->integer APIs like this, and it was changed _really_ fast...
Quoted 5 lines
Quoted 2 lines
Instead of hwmon providing this stuff, sysfs could always do it. There are a lot of non-hwmon attributes that could make use of it.
If sysfs ever offers a facility for this then yes, the hwmon subsystem should be converted to make use of it.
Agreed.
Signature · 9 lines
"One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors