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.
