* Re: [PATCH] Add Second Fan Support for Thinkpad P50
2018-04-02 18:27 [PATCH] Add Second Fan Support for Thinkpad P50 Alexander Kappner
@ 2018-04-02 19:08 ` Henrique de Moraes Holschuh
2018-04-02 21:45 ` [ibm-acpi-devel] " Alexander Kappner
2018-04-02 20:22 ` Andy Shevchenko
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Henrique de Moraes Holschuh @ 2018-04-02 19:08 UTC (permalink / raw)
To: Alexander Kappner
Cc: ibm-acpi, dvhart, andy, ibm-acpi-devel, platform-driver-x86,
linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [ibm-acpi-devel] [PATCH] Add Second Fan Support for Thinkpad P50
2018-04-02 19:08 ` Henrique de Moraes Holschuh
@ 2018-04-02 21:45 ` Alexander Kappner
0 siblings, 0 replies; 8+ messages in thread
From: Alexander Kappner @ 2018-04-02 21:45 UTC (permalink / raw)
To: Henrique de Moraes Holschuh, Andy Shevchenko
Cc: ibm-acpi, linux-kernel, platform-driver-x86, ibm-acpi-devel,
dvhart, andy
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)
>> {
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Add Second Fan Support for Thinkpad P50
2018-04-02 18:27 [PATCH] Add Second Fan Support for Thinkpad P50 Alexander Kappner
2018-04-02 19:08 ` Henrique de Moraes Holschuh
@ 2018-04-02 20:22 ` Andy Shevchenko
2018-04-04 23:20 ` Alexander Kappner
2018-04-03 8:13 ` Andy Shevchenko
2018-09-26 14:34 ` Stefan Assmann
3 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2018-04-02 20:22 UTC (permalink / raw)
To: Alexander Kappner
Cc: Henrique de Moraes Holschuh, Darren Hart, Andy Shevchenko,
ibm-acpi-devel, Platform Driver, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Add Second Fan Support for Thinkpad P50
2018-04-02 20:22 ` Andy Shevchenko
@ 2018-04-04 23:20 ` Alexander Kappner
0 siblings, 0 replies; 8+ messages in thread
From: Alexander Kappner @ 2018-04-04 23:20 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Henrique de Moraes Holschuh, Darren Hart, Andy Shevchenko,
ibm-acpi-devel, Platform Driver, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Add Second Fan Support for Thinkpad P50
2018-04-02 18:27 [PATCH] Add Second Fan Support for Thinkpad P50 Alexander Kappner
2018-04-02 19:08 ` Henrique de Moraes Holschuh
2018-04-02 20:22 ` Andy Shevchenko
@ 2018-04-03 8:13 ` Andy Shevchenko
2018-09-26 14:34 ` Stefan Assmann
3 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2018-04-03 8:13 UTC (permalink / raw)
To: Alexander Kappner
Cc: Henrique de Moraes Holschuh, Darren Hart, Andy Shevchenko,
ibm-acpi-devel, Platform Driver, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Add Second Fan Support for Thinkpad P50
2018-04-02 18:27 [PATCH] Add Second Fan Support for Thinkpad P50 Alexander Kappner
` (2 preceding siblings ...)
2018-04-03 8:13 ` Andy Shevchenko
@ 2018-09-26 14:34 ` Stefan Assmann
2019-07-20 15:22 ` Dario Messina
3 siblings, 1 reply; 8+ messages in thread
From: Stefan Assmann @ 2018-09-26 14:34 UTC (permalink / raw)
To: Alexander Kappner
Cc: ibm-acpi, dvhart, andy, ibm-acpi-devel, platform-driver-x86,
linux-kernel
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] Add Second Fan Support for Thinkpad P50
2018-09-26 14:34 ` Stefan Assmann
@ 2019-07-20 15:22 ` Dario Messina
0 siblings, 0 replies; 8+ messages in thread
From: Dario Messina @ 2019-07-20 15:22 UTC (permalink / raw)
To: Stefan Assmann
Cc: Alexander Kappner, ibm-acpi, dvhart, andy, ibm-acpi-devel,
platform-driver-x86, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread