Re: [PATCH v3 2/3] thermal/drivers/intel: Use generic trip points for intel_pch

[Zhang, Rui]

On Wed, 2023-01-04 at 23:21 +0100, Daniel Lezcano wrote:
> From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> 
> The thermal framework gives the possibility to register the trip
> points with the thermal zone. When that is done, no get_trip_* ops
> are
> needed and they can be removed.
> 
> Convert the ops content logic into generic trip points and register
> them with the thermal zone.
> 
> In order to consolidate the code, use the ACPI thermal framework API
> to fill the generic trip point from the ACPI tables.
> 
> It has been tested on a Intel i7-8650U - x280 with the INT3400, the
> PCH, ACPITZ, and x86_pkg_temp. No regression observed so far.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> ---
>     V3:
>       - The driver Kconfig option selects CONFIG_THERMAL_ACPI
> ---
>  drivers/thermal/intel/Kconfig             |  1 +
>  drivers/thermal/intel/intel_pch_thermal.c | 88 +++++--------------
> ----
>  2 files changed, 20 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/thermal/intel/Kconfig
> b/drivers/thermal/intel/Kconfig
> index f0c845679250..738b88b290f4 100644
> --- a/drivers/thermal/intel/Kconfig
> +++ b/drivers/thermal/intel/Kconfig
> @@ -75,6 +75,7 @@ config INTEL_BXT_PMIC_THERMAL
>  config INTEL_PCH_THERMAL
>  	tristate "Intel PCH Thermal Reporting Driver"
>  	depends on X86 && PCI
> +	select THERMAL_ACPI

THERMAL_ACPI depends on ACPI but the PCH thermal driver does not.
So we will run into "unmet dependencies" issue when CONFIG_ACPI is
cleared like below

WARNING: unmet direct dependencies detected for THERMAL_ACPI
  Depends on [n]: THERMAL [=y] && ACPI [=n]
  Selected by [m]:
  - INTEL_PCH_THERMAL [=m] && THERMAL [=y] && (X86 [=y] ||
X86_INTEL_QUARK [=n] || COMPILE_TEST [=n]) && X86 [=y] && PCI [=y]

