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 <[EMAIL PROTECTED]>
---
 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 [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to