[PATCH v2 1/2] platorm/x86: thinkpad_acpi: sysfs interface to reduce wlan tx power

[PATCH v2 1/2] platorm/x86: thinkpad_acpi: sysfs interface to reduce wlan tx power

[Nitin Joshi]

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@lenovo.com>
Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
---
 .../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;
+
+	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;
+
+	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;
+	bool state;
+
+	ret = kstrtobool(buf, &state);
+	if (ret)
+		return ret;
+
+	if (state)
+		err = dprc_command(DPRC_SET_WLAN_POWER_REDUCE, &output);
+	else
+		err = dprc_command(DPRC_SET_WLAN_POWER_FULL, &output);
+
+	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)
-- 
2.25.1

[PATCH v2 2/2] platorm/x86: thinkpad_acpi: sysfs interface to interface to get wwan antenna type

[Nitin Joshi]

On some newer Thinkpads we need to set SAR value based on antenna type.
This patch provides a sysfs interface that userspace can use to get
antenna type and set corresponding SAR value, as is required for FCC
certification.

Reviewed-by: Mark Pearson <markpearson@lenovo.com>
Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
---
 .../admin-guide/laptops/thinkpad-acpi.rst     | 21 ++++++
 drivers/platform/x86/thinkpad_acpi.c          | 65 +++++++++++++++++--
 2 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
index 10410811b990..df6904f23dea 100644
--- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
+++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
@@ -52,6 +52,7 @@ detailed description):
 	- LCD Shadow (PrivacyGuard) enable and disable
 	- Lap mode sensor
 	- WLAN transmission power control
+	- WWAN Antenna type
 
 A compatibility table by model and feature is maintained on the web
 site, http://ibm-acpi.sf.net/. I appreciate any success or failure
@@ -1465,6 +1466,26 @@ The available commands are::
 The first command restores the wlan transmission power and the latter one
 reduces wlan transmission power.
 
+WWAN Antenna type
+-----------------
+
+sysfs: wwan_antenna_type
+
+On some newer Thinkpads we need to set SAR value based on the antenna
+type. This interface will be used by userspace to get the antenna type
+and set the corresponding SAR value, as is required for FCC certification.
+
+The available commands are::
+
+        cat /sys/devices/platform/thinkpad_acpi/wwan_antenna_type
+
+Currently 2 antenna types are supported as mentioned below:
+- type a
+- type b
+
+The property is read-only. If the platform doesn't have support the sysfs
+class is not created.
+
 EXPERIMENTAL: UWB
 -----------------
 
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index af90251d79d7..1faf260f6bbf 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9992,8 +9992,13 @@ static struct ibm_struct proxsensor_driver_data = {
 #define DPRC_SET_WLAN_POWER_FULL        0x00030100
 #define DPRC_WLAN_POWER_REDUCE_BIT      BIT(4)
 #define DPRC_WLAN_POWER_FULL_BIT        BIT(8)
+#define DPRC_GET_WWAN_ANTENNA_TYPE      0x40000
+#define DPRC_WWAN_ANTENNA_TYPE_A_BIT    BIT(4)
+#define DPRC_WWAN_ANTENNA_TYPE_B_BIT    BIT(8)
 static bool has_wlantxreduce;
 static int wlan_txreduce;
+static bool has_antennatype;
+static int wwan_antennatype;
 
 static int dprc_command(int command, int *output)
 {
@@ -10017,6 +10022,25 @@ static int dprc_command(int command, int *output)
 	return 0;
 }
 
+static int get_wwan_antenna(int *wwan_antennatype)
+{
+	int output, err;
+
+	/* Get current Antenna type */
+	err = dprc_command(DPRC_GET_WWAN_ANTENNA_TYPE, &output);
+	if (err)
+		return err;
+
+	if (output & DPRC_WWAN_ANTENNA_TYPE_A_BIT)
+		*wwan_antennatype = 1;
+	else if (output & DPRC_WWAN_ANTENNA_TYPE_B_BIT)
+		*wwan_antennatype = 2;
+	else
+		return -ENODATA;
+
+	return 0;
+}
+
 static int get_wlan_state(int *wlan_txreduce)
 {
 	int output, err;
@@ -10036,6 +10060,21 @@ static int get_wlan_state(int *wlan_txreduce)
 	return 0;
 }
 
+/* sysfs wwan antenna type entry */
+static ssize_t wwan_antenna_type_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	switch (wwan_antennatype) {
+	case 1:
+		return sysfs_emit(buf, "type a\n");
+	case 2:
+		return sysfs_emit(buf, "type b\n");
+	default:
+		return -ENODATA;
+	}
+}
+
 /* sysfs wlan entry */
 static ssize_t wlan_tx_strength_reduce_show(struct device *dev,
 						struct device_attribute *attr,
@@ -10071,18 +10110,22 @@ static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
 	return count;
 }
 static DEVICE_ATTR_RW(wlan_tx_strength_reduce);
