Re: [PATCH 4/5] thermal/core: Move the thermal trip code to a dedicated file

From: "Zhang, Rui" <rui.zhang@intel.com>
To: "rafael@kernel.org" <rafael@kernel.org>,
	"daniel.lezcano@linaro.org" <daniel.lezcano@linaro.org>
Cc: "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"amitk@kernel.org" <amitk@kernel.org>
Subject: Re: [PATCH 4/5] thermal/core: Move the thermal trip code to a dedicated file
Date: Thu, 19 Jan 2023 07:24:54 +0000	[thread overview]
Message-ID: <2cc2165595c10571d5480044a80e947060486192.camel@intel.com> (raw)
In-Reply-To: <20230118211123.111493-4-daniel.lezcano@linaro.org>

Hi, Daniel,

Just one minor comment below.

On Wed, 2023-01-18 at 22:11 +0100, Daniel Lezcano wrote:
> The thermal_core.c files contains a lot of functions handling
> different thermal components like the governors, the trip points, the
> cooling device, the OF cooling device, etc ...
> 
> This organization does not help to migrate to a more sane code where
> there is a better self-encapsulation as all the components' internals
> can be directly accessed from a single file.
> 
> For the sake of clarity, let's move the thermal trip points code in a
> dedicated thermal_trip.c file and add a function to browse all the
> trip points like we do with the thermal zones, the govenors and the
> cooling devices.
> 
> The same can be done for the cooling devices and the governor code
> but
> that will come later as the current work in the thermal framework is
> to fix the trip point handling and use a generic trip point
> structure.

"No functional changes intended."

