Hello Hans, >-----Original Message----- >From: Hans de Goede <hdegoede@xxxxxxxxxx> >Sent: Friday, March 5, 2021 5:51 PM >To: Nitin Joshi1 <njoshi1@xxxxxxxxxx>; Nitin Joshi <nitjoshi@xxxxxxxxx> >Cc: ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx; platform-driver- >x86@xxxxxxxxxxxxxxx; Mark RH Pearson <markpearson@xxxxxxxxxx> >Subject: Re: [External] Re: [PATCH v2 1/2] platorm/x86: thinkpad_acpi: sysfs >interface to reduce wlan tx power > >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. > Noted with thanks . Thanks & Regards, Nitin Joshi >> >>> >>> >>>> + >>>> + 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) >>>> >>
