| Message ID | 1329905650-30161-2-git-send-email-amit.kachhap@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Amit, > -----Original Message----- > From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel > Kachhap > Sent: Wednesday, February 22, 2012 3:44 PM > To: linux-pm@lists.linux-foundation.org > Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux- > acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org; > amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org > Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device > instance number > > This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This > trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling > device instance number. This helps the cooling device registered as > different instances to perform appropriate cooling action decision in > the set_cur_state call back function. > > Also since the trip temperature's are in ascending order so some logic > is put in place to skip the un-necessary checks. > > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org> > --- > Documentation/thermal/sysfs-api.txt | 4 +- > drivers/thermal/thermal_sys.c | 45 ++++++++++++++++++++++++++++++++-- > include/linux/thermal.h | 1 + > 3 files changed, 45 insertions(+), 5 deletions(-) > > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs- > api.txt > index 1733ab9..7a0c080 100644 > --- a/Documentation/thermal/sysfs-api.txt > +++ b/Documentation/thermal/sysfs-api.txt > @@ -184,8 +184,8 @@ trip_point_[0-*]_temp > > trip_point_[0-*]_type > Strings which indicate the type of the trip point. > - E.g. it can be one of critical, hot, passive, active[0-*] for ACPI > - thermal zone. > + E.g. it can be one of critical, hot, passive, active[0-1], > + state-active[0-*] for ACPI thermal zone. > RO, Optional > > cdev[0-*] > diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c > index 220ce7e..d4c9b20 100644 > --- a/drivers/thermal/thermal_sys.c > +++ b/drivers/thermal/thermal_sys.c > @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct > device_attribute *attr, > return sprintf(buf, "passive\n"); > case THERMAL_TRIP_ACTIVE: > return sprintf(buf, "active\n"); > + case THERMAL_TRIP_STATE_ACTIVE: > + return sprintf(buf, "state-active\n"); > default: > return sprintf(buf, "unknown\n"); > } > @@ -1034,10 +1036,10 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister); > > void thermal_zone_device_update(struct thermal_zone_device *tz) > { > - int count, ret = 0; > - long temp, trip_temp; > + int count, ret = 0, inst_id; > + long temp, trip_temp, max_state, last_trip_change = 0; > enum thermal_trip_type trip_type; > - struct thermal_cooling_device_instance *instance; > + struct thermal_cooling_device_instance *instance, *state_instance; > struct thermal_cooling_device *cdev; > > mutex_lock(&tz->lock); > @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct > thermal_zone_device *tz) > cdev->ops->set_cur_state(cdev, 0); > } > break; > + case THERMAL_TRIP_STATE_ACTIVE: > + list_for_each_entry(instance, &tz->cooling_devices, > + node) { > + if (instance->trip != count) > + continue; > + > + if (temp <= last_trip_change) > + continue; > + > + inst_id = 0; > + /* > + *For this instance how many instance of same > + *cooling device occured before > + */ > + > + list_for_each_entry(state_instance, > + &tz->cooling_devices, node) { > + if (instance->cdev == > + state_instance->cdev) > + inst_id++; > + if (state_instance->trip == count) > + break; > + } Can you explain a bit more on this loop and the set_cur_state below ? Sorry, I don't get the logic behind this.. Thanks, Durga > + > + cdev = instance->cdev; > + cdev->ops->get_max_state(cdev, &max_state); > + > + if ((temp >= trip_temp) && > + (inst_id <= max_state)) > + cdev->ops->set_cur_state(cdev, inst_id); > + else if ((temp < trip_temp) && > + (--inst_id <= max_state)) > + cdev->ops->set_cur_state(cdev, inst_id); > + > + last_trip_change = trip_temp; > + } > + break; > case THERMAL_TRIP_PASSIVE: > if (temp >= trip_temp || tz->passive) > thermal_zone_device_passive(tz, temp, > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > index 796f1ff..8df901f 100644 > --- a/include/linux/thermal.h > +++ b/include/linux/thermal.h > @@ -42,6 +42,7 @@ enum thermal_trip_type { > THERMAL_TRIP_PASSIVE, > THERMAL_TRIP_HOT, > THERMAL_TRIP_CRITICAL, > + THERMAL_TRIP_STATE_ACTIVE, > }; > > struct thermal_zone_device_ops { > -- > 1.7.1
On 23 February 2012 12:16, R, Durgadoss <durgadoss.r@intel.com> wrote: > Hi Amit, > >> -----Original Message----- >> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel >> Kachhap >> Sent: Wednesday, February 22, 2012 3:44 PM >> To: linux-pm@lists.linux-foundation.org >> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux- >> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org; >> amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org >> Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device >> instance number >> >> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This >> trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling >> device instance number. This helps the cooling device registered as >> different instances to perform appropriate cooling action decision in >> the set_cur_state call back function. >> >> Also since the trip temperature's are in ascending order so some logic >> is put in place to skip the un-necessary checks. >> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org> >> --- >> Documentation/thermal/sysfs-api.txt | 4 +- >> drivers/thermal/thermal_sys.c | 45 ++++++++++++++++++++++++++++++++-- >> include/linux/thermal.h | 1 + >> 3 files changed, 45 insertions(+), 5 deletions(-) >> >> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs- >> api.txt >> index 1733ab9..7a0c080 100644 >> --- a/Documentation/thermal/sysfs-api.txt >> +++ b/Documentation/thermal/sysfs-api.txt >> @@ -184,8 +184,8 @@ trip_point_[0-*]_temp >> >> trip_point_[0-*]_type >> Strings which indicate the type of the trip point. >> - E.g. it can be one of critical, hot, passive, active[0-*] for ACPI >> - thermal zone. >> + E.g. it can be one of critical, hot, passive, active[0-1], >> + state-active[0-*] for ACPI thermal zone. >> RO, Optional >> >> cdev[0-*] >> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c >> index 220ce7e..d4c9b20 100644 >> --- a/drivers/thermal/thermal_sys.c >> +++ b/drivers/thermal/thermal_sys.c >> @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct >> device_attribute *attr, >> return sprintf(buf, "passive\n"); >> case THERMAL_TRIP_ACTIVE: >> return sprintf(buf, "active\n"); >> + case THERMAL_TRIP_STATE_ACTIVE: >> + return sprintf(buf, "state-active\n"); >> default: >> return sprintf(buf, "unknown\n"); >> } >> @@ -1034,10 +1036,10 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister); >> >> void thermal_zone_device_update(struct thermal_zone_device *tz) >> { >> - int count, ret = 0; >> - long temp, trip_temp; >> + int count, ret = 0, inst_id; >> + long temp, trip_temp, max_state, last_trip_change = 0; >> enum thermal_trip_type trip_type; >> - struct thermal_cooling_device_instance *instance; >> + struct thermal_cooling_device_instance *instance, *state_instance; >> struct thermal_cooling_device *cdev; >> >> mutex_lock(&tz->lock); >> @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct >> thermal_zone_device *tz) >> cdev->ops->set_cur_state(cdev, 0); >> } >> break; >> + case THERMAL_TRIP_STATE_ACTIVE: >> + list_for_each_entry(instance, &tz->cooling_devices, >> + node) { >> + if (instance->trip != count) >> + continue; >> + >> + if (temp <= last_trip_change) >> + continue; >> + >> + inst_id = 0; >> + /* >> + *For this instance how many instance of same >> + *cooling device occured before >> + */ >> + >> + list_for_each_entry(state_instance, >> + &tz->cooling_devices, node) { >> + if (instance->cdev == >> + state_instance->cdev) >> + inst_id++; >> + if (state_instance->trip == count) >> + break; >> + } > > Can you explain a bit more on this loop and the set_cur_state below ? > Sorry, I don't get the logic behind this.. This loop is basically finding the instance id of the same cooling device. Say we have done like this, thermal_zone_bind_cooling_device(thermal, 2, cdev); thermal_zone_bind_cooling_device(thermal, 3, cdev); thermal_zone_bind_cooling_device(thermal, 4, cdev); In above same cooling device cdev is binded to trip no 2,3 and 4 with inst_id generated as 1,2,3 respectively. so set_cur_state for those trip reached will be called as, set_cur_state(cdev, 1); set_cur_state(cdev, 2); set_cur_state(cdev, 3); Thanks, Amit D > > Thanks, > Durga > >> + >> + cdev = instance->cdev; >> + cdev->ops->get_max_state(cdev, &max_state); >> + >> + if ((temp >= trip_temp) && >> + (inst_id <= max_state)) >> + cdev->ops->set_cur_state(cdev, inst_id); >> + else if ((temp < trip_temp) && >> + (--inst_id <= max_state)) >> + cdev->ops->set_cur_state(cdev, inst_id); >> + >> + last_trip_change = trip_temp; >> + } >> + break; >> case THERMAL_TRIP_PASSIVE: >> if (temp >= trip_temp || tz->passive) >> thermal_zone_device_passive(tz, temp, >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h >> index 796f1ff..8df901f 100644 >> --- a/include/linux/thermal.h >> +++ b/include/linux/thermal.h >> @@ -42,6 +42,7 @@ enum thermal_trip_type { >> THERMAL_TRIP_PASSIVE, >> THERMAL_TRIP_HOT, >> THERMAL_TRIP_CRITICAL, >> + THERMAL_TRIP_STATE_ACTIVE, >> }; >> >> struct thermal_zone_device_ops { >> -- >> 1.7.1 >
Hello, On Thu, Feb 23, 2012 at 04:50:14PM +0530, Amit Kachhap wrote: > On 23 February 2012 12:16, R, Durgadoss <durgadoss.r@intel.com> wrote: > > Hi Amit, > > > >> -----Original Message----- > >> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel > >> Kachhap > >> Sent: Wednesday, February 22, 2012 3:44 PM > >> To: linux-pm@lists.linux-foundation.org > >> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux- > >> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org; > >> amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org > >> Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device > >> instance number > >> > >> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This > >> trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling > >> device instance number. This helps the cooling device registered as > >> different instances to perform appropriate cooling action decision in > >> the set_cur_state call back function. > >> > >> Also since the trip temperature's are in ascending order so some logic > >> is put in place to skip the un-necessary checks. > >> > >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org> > >> --- > >> Documentation/thermal/sysfs-api.txt | 4 +- > >> drivers/thermal/thermal_sys.c | 45 ++++++++++++++++++++++++++++++++-- > >> include/linux/thermal.h | 1 + > >> 3 files changed, 45 insertions(+), 5 deletions(-) > >> > >> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs- > >> api.txt > >> index 1733ab9..7a0c080 100644 > >> --- a/Documentation/thermal/sysfs-api.txt > >> +++ b/Documentation/thermal/sysfs-api.txt > >> @@ -184,8 +184,8 @@ trip_point_[0-*]_temp > >> > >> trip_point_[0-*]_type > >> Strings which indicate the type of the trip point. > >> - E.g. it can be one of critical, hot, passive, active[0-*] for ACPI > >> - thermal zone. > >> + E.g. it can be one of critical, hot, passive, active[0-1], > >> + state-active[0-*] for ACPI thermal zone. > >> RO, Optional > >> > >> cdev[0-*] > >> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c > >> index 220ce7e..d4c9b20 100644 > >> --- a/drivers/thermal/thermal_sys.c > >> +++ b/drivers/thermal/thermal_sys.c > >> @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct > >> device_attribute *attr, > >> return sprintf(buf, "passive\n"); > >> case THERMAL_TRIP_ACTIVE: > >> return sprintf(buf, "active\n"); > >> + case THERMAL_TRIP_STATE_ACTIVE: > >> + return sprintf(buf, "state-active\n"); > >> default: > >> return sprintf(buf, "unknown\n"); > >> } > >> @@ -1034,10 +1036,10 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister); > >> > >> void thermal_zone_device_update(struct thermal_zone_device *tz) > >> { > >> - int count, ret = 0; > >> - long temp, trip_temp; > >> + int count, ret = 0, inst_id; > >> + long temp, trip_temp, max_state, last_trip_change = 0; > >> enum thermal_trip_type trip_type; > >> - struct thermal_cooling_device_instance *instance; > >> + struct thermal_cooling_device_instance *instance, *state_instance; > >> struct thermal_cooling_device *cdev; > >> > >> mutex_lock(&tz->lock); > >> @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct > >> thermal_zone_device *tz) > >> cdev->ops->set_cur_state(cdev, 0); > >> } > >> break; > >> + case THERMAL_TRIP_STATE_ACTIVE: > >> + list_for_each_entry(instance, &tz->cooling_devices, > >> + node) { > >> + if (instance->trip != count) > >> + continue; > >> + > >> + if (temp <= last_trip_change) > >> + continue; > >> + > >> + inst_id = 0; > >> + /* > >> + *For this instance how many instance of same > >> + *cooling device occured before > >> + */ > >> + > >> + list_for_each_entry(state_instance, > >> + &tz->cooling_devices, node) { > >> + if (instance->cdev == > >> + state_instance->cdev) > >> + inst_id++; > >> + if (state_instance->trip == count) > >> + break; > >> + } > > > > Can you explain a bit more on this loop and the set_cur_state below ? > > Sorry, I don't get the logic behind this.. > > This loop is basically finding the instance id of the same cooling device. > Say we have done like this, > thermal_zone_bind_cooling_device(thermal, 2, cdev); > thermal_zone_bind_cooling_device(thermal, 3, cdev); > thermal_zone_bind_cooling_device(thermal, 4, cdev); > > In above same cooling device cdev is binded to trip no 2,3 and 4 with > inst_id generated as 1,2,3 respectively. so set_cur_state for those > trip reached will be called as, > set_cur_state(cdev, 1); > set_cur_state(cdev, 2); > set_cur_state(cdev, 3); In this case, why a simple state = get_cur_state() followed by a set_cur_state(++state) / set_cur_state(--state) is not enough? Another thing is if we want to do jumps in the sequence? set_cur_state(cdev, 1); set_cur_state(cdev, 3); set_cur_state(cdev, 6); But for that we need a table mapping, trip vs. state. What do you think? > > Thanks, > Amit D > > > > > Thanks, > > Durga > > > >> + > >> + cdev = instance->cdev; > >> + cdev->ops->get_max_state(cdev, &max_state); > >> + > >> + if ((temp >= trip_temp) && > >> + (inst_id <= max_state)) > >> + cdev->ops->set_cur_state(cdev, inst_id); > >> + else if ((temp < trip_temp) && > >> + (--inst_id <= max_state)) > >> + cdev->ops->set_cur_state(cdev, inst_id); > >> + > >> + last_trip_change = trip_temp; > >> + } > >> + break; > >> case THERMAL_TRIP_PASSIVE: > >> if (temp >= trip_temp || tz->passive) > >> thermal_zone_device_passive(tz, temp, > >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h > >> index 796f1ff..8df901f 100644 > >> --- a/include/linux/thermal.h > >> +++ b/include/linux/thermal.h > >> @@ -42,6 +42,7 @@ enum thermal_trip_type { > >> THERMAL_TRIP_PASSIVE, > >> THERMAL_TRIP_HOT, > >> THERMAL_TRIP_CRITICAL, > >> + THERMAL_TRIP_STATE_ACTIVE, > >> }; > >> > >> struct thermal_zone_device_ops { > >> -- > >> 1.7.1 > > > _______________________________________________ > linux-pm mailing list > linux-pm@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
Hi Eduardo, On 3 April 2012 19:45, Eduardo Valentin <eduardo.valentin@ti.com> wrote: > Hello, > > On Thu, Feb 23, 2012 at 04:50:14PM +0530, Amit Kachhap wrote: >> On 23 February 2012 12:16, R, Durgadoss <durgadoss.r@intel.com> wrote: >> > Hi Amit, >> > >> >> -----Original Message----- >> >> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel >> >> Kachhap >> >> Sent: Wednesday, February 22, 2012 3:44 PM >> >> To: linux-pm@lists.linux-foundation.org >> >> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux- >> >> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org; >> >> amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org >> >> Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device >> >> instance number >> >> >> >> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This >> >> trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling >> >> device instance number. This helps the cooling device registered as >> >> different instances to perform appropriate cooling action decision in >> >> the set_cur_state call back function. >> >> >> >> Also since the trip temperature's are in ascending order so some logic >> >> is put in place to skip the un-necessary checks. >> >> >> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org> >> >> --- >> >> Documentation/thermal/sysfs-api.txt | 4 +- >> >> drivers/thermal/thermal_sys.c | 45 ++++++++++++++++++++++++++++++++-- >> >> include/linux/thermal.h | 1 + >> >> 3 files changed, 45 insertions(+), 5 deletions(-) >> >> >> >> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs- >> >> api.txt >> >> index 1733ab9..7a0c080 100644 >> >> --- a/Documentation/thermal/sysfs-api.txt >> >> +++ b/Documentation/thermal/sysfs-api.txt >> >> @@ -184,8 +184,8 @@ trip_point_[0-*]_temp >> >> >> >> trip_point_[0-*]_type >> >> Strings which indicate the type of the trip point. >> >> - E.g. it can be one of critical, hot, passive, active[0-*] for ACPI >> >> - thermal zone. >> >> + E.g. it can be one of critical, hot, passive, active[0-1], >> >> + state-active[0-*] for ACPI thermal zone. >> >> RO, Optional >> >> >> >> cdev[0-*] >> >> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c >> >> index 220ce7e..d4c9b20 100644 >> >> --- a/drivers/thermal/thermal_sys.c >> >> +++ b/drivers/thermal/thermal_sys.c >> >> @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct >> >> device_attribute *attr, >> >> return sprintf(buf, "passive\n"); >> >> case THERMAL_TRIP_ACTIVE: >> >> return sprintf(buf, "active\n"); >> >> + case THERMAL_TRIP_STATE_ACTIVE: >> >> + return sprintf(buf, "state-active\n"); >> >> default: >> >> return sprintf(buf, "unknown\n"); >> >> } >> >> @@ -1034,10 +1036,10 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister); >> >> >> >> void thermal_zone_device_update(struct thermal_zone_device *tz) >> >> { >> >> - int count, ret = 0; >> >> - long temp, trip_temp; >> >> + int count, ret = 0, inst_id; >> >> + long temp, trip_temp, max_state, last_trip_change = 0; >> >> enum thermal_trip_type trip_type; >> >> - struct thermal_cooling_device_instance *instance; >> >> + struct thermal_cooling_device_instance *instance, *state_instance; >> >> struct thermal_cooling_device *cdev; >> >> >> >> mutex_lock(&tz->lock); >> >> @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct >> >> thermal_zone_device *tz) >> >> cdev->ops->set_cur_state(cdev, 0); >> >> } >> >> break; >> >> + case THERMAL_TRIP_STATE_ACTIVE: >> >> + list_for_each_entry(instance, &tz->cooling_devices, >> >> + node) { >> >> + if (instance->trip != count) >> >> + continue; >> >> + >> >> + if (temp <= last_trip_change) >> >> + continue; >> >> + >> >> + inst_id = 0; >> >> + /* >> >> + *For this instance how many instance of same >> >> + *cooling device occured before >> >> + */ >> >> + >> >> + list_for_each_entry(state_instance, >> >> + &tz->cooling_devices, node) { >> >> + if (instance->cdev == >> >> + state_instance->cdev) >> >> + inst_id++; >> >> + if (state_instance->trip == count) >> >> + break; >> >> + } >> > >> > Can you explain a bit more on this loop and the set_cur_state below ? >> > Sorry, I don't get the logic behind this.. >> >> This loop is basically finding the instance id of the same cooling device. >> Say we have done like this, >> thermal_zone_bind_cooling_device(thermal, 2, cdev); >> thermal_zone_bind_cooling_device(thermal, 3, cdev); >> thermal_zone_bind_cooling_device(thermal, 4, cdev); >> >> In above same cooling device cdev is binded to trip no 2,3 and 4 with >> inst_id generated as 1,2,3 respectively. so set_cur_state for those >> trip reached will be called as, >> set_cur_state(cdev, 1); >> set_cur_state(cdev, 2); >> set_cur_state(cdev, 3); > > In this case, why a simple state = get_cur_state() followed by a > set_cur_state(++state) / set_cur_state(--state) is not enough? Thanks for looking into the patch. Well actually what you are suggesting is exactly happening in PASSIVE trip types where the states are incremented or decremented based on thermal trend. On the contrary what this part of code is doing is to jump to a fixed state as and when a trip point is reached. The cooling effect of a frequency level is known beforehand and hence jumping into that is safe and also this does not cause performance degradation by going into a much lower frequency state just for temperature stablization. > > Another thing is if we want to do jumps in the sequence? > > set_cur_state(cdev, 1); > set_cur_state(cdev, 3); > set_cur_state(cdev, 6); > > But for that we need a table mapping, trip vs. state. > > > What do you think? In the current thermal_zone_device_update implementation all the checks are currently based on increase in temperature. So even though we want to go to set_cur_state(cdev, 6) we have to follow set_cur_state(cdev, 1), set_cur_state(cdev, 2) ,..... and finally set_cur_state(cdev, 6). So mapping table may not be needed as state transition is one by one. This a type of limitation but I think there can be some kind of modification in the way the thermal_zone_device_update calls all the lower temperature cooling devices instead of jumping directly to the final trip point cooling devices. > >> >> Thanks, >> Amit D >> >> > >> > Thanks, >> > Durga >> > >> >> + >> >> + cdev = instance->cdev; >> >> + cdev->ops->get_max_state(cdev, &max_state); >> >> + >> >> + if ((temp >= trip_temp) && >> >> + (inst_id <= max_state)) >> >> + cdev->ops->set_cur_state(cdev, inst_id); >> >> + else if ((temp < trip_temp) && >> >> + (--inst_id <= max_state)) >> >> + cdev->ops->set_cur_state(cdev, inst_id); >> >> + >> >> + last_trip_change = trip_temp; >> >> + } >> >> + break; >> >> case THERMAL_TRIP_PASSIVE: >> >> if (temp >= trip_temp || tz->passive) >> >> thermal_zone_device_passive(tz, temp, >> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h >> >> index 796f1ff..8df901f 100644 >> >> --- a/include/linux/thermal.h >> >> +++ b/include/linux/thermal.h >> >> @@ -42,6 +42,7 @@ enum thermal_trip_type { >> >> THERMAL_TRIP_PASSIVE, >> >> THERMAL_TRIP_HOT, >> >> THERMAL_TRIP_CRITICAL, >> >> + THERMAL_TRIP_STATE_ACTIVE, >> >> }; >> >> >> >> struct thermal_zone_device_ops { >> >> -- >> >> 1.7.1 >> > >> _______________________________________________ >> linux-pm mailing list >> linux-pm@lists.linux-foundation.org >> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
Hello, On Wed, Apr 04, 2012 at 09:53:15AM +0530, Amit Kachhap wrote: > Hi Eduardo, > > On 3 April 2012 19:45, Eduardo Valentin <eduardo.valentin@ti.com> wrote: > > Hello, > > > > On Thu, Feb 23, 2012 at 04:50:14PM +0530, Amit Kachhap wrote: > >> On 23 February 2012 12:16, R, Durgadoss <durgadoss.r@intel.com> wrote: > >> > Hi Amit, > >> > > >> >> -----Original Message----- > >> >> From: amit kachhap [mailto:amitdanielk@gmail.com] On Behalf Of Amit Daniel > >> >> Kachhap > >> >> Sent: Wednesday, February 22, 2012 3:44 PM > >> >> To: linux-pm@lists.linux-foundation.org > >> >> Cc: linux-kernel@vger.kernel.org; mjg59@srcf.ucam.org; linux- > >> >> acpi@vger.kernel.org; lenb@kernel.org; linaro-dev@lists.linaro.org; > >> >> amit.kachhap@linaro.org; R, Durgadoss; rob.lee@linaro.org; patches@linaro.org > >> >> Subject: [PATCH 1/4] thermal: Add a new trip type to use cooling device > >> >> instance number > >> >> > >> >> This patch adds a new trip type THERMAL_TRIP_STATE_ACTIVE. This > >> >> trip behaves same as THERMAL_TRIP_ACTIVE but also passes the cooling > >> >> device instance number. This helps the cooling device registered as > >> >> different instances to perform appropriate cooling action decision in > >> >> the set_cur_state call back function. > >> >> > >> >> Also since the trip temperature's are in ascending order so some logic > >> >> is put in place to skip the un-necessary checks. > >> >> > >> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org> > >> >> --- > >> >> Documentation/thermal/sysfs-api.txt | 4 +- > >> >> drivers/thermal/thermal_sys.c | 45 ++++++++++++++++++++++++++++++++-- > >> >> include/linux/thermal.h | 1 + > >> >> 3 files changed, 45 insertions(+), 5 deletions(-) > >> >> > >> >> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs- > >> >> api.txt > >> >> index 1733ab9..7a0c080 100644 > >> >> --- a/Documentation/thermal/sysfs-api.txt > >> >> +++ b/Documentation/thermal/sysfs-api.txt > >> >> @@ -184,8 +184,8 @@ trip_point_[0-*]_temp > >> >> > >> >> trip_point_[0-*]_type > >> >> Strings which indicate the type of the trip point. > >> >> - E.g. it can be one of critical, hot, passive, active[0-*] for ACPI > >> >> - thermal zone. > >> >> + E.g. it can be one of critical, hot, passive, active[0-1], > >> >> + state-active[0-*] for ACPI thermal zone. > >> >> RO, Optional > >> >> > >> >> cdev[0-*] > >> >> diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c > >> >> index 220ce7e..d4c9b20 100644 > >> >> --- a/drivers/thermal/thermal_sys.c > >> >> +++ b/drivers/thermal/thermal_sys.c > >> >> @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct > >> >> device_attribute *attr, > >> >> return sprintf(buf, "passive\n"); > >> >> case THERMAL_TRIP_ACTIVE: > >> >> return sprintf(buf, "active\n"); > >> >> + case THERMAL_TRIP_STATE_ACTIVE: > >> >> + return sprintf(buf, "state-active\n"); > >> >> default: > >> >> return sprintf(buf, "unknown\n"); > >> >> } > >> >> @@ -1034,10 +1036,10 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister); > >> >> > >> >> void thermal_zone_device_update(struct thermal_zone_device *tz) > >> >> { > >> >> - int count, ret = 0; > >> >> - long temp, trip_temp; > >> >> + int count, ret = 0, inst_id; > >> >> + long temp, trip_temp, max_state, last_trip_change = 0; > >> >> enum thermal_trip_type trip_type; > >> >> - struct thermal_cooling_device_instance *instance; > >> >> + struct thermal_cooling_device_instance *instance, *state_instance; > >> >> struct thermal_cooling_device *cdev; > >> >> > >> >> mutex_lock(&tz->lock); > >> >> @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct > >> >> thermal_zone_device *tz) > >> >> cdev->ops->set_cur_state(cdev, 0); > >> >> } > >> >> break; > >> >> + case THERMAL_TRIP_STATE_ACTIVE: > >> >> + list_for_each_entry(instance, &tz->cooling_devices, > >> >> + node) { > >> >> + if (instance->trip != count) > >> >> + continue; > >> >> + > >> >> + if (temp <= last_trip_change) > >> >> + continue; > >> >> + > >> >> + inst_id = 0; > >> >> + /* > >> >> + *For this instance how many instance of same > >> >> + *cooling device occured before > >> >> + */ > >> >> + > >> >> + list_for_each_entry(state_instance, > >> >> + &tz->cooling_devices, node) { > >> >> + if (instance->cdev == > >> >> + state_instance->cdev) > >> >> + inst_id++; > >> >> + if (state_instance->trip == count) > >> >> + break; > >> >> + } > >> > > >> > Can you explain a bit more on this loop and the set_cur_state below ? > >> > Sorry, I don't get the logic behind this.. > >> > >> This loop is basically finding the instance id of the same cooling device. > >> Say we have done like this, > >> thermal_zone_bind_cooling_device(thermal, 2, cdev); > >> thermal_zone_bind_cooling_device(thermal, 3, cdev); > >> thermal_zone_bind_cooling_device(thermal, 4, cdev); > >> > >> In above same cooling device cdev is binded to trip no 2,3 and 4 with > >> inst_id generated as 1,2,3 respectively. so set_cur_state for those > >> trip reached will be called as, > >> set_cur_state(cdev, 1); > >> set_cur_state(cdev, 2); > >> set_cur_state(cdev, 3); > > > > In this case, why a simple state = get_cur_state() followed by a > > set_cur_state(++state) / set_cur_state(--state) is not enough? > > Thanks for looking into the patch. Well actually what you are > suggesting is exactly happening in PASSIVE trip types where the states > are incremented or decremented based on thermal trend. On the contrary > what this part of code is doing is to jump to a fixed state as and > when a trip point is reached. The cooling effect of a frequency level > is known beforehand and hence jumping into that is safe and also this > does not cause performance degradation by going into a much lower > frequency state just for temperature stablization. Yeah, I see that you want to match an instance of the cooling device with a cooling state for that cooling device. > > > > > Another thing is if we want to do jumps in the sequence? > > > > set_cur_state(cdev, 1); > > set_cur_state(cdev, 3); > > set_cur_state(cdev, 6); > > > > But for that we need a table mapping, trip vs. state. > > > > > > What do you think? > In the current thermal_zone_device_update implementation all the > checks are currently based on increase in temperature. So even though > we want to go to set_cur_state(cdev, 6) we have to follow > set_cur_state(cdev, 1), set_cur_state(cdev, 2) ,..... and finally > set_cur_state(cdev, 6). So mapping table may not be needed as state > transition is one by one. This a type of limitation but I think there > can be some kind of modification in the way the > thermal_zone_device_update calls all the lower temperature cooling > devices instead of jumping directly to the final trip point cooling > devices. But, because you also force the thing to be increasing / decreasing 1 by 1, I don't understand why you don't have a similar implementation of the PASSIVE type. Just get its cur state and set the next state based on the curr. How that would be different than what you are currently doing? If you want to have a 1 to 1 relation between cooling device instance and cooling device state, then you could do the jumps I mentioned, but only if you would have the cooling state reference stored somewhere. What bugs me is the fact that you have to be iterating in the cooling device list to determine the next cooling state. > > > >> > >> Thanks, > >> Amit D > >> > >> > > >> > Thanks, > >> > Durga > >> > > >> >> + > >> >> + cdev = instance->cdev; > >> >> + cdev->ops->get_max_state(cdev, &max_state); > >> >> + > >> >> + if ((temp >= trip_temp) && > >> >> + (inst_id <= max_state)) > >> >> + cdev->ops->set_cur_state(cdev, inst_id); > >> >> + else if ((temp < trip_temp) && > >> >> + (--inst_id <= max_state)) > >> >> + cdev->ops->set_cur_state(cdev, inst_id); > >> >> + > >> >> + last_trip_change = trip_temp; > >> >> + } > >> >> + break; > >> >> case THERMAL_TRIP_PASSIVE: > >> >> if (temp >= trip_temp || tz->passive) > >> >> thermal_zone_device_passive(tz, temp, > >> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h > >> >> index 796f1ff..8df901f 100644 > >> >> --- a/include/linux/thermal.h > >> >> +++ b/include/linux/thermal.h > >> >> @@ -42,6 +42,7 @@ enum thermal_trip_type { > >> >> THERMAL_TRIP_PASSIVE, > >> >> THERMAL_TRIP_HOT, > >> >> THERMAL_TRIP_CRITICAL, > >> >> + THERMAL_TRIP_STATE_ACTIVE, > >> >> }; > >> >> > >> >> struct thermal_zone_device_ops { > >> >> -- > >> >> 1.7.1 > >> > > >> _______________________________________________ > >> linux-pm mailing list > >> linux-pm@lists.linux-foundation.org > >> https://lists.linuxfoundation.org/mailman/listinfo/linux-pm
different instances to perform appropriate cooling action decision in the set_cur_state call back function. Also since the trip temperature's are in ascending order so some logic is put in place to skip the un-necessary checks. Signed-off-by: Amit Daniel Kachhap <amit.kachhap@linaro.org> --- Documentation/thermal/sysfs-api.txt | 4 +- drivers/thermal/thermal_sys.c | 45 ++++++++++++++++++++++++++++++++-- include/linux/thermal.h | 1 + 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt index 1733ab9..7a0c080 100644 --- a/Documentation/thermal/sysfs-api.txt +++ b/Documentation/thermal/sysfs-api.txt @@ -184,8 +184,8 @@ trip_point_[0-*]_temp trip_point_[0-*]_type Strings which indicate the type of the trip point. - E.g. it can be one of critical, hot, passive, active[0-*] for ACPI - thermal zone. + E.g. it can be one of critical, hot, passive, active[0-1], + state-active[0-*] for ACPI thermal zone. RO, Optional cdev[0-*] diff --git a/drivers/thermal/thermal_sys.c b/drivers/thermal/thermal_sys.c index 220ce7e..d4c9b20 100644 --- a/drivers/thermal/thermal_sys.c +++ b/drivers/thermal/thermal_sys.c @@ -192,6 +192,8 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr, return sprintf(buf, "passive\n"); case THERMAL_TRIP_ACTIVE: return sprintf(buf, "active\n"); + case THERMAL_TRIP_STATE_ACTIVE: + return sprintf(buf, "state-active\n"); default: return sprintf(buf, "unknown\n"); } @@ -1034,10 +1036,10 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister); void thermal_zone_device_update(struct thermal_zone_device *tz) { - int count, ret = 0; - long temp, trip_temp; + int count, ret = 0, inst_id; + long temp, trip_temp, max_state, last_trip_change = 0; enum thermal_trip_type trip_type; - struct thermal_cooling_device_instance *instance; + struct thermal_cooling_device_instance *instance, *state_instance; struct thermal_cooling_device *cdev; mutex_lock(&tz->lock); @@ -1086,6 +1088,43 @@ void thermal_zone_device_update(struct thermal_zone_device *tz) cdev->ops->set_cur_state(cdev, 0); } break; + case THERMAL_TRIP_STATE_ACTIVE: + list_for_each_entry(instance, &tz->cooling_devices, + node) { + if (instance->trip != count) + continue; + + if (temp <= last_trip_change) + continue; + + inst_id = 0; + /* + *For this instance how many instance of same + *cooling device occured before + */ + + list_for_each_entry(state_instance, + &tz->cooling_devices, node) { + if (instance->cdev == + state_instance->cdev) + inst_id++; + if (state_instance->trip == count) + break; + } + + cdev = instance->cdev; + cdev->ops->get_max_state(cdev, &max_state); + + if ((temp >= trip_temp) && + (inst_id <= max_state)) + cdev->ops->set_cur_state(cdev, inst_id); + else if ((temp < trip_temp) && + (--inst_id <= max_state)) + cdev->ops->set_cur_state(cdev, inst_id); + + last_trip_change = trip_temp; + } + break; case THERMAL_TRIP_PASSIVE: if (temp >= trip_temp || tz->passive) thermal_zone_device_passive(tz, temp, diff --git a/include/linux/thermal.h b/include/linux/thermal.h index 796f1ff..8df901f 100644 --- a/include/linux/thermal.h +++ b/include/linux/thermal.h @@ -42,6 +42,7 @@ enum thermal_trip_type { THERMAL_TRIP_PASSIVE, THERMAL_TRIP_HOT, THERMAL_TRIP_CRITICAL, + THERMAL_TRIP_STATE_ACTIVE, }; struct thermal_zone_device_ops {