linux-kernel - Re: [PATCH v2] platform/x86: thinkpad_acpi: Add Thinkpad Edge E531 fan support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e4e32316-509c-28c6-fd18-38ed17e9e35a@linux.intel.com>
Date: Wed, 14 Aug 2024 13:47:56 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Matthias Fetzer <kontakt@...thias-fetzer.de>
cc: hmh@....eng.br, Hans de Goede <hdegoede@...hat.com>, 
    ibm-acpi-devel@...ts.sourceforge.net, platform-driver-x86@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] platform/x86: thinkpad_acpi: Add Thinkpad Edge E531
 fan support

On Tue, 13 Aug 2024, Matthias Fetzer wrote:

> Fan control on the E531 is done using the ACPI methods FANG and
> FANW. The correct parameters and register values were found by
> analyzing EC firmware as well as DSDT. This has been tested on
> my Thinkpad Edge E531 (6885CTO, BIOS HEET52WW 1.33).
> 
> Signed-off-by: Matthias Fetzer <kontakt@...thias-fetzer.de>
> ---

In general, you should have the patch history/changelog here below --- 
line (what you changed with different versions of the patch so reviewers 
don't have to guess).

> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 397b409064c9..31e5de21753f 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c

> @@ -8157,6 +8213,7 @@ static int fan2_get_speed(unsigned int *speed)
>  
>  static int fan_set_level(int level)
>  {
> +	int rc;

Please, add newline here as required by the normal coding style.

>  	if (!fan_control_allowed)
>  		return -EPERM;
>  

> @@ -8915,6 +9061,20 @@ static int fan_read(struct seq_file *m)
>  			       str_enabled_disabled(status), status);
>  		break;
>  
> +	case TPACPI_FAN_RD_ACPI_FANG:
> +		/* E531 */
> +		rc = fan_get_status_safe(&status);
> +		if (rc)
> +			return rc;
> +
> +		seq_printf(m, "status:\t\t%s\n", str_enabled_disabled(status));
> +
> +		rc = fan_get_speed(&speed);
> +		if (rc < 0)
> +			return rc;
> +		seq_printf(m, "speed:\t\t%d\n", speed);

Hmm, first of all, this should use %u because speed is unsigned int. But 
to find that out, I looked into that function and realized this is 100% 
duplicate of the first part of the case below it, no?

...And that case code block already has additional if for 
fan_status_access_mode specific handling so why not just change the else 
-> else if (fan_status_access_mode == TPACPI_FAN_RD_TPEC) instead of
adding all this code duplication?

(I know that other case block uses incorrectly %d when printing speed 
which you can fix if you want but please make another patch out of it,
don't mix it with this hw support add patch).

> +		break;
> +
>  	case TPACPI_FAN_RD_TPEC_NS:
>  	case TPACPI_FAN_RD_TPEC:
>  		/* all except 570, 600e/x, 770e, 770x */
> 

-- 
 i.