Re: [PATCH v2 2/4] thermal: power_allocator: relax the requirement of two passive trip points

[Daniel Kurtz]

Hi Javi,

tiny nits again...

On Tue, Aug 11, 2015 at 6:21 PM, Javi Merino <javi.merino@xxxxxxx> wrote:
> The power allocator governor currently requires that the thermal zone
> has at least two passive trip points.  If there aren't, the governor
> refuses to bind to the thermal zone.
>
> This commit relaxes that requirement.  Now the governor will bind to all
> thermal zones regardless of how many trip points they have.
>
> Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> Cc: Eduardo Valentin <edubezval@xxxxxxxxx>
> Signed-off-by: Javi Merino <javi.merino@xxxxxxx>
> ---
>  Documentation/thermal/power_allocator.txt |  2 +-
>  drivers/thermal/power_allocator.c         | 91 +++++++++++++++++--------------
>  2 files changed, 52 insertions(+), 41 deletions(-)
>
> diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt
> index c3797b529991..a1ce2235f121 100644
> --- a/Documentation/thermal/power_allocator.txt
> +++ b/Documentation/thermal/power_allocator.txt
> @@ -4,7 +4,7 @@ Power allocator governor tunables
>  Trip points
>  -----------
>
> -The governor requires the following two passive trip points:
> +The governor works optimally with the following two passive trip points:
>
>  1.  "switch on" trip point: temperature above which the governor
>      control loop starts operating.  This is the first passive trip
> diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
> index f78836c2da26..257c9af20f22 100644
> --- a/drivers/thermal/power_allocator.c
> +++ b/drivers/thermal/power_allocator.c
> @@ -24,6 +24,8 @@
>
>  #include "thermal_core.h"
>
> +#define INVALID_TRIP -1
> +
>  #define FRAC_BITS 10
>  #define int_to_frac(x) ((x) << FRAC_BITS)
>  #define frac_to_int(x) ((x) >> FRAC_BITS)
> @@ -61,6 +63,8 @@ static inline s64 div_frac(s64 x, s64 y)
>   *             Used to calculate the derivative term.
>   * @trip_switch_on:    first passive trip point of the thermal zone.  The
>   *                     governor switches on when this trip point is crossed.
> + *                     If the thermal zone only has one passive trip point,
> + *                     @trip_switch_on should be INVALID_TRIP.
>   * @trip_max_desired_temperature:      last passive trip point of the thermal
>   *                                     zone.  The temperature we are
>   *                                     controlling for.
> @@ -378,43 +382,66 @@ unlock:
>         return ret;
>  }
>
> -static int get_governor_trips(struct thermal_zone_device *tz,
> -                             struct power_allocator_params *params)
> +/**
> + * get_governor_trips() - get the number of the two trip points that are key for this governor
> + * @tz:        thermal zone to operate on
> + * @params:    pointer the private data for this governor
                  ^
                  pointer to private data

> + *
> + * The power allocator governor works optimally with two trips points:
> + * a "switch on" trip point and a "maximum desired temperature".  These
> + * are defined as the first and last passive trip points.
> + *
> + * If there is only one trip point, then that's considered to be the
> + * "maximum desired temperature" trip point and the governor is always
> + * on.  If there are no passive or active trip points, then the
> + * governor won't do anything.  In fact, its throttle function
> + * shouldn't be called at all.
      ^
      "shouldn't" or "won't" ?

[snip]

>  static void power_allocator_unbind(struct thermal_zone_device *tz)
> @@ -544,13 +561,7 @@ static int power_allocator_throttle(struct thermal_zone_device *tz, int trip)
>
>         ret = tz->ops->get_trip_temp(tz, params->trip_switch_on,
>                                      &switch_on_temp);
> -       if (ret) {
> -               dev_warn(&tz->device,
> -                        "Failed to get switch on temperature: %d\n", ret);
> -               return ret;
> -       }
> -
> -       if (current_temp < switch_on_temp) {
> +       if ((!ret) && (current_temp < switch_on_temp)) {

nit: I think the inner pair of () are not necessary.

Thanks,
-Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/