| Message ID | 1522693621-3124-1-git-send-email-agk@godking.net (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
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) > {
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 >
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) >> { >
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 >
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 <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 > >
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
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) {
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(+)