thanks,
rui
>  	help
>  	  Enable this to support thermal reporting on certain intel
> PCHs.
>  	  Thermal reporting device will provide temperature reading,
> diff --git a/drivers/thermal/intel/intel_pch_thermal.c
> b/drivers/thermal/intel/intel_pch_thermal.c
> index dabf11a687a1..530fe9b38381 100644
> --- a/drivers/thermal/intel/intel_pch_thermal.c
> +++ b/drivers/thermal/intel/intel_pch_thermal.c
> @@ -65,6 +65,8 @@
>  #define WPT_TEMP_OFFSET	(PCH_TEMP_OFFSET *
> MILLIDEGREE_PER_DEGREE)
>  #define GET_PCH_TEMP(x)	(((x) / 2) + PCH_TEMP_OFFSET)
>  
> +#define PCH_MAX_TRIPS 3 /* critical, hot, passive */
> +
>  /* Amount of time for each cooling delay, 100ms by default for now
> */
>  static unsigned int delay_timeout = 100;
>  module_param(delay_timeout, int, 0644);
> @@ -82,12 +84,7 @@ struct pch_thermal_device {
>  	const struct pch_dev_ops *ops;
>  	struct pci_dev *pdev;
>  	struct thermal_zone_device *tzd;
> -	int crt_trip_id;
> -	unsigned long crt_temp;
> -	int hot_trip_id;
> -	unsigned long hot_temp;
> -	int psv_trip_id;
> -	unsigned long psv_temp;
> +	struct thermal_trip trips[PCH_MAX_TRIPS];
>  	bool bios_enabled;
>  };
>  
> @@ -102,33 +99,22 @@ static void pch_wpt_add_acpi_psv_trip(struct
> pch_thermal_device *ptd,
>  				      int *nr_trips)
>  {
>  	struct acpi_device *adev;
> -
> -	ptd->psv_trip_id = -1;
> +	int ret;
>  
>  	adev = ACPI_COMPANION(&ptd->pdev->dev);
> -	if (adev) {
> -		unsigned long long r;
> -		acpi_status status;
> -
> -		status = acpi_evaluate_integer(adev->handle, "_PSV",
> NULL,
> -					       &r);
> -		if (ACPI_SUCCESS(status)) {
> -			unsigned long trip_temp;
> -
> -			trip_temp = deci_kelvin_to_millicelsius(r);
> -			if (trip_temp) {
> -				ptd->psv_temp = trip_temp;
> -				ptd->psv_trip_id = *nr_trips;
> -				++(*nr_trips);
> -			}
> -		}
> -	}
> +	if (!adev)
> +		return;
> +		
> +	ret = thermal_acpi_trip_psv(adev, &ptd->trips[*nr_trips]);
> +	if (ret)
> +		return;
> +
> +	++(*nr_trips);
>  }
>  #else
>  static void pch_wpt_add_acpi_psv_trip(struct pch_thermal_device
> *ptd,
>  				      int *nr_trips)
>  {
> -	ptd->psv_trip_id = -1;
>  
>  }
>  #endif
> @@ -163,21 +149,19 @@ static int pch_wpt_init(struct
> pch_thermal_device *ptd, int *nr_trips)
>  	}
>  
>  read_trips:
> -	ptd->crt_trip_id = -1;
>  	trip_temp = readw(ptd->hw_base + WPT_CTT);
>  	trip_temp &= 0x1FF;
>  	if (trip_temp) {
> -		ptd->crt_temp = GET_WPT_TEMP(trip_temp);
> -		ptd->crt_trip_id = 0;
> +		ptd->trips[*nr_trips].temperature =
> GET_WPT_TEMP(trip_temp);
> +		ptd->trips[*nr_trips].type = THERMAL_TRIP_CRITICAL;
>  		++(*nr_trips);
>  	}
>  
> -	ptd->hot_trip_id = -1;
>  	trip_temp = readw(ptd->hw_base + WPT_PHL);
>  	trip_temp &= 0x1FF;
>  	if (trip_temp) {
> -		ptd->hot_temp = GET_WPT_TEMP(trip_temp);
> -		ptd->hot_trip_id = *nr_trips;
> +		ptd->trips[*nr_trips].temperature =
> GET_WPT_TEMP(trip_temp);
> +		ptd->trips[*nr_trips].type = THERMAL_TRIP_HOT;
>  		++(*nr_trips);
>  	}
>  
> @@ -298,39 +282,6 @@ static int pch_thermal_get_temp(struct
> thermal_zone_device *tzd, int *temp)
>  	return	ptd->ops->get_temp(ptd, temp);
>  }
>  
> -static int pch_get_trip_type(struct thermal_zone_device *tzd, int
> trip,
> -			     enum thermal_trip_type *type)
> -{
> -	struct pch_thermal_device *ptd = tzd->devdata;
> -
> -	if (ptd->crt_trip_id == trip)
> -		*type = THERMAL_TRIP_CRITICAL;
> -	else if (ptd->hot_trip_id == trip)
> -		*type = THERMAL_TRIP_HOT;
> -	else if (ptd->psv_trip_id == trip)
> -		*type = THERMAL_TRIP_PASSIVE;
> -	else
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
> -static int pch_get_trip_temp(struct thermal_zone_device *tzd, int
> trip, int *temp)
> -{
> -	struct pch_thermal_device *ptd = tzd->devdata;
> -
> -	if (ptd->crt_trip_id == trip)
> -		*temp = ptd->crt_temp;
> -	else if (ptd->hot_trip_id == trip)
> -		*temp = ptd->hot_temp;
> -	else if (ptd->psv_trip_id == trip)
> -		*temp = ptd->psv_temp;
> -	else
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  static void pch_critical(struct thermal_zone_device *tzd)
>  {
>  	dev_dbg(&tzd->device, "%s: critical temperature reached\n",
> tzd->type);
> @@ -338,8 +289,6 @@ static void pch_critical(struct
> thermal_zone_device *tzd)
>  
>  static struct thermal_zone_device_ops tzd_ops = {
>  	.get_temp = pch_thermal_get_temp,
> -	.get_trip_type = pch_get_trip_type,
> -	.get_trip_temp = pch_get_trip_temp,
>  	.critical = pch_critical,
>  };
>  
> @@ -423,8 +372,9 @@ static int intel_pch_thermal_probe(struct pci_dev
> *pdev,
>  	if (err)
>  		goto error_cleanup;
>  
> -	ptd->tzd = thermal_zone_device_register(bi->name, nr_trips, 0,
> ptd,
> -						&tzd_ops, NULL, 0, 0);
> +	ptd->tzd = thermal_zone_device_register_with_trips(bi->name,
> ptd->trips,
> +							   nr_trips, 0,
> ptd,
> +							   &tzd_ops,
> NULL, 0, 0);
>  	if (IS_ERR(ptd->tzd)) {
>  		dev_err(&pdev->dev, "Failed to register thermal zone
> %s\n",
>  			bi->name);