The Thinkpad P50 has 2 fans. Add the 2FAN quirk so the tpacpi driver
properly reports both fan speeds.
Because the P50 doesn't report the version of its EC controller, we need to
identify it by BIOS version (N1).
Signed-off-by: Alexander Kappner <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index d5eaf3b1..ebdc300 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8699,16 +8699,24 @@ static const struct attribute_group fan_attr_group = {
.ec = TPID(__id1, __id2), \
.quirks = __quirks }
+#define TPACPI_FAN_QL_BIOS(__id1, __id2, __quirks) \
+ { .vendor = PCI_VENDOR_ID_LENOVO, \
+ .bios = TPID(__id1, __id2), \
+ .ec = TPACPI_MATCH_ANY, \
+ .quirks = __quirks }
+
static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
TPACPI_FAN_QI('1', 'Y', TPACPI_FAN_Q1),
TPACPI_FAN_QI('7', '8', TPACPI_FAN_Q1),
TPACPI_FAN_QI('7', '6', TPACPI_FAN_Q1),
TPACPI_FAN_QI('7', '0', TPACPI_FAN_Q1),
TPACPI_FAN_QL('7', 'M', TPACPI_FAN_2FAN),
+ TPACPI_FAN_QL_BIOS('N', '1', TPACPI_FAN_2FAN) // Thinkpad P50 has 2 fans
};
#undef TPACPI_FAN_QL
#undef TPACPI_FAN_QI
+#undef TPACPI_FAN_QL_BIOS
static int __init fan_init(struct ibm_init_struct *iibm)
{
--
2.1.4
On Mon, 02 Apr 2018, Alexander Kappner wrote:
> The Thinkpad P50 has 2 fans. Add the 2FAN quirk so the tpacpi driver
> properly reports both fan speeds.
> Because the P50 doesn't report the version of its EC controller, we need to
> identify it by BIOS version (N1).
>
> Signed-off-by: Alexander Kappner <[email protected]>
I assume you tested this on real hardware? If so:
Acked-by: Henrique de Moraes Holschuh <[email protected]>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d5eaf3b1..ebdc300 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8699,16 +8699,24 @@ static const struct attribute_group fan_attr_group = {
> .ec = TPID(__id1, __id2), \
> .quirks = __quirks }
>
> +#define TPACPI_FAN_QL_BIOS(__id1, __id2, __quirks) \
> + { .vendor = PCI_VENDOR_ID_LENOVO, \
> + .bios = TPID(__id1, __id2), \
> + .ec = TPACPI_MATCH_ANY, \
> + .quirks = __quirks }
> +
> static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
> TPACPI_FAN_QI('1', 'Y', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '8', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '6', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '0', TPACPI_FAN_Q1),
> TPACPI_FAN_QL('7', 'M', TPACPI_FAN_2FAN),
> + TPACPI_FAN_QL_BIOS('N', '1', TPACPI_FAN_2FAN) // Thinkpad P50 has 2 fans
> };
>
> #undef TPACPI_FAN_QL
> #undef TPACPI_FAN_QI
> +#undef TPACPI_FAN_QL_BIOS
>
> static int __init fan_init(struct ibm_init_struct *iibm)
> {
--
Henrique Holschuh
On Mon, Apr 2, 2018 at 9:27 PM, Alexander Kappner <[email protected]> wrote:
> The Thinkpad P50 has 2 fans. Add the 2FAN quirk so the tpacpi driver
> properly reports both fan speeds.
> Because the P50 doesn't report the version of its EC controller, we need to
> identify it by BIOS version (N1).
>
> Signed-off-by: Alexander Kappner <[email protected]>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d5eaf3b1..ebdc300 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8699,16 +8699,24 @@ static const struct attribute_group fan_attr_group = {
> .ec = TPID(__id1, __id2), \
> .quirks = __quirks }
>
> +#define TPACPI_FAN_QL_BIOS(__id1, __id2, __quirks) \
I would rather name it ..._QB() instead
Better of course to use three letters, though let's leave this when we
really have a need.
> + { .vendor = PCI_VENDOR_ID_LENOVO, \
> + .bios = TPID(__id1, __id2), \
> + .ec = TPACPI_MATCH_ANY, \
> + .quirks = __quirks }
> +
> static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
> TPACPI_FAN_QI('1', 'Y', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '8', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '6', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '0', TPACPI_FAN_Q1),
> TPACPI_FAN_QL('7', 'M', TPACPI_FAN_2FAN),
> + TPACPI_FAN_QL_BIOS('N', '1', TPACPI_FAN_2FAN) // Thinkpad P50 has 2 fans
> };
>
> #undef TPACPI_FAN_QL
> #undef TPACPI_FAN_QI
> +#undef TPACPI_FAN_QL_BIOS
>
> static int __init fan_init(struct ibm_init_struct *iibm)
> {
> --
> 2.1.4
>
--
With Best Regards,
Andy Shevchenko
Yes, I tested this successfully on my P50 (see below). Thanks for Ack'ing.
ak@REDDOT:~$ sensors
thinkpad-isa-0000
Adapter: ISA adapter
fan1: 3778 RPM
fan2: 3771 RPM
...
On 04/02/2018 01:22 PM, Andy Shevchenko wrote:
> I would rather name it ..._QB() instead
>
> Better of course to use three letters, though let's leave this when we
> really have a need.
Agreed; ideally we'd use a bitmask.
On 04/02/2018 12:08 PM, Henrique de Moraes Holschuh wrote:
> On Mon, 02 Apr 2018, Alexander Kappner wrote:
>> The Thinkpad P50 has 2 fans. Add the 2FAN quirk so the tpacpi driver
>> properly reports both fan speeds.
>> Because the P50 doesn't report the version of its EC controller, we need to
>> identify it by BIOS version (N1).
>>
>> Signed-off-by: Alexander Kappner <[email protected]>
>
> I assume you tested this on real hardware? If so:
>
> Acked-by: Henrique de Moraes Holschuh <[email protected]>
>
>> ---
>> drivers/platform/x86/thinkpad_acpi.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index d5eaf3b1..ebdc300 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -8699,16 +8699,24 @@ static const struct attribute_group fan_attr_group = {
>> .ec = TPID(__id1, __id2), \
>> .quirks = __quirks }
>>
>> +#define TPACPI_FAN_QL_BIOS(__id1, __id2, __quirks) \
>> + { .vendor = PCI_VENDOR_ID_LENOVO, \
>> + .bios = TPID(__id1, __id2), \
>> + .ec = TPACPI_MATCH_ANY, \
>> + .quirks = __quirks }
>> +
>> static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
>> TPACPI_FAN_QI('1', 'Y', TPACPI_FAN_Q1),
>> TPACPI_FAN_QI('7', '8', TPACPI_FAN_Q1),
>> TPACPI_FAN_QI('7', '6', TPACPI_FAN_Q1),
>> TPACPI_FAN_QI('7', '0', TPACPI_FAN_Q1),
>> TPACPI_FAN_QL('7', 'M', TPACPI_FAN_2FAN),
>> + TPACPI_FAN_QL_BIOS('N', '1', TPACPI_FAN_2FAN) // Thinkpad P50 has 2 fans
>> };
>>
>> #undef TPACPI_FAN_QL
>> #undef TPACPI_FAN_QI
>> +#undef TPACPI_FAN_QL_BIOS
>>
>> static int __init fan_init(struct ibm_init_struct *iibm)
>> {
>
On Mon, Apr 2, 2018 at 9:27 PM, Alexander Kappner <[email protected]> wrote:
> The Thinkpad P50 has 2 fans. Add the 2FAN quirk so the tpacpi driver
> properly reports both fan speeds.
> Because the P50 doesn't report the version of its EC controller, we need to
> identify it by BIOS version (N1).
I have pushed it with changes discussed to my review and testing queue, thanks!
I'm not sure if you were about to send a new version (no need
anymore), though for the next time, please be familiar with
https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst
(in particular, section 8).
And additional information, we are using special prefixes for our
subsystem, i.e.
"platform/x86: $DRIVER_NAME: ".
> Signed-off-by: Alexander Kappner <[email protected]>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d5eaf3b1..ebdc300 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8699,16 +8699,24 @@ static const struct attribute_group fan_attr_group = {
> .ec = TPID(__id1, __id2), \
> .quirks = __quirks }
>
> +#define TPACPI_FAN_QL_BIOS(__id1, __id2, __quirks) \
> + { .vendor = PCI_VENDOR_ID_LENOVO, \
> + .bios = TPID(__id1, __id2), \
> + .ec = TPACPI_MATCH_ANY, \
> + .quirks = __quirks }
> +
> static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
> TPACPI_FAN_QI('1', 'Y', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '8', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '6', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '0', TPACPI_FAN_Q1),
> TPACPI_FAN_QL('7', 'M', TPACPI_FAN_2FAN),
> + TPACPI_FAN_QL_BIOS('N', '1', TPACPI_FAN_2FAN) // Thinkpad P50 has 2 fans
> };
>
> #undef TPACPI_FAN_QL
> #undef TPACPI_FAN_QI
> +#undef TPACPI_FAN_QL_BIOS
>
> static int __init fan_init(struct ibm_init_struct *iibm)
> {
> --
> 2.1.4
>
--
With Best Regards,
Andy Shevchenko
Andy: Thanks for pushing, and for the pointers regarding formalities; I'll keep those in mind.
Also, it would be great if someone could test the fan reporting on a P70 Thinkpad (which has the same fan configuration as my P50 and thus likely needs the same quirk).
Best
Alexander Kappner
> On Mon, Apr 2, 2018 at 9:27 PM, Alexander Kappner <agk@...> wrote:
>> The Thinkpad P50 has 2 fans. Add the 2FAN quirk so the tpacpi driver
>> properly reports both fan speeds.
>> Because the P50 doesn't report the version of its EC controller, we need to
>> identify it by BIOS version (N1).
>
> I have pushed it with changes discussed to my review and testing queue, thanks!
>
> I'm not sure if you were about to send a new version (no need
> anymore), though for the next time, please be familiar with
> https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst
> (in particular, section 8).
>
> And additional information, we are using special prefixes for our
> subsystem, i.e.
> "platform/x86: $DRIVER_NAME: ".
>
>> Signed-off-by: Alexander Kappner <agk@...>
>> ---
>> drivers/platform/x86/thinkpad_acpi.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index d5eaf3b1..ebdc300 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -8699,16 +8699,24 @@ static const struct attribute_group fan_attr_group = {
>> .ec = TPID(__id1, __id2), \
>> .quirks = __quirks }
>>
>> +#define TPACPI_FAN_QL_BIOS(__id1, __id2, __quirks) \
>> + { .vendor = PCI_VENDOR_ID_LENOVO, \
>> + .bios = TPID(__id1, __id2), \
>> + .ec = TPACPI_MATCH_ANY, \
>> + .quirks = __quirks }
>> +
>> static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
>> TPACPI_FAN_QI('1', 'Y', TPACPI_FAN_Q1),
>> TPACPI_FAN_QI('7', '8', TPACPI_FAN_Q1),
>> TPACPI_FAN_QI('7', '6', TPACPI_FAN_Q1),
>> TPACPI_FAN_QI('7', '0', TPACPI_FAN_Q1),
>> TPACPI_FAN_QL('7', 'M', TPACPI_FAN_2FAN),
>> + TPACPI_FAN_QL_BIOS('N', '1', TPACPI_FAN_2FAN) // Thinkpad P50 has 2 fans
>> };
>>
>> #undef TPACPI_FAN_QL
>> #undef TPACPI_FAN_QI
>> +#undef TPACPI_FAN_QL_BIOS
>>
>> static int __init fan_init(struct ibm_init_struct *iibm)
>> {
>> --
>> 2.1.4
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
On 2018-04-02 11:27, Alexander Kappner wrote:
> The Thinkpad P50 has 2 fans. Add the 2FAN quirk so the tpacpi driver
> properly reports both fan speeds.
> Because the P50 doesn't report the version of its EC controller, we need to
> identify it by BIOS version (N1).
With 4.18 (including this patch) I'm seeing both fans on my P50.
[root@p50 ~]# sensors
[...]
thinkpad-isa-0000
Adapter: ISA adapter
fan1: 2319 RPM
fan2: 2323 RPM
Is there a way to actually control the second fan? There's pwm1, but no
pwm2. And pwm1 only affects fan1.
[root@p50 ~]# ls -al /sys/devices/platform/thinkpad_hwmon/hwmon/hwmon3/
total 0
drwxr-xr-x 3 root root 0 Sep 26 16:25 .
drwxr-xr-x 3 root root 0 Sep 26 16:23 ..
lrwxrwxrwx 1 root root 0 Sep 25 16:11 device -> ../../../thinkpad_hwmon
-r--r--r-- 1 root root 4096 Sep 25 16:11 fan1_input
-r--r--r-- 1 root root 4096 Sep 25 16:11 fan2_input
-r--r--r-- 1 root root 4096 Sep 25 16:11 name
drwxr-xr-x 2 root root 0 Sep 26 15:57 power
-rw-r--r-- 1 root root 4096 Sep 26 16:25 pwm1
-rw-r--r-- 1 root root 4096 Sep 26 16:25 pwm1_enable
lrwxrwxrwx 1 root root 0 Sep 25 16:11 subsystem -> ../../../../../class/hwmon
-rw-r--r-- 1 root root 4096 Sep 25 16:11 uevent
[root@p50 ~]# echo 2 > /sys/devices/platform/thinkpad_hwmon/hwmon/hwmon3/pwm1_enable
[root@p50 ~]# sensors
[...]
thinkpad-isa-0000
Adapter: ISA adapter
fan1: 3042 RPM
fan2: 2323 RPM
Thanks!
Stefan
> Signed-off-by: Alexander Kappner <[email protected]>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d5eaf3b1..ebdc300 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8699,16 +8699,24 @@ static const struct attribute_group fan_attr_group = {
> .ec = TPID(__id1, __id2), \
> .quirks = __quirks }
>
> +#define TPACPI_FAN_QL_BIOS(__id1, __id2, __quirks) \
> + { .vendor = PCI_VENDOR_ID_LENOVO, \
> + .bios = TPID(__id1, __id2), \
> + .ec = TPACPI_MATCH_ANY, \
> + .quirks = __quirks }
> +
> static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
> TPACPI_FAN_QI('1', 'Y', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '8', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '6', TPACPI_FAN_Q1),
> TPACPI_FAN_QI('7', '0', TPACPI_FAN_Q1),
> TPACPI_FAN_QL('7', 'M', TPACPI_FAN_2FAN),
> + TPACPI_FAN_QL_BIOS('N', '1', TPACPI_FAN_2FAN) // Thinkpad P50 has 2 fans
> };
>
> #undef TPACPI_FAN_QL
> #undef TPACPI_FAN_QI
> +#undef TPACPI_FAN_QL_BIOS
>
> static int __init fan_init(struct ibm_init_struct *iibm)
> {
> --
> 2.1.4
>
>
I have a Thinkpad P51 (BIOS version: N1UET74W, EC version: N1UHT26W).
This patch works on my computer and I can read both fan speeds through sysfs
interface.
On 2018-09-26 16:34:19 CEST, Stefan Assmann wrote:
> Is there a way to actually control the second fan? There's pwm1, but no
> pwm2. And pwm1 only affects fan1.
I have played with the driver code and I noticed that it is possible to
control both fans independently (unlike what the "Fan subdriver" comment in
the code says).
You can do that simply by calling fan_select_fan1 or fan_select_fan2, to
select a fan to be controlled, before calling fan_set_level. All control
parameters (disengaged, manual speeds, auto) are fully independent.
What is not smooth is reading back current control parameters from register
0x2f (like fan_pwm1_show or fan_pwm1_enable_show actually do), because the EC
ignores which fan is currently selected and it always returns the last written
value.
Distinti Saluti/Best Regards,
Dario Messina