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

[Zhang Rui <rui.zhang@intel.com>]

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

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?

thanks,
rui

>  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