Hi, On 3/5/21 1:42 AM, Nitin Joshi1 wrote: > Hello Hans, > >> -----Original Message----- >> From: Hans de Goede <hdegoede@xxxxxxxxxx> >> Sent: Thursday, March 4, 2021 8:44 PM >> To: Nitin Joshi <nitjoshi@xxxxxxxxx> >> Cc: ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx; platform-driver- >> x86@xxxxxxxxxxxxxxx; Nitin Joshi1 <njoshi1@xxxxxxxxxx>; Mark RH Pearson >> <markpearson@xxxxxxxxxx> >> Subject: [External] Re: [PATCH v2 1/2] platorm/x86: thinkpad_acpi: sysfs >> interface to reduce wlan tx power >> >> 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: > > Thank you for your comments and apologize for any inconvenience caused. > >> >> 1. Both patches have "platform" misspelled in the patch Subject. > Ack. I will correct it in next version. > >> 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. > Ack. I will take latest file and correct it in next version. > >> 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). > Ack with thanks > >> >>> --- >>> .../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. > Ack. I will modify it on next version. > >> >>> + >>> + 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? > We will have to call this sys for WLAN power reduce or full from userspace based on > some conditions. After setting we need to make sure if wlan is set correctly in BIOS . > I can understand that if setting is success, then we can store updated value on success. > However , since, we have command to get wlan tx state in "DPRC" method , so I just want to make sure > If wlan tx state is set correctly in BIOS as I don’t have any other way to confirm it. > So, I think it's better to keep this setting here. Keeping the querying of the setting here is fine. > >> >> >>> + >>> + 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. > Ack. I will modify it in next version. > >> >>> + bool state; >>> + >>> + ret = kstrtobool(buf, &state); >>> + if (ret) >>> + return ret; >> >> So change to using err here. >> > Ack. I will modify it in next version. > >>> + >>> + 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 ? > Ack. I will recheck it and modify it in next version. > > I will incorporate all comments and send updated patch by next week or asap. Thank you. 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
