On Wed, Aug 12, 2015 at 12:13:09PM +0100, Daniel Kurtz wrote:
> Hi Javi,
Hi Daniel,
> On Tue, Aug 11, 2015 at 6:21 PM, Javi Merino <javi.mer...@arm.com> 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.zh...@intel.com>
> > Cc: Eduardo Valentin <edubez...@gmail.com>
> > Signed-off-by: Javi Merino <javi.mer...@arm.com>
> > ---
> > 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
Fixed
> > + *
> > + * 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" ?
"won't". Sometimes I'm overly cautious. You know "never say never" ;-)
> [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.
They are not necessary, but I prefer the parenthesis around the
comparison. It makes it clearer to me. I've dropped the () around
!ret.
Thanks for the comments here and in the first patch.
Javi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
