Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <67bf0109-0606-a44c-7a58-606101bdb52d@redhat.com>
Date:   Mon, 8 Aug 2022 16:56:24 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH v1 23/26] thermal/drivers/acerhdf: Use generic
 thermal_zone_get_trip() function
Content-Language: en-US
To:     Daniel Lezcano <daniel.lezcano@linaro.org>, rafael@kernel.org
Cc:     rui.zhang@intel.com, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Peter Kaestle <peter@piie.net>,
        Mark Gross <markgross@kernel.org>,
        "open list:ACER ASPIRE ONE TEMPERATURE AND FAN DRIVER" 
        <platform-driver-x86@vger.kernel.org>
References: <20220805145729.2491611-1-daniel.lezcano@linaro.org>
 <20220805145729.2491611-24-daniel.lezcano@linaro.org>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20220805145729.2491611-24-daniel.lezcano@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 8/5/22 16:57, Daniel Lezcano wrote:
> 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 ops content logic into generic trip points and register them with the
> thermal zone.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Thanks, patch looks good to me, feel free to merge this
through the thermal tree together with the rest of the series:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  drivers/platform/x86/acerhdf.c | 73 ++++++++++++----------------------
>  1 file changed, 26 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index 3463629f8764..cf757f3a1e6b 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -46,6 +46,8 @@
>   * measured by the on-die thermal monitor are within 0 <= Tj <= 90. So,
>   * assume 89°C is critical temperature.
>   */
> +#define ACERHDF_DEFAULT_TEMP_FANON 60000
> +#define ACERHDF_DEFAULT_TEMP_FANOFF 53000
>  #define ACERHDF_TEMP_CRIT 89000
>  #define ACERHDF_FAN_OFF 0
>  #define ACERHDF_FAN_AUTO 1
> @@ -70,8 +72,8 @@ static int kernelmode;
>  #endif
>  
>  static unsigned int interval = 10;
> -static unsigned int fanon = 60000;
> -static unsigned int fanoff = 53000;
> +static unsigned int fanon = ACERHDF_DEFAULT_TEMP_FANON;
> +static unsigned int fanoff = ACERHDF_DEFAULT_TEMP_FANOFF;
>  static unsigned int verbose;
>  static unsigned int list_supported;
>  static unsigned int fanstate = ACERHDF_FAN_AUTO;
> @@ -137,6 +139,15 @@ struct ctrl_settings {
>  	int mcmd_enable;
>  };
>  
> +static struct thermal_trip trips[] = {
> +	[0] = { .temperature = ACERHDF_DEFAULT_TEMP_FANON,
> +		.hysteresis = ACERHDF_DEFAULT_TEMP_FANON - ACERHDF_DEFAULT_TEMP_FANOFF,
> +		.type = THERMAL_TRIP_ACTIVE },
> +
> +	[1] = { .temperature = ACERHDF_TEMP_CRIT,
> +		.type = THERMAL_TRIP_CRITICAL }
> +};
> +
>  static struct ctrl_settings ctrl_cfg __read_mostly;
>  
>  /* Register addresses and values for different BIOS versions */
> @@ -326,6 +337,15 @@ static void acerhdf_check_param(struct thermal_zone_device *thermal)
>  		fanon = ACERHDF_MAX_FANON;
>  	}
>  
> +	if (fanon < fanoff) {
> +		pr_err("fanoff temperature (%d) is above fanon temperature (%d), clamping to %d\n",
> +		       fanoff, fanon, fanon);
> +		fanoff = fanon;
> +	};
> +	
> +	trips[0].temperature = fanon;
> +	trips[0].hysteresis  = fanon - fanoff;
> +	
>  	if (kernelmode && prev_interval != interval) {
>  		if (interval > ACERHDF_MAX_INTERVAL) {
>  			pr_err("interval too high, set to %d\n",
> @@ -424,43 +444,6 @@ static int acerhdf_change_mode(struct thermal_zone_device *thermal,
>  	return 0;
>  }
>  
> -static int acerhdf_get_trip_type(struct thermal_zone_device *thermal, int trip,
> -				 enum thermal_trip_type *type)
> -{
> -	if (trip == 0)
> -		*type = THERMAL_TRIP_ACTIVE;
> -	else if (trip == 1)
> -		*type = THERMAL_TRIP_CRITICAL;
> -	else
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
> -static int acerhdf_get_trip_hyst(struct thermal_zone_device *thermal, int trip,
> -				 int *temp)
> -{
> -	if (trip != 0)
> -		return -EINVAL;
> -
> -	*temp = fanon - fanoff;
> -
> -	return 0;
> -}
> -
> -static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int trip,
> -				 int *temp)
> -{
> -	if (trip == 0)
> -		*temp = fanon;
> -	else if (trip == 1)
> -		*temp = ACERHDF_TEMP_CRIT;
> -	else
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  static int acerhdf_get_crit_temp(struct thermal_zone_device *thermal,
>  				 int *temperature)
>  {
> @@ -474,13 +457,9 @@ static struct thermal_zone_device_ops acerhdf_dev_ops = {
>  	.unbind = acerhdf_unbind,
>  	.get_temp = acerhdf_get_ec_temp,
>  	.change_mode = acerhdf_change_mode,
> -	.get_trip_type = acerhdf_get_trip_type,
> -	.get_trip_hyst = acerhdf_get_trip_hyst,
> -	.get_trip_temp = acerhdf_get_trip_temp,
>  	.get_crit_temp = acerhdf_get_crit_temp,
>  };
>  
> -
>  /*
>   * cooling device callback functions
>   * get maximal fan cooling state
> @@ -710,10 +689,10 @@ static int __init acerhdf_register_thermal(void)
>  	if (IS_ERR(cl_dev))
>  		return -EINVAL;
>  
> -	thz_dev = thermal_zone_device_register("acerhdf", 2, 0, NULL,
> -					      &acerhdf_dev_ops,
> -					      &acerhdf_zone_params, 0,
> -					      (kernelmode) ? interval*1000 : 0);
> +	thz_dev = thermal_zone_device_register_with_trips("acerhdf", trips, ARRAY_SIZE(trips),
> +							  0, NULL, &acerhdf_dev_ops,
> +							  &acerhdf_zone_params, 0,
> +							  (kernelmode) ? interval*1000 : 0);
>  	if (IS_ERR(thz_dev))
>  		return -EINVAL;
>