On 四, 2012-07-19 at 22:27 +0200, Rafael J. Wysocki wrote:

On Thursday, July 19, 2012, Zhang Rui wrote:

quoted

From: Durgadoss R <redacted>

Some of the thermal drivers using the Generic Thermal Framework
require (all/some) trip points to be writeable. This patch makes
the trip point temperatures writeable on a per-trip point basis,
and modifies the required function call in thermal.c. This patch
also updates the Documentation to reflect the new change.

Signed-off-by: Durgadoss R <redacted>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 Documentation/thermal/sysfs-api.txt      |    4 +-
 drivers/acpi/thermal.c                   |    4 +-
 drivers/platform/x86/acerhdf.c           |    2 +-
 drivers/platform/x86/intel_mid_thermal.c |    2 +-
 drivers/thermal/spear_thermal.c          |    2 +-
 drivers/thermal/thermal_sys.c            |  147 +++++++++++++++++++++---------
 include/linux/thermal.h                  |   15 ++-
 7 files changed, 125 insertions(+), 51 deletions(-)

diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
index 1733ab9..0c7c423 100644
--- a/Documentation/thermal/sysfs-api.txt
+++ b/Documentation/thermal/sysfs-api.txt

@@ -32,7 +32,8 @@ temperature) and throttle appropriate devices.
 
 1.1 thermal zone device interface
 1.1.1 struct thermal_zone_device *thermal_zone_device_register(char *name,
-		int trips, void *devdata, struct thermal_zone_device_ops *ops)
+		int trips, int flag, void *devdata,

The 'flags' argument should rather be called 'mask', or even 'writable_mask'.

Agreed.

quoted

 #define to_cooling_device(_dev)	\
 	container_of(_dev, struct thermal_cooling_device, device)

@@ -1089,9 +1084,83 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
 EXPORT_SYMBOL(thermal_zone_device_update);
 >  /**
+ * create_trip_attrs - create attributes for trip points
+ * @tz:		the thermal zone device
+ * @flag:	Writeable trip point bitmap.
+ */
+static int create_trip_attrs(struct thermal_zone_device *tz, int flag)
+{
+	int indx;
+	int writeable;
+
+	tz->trip_type_attrs =
+		kzalloc(sizeof(struct thermal_attr) * tz->trips, GFP_KERNEL);
+	if (!tz->trip_type_attrs)
+		return -ENOMEM;
+
+	tz->trip_temp_attrs =
+		kzalloc(sizeof(struct thermal_attr) * tz->trips, GFP_KERNEL);
+	if (!tz->trip_temp_attrs) {
+		kfree(tz->trip_type_attrs);
+		return -ENOMEM;
+	}
+
+	for (indx = 0; indx < tz->trips; indx++) {
+
+		/* create trip type attribute */
+		snprintf(tz->trip_type_attrs[indx].name, THERMAL_NAME_LENGTH,
+			 "trip_point_%d_type", indx);

If this really truncates the name, we'll run into problems in _show() and _store().
Is there any solution to that?

as THERMAL_MAX_TRIP is smaller than 100, so the answer is no.

quoted

+
+		sysfs_attr_init(&tz->trip_type_attrs[count].attr.attr);
+		tz->trip_type_attrs[indx].attr.attr.name =
+						tz->trip_type_attrs[indx].name;
+		tz->trip_type_attrs[indx].attr.attr.mode = S_IRUGO;
+		tz->trip_type_attrs[indx].attr.show = trip_point_type_show;
+
+		device_create_file(&tz->device,
+				   &tz->trip_type_attrs[indx].attr);
+
+		/* create trip temp attribute */
+		writeable = flag & (1 << indx);
+		snprintf(tz->trip_temp_attrs[indx].name, THERMAL_NAME_LENGTH,
+			 "trip_point_%d_temp", indx);
+
+		sysfs_attr_init(&tz->trip_temp_attrs[indx].attr.attr);
+		tz->trip_temp_attrs[indx].attr.attr.name =
+						tz->trip_temp_attrs[indx].name;
+		tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
+		tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
+		if (writeable) {

Why don't you put the "flag & (1 << indx)" here directly instead of 'writable'?

agreed.

quoted

+			tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
+			tz->trip_temp_attrs[indx].attr.store =
+							trip_point_temp_store;
+		}
+
+		device_create_file(&tz->device,
+				   &tz->trip_temp_attrs[indx].attr);
+	}
+	return 0;
+}
+
+static void remove_trip_attrs(struct thermal_zone_device *tz)
+{
+	int indx;
+
+	for (indx = 0; indx < tz->trips; indx++) {
+		device_remove_file(&tz->device,
+				   &tz->trip_type_attrs[indx].attr);
+		device_remove_file(&tz->device,
+				   &tz->trip_temp_attrs[indx].attr);
+	}
+	kfree(tz->trip_type_attrs);
+	kfree(tz->trip_temp_attrs);
+}
+
+/**
  * thermal_zone_device_register - register a new thermal zone device
  * @type:	the thermal zone device type
  * @trips:	the number of trip points the thermal zone support
+ * @flag:	a bit string indicating the writeablility of trip points
  * @devdata:	private device data
  * @ops:	standard thermal zone device callbacks
  * @tc1:	thermal coefficient 1 for passive calculations

@@ -1107,7 +1176,7 @@ EXPORT_SYMBOL(thermal_zone_device_update);
  * section 11.1.5.1 of the ACPI specification 3.0.
  */
 struct thermal_zone_device *thermal_zone_device_register(char *type,
-	int trips, void *devdata,
+	int trips, int flag, void *devdata,
 	const struct thermal_zone_device_ops *ops,
 	int tc1, int tc2, int passive_delay, int polling_delay)
 {

@@ -1124,6 +1193,9 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
 	if (trips > THERMAL_MAX_TRIPS || trips < 0)
 		return ERR_PTR(-EINVAL);
 
+	if (flag >> trips)
+		return ERR_PTR(-EINVAL);

I'm not sure if this check is necessary.  Does it actually matter is someone
passes ones in the unused bit positions?

I do not think we can trust this value if the caller even wants to set
writable mask for a non-exist trip point.

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html