Re: [ibm-acpi-devel] [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
śr., 29 kwi 2020 o 02:21 la...@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
Re: [ibm-acpi-devel] [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
On Wed, 29 Apr 2020, Stefan Assmann wrote: > On 29.04.20 02:20, la...@apache.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 ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
pon., 27 kwi 2020 o 20:41 Dario Messina napisał(a): > > On Thu, Apr 23, 2020 at 23:57:59 CEST, Lars 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
Re: [ibm-acpi-devel] [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
On Thu, Apr 23, 2020 at 23:57:59 CEST, Lars 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 ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
On Mon, Apr 27, 2020 at 10:44 AM la...@apache.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 No need for this one, I will update locally, thanks! > > -- Lars > > On Friday, April 24, 2020, 4:12:05 AM PDT, Andy Shevchenko > wrote: > > > > > > On Fri, Apr 24, 2020 at 12:57 AM Lars 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 > > 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 ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
On Fri, Apr 24, 2020 at 12:57 AM Lars 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 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 ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models
On Fri, Apr 24, 2020 at 12:57 AM Lars 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 > --- > 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_2FAN0x0002 /* EC 0x31 bit 0 selects fan2 > */ > +#define TPACPI_FAN_2CTL0x0004 /* 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; >