[RFC,1/2] 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

Hi Amit,

I'm not sure that using the trip index for setting the state of a
cooling device is a generic solution because you are adding a
dependency between the trip index and the cooling device state that
might be difficult to handle. This dependency implies that a cooling
device like cpufreq_cooling_device must be registered in the 1st trips
of a thermal_zone which is not possible when we want to register 2
cpufreq_cooling_devices in the same thermal_zone.
You should only rely on the current and last temperatures to detect if
a trip_temp has been crossed and you should increase or decrease the
current state of the cooling device accordingly.

something like below should work with cpufreq_cooling_device and will
not add any constraint on the trip index. The state of a cooling
device is increased/decreased once for each trip

+		case THERMAL_TRIP_STATE_ACTIVE:
+			list_for_each_entry(instance, &tz->cooling_devices,
+					    node) {
+				if (instance->trip != count)
+					continue;
+
+				cdev = instance->cdev;
+
+				if ((temp >= trip_temp)
+					&& (trip_temp > tz->last_temperature)) {
+					cdev->ops->get_max_state(cdev,
+							&max_state);
+					cdev->ops->get_cur_state(cdev,
+							&current_state);
+					if (++current_state <= max_state)
+						cdev->ops->set_cur_state(cdev,
+								current_state);
+				}
+				else if ((temp < trip_temp)
+					&& (trip_temp <= tz->last_temperature)) {
+					cdev->ops->get_cur_state(cdev,
+							&current_state);
+					if (current_state > 0)
+						cdev->ops->set_cur_state(cdev,
+								--current_state);
+			}
+			break;

Regards,
Vincent

On 13 December 2011 16:13, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
> 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       |   27 ++++++++++++++++++++++++++-
>  include/linux/thermal.h             |    1 +
>  3 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> index b61e46f..5c1d44e 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 dd9a574..72b1ab3 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");
>        }
> @@ -1035,7 +1037,7 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
>  void thermal_zone_device_update(struct thermal_zone_device *tz)
>  {
>        int count, ret = 0;
> -       long temp, trip_temp;
> +       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 *cdev;
> @@ -1086,6 +1088,29 @@ 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;
> +
> +                               cdev = instance->cdev;
> +                               cdev->ops->get_max_state(cdev, &max_state);
> +
> +                               if ((temp >= trip_temp) &&
> +                                               ((count + 1) <= max_state))
> +                                       cdev->ops->set_cur_state(cdev,
> +                                                               count + 1);
> +                               else if ((temp < trip_temp) &&
> +                                                       (count <= max_state))
> +                                       cdev->ops->set_cur_state(cdev, count);
> +
> +                               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 47b4a27..d7d0a27 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 Vincent,

Thanks for the review.
Well actually your are correct that current temperature and last
temperature can be used to increase or decrease the cufreq. But this has to
be done again in cooling devices so to make the cooling devices generic and
to avoid the temperature comparision again this new trip type passes the
cooling device instance id.
Also about your queries that this may add dependency between trip index and
cooling state. This is actually needed and this dependency is created when
the cooling device is binded with trip points(For cpufreq type cooling
device just the instance of cooling device is associated with trip points).
More over the existing PASSIVE cooling trip type does the same thing and
iterates across all the cooling state.

Thanks,
Amit Daniel

On 20 December 2011 18:07, Vincent Guittot <vincent.guittot@linaro.org>wrote:

> Hi Amit,
>
> I'm not sure that using the trip index for setting the state of a
> cooling device is a generic solution because you are adding a
> dependency between the trip index and the cooling device state that
> might be difficult to handle. This dependency implies that a cooling
> device like cpufreq_cooling_device must be registered in the 1st trips
> of a thermal_zone which is not possible when we want to register 2
> cpufreq_cooling_devices in the same thermal_zone.
> You should only rely on the current and last temperatures to detect if
> a trip_temp has been crossed and you should increase or decrease the
> current state of the cooling device accordingly.
>
> something like below should work with cpufreq_cooling_device and will
> not add any constraint on the trip index. The state of a cooling
> device is increased/decreased once for each trip
>
> +               case THERMAL_TRIP_STATE_ACTIVE:
> +                       list_for_each_entry(instance, &tz->cooling_devices,
> +                                           node) {
> +                               if (instance->trip != count)
> +                                       continue;
> +
> +                               cdev = instance->cdev;
> +
> +                               if ((temp >= trip_temp)
> +                                       && (trip_temp >
> tz->last_temperature)) {
> +                                       cdev->ops->get_max_state(cdev,
> +                                                       &max_state);
> +                                       cdev->ops->get_cur_state(cdev,
> +                                                       &current_state);
> +                                       if (++current_state <= max_state)
> +
> cdev->ops->set_cur_state(cdev,
> +
> current_state);
> +                               }
> +                               else if ((temp < trip_temp)
> +                                       && (trip_temp <=
> tz->last_temperature)) {
> +                                       cdev->ops->get_cur_state(cdev,
> +                                                       &current_state);
> +                                       if (current_state > 0)
> +
> cdev->ops->set_cur_state(cdev,
> +
> --current_state);
> +                       }
> +                       break;
>
> Regards,
> Vincent
>
> On 13 December 2011 16:13, Amit Daniel Kachhap <amit.kachhap@linaro.org>
> wrote:
> > 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       |   27 ++++++++++++++++++++++++++-
> >  include/linux/thermal.h             |    1 +
> >  3 files changed, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/thermal/sysfs-api.txt
> b/Documentation/thermal/sysfs-api.txt
> > index b61e46f..5c1d44e 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 dd9a574..72b1ab3 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");
> >        }
> > @@ -1035,7 +1037,7 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
> >  void thermal_zone_device_update(struct thermal_zone_device *tz)
> >  {
> >        int count, ret = 0;
> > -       long temp, trip_temp;
> > +       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 *cdev;
> > @@ -1086,6 +1088,29 @@ 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;
> > +
> > +                               cdev = instance->cdev;
> > +                               cdev->ops->get_max_state(cdev,
> &max_state);
> > +
> > +                               if ((temp >= trip_temp) &&
> > +                                               ((count + 1) <=
> max_state))
> > +                                       cdev->ops->set_cur_state(cdev,
> > +                                                               count +
> 1);
> > +                               else if ((temp < trip_temp) &&
> > +                                                       (count <=
> max_state))
> > +                                       cdev->ops->set_cur_state(cdev,
> count);
> > +
> > +                               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 47b4a27..d7d0a27 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 Vincent,

Thanks for the review.
Well actually your are correct that current temperature and last
temperature can be used to increase or decrease the cpu frequency. But
this has to be done again in cooling devices so to make the cooling
devices generic and to avoid the temperature comparison again this new
trip type passes the cooling device instance id.
Also about your queries that this may add dependency between trip
index and cooling state. This is actually needed and this dependency
is created when the cooling device is binded with trip points(For
cpufreq type cooling device just the instance of cooling device is
associated with trip points). More over the existing PASSIVE cooling
trip type does the same thing and iterates across all the cooling
state.

 Thanks,
 Amit Daniel
>
> On 20 December 2011 18:07, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>>
>> Hi Amit,
>>
>> I'm not sure that using the trip index for setting the state of a
>> cooling device is a generic solution because you are adding a
>> dependency between the trip index and the cooling device state that
>> might be difficult to handle. This dependency implies that a cooling
>> device like cpufreq_cooling_device must be registered in the 1st trips
>> of a thermal_zone which is not possible when we want to register 2
>> cpufreq_cooling_devices in the same thermal_zone.
>> You should only rely on the current and last temperatures to detect if
>> a trip_temp has been crossed and you should increase or decrease the
>> current state of the cooling device accordingly.
>>
>> something like below should work with cpufreq_cooling_device and will
>> not add any constraint on the trip index. The state of a cooling
>> device is increased/decreased once for each trip
>>
>> +               case THERMAL_TRIP_STATE_ACTIVE:
>> +                       list_for_each_entry(instance, &tz->cooling_devices,
>> +                                           node) {
>> +                               if (instance->trip != count)
>> +                                       continue;
>> +
>> +                               cdev = instance->cdev;
>> +
>> +                               if ((temp >= trip_temp)
>> +                                       && (trip_temp > tz->last_temperature)) {
>> +                                       cdev->ops->get_max_state(cdev,
>> +                                                       &max_state);
>> +                                       cdev->ops->get_cur_state(cdev,
>> +                                                       &current_state);
>> +                                       if (++current_state <= max_state)
>> +                                               cdev->ops->set_cur_state(cdev,
>> +                                                               current_state);
>> +                               }
>> +                               else if ((temp < trip_temp)
>> +                                       && (trip_temp <= tz->last_temperature)) {
>> +                                       cdev->ops->get_cur_state(cdev,
>> +                                                       &current_state);
>> +                                       if (current_state > 0)
>> +                                               cdev->ops->set_cur_state(cdev,
>> +                                                               --current_state);
>> +                       }
>> +                       break;
>>
>> Regards,
>> Vincent
>>
>> On 13 December 2011 16:13, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
>> > 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       |   27 ++++++++++++++++++++++++++-
>> >  include/linux/thermal.h             |    1 +
>> >  3 files changed, 29 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>> > index b61e46f..5c1d44e 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 dd9a574..72b1ab3 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");
>> >        }
>> > @@ -1035,7 +1037,7 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
>> >  void thermal_zone_device_update(struct thermal_zone_device *tz)
>> >  {
>> >        int count, ret = 0;
>> > -       long temp, trip_temp;
>> > +       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 *cdev;
>> > @@ -1086,6 +1088,29 @@ 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;
>> > +
>> > +                               cdev = instance->cdev;
>> > +                               cdev->ops->get_max_state(cdev, &max_state);
>> > +
>> > +                               if ((temp >= trip_temp) &&
>> > +                                               ((count + 1) <= max_state))
>> > +                                       cdev->ops->set_cur_state(cdev,
>> > +                                                               count + 1);
>> > +                               else if ((temp < trip_temp) &&
>> > +                                                       (count <= max_state))
>> > +                                       cdev->ops->set_cur_state(cdev, count);
>> > +
>> > +                               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 47b4a27..d7d0a27 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
>
>

Hey Amit/Vincent,

It appears that with this implementation the STATE_ACTIVE trip number
used will also be the index of the cool_freq_tab used.  If that is
true, then perhaps a common structure would be beneficial that links
each STATE_ACTIVE trip point with its corresponding cooling data.

BR,
Rob

On Tue, Dec 20, 2011 at 11:11 PM, Amit Kachhap <amit.kachhap@linaro.org> wrote:
>  Hi Vincent,
>
> Thanks for the review.
> Well actually your are correct that current temperature and last
> temperature can be used to increase or decrease the cpu frequency. But
> this has to be done again in cooling devices so to make the cooling
> devices generic and to avoid the temperature comparison again this new
> trip type passes the cooling device instance id.
> Also about your queries that this may add dependency between trip
> index and cooling state. This is actually needed and this dependency
> is created when the cooling device is binded with trip points(For
> cpufreq type cooling device just the instance of cooling device is
> associated with trip points). More over the existing PASSIVE cooling
> trip type does the same thing and iterates across all the cooling
> state.
>
>  Thanks,
>  Amit Daniel
>>
>> On 20 December 2011 18:07, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>>>
>>> Hi Amit,
>>>
>>> I'm not sure that using the trip index for setting the state of a
>>> cooling device is a generic solution because you are adding a
>>> dependency between the trip index and the cooling device state that
>>> might be difficult to handle. This dependency implies that a cooling
>>> device like cpufreq_cooling_device must be registered in the 1st trips
>>> of a thermal_zone which is not possible when we want to register 2
>>> cpufreq_cooling_devices in the same thermal_zone.
>>> You should only rely on the current and last temperatures to detect if
>>> a trip_temp has been crossed and you should increase or decrease the
>>> current state of the cooling device accordingly.
>>>
>>> something like below should work with cpufreq_cooling_device and will
>>> not add any constraint on the trip index. The state of a cooling
>>> device is increased/decreased once for each trip
>>>
>>> +               case THERMAL_TRIP_STATE_ACTIVE:
>>> +                       list_for_each_entry(instance, &tz->cooling_devices,
>>> +                                           node) {
>>> +                               if (instance->trip != count)
>>> +                                       continue;
>>> +
>>> +                               cdev = instance->cdev;
>>> +
>>> +                               if ((temp >= trip_temp)
>>> +                                       && (trip_temp > tz->last_temperature)) {
>>> +                                       cdev->ops->get_max_state(cdev,
>>> +                                                       &max_state);
>>> +                                       cdev->ops->get_cur_state(cdev,
>>> +                                                       &current_state);
>>> +                                       if (++current_state <= max_state)
>>> +                                               cdev->ops->set_cur_state(cdev,
>>> +                                                               current_state);
>>> +                               }
>>> +                               else if ((temp < trip_temp)
>>> +                                       && (trip_temp <= tz->last_temperature)) {
>>> +                                       cdev->ops->get_cur_state(cdev,
>>> +                                                       &current_state);
>>> +                                       if (current_state > 0)
>>> +                                               cdev->ops->set_cur_state(cdev,
>>> +                                                               --current_state);
>>> +                       }
>>> +                       break;
>>>
>>> Regards,
>>> Vincent
>>>
>>> On 13 December 2011 16:13, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
>>> > 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       |   27 ++++++++++++++++++++++++++-
>>> >  include/linux/thermal.h             |    1 +
>>> >  3 files changed, 29 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>>> > index b61e46f..5c1d44e 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 dd9a574..72b1ab3 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");
>>> >        }
>>> > @@ -1035,7 +1037,7 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
>>> >  void thermal_zone_device_update(struct thermal_zone_device *tz)
>>> >  {
>>> >        int count, ret = 0;
>>> > -       long temp, trip_temp;
>>> > +       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 *cdev;
>>> > @@ -1086,6 +1088,29 @@ 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;
>>> > +
>>> > +                               cdev = instance->cdev;
>>> > +                               cdev->ops->get_max_state(cdev, &max_state);
>>> > +
>>> > +                               if ((temp >= trip_temp) &&
>>> > +                                               ((count + 1) <= max_state))
>>> > +                                       cdev->ops->set_cur_state(cdev,
>>> > +                                                               count + 1);
>>> > +                               else if ((temp < trip_temp) &&
>>> > +                                                       (count <= max_state))
>>> > +                                       cdev->ops->set_cur_state(cdev, count);
>>> > +
>>> > +                               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 47b4a27..d7d0a27 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
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Hi Rob,

I got your point. The main idea of doing like this is to keep the
cooling implementation independent from thermal zone
algorithms(thermal_sys.c). But binding freq_tab index to the trip
numbers may be not be bad idea. I will give more thought into it in
the next patchset.

Regards,
Amit D

On 11 January 2012 13:42, Rob Lee <rob.lee@linaro.org> wrote:
> Hey Amit/Vincent,
>
> It appears that with this implementation the STATE_ACTIVE trip number
> used will also be the index of the cool_freq_tab used.  If that is
> true, then perhaps a common structure would be beneficial that links
> each STATE_ACTIVE trip point with its corresponding cooling data.
>
> BR,
> Rob
>
> On Tue, Dec 20, 2011 at 11:11 PM, Amit Kachhap <amit.kachhap@linaro.org> wrote:
>>  Hi Vincent,
>>
>> Thanks for the review.
>> Well actually your are correct that current temperature and last
>> temperature can be used to increase or decrease the cpu frequency. But
>> this has to be done again in cooling devices so to make the cooling
>> devices generic and to avoid the temperature comparison again this new
>> trip type passes the cooling device instance id.
>> Also about your queries that this may add dependency between trip
>> index and cooling state. This is actually needed and this dependency
>> is created when the cooling device is binded with trip points(For
>> cpufreq type cooling device just the instance of cooling device is
>> associated with trip points). More over the existing PASSIVE cooling
>> trip type does the same thing and iterates across all the cooling
>> state.
>>
>>  Thanks,
>>  Amit Daniel
>>>
>>> On 20 December 2011 18:07, Vincent Guittot <vincent.guittot@linaro.org> wrote:
>>>>
>>>> Hi Amit,
>>>>
>>>> I'm not sure that using the trip index for setting the state of a
>>>> cooling device is a generic solution because you are adding a
>>>> dependency between the trip index and the cooling device state that
>>>> might be difficult to handle. This dependency implies that a cooling
>>>> device like cpufreq_cooling_device must be registered in the 1st trips
>>>> of a thermal_zone which is not possible when we want to register 2
>>>> cpufreq_cooling_devices in the same thermal_zone.
>>>> You should only rely on the current and last temperatures to detect if
>>>> a trip_temp has been crossed and you should increase or decrease the
>>>> current state of the cooling device accordingly.
>>>>
>>>> something like below should work with cpufreq_cooling_device and will
>>>> not add any constraint on the trip index. The state of a cooling
>>>> device is increased/decreased once for each trip
>>>>
>>>> +               case THERMAL_TRIP_STATE_ACTIVE:
>>>> +                       list_for_each_entry(instance, &tz->cooling_devices,
>>>> +                                           node) {
>>>> +                               if (instance->trip != count)
>>>> +                                       continue;
>>>> +
>>>> +                               cdev = instance->cdev;
>>>> +
>>>> +                               if ((temp >= trip_temp)
>>>> +                                       && (trip_temp > tz->last_temperature)) {
>>>> +                                       cdev->ops->get_max_state(cdev,
>>>> +                                                       &max_state);
>>>> +                                       cdev->ops->get_cur_state(cdev,
>>>> +                                                       &current_state);
>>>> +                                       if (++current_state <= max_state)
>>>> +                                               cdev->ops->set_cur_state(cdev,
>>>> +                                                               current_state);
>>>> +                               }
>>>> +                               else if ((temp < trip_temp)
>>>> +                                       && (trip_temp <= tz->last_temperature)) {
>>>> +                                       cdev->ops->get_cur_state(cdev,
>>>> +                                                       &current_state);
>>>> +                                       if (current_state > 0)
>>>> +                                               cdev->ops->set_cur_state(cdev,
>>>> +                                                               --current_state);
>>>> +                       }
>>>> +                       break;
>>>>
>>>> Regards,
>>>> Vincent
>>>>
>>>> On 13 December 2011 16:13, Amit Daniel Kachhap <amit.kachhap@linaro.org> wrote:
>>>> > 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       |   27 ++++++++++++++++++++++++++-
>>>> >  include/linux/thermal.h             |    1 +
>>>> >  3 files changed, 29 insertions(+), 3 deletions(-)
>>>> >
>>>> > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
>>>> > index b61e46f..5c1d44e 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 dd9a574..72b1ab3 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");
>>>> >        }
>>>> > @@ -1035,7 +1037,7 @@ EXPORT_SYMBOL(thermal_cooling_device_unregister);
>>>> >  void thermal_zone_device_update(struct thermal_zone_device *tz)
>>>> >  {
>>>> >        int count, ret = 0;
>>>> > -       long temp, trip_temp;
>>>> > +       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 *cdev;
>>>> > @@ -1086,6 +1088,29 @@ 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;
>>>> > +
>>>> > +                               cdev = instance->cdev;
>>>> > +                               cdev->ops->get_max_state(cdev, &max_state);
>>>> > +
>>>> > +                               if ((temp >= trip_temp) &&
>>>> > +                                               ((count + 1) <= max_state))
>>>> > +                                       cdev->ops->set_cur_state(cdev,
>>>> > +                                                               count + 1);
>>>> > +                               else if ((temp < trip_temp) &&
>>>> > +                                                       (count <= max_state))
>>>> > +                                       cdev->ops->set_cur_state(cdev, count);
>>>> > +
>>>> > +                               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 47b4a27..d7d0a27 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
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/