| Message ID | 1344516365-7230-14-git-send-email-durgadoss.r@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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. > +}; > + > static void mrst_power_off(void) > { > } > @@ -983,10 +1008,27 @@ static int __init sfi_parse_devs(struct sfi_table_header *table) > return 0; > } > > +static int mrst_get_thermal_params(struct thermal_zone_device *tz) > +{ > + int i; > + > + for (i = 0; i < MRST_THERMAL_ZONES; i++) { > + if (!strcmp(tzp[i].thermal_zone_name, tz->type)) { > + tz->tzp = &tzp[i]; > + return 0; > + } > + } > + return -ENODEV; > +} > + > static int __init mrst_platform_init(void) > { > sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio); > sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs); > + > + /* Set platform thermal data pointer */ > + get_platform_thermal_params = mrst_get_thermal_params; > + > return 0; > } > arch_initcall(mrst_platform_init); > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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 ? :-) Thanks, Durga > > > +}; > > + > > static void mrst_power_off(void) > > { > > } > > @@ -983,10 +1008,27 @@ static int __init sfi_parse_devs(struct > sfi_table_header *table) > > return 0; > > } > > > > +static int mrst_get_thermal_params(struct thermal_zone_device *tz) > > +{ > > + int i; > > + > > + for (i = 0; i < MRST_THERMAL_ZONES; i++) { > > + if (!strcmp(tzp[i].thermal_zone_name, tz->type)) { > > + tz->tzp = &tzp[i]; > > + return 0; > > + } > > + } > > + return -ENODEV; > > +} > > + > > static int __init mrst_platform_init(void) > > { > > sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio); > > sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs); > > + > > + /* Set platform thermal data pointer */ > > + get_platform_thermal_params = mrst_get_thermal_params; > > + > > return 0; > > } > > arch_initcall(mrst_platform_init); > > -- > > 1.7.9.5 > > > -- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On ?, 2012-08-20 at 23:52 -0600, R, Durgadoss wrote: > > > -----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. > Thanks, > Durga > > > > > > +}; > > > + > > > static void mrst_power_off(void) > > > { > > > } > > > @@ -983,10 +1008,27 @@ static int __init sfi_parse_devs(struct > > sfi_table_header *table) > > > return 0; > > > } > > > > > > +static int mrst_get_thermal_params(struct thermal_zone_device *tz) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < MRST_THERMAL_ZONES; i++) { > > > + if (!strcmp(tzp[i].thermal_zone_name, tz->type)) { > > > + tz->tzp = &tzp[i]; > > > + return 0; > > > + } > > > + } > > > + return -ENODEV; > > > +} > > > + > > > static int __init mrst_platform_init(void) > > > { > > > sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio); > > > sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs); > > > + > > > + /* Set platform thermal data pointer */ > > > + get_platform_thermal_params = mrst_get_thermal_params; > > > + > > > return 0; > > > } > > > arch_initcall(mrst_platform_init); > > > -- > > > 1.7.9.5 > > > > > -- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ? 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
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-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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@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 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-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On ?, 2012-08-21 at 03:28 -0600, R, Durgadoss wrote: > 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]; no, not num_cdevs. Say CPU is used for both passive trip point 1 and passive trip point 2, it has different lower/upper limit and weight. and we need two thermal_bind_params here. > }; > > Where struct thermal_bind_params will be like this: > { > .cdev_name = "CPU" and the cooling device name must be unique in this case, which I do not think it is reasonable. because cooling devices are registered from different drivers, we do not have a rule that followed by all these drivers. > .trip_mask = 0x0F > .weight = 70 > .lower = 2 > .upper = 4 > .sample_period = 1000 (1 ms) > }; > if we want a way to register a couple of binding information to thermal framework together with the thermal zone, I'd prefer something like this, struct thermal_zone_params { struct thermal_zone_device *tz; int count; struct thermal_bind_params *bindings; } struct thermal_bind_params { .id = 1; .match = platform_match_callback(); .cdev = NULL; .weight = 70 ... } and in thermal_zone_device_register, we can have the code like this: list_for_each_entry(cdev, &thermal_cdev_list, node) { for (i = 0; i < tz->params->count, i++) { if (tz->params->bindings[i].cdev) continue; /* check if this is the binding for this device */ if (tz->params->bindings[i].match(tz, cdev, i)) continue; tz->params->bindings[i]->cdev = cdev; thermal_zone_bind_cooling_device(tz, cdev, &tz->params->binding[i]); } } and we can have similar code in thermal_cdev_register(). you can do whatever in your platform_match_callback(), either strcmp or checking devdata, or anything else. thanks, rui -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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}, } +}; + static void mrst_power_off(void) { } @@ -983,10 +1008,27 @@ static int __init sfi_parse_devs(struct sfi_table_header *table) return 0; } +static int mrst_get_thermal_params(struct thermal_zone_device *tz) +{ + int i; + + for (i = 0; i < MRST_THERMAL_ZONES; i++) { + if (!strcmp(tzp[i].thermal_zone_name, tz->type)) { + tz->tzp = &tzp[i]; + return 0; + } + } + return -ENODEV; +} + static int __init mrst_platform_init(void) { sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio); sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs); + + /* Set platform thermal data pointer */ + get_platform_thermal_params = mrst_get_thermal_params; + return 0; } arch_initcall(mrst_platform_init);
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(+)