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. > > >> + >> + 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. Thanks & Regards, Nitin Joshi > >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) >>