thanks,
rui

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/Makefile          |   4 +-
>  drivers/thermal/thermal_core.c    |  87 --------------
>  drivers/thermal/thermal_helpers.c |  62 ----------
>  drivers/thermal/thermal_trip.c    | 181
> ++++++++++++++++++++++++++++++
>  4 files changed, 183 insertions(+), 151 deletions(-)
>  create mode 100644 drivers/thermal/thermal_trip.c
> 
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 60f0dfa9aae2..2f0db88240b5 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -4,8 +4,8 @@
>  #
>  
>  obj-$(CONFIG_THERMAL)		+= thermal_sys.o
> -thermal_sys-y			+= thermal_core.o
> thermal_sysfs.o \
> -					thermal_helpers.o
> +thermal_sys-y			+= thermal_core.o
> thermal_sysfs.o	
> +thermal_sys-y			+= thermal_trip.o
> thermal_helpers.o
>  
>  # netlink interface to manage the thermal framework
>  thermal_sys-$(CONFIG_THERMAL_NETLINK)		+=
> thermal_netlink.o
> diff --git a/drivers/thermal/thermal_core.c
> b/drivers/thermal/thermal_core.c
> index ea78c93277be..d0577685085a 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1155,12 +1155,6 @@ static void thermal_set_delay_jiffies(unsigned
> long *delay_jiffies, int delay_ms
>  		*delay_jiffies = round_jiffies(*delay_jiffies);
>  }
>  
> -int thermal_zone_get_num_trips(struct thermal_zone_device *tz)
> -{
> -	return tz->num_trips;
> -}
> -EXPORT_SYMBOL_GPL(thermal_zone_get_num_trips);
> -
>  int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int
> *temp)
>  {
>  	int i, ret = -EINVAL;
> @@ -1187,87 +1181,6 @@ int thermal_zone_get_crit_temp(struct
> thermal_zone_device *tz, int *temp)
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>  
> -int __thermal_zone_get_trip(struct thermal_zone_device *tz, int
> trip_id,
> -			    struct thermal_trip *trip)
> -{
> -	int ret;
> -
> -	if (!tz || trip_id < 0 || trip_id >= tz->num_trips || !trip)
> -		return -EINVAL;
> -
> -	if (tz->trips) {
> -		*trip = tz->trips[trip_id];
> -		return 0;
> -	}
> -
> -	if (tz->ops->get_trip_hyst) {
> -		ret = tz->ops->get_trip_hyst(tz, trip_id, &trip-
> >hysteresis);
> -		if (ret)
> -			return ret;
> -	} else {
> -		trip->hysteresis = 0;
> -	}
> -
> -	ret = tz->ops->get_trip_temp(tz, trip_id, &trip->temperature);
> -	if (ret)
> -		return ret;
> -
> -	return tz->ops->get_trip_type(tz, trip_id, &trip->type);
> -}
> -EXPORT_SYMBOL_GPL(__thermal_zone_get_trip);
> -
> -int thermal_zone_get_trip(struct thermal_zone_device *tz, int
> trip_id,
> -			  struct thermal_trip *trip)
> -{
> -	int ret;
> -
> -	mutex_lock(&tz->lock);
> -	ret = __thermal_zone_get_trip(tz, trip_id, trip);
> -	mutex_unlock(&tz->lock);
> -
> -	return ret;
> -}
> -EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
> -
> -int thermal_zone_set_trip(struct thermal_zone_device *tz, int
> trip_id,
> -			  const struct thermal_trip *trip)
> -{
> -	struct thermal_trip t;
> -	int ret;
> -
> -	if (!tz->ops->set_trip_temp && !tz->ops->set_trip_hyst && !tz-
> >trips)
> -		return -EINVAL;
> -
> -	ret = __thermal_zone_get_trip(tz, trip_id, &t);
> -	if (ret)
> -		return ret;
> -
> -	if (t.type != trip->type)
> -		return -EINVAL;
> -
> -	if (t.temperature != trip->temperature && tz->ops-
> >set_trip_temp) {
> -		ret = tz->ops->set_trip_temp(tz, trip_id, trip-
> >temperature);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	if (t.hysteresis != trip->hysteresis && tz->ops->set_trip_hyst) 
> {
> -		ret = tz->ops->set_trip_hyst(tz, trip_id, trip-
> >hysteresis);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	if (tz->trips && (t.temperature != trip->temperature ||
> t.hysteresis != trip->hysteresis))
> -		tz->trips[trip_id] = *trip;
> -
> -	thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
> -				      trip->temperature, trip-
> >hysteresis);
> -
> -	__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
> -	
> -	return 0;
> -}
> -
>  /**
>   * thermal_zone_device_register_with_trips() - register a new
> thermal zone device
>   * @type:	the thermal zone device type
> diff --git a/drivers/thermal/thermal_helpers.c
> b/drivers/thermal/thermal_helpers.c
> index 8977d5ddc23c..0f648131b0b5 100644
> --- a/drivers/thermal/thermal_helpers.c
> +++ b/drivers/thermal/thermal_helpers.c
> @@ -146,68 +146,6 @@ int thermal_zone_get_temp(struct
> thermal_zone_device *tz, int *temp)
>  }
>  EXPORT_SYMBOL_GPL(thermal_zone_get_temp);
>  
> -/**
> - * __thermal_zone_set_trips - Computes the next trip points for the
> driver
> - * @tz: a pointer to a thermal zone device structure
> - *
> - * The function computes the next temperature boundaries by browsing
> - * the trip points. The result is the closer low and high trip
> points
> - * to the current temperature. These values are passed to the
> backend
> - * driver to let it set its own notification mechanism (usually an
> - * interrupt).
> - *
> - * This function must be called with tz->lock held. Both tz and tz-
> >ops
> - * must be valid pointers.
> - *
> - * It does not return a value
> - */
> -void __thermal_zone_set_trips(struct thermal_zone_device *tz)
> -{
> -	struct thermal_trip trip;
> -	int low = -INT_MAX, high = INT_MAX;
> -	int i, ret;
> -
> -	lockdep_assert_held(&tz->lock);
> -
> -	if (!tz->ops->set_trips)
> -		return;
> -
> -	for (i = 0; i < tz->num_trips; i++) {
> -		int trip_low;
> -
> -		ret = __thermal_zone_get_trip(tz, i , &trip);
> -		if (ret)
> -			return;
> -
> -		trip_low = trip.temperature - trip.hysteresis;
> -
> -		if (trip_low < tz->temperature && trip_low > low)
> -			low = trip_low;
> -
> -		if (trip.temperature > tz->temperature &&
> -		    trip.temperature < high)
> -			high = trip.temperature;
> -	}
> -
> -	/* No need to change trip points */
> -	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> -		return;
> -
> -	tz->prev_low_trip = low;
> -	tz->prev_high_trip = high;
> -
> -	dev_dbg(&tz->device,
> -		"new temperature boundaries: %d < x < %d\n", low,
> high);
> -
> -	/*
> -	 * Set a temperature window. When this window is left the
> driver
> -	 * must inform the thermal core via thermal_zone_device_update.
> -	 */
> -	ret = tz->ops->set_trips(tz, low, high);
> -	if (ret)
> -		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
> -}
> -
>  static void thermal_cdev_set_cur_state(struct thermal_cooling_device
> *cdev,
>  				       int target)
>  {
> diff --git a/drivers/thermal/thermal_trip.c
> b/drivers/thermal/thermal_trip.c
> new file mode 100644
> index 000000000000..2ef61ff7ffc3
> --- /dev/null
> +++ b/drivers/thermal/thermal_trip.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  Copyright (C) 2008 Intel Corp
> + *  Copyright (C) 2008 Zhang Rui <rui.zhang@intel.com>
> + *  Copyright (C) 2008 Sujith Thomas <sujith.thomas@intel.com>
> + *  Copyright 2022 Linaro Limited
> + *
> + * Thermal trips handling
> + */
> +#include "thermal_core.h"
> +
> +int __for_each_thermal_trip(struct thermal_zone_device *tz,
> +			    int (*cb)(struct thermal_trip *,
> +				      int trip_id, void *),
> +			    void *data)
> +{
> +	int i, ret;
> +	struct thermal_trip trip;
> +
> +	for (i = 0; i < tz->num_trips; i++) {
> +
> +		ret = __thermal_zone_get_trip(tz, i, &trip);
> +		if (ret)
> +			return ret;
> +		
> +		ret = cb(&trip, i, data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int thermal_zone_get_num_trips(struct thermal_zone_device *tz)
> +{
> +	return tz->num_trips;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_get_num_trips);
> +
> +/**
> + * __thermal_zone_set_trips - Computes the next trip points for the
> driver
> + * @tz: a pointer to a thermal zone device structure
> + *
> + * The function computes the next temperature boundaries by browsing
> + * the trip points. The result is the closer low and high trip
> points
> + * to the current temperature. These values are passed to the
> backend
> + * driver to let it set its own notification mechanism (usually an
> + * interrupt).
> + *
> + * This function must be called with tz->lock held. Both tz and tz-
> >ops
> + * must be valid pointers.
> + *
> + * It does not return a value
> + */
> +void __thermal_zone_set_trips(struct thermal_zone_device *tz)
> +{
> +	struct thermal_trip trip;
> +	int low = -INT_MAX, high = INT_MAX;
> +	int i, ret;
> +
> +	lockdep_assert_held(&tz->lock);
> +
> +	if (!tz->ops->set_trips)
> +		return;
> +
> +	for (i = 0; i < tz->num_trips; i++) {
> +		int trip_low;
> +
> +		ret = __thermal_zone_get_trip(tz, i , &trip);
> +		if (ret)
> +			return;
> +
> +		trip_low = trip.temperature - trip.hysteresis;
> +
> +		if (trip_low < tz->temperature && trip_low > low)
> +			low = trip_low;
> +
> +		if (trip.temperature > tz->temperature &&
> +		    trip.temperature < high)
> +			high = trip.temperature;
> +	}
> +
> +	/* No need to change trip points */
> +	if (tz->prev_low_trip == low && tz->prev_high_trip == high)
> +		return;
> +
> +	tz->prev_low_trip = low;
> +	tz->prev_high_trip = high;
> +
> +	dev_dbg(&tz->device,
> +		"new temperature boundaries: %d < x < %d\n", low,
> high);
> +
> +	/*
> +	 * Set a temperature window. When this window is left the
> driver
> +	 * must inform the thermal core via thermal_zone_device_update.
> +	 */
> +	ret = tz->ops->set_trips(tz, low, high);
> +	if (ret)
> +		dev_err(&tz->device, "Failed to set trips: %d\n", ret);
> +}
> +
> +int __thermal_zone_get_trip(struct thermal_zone_device *tz, int
> trip_id,
> +			    struct thermal_trip *trip)
> +{
> +	int ret;
> +
> +	if (!tz || trip_id < 0 || trip_id >= tz->num_trips || !trip)
> +		return -EINVAL;
> +
> +	if (tz->trips) {
> +		*trip = tz->trips[trip_id];
> +		return 0;
> +	}
> +
> +	if (tz->ops->get_trip_hyst) {
> +		ret = tz->ops->get_trip_hyst(tz, trip_id, &trip-
> >hysteresis);
> +		if (ret)
> +			return ret;
> +	} else {
> +		trip->hysteresis = 0;
> +	}
> +
> +	ret = tz->ops->get_trip_temp(tz, trip_id, &trip->temperature);
> +	if (ret)
> +		return ret;
> +
> +	return tz->ops->get_trip_type(tz, trip_id, &trip->type);
> +}
> +EXPORT_SYMBOL_GPL(__thermal_zone_get_trip);
> +
> +int thermal_zone_get_trip(struct thermal_zone_device *tz, int
> trip_id,
> +			  struct thermal_trip *trip)
> +{
> +	int ret;
> +
> +	mutex_lock(&tz->lock);
> +	ret = __thermal_zone_get_trip(tz, trip_id, trip);
> +	mutex_unlock(&tz->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_get_trip);
> +
> +int thermal_zone_set_trip(struct thermal_zone_device *tz, int
> trip_id,
> +			  const struct thermal_trip *trip)
> +{
> +	struct thermal_trip t;
> +	int ret;
> +
> +	if (!tz->ops->set_trip_temp && !tz->ops->set_trip_hyst && !tz-
> >trips)
> +		return -EINVAL;
> +
> +	ret = __thermal_zone_get_trip(tz, trip_id, &t);
> +	if (ret)
> +		return ret;
> +
> +	if (t.type != trip->type)
> +		return -EINVAL;
> +
> +	if (t.temperature != trip->temperature && tz->ops-
> >set_trip_temp) {
> +		ret = tz->ops->set_trip_temp(tz, trip_id, trip-
> >temperature);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (t.hysteresis != trip->hysteresis && tz->ops->set_trip_hyst) 
> {
> +		ret = tz->ops->set_trip_hyst(tz, trip_id, trip-
> >hysteresis);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (tz->trips && (t.temperature != trip->temperature ||
> t.hysteresis != trip->hysteresis))
> +		tz->trips[trip_id] = *trip;
> +
> +	thermal_notify_tz_trip_change(tz->id, trip_id, trip->type,
> +				      trip->temperature, trip-
> >hysteresis);
> +
> +	__thermal_zone_device_update(tz, THERMAL_TRIP_CHANGED);
> +	
> +	return 0;
> +}