Re: [PATCH 1/2] thermal: add hwmon sys I/F for thermal device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Zhang, Rui wrote:

Add hwmon sys I/F for the generic thermal device.



Great!

But I have several remarks:
1) Looking at the new code, you only add temp1_input, so I'm guessing that you
are registering a seperate hwmon class entry per zone? Please don't do that, please register one hwmon class entry, and add multiple temp#_input attr to it (and the same for crit). 

2) I see that temp_crit may not be always available:
> +static ssize_t
> +crit_trip_temp_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct device *device = dev->parent;
> +	struct thermal_zone_device *tz = to_thermal_zone(device);
> +	int result;
> +
> +	if (!tz->ops->get_trip_temp )
> +		return -EPERM;
> +
> +	/* assume trip 0 to be the critical trip point by default */
> +	if (tz->ops->get_trip_type) {
> +		result = tz->ops->get_trip_type(tz, 0, buf);
> +		if (result < 0)
> +			return result;
> +		if (strcmp(buf, "critical\n"))
> +			return -ENODEV;
> +	}
> +
> +	return tz->ops->get_trip_temp(tz, 0, buf);
> +}
But you do always register a temp#_crit sysfs attr, it would be much much better to only add this if reading it actually has a chance of returning a value, so if tz->ops->get_trip_temp != NULL and tz->ops->get_trip_type(tz, 0, buf) returns "critical" in buf. 

Thanks & Regards,

Hans




Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
---
 Documentation/thermal/sysfs-api.txt |   22 +++++----
drivers/thermal/Kconfig | 1 drivers/thermal/thermal.c | 86 ++++++++++++++++++++++++++++++++---- include/linux/thermal.h | 1 4 files changed, 92 insertions(+), 18 deletions(-) 

Index: linux-2.6.25-rc2/Documentation/thermal/sysfs-api.txt
===================================================================
--- linux-2.6.25-rc2.orig/Documentation/thermal/sysfs-api.txt	2008-02-25 22:43:29.000000000 -0500
+++ linux-2.6.25-rc2/Documentation/thermal/sysfs-api.txt	2008-02-25 22:43:38.000000000 -0500
@@ -141,12 +141,14 @@
type Strings which represent the thermal zone type. 
 				This is given by thermal zone driver as part of registration.
+				In order to keep the compatibility with hwmon,
+				it should not contain any spaces or dashes.
 				Eg: "ACPI thermal zone" indicates it's a ACPI thermal device
 				RO
-				Optional
+				Required
temp Current temperature as reported by thermal zone (sensor) 
-				Unit: degree Celsius
+				Unit: millidegree Celsius
 				RO
 				Required
@@ -163,7 +165,7 @@ 
 					  charge of the thermal management.
trip_point_[0-*]_temp The temperature above which trip point will be fired 
-				Unit: degree Celsius
+				Unit: millidegree Celsius
 				RO
 				Optional
@@ -219,16 +221,16 @@ |thermal_zone1: 
 	|-----type:			ACPI thermal zone
-	|-----temp:			37
+	|-----temp:			37000
 	|-----mode:			kernel
-	|-----trip_point_0_temp:	100
+	|-----trip_point_0_temp:	100000
 	|-----trip_point_0_type:	critical
-	|-----trip_point_1_temp:	80
+	|-----trip_point_1_temp:	80000
 	|-----trip_point_1_type:	passive
-	|-----trip_point_2_temp:	70
-	|-----trip_point_2_type:	active[0]
-	|-----trip_point_3_temp:	60
-	|-----trip_point_3_type:	active[1]
+	|-----trip_point_2_temp:	70000
+	|-----trip_point_2_type:	active0
+	|-----trip_point_3_temp:	60000
+	|-----trip_point_3_type:	active1
 	|-----cdev0:			--->/sys/class/thermal/cooling_device0
 	|-----cdev0_trip_point:		1	/* cdev0 can be used for passive */
 	|-----cdev1:			--->/sys/class/thermal/cooling_device3
Index: linux-2.6.25-rc2/drivers/thermal/Kconfig
===================================================================
--- linux-2.6.25-rc2.orig/drivers/thermal/Kconfig	2008-02-25 22:43:29.000000000 -0500
+++ linux-2.6.25-rc2/drivers/thermal/Kconfig	2008-02-25 22:43:38.000000000 -0500
@@ -4,6 +4,7 @@
menuconfig THERMAL 
 	bool "Generic Thermal sysfs driver"
+	select HWMON
 	default y
 	help
 	  Generic Thermal Sysfs driver offers a generic mechanism for
Index: linux-2.6.25-rc2/drivers/thermal/thermal.c
===================================================================
--- linux-2.6.25-rc2.orig/drivers/thermal/thermal.c	2008-02-25 22:43:29.000000000 -0500
+++ linux-2.6.25-rc2/drivers/thermal/thermal.c	2008-02-26 00:37:57.000000000 -0500
@@ -30,8 +30,9 @@
 #include <linux/idr.h>
 #include <linux/thermal.h>
 #include <linux/spinlock.h>
