Hi, On 2/16/21 8:36 AM, Nitin Joshi wrote: > Some newer Thinkpads have the WLAN antenna placed close to the WWAN > antenna. In these cases FCC certification requires that when WWAN is > active we reduce WLAN transmission power. A new Dynamic Power > Reduction Control (DPRC) method is available from the BIOS on these > platforms to reduce or restore WLAN Tx power. > > This patch provides a sysfs interface that userspace can use to adjust the > WLAN power appropriately. > > Reviewed-by: Mark Pearson <markpearson@xxxxxxxxxx> > Signed-off-by: Nitin Joshi <njoshi1@xxxxxxxxxx> Thank you for your patches, I'm afraid that there are still a couple of small issues which need to be fixed before I can apply these: 1. Both patches have "platform" misspelled in the patch Subject. 2. The patches don't apply cleanly because your kbdlang patch has been merged and these are based on a thinkpad_acpi version without these. 3. I've some review remarks about this patch, see my inline comments below. Note some of these remarks apply to patch 2/2 too (I've indicated when this is the case). > --- > .../admin-guide/laptops/thinkpad-acpi.rst | 18 +++ > drivers/platform/x86/thinkpad_acpi.c | 130 ++++++++++++++++++ > 2 files changed, 148 insertions(+) > > diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst > index 5fe1ade88c17..10410811b990 100644 > --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst > +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst > @@ -51,6 +51,7 @@ detailed description): > - UWB enable and disable > - LCD Shadow (PrivacyGuard) enable and disable > - Lap mode sensor > + - WLAN transmission power control > > A compatibility table by model and feature is maintained on the web > site, http://ibm-acpi.sf.net/. I appreciate any success or failure > @@ -1447,6 +1448,23 @@ 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. > > +WLAN transmission power control > +-------------------------------- > + > +sysfs: wlan_tx_strength_reduce > + > +Some newer Thinkpads have the WLAN antenna placed close to the WWAN antenna. > +This interface will be used by userspace to reduce the WLAN Tx power strength > +when WWAN is active, as is required for FCC certification. > + > +The available commands are:: > + > + echo '0' > /sys/devices/platform/thinkpad_acpi/wlan_tx_strength_reduce > + echo '1' > /sys/devices/platform/thinkpad_acpi/wlan_tx_strength_reduce > + > +The first command restores the wlan transmission power and the latter one > +reduces wlan transmission power. > + > EXPERIMENTAL: UWB > ----------------- > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index f3e8eca8d86d..af90251d79d7 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -9983,6 +9983,132 @@ static struct ibm_struct proxsensor_driver_data = { > .exit = proxsensor_exit, > }; > > +/************************************************************************* > + * DPRC(Dynamic Power Reduction Control) subdriver, for the Lenovo WWAN > + * and WLAN feature. > + */ > +#define DPRC_GET_WLAN_STATE 0x20000 > +#define DPRC_SET_WLAN_POWER_REDUCE 0x00030010 > +#define DPRC_SET_WLAN_POWER_FULL 0x00030100 > +#define DPRC_WLAN_POWER_REDUCE_BIT BIT(4) > +#define DPRC_WLAN_POWER_FULL_BIT BIT(8) > +static bool has_wlantxreduce; > +static int wlan_txreduce; > + > +static int dprc_command(int command, int *output) > +{ > + acpi_handle dprc_handle; > + > + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DPRC", &dprc_handle))) { > + /* Platform doesn't support DPRC */ > + return -ENODEV; > + } > + > + if (!acpi_evalf(dprc_handle, output, NULL, "dd", command)) > + return -EIO; > + > + /* > + * METHOD_ERR gets returned on devices where few commands are not supported > + * for example WLAN power reduce command is not supported on some devices. > + */ > + if (*output & METHOD_ERR) > + return -ENODEV; > + > + return 0; > +} > + > +static int get_wlan_state(int *wlan_txreduce) > +{ > + int output, err; > + > + /* Get current WLAN Power Transmission state */ > + err = dprc_command(DPRC_GET_WLAN_STATE, &output); > + if (err) > + return err; > + > + if (output & DPRC_WLAN_POWER_REDUCE_BIT) > + *wlan_txreduce = 1; > + else if (output & DPRC_WLAN_POWER_FULL_BIT) > + *wlan_txreduce = 0; > + else > + return -ENODATA; If you return -ENODEV here, then the error handling in tpacpi_dprc_init() becomes a lot simpler / easier to read. Note this remark applies to patch 2/2 too. > + > + return 0; > +} > + > +/* sysfs wlan entry */ > +static ssize_t wlan_tx_strength_reduce_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int err; > + > + err = get_wlan_state(&wlan_txreduce); > + if (err) > + return err; Is it necessary to re-query the setting here? Can't you just query it from tpacpi_dprc_init() once and store the updated value in wlan_tx_strength_reduce_store() on success? > + > + return sysfs_emit(buf, "%d\n", wlan_txreduce); > +} > + > +static ssize_t wlan_tx_strength_reduce_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int output, err, ret; Please use just err here, there is no need to have both err and ret. > + bool state; > + > + ret = kstrtobool(buf, &state); > + if (ret) > + return ret; So change to using err here. > + > + if (state) > + err = dprc_command(DPRC_SET_WLAN_POWER_REDUCE, &output); > + else > + err = dprc_command(DPRC_SET_WLAN_POWER_FULL, &output); You are not doing anything with err here, shouldn't this have a: if (err) return err; here ? Regards, Hans > + > + sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "wlan_tx_strength_reduce"); > + > + return count; > +} > +static DEVICE_ATTR_RW(wlan_tx_strength_reduce); > + > +static int tpacpi_dprc_init(struct ibm_init_struct *iibm) > +{ > + int wlantx_err, err; > + > + wlantx_err = get_wlan_state(&wlan_txreduce); > + /* > + * If support isn't available (ENODEV) for both devices then quit, but > + * don't return an error. > + */ > + if ((wlantx_err == -ENODEV) || (wlantx_err == -ENODATA)) > + return 0; > + /* Otherwise, if there was an error return it */ > + if (wlantx_err && (wlantx_err != -ENODEV) && (wlantx_err != -ENODATA)) > + return wlantx_err; > + else if (!wlantx_err) > + has_wlantxreduce = true; > + > + if (has_wlantxreduce) { > + err = sysfs_create_file(&tpacpi_pdev->dev.kobj, > + &dev_attr_wlan_tx_strength_reduce.attr); > + if (err) > + return err; > + } > + return 0; > +} > + > +static void dprc_exit(void) > +{ > + if (has_wlantxreduce) > + sysfs_remove_file(&tpacpi_pdev->dev.kobj, &dev_attr_wlan_tx_strength_reduce.attr); > +} > + > +static struct ibm_struct dprc_driver_data = { > + .name = "dprc", > + .exit = dprc_exit, > +}; > + > /**************************************************************************** > **************************************************************************** > * > @@ -10475,6 +10601,10 @@ static struct ibm_init_struct ibms_init[] __initdata = { > .init = tpacpi_proxsensor_init, > .data = &proxsensor_driver_data, > }, > + { > + .init = tpacpi_dprc_init, > + .data = &dprc_driver_data, > + }, > }; > > static int __init set_ibm_param(const char *val, const struct kernel_param *kp) > _______________________________________________ ibm-acpi-devel mailing list ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
