| Message ID | 1807510.VLH7GnMWUR@rjwysocki.net (mailing list archive) |
|---|---|
| State | In Next |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | thermal: core: Use lists of trips for trip crossing detection and handling | expand |
On 10/16/24 12:33, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Since thermal_zone_set_trip_temp() is not located in the same file nit: s/not/now > as thermal_trip_crossed(), it can invoke the latter directly without > using the thermal_zone_trip_down() wrapper that has no other users. > > Update thermal_zone_set_trip_temp() accordingly and drop > thermal_zone_trip_down(). > > No functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/thermal/thermal_core.c | 8 +------- > drivers/thermal/thermal_core.h | 2 -- > 2 files changed, 1 insertion(+), 9 deletions(-) > > Index: linux-pm/drivers/thermal/thermal_core.c > =================================================================== > --- linux-pm.orig/drivers/thermal/thermal_core.c > +++ linux-pm/drivers/thermal/thermal_core.c > @@ -565,7 +565,7 @@ void thermal_zone_set_trip_temp(struct t > * are needed to compensate for the lack of it going forward. > */ > if (tz->temperature >= td->threshold) > - thermal_zone_trip_down(tz, td); > + thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false); minor thing: won't that be too long line? IMHO we can add somewhere earlier: struct thermal_governor *gov = thermal_get_tz_governor(tz); and use it here > > /* > * Invalidate the threshold to avoid triggering a spurious > @@ -699,12 +699,6 @@ void thermal_zone_device_update(struct t > } > EXPORT_SYMBOL_GPL(thermal_zone_device_update); > > -void thermal_zone_trip_down(struct thermal_zone_device *tz, > - struct thermal_trip_desc *td) > -{ > - thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false); > -} > - > int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *), > void *data) > { > Index: linux-pm/drivers/thermal/thermal_core.h > =================================================================== > --- linux-pm.orig/drivers/thermal/thermal_core.h > +++ linux-pm/drivers/thermal/thermal_core.h > @@ -273,8 +273,6 @@ void thermal_zone_set_trips(struct therm > int thermal_zone_trip_id(const struct thermal_zone_device *tz, > const struct thermal_trip *trip); > int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp); > -void thermal_zone_trip_down(struct thermal_zone_device *tz, > - struct thermal_trip_desc *td); > void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz, > struct thermal_trip *trip, int hyst); > > > > other than that, LGTM Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
On Thu, Oct 24, 2024 at 12:32 PM Lukasz Luba <lukasz.luba@arm.com> wrote: > > > > On 10/16/24 12:33, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Since thermal_zone_set_trip_temp() is not located in the same file > > nit: s/not/now Thanks, will fix when applying the patch. > > as thermal_trip_crossed(), it can invoke the latter directly without > > using the thermal_zone_trip_down() wrapper that has no other users. > > > > Update thermal_zone_set_trip_temp() accordingly and drop > > thermal_zone_trip_down(). > > > > No functional impact. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/thermal/thermal_core.c | 8 +------- > > drivers/thermal/thermal_core.h | 2 -- > > 2 files changed, 1 insertion(+), 9 deletions(-) > > > > Index: linux-pm/drivers/thermal/thermal_core.c > > =================================================================== > > --- linux-pm.orig/drivers/thermal/thermal_core.c > > +++ linux-pm/drivers/thermal/thermal_core.c > > @@ -565,7 +565,7 @@ void thermal_zone_set_trip_temp(struct t > > * are needed to compensate for the lack of it going forward. > > */ > > if (tz->temperature >= td->threshold) > > - thermal_zone_trip_down(tz, td); > > + thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false); > > minor thing: > won't that be too long line? It is longer than 80 characters, but this is not a hard boundary - see "2) Breaking long lines and strings" in Documentation/process/coding-style.rst). Well, you can argue about the "hide information" part, but IMV this line just looks cleaner the way it is than when it would be broken in any way. > IMHO we can add somewhere earlier: > struct thermal_governor *gov = thermal_get_tz_governor(tz); > and use it here That would have been harder to follow than the current code IMO. > > > > /* > > * Invalidate the threshold to avoid triggering a spurious > > @@ -699,12 +699,6 @@ void thermal_zone_device_update(struct t > > } > > EXPORT_SYMBOL_GPL(thermal_zone_device_update); > > > > -void thermal_zone_trip_down(struct thermal_zone_device *tz, > > - struct thermal_trip_desc *td) > > -{ > > - thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false); > > -} > > - > > int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *), > > void *data) > > { > > Index: linux-pm/drivers/thermal/thermal_core.h > > =================================================================== > > --- linux-pm.orig/drivers/thermal/thermal_core.h > > +++ linux-pm/drivers/thermal/thermal_core.h > > @@ -273,8 +273,6 @@ void thermal_zone_set_trips(struct therm > > int thermal_zone_trip_id(const struct thermal_zone_device *tz, > > const struct thermal_trip *trip); > > int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp); > > -void thermal_zone_trip_down(struct thermal_zone_device *tz, > > - struct thermal_trip_desc *td); > > void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz, > > struct thermal_trip *trip, int hyst); > > > > > > > > > > other than that, LGTM > > Reviewed-by: Lukasz Luba <lukasz.luba@arm.com> Thank you!
On 10/24/24 13:33, Rafael J. Wysocki wrote: > On Thu, Oct 24, 2024 at 12:32 PM Lukasz Luba <lukasz.luba@arm.com> wrote: >> >> >> >> On 10/16/24 12:33, Rafael J. Wysocki wrote: >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> >>> Since thermal_zone_set_trip_temp() is not located in the same file >> >> nit: s/not/now > > Thanks, will fix when applying the patch. > >>> as thermal_trip_crossed(), it can invoke the latter directly without >>> using the thermal_zone_trip_down() wrapper that has no other users. >>> >>> Update thermal_zone_set_trip_temp() accordingly and drop >>> thermal_zone_trip_down(). >>> >>> No functional impact. >>> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> --- >>> drivers/thermal/thermal_core.c | 8 +------- >>> drivers/thermal/thermal_core.h | 2 -- >>> 2 files changed, 1 insertion(+), 9 deletions(-) >>> >>> Index: linux-pm/drivers/thermal/thermal_core.c >>> =================================================================== >>> --- linux-pm.orig/drivers/thermal/thermal_core.c >>> +++ linux-pm/drivers/thermal/thermal_core.c >>> @@ -565,7 +565,7 @@ void thermal_zone_set_trip_temp(struct t >>> * are needed to compensate for the lack of it going forward. >>> */ >>> if (tz->temperature >= td->threshold) >>> - thermal_zone_trip_down(tz, td); >>> + thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false); >> >> minor thing: >> won't that be too long line? > > It is longer than 80 characters, but this is not a hard boundary - see > "2) Breaking long lines and strings" in > Documentation/process/coding-style.rst). > > Well, you can argue about the "hide information" part, but IMV this > line just looks cleaner the way it is than when it would be broken in > any way. > >> IMHO we can add somewhere earlier: >> struct thermal_governor *gov = thermal_get_tz_governor(tz); >> and use it here > > That would have been harder to follow than the current code IMO. fair enough
Index: linux-pm/drivers/thermal/thermal_core.c =================================================================== --- linux-pm.orig/drivers/thermal/thermal_core.c +++ linux-pm/drivers/thermal/thermal_core.c @@ -565,7 +565,7 @@ void thermal_zone_set_trip_temp(struct t * are needed to compensate for the lack of it going forward. */ if (tz->temperature >= td->threshold) - thermal_zone_trip_down(tz, td); + thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false); /* * Invalidate the threshold to avoid triggering a spurious @@ -699,12 +699,6 @@ void thermal_zone_device_update(struct t } EXPORT_SYMBOL_GPL(thermal_zone_device_update); -void thermal_zone_trip_down(struct thermal_zone_device *tz, - struct thermal_trip_desc *td) -{ - thermal_trip_crossed(tz, td, thermal_get_tz_governor(tz), false); -} - int for_each_thermal_governor(int (*cb)(struct thermal_governor *, void *), void *data) { Index: linux-pm/drivers/thermal/thermal_core.h =================================================================== --- linux-pm.orig/drivers/thermal/thermal_core.h +++ linux-pm/drivers/thermal/thermal_core.h @@ -273,8 +273,6 @@ void thermal_zone_set_trips(struct therm int thermal_zone_trip_id(const struct thermal_zone_device *tz, const struct thermal_trip *trip); int __thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp); -void thermal_zone_trip_down(struct thermal_zone_device *tz, - struct thermal_trip_desc *td); void thermal_zone_set_trip_hyst(struct thermal_zone_device *tz, struct thermal_trip *trip, int hyst);