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

[Eduardo Valentin <eduardo.valentin@ti.com>]

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

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