Hi, Thank you for your comments. >-----Original Message----- >From: Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> >Sent: Friday, February 12, 2021 5:56 PM >To: Nitin Joshi <nitjoshi@xxxxxxxxx> >Cc: hdegoede@xxxxxxxxxx; ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx; platform- >driver-x86@xxxxxxxxxxxxxxx; Nitin Joshi1 <njoshi1@xxxxxxxxxx>; Mark RH >Pearson <markpearson@xxxxxxxxxx> >Subject: [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs >interface to reduce wlan tx power > >Hi > > >2021. február 12., péntek 6:58 keltezéssel, Nitin Joshi írta: > >> 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> >> --- >> .../admin-guide/laptops/thinkpad-acpi.rst | 18 +++ >> drivers/platform/x86/thinkpad_acpi.c | 136 ++++++++++++++++++ >> 2 files changed, 154 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..6dbbd4a14264 100644 >> --- a/drivers/platform/x86/thinkpad_acpi.c >> +++ b/drivers/platform/x86/thinkpad_acpi.c >> @@ -9983,6 +9983,138 @@ 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 >> +static int has_wlantxreduce; > >I think `bool` would be better. Ack . I will modify it in next version. > > >> +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 *has_wlantxreduce, 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 & BIT(4)) > >I believe it'd be preferable to name `BIT(4)` and `BIT(8)`. E.g.: > > #define DPRC_GET_WLAN_STATE_RES_REDUCED BIT(4) > #define DPRC_GET_WLAN_STATE_RES_FULL BIT(8) > >(or any name you like). > Ack . I will modify it in next version. > >> + *wlan_txreduce = 1; >> + else if (output & BIT(8)) >> + *wlan_txreduce = 0; >> + else >> + *wlan_txreduce = -1; > >Can you elaborate what -1 means here? Unknown/not >available/invalid/error? -1 means "error" . I had found that when "DPRC" method return METHOD_ERR i.e BIT(31) as 0 then it goes to this condition. So , therefore I had added METHOD_ERR check in dprc_command() and now , this doesnot goes here. But, I have still kept it here , just in case if any other error occurred . Can you please suggest , if I should remove it (i.e remove *wlan_txreduce = -1; )? as I still think, there is no harm keeping like this. > > >> + >> + *has_wlantxreduce = 1; > >Is it necessary for the getter to set this? Couldn't it be set in >`tpacpi_dprc_init()` once during initialization? Actually, yes we can keep it in init function also but I have not kept it because of other patch (PATCH 2/2) which I had sent . patch 1 (this patch) and patch 2 ( other patch which create sysfs of WWWAN Antenna type) both uses "DPRC" method . So , we will need a flag to create sysfs because few system will not have this "wlan tx reduce" even if it has "DPRC" method exists and vice versa . So , in this case, we need to explicitly check whether it require to create corresponding sysfs or not. > > >> + return 0; >> +} >> + >> +/* sysfs wlan entry */ >> +static ssize_t wlan_tx_strength_reduce_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) > >Please align the arguments: > > ..._show(struct device *dev, > struct device_attribute ... > ...); > Ack . I will modify it in next version. Also , I will correct it in my other patch(PATCH 2/2) also. > >> +{ >> + int err; >> + >> + err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce); >> + if (err) >> + return err; >> + > >Wouldn't it be better to return -ENODATA or something similar when >`wlan_txreduce` == -1? Ack . I think EOPNOTSUPP will be better ? reason is that "DPRC" method is available but there is error . So , its more likely that command is not supported. However, I will modify it after I get feedback about my previous comment. > > >> + 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) > >Same here. > Ack . I will modify it in next version. > >> +{ >> + int output, err; >> + unsigned long t; >> + >> + if (parse_strtoul(buf, 1, &t)) > >Maybe `kstrtobool`? Thank you for your suggestion. I can use 'kstrtobool' and will modify on my next version. > > >> + return -EINVAL; >> + >> + tpacpi_disclose_usertask(attr->attr.name, >> + "WLAN tx strength reduced %lu\n", t); >> + >> + switch (t) { >> + case 0: >> + err = dprc_command(DPRC_SET_WLAN_POWER_FULL, >&output); >> + break; >> + case 1: >> + err = dprc_command(DPRC_SET_WLAN_POWER_REDUCE, >&output); >> + break; >> + default: >> + err = -EINVAL; >> + dev_err(&tpacpi_pdev->dev, "Unknown operating mode. >Ignoring\n"); >> + break; >> + } >> + > >If I'm not mistaken, `err` is never returned, so the write() will always seem to >succeed. Yes , its correct . I will use 'kstrtobool' and will drop this : "err = -EINVAL;" Is it Ok ? > > >> + 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(&has_wlantxreduce, &wlan_txreduce); >> + /* >> + * If support isn't available (ENODEV) for both devices then quit, but >> + * don't return an error. >> + */ >> + if (wlantx_err == -ENODEV) >> + return 0; >> + /* Otherwise, if there was an error return it */ >> + if (wlantx_err && (wlantx_err != -ENODEV)) >> + return wlantx_err; >> + >> + if (has_wlantxreduce) { >> + err = sysfs_create_file(&tpacpi_pdev->dev.kobj, >> + &dev_attr_wlan_tx_strength_reduce.attr); > >I believe `device_create_file()` would be better. > Since, file will be created in /sys/ directory , hence I think its better to use sysfs_create_file. Also, by checking in this file, it looks like sysfs_create_file will be more reasonable to do . Please let me know, if its Ok to continue using sysfs_create_file or you still feel. we need to use device_create_file() also feel free to correct me, if I am wrong. > >> + 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); > >And similarly, `device_remove_file()`. Same comment as above . I feel, sysfs_remove_file is more reasonable to do. > > >> +} >> + >> +static struct ibm_struct dprc_driver_data = { >> + .name = "dprc", >> + .exit = dprc_exit, >> +}; >> + >> >/**************************************************************** >************ >> >***************************************************************** >*********** >> * >> @@ -10475,6 +10607,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) >> -- >> 2.25.1 > > >Regards, >Barnabás Pőcze Thanks & Regards, Nitin Joshi
