On 二, 2012-08-21 at 11:51 +0300, Eduardo Valentin wrote: > 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@xxxxxxxxxxxxxxx [mailto:linux-acpi- > > > > > > owner@xxxxxxxxxxxxxxx] On Behalf Of Eduardo Valentin > > > > > > Sent: Tuesday, August 21, 2012 11:10 AM > > > > > > To: R, Durgadoss > > > > > > Cc: lenb@xxxxxxxxxx; Zhang, Rui; rjw@xxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; > > > > > > linux-pm@xxxxxxxxxxxxxxx; eduardo.valentin@xxxxxx; > > > > > > amit.kachhap@xxxxxxxxxx; wni@xxxxxxxxxx > > > > > > 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@xxxxxxxxx> > > > > > > > --- > > > > > > > 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 agreed. > than writing for every platform > driver a set of function calls with static definitions telling what cooling > to do for each thermal zone. > please define "a set of function calls". thanks, rui > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
