[PATCH] Add Second Fan Support for Thinkpad P50

[PATCH] Add Second Fan Support for Thinkpad P50

[Alexander Kappner]

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 <agk@godking.net>
---
 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

Re: [PATCH] Add Second Fan Support for Thinkpad P50

[Henrique de Moraes Holschuh]

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 <agk@godking.net>

I assume you tested this on real hardware?  If so:

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

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

Re: [PATCH] Add Second Fan Support for Thinkpad P50

[Andy Shevchenko]

On Mon, Apr 2, 2018 at 9:27 PM, Alexander Kappner <agk@godking.net> 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 <agk@godking.net>
> ---
>  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

Re: [ibm-acpi-devel] [PATCH] Add Second Fan Support for Thinkpad P50

[Alexander Kappner]

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 <agk@godking.net>
> 
> I assume you tested this on real hardware?  If so:
> 
> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> 
>> ---
>>  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)
>>  {
> 

Re: [PATCH] Add Second Fan Support for Thinkpad P50

[Andy Shevchenko]

On Mon, Apr 2, 2018 at 9:27 PM, Alexander Kappner <agk@godking.net> 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@godking.net>
> ---
>  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

Re: [PATCH] Add Second Fan Support for Thinkpad P50

[Alexander Kappner]

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

Re: [PATCH] Add Second Fan Support for Thinkpad P50

[Stefan Assmann]

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 <agk@godking.net>
> ---
>  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
> 
> 

Re: [PATCH] Add Second Fan Support for Thinkpad P50

[Dario Messina]

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