Re: [PATCH v2] Fix invalid fan speed on ThinkPad X120e

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4060 bytes --]

On Mon, 3 Feb 2025, Sybil Isabel Dorsett wrote:

> On ThinkPad X120e, fan speed is incorrectly reported in ticks

I'd drop "incorrectly".

> per revolution rather than RPM.
> 
> Recaculate the fan speed value reported for ThinkPad X120e

Recalculate

With those two changed, feel free to add:

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

--
 i.

> to RPM based on a 22.5 kHz clock.
> 
> Based on the information on
> https://www.thinkwiki.org/wiki/How_to_control_fan_speed,
> the same problem is highly likely to be relevant to at least Edge11,
> but Edge11 is not addressed in this patch.
> 
> Signed-off-by: Sybil Isabel Dorsett <sybdorsett@proton.me>
> ---
> 
> Notes:
>     v2: addressed review comments
>         - name of the new variable is changed
>           from "fan_speed_requires_conversion" to "fan_speed_in_tpr";
>         - rephrased the commit message;
>         - "KHz" is respelled as "kHz".
> 
>  drivers/platform/x86/thinkpad_acpi.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 1fcb0f996..e7778ea41 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -7885,6 +7885,7 @@ static struct ibm_struct volume_driver_data = {
>  
>  #define FAN_NS_CTRL_STATUS	BIT(2)		/* Bit which determines control is enabled or not */
>  #define FAN_NS_CTRL		BIT(4)		/* Bit which determines control is by host or EC */
> +#define FAN_CLOCK_TPM		(22500*60)	/* Ticks per minute for a 22.5 kHz clock */
>  
>  enum {					/* Fan control constants */
>  	fan_status_offset = 0x2f,	/* EC register 0x2f */
> @@ -7940,6 +7941,7 @@ static int fan_watchdog_maxinterval;
>  
>  static bool fan_with_ns_addr;
>  static bool ecfw_with_fan_dec_rpm;
> +static bool fan_speed_in_tpr;
>  
>  static struct mutex fan_mutex;
>  
> @@ -8142,8 +8144,11 @@ static int fan_get_speed(unsigned int *speed)
>  			     !acpi_ec_read(fan_rpm_offset + 1, &hi)))
>  			return -EIO;
>  
> -		if (likely(speed))
> +		if (likely(speed)) {
>  			*speed = (hi << 8) | lo;
> +			if (fan_speed_in_tpr && *speed != 0)
> +				*speed = FAN_CLOCK_TPM / *speed;
> +		}
>  		break;
>  	case TPACPI_FAN_RD_TPEC_NS:
>  		if (!acpi_ec_read(fan_rpm_status_ns, &lo))
> @@ -8176,8 +8181,11 @@ static int fan2_get_speed(unsigned int *speed)
>  		if (rc)
>  			return -EIO;
>  
> -		if (likely(speed))
> +		if (likely(speed)) {
>  			*speed = (hi << 8) | lo;
> +			if (fan_speed_in_tpr && *speed != 0)
> +				*speed = FAN_CLOCK_TPM / *speed;
> +		}
>  		break;
>  
>  	case TPACPI_FAN_RD_TPEC_NS:
> @@ -8788,6 +8796,7 @@ static const struct attribute_group fan_driver_attr_group = {
>  #define TPACPI_FAN_NOFAN	0x0008		/* no fan available */
>  #define TPACPI_FAN_NS		0x0010		/* For EC with non-Standard register addresses */
>  #define TPACPI_FAN_DECRPM	0x0020		/* For ECFW's with RPM in register as decimal */
> +#define TPACPI_FAN_TPR		0x0040		/* Fan speed is in Ticks Per Revolution */
>  
>  static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
>  	TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1),
> @@ -8817,6 +8826,7 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
>  	TPACPI_Q_LNV3('R', '0', 'V', TPACPI_FAN_NS),	/* 11e Gen5 KL-Y */
>  	TPACPI_Q_LNV3('N', '1', 'O', TPACPI_FAN_NOFAN),	/* X1 Tablet (2nd gen) */
>  	TPACPI_Q_LNV3('R', '0', 'Q', TPACPI_FAN_DECRPM),/* L480 */
> +	TPACPI_Q_LNV('8', 'F', TPACPI_FAN_TPR),		/* ThinkPad x120e */
>  };
>  
>  static int __init fan_init(struct ibm_init_struct *iibm)
> @@ -8887,6 +8897,8 @@ static int __init fan_init(struct ibm_init_struct *iibm)
>  
>  			if (quirks & TPACPI_FAN_Q1)
>  				fan_quirk1_setup();
> +			if (quirks & TPACPI_FAN_TPR)
> +				fan_speed_in_tpr = true;
>  			/* Try and probe the 2nd fan */
>  			tp_features.second_fan = 1; /* needed for get_speed to work */
>  			res = fan2_get_speed(&speed);
>