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

[Ilpo Järvinen]

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@xxxxxxxxxxxxxxxxxx>
> ---

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.