+static DEVICE_ATTR_RO(wwan_antenna_type);
 
 static int tpacpi_dprc_init(struct ibm_init_struct *iibm)
 {
-	int wlantx_err, err;
+	int wlantx_err, wwanantenna_err, err;
 
 	wlantx_err = get_wlan_state(&wlan_txreduce);
+	wwanantenna_err = get_wwan_antenna(&wwan_antennatype);
 	/*
-	 * If support isn't available (ENODEV) for both devices then quit, but
-	 * don't return an error.
+	 * If support isn't available (ENODEV) or no data available (ENODATA) for both devices
+	 * then quit, but don't return an error.
 	 */
-	if ((wlantx_err == -ENODEV) || (wlantx_err == -ENODATA))
+	if (((wlantx_err == -ENODEV) || (wlantx_err == -ENODATA)) &&
+		((wwanantenna_err == -ENODEV) || (wwanantenna_err == -ENODATA)))
 		return 0;
+
 	/* Otherwise, if there was an error return it */
 	if (wlantx_err && (wlantx_err != -ENODEV) && (wlantx_err != -ENODATA))
 		return wlantx_err;
@@ -10095,6 +10138,18 @@ static int tpacpi_dprc_init(struct ibm_init_struct *iibm)
 		if (err)
 			return err;
 	}
+
+	/* if there was an error return it */
+	if (wwanantenna_err && (wwanantenna_err != -ENODEV) && (wwanantenna_err != -ENODATA))
+		return wwanantenna_err;
+	else if (!wwanantenna_err)
+		has_antennatype = true;
+
+	if (has_antennatype) {
+		err = sysfs_create_file(&tpacpi_pdev->dev.kobj, &dev_attr_wwan_antenna_type.attr);
+		if (err)
+			return err;
+	}
 	return 0;
 }
 
@@ -10102,6 +10157,8 @@ static void dprc_exit(void)
 {
 	if (has_wlantxreduce)
 		sysfs_remove_file(&tpacpi_pdev->dev.kobj, &dev_attr_wlan_tx_strength_reduce.attr);
+	if (has_antennatype)
+		sysfs_remove_file(&tpacpi_pdev->dev.kobj, &dev_attr_wwan_antenna_type.attr);
 }
 
 static struct ibm_struct dprc_driver_data = {
-- 
2.25.1

Re: [PATCH v2 1/2] platorm/x86: thinkpad_acpi: sysfs interface to reduce wlan tx power

[Hans de Goede]

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@lenovo.com>
> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>

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)
> 

RE: [External] Re: [PATCH v2 1/2] platorm/x86: thinkpad_acpi: sysfs interface to reduce wlan tx power

[Nitin Joshi1]

Hello Hans,

>-----Original Message-----
>From: Hans de Goede <hdegoede@redhat.com>
>Sent: Thursday, March 4, 2021 8:44 PM
>To: Nitin Joshi <nitjoshi@gmail.com>
>Cc: ibm-acpi-devel@lists.sourceforge.net; platform-driver-
>x86@vger.kernel.org; Nitin Joshi1 <njoshi1@lenovo.com>; Mark RH Pearson
><markpearson@lenovo.com>
>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@lenovo.com>
>> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
>
>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)
>>

Re: [External] Re: [PATCH v2 1/2] platorm/x86: thinkpad_acpi: sysfs interface to reduce wlan tx power

[Hans de Goede]

Hi,

On 3/5/21 1:42 AM, Nitin Joshi1 wrote:
> Hello Hans,
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Thursday, March 4, 2021 8:44 PM
>> To: Nitin Joshi <nitjoshi@gmail.com>
>> Cc: ibm-acpi-devel@lists.sourceforge.net; platform-driver-
>> x86@vger.kernel.org; Nitin Joshi1 <njoshi1@lenovo.com>; Mark RH Pearson
>> <markpearson@lenovo.com>
>> 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@lenovo.com>
>>> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
>>
>> 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)
>>>
> 

RE: [External] Re: [PATCH v2 1/2] platorm/x86: thinkpad_acpi: sysfs interface to reduce wlan tx power

[Nitin Joshi1]

Hello Hans,

>-----Original Message-----
>From: Hans de Goede <hdegoede@redhat.com>
>Sent: Friday, March 5, 2021 5:51 PM
>To: Nitin Joshi1 <njoshi1@lenovo.com>; Nitin Joshi <nitjoshi@gmail.com>
>Cc: ibm-acpi-devel@lists.sourceforge.net; platform-driver-
>x86@vger.kernel.org; Mark RH Pearson <markpearson@lenovo.com>
>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@redhat.com>
>>> Sent: Thursday, March 4, 2021 8:44 PM
>>> To: Nitin Joshi <nitjoshi@gmail.com>
>>> Cc: ibm-acpi-devel@lists.sourceforge.net; platform-driver-
>>> x86@vger.kernel.org; Nitin Joshi1 <njoshi1@lenovo.com>; Mark RH
>>> Pearson <markpearson@lenovo.com>
>>> 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@lenovo.com>
>>>> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
>>>
>>> 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)
>>>>
>>