RE: [PATCH 13/13] Thermal: Platform layer changes to provide thermal data

From: "R, Durgadoss" <durgadoss.r@intel.com>
To: "Zhang, Rui" <rui.zhang@intel.com>
Cc: "eduardo.valentin@ti.com" <eduardo.valentin@ti.com>,
	"lenb@kernel.org" <lenb@kernel.org>, "rjw@sisk.pl" <rjw@sisk.pl>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"amit.kachhap@linaro.org" <amit.kachhap@linaro.org>,
	"wni@nvidia.com" <wni@nvidia.com>
Subject: RE: [PATCH 13/13] Thermal: Platform layer changes to provide thermal data
Date: Tue, 21 Aug 2012 09:28:59 +0000	[thread overview]
Message-ID: <4D68720C2E767A4AA6A8796D42C8EB591AAB2A@BGSMSX101.gar.corp.intel.com> (raw)
In-Reply-To: <1345531954.1682.965.camel@rui.sh.intel.com>

Hi Rui,

> 
> On 二, 2012-08-21 at 00:41 -0600, R, Durgadoss wrote:
> > Hi Rui,
> >
> > > > > -----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:
> > > > > > 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 <durgadoss.r@intel.com>
> > > > > > ---
> > > > > >  arch/x86/platform/mrst/mrst.c |   42
> > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 42 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/x86/platform/mrst/mrst.c
> > > > > b/arch/x86/platform/mrst/mrst.c
> > > > > > 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];
> > > > > >  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;
> }

Yes, this can work with little bit change like below:

struct thermal_zone_params {
	const char *zone_name;
	int num_cdevs;
	struct thermal_bind_params[num_cdevs];
}; 

Where struct thermal_bind_params will be like this:
	{
	.cdev_name = "CPU"
	.trip_mask = 0x0F
	.weight = 70
	.lower = 2
	.upper = 4
	.sample_period = 1000 (1 ms)
	};

Let me know what you think.

Thanks,
Durga