Hello,

On Tue, Aug 21, 2012 at 02:52:34PM +0800, Zhang Rui wrote:

On 二, 2012-08-21 at 00:41 -0600, R, Durgadoss wrote:

quoted

Hi Rui,

quoted

quoted

quoted

-----Original Message-----
From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
owner@vger.kernel.org] On Behalf Of Eduardo Valentin
Sent: Tuesday, August 21, 2012 11:10 AM
To: R, Durgadoss
Cc: lenb@kernel.org; Zhang, Rui; rjw@sisk.pl; linux-acpi@vger.kernel.org;
linux-pm@vger.kernel.org; eduardo.valentin@ti.com;
amit.kachhap@linaro.org; wni@nvidia.com
Subject: Re: [PATCH 13/13] Thermal: Platform layer changes to provide
thermal data

Hello,

On Thu, Aug 09, 2012 at 06:16:05PM +0530, Durgadoss R wrote:

quoted

This patch shows how can we add platform specific thermal data
required by the thermal framework. This is just an example
patch, and _not_ for merge.

Signed-off-by: Durgadoss R <redacted>
---
 arch/x86/platform/mrst/mrst.c |   42

+++++++++++++++++++++++++++++++++++++++++

quoted

 1 file changed, 42 insertions(+)

diff --git a/arch/x86/platform/mrst/mrst.c

b/arch/x86/platform/mrst/mrst.c

quoted

index fd41a92..0440db5 100644

--- a/arch/x86/platform/mrst/mrst.c
+++ b/arch/x86/platform/mrst/mrst.c

@@ -30,6 +30,7 @@
 #include <linux/mfd/intel_msic.h>
 #include <linux/gpio.h>
 #include <linux/i2c/tc35876x.h>
+#include <linux/thermal.h>

 #include <asm/setup.h>
 #include <asm/mpspec_def.h>

@@ -78,6 +79,30 @@ struct sfi_rtc_table_entry

sfi_mrtc_array[SFI_MRTC_MAX];

quoted

 EXPORT_SYMBOL_GPL(sfi_mrtc_array);
 int sfi_mrtc_num;

+#define MRST_THERMAL_ZONES	3
+struct thermal_zone_params tzp[MRST_THERMAL_ZONES] = {
+	{ .thermal_zone_name = "CPU",
+	.throttle_policy = THERMAL_FAIR_SHARE,
+	.num_cdevs = 2,
+	.cdevs_name = {"CPU", "Battery"},
+	.trip_mask = {0x0F, 0x08},
+	.weights = {80, 20}, },
+
+	{ .thermal_zone_name = "Battery",
+	.throttle_policy = THERMAL_FAIR_SHARE,
+	.num_cdevs = 1,
+	.cdevs_name = {"Battery"},
+	.trip_mask = {0x0F},
+	.weights = {100}, },
+
+	{ .thermal_zone_name = "Skin",
+	.throttle_policy = THERMAL_FAIR_SHARE,
+	.num_cdevs = 2,
+	.cdevs_name = {"Display", "Battery"},
+	.trip_mask = {0x0F, 0x0F},
+	.weights = {50, 50}, }

Please consider the comment I sent on your data definition and also the
comment I made on this patch on your RFC series.

Yes.. I don't know why/how I missed it.
Also, saw the same comment on one of the other patches also.

Will surely fix this thing in v2.

BTW, any suggestion for the 'name' of that structure ? :-)

hmmm,
do we still have thermal_zone_platforms in patch v2?
I do not think we need this if we only bind devices via .bind()
callback.

We can bind devices via .bind call back, and that will take some load
off the framework code. But even then, we would need this structure
right ?

why?
I'd prefer introduce something like this,
struct thermal_bind_params {
	int trip;
	unsigned long upper;
	unsinged long lower;
	int weight;
	int sample_period;
}

and use thermal_zone_bind_cooling_device(tz, cdev, thermal_bind_params),
throttle_policy should be set when invoking
thermal_zone_device_register.

is there any information in thermal_zone_params can not be convert to
thermal_bind_params?

IMO, we need to think here carefully. Ideally, we should have a set of data
describing the thermal bindings. This way the code would look simpler and cleaner.
If we define a good way to describe the thermal bindings, I don't see why
we would need much complexity in the platform driver.

Assuming a good data structure design, the task of a platform driver would then be
to fetch the thermal info, either from bootloader, parameters, DT, etc, then
translate that into our binding descriptors, and pushing that data set forward
to the FW.

It think the above approach is much cleaner than writing for every platform
driver a set of function calls with static definitions telling what cooling
to do for each thermal zone.

What do you think?

thanks,
rui

quoted

 Say, when we obtain platform data from a thermal driver, it
should know 'what format the platform data is' ..correct ?

I theoretically agree with you that individual platform drivers can
have data in their own format, but that will be a heavy loss on
standardization.

So,
I will remove the extra bind code I added to framework, and
(keep it the old way it was) but still prefer to have the structure
put in thermal.h.

Thanks,
Durga

--
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