* Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
[not found] ` <20200423215709.72993-1-larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>
@ 2020-04-24 9:59 ` Andy Shevchenko
2020-04-24 11:11 ` Andy Shevchenko
2020-04-27 18:41 ` Dario Messina
2 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2020-04-24 9:59 UTC (permalink / raw
To: Lars
Cc: Alexander Kappner, Kevin Slagle, Sebastian Dörner,
Dario Messina, Henrique de Moraes Holschuh, Stefan Assmann,
Platform Driver, Thinkpad-acpi devel ML,
arc-e1Wt+8K1Ta8ZEwqihLH0+Q
On Fri, Apr 24, 2020 at 12:57 AM Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org> wrote:
>
> This adds dual fan control for the following models:
> P50, P51, P52, P70, P71, P72, P1 gen1, X1E gen1, P2 gen2, and X1E gen2.
>
> Both fans are controlled together as if they were a single fan.
>
> Tested on an X1 Extreme Gen1, an X1 Extreme Gen2, and a P50.
>
> The patch is defensive, it adds only specific supported machines, and falls
> back to the old behavior if both fans cannot be controlled.
>
> Background:
> I tested the BIOS default behavior on my X1E gen2 and both fans are always
> changed together. So rather than adding controls for each fan, this controls
> both fans together as the BIOS would do.
>
> This was inspired by a discussion on dual fan support for the thinkfan tool
> [1].
> All BIOS ids are taken from there. The X1E gen2 id is verified on my machine.
>
> Thanks to GitHub users voidworker and civic9 for the earlier patches and BIOS
> ids, and to users peter-stoll and sassman for testing the patch on their
> machines.
>
Pushed to my review and testing queue, thank you!
> [1]: https://github.com/vmatare/thinkfan/issues/58
>
> Signed-off-by: Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 43 ++++++++++++++++++++++++----
> 1 file changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index da794dcfdd92..9e0f65e567be 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -318,6 +318,7 @@ static struct {
> u32 uwb:1;
> u32 fan_ctrl_status_undef:1;
> u32 second_fan:1;
> + u32 second_fan_ctl:1;
> u32 beep_needs_two_args:1;
> u32 mixer_no_level_control:1;
> u32 battery_force_primary:1;
> @@ -8325,11 +8326,19 @@ static int fan_set_level(int level)
>
> switch (fan_control_access_mode) {
> case TPACPI_FAN_WR_ACPI_SFAN:
> - if (level >= 0 && level <= 7) {
> - if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level))
> - return -EIO;
> - } else
> + if ((level < 0) || (level > 7))
> return -EINVAL;
> +
> + if (tp_features.second_fan_ctl) {
> + if (!fan_select_fan2() ||
> + !acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) {
> + pr_warn("Couldn't set 2nd fan level, disabling support\n");
> + tp_features.second_fan_ctl = 0;
> + }
> + fan_select_fan1();
> + }
> + if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level))
> + return -EIO;
> break;
>
> case TPACPI_FAN_WR_ACPI_FANS:
> @@ -8346,6 +8355,15 @@ static int fan_set_level(int level)
> else if (level & TP_EC_FAN_AUTO)
> level |= 4; /* safety min speed 4 */
>
> + if (tp_features.second_fan_ctl) {
> + if (!fan_select_fan2() ||
> + !acpi_ec_write(fan_status_offset, level)) {
> + pr_warn("Couldn't set 2nd fan level, disabling support\n");
> + tp_features.second_fan_ctl = 0;
> + }
> + fan_select_fan1();
> +
> + }
> if (!acpi_ec_write(fan_status_offset, level))
> return -EIO;
> else
> @@ -8764,6 +8782,7 @@ static const struct attribute_group fan_attr_group = {
>
> #define TPACPI_FAN_Q1 0x0001 /* Unitialized HFSP */
> #define TPACPI_FAN_2FAN 0x0002 /* EC 0x31 bit 0 selects fan2 */
> +#define TPACPI_FAN_2CTL 0x0004 /* selects fan2 control */
>
> static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
> TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1),
> @@ -8772,6 +8791,13 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
> TPACPI_QEC_IBM('7', '0', TPACPI_FAN_Q1),
> TPACPI_QEC_LNV('7', 'M', TPACPI_FAN_2FAN),
> TPACPI_Q_LNV('N', '1', TPACPI_FAN_2FAN),
> + TPACPI_Q_LNV3('N', '1', 'D', TPACPI_FAN_2CTL), /* P70 */
> + TPACPI_Q_LNV3('N', '1', 'E', TPACPI_FAN_2CTL), /* P50 */
> + TPACPI_Q_LNV3('N', '1', 'T', TPACPI_FAN_2CTL), /* P71 */
> + TPACPI_Q_LNV3('N', '1', 'U', TPACPI_FAN_2CTL), /* P51 */
> + TPACPI_Q_LNV3('N', '2', 'C', TPACPI_FAN_2CTL), /* P52 / P72 */
> + TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2CTL), /* P1 / X1 Extreme (1st gen) */
> + TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL), /* P1 / X1 Extreme (2nd gen) */
> };
>
> static int __init fan_init(struct ibm_init_struct *iibm)
> @@ -8789,6 +8815,7 @@ static int __init fan_init(struct ibm_init_struct *iibm)
> fan_watchdog_maxinterval = 0;
> tp_features.fan_ctrl_status_undef = 0;
> tp_features.second_fan = 0;
> + tp_features.second_fan_ctl = 0;
> fan_control_desired_level = 7;
>
> if (tpacpi_is_ibm()) {
> @@ -8813,8 +8840,12 @@ static int __init fan_init(struct ibm_init_struct *iibm)
> fan_quirk1_setup();
> if (quirks & TPACPI_FAN_2FAN) {
> tp_features.second_fan = 1;
> - dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_FAN,
> - "secondary fan support enabled\n");
> + pr_info("secondary fan support enabled\n");
> + }
> + if (quirks & TPACPI_FAN_2CTL) {
> + tp_features.second_fan = 1;
> + tp_features.second_fan_ctl = 1;
> + pr_info("secondary fan control enabled\n");
> }
> } else {
> pr_err("ThinkPad ACPI EC access misbehaving, fan status and control unavailable\n");
> --
> 2.25.3
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
[not found] ` <20200423215709.72993-1-larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>
2020-04-24 9:59 ` [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models Andy Shevchenko
@ 2020-04-24 11:11 ` Andy Shevchenko
[not found] ` <1630425700.517847.1587973463950@mail.yahoo.com>
2020-04-27 18:41 ` Dario Messina
2 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2020-04-24 11:11 UTC (permalink / raw
To: Lars
Cc: Alexander Kappner, Kevin Slagle, Sebastian Dörner,
Dario Messina, Henrique de Moraes Holschuh, Marc Burkhardt,
Stefan Assmann, Platform Driver, Thinkpad-acpi devel ML
On Fri, Apr 24, 2020 at 12:57 AM Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org> wrote:
>
> This adds dual fan control for the following models:
> P50, P51, P52, P70, P71, P72, P1 gen1, X1E gen1, P2 gen2, and X1E gen2.
>
> Both fans are controlled together as if they were a single fan.
>
> Tested on an X1 Extreme Gen1, an X1 Extreme Gen2, and a P50.
>
> The patch is defensive, it adds only specific supported machines, and falls
> back to the old behavior if both fans cannot be controlled.
>
> Background:
> I tested the BIOS default behavior on my X1E gen2 and both fans are always
> changed together. So rather than adding controls for each fan, this controls
> both fans together as the BIOS would do.
>
> This was inspired by a discussion on dual fan support for the thinkfan tool
> [1].
> All BIOS ids are taken from there. The X1E gen2 id is verified on my machine.
>
> Thanks to GitHub users voidworker and civic9 for the earlier patches and BIOS
> ids, and to users peter-stoll and sassman for testing the patch on their
> machines.
>
> [1]: https://github.com/vmatare/thinkfan/issues/58
>
> Signed-off-by: Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>
One question though, is Lars your real name here? [1]
[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
[not found] ` <1630425700.517847.1587973463950-sAHhhX/85wgbqTNvkayDYw@public.gmane.org>
@ 2020-04-27 9:40 ` Andy Shevchenko
0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2020-04-27 9:40 UTC (permalink / raw
To: larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org
Cc: Alexander Kappner, Kevin Slagle, Sebastian Dörner,
Dario Messina, Henrique de Moraes Holschuh, Marc Burkhardt,
Stefan Assmann, Platform Driver, Thinkpad-acpi devel ML
On Mon, Apr 27, 2020 at 10:44 AM larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org> wrote:
>
>
> Hi Andy,
>
> my full name is Lars Hofhansl.
> Should I send a new post?
>
> Just in case, I hereby:
>
> Signed-off-by: Lars Hofhansl <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>
No need for this one, I will update locally, thanks!
>
> -- Lars
>
> On Friday, April 24, 2020, 4:12:05 AM PDT, Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>
>
>
>
> On Fri, Apr 24, 2020 at 12:57 AM Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org> wrote:
> >
> > This adds dual fan control for the following models:
> > P50, P51, P52, P70, P71, P72, P1 gen1, X1E gen1, P2 gen2, and X1E gen2.
> >
> > Both fans are controlled together as if they were a single fan.
> >
> > Tested on an X1 Extreme Gen1, an X1 Extreme Gen2, and a P50.
> >
> > The patch is defensive, it adds only specific supported machines, and falls
> > back to the old behavior if both fans cannot be controlled.
> >
> > Background:
> > I tested the BIOS default behavior on my X1E gen2 and both fans are always
> > changed together. So rather than adding controls for each fan, this controls
> > both fans together as the BIOS would do.
> >
> > This was inspired by a discussion on dual fan support for the thinkfan tool
> > [1].
> > All BIOS ids are taken from there. The X1E gen2 id is verified on my machine.
> >
> > Thanks to GitHub users voidworker and civic9 for the earlier patches and BIOS
> > ids, and to users peter-stoll and sassman for testing the patch on their
> > machines.
> >
> > [1]: https://github.com/vmatare/thinkfan/issues/58
> >
> > Signed-off-by: Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>
>
> One question though, is Lars your real name here? [1]
>
>
> [1]:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
>
> --
> With Best Regards,
> Andy Shevchenko
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
[not found] ` <20200423215709.72993-1-larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>
2020-04-24 9:59 ` [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models Andy Shevchenko
2020-04-24 11:11 ` Andy Shevchenko
@ 2020-04-27 18:41 ` Dario Messina
2020-04-28 21:18 ` civic9
2 siblings, 1 reply; 7+ messages in thread
From: Dario Messina @ 2020-04-27 18:41 UTC (permalink / raw
To: Lars
Cc: agk-mA9ux5SlULTR7s880joybQ, kjslag-Re5JQEeQqe8AvxtiuMwx3w,
bastidoerner-Re5JQEeQqe8AvxtiuMwx3w,
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
ibm-acpi-N3TV7GIv+o9fyO9Q7EP/yw,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA,
sassmann-llIHtaV5axyzQB+pC5nmwQ, arc-e1Wt+8K1Ta8ZEwqihLH0+Q
On Thu, Apr 23, 2020 at 23:57:59 CEST, Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org> wrote:
> This adds dual fan control for the following models:
> P50, P51, P52, P70, P71, P72, P1 gen1, X1E gen1, P2 gen2, and X1E gen2.
>
> Both fans are controlled together as if they were a single fan.
> [...]
> Background:
> I tested the BIOS default behavior on my X1E gen2 and both fans are always
> changed together. So rather than adding controls for each fan, this controls
> both fans together as the BIOS would do.
Hi Lars, why have you chosen to control multiple fans in this way?
I know that BIOS controls both fans together, but the EC has the capabilities
to control both fans independently, so maybe it can be convenient to expose
this feature.
Distinti saluti/Best regards,
Dario Messina
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
2020-04-27 18:41 ` Dario Messina
@ 2020-04-28 21:18 ` civic9
[not found] ` <1605997626.1278142.1588119634625@mail.yahoo.com>
0 siblings, 1 reply; 7+ messages in thread
From: civic9 @ 2020-04-28 21:18 UTC (permalink / raw
To: Dario Messina
Cc: agk-mA9ux5SlULTR7s880joybQ, kjslag-Re5JQEeQqe8AvxtiuMwx3w,
bastidoerner-Re5JQEeQqe8AvxtiuMwx3w,
ibm-acpi-N3TV7GIv+o9fyO9Q7EP/yw, sassmann-llIHtaV5axyzQB+pC5nmwQ,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA,
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Lars,
arc-e1Wt+8K1Ta8ZEwqihLH0+Q
pon., 27 kwi 2020 o 20:41 Dario Messina <nanodario@gmail.com> napisał(a):
>
> On Thu, Apr 23, 2020 at 23:57:59 CEST, Lars <larsh@apache.org> wrote:
> > This adds dual fan control for the following models:
> > P50, P51, P52, P70, P71, P72, P1 gen1, X1E gen1, P2 gen2, and X1E gen2.
> >
> > Both fans are controlled together as if they were a single fan.
> > [...]
> > Background:
> > I tested the BIOS default behavior on my X1E gen2 and both fans are always
> > changed together. So rather than adding controls for each fan, this controls
> > both fans together as the BIOS would do.
> Hi Lars, why have you chosen to control multiple fans in this way?
> I know that BIOS controls both fans together, but the EC has the capabilities
> to control both fans independently, so maybe it can be convenient to expose
> this feature.
+1
Previous version of the patch [1] allows to control both fans independently.
However some software like thinkfan is not ready to control two fans.
But I also think this feature should be at least optionally exposed.
[1] https://github.com/civic9/thinkpad_acpi.2ndfan.patch/blob/master/thinkpad_acpi.2ndfan.patch/thinkpad_acpi.2ndfan.patch
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
[not found] ` <91feeffa-1f48-347a-fb90-7bf733647476-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
@ 2020-04-30 21:40 ` Henrique de Moraes Holschuh
0 siblings, 0 replies; 7+ messages in thread
From: Henrique de Moraes Holschuh @ 2020-04-30 21:40 UTC (permalink / raw
To: Stefan Assmann
Cc: agk-mA9ux5SlULTR7s880joybQ, kjslag-Re5JQEeQqe8AvxtiuMwx3w,
Dario Messina, civic9, ibm-acpi-N3TV7GIv+o9fyO9Q7EP/yw,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA,
bastidoerner-Re5JQEeQqe8AvxtiuMwx3w,
larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org,
arc-e1Wt+8K1Ta8ZEwqihLH0+Q,
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
On Wed, 29 Apr 2020, Stefan Assmann wrote:
> On 29.04.20 02:20, larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org wrote:
> > Do you have a use case for that behavior?
> >
> > The previous patch broke the /proc interface, didn't not work with the current version of thinkfan
> > (but a a version with multi-fan support is in the works), and it had hard to track internal mutable state.
> >
> > The proposed change is clean on all these fronts.
> >
> > I'm not a fan of surprising the user with unnecessarily complex behavior (but perhaps this can be added as an option in the future.)
>
> I concur to keep the patch as is. Any additional functionality could be
> added later on, if deemed necessary.
Yes, but let's avoid changing exposed APIs as much as possible...
Anyway, the correct interface *when exposing both fans* is:
1. Use the "hwmon" sysfs interface to expose each fan separately, and
control them separately.
1a. it is quite acceptable to control them in group by default, and have
a module parameter to select grouped, or separate behavior.
2. /proc/acpi/ibm/fan shall control both of them at the same time, and
report data from the first fan. THIS INTERFACE IS FROZEN, LET'S NOT
MESS WITH IT.
Also, "fail-safe" for this is to have the two fans on automatic, and to
enable both fans.
All of that said, *it is very much acceptable* to merge a first set of
patches that controls both fans simultaneously and exposes the fan group
as if it were just the main fan, and later improve on the patch to
expose the second fan separately (provided safe group behavior is
maintained, see above).
--
Henrique Holschuh
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
[not found] ` <1605997626.1278142.1588119634625-sAHhhX/85wgbqTNvkayDYw@public.gmane.org>
@ 2020-05-01 18:48 ` civic9
0 siblings, 0 replies; 7+ messages in thread
From: civic9 @ 2020-05-01 18:48 UTC (permalink / raw
To: larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org
Cc: agk-mA9ux5SlULTR7s880joybQ, kjslag-Re5JQEeQqe8AvxtiuMwx3w,
bastidoerner-Re5JQEeQqe8AvxtiuMwx3w, Dario Messina,
ibm-acpi-N3TV7GIv+o9fyO9Q7EP/yw, sassmann-llIHtaV5axyzQB+pC5nmwQ,
platform-driver-x86-u79uwXL29TY76Z2rM5mHXA,
ibm-acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
arc-e1Wt+8K1Ta8ZEwqihLH0+Q
śr., 29 kwi 2020 o 02:21 larsh@apache.org <larsh@apache.org> napisał(a):
>
> Do you have a use case for that behavior?
I work very often with just one fan enabled at the lowest level. It is
inaudible for me and it does its job for not too heavy usage. If the
second one is also enabled I can hear them. We love Linux for such
small extra features too.
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-01 18:48 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200423165457.54388-1-larsh@apache.org>
[not found] ` <20200423215709.72993-1-larsh@apache.org>
[not found] ` <20200423215709.72993-1-larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>
2020-04-24 9:59 ` [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models Andy Shevchenko
2020-04-24 11:11 ` Andy Shevchenko
[not found] ` <1630425700.517847.1587973463950@mail.yahoo.com>
[not found] ` <1630425700.517847.1587973463950-sAHhhX/85wgbqTNvkayDYw@public.gmane.org>
2020-04-27 9:40 ` Andy Shevchenko
2020-04-27 18:41 ` Dario Messina
2020-04-28 21:18 ` civic9
[not found] ` <1605997626.1278142.1588119634625@mail.yahoo.com>
[not found] ` <91feeffa-1f48-347a-fb90-7bf733647476@kpanic.de>
[not found] ` <91feeffa-1f48-347a-fb90-7bf733647476-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
2020-04-30 21:40 ` Henrique de Moraes Holschuh
[not found] ` <1605997626.1278142.1588119634625-sAHhhX/85wgbqTNvkayDYw@public.gmane.org>
2020-05-01 18:48 ` civic9
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.