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 at intel.com>
> ---
> 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;
>
>
>