+#include <linux/hwmon.h>
-MODULE_AUTHOR("Zhang Rui") 
+MODULE_AUTHOR("Zhang Rui");
 MODULE_DESCRIPTION("Generic thermal management sysfs support");
 MODULE_LICENSE("GPL");
@@ -171,6 +172,47 @@ 
 	return tz->ops->get_trip_temp(tz, trip, buf);
 }
+static ssize_t 
+hwmon_type_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct device *device = dev->parent;
+	return type_show(device, attr, buf);
+}
+
+static ssize_t
+hwmon_temp_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct device *device = dev->parent;
+	return temp_show(device, attr, buf);
+}
+
+static ssize_t
+crit_trip_temp_show(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct device *device = dev->parent;
+	struct thermal_zone_device *tz = to_thermal_zone(device);
+	int result;
+
+	if (!tz->ops->get_trip_temp )
+		return -EPERM;
+
+	/* assume trip 0 to be the critical trip point by default */
+	if (tz->ops->get_trip_type) {
+		result = tz->ops->get_trip_type(tz, 0, buf);
+		if (result < 0)
+			return result;
+		if (strcmp(buf, "critical\n"))
+			return -ENODEV;
+	}
+
+	return tz->ops->get_trip_temp(tz, 0, buf);
+}
+
+static DEVICE_ATTR(name, 0444, hwmon_type_show, NULL);
+static DEVICE_ATTR(temp1_input, 0444, hwmon_temp_show, NULL);
+static DEVICE_ATTR(temp1_crit, 0444, crit_trip_temp_show, NULL);
+
 static DEVICE_ATTR(type, 0444, type_show, NULL);
 static DEVICE_ATTR(temp, 0444, temp_show, NULL);
 static DEVICE_ATTR(mode, 0644, mode_show, mode_store);
@@ -569,6 +611,9 @@
 	int result;
 	int count;
+ if (!type) 
+		return ERR_PTR(-EINVAL);
+
 	if (strlen(type) >= THERMAL_NAME_LENGTH)
 		return NULL;
@@ -604,13 +649,33 @@ 
 		return NULL;
 	}
- /* sys I/F */ 
-	if (type) {
-		result = device_create_file(&tz->device, &dev_attr_type);
-		if (result)
-			goto unregister;
+	/* hwmon sys I/F */
+	tz->hwmon = hwmon_device_register(&tz->device);
+	if (IS_ERR(tz->hwmon)) {
+		result = PTR_ERR(tz->hwmon);
+		tz->hwmon = NULL;
+		printk(KERN_ERR PREFIX
+			"unable to register hwmon device\n");
+		goto unregister;
 	}
+ result = device_create_file(tz->hwmon, &dev_attr_name); 
+	if (result)
+		goto unregister;
+
+	result = device_create_file(tz->hwmon, &dev_attr_temp1_input);
+	if (result)
+		goto unregister;
+
+	result = device_create_file(tz->hwmon, &dev_attr_temp1_crit);
+	if (result)
+		goto unregister;
+
+	/* sys I/F */
+	result = device_create_file(&tz->device, &dev_attr_type);
+	if (result)
+		goto unregister;
+
 	result = device_create_file(&tz->device, &dev_attr_temp);
 	if (result)
 		goto unregister;
@@ -676,8 +741,13 @@
 		    tz->ops->unbind(tz, cdev);
 	mutex_unlock(&thermal_list_lock);
- if (tz->type[0]) 
-		device_remove_file(&tz->device, &dev_attr_type);
+	device_remove_file(&tz->device, &dev_attr_name);
+	device_remove_file(&tz->device, &dev_attr_temp1_input);
+	device_remove_file(&tz->device, &dev_attr_temp1_crit);
+	hwmon_device_unregister(tz->hwmon);
+	tz->hwmon = NULL;
+
+	device_remove_file(&tz->device, &dev_attr_type);
 	device_remove_file(&tz->device, &dev_attr_temp);
 	if (tz->ops->get_mode)
 		device_remove_file(&tz->device, &dev_attr_mode);
Index: linux-2.6.25-rc2/include/linux/thermal.h
===================================================================
--- linux-2.6.25-rc2.orig/include/linux/thermal.h	2008-02-25 22:43:29.000000000 -0500
+++ linux-2.6.25-rc2/include/linux/thermal.h	2008-02-25 22:43:38.000000000 -0500
@@ -69,6 +69,7 @@
 	int id;
 	char type[THERMAL_NAME_LENGTH];
 	struct device device;
+	struct device *hwmon;
 	void *devdata;
 	int trips;
 	struct thermal_zone_device_ops *ops;





-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux