[ibm-acpi-devel] [PATCH v1 2/3] platform/x86: thinkpad_acpi: Join string literals back
There is no point to keep string literal split. It even makes slightly
harder to maintain and debug.
Join string literals back to be oneliners.
While here, print negative error without changing a sign as it is a
common pattern in the kernel.
Other than above there were no functional changes intended.
Signed-off-by: Andy Shevchenko
---
drivers/platform/x86/thinkpad_acpi.c | 182 +--
1 file changed, 65 insertions(+), 117 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c
b/drivers/platform/x86/thinkpad_acpi.c
index 7740b5e1b998..e6fbb2579dd9 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -590,8 +590,8 @@ static int acpi_evalf(acpi_handle handle,
break;
/* add more types as needed */
default:
- pr_err("acpi_evalf() called "
- "with invalid format character '%c'\n", c);
+ pr_err("acpi_evalf() called with invalid format
character '%c'\n",
+ c);
va_end(ap);
return 0;
}
@@ -619,8 +619,8 @@ static int acpi_evalf(acpi_handle handle,
break;
/* add more types as needed */
default:
- pr_err("acpi_evalf() called "
- "with invalid format character '%c'\n", res_type);
+ pr_err("acpi_evalf() called with invalid format character
'%c'\n",
+ res_type);
return 0;
}
@@ -790,8 +790,8 @@ static int __init setup_acpi_notify(struct ibm_struct *ibm)
ibm->acpi->type, dispatch_acpi_notify, ibm);
if (ACPI_FAILURE(status)) {
if (status == AE_ALREADY_EXISTS) {
- pr_notice("another device driver is already "
- "handling %s events\n", ibm->name);
+ pr_notice("another device driver is already handling %s
events\n",
+ ibm->name);
} else {
pr_err("acpi_install_notify_handler(%s) failed: %s\n",
ibm->name, acpi_format_exception(status));
@@ -1095,8 +1095,7 @@ static void printk_deprecated_attribute(const char *
const what,
const char * const details)
{
tpacpi_log_usertask("deprecated sysfs attribute");
- pr_warn("WARNING: sysfs attribute %s is deprecated and "
- "will be removed. %s\n",
+ pr_warn("WARNING: sysfs attribute %s is deprecated and will be removed.
%s\n",
what, details);
}
@@ -1828,8 +1827,7 @@ static void __init tpacpi_check_outdated_fw(void)
* best if the user upgrades the firmware anyway.
*/
pr_warn("WARNING: Outdated ThinkPad BIOS/EC firmware\n");
- pr_warn("WARNING: This firmware may be missing critical bug "
- "fixes and/or important features\n");
+ pr_warn("WARNING: This firmware may be missing critical bug
fixes and/or important features\n");
}
}
@@ -2198,8 +2196,7 @@ static int hotkey_mask_set(u32 mask)
* a given event.
*/
if (!hotkey_mask_get() && !rc && (fwmask & ~hotkey_acpi_mask)) {
- pr_notice("asked for hotkey mask 0x%08x, but "
- "firmware forced it to 0x%08x\n",
+ pr_notice("asked for hotkey mask 0x%08x, but firmware forced it
to 0x%08x\n",
fwmask, hotkey_acpi_mask);
}
@@ -2224,11 +2221,9 @@ static int hotkey_user_mask_set(const u32 mask)
(mask == 0x || mask == 0xff ||
mask == 0x)) {
tp_warned.hotkey_mask_ff = 1;
- pr_notice("setting the hotkey mask to 0x%08x is likely "
- "not the best way to go about it\n", mask);
- pr_notice("please consider using the driver defaults, "
- "and refer to up-to-date thinkpad-acpi "
- "documentation\n");
+ pr_notice("setting the hotkey mask to 0x%08x is likely not the
best way to go about it\n",
+ mask);
+ pr_notice("please consider using the driver defaults, and refer
to up-to-date thinkpad-acpi documentation\n");
}
/* Try to enable what the user asked for, plus whatever we need.
@@ -2603,17 +2598,14 @@ sta
Re: [ibm-acpi-devel] [PATCH v1 2/3] platform/x86: thinkpad_acpi: Join string literals back
On Tue, 2017-05-09 at 21:24 -0300, Henrique de Moraes Holschuh wrote:
> On Tue, May 9, 2017, at 14:33, Andy Shevchenko wrote:
> > On Tue, 2017-05-09 at 14:10 -0300, Henrique de Moraes Holschuh
> > wrote:
> > > > While here, print negative error without changing a sign as it
> > > > is a
> > > > common pattern in the kernel.
> > >
> > > A separate patch for this would be better: it would be easier to
> > > actually check that no functional changes crept in by mistake.
> >
> > It doesn't make sense to me. It would touch same lines of code I do
> > already here and it's only one place, see below.
>
> I had to go line-by-line looking for the darn thing, instead of just
> compiling before-and-after and checking for an unchanged object file.
>
> > > > rc = fan_set_enable();
> > > > if (rc < 0) {
> > > > - pr_err("fan watchdog: error %d while enabling
> > > > fan,
> > > > "
> > > > - "will try again later...\n", -rc);
> > > > + pr_err("fan watchdog: error %d while enabling
> > > > fan,
> > > > will try again later...\n",
> > > > + rc);
>
> Yeah. This one. I don't have a problem with this change at all (I
> acked
> it), but it took some effort to find the nail in the hailstack.
Okay, what I'm going to do is:
1) drop patch 1 for now;
2) split patch 2 into two patches (and append your Ack on both);
3) push to our testing branch (I can send v2 if we need one more round
of review).
Tell me if there is any objection.
--
Andy Shevchenko
Intel Finland Oy
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 v1 2/3] platform/x86: thinkpad_acpi: Join string literals back
On Tue, 2017-05-09 at 14:10 -0300, Henrique de Moraes Holschuh wrote:
Thanks for review, my comments below.
> On Tue, 09 May 2017, Andy Shevchenko wrote:
> > There is no point to keep string literal split. It even makes
> > slightly
> > harder to maintain and debug.
>
> Ooold code, from a time when random people would annoy others over
> 80-column instead of doing useful reviews...
Yes, I know, nowadays checkpatch doesn't complain on them.
> > While here, print negative error without changing a sign as it is a
> > common pattern in the kernel.
>
> A separate patch for this would be better: it would be easier to
> actually check that no functional changes crept in by mistake.
It doesn't make sense to me. It would touch same lines of code I do
already here and it's only one place, see below.
> > rc = fan_set_enable();
> > if (rc < 0) {
> > - pr_err("fan watchdog: error %d while enabling fan,
> > "
> > - "will try again later...\n", -rc);
> > + pr_err("fan watchdog: error %d while enabling fan,
> > will try again later...\n",
> > + rc);
--
Andy Shevchenko
Intel Finland Oy
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
[ibm-acpi-devel] [PATCH v1 1/3] platform/x86: thinkpad_acpi: Make logic straight in hotkey_exit()
The commit 4be73005e4dc
("thinkpad-acpi: remove uneeded tp_features.hotkey tests in
hotkey_exit")
adds a complex logic behind hotkey status check in a way
it started mixing logical operations with bitwise ones.
Refactor the code to make it straight and slightly clearer.
Signed-off-by: Andy Shevchenko
---
drivers/platform/x86/thinkpad_acpi.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c
b/drivers/platform/x86/thinkpad_acpi.c
index 7b6cb0c69b02..7740b5e1b998 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3090,6 +3090,8 @@ static void tpacpi_send_radiosw_update(void)
static void hotkey_exit(void)
{
+ int res;
+
#ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
mutex_lock(&hotkey_mutex);
hotkey_poll_stop_sync();
@@ -3101,11 +3103,8 @@ static void hotkey_exit(void)
dbg_printk(TPACPI_DBG_EXIT | TPACPI_DBG_HKEY,
"restoring original HKEY status and mask\n");
- /* yes, there is a bitwise or below, we want the
-* functions to be called even if one of them fail */
- if (((tp_features.hotkey_mask &&
- hotkey_mask_set(hotkey_orig_mask)) |
-hotkey_status_set(false)) != 0)
+ res = tp_features.hotkey_mask ? hotkey_mask_set(hotkey_orig_mask) : 0;
+ if (hotkey_status_set(false) || res)
pr_err("failed to restore hot key mask "
"to BIOS defaults\n");
}
--
2.11.0
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 v1 1/3] platform/x86: thinkpad_acpi: Make logic straight in hotkey_exit()
On Tue, 2017-05-09 at 14:02 -0300, Henrique de Moraes Holschuh wrote:
Thanks for review, my comments below.
> On Tue, 09 May 2017, Andy Shevchenko wrote:
> > The commit 4be73005e4dc
> >
> > ("thinkpad-acpi: remove uneeded tp_features.hotkey tests in
> > hotkey_exit")
> >
> > adds a complex logic behind hotkey status check in a way
> > it started mixing logical operations with bitwise ones.
> >
> > Refactor the code to make it straight and slightly clearer.
>
> Eh, I find this actually less clear, given the comment that was in the
> old code, which you deleted.
For me doing 'bitwise or' on negative return code and boolean looks
weird...
>
> Please keep the important part of the comment at the very least.
...that's why comment has been added I suppose, and my patch makes it
not needed.
Though, I better just drop the change completely, I didn't pay attention
if compiler warns about current implementation, I think it should.
>
> > Signed-off-by: Andy Shevchenko
> > ---
> > drivers/platform/x86/thinkpad_acpi.c | 9 -
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/platform/x86/thinkpad_acpi.c
> > b/drivers/platform/x86/thinkpad_acpi.c
> > index 7b6cb0c69b02..7740b5e1b998 100644
> > --- a/drivers/platform/x86/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > @@ -3090,6 +3090,8 @@ static void tpacpi_send_radiosw_update(void)
> >
> > static void hotkey_exit(void)
> > {
> > + int res;
> > +
> > #ifdef CONFIG_THINKPAD_ACPI_HOTKEY_POLL
> > mutex_lock(&hotkey_mutex);
> > hotkey_poll_stop_sync();
> > @@ -3101,11 +3103,8 @@ static void hotkey_exit(void)
> >
> > dbg_printk(TPACPI_DBG_EXIT | TPACPI_DBG_HKEY,
> > "restoring original HKEY status and mask\n");
> > - /* yes, there is a bitwise or below, we want the
> > - * functions to be called even if one of them fail */
> > - if (((tp_features.hotkey_mask &&
> > - hotkey_mask_set(hotkey_orig_mask)) |
> > - hotkey_status_set(false)) != 0)
> > + res = tp_features.hotkey_mask ?
> > hotkey_mask_set(hotkey_orig_mask) : 0;
> > + if (hotkey_status_set(false) || res)
> > pr_err("failed to restore hot key mask "
> > "to BIOS defaults\n");
> > }
>
>
--
Andy Shevchenko
Intel Finland Oy
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
[ibm-acpi-devel] [PATCH v1 3/3] platform/x86: thinkpad_acpi: Add a comment about 0 in module_param_call()
As per discussion [1] there are only few users of module_param_call() in kernel which prevent to read module parameters back. It thinkpad_acpi driver there is even no method do so. Thus, for now, add just a comment to explain why 0 is used as permissions in module_param_call(). [1]: https://patchwork.ozlabs.org/patch/713245/ Cc: Richard Weinberger Signed-off-by: Andy Shevchenko --- drivers/platform/x86/thinkpad_acpi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index e6fbb2579dd9..f5bc888b2ef4 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -9663,6 +9663,7 @@ module_param_named(enable, alsa_enable, bool, 0444); MODULE_PARM_DESC(enable, "Enable the ALSA interface for the ACPI EC Mixer"); #endif /* CONFIG_THINKPAD_ACPI_ALSA_SUPPORT */ +/* The module parameter can't be read back, that's why 0 is used here */ #define TPACPI_PARAM(feature) \ module_param_call(feature, set_ibm_param, NULL, NULL, 0); \ MODULE_PARM_DESC(feature, "Simulates thinkpad-acpi procfs command at module load, see documentation") -- 2.11.0 -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 09/11] platform: thinkpad_acpi: convert to use DRIVER_ATTR_RO/RW
On Fri, Jun 9, 2017 at 12:03 PM, Greg Kroah-Hartman
wrote:
> We are trying to get rid of DRIVER_ATTR(), and the thinkpad_acpi
> driver's attributes can be trivially changed to use DRIVER_ATTR_RO() and
> DRIVER_ATTR_RW().
Which tree is it supposed to go through?
We might need an immutable tag / branch.
P.S. Change is good to me, though let's give a chance to Darren and
Henrique to comment.
> Cc: Henrique de Moraes Holschuh
> Cc: Darren Hart
> Cc: Andy Shevchenko
> Cc:
> Cc:
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/platform/x86/thinkpad_acpi.c | 91
> +++-
> 1 file changed, 28 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 7b6cb0c69b02..f6861b551178 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -1438,25 +1438,20 @@ static int tpacpi_rfk_procfs_write(const enum
> tpacpi_rfk_id id, char *buf)
> */
>
> /* interface_version --- */
> -static ssize_t tpacpi_driver_interface_version_show(
> - struct device_driver *drv,
> - char *buf)
> +static ssize_t interface_version_show(struct device_driver *drv, char *buf)
> {
> return snprintf(buf, PAGE_SIZE, "0x%08x\n", TPACPI_SYSFS_VERSION);
> }
> -
> -static DRIVER_ATTR(interface_version, S_IRUGO,
> - tpacpi_driver_interface_version_show, NULL);
> +static DRIVER_ATTR_RO(interface_version);
>
> /* debug_level - */
> -static ssize_t tpacpi_driver_debug_show(struct device_driver *drv,
> - char *buf)
> +static ssize_t debug_level_show(struct device_driver *drv, char *buf)
> {
> return snprintf(buf, PAGE_SIZE, "0x%04x\n", dbg_level);
> }
>
> -static ssize_t tpacpi_driver_debug_store(struct device_driver *drv,
> - const char *buf, size_t count)
> +static ssize_t debug_level_store(struct device_driver *drv, const char *buf,
> +size_t count)
> {
> unsigned long t;
>
> @@ -1467,34 +1462,28 @@ static ssize_t tpacpi_driver_debug_store(struct
> device_driver *drv,
>
> return count;
> }
> -
> -static DRIVER_ATTR(debug_level, S_IWUSR | S_IRUGO,
> - tpacpi_driver_debug_show, tpacpi_driver_debug_store);
> +static DRIVER_ATTR_RW(debug_level);
>
> /* version - */
> -static ssize_t tpacpi_driver_version_show(struct device_driver *drv,
> - char *buf)
> +static ssize_t version_show(struct device_driver *drv, char *buf)
> {
> return snprintf(buf, PAGE_SIZE, "%s v%s\n",
> TPACPI_DESC, TPACPI_VERSION);
> }
> -
> -static DRIVER_ATTR(version, S_IRUGO,
> - tpacpi_driver_version_show, NULL);
> +static DRIVER_ATTR_RO(version);
>
> /* - */
>
> #ifdef CONFIG_THINKPAD_ACPI_DEBUGFACILITIES
>
> /* wlsw_emulstate -- */
> -static ssize_t tpacpi_driver_wlsw_emulstate_show(struct device_driver *drv,
> - char *buf)
> +static ssize_t wlsw_emulstate_show(struct device_driver *drv, char *buf)
> {
> return snprintf(buf, PAGE_SIZE, "%d\n", !!tpacpi_wlsw_emulstate);
> }
>
> -static ssize_t tpacpi_driver_wlsw_emulstate_store(struct device_driver *drv,
> - const char *buf, size_t count)
> +static ssize_t wlsw_emulstate_store(struct device_driver *drv, const char
> *buf,
> + size_t count)
> {
> unsigned long t;
>
> @@ -1508,22 +1497,16 @@ static ssize_t
> tpacpi_driver_wlsw_emulstate_store(struct device_driver *drv,
>
> return count;
> }
> -
> -static DRIVER_ATTR(wlsw_emulstate, S_IWUSR | S_IRUGO,
> - tpacpi_driver_wlsw_emulstate_show,
> - tpacpi_driver_wlsw_emulstate_store);
> +static DRIVER_ATTR_RW(wlsw_emulstate);
>
> /* bluetooth_emulstate - */
> -static ssize_t tpacpi_driver_bluetooth_emulstate_show(
> - struct device_driver *drv,
> - char *buf)
> +static ssize_t bluetooth_emulstate_show(struct device_driver
Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: Implement tablet mode resolving using GMMS method
On Mon, Sep 18, 2017 at 7:00 PM, Lyude Paul wrote: > Reviewed-by: Lyude Paul > > On Fri, 2017-09-15 at 15:20 +0200, Benjamin Berg wrote: >> Many thinkpad laptops and convertibles provide the GMMS method to >> resolve how far the laptop has been opened and whether it has been >> converted into tablet mode. This allows reporting a more precise tablet >> mode state to userspace. >> >> The current implementation only reports a summarized tablet mode state >> which is triggered as soon as the input devices become unusable as they >> are folded away from the display. >> >> This will work on all models where the CMMD method was used previously and >> it may also work in other cases. >> >> Thanks to Peter Zhang of Lenovo for providing information on how to use the >> GMMS method to query the tablet mode. I dunno why this one didn't appear in our patchwork database. I'll go to apply manually soon. Sorry for the delay. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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] thinkpad_acpi: Implement tablet mode resolving using GMMS method
On Sun, Oct 1, 2017 at 12:31 PM, Andy Shevchenko wrote: > On Mon, Sep 18, 2017 at 7:00 PM, Lyude Paul wrote: >> Reviewed-by: Lyude Paul >> >> On Fri, 2017-09-15 at 15:20 +0200, Benjamin Berg wrote: >>> Many thinkpad laptops and convertibles provide the GMMS method to >>> resolve how far the laptop has been opened and whether it has been >>> converted into tablet mode. This allows reporting a more precise tablet >>> mode state to userspace. >>> >>> The current implementation only reports a summarized tablet mode state >>> which is triggered as soon as the input devices become unusable as they >>> are folded away from the display. >>> >>> This will work on all models where the CMMD method was used previously and >>> it may also work in other cases. >>> >>> Thanks to Peter Zhang of Lenovo for providing information on how to use the >>> GMMS method to query the tablet mode. > > I dunno why this one didn't appear in our patchwork database. > I'll go to apply manually soon. > Sorry for the delay. Okay, now pushed to my testing queue. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCHv2] thinkpad_acpi: Implement tablet mode resolving using GMMS method
_MODE_STAND |
> + TP_ACPI_MULTI_MODE_TENT;
> + break;
> + case 5:
> + valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
> + TP_ACPI_MULTI_MODE_FLAT |
> + TP_ACPI_MULTI_MODE_TABLET |
> + TP_ACPI_MULTI_MODE_STAND |
> + TP_ACPI_MULTI_MODE_TENT;
> + break;
> + default:
> + pr_err("Unknown multi mode status type %d with value 0x%04X,
> please report this to %s\n",
> + type, value, TPACPI_MAIL);
> + return 0;
> + }
> +
> + if (has_tablet_mode && (valid_modes & TP_ACPI_MULTI_MODE_TABLET_LIKE))
> + *has_tablet_mode = 1;
> +
> + switch (value) {
> + case 1:
> + mode = TP_ACPI_MULTI_MODE_LAPTOP;
> + break;
> + case 2:
> + mode = TP_ACPI_MULTI_MODE_FLAT;
> + break;
> + case 3:
> + mode = TP_ACPI_MULTI_MODE_TABLET;
> + break;
> + case 4:
> + if (type == 1)
> + mode = TP_ACPI_MULTI_MODE_STAND_TENT;
> + else
> + mode = TP_ACPI_MULTI_MODE_STAND;
> + break;
> + case 5:
> + mode = TP_ACPI_MULTI_MODE_TENT;
> + break;
> + default:
> + if (type == 5 && value == 0x) {
> + pr_warn("Multi mode status is undetected, assuming
> laptop\n");
> + return 0;
> + }
> + }
> +
> + if (!(mode & valid_modes)) {
> + pr_err("Unknown/reserved multi mode value 0x%04X for type %d,
> please report this to %s\n",
> + value, type, TPACPI_MAIL);
> + return 0;
> + }
> +
> + return !!(mode & TP_ACPI_MULTI_MODE_TABLET_LIKE);
> +}
> +
> static int hotkey_get_tablet_mode(int *status)
> {
> int s;
> @@ -2077,11 +2183,11 @@ static int hotkey_get_tablet_mode(int *status)
>
> *status = ((s & TP_HOTKEY_TABLET_MASK) != 0);
> break;
> - case TP_HOTKEY_TABLET_USES_CMMD:
> - if (!acpi_evalf(ec_handle, &s, "CMMD", "d"))
> + case TP_HOTKEY_TABLET_USES_GMMS:
> + if (!acpi_evalf(hkey_handle, &s, "GMMS", "dd", 0))
> return -EIO;
>
> - *status = (s == TP_EC_CMMD_TABLET_MODE);
> + *status = hotkey_gmms_get_tablet_mode(s, NULL);
> break;
> default:
> break;
> @@ -3113,16 +3219,19 @@ static int hotkey_init_tablet_mode(void)
> int in_tablet_mode = 0, res;
> char *type = NULL;
>
> - if (acpi_evalf(hkey_handle, &res, "MHKG", "qd")) {
> + if (acpi_evalf(hkey_handle, &res, "GMMS", "qdd", 0)) {
> + int has_tablet_mode;
> +
> + in_tablet_mode = hotkey_gmms_get_tablet_mode(res,
> +
> &has_tablet_mode);
> + if (has_tablet_mode)
> + tp_features.hotkey_tablet =
> TP_HOTKEY_TABLET_USES_GMMS;
> + type = "GMMS";
> + } else if (acpi_evalf(hkey_handle, &res, "MHKG", "qd")) {
> /* For X41t, X60t, X61t Tablets... */
> tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_MHKG;
> in_tablet_mode = !!(res & TP_HOTKEY_TABLET_MASK);
> type = "MHKG";
> - } else if (acpi_evalf(ec_handle, &res, "CMMD", "qd")) {
> - /* For X1 Yoga (2016) */
> - tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_CMMD;
> - in_tablet_mode = res == TP_EC_CMMD_TABLET_MODE;
> - type = "CMMD";
> }
>
> if (!tp_features.hotkey_tablet)
> --
> 2.13.6
>
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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] platform/x86: thinkpad_acpi: Accept flat mode for type 4 multi mode status
On Tue, Nov 14, 2017 at 6:14 PM, Benjamin Berg wrote: > On the X1 Yoga 2nd Generation and most likely other notebooks the FLAT > mode is reported. Decode it correctly rather than warning about an > unexpected multi mode status to be reported. If we can't check on all available hardware, better to avoid "doing for all" fixes. I suppose DMI match can help here. It would be nice to hear from Henrique and others as well. > case 4: > - valid_modes = TP_ACPI_MULTI_MODE_LAPTOP | > - TP_ACPI_MULTI_MODE_TABLET | > - TP_ACPI_MULTI_MODE_STAND | > - TP_ACPI_MULTI_MODE_TENT; > - break; The common practice is to put /* fallthrough */ instead. > case 5: > + /* In mode 4, FLAT is not specified as a valid mode. However, > +* it can be seen at least on the X1 Yoga 2nd Generation. > +*/ We don't use network subsystem style of comments. > valid_modes = TP_ACPI_MULTI_MODE_LAPTOP | > TP_ACPI_MULTI_MODE_FLAT | > TP_ACPI_MULTI_MODE_TABLET | > TP_ACPI_MULTI_MODE_STAND | > TP_ACPI_MULTI_MODE_TENT; > break; -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 0/1] Use checkpatch.pl to make thinkpad_acpi.c error free: octal permissions
On Wed, Nov 15, 2017 at 1:14 AM, Simranjit Singh wrote: > From: Simranjit Singh > > Using the checkpatch.pl script, there were 8 errors in thinkpad_acpi.c. I > fixed them by changing permissions to octal, > and by adding parenthesis. > On the current tree, if you use checkpatch.pl, thinkpad_acpi.c will have 8 > errors. With my changes, there are 0 errors. > However, I have added 3 new warnings. > I'm very new to kernel development, so if I messed up somewhere, please do > tell me. > > Changed binary permissions to octal in accordance with checkpatch.pl in > thinkpad_acpi.c Besides comments you got already, keep in mind that patch numbering starts from 1, 0 is a special dedicated value for cover letter which you apparently miss. Of course, in case of this "series" you are not supposed to make it as a series. And one more time, it's a contributor's responsibility to test (*) changes before sending _by default _. *) sometimes compilation test is enough, though most of the times the real run would be required. As per change 0 to 0444 for the cases you did, you obviously didn't read the code and thus may not understand the ABI of this module. So, obvious NAK for this patch. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 1/1] Use checkpatch.pl to make thinkpad_acpi.c error free: add parenthesis to complex macros
On Wed, Nov 15, 2017 at 1:14 AM, Simranjit Singh wrote: > From: Simranjit Singh > > Added parentheses around complex macros following checkpatch.pl for > thinkpad_acpi.c Same story. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 1/3 v5] battery: Add the battery hooking API
On Sun, Dec 10, 2017 at 8:01 PM, Ognjen Galic wrote: > drivers/acpi/battery.h | 11 > include/acpi/battery.h | 21 Just a hint to the future: use -M -C (with perhaps some non-default thresholds in some cases) to avoid above. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 3/3 v5] thinkpad_acpi: Add support for battery thresholds
supply subsystem?
> +
> + if (!supply) {
> + pr_crit("Can't upcast to power_supply!");
I recommend to reconsider levels on which you print some messages
here. Do we really need KERN_CRIT?
> + return -ENODEV;
> + }
> +
> + if (battery_info.individual_addressing)
> + /* BAT_PRIMARY or BAT_SECONDARY */
> + battery = tpacpi_battery_get_id(supply->desc->name);
> + else
> + battery = BAT_PRIMARY;
> +
> + if (kstrtol(buf, 10, &value))
...ul()?
> + return -EINVAL;
> +
> + attr = tpacpi_battery_get_attr(devattr);
> +
> + value = value == 100 ? 0 : value;
if (value == 100)
value = 0;
> +}
> +
> +static ssize_t tpacpi_battery_show(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + int ret = 0x0, attr, battery;
> + struct power_supply *supply =
> + container_of(dev, struct power_supply, dev);
See above.
> +}
> +
> +static const struct device_attribute battery_start = {
> + .show = tpacpi_battery_show,
> + .store = tpacpi_battery_store,
> + .attr = {
> + .name = START_ATTR,
> + .mode = 0644,
> + },
> +};
> +
> +static const struct device_attribute battery_stop = {
> + .show = tpacpi_battery_show,
> + .store = tpacpi_battery_store,
> + .attr = {
> + .name = STOP_ATTR,
> + .mode = 0644,
> + },
> +};
Don't we have some helper macros for above?
Something based on __ATTR_RW() ?
> +static int tpacpi_battery_add(struct power_supply *battery)
> +{
> + int batteryid = tpacpi_battery_get_id(battery->desc->name);
> +
> + pr_info("battery %s added", battery->desc->name);
> +
> + if (tpacpi_battery_probe(batteryid))
> + return -ENODEV;
> +
> + if (device_create_file(&battery->dev, &battery_start))
> + return -ENODEV;
> +
> + if (device_create_file(&battery->dev, &battery_stop))
> + return -ENODEV;
Shouldn't be done via group attribures?
> +
> + return 0;
> +}
> +static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
> +{
> + tp_features.battery = 0;
> + battery_hook_register(&battery_hook);
> +
> + pr_info("battery driver initialized");
This kind of messages is somehow useless. 'initcall_debug' will show you this.
> +
> + battery_info.individual_addressing = 0;
> + tp_features.battery = 1;
> +
> + return 0;
> +}
> +
> +static void tpacpi_battery_exit(void)
> +{
> + if (!tp_features.battery) {
> + pr_info("battery driver was not loaded, not de-registering");
> + return;
> + }
When might it happen?
> +
> + battery_hook_unregister(&battery_hook);
> +}
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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] platform/x86: thinkpad_acpi: Accept flat mode for type 4 multi mode status
On Tue, Nov 14, 2017 at 6:14 PM, Benjamin Berg wrote:
> On the X1 Yoga 2nd Generation and most likely other notebooks the FLAT
> mode is reported. Decode it correctly rather than warning about an
> unexpected multi mode status to be reported.
>
> Signed-off-by: Benjamin Berg
> Cc: Peter FP1 Zhang Cc: Lyude
Applied to my review and testing queue, thanks!
> ---
> drivers/platform/x86/thinkpad_acpi.c | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 91fab1a13a6d..d23c84222e9f 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -2106,26 +2106,24 @@ static int hotkey_gmms_get_tablet_mode(int s, int
> *has_tablet_mode)
> TP_ACPI_MULTI_MODE_FLAT |
> TP_ACPI_MULTI_MODE_TABLET |
> TP_ACPI_MULTI_MODE_STAND |
> TP_ACPI_MULTI_MODE_TENT;
> break;
> case 3:
> valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
> TP_ACPI_MULTI_MODE_FLAT;
> break;
> case 4:
> - valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
> - TP_ACPI_MULTI_MODE_TABLET |
> - TP_ACPI_MULTI_MODE_STAND |
> - TP_ACPI_MULTI_MODE_TENT;
> - break;
> case 5:
> + /* In mode 4, FLAT is not specified as a valid mode. However,
> +* it can be seen at least on the X1 Yoga 2nd Generation.
> +*/
> valid_modes = TP_ACPI_MULTI_MODE_LAPTOP |
> TP_ACPI_MULTI_MODE_FLAT |
> TP_ACPI_MULTI_MODE_TABLET |
> TP_ACPI_MULTI_MODE_STAND |
> TP_ACPI_MULTI_MODE_TENT;
> break;
> default:
> pr_err("Unknown multi mode status type %d with value 0x%04X,
> please report this to %s\n",
>type, value, TPACPI_MAIL);
> return 0;
> --
> 2.13.6
>
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 2/3 v6] battery: Add the ThinkPad "Not Charging" quirk
On Mon, Dec 18, 2017 at 12:21 PM, Ognjen Galić wrote: > On 16/12/2017, Rafael J. Wysocki wrote: >> I would reorder it as the first patch in the series, then, because the >> other two are more related to each other. > Do I really need now to re-order and re-send the whole patchset for it > to get merged? Through which tree it's supposed to go? -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 2/3 v6] battery: Add the ThinkPad "Not Charging" quirk
On Mon, Dec 18, 2017 at 7:31 PM, Rafael J. Wysocki wrote: > On Mon, Dec 18, 2017 at 1:36 PM, Andy Shevchenko > wrote: >> On Mon, Dec 18, 2017 at 12:21 PM, Ognjen Galić wrote: >>> On 16/12/2017, Rafael J. Wysocki wrote: >> >>>> I would reorder it as the first patch in the series, then, because the >>>> other two are more related to each other. >> >>> Do I really need now to re-order and re-send the whole patchset for it >>> to get merged? >> >> Through which tree it's supposed to go? > > Wait, wait. I haven't commented patches [1,3/3] yet, so no ACKs > assumed, please. I would prefer if you can take it through linux-pm. Whenever you are okay with the series let me know I would review PDx86 bits. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 0/2] platform/x86/thinkpad_acpi: Adjustments for four function implementations
On Mon, Dec 18, 2017 at 11:26 PM, SF Markus Elfring wrote: > From: Markus Elfring > Date: Mon, 18 Dec 2017 22:23:45 +0100 > > Two update suggestions were taken into account > from static source code analysis. > > Markus Elfring (2): > Delete an error message for a failed memory allocation in three functions This one is questionable since it prints error messages at ->init() stage. I would rather not touch this. > Improve a size determination in tpacpi_new_rfkill() Doesn't make any sense right now. One style over the other. Nothing gets better or worth at this point. Sorry, but NAK for both. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] platform/x86/thinkpad_acpi: Adjustments for four function implementations
On Tue, Dec 19, 2017 at 6:23 PM, SF Markus Elfring wrote: >>> Delete an error message for a failed memory allocation in three functions >> >> This one is questionable since it prints error messages at ->init() stage. >> I would rather not touch this. > > Do you find the Linux allocation failure report insufficient in this case? It is up to the current driver maintainer. For me it sounds better to have than not to. >>> Improve a size determination in tpacpi_new_rfkill() >> >> Doesn't make any sense right now. One style over the other. >> Nothing gets better or worth at this point. > > Would you like to care for a bit more compliance with information > from the section “14) Allocating memory” in the document “coding-style.rst”? You know, in Russian we have an adverb: Заставь дурака б-гу молиться, он и лоб расшибёт. Which has English equivalent: Give a man enough rope and he’ll hang himself. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 2/3 v8] thinkpad_acpi: Add support for battery thresholds
On Thu, Dec 21, 2017 at 12:00 PM, Ognjen Galic wrote:
> thinkpad_acpi registers two new attributes for each battery:
>
> 1) Charge start threshold
> /sys/class/power_supply/BATN/charge_start_threshold
>
> Valid values are [0, 99]. A value of 0 turns off the
> start threshold wear control.
>
> 2) Charge stop threshold
> /sys/class/power_supply/BATN/charge_stop_threshold
>
> Valid values are [1, 100]. A value of 100 turns off
> the stop threshold wear control. This must be
> configured first.
>
> This patch depends on the following patches:
>
> "battery: Add the battery hooking API"
Sorry, didn't notice earlier some things commented below.
> #include
>
> +
> /* ThinkPad CMOS commands */
Redundant change.
> +enum {
> + /* Error condition bit */
> + METHOD_ERR = (1 << 31),
bitops.h but no BIT() use?
BIT(31) ?
> +};
> +static int tpacpi_battery_set(int what, int battery, int value)
> +{
> +
Redundant.
> + int param = 0x0, ret = 0x;
Useless assignment for param, not sure abour ret.
> +
> + /* The first 8 bits are the value of the threshold */
> + param = value;
> + /* The battery ID is in bits 8-9, 2 bits */
> + param |= battery << 8;
> +
> + switch (what) {
> +
> + case THRESHOLD_START:
> +
> + if (tpacpi_battery_acpi_eval(SET_START, &ret, param)) {
> + pr_err("failed to set charge threshold on battery %d",
> + battery);
> + return -ENODEV;
> + }
> +
> + return 0;
> +
> + case THRESHOLD_STOP:
> +
> + if (tpacpi_battery_acpi_eval(SET_STOP, &ret, param)) {
> + pr_err("failed to set charge stop threshold: %d",
> battery);
> + return -ENODEV;
> + }
> +
> + return 0;
> +
> + default:
> + pr_crit("wrong parameter: %d", what);
> + return -EINVAL;
> + }
> +
Redundant.
> +}
> +
> +static int tpacpi_battery_probe(int battery)
> +{
> + int ret = 0;
> +
> + /* Reset the struct */
Useless.
> + memset(&battery_info, 0, sizeof(struct tpacpi_battery_driver_data));
> +}
> +static ssize_t tpacpi_battery_store(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + int attr, battery;
> + unsigned long value;
> + struct power_supply *supply = to_power_supply(dev);
Can you use reverse xmas tree, esp. put assignments first.
> + if (!supply) {
How this could be possible?!
> + pr_err("Can't upcast to power_supply!");
> + return -ENODEV;
> + }
> + if (kstrtoul(buf, 10, &value))
> + return -EINVAL;
Don't shadow an error code from kstrtoul().
> +static ssize_t tpacpi_battery_show(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + int ret = 0x0, attr, battery;
> + struct power_supply *supply = to_power_supply(dev);
> + if (!supply) {
How this could be possible?!
> + pr_crit("Can't upcast to power_supply!");
> + return -ENODEV;
> + }
> + return sprintf(buf, "%d\n", ret);
> +}
> +
> +DEVICE_ATTR(charge_start_threshold, 0644,
> + tpacpi_battery_show, tpacpi_battery_store);
> +DEVICE_ATTR(charge_stop_threshold, 0644,
> + tpacpi_battery_show, tpacpi_battery_store);
DEVICE_ATTR_RW().
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -371,6 +371,8 @@ devm_power_supply_register_no_ws(struct device *parent,
> extern void power_supply_unregister(struct power_supply *psy);
> extern int power_supply_powers(struct power_supply *psy, struct device *dev);
>
> +#define to_power_supply(device) container_of(device, struct power_supply,
> dev)
> +
> extern void *power_supply_get_drvdata(struct power_supply *psy);
> /* For APM emulation, think legacy userspace. */
> extern struct class *power_supply_class;
This one sounds to me as a separate change.
At the same time you may convert the current user of it to make sense
of the change.
drivers/power/supply/power_supply_core.c:671:
I think I pointed to this out once.
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 1/3 v8] battery: Add the battery hooking API
On Thu, Dec 21, 2017 at 12:00 PM, Ognjen Galic wrote: > This is a patch that implements a generic hooking API for the > generic ACPI battery driver. > > With this new generic API, drivers can expose platform specific > behaviour via sysfs attributes in /sys/class/power_supply/BATn/ > in a generic way. > > A perfect example of the need for this API are Lenovo ThinkPads. > > Lenovo ThinkPads have a ACPI extension that allows the setting of > start and stop charge thresholds in the EC and battery firmware > via ACPI. The thinkpad_acpi module can use this API to expose > sysfs attributes that it controls inside the ACPI battery driver > sysfs tree, under /sys/class/power_supply/BATN/. > > The file drivers/acpi/battery.h has been moved to > include/acpi/battery.h and the includes inside ac.c, sbs.c, and > battery.c have been adjusted to reflect that. > > When drivers hooks into the API, the API calls add_battery() for > each battery in the system that passes it a acpi_battery > struct. Then, the drivers can use device_create_file() to create > new sysfs attributes with that struct and identify the batteries > for per-battery attributes. > > v7: > * Implemented mutual exclusion for hooking methods and battery > callbacks > * Fixed a BUG where errors in other modules would occur when the > modules that depend on battery get unloaded > v8: > * Use list_for_each_safe instead of list_for_each for the module > exit function where deletion of nodes occurs Just two workflow / style comments: - usually we put changelog under --- line; - when you prepare patch series you may use -v to create a version automatically. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 2/3 v8] thinkpad_acpi: Add support for battery thresholds
On Thu, Dec 21, 2017 at 5:24 PM, Ognjen Galić wrote: > On 21 Dec 2017 2:53 pm, "Andy Shevchenko" wrote: > On Thu, Dec 21, 2017 at 12:00 PM, Ognjen Galic wrote: You need stop using HTML as well. >> +DEVICE_ATTR(charge_start_threshold, 0644, >> + tpacpi_battery_show, tpacpi_battery_store); >> +DEVICE_ATTR(charge_stop_threshold, 0644, >> + tpacpi_battery_show, tpacpi_battery_store); > > DEVICE_ATTR_RW(). > I did not use DEVICE_ATTR_RW() because I can't use a common store and show > function for both attributes to minimize the code I need. If I used > DEVICE_ATTR_RW() I would need to define 4 more functions that do the same. > That seems redundant. _RW() variant still preferable since it makes much more easy to understand that this attribute doesn't need any *special* permissions. I think it's not big payment to make it so. >> +#define to_power_supply(device) container_of(device, struct power_supply, >> dev) >> This one sounds to me as a separate change. > At the same time you may convert the current user of it to make sense > of the change. > drivers/power/supply/power_supply_core.c:671: > > I think I pointed to this out once. > I wanted to minimize the changes in pm to avoid going through all the review > process hassle for a few simple changes, because I'm modifying a third > subsystem. But I will do it later tonight. Thanks! > This is my first patchset and I did not expect for the review process to be > this agonizingly slow, so I wanted to speed it up by not touching pm. I see, though you actually made an opposite by that decision :-) -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 v10 2/4] pm: add to_power_supply macro to the API
On Sat, Dec 23, 2017 at 12:53 PM, Ognjen Galic wrote:
> This patch adds the to_power_supply macro to upcast
> a device to a power_supply struct.
>
> This is needed because the same piece of code using
> container_of is used in various other places, so we
> abstract away such low-level operations via a macro.
> ---
This is wrong! You have to use *existing* --- line below. Otherwise
all mail parsers will cut this out including your SoB tag.
>
> v9:
> * Split the pm changes from the thinkpad_acpi patch
> into its own patch
>
> v10:
> * No changes in this patch in v10
>
> Signed-off-by: Ognjen Galic
Missed:
Suggested-by: Andy Shevchenko
> ---
> drivers/power/supply/power_supply_core.c | 2 +-
> include/linux/power_supply.h | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/supply/power_supply_core.c
> b/drivers/power/supply/power_supply_core.c
> index 82f998a..feac7b0 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -668,7 +668,7 @@ EXPORT_SYMBOL_GPL(power_supply_powers);
>
> static void power_supply_dev_release(struct device *dev)
> {
> - struct power_supply *psy = container_of(dev, struct power_supply,
> dev);
> + struct power_supply *psy = to_power_supply(dev);
> dev_dbg(dev, "%s\n", __func__);
> kfree(psy);
> }
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 79e90b3..f0139b4 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -371,6 +371,8 @@ devm_power_supply_register_no_ws(struct device *parent,
> extern void power_supply_unregister(struct power_supply *psy);
> extern int power_supply_powers(struct power_supply *psy, struct device *dev);
>
> +#define to_power_supply(device) container_of(device, struct power_supply,
> dev)
> +
> extern void *power_supply_get_drvdata(struct power_supply *psy);
> /* For APM emulation, think legacy userspace. */
> extern struct class *power_supply_class;
Should fold in the changes you sent as a separate patch.
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 v10 2/4] pm: add to_power_supply macro to the API
On Sat, Dec 30, 2017 at 1:49 AM, Ognjen Galić wrote: > On Čet, 2017-12-28 at 10:19 +0200, Andy Shevchenko wrote: >> On Sat, Dec 23, 2017 at 12:53 PM, Ognjen Galic >> wrote: >> > >> > This patch adds the to_power_supply macro to upcast >> > a device to a power_supply struct. >> > >> > This is needed because the same piece of code using >> > container_of is used in various other places, so we >> > abstract away such low-level operations via a macro. >> > >> > --- >> This is wrong! You have to use *existing* --- line below. Otherwise >> all mail parsers will cut this out including your SoB tag. >> > > My bad I guess. Want another patch revision with that fixed or > something? > You definitely need to send a new revision with all comments addressed. So far you didn't responce to them which I recognize as total agreement that they have to be addressed. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 v10 2/4] pm: add to_power_supply macro to the API
On Sun, Dec 31, 2017 at 1:17 PM, Rafael J. Wysocki wrote: > On Sun, Dec 31, 2017 at 10:37 AM, Andy Shevchenko > wrote: >> On Sat, Dec 30, 2017 at 1:49 AM, Ognjen Galić wrote: >> You definitely need to send a new revision with all comments >> addressed. So far you didn't responce to them which I recognize as >> total agreement that they have to be addressed. > > I asked for not sending new versions before I have a chance to look at > the current one in detail, but enough time has passed after the last > one, so I agree. Sending a new version at this point won't hurt. :-) Thanks, Rafael, for clarification, that's exactly what I kept in mind when proposing a new version (I'm concerned as well of frequency of patch series to be not so high). -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 v11 1/5] battery: Add the battery hooking API
On Sun, Dec 31, 2017 at 4:16 PM, Ognjen Galic wrote:
> This is a patch that implements a generic hooking API for the
> generic ACPI battery driver.
>
> With this new generic API, drivers can expose platform specific
> behaviour via sysfs attributes in /sys/class/power_supply/BATn/
> in a generic way.
>
> A perfect example of the need for this API are Lenovo ThinkPads.
>
> Lenovo ThinkPads have a ACPI extension that allows the setting of
> start and stop charge thresholds in the EC and battery firmware
> via ACPI. The thinkpad_acpi module can use this API to expose
> sysfs attributes that it controls inside the ACPI battery driver
> sysfs tree, under /sys/class/power_supply/BATN/.
>
> The file drivers/acpi/battery.h has been moved to
> include/acpi/battery.h and the includes inside ac.c, sbs.c, and
> battery.c have been adjusted to reflect that.
>
> When drivers hooks into the API, the API calls add_battery() for
> each battery in the system that passes it a acpi_battery
> struct. Then, the drivers can use device_create_file() to create
> new sysfs attributes with that struct and identify the batteries
> for per-battery attributes.
Thanks for an update. Few comments below.
(Keep in mind, please, that we are not expecting new version
immediately, and Rafael might also apply this one without changes,
since they are minors)
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -30,6 +31,7 @@
> #include
> #include
> #include
> +#include
Just a nit: perhaps add this before module.h ?
> +void __battery_hook_unregister(struct acpi_battery_hook *hook, int force)
> +{
> + struct list_head *position;
> + struct acpi_battery *battery;
> +
> + /*
> +* In order to remove a hook, we first need to
> +* de-register all the batteries that are registered.
> +*/
> +
> + if (!force)
I'm not sure force is a good name here, perhaps "locked"?
Btw, would it be boolean?
> + mutex_lock(&hook_mutex);
> + list_for_each(position, &acpi_battery_list) {
> + battery = list_entry(position, struct acpi_battery, list);
> + hook->remove_battery(battery->bat);
> + }
list_for_each_entry()
> +
> + /* Then, just remove the hook */
> +
> + list_del(&hook->list);
> +
> + if (!force)
> + mutex_unlock(&hook_mutex);
> +
> + pr_info("battery: extension unregistered: %s\n", hook->name);
> +}
> +void battery_hook_register(struct acpi_battery_hook *hook)
> +{
> + list_for_each(position, &acpi_battery_list) {
> + battery = list_entry(position, struct acpi_battery, list);
list_for_each_entry()
> + __battery_hook_unregister(hook, 1);
> + return;
> +
Perhaps you meant empty line followed by return statement.
> + }
> + }
> +
> + pr_info("battery: new extension: %s\n", hook->name);
Perhaps move "battery:" to pr_fmt() ?
I have counted already 3 prints with it and see more below.
> + list_for_each(position, &battery_hook_list) {
> + hook_node = list_entry(position, struct acpi_battery_hook,
> list);
list_for_each_entry()
> + list_for_each(position, &battery_hook_list) {
> + hook = list_entry(position, struct acpi_battery_hook, list);
Ditto.
> + list_for_each_safe(position, temp, &battery_hook_list) {
> + hook = list_entry(position, struct acpi_battery_hook, list);
Ditto.
> --- a/drivers/acpi/battery.h
> +++ /dev/null
Apparently you didn't apply -M -C (perhaps with some non-default
threshold) to avoid such thing in the patch.
> --- /dev/null
> +++ b/include/acpi/battery.h
Ditto.
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 v11 2/5] pm: add to_power_supply macro to the API
On Sun, Dec 31, 2017 at 4:17 PM, Ognjen Galic wrote: > This patch adds the to_power_supply macro to upcast > a device to a power_supply struct. > > This is needed because the same piece of code using > container_of is used in various other places, so we > abstract away such low-level operations via a macro. The patch 5 must be folded into this one. I told you that. Otherwise you will leave bisectability issue. > Suggested-by: Andy Shevchenko > Signed-off-by: Ognjen Galic > > v9: > * Split the pm changes from the thinkpad_acpi patch > into its own patch > > v10: > * No changes in this patch in v10 > > v11: > * Fix changelog formatting Changelog should go *after* --- line... > --- ...to here. > drivers/power/supply/power_supply_core.c | 2 +- > include/linux/power_supply.h | 2 ++ -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 v11 5/5] pm: Remove to_power_supply impl in ds278*
On Sun, Dec 31, 2017 at 4:17 PM, Ognjen Galic wrote: > This patch fixes a issue introduced by patch: > pm: add to_power_supply macro to the API > > In that patch a new macro has been defined in the > power_supply.h header with the same name as the private > function name in the ds278* battery modules that does the > same thing, thus clashing and producing build errors. > > This patch removes that private implementation. Must be folded into patch 2. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 v11 3/5] thinkpad_acpi: Add support for battery thresholds
Please, get rid of all useless empty lines like this one, or above (in
between of two if:s).
> + }
> +/* General helper functions */
> +
> +static int tpacpi_battery_get_id(const char *battery_name)
> +{
> +
> + if (strcmp(battery_name, "BAT0") == 0)
> + return BAT_PRIMARY;
> + if (strcmp(battery_name, "BAT1") == 0)
> + return BAT_SECONDARY;
> +
> + /*
> +* If for some reason the battery is not BAT0 nor is it
> +* BAT1, we will assume it's the default, first battery,
> +* AKA primary.
> +*/
> + pr_warn("unknown battery %s, assuming primary", battery_name);
> + return BAT_PRIMARY;
> +}
> +
> +/* sysfs interface */
> +
> +static ssize_t tpacpi_battery_store(int what,
> + struct device *dev,
> + const char *buf, size_t count)
> +{
> + struct power_supply *supply = to_power_supply(dev);
> + unsigned long value;
> + int battery, errno;
> + errno = kstrtoul(buf, 10, &value);
> +
Dont' split lines like these where one returns some error code
followed by conditional check.
> + if (errno)
> + return errno;
Besides, errno is a bad name, please consider what is used elsewhere
in this module (rc, ret, rval, ...?).
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 v12 1/4] battery: Add the battery hooking API
On Wed, Jan 3, 2018 at 1:58 PM, Ognjen Galic wrote:
> This is a patch that implements a generic hooking API for the
> generic ACPI battery driver.
>
> With this new generic API, drivers can expose platform specific
> behaviour via sysfs attributes in /sys/class/power_supply/BATn/
> in a generic way.
>
> A perfect example of the need for this API are Lenovo ThinkPads.
>
> Lenovo ThinkPads have a ACPI extension that allows the setting of
> start and stop charge thresholds in the EC and battery firmware
> via ACPI. The thinkpad_acpi module can use this API to expose
> sysfs attributes that it controls inside the ACPI battery driver
> sysfs tree, under /sys/class/power_supply/BATN/.
>
> The file drivers/acpi/battery.h has been moved to
> include/acpi/battery.h and the includes inside ac.c, sbs.c, and
> battery.c have been adjusted to reflect that.
>
> When drivers hooks into the API, the API calls add_battery() for
> each battery in the system that passes it a acpi_battery
> struct. Then, the drivers can use device_create_file() to create
> new sysfs attributes with that struct and identify the batteries
> for per-battery attributes.
Thanks for an update. I have couple of minors. Otherwise look pretty much good!
> drivers/acpi/battery.h | 11
> include/acpi/battery.h | 21 +++
There are -M and -C command line parameters to git format-patch.
They can take an optional argument (percentage) of threshold.
Playing with those numbers you can achieve
rename ...
line and see actual diff.
No need to resend because of this. Just an explanation for the future Git work.
> +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
> +{
> + struct list_head *position;
> + struct acpi_battery *battery;
Missed empty line?
> + /*
> +* In order to remove a hook, we first need to
> +* de-register all the batteries that are registered.
> +*/
> + if (lock)
> + mutex_lock(&hook_mutex);
> + list_for_each(position, &acpi_battery_list) {
> + battery = list_entry(position, struct acpi_battery, list);
list_for_each_enrty() ?
Or I'm missing something why it can't be used?
> + hook->remove_battery(battery->bat);
> + }
> +void battery_hook_register(struct acpi_battery_hook *hook)
> +{
> + struct acpi_battery *battery;
> + list_for_each_entry(battery, &acpi_battery_list, list) {
> + if (hook->add_battery(battery->bat)) {
> + /*
> +* If a add-battery returns non-zero,
> +* the registration of the extension has failed,
> +* and we will not add it to the list of loaded
> +* hooks.
> +*/
> + pr_err("extension failed to load: %s",
> + hook->name);
Can it fit 80 characters?
I mean if it would be one line...
> + __battery_hook_unregister(hook, 0);
> + return;
> + }
> + }
> + pr_info("new extension: %s\n", hook->name);
> + mutex_unlock(&hook_mutex);
> +}
> +static void battery_hook_add_battery(struct acpi_battery *battery)
> +{
> + /*
> +* This function gets called right after the battery sysfs
> +* attributes have been added, so that the drivers that
> +* define custom sysfs attributes can add their own.
> +*/
Perhaps it should go before function definition.
> + struct acpi_battery_hook *hook_node;
> +}
> +static void __exit battery_hook_exit(void)
> +{
> + struct acpi_battery_hook *hook;
> + struct acpi_battery_hook *ptr;
Missed empty line?
> + /*
> +* At this point, the acpi_bus_unregister_driver
> +* has called remove for all batteries. We just
> +* need to remove the hooks.
> +*/
A common pattern to use func() [note parens] when refer to function.
> + list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) {
> + __battery_hook_unregister(hook, 1);
> + }
> + mutex_destroy(&hook_mutex);
> +}
> battery->bat = power_supply_register_no_ws(&battery->device->dev,
> &battery->bat_desc, &psy_cfg);
>
> + battery_hook_add_battery(battery);
> +
> if (IS_ERR(battery->bat)) {
> int result = PTR_ERR(battery->bat);
I'm not sure why you need to add hook when power_supply_register_no_ws() failed.
> }
> -
> +
Re: [ibm-acpi-devel] [PATCH v12 2/4] pm: add to_power_supply macro to the API
On Wed, Jan 3, 2018 at 1:59 PM, Ognjen Galic wrote:
> This patch adds the to_power_supply macro to upcast
> a device to a power_supply struct.
>
> This is needed because the same piece of code using
> container_of is used in various other places, so we
> abstract away such low-level operations via a macro.
>
> Suggested-by: Andy Shevchenko
> Signed-off-by: Ognjen Galic
It took me a while to understand if those private implementations are
equivalent to what you introduced.
Looks so sysfs_create_group() adds the attributes to power supply
device kobject, thus the struct device pointer in the callback will be
power supply child, not the actual hardware which seems correct.
Reviewed-by: Andy Shevchenko
> ---
>
> Notes:
> v9:
> * Split the pm changes from the thinkpad_acpi patch
> into its own patch
>
> v10:
> * No changes in this patch in v10
>
> v11:
> * Fix changelog formatting
>
> v12:
> * Fix build issues in ds2781 and ds2780 battery drivers
>
> drivers/power/supply/ds2780_battery.c| 5 -
> drivers/power/supply/ds2781_battery.c| 5 -
> drivers/power/supply/power_supply_core.c | 2 +-
> include/linux/power_supply.h | 2 ++
> 4 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/power/supply/ds2780_battery.c
> b/drivers/power/supply/ds2780_battery.c
> index e5d81b4..370e910 100644
> --- a/drivers/power/supply/ds2780_battery.c
> +++ b/drivers/power/supply/ds2780_battery.c
> @@ -56,11 +56,6 @@ to_ds2780_device_info(struct power_supply *psy)
> return power_supply_get_drvdata(psy);
> }
>
> -static inline struct power_supply *to_power_supply(struct device *dev)
> -{
> - return dev_get_drvdata(dev);
> -}
> -
> static inline int ds2780_battery_io(struct ds2780_device_info *dev_info,
> char *buf, int addr, size_t count, int io)
> {
> diff --git a/drivers/power/supply/ds2781_battery.c
> b/drivers/power/supply/ds2781_battery.c
> index efe83ef..d1b5a19 100644
> --- a/drivers/power/supply/ds2781_battery.c
> +++ b/drivers/power/supply/ds2781_battery.c
> @@ -54,11 +54,6 @@ to_ds2781_device_info(struct power_supply *psy)
> return power_supply_get_drvdata(psy);
> }
>
> -static inline struct power_supply *to_power_supply(struct device *dev)
> -{
> - return dev_get_drvdata(dev);
> -}
> -
> static inline int ds2781_battery_io(struct ds2781_device_info *dev_info,
> char *buf, int addr, size_t count, int io)
> {
> diff --git a/drivers/power/supply/power_supply_core.c
> b/drivers/power/supply/power_supply_core.c
> index 82f998a..feac7b0 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -668,7 +668,7 @@ EXPORT_SYMBOL_GPL(power_supply_powers);
>
> static void power_supply_dev_release(struct device *dev)
> {
> - struct power_supply *psy = container_of(dev, struct power_supply,
> dev);
> + struct power_supply *psy = to_power_supply(dev);
> dev_dbg(dev, "%s\n", __func__);
> kfree(psy);
> }
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 79e90b3..f0139b4 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -371,6 +371,8 @@ devm_power_supply_register_no_ws(struct device *parent,
> extern void power_supply_unregister(struct power_supply *psy);
> extern int power_supply_powers(struct power_supply *psy, struct device *dev);
>
> +#define to_power_supply(device) container_of(device, struct power_supply,
> dev)
> +
> extern void *power_supply_get_drvdata(struct power_supply *psy);
> /* For APM emulation, think legacy userspace. */
> extern struct class *power_supply_class;
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 v12 3/4] thinkpad_acpi: Add support for battery thresholds
On Wed, Jan 3, 2018 at 1:59 PM, Ognjen Galic wrote: > thinkpad_acpi registers two new attributes for each battery: > > 1) Charge start threshold > /sys/class/power_supply/BATN/charge_start_threshold > > Valid values are [0, 99]. A value of 0 turns off the > start threshold wear control. > > 2) Charge stop threshold > /sys/class/power_supply/BATN/charge_stop_threshold > > Valid values are [1, 100]. A value of 100 turns off > the stop threshold wear control. This must be > configured first. > > Signed-off-by: Ognjen Galic Thanks for an update! > --- > > Notes: > * Change int to acpi_status in tpacpi_battery_acpi_eval Then you need to check return code with ACPI_FAILURE() or ACPI_SUCCESS() macros. That is one of the additional burden while I can't see usefulness of ACPI return codes in that function and why I suggested to use plain int. One more question: why don't you use dev_err()/dev_*() macros instead of pr_*() ones? (Note, it's a question, needs a bit of discussion, I would like to hear a rationale of this, I think it might be one) -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 v12 4/4] battery: Add the ThinkPad "Not Charging" quirk
On Wed, Jan 3, 2018 at 1:59 PM, Ognjen Galic wrote:
> The EC/ACPI firmware on Lenovo ThinkPads used to report a status
> of "Unknown" when the battery is between the charge start and
> charge stop thresholds. On Windows, it reports "Not Charging"
> so the quirk has been added to also report correctly.
>
> Now the "status" attribute returns "Not Charging" when the
> battery on ThinkPads is not physicaly charging.
>
> Signed-off-by: Ognjen Galic
Reviewed-by: Andy Shevchenko
> ---
>
> Notes:
> v7:
> * Added all the style changes as suggested by Rafael
> * Re-ordered the series to make this a post-implement
> bugfix
>
> v8:
> * No changes in this patch in v8
>
> v9:
> * No changes in this patch in v9
>
> v10:
> * No changes in this patch in v10
>
> v11:
> * Fix formatting of changelog
>
> v12:
> * No changes in this patch in v12
>
> drivers/acpi/battery.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 7089e31..279516e 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -74,6 +74,7 @@ static async_cookie_t async_cookie;
> static bool battery_driver_registered;
> static int battery_bix_broken_package;
> static int battery_notification_delay_ms;
> +static int battery_quirk_notcharging;
> static unsigned int cache_time = 1000;
> module_param(cache_time, uint, 0644);
> MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
> @@ -225,6 +226,8 @@ static int acpi_battery_get_property(struct power_supply
> *psy,
> val->intval = POWER_SUPPLY_STATUS_CHARGING;
> else if (acpi_battery_is_charged(battery))
> val->intval = POWER_SUPPLY_STATUS_FULL;
> + else if (battery_quirk_notcharging)
> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> else
> val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> break;
> @@ -1309,6 +1312,12 @@ battery_notification_delay_quirk(const struct
> dmi_system_id *d)
> return 0;
> }
>
> +static int __init battery_quirk_not_charging(const struct dmi_system_id *d)
> +{
> + battery_quirk_notcharging = 1;
> + return 0;
> +}
> +
> static const struct dmi_system_id bat_dmi_table[] __initconst = {
> {
> .callback = battery_bix_broken_package_quirk,
> @@ -1326,6 +1335,19 @@ static const struct dmi_system_id bat_dmi_table[]
> __initconst = {
> DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-573G"),
> },
> },
> + {
> + /*
> +* On Lenovo ThinkPads the BIOS specification defines
> +* a state when the bits for charging and discharging
> +* are both set to 0. That state is "Not Charging".
> +*/
> + .callback = battery_quirk_not_charging,
> + .ident = "Lenovo ThinkPad",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> + DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad"),
> + },
> + },
> {},
> };
>
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 v12 1/4] battery: Add the battery hooking API
On Wed, Jan 3, 2018 at 4:53 PM, Ognjen Galić wrote:
> On Wed, Jan 03, 2018 at 04:25:42PM +0200, Andy Shevchenko wrote:
>> On Wed, Jan 3, 2018 at 1:58 PM, Ognjen Galic wrote:
>> Thanks for an update. I have couple of minors. Otherwise look pretty much
>> good!
>>
>> > drivers/acpi/battery.h | 11
>> > include/acpi/battery.h | 21 +++
>>
>> There are -M and -C command line parameters to git format-patch.
>> They can take an optional argument (percentage) of threshold.
>>
>> Playing with those numbers you can achieve
Pay attention to the above
>>
>> rename ...
>>
>> line and see actual diff.
>>
>> No need to resend because of this. Just an explanation for the future Git
>> work.
>
> I did use thos options. I used the following command:
>
> git format-patch -M -C --notes -v12 -o ~/patches/. @
>
> I really don't know what you are targeting. :)
Please, read what I wrote above and the manual of git-format-patch.
>> > +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
>> > +{
>> > + struct list_head *position;
>> > + struct acpi_battery *battery;
>>
>> Missed empty line?
>
> checkpatch.pl complains if there are NOT empty lines between
> declarations and statements.
checkpatch some times on one hand complains about something which it
should not, on the other didn't take into consideration cases like
this one.
Your statement started with comment, btw.
>> > + /*
>> > +* In order to remove a hook, we first need to
>> > +* de-register all the batteries that are registered.
>> > +*/
>> > + if (lock)
>> > + mutex_lock(&hook_mutex);
> I mean, it's not game-breaking, its just minor style stuff. I won't be
> sending more revisions because of these small issues, as I think its
> uneccessary to flood both Rafael and the mailing lists with patch
> revisions that remove or add a few spaces. No offence, it just got old.
Yes, his call anyway to apply or ask you for amendments. I'm just
helping with review.
>> > +static void __exit battery_hook_exit(void)
>> > +{
>> > + struct acpi_battery_hook *hook;
>> > + struct acpi_battery_hook *ptr;
>>
>> Missed empty line?
>
> See above about the checkpatch.pl stuff.
See above the answer.
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] ThinkPad Yoga 370 thinkpad_acpi unhandled events
+Cc: PDx86 ML, Darren (Luckily entire discussion is kept in this mail) On Thu, Feb 1, 2018 at 4:07 AM, Peter FP1 Zhang wrote: > Dear all, > > I was told by our BIOS/Thermal team that the two events are designed for > Windows OS, for Linux we can just ignore them directly. > > Thanks, > > Peter Zhang \ 张福平, PMP > ThinkPad & ThinkStation Linux Solutions > Tel: (+86) 181-1611-8005 | Lenovo Shanghai > > Linux for Those Who Do - http://www.lenovo.com/linux > > > -Original Message- > From: Peter FP1 Zhang > Sent: Wednesday, January 31, 2018 10:48 AM > To: 'Jordan Glover'; 'Christian Kellner'; ibm-acpi-devel@lists.sourceforge.net > Cc: bb...@redhat.com; andy.shevche...@gmail.com > Subject: RE: ThinkPad Yoga 370 thinkpad_acpi unhandled events > > Hello Jordan, > > Appreciate your report. I ever discussed this topic with Chris half a year > ago. The two events are both about thermal control. > > Hello Chris and upstream team, > > Would you mind telling me what info is needed to enable the two events? > > 0x60F0 HK_THERMAL_TRANSFORMATION_CHANGED, ASL method is GTMS > 0x6032 HK_DYNAMIC_THERMAL_CONTROL_SET_COMMAND_COMPLIETION, ASL method is > DYTC > > Thanks, > > Peter Zhang \ 张福平, PMP > ThinkPad & ThinkStation Linux Solutions > Tel: (+86) 181-1611-8005 | Lenovo Shanghai > > Linux for Those Who Do - http://www.lenovo.com/linux > > -Original Message- > From: Jordan Glover [mailto:golden_mille...@protonmail.ch] > Sent: Wednesday, January 31, 2018 1:54 AM > To: ibm-acpi-devel@lists.sourceforge.net > Cc: Peter FP1 Zhang; bb...@redhat.com; andy.shevche...@gmail.com > Subject: ThinkPad Yoga 370 thinkpad_acpi unhandled events > > > Hello > > I noticed some messages about thinkpad_acpi events in my system logs. > My hardware is Lenovo ThinkPad Yoga 370, model 20JJS0HD00 2in1 laptop/tablet. > I tested this with kernel 4.15.rc8 with > https://patchwork.kernel.org/patch/10057867/ patch added. > > 1. Happens at boot: > > thinkpad_acpi: ThinkPad ACPI Extras v0.25 > thinkpad_acpi: http://ibm-acpi.sf.net/ > thinkpad_acpi: ThinkPad BIOS R0HET41W (1.21 ), EC unknown > thinkpad_acpi: Lenovo ThinkPad Yoga 370, model 20JJS0HD00 > thinkpad_acpi: radio switch found; radios are enabled > thinkpad_acpi: Unknown/reserved multi mode value 0x for type 4, please > report this to ibm-acpi-devel@lists.sourceforge.net > ... > thinkpad_acpi: unknown possible thermal alarm or keyboard event received > thinkpad_acpi: unhandled HKEY event 0x60f0 > thinkpad_acpi: please report the conditions when this event happened to > ibm-acpi-devel@lists.sourceforge.net > > > > 2. Happens after 2in1 mode switch and back: > > thinkpad_acpi: unknown possible thermal alarm or keyboard event received > thinkpad_acpi: unhandled HKEY event 0x6032 > thinkpad_acpi: please report the conditions when this event happened to > ibm-acpi-devel@lists.sourceforge.net > thinkpad_acpi: unknown possible thermal alarm or keyboard event received > thinkpad_acpi: unhandled HKEY event 0x60f0 > thinkpad_acpi: please report the conditions when this event happened to > ibm-acpi-devel@lists.sourceforge.net > thinkpad_acpi: unknown possible thermal alarm or keyboard event received > thinkpad_acpi: unhandled HKEY event 0x60f0 > thinkpad_acpi: please report the conditions when this event happened to > ibm-acpi-devel@lists.sourceforge.net > thinkpad_acpi: unknown possible thermal alarm or keyboard event received > thinkpad_acpi: unhandled HKEY event 0x60f0 > thinkpad_acpi: please report the conditions when this event happened to > ibm-acpi-devel@lists.sourceforge.net > > I attached output of: > acpidump > dmidecode > dmesg |grep thinkpad_acpi > > > Jordan -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 v13 3/4] thinkpad_acpi: Add support for battery thresholds
On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić wrote: > Do you guys want me to send in another revision of the patch with some > documentation on the sysfs API? I noticed I didn't apply it because of some pending changes discussed, perhaps this one above. So, definitely please send a new version which addresses comments. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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] Add Second Fan Support for Thinkpad P50
On Mon, Apr 2, 2018 at 9:27 PM, 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
> ---
> 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
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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] Add Second Fan Support for Thinkpad P50
On Mon, Apr 2, 2018 at 9:27 PM, 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).
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
> ---
> 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
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] ThinkPad Yoga 370 thinkpad_acpi unhandled events
On Tue, Apr 24, 2018 at 2:28 PM, Henrique de Moraes Holschuh wrote: > On Tue, 24 Apr 2018, Jordan Glover via ibm-acpi-devel wrote: >> On April 20, 2018 5:17 PM, Henrique de Moraes Holschuh >> wrote: >> > I will send the (untested) patches, please test. >> >> I tested patches on top of Linux 4.17-rc1. The warning message is gone, >> everything works good. > > Thanks for testing! > > Andy, Darren, looks like we can add Jordan's Tested-by: and schedule the > three patches for the next merge window... > > Do you want me to respin the patches with the tested-by and without the > untested prefix, or can you take them directly? Yes, please. It will make our life easier. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 3/3] thinkpad-acpi: silence false-positive-prone pr_warn
On Tue, Apr 24, 2018 at 10:56 PM, Henrique de Moraes Holschuh
wrote:
> Do not consider unknown HKEY events in the 0x6000 range to be thermal
> warnings. Instead, handle them as a generic unknown HKEY event, which
> are reported to the kernel log at priority "notice", and do not trigger
> a thermal registers state dump to the log.
>
All 3 are pushed to my review and testing queue, thanks!
> Signed-off-by: Henrique de Moraes Holschuh
> Tested-by: Jordan Glover
> ---
> drivers/platform/x86/thinkpad_acpi.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 3d70ef7e8a68..a0e9ce0d85b9 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4039,8 +4039,6 @@ static bool hotkey_notify_6xxx(const u32 hkey,
> bool *send_acpi_ev,
> bool *ignore_acpi_ev)
> {
> - bool known = true;
> -
> /* 0x6000-0x6FFF: thermal alarms/notices and keyboard events */
> *send_acpi_ev = true;
> *ignore_acpi_ev = false;
> @@ -4107,13 +4105,12 @@ static bool hotkey_notify_6xxx(const u32 hkey,
> return true;
>
> default:
> - pr_warn("unknown possible thermal alarm or keyboard event
> received\n");
> - known = false;
> + /* report simply as unknown, no sensor dump */
> + return false;
> }
>
> thermal_dump_all_sensors();
> -
> - return known;
> + return true;
> }
>
> static void hotkey_notify(struct ibm_struct *ibm, u32 event)
> --
> 2.11.0
>
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 1/3] thinkpad_acpi: add support for inhibit_charge
On Mon, May 14, 2018 at 12:46 PM, Christoph Böhmwalder wrote: > On Sun, May 13, 2018 at 05:29:37PM +0200, Ognjen Galic wrote: >> Lenovo ThinkPad systems support the prevention of >> battery charging via a manual override called Inhibit Charge. >> >> This patch adds support for that attribute and exposes it via the >> battery ACPI driver in the generic location: >> >> /sys/class/power_supply/BATX/inhibit_charge >> + /* When setting inhbitit charge, we set a default vaulue of > > This comment does not adhere to the Linux coding style While you are right in principle, the whole driver is so old and uses this style. So, for such cases we, as maintainers, prefer less deviation work, i.e. keeping the style is a good thing to do. >> + /* The only valid values are 1 and 0 */ >> + if (value != 0 && value != 1) > > I'm not sure, but maybe `if (value < 2)` is better here? Since it's about integer-as-a-boolean, test for bit 0 would be sufficient, i.e. ~BIT(0). Though, I find this form not so readable since the input comes actually from the user. It would be nice to have just kstrtobool() called instead for such options, but see above. It would need a (huge) refactoring of the driver first. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] ThinkPad T480s & thinkpad_acpi.ko
On Fri, Jun 8, 2018 at 11:12 AM, Pali Rohár wrote: > Hi! I just found out that thinkpad_acpi.ko is not automatically loaded > on new ThinkPad T480s. Is there any reason for it? Manually calling > modprobe thinkpad_acpi is working fine, but I would expect that it > should be loaded automatically. > > Here is dmesg output after loading it manually: > > thinkpad_acpi: ThinkPad ACPI Extras v0.25 > thinkpad_acpi: http://ibm-acpi.sf.net/ > thinkpad_acpi: ThinkPad BIOS N22ET34W (1.11 ), EC unknown > thinkpad_acpi: Lenovo ThinkPad T480s, model 20L7S03000 > thinkpad_acpi: radio switch found; radios are enabled > thinkpad_acpi: This ThinkPad has standard ACPI backlight brightness control, > supported by the ACPI video driver > thinkpad_acpi: Disabling thinkpad-acpi brightness events by default... > thinkpad_acpi: rfkill switch tpacpi_bluetooth_sw: radio is unblocked > thinkpad_acpi: Standard ACPI backlight interface available, not loading > native one Did you run depmod -a? Is it present in the output? There are three ways of enumeration as far as I can see from the driver's code: - by short driver name "tpacpi" (not likely our case) - by DMI matching (only for old BIOS'es) - by ACPI ID So, does the ACPI contain one of the listed IDs? #define TPACPI_ACPI_IBM_HKEY_HID"IBM0068" #define TPACPI_ACPI_LENOVO_HKEY_HID "LEN0068" #define TPACPI_ACPI_LENOVO_HKEY_V2_HID "LEN0268" ? -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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 v2 1/2] platform/x86: thinkpad_acpi: Proper model/release matching
On Wed, Aug 1, 2018 at 12:22 PM, Jouke Witteveen wrote: > Modern Thinkpads have three character model designators. Previously, they > would be accepted, but recorded incompletely. Revision matching extracted > the wrong bytes from the ID string. This made the use of quirks for modern > machines impossible. > > Fixes: 1b0eb5bc2413 > Signed-off-by: Jouke Witteveen > --- > > This has now been tested on both 3-character models (Thinkpad 13) and two > character models (Thinkpad 11e). > The part one of this series is identical to the previously submitted patch. The patches you submitted before had been applied. Please, rebase your stuff against our for-next branch and resend. -- With Best Regards, Andy Shevchenko -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ 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] platform/x86: thinkpad_acpi: Extend battery quirk coverage
On Wed, Aug 1, 2018 at 5:32 PM, Jouke Witteveen wrote:
> Based on bug reports and a web search for
> "Thinkpad_acpi: Error probing battery 2"
> four more models were found that require the battery quirk:
> Lenovo B5400, Thinkpad 11e (original and gen 3), Thinkpad 13 gen 3.
I think you may add Henrique's ACK here since he did for previous "v2".
But I let him speak for himself.
> I could only get someone to verify this on the 11e. Other requests have
> gone unanswered. The error message (Error probing battery 2) is a pretty
> clear indication that the quirk is needed, however.
I see. Care to Cc that guy (if they wish, of course, to be Cc:ed)?
> - TPACPI_Q_LNV3('R', '0', 'C', true), /* Thinkpad 13 */
> + TPACPI_Q_LNV3('R', '0', 'C', true), /* 13 */
Please, avoid ping-pong style of fixes. Here just leave an original line.
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 v2] platform/x86: thinkpad_acpi: Fix multi-battery bug
On Thu, Aug 2, 2018 at 1:24 AM, Thomas Weißschuh wrote:
> The struct containing the supported operations for all batteries is
> being zeroed on each battery probe. This prevents all other batteries
> except the lastly probed one from being configured.
You forgot to include subsystem maintainers along with driver maintainer.
I pushed this to my review and testing queue, meanwhile I would wait
for Ack from Henrique.
>
> Signed-off-by: Thomas Weißschuh
> ---
>
> Changes since v1:
>
> * Missing sign-off added
>
> drivers/platform/x86/thinkpad_acpi.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 3dbff2dda9be..60de1c577ce0 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9374,7 +9374,9 @@ static int tpacpi_battery_probe(int battery)
> {
> int ret = 0;
>
> - memset(&battery_info, 0, sizeof(struct tpacpi_battery_driver_data));
> + memset(&battery_info.batteries[battery], 0,
> + sizeof(battery_info.batteries[battery]));
> +
> /*
> * 1) Get the current start threshold
> * 2) Check for support
> @@ -9616,6 +9618,8 @@ static const struct tpacpi_quirk battery_quirk_table[]
> __initconst = {
>
> static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
> {
> + memset(&battery_info, 0, sizeof(battery_info));
> +
> tp_features.battery_force_primary = tpacpi_check_quirks(
> battery_quirk_table,
> ARRAY_SIZE(battery_quirk_table));
> --
> 2.18.0
>
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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 v2] platform/x86: thinkpad_acpi: extend battery quirk coverage
On Thu, Aug 2, 2018 at 11:28 PM, Jouke Witteveen wrote:
> Based on bug reports and a web search for
> "Thinkpad_acpi: Error probing battery 2"
> four more models were found that require the battery quirk:
> Lenovo B5400, Thinkpad 11e, Thinkpad 11e gen 3, Thinkpad 13 gen 3.
>
Pushed to my review and testing queue, thanks!
> Signed-off-by: Jouke Witteveen
> Tested-by: James Cheshire
> Acked-by: Henrique de Moraes Holschuh
> ---
>
> I avoided the ping-pong style and added James Cheshire as a tester.
> Andy suggested to Cc: him, but if I understand the kernel documentation on
> submitting patches correctly, a Tested-by: is in order. James tested on a
> Thinkpad 11e and agreed to be included in the commit message.
>
> drivers/platform/x86/thinkpad_acpi.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 3dbff2dd..fd49855a 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9611,7 +9611,11 @@ static const struct tpacpi_quirk battery_quirk_table[]
> __initconst = {
> * Individual addressing is broken on models that expose the
> * primary battery as BAT1.
> */
> + TPACPI_Q_LNV('J', '7', true), /* B5400 */
> + TPACPI_Q_LNV('J', 'I', true), /* Thinkpad 11e */
> + TPACPI_Q_LNV3('R', '0', 'B', true), /* Thinkpad 11e gen 3 */
> TPACPI_Q_LNV3('R', '0', 'C', true), /* Thinkpad 13 */
> + TPACPI_Q_LNV3('R', '0', 'J', true), /* Thinkpad 13 gen 2 */
> };
>
> static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
> --
> 2.18.0
>
--
With Best Regards,
Andy Shevchenko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
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] platform/x86: thinkpad_acpi: Change the keymap for Favorites hotkey
On Thu, Oct 18, 2018 at 5:30 AM wrote:
>
> Could you take a little time to review the patch? Thanks
>
Pushed to my review and testing queue, thanks!
> --Original Mail--
> Sender: zhang xianwei10129966
> To: ibm-a...@hmh.eng.br dvh...@infradead.org
> a...@infradead.org
> CC: ibm-acpi-devel@lists.sourceforge.net
> platform-driver-...@vger.kernel.org
> linux-ker...@vger.kernel.org
> zhong weidong10001088;zhang xianwei10129966;
> Date: 2018/10/11 13:52
> Subject: [PATCH] platform/x86: thinkpad_acpi: Change the keymap for Favorites
> hotkey
> The keycode KEY_FAVORITES(0x16c) used in thinkpad_acpi driver is too
> big (out of range > 255) for xorg to handle.
> xkeyboard-config has already mapped KEY_BOOKMARKS(156) to
> XF86Favorites:
>
> keycodes/evdev:
> = 164; // #define KEY_BOOKMARKS 156
>
> symbols/inet:
> key{ [ XF86Favorites ] };
>
> So change the keymap to KEY_BOOKMARKS for Favorites hotkey.
>
> Signed-off-by: Zhang Xianwei
> ---
> drivers/platform/x86/thinkpad_acpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index fde08a9..a86cf47 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -3457,7 +3457,7 @@ static int __init hotkey_init(struct ibm_init_struct
> *iibm)
> KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
> KEY_UNKNOWN,
> -KEY_FAVORITES, /* Favorite app, 0x311 */
> +KEY_BOOKMARKS, /* Favorite app, 0x311 */
> KEY_RESERVED,/* Clipping tool */
> KEY_CALC,/* Calculator (above numpad, P52) */
> KEY_BLUETOOTH, /* Bluetooth */
> --
> 2.9.5
--
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] platform/x86: thinkpad_acpi: add unsafe_leds parameter
Ah, last but not least, you forgot to include subsystem/driver
maintainers in the Cc list (in my mailbox it's even got directly to a
spam).
On Mon, Oct 29, 2018 at 4:05 PM Andy Shevchenko
wrote:
>
> On Sat, Oct 27, 2018 at 9:15 PM Milan Hauth wrote:
>
> Thank you for the patch. I have few comments here.
>
> >
> > move CONFIG_THINKPAD_ACPI_UNSAFE_LEDS compile config
> > to thinkpad_acpi.unsafe_leds module parameter
> >
> > so there is no more need to re-compile
> > to control important LEDs, which is unsafe
>
> First of all, please use normal English writing, i.e. Capitalize
> sentences, use proper punctuation like commas and periods.
> Second, check your mail clients and software, patch is broken and
> can't be applied.
>
> See my further comments below.
>
> >
> > Signed-off-by: Milan Hauth
> >
> > ---
> >
> > usage
> >
> > echo 'options thinkpad_acpi unsafe_leds=Y' \
> > | sudo tee /etc/modprobe.d/thinkpad_acpi-unsafe_leds.conf
> >
> > for ((i=0;i<=15;i=i+1)); do echo "$i off" | sudo tee /proc/acpi/ibm/led;
> > done
> >
> > this will only disable power, battery, standby, dock LEDs
> > but will keep wifi, bluetooth, harddrive, AC power LEDs [and maybe more]
>
>
> This has to be a part of Documentation.
>
> > references
> >
> > [SOLVED] ThinkPad LED's control / Laptop Issues / Arch Linux Forums
> > https://bbs.archlinux.org/viewtopic.php?id=177337#p1384354
> >
> > thinkpad-acpi: restrict access to some firmware LEDs
> > commit a4d5effcc73749ee3ebbf578d162905e6fa4e07d
> > date 2009-04-04
>
> This should be part of commit message in a form of plain text and/or
> specific tags (BugLink, Fixes).
>
> > --- a/drivers/platform/x86/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/thinkpad_acpi.c
> > @@ -88,6 +88,27 @@
> > #include
> > #include
> > #include
> > +#include
> > +
> > +/* parameters for module thinkpad_acpi */
> > +
> > +/*
> > + * parameter unsafe_leds
> > + *
> > + * Allow control of important LEDs? (unsafe)
> > + * N = no [default]
> > + * Y = yes
> > + *
> > + * docs:
> > + * Documentation/laptops/thinkpad-acpi.txt
> > + * section 'LED control'
> > + *
> > + * moved from compile config
> > + * CONFIG_THINKPAD_ACPI_UNSAFE_LEDS
> > + */
> > +static bool unsafe_leds = false;
> > +module_param(unsafe_leds, bool, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
> > +MODULE_PARM_DESC(unsafe_leds, "Allow control of important LEDs? (unsafe)");
>
> We are not accepting new module parameters without a very strong argument.
> Here I don't see why it can't be a sysfs node which enables this
> behaviour (or disables) at run time.
>
> (Note: if you introduce a sysfs node you need to add a proper ABI
> subsection to Documentation)
>
> >
> > /* ThinkPad CMOS commands */
> > #define TP_CMOS_VOLUME_DOWN0
> > @@ -5754,11 +5775,10 @@ static const char * const tpacpi_led_nam
> >
> > static inline bool tpacpi_is_led_restricted(const unsigned int led)
> > {
> > -#ifdef CONFIG_THINKPAD_ACPI_UNSAFE_LEDS
> > - return false;
> > -#else
> > - return (1U & (TPACPI_SAFE_LEDS >> led)) == 0;
> > -#endif
>
> > + if (unsafe_leds)
> > + return false;
> > + else
> > + return (1U & (TPACPI_SAFE_LEDS >> led)) == 0;
>
> return unsafe_leds ? false : ...;
>
> But this, perhaps, will be modified when switching to sysfs.
>
> > }
> >
> > static int led_get_status(const unsigned int led)
> > @@ -6048,9 +6068,9 @@ static int __init led_init(struct ibm_in
> > }
> > }
> >
> > -#ifdef CONFIG_THINKPAD_ACPI_UNSAFE_LEDS
> > - pr_notice("warning: userspace override of important firmware LEDs is
> > enabled\n");
> > -#endif
> > + if (unsafe_leds)
> > + pr_notice("warning: userspace override of important
> > firmware LEDs
> > is enabled\n");
> > +
> > return 0;
> > }
> >
> >
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -507,29 +507,6 @@ config THINKPAD_ACPI_DEBUG
> >
> > If you are not sure, say N here.
> >
> > -config THINKPAD_ACPI_UNSAFE_LEDS
>
> People, who are using this option will be surprised by a changed behaviour.
> So, please leave it and use as enforcement of t
Re: [ibm-acpi-devel] [PATCH 1/6] leds: trigger: Introduce audio mute LED trigger
On Mon, Nov 26, 2018 at 7:14 PM Takashi Iwai wrote: > > This patch adds a new LED trigger for coupling the audio mixer change > with the LED on laptops or other devices. Currently there are two > trigger types, "audio-mute" and "audio-micmute". > > The audio driver triggers the LED brightness change via > ledtrig_audio_set() call with the proper type (either mute or > mic-mute). OTOH, the consumers may call ledtrig_audio_get() for the > initial brightness value that may have been set by the audio driver > beforehand. > > This new stuff will be used by HD-audio codec driver and some platform > drivers (thinkpad_acpi and dell-laptop, also upcoming huawei-wmi). > +#include > +#include Only on of above is needed, I think you meant module.h. -- 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 3/6] platform/x86: thinkpad_acpi: Add audio mute LED classdev support
On Mon, Nov 26, 2018 at 7:13 PM Takashi Iwai wrote:
>
> In the upcoming change, the binding of audio mute / mic-mute LED
> controls will be switched with LED trigger. This patch is the last
> piece of preparation: adding the audio mute / mic-mute LED class
> devices to thinkpad_acpi driver.
>
> Two devices, tpacpi::mute and tpacpi::micmute, will be added for
> controlling the mute LED and mic-mute LED, respectively.
>
> Also this selects CONFIG_LEDS_TRIGGERS and CONFIG_LEDS_TRIGGERS_AUDIO
> unconditionally. Strictly speaking, these aren't 100% mandatory, but
> leaving these manual selections would lead to a functional regression
> easily once after converting from the dynamic symbol binding to the
> LEDs trigger in a later patch.
> + if (!ACPI_SUCCESS(acpi_get_handle(hkey_handle, t->name,
> &temp))) {
ACPI_FAILURE()
> t->state = -ENODEV;
> + continue;
> + }
> + err = led_classdev_register(&tpacpi_pdev->dev,
> &mute_led_cdev[i]);
> + if (err < 0) {
> + while (--i >= 0)
Needs { } due to two liner below.
Might be converted to simple
while (i--) {
...
}
btw.
> + if (led_tables[i].state >= 0)
> +
> led_classdev_unregister(&mute_led_cdev[i]);
> + return err;
> + }
--
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 0/6] Introduce audio-mute LED trigger (and conversions to it)
On Mon, Nov 26, 2018 at 7:14 PM Takashi Iwai wrote: > > Hi, > > this is patch series I've hacked after useful conversation with > Pavel. Basically this adds a new LED trigger audio-mute and > audio-micmute, and convert the HD-audio driver and the platform > drivers to use the LED trigger instead of the ugly direct dynamic > symbol binding. > > The latest version of patches are found in topic/leds-trigger branch > in my sound git tree. > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > > As these are cross-tree patches, the branch above is based cleanly on > v4.20-rc3, so that it can be merged well to multiple trees. > > Once after getting the ACK's, I'll add tags and fixate for merges. > > This patch series don't include huawei-wmi stuff; so Huawei patches > need rework. I already have some piece of changes for huawei-wmi, so > please ping me if needed. > > I checked briefly on my Dell laptop, and a Thinkpad model. > Wider tests are appreciated, of course. > Few minor comments, otherwise I like this series. Acked-by: Andy Shevchenko for PDx86 bits. > > thanks, > > Takashi > > === > > Takashi Iwai (6): > leds: trigger: Introduce audio mute LED trigger > platform/x86: dell-laptop: Add micmute LED trigger support > platform/x86: thinkpad_acpi: Add audio mute LED classdev support > ALSA: hda - Support led audio trigger > platform/x86: dell-laptop: Drop superfluous exported function > platform/x86: thinkpad_acpi: Drop superfluous exported function > > drivers/leds/trigger/Kconfig | 7 +++ > drivers/leds/trigger/Makefile| 1 + > drivers/leds/trigger/ledtrig-audio.c | 45 +++ > drivers/platform/x86/Kconfig | 4 ++ > drivers/platform/x86/dell-laptop.c | 27 +--- > drivers/platform/x86/thinkpad_acpi.c | 66 +--- > include/linux/dell-led.h | 7 --- > include/linux/leds.h | 20 + > include/linux/thinkpad_acpi.h| 16 --- > sound/pci/hda/dell_wmi_helper.c | 48 > sound/pci/hda/hda_generic.c | 31 + > sound/pci/hda/hda_generic.h | 2 + > sound/pci/hda/patch_realtek.c| 17 +++ > sound/pci/hda/thinkpad_helper.c | 43 +++--- > 14 files changed, 194 insertions(+), 140 deletions(-) > create mode 100644 drivers/leds/trigger/ledtrig-audio.c > delete mode 100644 include/linux/dell-led.h > delete mode 100644 include/linux/thinkpad_acpi.h > delete mode 100644 sound/pci/hda/dell_wmi_helper.c > > -- > 2.19.1 > -- 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 0/6] Introduce audio-mute LED trigger (and conversions to it)
On Wed, Nov 28, 2018 at 2:59 PM Takashi Iwai wrote: > On Wed, 28 Nov 2018 13:25:05 +0100, > > On Wed 2018-11-28 12:38:19, Takashi Iwai wrote: > > Just use sys:: :-). > > > > laptop:: would work for me, too. (It is always laptop in the cases we > > are handling now, right?) > > In theory, such a thing can be on all-in-one desktops, too. > > > When we get a keyboard with mute led, we'll have to decide if it > > should be input6::mute -- because it is on keyboard, or if it is > > sys::mute -- because the key is expected to mute whole system. > > OK, I'll wait for more comments in today and update accordingly. sys sounds plausible to me. In any case we may revisit in the future if something happens to it. OTOH it would be part of ABI, right? -- 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 0/6] Introduce audio-mute LED trigger (and conversions to it)
On Thu, Nov 29, 2018 at 10:23:31PM +0100, Jacek Anaszewski wrote: > Regarding sound devices - how would we name micmute LED for USB > microphone? I suppose that it is possible to discover some unique > identifier in runtime - this is a question to ALSA people. USB path (e.g. 2-1.1.2.4.3) + '::' + name ? -- 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] platform/x86: thinkpad_acpi: add unsafe_leds parameter
On Tue, Nov 13, 2018 at 6:41 PM Milan Hauth wrote: > thanks for your replies. Thanks for an update, though made in wrong format: - no top post - should be send as a separate patch in a separate thread - should have increased version But the main one is... > > On Sat, 27 Oct 2018, Milan Hauth wrote: > > > move CONFIG_THINKPAD_ACPI_UNSAFE_LEDS compile config > > > to thinkpad_acpi.unsafe_leds module parameter > > > > > > so there is no more need to re-compile > > > to control important LEDs, which is unsafe > > > > > > Signed-off-by: Milan Hauth > > > > I am not confortable signing-off on this one. ...I'm not familiar with this interface and I rely on Henrique's opinion, so, I can't take it. Sorry. -- 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] platform/x86: thinkpad_acpi: Cleanup quirks macros
On Fri, Nov 16, 2018 at 3:18 PM Jouke Witteveen wrote:
>
Please, submit a new version with changelog.
Though, before doing this, I would hear if Henrique is OK with the
change (for me it looks good)
> Signed-off-by: Jouke Witteveen
> ---
> The only difference in the generated code is that the quirk values are
> now within parentheses.
>
> drivers/platform/x86/thinkpad_acpi.c | 45
> 1 file changed, 13 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index fde08a997..e1e51bfb3 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -478,6 +478,12 @@ do {
> \
> .ec = TPACPI_MATCH_ANY, \
> .quirks = (__quirk) }
>
> +#define TPACPI_QEC_IBM(__id1, __id2, __quirk) \
> + { .vendor = PCI_VENDOR_ID_IBM, \
> + .bios = TPACPI_MATCH_ANY, \
> + .ec = TPID(__id1, __id2), \
> + .quirks = (__quirk) }
> +
> #define TPACPI_QEC_LNV(__id1, __id2, __quirk) \
> { .vendor = PCI_VENDOR_ID_LENOVO, \
> .bios = TPACPI_MATCH_ANY, \
> @@ -5973,9 +5979,6 @@ static const struct tpacpi_quirk led_useful_qtable[]
> __initconst = {
> },
> };
>
> -#undef TPACPI_LEDQ_IBM
> -#undef TPACPI_LEDQ_LNV
> -
> static enum led_access_mode __init led_init_detect_mode(void)
> {
> acpi_status status;
> @@ -8710,40 +8713,18 @@ static const struct attribute_group fan_attr_group = {
> .attrs = fan_attributes,
> };
>
> -#defineTPACPI_FAN_Q1 0x0001 /* Unitialized HFSP */
> +#define TPACPI_FAN_Q1 0x0001 /* Unitialized HFSP */
> #define TPACPI_FAN_2FAN0x0002 /* EC 0x31 bit 0 selects fan2
> */
>
> -#define TPACPI_FAN_QI(__id1, __id2, __quirks) \
> - { .vendor = PCI_VENDOR_ID_IBM, \
> - .bios = TPACPI_MATCH_ANY, \
> - .ec = TPID(__id1, __id2), \
> - .quirks = __quirks }
> -
> -#define TPACPI_FAN_QL(__id1, __id2, __quirks) \
> - { .vendor = PCI_VENDOR_ID_LENOVO, \
> - .bios = TPACPI_MATCH_ANY, \
> - .ec = TPID(__id1, __id2), \
> - .quirks = __quirks }
> -
> -#define TPACPI_FAN_QB(__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_QB('N', '1', TPACPI_FAN_2FAN),
> + TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1),
> + TPACPI_QEC_IBM('7', '8', TPACPI_FAN_Q1),
> + TPACPI_QEC_IBM('7', '6', TPACPI_FAN_Q1),
> + TPACPI_QEC_IBM('7', '0', TPACPI_FAN_Q1),
> + TPACPI_QEC_LNV('7', 'M', TPACPI_FAN_2FAN),
> + TPACPI_Q_LNV('N', '1', TPACPI_FAN_2FAN),
> };
>
> -#undef TPACPI_FAN_QL
> -#undef TPACPI_FAN_QI
> -#undef TPACPI_FAN_QB
> -
> static int __init fan_init(struct ibm_init_struct *iibm)
> {
> int rc;
> --
> 2.19.1
>
--
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] platform/x86: thinkpad_acpi: Cleanup quirks macros
On Tue, Dec 4, 2018 at 1:20 PM Henrique de Moraes Holschuh wrote: > > On Mon, 03 Dec 2018, Andy Shevchenko wrote: > > On Fri, Nov 16, 2018 at 3:18 PM Jouke Witteveen > > wrote: > > > > Please, submit a new version with changelog. > > > > Though, before doing this, I would hear if Henrique is OK with the > > change (for me it looks good) > > Acked-by: Henrique de Moraes Holschuh Thank you! Jouke, so, I would wait for v2 with changelog and don't forget to append Henrique's tag as well. -- 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 v2] platform/x86: thinkpad_acpi: Cleanup quirks macros
On Wed, Dec 5, 2018 at 1:18 PM Jouke Witteveen wrote:
>
> - Use generic quirks macros for fan quirks
> The fan-specific quirks macros were duplicates of the generic ones.
>
> - Remove useless #undef lines
> The referenced macros are not defined anywhere.
>
Patch had been pushed, thanks!
> Signed-off-by: Jouke Witteveen
> Acked-by: Henrique de Moraes Holschuh
> ---
>
> This time, with a more detailed commit message.
>
> drivers/platform/x86/thinkpad_acpi.c | 45
> 1 file changed, 13 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index fde08a997..e1e51bfb3 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -478,6 +478,12 @@ do {
> \
> .ec = TPACPI_MATCH_ANY, \
> .quirks = (__quirk) }
>
> +#define TPACPI_QEC_IBM(__id1, __id2, __quirk) \
> + { .vendor = PCI_VENDOR_ID_IBM, \
> + .bios = TPACPI_MATCH_ANY, \
> + .ec = TPID(__id1, __id2), \
> + .quirks = (__quirk) }
> +
> #define TPACPI_QEC_LNV(__id1, __id2, __quirk) \
> { .vendor = PCI_VENDOR_ID_LENOVO, \
> .bios = TPACPI_MATCH_ANY, \
> @@ -5973,9 +5979,6 @@ static const struct tpacpi_quirk led_useful_qtable[]
> __initconst = {
> },
> };
>
> -#undef TPACPI_LEDQ_IBM
> -#undef TPACPI_LEDQ_LNV
> -
> static enum led_access_mode __init led_init_detect_mode(void)
> {
> acpi_status status;
> @@ -8710,40 +8713,18 @@ static const struct attribute_group fan_attr_group = {
> .attrs = fan_attributes,
> };
>
> -#defineTPACPI_FAN_Q1 0x0001 /* Unitialized HFSP */
> +#define TPACPI_FAN_Q1 0x0001 /* Unitialized HFSP */
> #define TPACPI_FAN_2FAN0x0002 /* EC 0x31 bit 0 selects fan2
> */
>
> -#define TPACPI_FAN_QI(__id1, __id2, __quirks) \
> - { .vendor = PCI_VENDOR_ID_IBM, \
> - .bios = TPACPI_MATCH_ANY, \
> - .ec = TPID(__id1, __id2), \
> - .quirks = __quirks }
> -
> -#define TPACPI_FAN_QL(__id1, __id2, __quirks) \
> - { .vendor = PCI_VENDOR_ID_LENOVO, \
> - .bios = TPACPI_MATCH_ANY, \
> - .ec = TPID(__id1, __id2), \
> - .quirks = __quirks }
> -
> -#define TPACPI_FAN_QB(__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_QB('N', '1', TPACPI_FAN_2FAN),
> + TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1),
> + TPACPI_QEC_IBM('7', '8', TPACPI_FAN_Q1),
> + TPACPI_QEC_IBM('7', '6', TPACPI_FAN_Q1),
> + TPACPI_QEC_IBM('7', '0', TPACPI_FAN_Q1),
> + TPACPI_QEC_LNV('7', 'M', TPACPI_FAN_2FAN),
> + TPACPI_Q_LNV('N', '1', TPACPI_FAN_2FAN),
> };
>
> -#undef TPACPI_FAN_QL
> -#undef TPACPI_FAN_QI
> -#undef TPACPI_FAN_QB
> -
> static int __init fan_init(struct ibm_init_struct *iibm)
> {
> int rc;
> --
> 2.19.2
>
--
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] platform/x86: thinkpad_acpi: disable bluetooth for some machines
On Thu, Mar 7, 2019 at 10:09 AM Jiaxun Yang wrote:
>
> Some AMD based ThinkPads have a firmware bug that calling
> "GBDC" will cause bluetooth on Intel wireless cards blocked.
>
> Probe these models by DMI match and disable bluetooth subdriver
> if specified Intel wireless card exist.
Thanks for a patch. My comments below.
(and take your time, there is merge window going on, so, your fix is
probably material for v5.1-rc2)
> @@ -79,6 +79,7 @@
> #include
> #include
> #include
> +#include
> #include
The new include has included the old one with IDs.
> #include
> #include
> @@ -4501,11 +4502,80 @@ static void bluetooth_exit(void)
> bluetooth_shutdown();
> }
> +static int __init have_bt_fwbug(void)
> +{
> + /* Some AMD based ThinkPads have a firmware bug that calling
> +* "GBDC" will cause bluetooth on Intel wireless cards blocked
> +*/
/*
* Comments in multi-line style example.
* Should look like this one.
*/
> + if (dmi_check_system(bt_fwbug_list)) {
> + if (pci_get_device(PCI_VENDOR_ID_INTEL, 0x24F3, NULL) || \
> + pci_get_device(PCI_VENDOR_ID_INTEL, 0x24FD, NULL) || \
> + pci_get_device(PCI_VENDOR_ID_INTEL, 0x2526, NULL))
You can do a table and use pci_match_device().
On top of it
if (x) {
if (y)
...
}
equivalent to
if (x && y) {
...
}
> + return 1;
> + else
> + return 0;
> + } else {
> + return 0;
> + }
> +}
> + if (have_bt_fwbug()) {
> + vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_RFKILL,
> + FW_BUG "disable bluetooth subdriver for Intel
> cards\n");
> + return 1;
> + }
> vdbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_RFKILL,
> "initializing bluetooth subdriver\n");
Below I see the following:
/* bluetooth not supported on 570, 600e/x, 770e, 770x, A21e, A2xm/p,
G4x, R30, R31, R40e, R50e, T20-22, X20-21 */
tp_features.bluetooth = hkey_handle &&
acpi_evalf(hkey_handle, &status, "GBDC", "qd");
Is it possible to integrate your stuff to this one?
Or can you do similar, like tp_features.bluetooth = !have_bt_fwbug(); ?
--
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] platform/x86: thinkpad_acpi: disable bluetooth for some machines
On Thu, Mar 7, 2019 at 2:30 PM Jiaxun Yang wrote: > > On Thu, Mar 07, 2019 at 04:08:20PM +0800, Jiaxun Yang wrote: > >> Some AMD based ThinkPads have a firmware bug that calling > >> "GBDC" will cause bluetooth on Intel wireless cards blocked. > >> > >> Probe these models by DMI match and disable bluetooth subdriver > >> if specified Intel wireless card exist. > Thanks for remind. > > Should I resend this patch in correct way? It still needs a bit of work, and as I told you, we are now during an opened merge window. -- 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] Bugfix thinkpad_acpi.c / proper fanquirk-matching for P50 series
On Wed, Mar 27, 2019 at 12:32:50PM +0100, Matthias Hensler wrote:
> Hi Andy,
>
> as far as I see you are responsible for pushing bugfixes to the
> thinkpad_acpi-module. If not please point me in the correct direction,
> thanks a lot.
Thanks for the report and fix.
The best way to submit this is to use kernel source tree, where you can run
scripts/get_maintainer.pl -f drivers/platform/x86/thinkpad_acpi.c
to get people responsible for the driver in upstream, followed by
git format-patch ... (you need to commit your fix first)
git send-email...
commands.
For your convenience I Cc'ed this mail to all stakeholders, though it doesn't
abrogate the need to follow the process described above.
> Commit 846a416b4630fc960c10500b4194ad504bc22d4b changed the
> quirk-matching for modern thinkpads to use three characters instead of
> two. Unfortunately that commit did break the fan-quirk matching for my
> P50. With commit 599eefffcf6b9cff3e8cc2d96fd8eebfadab339c there were
> some cleanups in the quirk-macros, but still did not fix the wrong
> matching for the P50.
>
> Please find attached a patch to fix the quirk-macro to properly detect
> the second fan on the P50 (Modell N1Exxx). I guess there should be some
> additional definitions for the P70 and newer P-series, but I do not know
> the exact model-numbers unfortunately.
>
>
> Additional what I would also like to see, is a possibility to also
> control the second fan. I use a modified version of a patch that was
> posted some years ago to the mailinglist for that, but it is really
> ugly. If you want I can try to reimplement that in a cleaner way, but
> for now at least the wrong quirk-matching should be fixed. Thanks again.
>
>
> Regards,
> Matthias
> --- thinkpad_acpi.c 2019-03-27 12:26:11.977106074 +0100
> +++ thinkpad_acpi.c.fixed 2019-03-27 12:27:44.090340050 +0100
> @@ -8721,7 +8721,7 @@
> TPACPI_QEC_IBM('7', '6', TPACPI_FAN_Q1),
> 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', 'E', TPACPI_FAN_2FAN),
> };
>
> static int __init fan_init(struct ibm_init_struct *iibm)
--
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] fix fan-quirk macro for Lenovo P50
On Wed, Mar 27, 2019 at 9:25 PM Matthias Hensler wrote:
>
> Commit 846a416b4630fc960c10500b4194ad504bc22d4b changed the
> quirk-matching for modern thinkpads to use three characters instead of
> two. Unfortunately that commit did break the fan-quirk matching for the
> P50. With commit 599eefffcf6b9cff3e8cc2d96fd8eebfadab339c there were
> some cleanups in the quirk-macros, but still did not fix the wrong
> matching for the P50.
>
> Note: I guess the quirk-list needs to be expanded for the P70 and
> newer models.
You need to put your Signed-off-by tag as well.
This is according to the process:
https://www.kernel.org/doc/Documentation/process/submitting-patches.rst
In any case, don't send updated version right now. We still have time
in this release cycle and I would like to hear Henrique and others
opinions.
> ---
> drivers/platform/x86/thinkpad_acpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 726341f2b638..575a1ea161f0 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8721,7 +8721,7 @@ static const struct tpacpi_quirk fan_quirk_table[]
> __initconst = {
> TPACPI_QEC_IBM('7', '6', TPACPI_FAN_Q1),
> 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', 'E', TPACPI_FAN_2FAN),
> };
>
> static int __init fan_init(struct ibm_init_struct *iibm)
> --
> 2.20.1
>
--
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] Question about thinkpad_acpi
On Thu, Mar 21, 2019 at 4:59 PM Sebastian Dörner wrote: > > I totally forgot to mention: > There are not only warnings in journal, it simply does not work. > /proc/acpi/ibm/fan always shows "status: disabled", level and speed are > 0, and commands via echo do not work... Thank you for the report. I Cc'ed this to public mailing lists and maintainers of the driver and subsystem. > On 21.03.19 15:53, Sebastian Dörner wrote: > > Hi Mr. Shevchenko, > > > > I'm writing to you because I noticed that you were one of the most > > active contributors to the thinkpad_acpi linux driver on github last > > year. > > I've got a question about this driver and would be happy if you've got > > time for advice. > > My problem is, that my new laptop's fan, a Thinkpad X380, is > > annoyingly loud, even if the machine is idle. > > So I figured to manage the fan speed with thinkfan, which uses > > thinkpad_acpi's /proc/acpi/ibm/fan to control the fan. > > But each time I try to cat from or echo to /proc/acpi/ibm/fan I get > > several kernel ACPI warnings: > > "kernel: ACPI: \_SB.PCI0.LPCB.EC.ECRD: 1 arguments were passed to a > > non-method ACPI object (RegionField) (20170831/nsarguments-243)" > > I tried this with the latest kernel (5.0.3) and activated fan_control > > in the thinkpad_acpi module. > > I guess this means that some ACPI stuff between kernel and machine > > went wrong?! don't know much about this. > > So my question is: > > Is there a way I can fix this, don't know, by figuring out the correct > > fan control registers for my machine and then patch it on my own. Or > > am I screwed with this machine and have to wait for Lenovo to patch this? > > You see this really bugs me :-) and I'm looking forward to your council. > > > > Greetings, > > Sebastian Dörner > > > -- 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] platform/x86: thinkpad_acpi: Read EC information on newer models
On Fri, Mar 8, 2019 at 3:15 PM Jiaxun Yang wrote:
>
> Newer ThinkPads have a totally different EC program information DMI
> table. And thermal subdriver can't work without correct EC version.
>
> Read from this entry if the old method failed to get EC information.
>
Thank you for the patch. See my comments below.
> Signed-off-by: Jiaxun Yang
> ---
> drivers/platform/x86/thinkpad_acpi.c | 62 ++--
> 1 file changed, 49 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 2e24ee42a3c6..17fd2065000b 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9959,6 +9959,37 @@ static char __init tpacpi_parse_fw_id(const char *
> const s,
> return '\0';
> }
>
> +static void find_new_ec_fwstr(const struct dmi_header *dm, void *private)
> +{
Today it's a new one, tomorrow something else might come. Care to use
versioning instead?
Something line find_ec_fwstr_vXYZ().
> +}
> - tp->ec_version_str = kstrdup(ec_fw_string,
> GFP_KERNEL);
> - if (!tp->ec_version_str)
> - return -ENOMEM;
>
> - t = tpacpi_parse_fw_id(ec_fw_string,
> - &tp->ec_model,
> &tp->ec_release);
> - if (t != 'H') {
> - pr_notice("ThinkPad firmware release %s
> doesn't match the known patterns\n",
> - ec_fw_string);
> - pr_notice("please report this to %s\n",
> - TPACPI_MAIL);
> - }
Seems to me that this is the same to below. Can you leave indentation
in place and fix it in a separate (following) patch?
> - break;
> + if (ec_fw_string[0]) {
> + tp->ec_version_str = kstrdup(ec_fw_string, GFP_KERNEL);
> + if (!tp->ec_version_str)
> + return -ENOMEM;
> +
> + t = tpacpi_parse_fw_id(ec_fw_string,
> +&tp->ec_model, &tp->ec_release);
> + if (t != 'H') {
> + pr_notice("ThinkPad firmware release %s doesn't match
> the known patterns\n",
> + ec_fw_string);
> + pr_notice("please report this to %s\n", TPACPI_MAIL);
> }
> }
--
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] platform/x86: thinkpad_acpi: fix spelling mistake "capabilites" -> "capabilities"
On Tue, Apr 16, 2019 at 9:58 AM Mukesh Ojha wrote: > > > On 4/15/2019 6:22 PM, Colin King wrote: > > From: Colin Ian King > > > > There is a spelling mistake in a module parameter description. Fix it. > > Pushed to my review and testing queue, thanks! > > Signed-off-by: Colin Ian King > Reviewed-by: Mukesh Ojha > > Cheers, > -Mukesh > > > --- > > drivers/platform/x86/thinkpad_acpi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/platform/x86/thinkpad_acpi.c > > b/drivers/platform/x86/thinkpad_acpi.c > > index 57d9ae9d8e56..9192b686e9a6 100644 > > --- a/drivers/platform/x86/thinkpad_acpi.c > > +++ b/drivers/platform/x86/thinkpad_acpi.c > > @@ -10269,7 +10269,7 @@ MODULE_PARM_DESC(volume_mode, > > > > module_param_named(volume_capabilities, volume_capabilities, uint, 0444); > > MODULE_PARM_DESC(volume_capabilities, > > - "Selects the mixer capabilites: 0=auto, 1=volume and mute, > > 2=mute only"); > > + "Selects the mixer capabilities: 0=auto, 1=volume and mute, > > 2=mute only"); > > > > module_param_named(volume_control, volume_control_allowed, bool, 0444); > > MODULE_PARM_DESC(volume_control, -- 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] platform/x86: thinkpad_acpi: Mark expected switch fall-throughs
On Wed, May 1, 2019 at 11:44 PM Gustavo A. R. Silva wrote: > > > > On 4/24/19 7:55 PM, ibm-a...@hmh.eng.br wrote: > > On Wed, Apr 24, 2019, at 16:05, Kees Cook wrote: > >> On Wed, Apr 24, 2019 at 11:15 AM Gustavo A. R. Silva > >> wrote: > >>> > >>> In preparation to enabling -Wimplicit-fallthrough, mark switch > >>> cases where we are expecting to fall through. > >>> > >>> This patch fixes the following warnings: > >>> > > [..] > > >>> > >>> Warning level 3 was used: -Wimplicit-fallthrough=3 > >>> > >>> Notice that, in this particular case, the code comments are modified > >>> in accordance with what GCC is expecting to find. > >>> > >>> This patch is part of the ongoing efforts to enable > >>> -Wimplicit-fallthrough. > >>> > >>> Signed-off-by: Gustavo A. R. Silva > >> > >> Reviewed-by: Kees Cook > > > > Acked-by: Henrique de Moraes Holschuh > > > > Thank you both, Kees and Henrique. > > Friendly ping: > > Who can take this? Pushed to my review and testing queue, thanks! > > Thanks > -- > Gustavo -- 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] leds: tpacpi: cleanup for Thinkpad ACPI led
On Mon, Apr 29, 2019 at 6:22 PM Pavel Machek wrote: > > > Make error returns more consistent... no behaviour change intended. > Pushed to my review and testing queue, thanks! P.S. I fixed prefix accordingly. > Signed-off-by: Pavel Machek > > diff --git a/drivers/platform/x86/thinkpad_acpi.c > b/drivers/platform/x86/thinkpad_acpi.c > index 726341f..7008a7f 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -5808,7 +5808,7 @@ static int led_set_status(const unsigned int led, > return -EPERM; > if (!acpi_evalf(led_handle, NULL, NULL, "vdd", > (1 << led), led_sled_arg1[ledstatus])) > - rc = -EIO; > + return -EIO; > break; > case TPACPI_LED_OLD: > /* 600e/x, 770e, 770x, A21e, A2xm/p, T20-22, X20 */ > @@ -5832,10 +5832,10 @@ static int led_set_status(const unsigned int led, > return -EPERM; > if (!acpi_evalf(led_handle, NULL, NULL, "vdd", > led, led_led_arg1[ledstatus])) > - rc = -EIO; > + return -EIO; > break; > default: > - rc = -ENXIO; > + return -ENXIO; > } > > if (!rc) > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) > http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- 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] Add Lenovo PrivacyGuard feature found in T480s, T490, T490s
On Tue, May 21, 2019 at 3:56 PM Alexander Schremmer wrote: > I wonder whether you have received my kernel patch, referenced below. It > might have been caught by the spam filter. Yes, you may always consult with patchwork [1]. If it's they, it won't be lost. [1]: https://patchwork.kernel.org/project/platform-driver-x86/list/ -- 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] Add Lenovo PrivacyGuard feature found in T480s, T490, T490s
On Sun, May 19, 2019 at 7:46 PM Alexander Schremmer
wrote:
>
> From 6bfe30cae2be3f4fbe9f9990a4e83302569ff7e9 Mon Sep 17 00:00:00 2001
> From: Alexander Schremmer
> Date: Sun, 19 May 2019 18:13:05 +0200
> Subject: [PATCH] platform/x86: Add Lenovo ThinkPad PrivacyGuard.
>
> This feature is found optionally in T480s, T490, T490s.
>
> The feature is called lcdshadow and visible via
> /proc/acpi/ibm/lcdshadow.
>
> The ACPI methods \_SB.PCI0.LPCB.EC.HKEY.{GSSS,,TSSS,CSSS} are
> available in these machines. They get, set, toggle or change the state
> apparently.
>
> The patch was tested on a 5.0 series kernel on a T480s.
While being in my review and testing queue it doesn't produce any
warnings the ABI extension lacks of documentation part.
>
> Signed-off-by: Alexander Schremmer
> ---
> drivers/platform/x86/thinkpad_acpi.c | 108 +++
> 1 file changed, 108 insertions(+)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 71cfaf26efd1..f2603643b067 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9729,6 +9729,110 @@ static struct ibm_struct battery_driver_data = {
> .exit = tpacpi_battery_exit,
> };
>
> +/*
> + * LCD Shadow subdriver, for the Lenovo PrivacyGuard feature
> + */
> +
> +
> +static int lcdshadow_state;
> +
> +static int lcdshadow_on_off(bool state)
> +{
> + acpi_handle set_shadow_handle;
> + int output;
> +
> + if (ACPI_FAILURE(acpi_get_handle(
> + hkey_handle,
> + "",
> + &set_shadow_handle))) {
> + pr_warn("Thinkpad ACPI has no %s interface.\n", "");
> + return -EIO;
> + }
> +
> + if (!acpi_evalf(set_shadow_handle, &output, NULL, "dd", (int)state))
> + return -EIO;
> +
> + lcdshadow_state = state;
> + return 0;
> +}
> +
> +static int lcdshadow_set(bool on)
> +{
> + if (lcdshadow_state < 0 || lcdshadow_state == on)
> + return lcdshadow_state;
> + return lcdshadow_on_off(on);
> +}
> +
> +static int tpacpi_lcdshadow_init(struct ibm_init_struct *iibm)
> +{
> + acpi_handle get_shadow_handle;
> + int output;
> +
> + if (ACPI_FAILURE(acpi_get_handle(
> + hkey_handle,
> + "GSSS",
> + &get_shadow_handle))) {
> + lcdshadow_state = -ENODEV;
> + return 0;
> + }
> +
> + if (!acpi_evalf(get_shadow_handle, &output, NULL, "dd", 0))
> + return -EIO;
> + if (!(output & 0x1)) {
> + lcdshadow_state = -ENODEV;
> + return 0;
> + }
> + lcdshadow_state = output & 0x1;
> +
> + return 0;
> +}
> +
> +static void lcdshadow_resume(void)
> +{
> + if (lcdshadow_state >= 0)
> + lcdshadow_on_off(lcdshadow_state);
> +}
> +
> +static int lcdshadow_read(struct seq_file *m)
> +{
> + if (lcdshadow_state < 0) {
> + seq_puts(m, "status:\t\tnot supported\n");
> + } else {
> + seq_printf(m, "status:\t\t%d\n", lcdshadow_state);
> + seq_puts(m, "commands:\t0, 1\n");
> + }
> +
> + return 0;
> +}
> +
> +static int lcdshadow_write(char *buf)
> +{
> + char *cmd;
> + int state = -1;
> +
> + if (lcdshadow_state < 0)
> + return -ENODEV;
> +
> + while ((cmd = next_cmd(&buf))) {
> + if (strlencmp(cmd, "0") == 0)
> + state = 0;
> + else if (strlencmp(cmd, "1") == 0)
> + state = 1;
> + }
> +
> + if (state == -1)
> + return -EINVAL;
> +
> + return lcdshadow_set(state);
> +}
> +
> +static struct ibm_struct lcdshadow_driver_data = {
> + .name = "lcdshadow",
> + .resume = lcdshadow_resume,
> + .read = lcdshadow_read,
> + .write = lcdshadow_write,
> +};
> +
> /****
>
> *
> @@ -10210,6 +10314,10 @@ stati
Re: [ibm-acpi-devel] [PATCH 19/30] platform/x86: Use kmemdup rather than duplicating its implementation
On Wed, Jul 3, 2019 at 4:19 PM Fuqian Huang wrote:
>
> kmemdup is introduced to duplicate a region of memory in a neat way.
> Rather than kmalloc/kzalloc + memset, which the programmer needs to
> write the size twice (sometimes lead to mistakes), kmemdup improves
> readability, leads to smaller code and also reduce the chances of mistakes.
> Suggestion to use kmemdup rather than using kmalloc/kzalloc + memset.
>
Please, split on driver basis and use correct prefix.
> Signed-off-by: Fuqian Huang
> ---
> drivers/platform/x86/asus-wmi.c | 3 +--
> drivers/platform/x86/thinkpad_acpi.c | 17 +++--
> 2 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 9b18a184e0aa..472b317ad814 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -260,12 +260,11 @@ static int asus_wmi_evaluate_method_agfn(const struct
> acpi_buffer args)
> * Copy to dma capable address otherwise memory corruption occurs as
> * bios has to be able to access it.
> */
> - input.pointer = kzalloc(args.length, GFP_DMA | GFP_KERNEL);
> + input.pointer = kmemdup(args.pointer, args.length, GFP_DMA |
> GFP_KERNEL);
> input.length = args.length;
> if (!input.pointer)
> return -ENOMEM;
> phys_addr = virt_to_phys(input.pointer);
> - memcpy(input.pointer, args.pointer, args.length);
>
> status = asus_wmi_evaluate_method(ASUS_WMI_METHODID_AGFN,
> phys_addr, 0, &retval);
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 7bde4640ef34..d379bdf98a0f 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -3647,22 +3647,19 @@ static int __init hotkey_init(struct ibm_init_struct
> *iibm)
> goto err_exit;
>
> /* Set up key map */
> - hotkey_keycode_map = kmalloc(TPACPI_HOTKEY_MAP_SIZE,
> - GFP_KERNEL);
> - if (!hotkey_keycode_map) {
> - pr_err("failed to allocate memory for key map\n");
> - res = -ENOMEM;
> - goto err_exit;
> - }
> -
> keymap_id = tpacpi_check_quirks(tpacpi_keymap_qtable,
> ARRAY_SIZE(tpacpi_keymap_qtable));
> BUG_ON(keymap_id >= ARRAY_SIZE(tpacpi_keymaps));
> dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_HKEY,
>"using keymap number %lu\n", keymap_id);
>
> - memcpy(hotkey_keycode_map, &tpacpi_keymaps[keymap_id],
> - TPACPI_HOTKEY_MAP_SIZE);
> + hotkey_keycode_map = kmemdup(&tpacpi_keymaps[keymap_id],
> + TPACPI_HOTKEY_MAP_SIZE, GFP_KERNEL);
> + if (!hotkey_keycode_map) {
> + pr_err("failed to allocate memory for key map\n");
> + res = -ENOMEM;
> + goto err_exit;
> + }
>
> input_set_capability(tpacpi_inputdev, EV_MSC, MSC_SCAN);
> tpacpi_inputdev->keycodesize = TPACPI_HOTKEY_MAP_TYPESIZE;
> --
> 2.11.0
>
--
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 v2 21/35] platform/x86/thinkpad: Use kmemdup rather than duplicating its implementation
On Wed, Jul 3, 2019 at 7:30 PM Fuqian Huang wrote:
>
> kmemdup is introduced to duplicate a region of memory in a neat way.
> Rather than kmalloc/kzalloc + memcpy, which the programmer needs to
> write the size twice (sometimes lead to mistakes), kmemdup improves
> readability, leads to smaller code and also reduce the chances of mistakes.
> Suggestion to use kmemdup rather than using kmalloc/kzalloc + memcpy.
>
Pushed to my review and testing queue, thanks!
> Signed-off-by: Fuqian Huang
> ---
> Changes in v2:
> - Fix a typo in commit message (memset -> memcpy)
>
> drivers/platform/x86/thinkpad_acpi.c | 17 +++--
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 7bde4640ef34..d379bdf98a0f 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -3647,22 +3647,19 @@ static int __init hotkey_init(struct ibm_init_struct
> *iibm)
> goto err_exit;
>
> /* Set up key map */
> - hotkey_keycode_map = kmalloc(TPACPI_HOTKEY_MAP_SIZE,
> - GFP_KERNEL);
> - if (!hotkey_keycode_map) {
> - pr_err("failed to allocate memory for key map\n");
> - res = -ENOMEM;
> - goto err_exit;
> - }
> -
> keymap_id = tpacpi_check_quirks(tpacpi_keymap_qtable,
> ARRAY_SIZE(tpacpi_keymap_qtable));
> BUG_ON(keymap_id >= ARRAY_SIZE(tpacpi_keymaps));
> dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_HKEY,
>"using keymap number %lu\n", keymap_id);
>
> - memcpy(hotkey_keycode_map, &tpacpi_keymaps[keymap_id],
> - TPACPI_HOTKEY_MAP_SIZE);
> + hotkey_keycode_map = kmemdup(&tpacpi_keymaps[keymap_id],
> + TPACPI_HOTKEY_MAP_SIZE, GFP_KERNEL);
> + if (!hotkey_keycode_map) {
> + pr_err("failed to allocate memory for key map\n");
> + res = -ENOMEM;
> + goto err_exit;
> + }
>
> input_set_capability(tpacpi_inputdev, EV_MSC, MSC_SCAN);
> tpacpi_inputdev->keycodesize = TPACPI_HOTKEY_MAP_TYPESIZE;
> --
> 2.11.0
>
--
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] platform/x86: Add Lenovo ThinkPad PrivacyGuard.
lcdshadow_on_off(lcdshadow_state);
> +}
> +
> +static int lcdshadow_read(struct seq_file *m)
> +{
> + if (lcdshadow_state < 0) {
> + seq_puts(m, "status:\t\tnot supported\n");
> + } else {
> + seq_printf(m, "status:\t\t%d\n", lcdshadow_state);
> + seq_puts(m, "commands:\t0, 1\n");
> + }
> +
> + return 0;
> +}
> +
> +static int lcdshadow_write(char *buf)
> +{
> + char *cmd;
> + int state = -1;
> +
> + if (lcdshadow_state < 0)
> + return -ENODEV;
> +
> + while ((cmd = next_cmd(&buf))) {
> + if (strlencmp(cmd, "0") == 0)
> + state = 0;
> + else if (strlencmp(cmd, "1") == 0)
> + state = 1;
> + }
> +
> + if (state == -1)
> + return -EINVAL;
> +
> + return lcdshadow_set(state);
> +}
> +
> +static struct ibm_struct lcdshadow_driver_data = {
> + .name = "lcdshadow",
> + .resume = lcdshadow_resume,
> + .read = lcdshadow_read,
> + .write = lcdshadow_write,
> +};
> +
> /
>
> *
> @@ -10210,6 +10318,10 @@ static struct ibm_init_struct ibms_init[] __initdata
> = {
> .init = tpacpi_battery_init,
> .data = &battery_driver_data,
> },
> + {
> + .init = tpacpi_lcdshadow_init,
> + .data = &lcdshadow_driver_data,
> + },
> };
>
> static int __init set_ibm_param(const char *val, const struct kernel_param
> *kp)
> --
> 2.20.1
>
--
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] platform/x86: Add Lenovo ThinkPad PrivacyGuard.
(lcdshadow_state);
> +}
> +
> +static int lcdshadow_read(struct seq_file *m)
> +{
> + if (lcdshadow_state < 0) {
> + seq_puts(m, "status:\t\tnot supported\n");
> + } else {
> + seq_printf(m, "status:\t\t%d\n", lcdshadow_state);
> + seq_puts(m, "commands:\t0, 1\n");
> + }
> +
> + return 0;
> +}
> +
> +static int lcdshadow_write(char *buf)
> +{
> + char *cmd;
> + int state = -1;
> +
> + if (lcdshadow_state < 0)
> + return -ENODEV;
> +
> + while ((cmd = next_cmd(&buf))) {
> + if (strlencmp(cmd, "0") == 0)
> + state = 0;
> + else if (strlencmp(cmd, "1") == 0)
> + state = 1;
> + }
> +
> + if (state == -1)
> + return -EINVAL;
> +
> + return lcdshadow_set(state);
> +}
> +
> +static struct ibm_struct lcdshadow_driver_data = {
> + .name = "lcdshadow",
> + .resume = lcdshadow_resume,
> + .read = lcdshadow_read,
> + .write = lcdshadow_write,
> +};
> +
> /
>
> *
> @@ -10195,6 +10303,10 @@ static struct ibm_init_struct ibms_init[] __initdata
> = {
> .init = tpacpi_battery_init,
> .data = &battery_driver_data,
> },
> + {
> + .init = tpacpi_lcdshadow_init,
> + .data = &lcdshadow_driver_data,
> + },
> };
>
> static int __init set_ibm_param(const char *val, const struct kernel_param
> *kp)
> --
> 2.20.1
>
--
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] platform/x86: Add Lenovo ThinkPad PrivacyGuard.
On Fri, Oct 25, 2019 at 2:33 PM Pali Rohár wrote:
> On Thursday 22 August 2019 13:48:33 Alexander Schremmer wrote:
> > This feature is found optionally in T480s, T490, T490s.
> >
> > The feature is called lcdshadow and visible via
> > /proc/acpi/ibm/lcdshadow.
> >
> > The ACPI methods \_SB.PCI0.LPCB.EC.HKEY.{GSSS,,TSSS,CSSS} are
> > available in these machines. They get, set, toggle or change the state
> > apparently.
> >
> > The patch was tested on a 5.0 series kernel on a T480s.
> > + echo '0' >/proc/acpi/ibm/lcdshadow
> > + echo '1' >/proc/acpi/ibm/lcdshadow
> Hello! Is not the whole /proc/apci/ibm API for new things obsoleted or
> deprecated?
> And should not rather it use platform driver in /sys/ (class?) namespace?
There is an ongoing discussion about API to this kind of devices
somewhere in GPU mailing lists.
This interface is custom and probably shall never have been appeared.
However no-one prevented this from happening.
--
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] thinkpad_acpi: Add sysfs entry for lcdshadow feature
On Thu, Feb 20, 2020 at 9:48 AM Nitin Joshi wrote: > > This feature is supported on some Thinkpad products like T490s, Thinkpad > X1 yoga 4th Gen etc . The lcdshadow feature can be enabled and disabled > when user press "Fn" + "D" key. Currently, no user feedback is given for > this action. Adding as sysfs entry allows userspace to show an On Screen > Display whenever the setting changes. > > Summary of changes is mentioned below : > > - Added TP_HKEY_EV_LCDSHADOW_CHANGED for consistency inside the driver > - Added unmapped LCDSHADOW to keymap > - Added lcdshadow_get function to read value using ACPI > - Added lcdshadow_refresh function to re-read value and send notification > - Added sysfs group creation to tpaci_lcdshadow_init > - Added lcdshadow_exit to remove sysfs group again > - Implemented lcdshadow_enable_show/lcdshadow_enable_store > - Added handler to tpacpi_driver_event to update refresh lcdshadow > - Explicitly call tpacpi_driver_event for extended keyset Adding custom PrivacyGuard support to this driver was my mistake, There is a discussion [1] how to do this in generic way to cover other possible users. I Cc this to people from that discussion. [1]: https://lore.kernel.org/dri-devel/cal_quvrknssvvxn3q_se0hrziw2otns3ennoehyhvcicrq9...@mail.gmail.com/ -- 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] thinkpad_acpi: Add support for dual fan control on select models
On Sun, Apr 5, 2020 at 11:00 PM la...@apache.org wrote:
>
> This patch allows controlling multiple fans as if they were a single fan.
>
> This adds P52, P72, X1E, and X1E gen to dual fan quirks. Both fans are
> controlled together.
>
> Tested on an X1 Extreme Gen2.
>
> The patch is defensive, it adds only specific supported machines, and falls
> back to the old behavior if both fans cannot be controlled.
> It does attempt single fan control for all previously white-listed Thinkpads.
>
> 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
> (https://github.com/vmatare/thinkfan/issues/58).
> (Thanks to Github users voidworker, and civic9.)
>
> The BIOS ids for P52/P72 and X1E are taken from there. The X1E gen2 id is
> verified on my machine.
>
It doesn't apply :-(
Please, fix and resend.
> Signed-off-by: Lars Hofhansl
>
> ---
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index da794dcfdd92..8d46b4c2bde8 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8325,11 +8325,20 @@ 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) {
> +if (!fan_select_fan2() ||
> +!acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) {
> +fan_select_fan1();
> +pr_warn("Couldn't set 2nd fan level, disabling support\n");
> +tp_features.second_fan = 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,16 @@ 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) {
> +if (!fan_select_fan2() ||
> +!acpi_ec_write(fan_status_offset, level)) {
> +fan_select_fan1();
> +pr_warn("Couldn't set 2nd fan level, disabling support\n");
> +tp_features.second_fan = 0;
> +}
> +fan_select_fan1();
> +
> +}
> if (!acpi_ec_write(fan_status_offset, level))
> return -EIO;
> else
> @@ -8772,6 +8791,9 @@ 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', '2', 'C', TPACPI_FAN_2FAN),/* P52 / P72 */
> +TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2FAN),/* X1 Extreme (1st
> gen) */
> +TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2FAN),/* X1 Extreme (2nd
> gen) */
> };
>
> static int __init fan_init(struct ibm_init_struct *iibm)
> @@ -8813,8 +8835,7 @@ 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");
> }
> } else {
> pr_err("ThinkPad ACPI EC access misbehaving, fan status and
> control unavailable\n");
--
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 v2] thinkpad_acpi: Add support for dual fan control on select models
On Fri, Apr 17, 2020 at 11:15 PM Lars wrote:
>
> This patch allows controlling multiple fans as if they were a single fan.
>
> This adds P52, P72, X1E, and X1E gen2 to dual fan quirks. Both fans are
> controlled together.
>
> Tested on an X1 Extreme Gen2.
>
> The patch is defensive, it adds only specific supported machines, and falls
> back to the old behavior if both fans cannot be controlled.
> However, it does attempt single fan control for all previously white-listed
> Thinkpads.
>
> 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
> (https://github.com/vmatare/thinkfan/issues/58).
> (Thanks to Github users voidworker, and civic9.)
>
> The BIOS ids for P52/P72 and X1E are taken from there. The X1E gen2 id is
> verified on my machine.
>
Thanks for an update. I have pushed it to my review and testing queue, thanks!
JFYI: there are two issues (I have fixed them, no need to resend) with
this. Commit message lines are too long and...
> (In the first version my mail client botched the white-spacing - my
> apologies, this is my first Kernel patch. Used git send-email and gmail this
> time.)
...this kind of comments should go after cut line ('---' below).
> Signed-off-by: Lars
> ---
--
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 v2] thinkpad_acpi: Add support for dual fan control on select models
+Cc: people who lately involved in 2nd fan discussions here and there
Lars, also one question regarding the code below.
On Fri, Apr 17, 2020 at 11:15 PM Lars wrote:
>
> This patch allows controlling multiple fans as if they were a single fan.
>
> This adds P52, P72, X1E, and X1E gen2 to dual fan quirks. Both fans are
> controlled together.
>
> Tested on an X1 Extreme Gen2.
>
> The patch is defensive, it adds only specific supported machines, and falls
> back to the old behavior if both fans cannot be controlled.
> However, it does attempt single fan control for all previously white-listed
> Thinkpads.
>
> 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
> (https://github.com/vmatare/thinkfan/issues/58).
> (Thanks to Github users voidworker, and civic9.)
>
> The BIOS ids for P52/P72 and X1E are taken from there. The X1E gen2 id is
> verified on my machine.
>
> (In the first version my mail client botched the white-spacing - my
> apologies, this is my first Kernel patch. Used git send-email and gmail this
> time.)
>
> Signed-off-by: Lars
> ---
> drivers/platform/x86/thinkpad_acpi.c | 33 +++-
> 1 file changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 8eaadbaf8ffa..cbc0e85d89d2 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8324,11 +8324,20 @@ 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) {
> + if (!fan_select_fan2() ||
> + !acpi_evalf(sfan_handle, NULL, NULL, "vd",
> level)) {
> + fan_select_fan1();
> + pr_warn("Couldn't set 2nd fan level,
> disabling support\n");
> + tp_features.second_fan = 0;
> + }
> + fan_select_fan1();
> + }
> + if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level))
> + return -EIO;
> break;
>
> case TPACPI_FAN_WR_ACPI_FANS:
> @@ -8345,6 +8354,16 @@ 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) {
> + if (!fan_select_fan2() ||
> + !acpi_ec_write(fan_status_offset, level)) {
> + fan_select_fan1();
(1)
> + pr_warn("Couldn't set 2nd fan level,
> disabling support\n");
> + tp_features.second_fan = 0;
> + }
> + fan_select_fan1();
(2)
I'm not sure I got a logic behind this. Why do you need to call it twice?
> +
> + }
> if (!acpi_ec_write(fan_status_offset, level))
> return -EIO;
> else
> @@ -8771,6 +8790,9 @@ 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', '2', 'C', TPACPI_FAN_2FAN), /* P52 / P72 */
> + TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2FAN), /* X1 Extreme (1st
> gen) */
> + TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2FAN), /* X1 Extreme (2nd
> gen) */
> };
>
> static int __init fan_init(struct ibm_init_struct *iibm)
> @@ -8812,8 +8834,7 @@ static int __init fan_init(struct ibm_init_struct *iibm)
> fan_quirk1_setup();
> if (quirk
Re: [ibm-acpi-devel] [PATCH v3] thinkpad_acpi: Add support for dual fan control on select models
On Thu, Apr 23, 2020 at 7:56 PM Lars wrote:
>
> This patch allows controlling multiple fans as if they were a single fan.
>
> This adds P52, P72, X1E, and X1E gen2 to dual fan quirks.
> Both fans are controlled together.
>
> Tested on an X1 Extreme Gen2.
>
> 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]. Thanks to Github users voidworker and civic9.
GitHub
>
> The BIOS ids are taken from there. The X1E gen2 id is verified on my machine.
>
I got two mails, which one is correct?
So, please send a new version only one time, thanks.
Some comments below.
> [1]: vmatare/thinkfan#58
Please, use full URL her.
...
> + if (((level < 0) || (level > 7)))
> return -EINVAL;
Maybe you didn't check what I did for previous version...
Here is too many parentheses.
> + 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), /* X1 Extreme (1st
> gen) */
> + TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL), /* X1 Extreme (2nd
> gen) */
This has been expanded, but commit message still old, please, update
commit message as well (and perhaps give a credit to people who
suggested / tested other models).
--
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
#x27;, '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
___
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 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] platform/x86: thinkpad_acpi: Remove always false 'value < 0' statement
On Wed, Apr 29, 2020 at 12:06 PM Xiongfeng Wang wrote: > > Since 'value' is declared as unsigned long, the following statement is > always false. > value < 0 > > So let's remove it. It's bogus. The warning here doesn't tell anything useful, so doesn't the patch. -- 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] Low Latency Tolerance preventing Intel Package from entering deep sleep states
LTR: RAW: 0x8000800 > Non-Snoop(ns): 0Snoop(ns): 0 > EVA LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > SOUTHPORT_C LTR: RAW: 0x88468846 > Non-Snoop(ns): 71680Snoop(ns): 71680 <- > HD_AUDIOLTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > CNV LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > LPSSLTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > SOUTHPORT_D LTR: RAW: 0x8c548c54 > Non-Snoop(ns): 2752512 Snoop(ns): 2752512 > SOUTHPORT_E LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > CAMERA LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > ESPILTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > SCC LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > ISH LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > UFSX2 LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > EMMCLTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > WIGIG LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > CURRENT_PLATFORMLTR: RAW: 0x40201 > Non-Snoop(ns): 0Snoop(ns): 0 > AGGREGATED_SYSTEM LTR: RAW: 0x904824 > Non-Snoop(ns): 0Snoop(ns): 0 > > Does anybody know what's going on or how to debug this further? > > As stated above, I was able to work around this problem by ignoring > SOUTHPORT_A via /sys/kernel/debug/pmc_core/ltr_ignore. > There has to be a better way, and I'm sure I'm not the only one running into > this. > > Thanks. > > -- Lars -- 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] Low Latency Tolerance preventing Intel Package from entering deep sleep states
+Cc: Adrian On Fri, May 22, 2020 at 9:15 AM la...@apache.org wrote: > > Thanks David! > > With this I tracked down the SD Card Reader (Genesys Logic, Inc Device 9755) > as the culprit. > These are standard in many ThinkPads. > The curious part is that resume from suspend (S3 or S0iX) also fixes the > problem. > Looks like the driver is not initializing correctly at boot time. > > Transcript: > > $ cat /sys/kernel/debug/pmc_core/ltr_show | grep SOUTHPORT > SOUTHPORT_A LTR: RAW: 0x88018c01 > Non-Snoop(ns): 1024 Snoop(ns): 32768 > SOUTHPORT_B LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > SOUTHPORT_C LTR: RAW: 0x9f409f4 > Non-Snoop(ns): 0Snoop(ns): 0 > SOUTHPORT_D LTR: RAW: 0x88aa88aa > Non-Snoop(ns): 174080 Snoop(ns): 174080 > SOUTHPORT_E LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > > $ lspci -t > -[:00]-+-00.0 >+-01.0-[01]--+-00.0 >|\-00.1 >+-02.0 >+-04.0 >+-08.0 >+-12.0 >+-14.0 >+-14.2 >+-15.0 >+-16.0 >+-1c.0-[53]00.0 >+-1d.0-[02]00.0 >+-1d.6-[52]00.0 >+-1e.0 >+-1f.0 >+-1f.3 >+-1f.4 >+-1f.5 >\-1f.6 > > $ lspci | grep 53 > 53:00.0 SD Host controller: Genesys Logic, Inc Device 9755 > > $ cat /sys/bus/pci/devices/\:53\:00.0/power/control > auto > > $ echo 1 > /sys/bus/pci/devices/\:53\:00.0/remove > 1 > > $ cat /sys/kernel/debug/pmc_core/ltr_show | grep SOUTHPORT > SOUTHPORT_A LTR: RAW: 0x8010c01 > Non-Snoop(ns): 0Snoop(ns): 0 > SOUTHPORT_B LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > SOUTHPORT_C LTR: RAW: 0x9f409f4 > Non-Snoop(ns): 0Snoop(ns): 0 > SOUTHPORT_D LTR: RAW: 0x8c548c54 > Non-Snoop(ns): 2752512 Snoop(ns): 2752512 > SOUTHPORT_E LTR: RAW: 0x0 > Non-Snoop(ns): 0Snoop(ns): 0 > > Cheers. > > -- Lars > > > > > > > > > On Tuesday, May 19, 2020, 9:03:53 AM PDT, David E. Box > wrote: > > > > > > > > > Does anybody know what's going on or how to debug this further? > > > > As stated above, I was able to work around this problem by > > > > ignoring SOUTHPORT_A via /sys/kernel/debug/pmc_core/ltr_ignore. > > > > There has to be a better way, and I'm sure I'm not the only one > > > > running into this. > > ltr_show shows the PMC's (Power Management Controller) view of SoC > devices and busses. The SOUTHPORTs are the PCIe root ports on your > system. When you run lspci they are the PCI bridges. Generally, the > bridges are enumerated in the same order as the SOUTHPORTs, so > SOUTHPORT_A is your first bridge and the device attached to it (shown > in lspci -t) is the device that was blocking deeper PC states according > to your debug. > > Determine what this device is on your system. If the ltr was low it's > because that is what the device requested. You should first check that > runtime pm is enabled for the device. To do this, check the control > file in /sys/bus/pci/devices//power, where :BB:DD.F > is the enumeration of your device as shown in lspci. If it is 'on' then > runtime pm is disabled. To enable it echo 'auto' into the file with > root privileges. Enabling runtime pm should allow the driver to reduce > functionality of the device when idle. This should lead to a larger > latency request on the PCI bus which should be reflected in ltr_show. > You can see if the device is actually runtime suspended and how much > time it's been suspended (or active) by reading the associated files in > the power folder. > > If this doesn't work, then it's possible that your device doesn't > support runtime pm. This may be purposely for reliability reasons or > the driver may just lack support. Check forums discussing issues with > the device and look for possible options in the driver to force pm > support (generally this will be centered around enabling ASPM). > > You can also download powertop to see the package c-state residencies > more clearly as percentages of time. powertop also has a tunables tab > that will show the status of runtime pm on all devices on the system > and allow you to enable them individually. > > > David > -- 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] platform/x86: thinkpad_acpi: lap or desk mode interface
On Mon, Jun 29, 2020 at 10:23 PM Mark Pearson wrote:
Thanks for the patch, my comments below.
> Newer Lenovo Thinkpad platforms have support to identify whether the
> system is on-lap or not using an ACPI DYTC event from the firmware.
>
> This patch provides the ability to retrieve the current mode via sysfs
> entrypoints and will be used by userspace for thermal mode and WWAN
> functionality
Please use proper indentation (no need to have spaces) and punctuation
(like period at the end of sentences).
...
> drivers/platform/x86/thinkpad_acpi.c | 111 ++-
> 1 file changed, 109 insertions(+), 2 deletions(-)
You specifically added a new ABI, where is documentation? It's a show stopper.
...
> + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
> + "dytc_lapmode");
One line?
...
> + int output;
> +
> + output = dytc_command(DYTC_CMD_GET);
> + if ((output == -ENODEV) || (output == -EIO))
> + return output;
Why not simple
if (output < 0)
return output;
Btw, how will this survive the 31st bit (if some method would like to use it)?
I think your prototype should be
int foo(cmd, *output);
...
> + new_state = dytc_lapmode_get();
> + if ((new_state == -ENODEV) || (new_state == -EIO) || (new_state ==
> dytc_lapmode))
> + return;
What about other errors?
What about MSB being set?
...
> + dytc_lapmode = dytc_lapmode_get();
> +
> + if (dytc_lapmode < 0 && dytc_lapmode != -ENODEV)
> + return dytc_lapmode;
> + res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
> + &dytc_attr_group);
> +
> + return res;
return ...(...);
So, we create a group for all possible error cases but ENODEV. Why?
> +}
...
> + sysfs_remove_group(&tpacpi_pdev->dev.kobj,
> + &dytc_attr_group);
One line?
...
> +static struct ibm_struct dytc_driver_data = {
> + .name = "dytc",
> + .exit = dytc_exit
Comma is missed.
> +};
--
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] platform/x86: thinkpad_acpi: not loading brightness_init when _BCL invalid
On Thu, Jul 2, 2020 at 11:55 AM Aaron Ma wrote:
>
> When _BCL invalid, disable thinkpad_acpi backlight brightness control.
>
> brightness_enable is already checked at the beginning,
> Always print notice when enabled brightness control.
Why?
...
> + pr_notice("thinkpad_acpi native brightness control enabled\n");
'notice' level is quite high, why do we spam users with this?
--
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] [External] Re: [PATCH v4] platform/x86: thinkpad_acpi: lap or desk mode interface
On Thu, Jul 2, 2020 at 1:45 PM Mark Pearson wrote: > On 7/2/2020 5:29 AM, Andy Shevchenko wrote: > > On Mon, Jun 29, 2020 at 10:23 PM Mark Pearson > > wrote: ... > > You specifically added a new ABI, where is documentation? It's a show > > stopper. > Ah - my apologies I didn't know that was a requirement. > > Any pointers on where to add it? I looked in Documentation/ABI and I > couldn't find anything around thinkpad_acpi to add this to. > Should there be a sysfs-devices-platform-thinkpad_acpi file? > > If that's the case I'm happy to look at creating that but as a first > time kernel contributor would you object if I took that on as a separate > exercise rather than as part of this patch. I'm guessing it would need > more time, care and reviewers from other contributors to the > thinkpad_acpi.c driver Since it's an old driver its ABI is listed here https://elixir.bootlin.com/linux/latest/source/Documentation/admin-guide/laptops/thinkpad-acpi.rst ... > > Why not simple > > > > if (output < 0) > > return output; > Agreed. I'll fix > > I think your prototype should be > > > > int foo(cmd, *output); > Looking at it again - I agree. And after returning only error codes, you may do above as simple as int ret; ret = ...(.., &output); if (ret) return ret; ... return 0; ... > As a minor note I think these all arose because of getting checkpatch to > run cleanly. I prefer one line too and if that's your preference it > works for me. Checkpatch shouldn't complain (update it if it does). -- 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] platform/x86: thinkpad_acpi: not loading brightness_init when _BCL invalid
On Thu, Jul 2, 2020 at 1:51 PM Aaron Ma wrote:
> On 7/2/20 5:30 PM, Andy Shevchenko wrote:
> > On Thu, Jul 2, 2020 at 11:55 AM Aaron Ma wrote:
> >>
> >> When _BCL invalid, disable thinkpad_acpi backlight brightness control.
> >>
> >> brightness_enable is already checked at the beginning,
> >
> >> Always print notice when enabled brightness control.
> >
> > Why?
> >
>
> Default brightness_enable = 2, so this message will always be printed as
> before
> Actually no change here.
>
> > ...
> >
> >> + pr_notice("thinkpad_acpi native brightness control enabled\n");
> >
> > 'notice' level is quite high, why do we spam users with this?
> >
>
> Like above.
>
> Another reason is most thinkpads are using native gpu driver to control
> brightness, notice when thinkpad_acpi brightness is enabled.
So, based on the above, please elaborate and explain all this in the
commit message of new version, thanks!
--
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 v5] platform/x86: thinkpad_acpi: lap or desk mode interface
On Fri, Jul 3, 2020 at 4:24 AM Mark Pearson wrote:
>
> Newer Lenovo Thinkpad platforms have support to identify whether the
> system is on-lap or not using an ACPI DYTC event from the firmware.
>
> This patch provides the ability to retrieve the current mode via sysfs
> entrypoints and will be used by userspace for thermal mode and WWAN
> functionality
Hans, do you think it's good to have custom ABI for this? I think you
may be know better what types of ABI we already have for such thing.
> Co-developed-by: Nitin Joshi
> Signed-off-by: Nitin Joshi
> Reviewed-by: Sugumaran
> Reviewed-by: Bastien Nocera
> Signed-off-by: Mark Pearson
> ---
> Changes in v5:
> - Updated with review changes from Andy Shevchenko
> - Added ABI information to thinkpad-acpi.rst
> - improved error handling and parameter passing as recommended
> - code cleanup as recommended
> - added review tag from bnocera
> Changes in v4:
> - Correct hotkey event comment as we're handling event
> - Remove unnecessary check in dytc_lapmode_refresh
> Changes in v3:
> - Fixed inaccurate comments
> - Used BIT macro to check lapmode bit setting as recommended and update
> define name
> - Check for new_state == dytc_lapmode in dytc_lapmode_refresh
> Changes in v2:
> - cleaned up initialisation sequence to be cleaner and avoid spamming
> platforms that don't have DYTC with warning message. Tested on P52
> - Adding platform-driver-x86 mailing list for review as requested
>
> .../admin-guide/laptops/thinkpad-acpi.rst | 15 +++
> drivers/platform/x86/thinkpad_acpi.c | 111 +-
> 2 files changed, 124 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 822907dcc845..99066aa8d97b 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -50,6 +50,7 @@ detailed description):
> - WAN enable and disable
> - UWB enable and disable
> - LCD Shadow (PrivacyGuard) enable and disable
> + - Lap mode sensor
>
> A compatibility table by model and feature is maintained on the web
> site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1432,6 +1433,20 @@ The first command ensures the best viewing angle and
> the latter one turns
> on the feature, restricting the viewing angles.
>
>
> +DYTC Lapmode sensor
> +--
> +
> +sysfs: dytc_lapmode
> +
> +Newer thinkpads and mobile workstations have the ability to determine if
> +the device is in deskmode or lapmode. This feature is used by user space
> +to decide if WWAN transmission can be increased to maximum power and is
> +also useful for understanding the different thermal modes available as
> +they differ between desk and lap mode.
> +
> +The property is read-only. If the platform doesn't have support the sysfs
> +class is not created.
> +
> EXPERIMENTAL: UWB
> -
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index ff7f0a4f2475..037eb77414f9 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4022,8 +4022,8 @@ static bool hotkey_notify_6xxx(const u32 hkey,
> return true;
> case TP_HKEY_EV_THM_CSM_COMPLETED:
> pr_debug("EC reports: Thermal Control Command set completed
> (DYTC)\n");
> - /* recommended action: do nothing, we don't have
> -* Lenovo ATM information */
> + /* Thermal event - pass on to event handler */
> + tpacpi_driver_event(hkey);
> return true;
> case TP_HKEY_EV_THM_TRANSFM_CHANGED:
> pr_debug("EC reports: Thermal Transformation changed
> (GMTS)\n");
> @@ -9795,6 +9795,105 @@ static struct ibm_struct lcdshadow_driver_data = {
> .write = lcdshadow_write,
> };
>
> +/*
> + * DYTC subdriver, for the Lenovo lapmode feature
> + */
> +
> +#define DYTC_CMD_GET 2 /* To get current IC function and mode */
> +#define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
> +
> +static bool dytc_lapmode;
> +
> +static void dytc_lapmode_notify_change(void)
> +{
> + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
> +}
> +
> +static int dytc_command(int command, int *output)
> +{
> + acpi_handle dytc_handle;
> +
> + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC
Re: [ibm-acpi-devel] [v2][PATCH] platform/x86: thinkpad_acpi: not loading brightness_init when _BCL invalid
On Thu, Jul 2, 2020 at 2:07 PM Aaron Ma wrote:
>
> When _BCL invalid, disable thinkpad_acpi backlight brightness control.
>
> brightness_enable is already checked at the beginning.
> Most new thinkpads are using GPU driver to control brightness now,
> print notice when enabled brightness control even when brightness_enable = 1.
> + } else if (!tp_features.bright_acpimode) {
> + pr_notice("thinkpad_acpi backlight interface not
> available\n");
> + return 1;
> }
>
> + pr_notice("thinkpad_acpi native brightness control enabled\n");
In both cases don't you see the duplication of module name in the messages?
--
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 v5] platform/x86: thinkpad_acpi: lap or desk mode interface
On Fri, Jul 10, 2020 at 11:00 AM Hans de Goede wrote: > > Hi, > > On 7/9/20 8:02 PM, Andy Shevchenko wrote: > > On Fri, Jul 3, 2020 at 4:24 AM Mark Pearson wrote: > >> > >> Newer Lenovo Thinkpad platforms have support to identify whether the > >> system is on-lap or not using an ACPI DYTC event from the firmware. > >> > >> This patch provides the ability to retrieve the current mode via sysfs > >> entrypoints and will be used by userspace for thermal mode and WWAN > >> functionality > > > > Hans, do you think it's good to have custom ABI for this? I think you > > may be know better what types of ABI we already have for such thing. > > Actually, Mark asked me the same question before submitting his > patch upstream. I'm never a fan of custom ABI for this. But for now > the solution Lenovo has chosen to deal with thermal management > issues on modern hw is unique to Lenovo and we do not have anything > like this anywhere else. > > So for now I believe that a custom ABI is best. > > If we see this becoming a common feature on more platforms then we can > design a generic API for it once we have a better idea how this would > look like when implemented by others and then thinkpad_acpi can easily > add support for the new generic interface, while keeping its own > custom interface for backward compatibility. Thank you very much for the elaborative comment, appreciated! -- 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] [v3][PATCH] platform/x86: thinkpad_acpi: not loading brightness_init when _BCL invalid
On Fri, Jul 10, 2020 at 4:56 AM Aaron Ma wrote:
>
> When _BCL invalid, disable thinkpad_acpi backlight brightness control.
>
> brightness_enable is already checked at the beginning.
> Most new thinkpads are using GPU driver to control brightness now,
> print notice when enabled brightness control even when brightness_enable = 1.
>
Pushed to my review and testing queue, thanks!
> Signed-off-by: Aaron Ma
> ---
> drivers/platform/x86/thinkpad_acpi.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index ff7f0a4f2475..2b36d5416a3b 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6955,10 +6955,13 @@ static int __init brightness_init(struct
> ibm_init_struct *iibm)
> pr_warn("Cannot enable backlight brightness support,
> ACPI is already handling it. Refer to the acpi_backlight kernel
> parameter.\n");
> return 1;
> }
> - } else if (tp_features.bright_acpimode && brightness_enable > 1) {
> - pr_notice("Standard ACPI backlight interface not available,
> thinkpad_acpi native brightness control enabled\n");
> + } else if (!tp_features.bright_acpimode) {
> + pr_notice("ACPI backlight interface not available\n");
> + return 1;
> }
>
> + pr_notice("ACPI native brightness control enabled\n");
> +
> /*
> * Check for module parameter bogosity, note that we
> * init brightness_mode to TPACPI_BRGHT_MODE_MAX in order to be
> --
> 2.26.2
>
--
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 v5] platform/x86: thinkpad_acpi: lap or desk mode interface
On Fri, Jul 3, 2020 at 4:24 AM Mark Pearson wrote:
>
> Newer Lenovo Thinkpad platforms have support to identify whether the
> system is on-lap or not using an ACPI DYTC event from the firmware.
>
> This patch provides the ability to retrieve the current mode via sysfs
> entrypoints and will be used by userspace for thermal mode and WWAN
> functionality
>
Pushed to my review and testing queue, thanks!
> Co-developed-by: Nitin Joshi
> Signed-off-by: Nitin Joshi
> Reviewed-by: Sugumaran
> Reviewed-by: Bastien Nocera
> Signed-off-by: Mark Pearson
> ---
> Changes in v5:
> - Updated with review changes from Andy Shevchenko
> - Added ABI information to thinkpad-acpi.rst
> - improved error handling and parameter passing as recommended
> - code cleanup as recommended
> - added review tag from bnocera
> Changes in v4:
> - Correct hotkey event comment as we're handling event
> - Remove unnecessary check in dytc_lapmode_refresh
> Changes in v3:
> - Fixed inaccurate comments
> - Used BIT macro to check lapmode bit setting as recommended and update
> define name
> - Check for new_state == dytc_lapmode in dytc_lapmode_refresh
> Changes in v2:
> - cleaned up initialisation sequence to be cleaner and avoid spamming
> platforms that don't have DYTC with warning message. Tested on P52
> - Adding platform-driver-x86 mailing list for review as requested
>
> .../admin-guide/laptops/thinkpad-acpi.rst | 15 +++
> drivers/platform/x86/thinkpad_acpi.c | 111 +-
> 2 files changed, 124 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> index 822907dcc845..99066aa8d97b 100644
> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
> @@ -50,6 +50,7 @@ detailed description):
> - WAN enable and disable
> - UWB enable and disable
> - LCD Shadow (PrivacyGuard) enable and disable
> + - Lap mode sensor
>
> A compatibility table by model and feature is maintained on the web
> site, http://ibm-acpi.sf.net/. I appreciate any success or failure
> @@ -1432,6 +1433,20 @@ The first command ensures the best viewing angle and
> the latter one turns
> on the feature, restricting the viewing angles.
>
>
> +DYTC Lapmode sensor
> +--
> +
> +sysfs: dytc_lapmode
> +
> +Newer thinkpads and mobile workstations have the ability to determine if
> +the device is in deskmode or lapmode. This feature is used by user space
> +to decide if WWAN transmission can be increased to maximum power and is
> +also useful for understanding the different thermal modes available as
> +they differ between desk and lap mode.
> +
> +The property is read-only. If the platform doesn't have support the sysfs
> +class is not created.
> +
> EXPERIMENTAL: UWB
> -
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index ff7f0a4f2475..037eb77414f9 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -4022,8 +4022,8 @@ static bool hotkey_notify_6xxx(const u32 hkey,
> return true;
> case TP_HKEY_EV_THM_CSM_COMPLETED:
> pr_debug("EC reports: Thermal Control Command set completed
> (DYTC)\n");
> - /* recommended action: do nothing, we don't have
> -* Lenovo ATM information */
> + /* Thermal event - pass on to event handler */
> + tpacpi_driver_event(hkey);
> return true;
> case TP_HKEY_EV_THM_TRANSFM_CHANGED:
> pr_debug("EC reports: Thermal Transformation changed
> (GMTS)\n");
> @@ -9795,6 +9795,105 @@ static struct ibm_struct lcdshadow_driver_data = {
> .write = lcdshadow_write,
> };
>
> +/*
> + * DYTC subdriver, for the Lenovo lapmode feature
> + */
> +
> +#define DYTC_CMD_GET 2 /* To get current IC function and mode */
> +#define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */
> +
> +static bool dytc_lapmode;
> +
> +static void dytc_lapmode_notify_change(void)
> +{
> + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "dytc_lapmode");
> +}
> +
> +static int dytc_command(int command, int *output)
> +{
> + acpi_handle dytc_handle;
> +
> + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle)))
> {
> + /* Platform doesn't supp
Re: [ibm-acpi-devel] [PATCH 5.8 regression fix] platform/x86: thinkpad_acpi: Revert: Use strndup_user() in dispatch_proc_write()
On Tue, Jul 14, 2020 at 11:15 AM Hans de Goede wrote:
>
> Commit 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user()
> in dispatch_proc_write()") cleaned up dispatch_proc_write() by replacing
> the code to copy the passed in data from userspae with strndup_user().
user space
> But strndup_user() expects a 0 terminated input buffer and the buffer
> passed to dispatch_proc_write() is NOT 0 terminated.
>
> So this change leads to strndup_user() copying some extra random bytes
> from userspace till it hits a 0 byte.
>
> This commit reverts the change to use strndup_user() fixing the
> buffer being passed to the ibm_struct.write() call back containing extra
> junk at the end.
Can we simply use memdup_user()?
And thanks for catching this up!
--
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 5.8 regression fix] platform/x86: thinkpad_acpi: Revert: Use strndup_user() in dispatch_proc_write()
On Tue, Jul 14, 2020 at 11:21 AM Andy Shevchenko
wrote:
> On Tue, Jul 14, 2020 at 11:15 AM Hans de Goede wrote:
> >
> > Commit 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user()
> > in dispatch_proc_write()") cleaned up dispatch_proc_write() by replacing
> > the code to copy the passed in data from userspae with strndup_user().
>
> user space
>
> > But strndup_user() expects a 0 terminated input buffer and the buffer
> > passed to dispatch_proc_write() is NOT 0 terminated.
Second though, perhaps it's a simple wrong count parameter?
strndup_user(..., min(count, PAGE_SIZE)) or so would work?
> > So this change leads to strndup_user() copying some extra random bytes
> > from userspace till it hits a 0 byte.
> >
> > This commit reverts the change to use strndup_user() fixing the
> > buffer being passed to the ibm_struct.write() call back containing extra
> > junk at the end.
>
> Can we simply use memdup_user()?
> And thanks for catching this up!
--
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
[ibm-acpi-devel] [PATCH v1] platform/x86: thinkpad_acpi: Limit size when call strndup_user()
During conversion to use strndup_user() the commit 35d13c7a0512
("platform/x86: thinkpad_acpi: Use strndup_user() in dispatch_proc_write()")
missed the fact that buffer coming thru procfs is not immediately NULL
terminated. We have to limit size when calling strndup_user().
Fixes: 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user() in
dispatch_proc_write()")
Reported-by: Hans de Goede
Signed-off-by: Andy Shevchenko
---
drivers/platform/x86/thinkpad_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c
b/drivers/platform/x86/thinkpad_acpi.c
index f571d6254e7c..f411ad814cab 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -886,7 +886,7 @@ static ssize_t dispatch_proc_write(struct file *file,
if (!ibm || !ibm->write)
return -EINVAL;
- kernbuf = strndup_user(userbuf, PAGE_SIZE);
+ kernbuf = strndup_user(userbuf, min_t(long, count, PAGE_SIZE));
if (IS_ERR(kernbuf))
return PTR_ERR(kernbuf);
--
2.27.0
___
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 5.8 regression fix] platform/x86: thinkpad_acpi: Revert: Use strndup_user() in dispatch_proc_write()
On Tue, Jul 14, 2020 at 12:33 PM Hans de Goede wrote:
> On 7/14/20 10:27 AM, Andy Shevchenko wrote:
> > On Tue, Jul 14, 2020 at 11:21 AM Andy Shevchenko
> > wrote:
> >> On Tue, Jul 14, 2020 at 11:15 AM Hans de Goede wrote:
> >>>
> >>> Commit 35d13c7a0512 ("platform/x86: thinkpad_acpi: Use strndup_user()
> >>> in dispatch_proc_write()") cleaned up dispatch_proc_write() by replacing
> >>> the code to copy the passed in data from userspae with strndup_user().
> >>
> >> user space
>
> Ack, do you want me to send a v2, or can you fix this up after applying.
>
> >>> But strndup_user() expects a 0 terminated input buffer and the buffer
> >>> passed to dispatch_proc_write() is NOT 0 terminated.
> >
> > Second though, perhaps it's a simple wrong count parameter?
> > strndup_user(..., min(count, PAGE_SIZE)) or so would work?
>
> I honestly have not looked at a better fix and given that you've just come
> up with 2 suggestions which may or may not work, combined with
> where we are in the 5.8 cycle I believe it would be best to just
> play it safe and go with the revert for 5.8.
>
> If you then take a second attempt at cleaning this up for 5.9 and
> send it to me, I can test it for you.
I guess there is no need to revert. I have looked at the documentation
and see that all of the procfs parameters are normal strings, but you
are right they are not NULL terminated per se. So, the problematic
place is the size of data we retrieve from user space.
I have sent a patch.
--
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 v1] platform/x86: thinkpad_acpi: Limit size when call strndup_user()
On Tue, Jul 14, 2020 at 4:30 PM Hans de Goede wrote: > On 7/14/20 12:42 PM, Andy Shevchenko wrote: ... > > + kernbuf = strndup_user(userbuf, min_t(long, count, PAGE_SIZE)); > This is not going to work: You are right! > Can we please just go with the revert for now? Yes, I have reverted it. Sorry for troubles. -- 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 0/3] Add 3 new keycodes and use them for 3 new hotkeys on new Lenovo Thinkpads
On Mon, Jul 27, 2020 at 10:45 AM Hans de Goede wrote: > > Hi, > > On 7/27/20 2:50 AM, Henrique de Moraes Holschuh wrote: > > On Tue, 21 Jul 2020, Dmitry Torokhov wrote: > >> On Sun, Jul 19, 2020 at 07:56:49PM -0300, Henrique de Moraes Holschuh > >> wrote: > >>> On Fri, 17 Jul 2020, Hans de Goede wrote: > >>>> This is a simple patch-series adding support for 3 new hotkeys found > >>>> on various new Lenovo Thinkpad models. > >>> > >>> For all three patches, pending an ack for the new keycodes by the input > >>> maintainers: > >>> > >>> Acked-by: Henrique de Moraes Holschuh > >> > >> Do you want me to merge all 3 through input tree? > > > > Hans, Daren, Andy, what do you prefer? > > Taking all this upstream through Dmitry's input tree is fine with > me, but this really is up to Andy and/or Daren. Fine with me. -- 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
