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

[PATCH 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          | 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;
+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))
+		*wlan_txreduce = 1;
+	else if (output & BIT(8))
+		*wlan_txreduce = 0;
+	else
+		*wlan_txreduce = -1;
+
+	*has_wlantxreduce = 1;
+	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(&has_wlantxreduce, &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;
+	unsigned long t;
+
+	if (parse_strtoul(buf, 1, &t))
+		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;
+	}
+
+	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);
+		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 +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

[PATCH 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          | 55 ++++++++++++++++++-
 2 files changed, 74 insertions(+), 2 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 6dbbd4a14264..db016a2a1837 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9990,8 +9990,11 @@ static struct ibm_struct proxsensor_driver_data = {
 #define DPRC_GET_WLAN_STATE             0x20000
 #define DPRC_SET_WLAN_POWER_REDUCE      0x00030010
 #define DPRC_SET_WLAN_POWER_FULL        0x00030100
+#define DPRC_GET_WWAN_ANTENNA_TYPE      0x40000
 static int has_wlantxreduce;
 static int wlan_txreduce;
+static int has_antennatype;
+static int wwan_antennatype;
 
 static int dprc_command(int command, int *output)
 {
@@ -10035,6 +10038,26 @@ static int get_wlan_state(int *has_wlantxreduce, int *wlan_txreduce)
 	return 0;
 }
 
+static int get_wwan_antenna(int *has_antennatype, 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 & BIT(4))
+		*wwan_antennatype = 1;
+	else if (output & BIT(8))
+		*wwan_antennatype = 2;
+	else
+		*wwan_antennatype = -1;
+
+	*has_antennatype = 1;
+	return 0;
+}
+
 /* sysfs wlan entry */
 static ssize_t wlan_tx_strength_reduce_show(struct device *dev,
 				struct device_attribute *attr,
@@ -10049,6 +10072,21 @@ static ssize_t wlan_tx_strength_reduce_show(struct device *dev,
 	return sysfs_emit(buf, "%d\n", wlan_txreduce);
 }
 
+/* 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 sysfs_emit(buf, "unknown type\n");
+	}
+}
+
 static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
 				struct device_attribute *attr,
 				const char *buf, size_t count)
@@ -10076,24 +10114,29 @@ static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
 	}
 
 	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "wlan_tx_strength_reduce");
+
 	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(&has_wlantxreduce, &wlan_txreduce);
+	wwanantenna_err = get_wwan_antenna(&has_antennatype, &wwan_antennatype);
 	/*
 	 * If support isn't available (ENODEV) for both devices then quit, but
 	 * don't return an error.
 	 */
-	if (wlantx_err == -ENODEV)
+	if ((wlantx_err == -ENODEV) && (wwanantenna_err == -ENODEV))
 		return 0;
 	/* Otherwise, if there was an error return it */
 	if (wlantx_err && (wlantx_err != -ENODEV))
 		return wlantx_err;
+	if (wwanantenna_err && (wwanantenna_err != -ENODEV))
+		return wwanantenna_err;
 
 	if (has_wlantxreduce) {
 		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
@@ -10101,6 +10144,12 @@ static int tpacpi_dprc_init(struct ibm_init_struct *iibm)
 		if (err)
 			return err;
 	}
+
+	if (has_antennatype) {
+		err = sysfs_create_file(&tpacpi_pdev->dev.kobj, &dev_attr_wwan_antenna_type.attr);
+		if (err)
+			return err;
+	}
 	return 0;
 }
 
@@ -10108,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 2/2] platorm/x86: thinkpad_acpi: sysfs interface to interface to get wwan antenna type

[Barnabás Pőcze]

Hi


2021. február 12., péntek 6:58 keltezéssel, Nitin Joshi írta:

> [...]
> +/* 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 sysfs_emit(buf, "unknown type\n");

I feel like returning -ENODATA would be more appropriate here. Or maybe you could choose
not to create the attribute if the antenna type is unknown. And I'm not sure if
the "type" prefix is necessary. I believe the name of the attribute 'wwan_antenna_type'
already implies that the content will describe a type. Furthermore, I think you could
omit the `has_antennatype` variable altogether, storing only `wwan_antennatype` is enough.


> +	}
> +}
> +
>  static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
>  				struct device_attribute *attr,
>  				const char *buf, size_t count)
> @@ -10076,24 +10114,29 @@ static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
>  	}
>
>  	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL, "wlan_tx_strength_reduce");
> +

If you want the empty line here, I think you should place it in the previous patch.


>  	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(&has_wlantxreduce, &wlan_txreduce);
> +	wwanantenna_err = get_wwan_antenna(&has_antennatype, &wwan_antennatype);
>  	/*
>  	 * If support isn't available (ENODEV) for both devices then quit, but
>  	 * don't return an error.
>  	 */
> -	if (wlantx_err == -ENODEV)
> +	if ((wlantx_err == -ENODEV) && (wwanantenna_err == -ENODEV))
>  		return 0;
>  	/* Otherwise, if there was an error return it */
>  	if (wlantx_err && (wlantx_err != -ENODEV))
>  		return wlantx_err;
> +	if (wwanantenna_err && (wwanantenna_err != -ENODEV))
> +		return wwanantenna_err;
>
>  	if (has_wlantxreduce) {
>  		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
> @@ -10101,6 +10144,12 @@ static int tpacpi_dprc_init(struct ibm_init_struct *iibm)
>  		if (err)
>  			return err;
>  	}
> +
> +	if (has_antennatype) {
> +		err = sysfs_create_file(&tpacpi_pdev->dev.kobj, &dev_attr_wwan_antenna_type.attr);
> +		if (err)
> +			return err;
> +	}
>  	return 0;
>  }
> [...]


Regards,
Barnabás Pőcze

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

[Nitin Joshi1]

Hi

>-----Original Message-----
>From: Barnabás Pőcze <pobrn@protonmail.com>
>Sent: Sunday, February 14, 2021 5:43 AM
>To: Nitin Joshi <nitjoshi@gmail.com>
>Cc: hdegoede@redhat.com; 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 2/2] platorm/x86: thinkpad_acpi: sysfs
>interface to interface to get wwan antenna type
>
>Hi
>
>
>2021. február 12., péntek 6:58 keltezéssel, Nitin Joshi írta:
>
>> [...]
>> +/* 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 sysfs_emit(buf, "unknown type\n");
>
>I feel like returning -ENODATA would be more appropriate here. Or maybe
>you could choose not to create the attribute if the antenna type is unknown.
>And I'm not sure if the "type" prefix is necessary. I believe the name of the
>attribute 'wwan_antenna_type'
Ack . I will check it. 
Regarding prefix, it's not so necessary but let me keep "type" prefix.
   
>already implies that the content will describe a type. Furthermore, I think you
>could omit the `has_antennatype` variable altogether, storing only
>`wwan_antennatype` is enough.
Ack . I will check it
>
>
>> +	}
>> +}
>> +
>>  static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
>>  				struct device_attribute *attr,
>>  				const char *buf, size_t count)
>> @@ -10076,24 +10114,29 @@ static ssize_t
>wlan_tx_strength_reduce_store(struct device *dev,
>>  	}
>>
>>  	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>> "wlan_tx_strength_reduce");
>> +
>
>If you want the empty line here, I think you should place it in the previous
>patch.
Ack . I will remove it.
>
>
>>  	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(&has_wlantxreduce, &wlan_txreduce);
>> +	wwanantenna_err = get_wwan_antenna(&has_antennatype,
>&wwan_antennatype);
>>  	/*
>>  	 * If support isn't available (ENODEV) for both devices then quit, but
>>  	 * don't return an error.
>>  	 */
>> -	if (wlantx_err == -ENODEV)
>> +	if ((wlantx_err == -ENODEV) && (wwanantenna_err == -ENODEV))
>>  		return 0;
>>  	/* Otherwise, if there was an error return it */
>>  	if (wlantx_err && (wlantx_err != -ENODEV))
>>  		return wlantx_err;
>> +	if (wwanantenna_err && (wwanantenna_err != -ENODEV))
>> +		return wwanantenna_err;
>>
>>  	if (has_wlantxreduce) {
>>  		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>> @@ -10101,6 +10144,12 @@ static int tpacpi_dprc_init(struct
>ibm_init_struct *iibm)
>>  		if (err)
>>  			return err;
>>  	}
>> +
>> +	if (has_antennatype) {
>> +		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>&dev_attr_wwan_antenna_type.attr);
>> +		if (err)
>> +			return err;
>> +	}
>>  	return 0;
>>  }
>> [...]
>
>
>Regards,
>Barnabás Pőcze

Thanks & regards,
Nitin Joshi 

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

[Barnabás Pőcze]

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


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


> +		*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?


> +
> +	*has_wlantxreduce = 1;

Is it necessary for the getter to set this? Couldn't it be set in
`tpacpi_dprc_init()` once during initialization?


> +	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 ...
           ...);


> +{
> +	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?


> +	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.


> +{
> +	int output, err;
> +	unsigned long t;
> +
> +	if (parse_strtoul(buf, 1, &t))

Maybe `kstrtobool`?


> +		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.


> +	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.


> +		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()`.


> +}
> +
> +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

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

[Nitin Joshi1]

Hi,

Thank you for your comments.

>-----Original Message-----
>From: Barnabás Pőcze <pobrn@protonmail.com>
>Sent: Friday, February 12, 2021 5:56 PM
>To: Nitin Joshi <nitjoshi@gmail.com>
>Cc: hdegoede@redhat.com; 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 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@lenovo.com>
>> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com>
>> ---
>>  .../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 

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

[Barnabás Pőcze]

Hi


2021. február 12., péntek 11:40 keltezéssel, Nitin Joshi1 <njoshi1@lenovo.com> írta:

> [...]
> >>
> >+/***************************************************************
> >*****
> >> +*****
> >> + * 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.
>

If `dprc_command()` handles all error cases (i.e. it is not possible that `dprc_command()`
returns 0, but there was an error) - which seems to be the case - then I think in
that branch you should return -ENODATA and not touch `wlan_txreduce`. Because if
that branch runs, then there was no error, but the state cannot be determined,
so I think -ENODATA is an appropriate error code.


> >
> >
> >> +
> >> +	*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.
>

I was thinking of something like the following:

  static int tpacpi_dprc_init(...) {
    ...
    int err = get_wlan_state(&reduced);
    if (err && err != -ENODEV)
      return err;
    else if (!err)
      has_wlantxreduce = true;

    err = get_wwan_antenna(&antenna);
    if (err && err != -ENODEV)
      return err;
    else if (!err)
      has_antennatype = true; /* as I've commented on the second patch, this
                                 variable is probably not needed */
    ...

If I'm not mistaken this is enough not to need the second argument in either
`get_wlan_state()` or `get_wwan_antenna()`. Note, that if both functions may
return -ENODATA, you might also want to add `err != -ENODATA` to the
conditions.


> >
> >
> >> +	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.
>

I think the place to determine whether the operation is supported or not
is in `tpacpi_dprc_init()` and the attribute should not be created
if it's not supported.


> >
> >
> >> +	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 ?
>

If you use `kstrtobool()`, you can even drop the entire switch statement.


> >
> >
> >> +	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.
>

There's not much difference, so sysfs_{create,remove}_file() would work just as fine
here. The reason why I believe `device_{create,remove}_file()` is possibly preferable
is that you don't need to reference the kobj and the attribute when calling it,
and you're adding an attribute to a *device* (not any kobj), so device_*() functions
are a better fit syntactically in my opinion. But in the end, both will achieve
the same effect, so you are free to choose whichever you like.


> >
> >> +		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.
> [...]


Regards,
Barnabás Pőcze

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

[Nitin Joshi1]

Hi,

>-----Original Message-----
>From: Barnabás Pőcze <pobrn@protonmail.com>
>Sent: Sunday, February 14, 2021 5:48 AM
>To: Nitin Joshi1 <njoshi1@lenovo.com>
>Cc: Nitin Joshi <nitjoshi@gmail.com>; hdegoede@redhat.com; ibm-acpi-
>devel@lists.sourceforge.net; platform-driver-x86@vger.kernel.org; Mark RH
>Pearson <markpearson@lenovo.com>
>Subject: RE: [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs
>interface to reduce wlan tx power
>
>Hi
>
>
>2021. február 12., péntek 11:40 keltezéssel, Nitin Joshi1
><njoshi1@lenovo.com> írta:
>
>> [...]
>> >>
>>
>>+/**************************************************************
>*
>> >*****
>> >> +*****
>> >> + * 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.
>>
>
>If `dprc_command()` handles all error cases (i.e. it is not possible that
>`dprc_command()` returns 0, but there was an error) - which seems to be the
>case - then I think in that branch you should return -ENODATA and not touch
>`wlan_txreduce`. Because if that branch runs, then there was no error, but the
>state cannot be determined, so I think -ENODATA is an appropriate error code.
Ack . Noted, I will check it
>
>
>> >
>> >
>> >> +
>> >> +	*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.
>>
>
>I was thinking of something like the following:
>
>  static int tpacpi_dprc_init(...) {
>    ...
>    int err = get_wlan_state(&reduced);
>    if (err && err != -ENODEV)
>      return err;
>    else if (!err)
>      has_wlantxreduce = true;
>
>    err = get_wwan_antenna(&antenna);
>    if (err && err != -ENODEV)
>      return err;
>    else if (!err)
>      has_antennatype = true; /* as I've commented on the second patch, this
>                                 variable is probably not needed */
>    ...
>
Ack . I will check it

>If I'm not mistaken this is enough not to need the second argument in either
>`get_wlan_state()` or `get_wwan_antenna()`. Note, that if both functions may
>return -ENODATA, you might also want to add `err != -ENODATA` to the
>conditions.
>
Ack . yes.

>
>> >
>> >
>> >> +	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.
>>
>
>I think the place to determine whether the operation is supported or not is in
>`tpacpi_dprc_init()` and the attribute should not be created if it's not
>supported.
Ack . I will check it and send next patch version soon.
>
>
>> >
>> >
>> >> +	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 ?
>>
>
>If you use `kstrtobool()`, you can even drop the entire switch statement.
Ack . yes , I think so too.
>
>
>> >
>> >
>> >> +	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.
>>
>
>There's not much difference, so sysfs_{create,remove}_file() would work just
>as fine here. The reason why I believe `device_{create,remove}_file()` is
>possibly preferable is that you don't need to reference the kobj and the
>attribute when calling it, and you're adding an attribute to a *device* (not any
>kobj), so device_*() functions are a better fit syntactically in my opinion. But in
>the end, both will achieve the same effect, so you are free to choose
>whichever you like.
Ack . Noted.
I would like to keep sysfs_{ create,remove}_file() now.

I will recheck code, incorporate comments and send next patch version soon.

>
>> >
>> >> +		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.
>> [...]
>
>
>Regards,
>Barnabás Pőcze

Thanks & Regards,
Nitin Joshi

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

[Hans de Goede]

Hi Nitin,

On 2/12/21 6:58 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>

This patch as well as patch 2/2 generally look ok to me.

The one thing which stands out is the:

		*wlan_txreduce = -1;

Resp:

		*wwan_antennatype = -1;

	default:
		return sysfs_emit(buf, "unknown type\n");

Bits, which Barnabás already pointed out.

If you can prepare a v2 of this patch-set addressing all the
review remarks which you have received so far then that
would be great.

Regards,

Hans




> ---
>  .../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;
> +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))
> +		*wlan_txreduce = 1;
> +	else if (output & BIT(8))
> +		*wlan_txreduce = 0;
> +	else
> +		*wlan_txreduce = -1;
> +
> +	*has_wlantxreduce = 1;
> +	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(&has_wlantxreduce, &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;
> +	unsigned long t;
> +
> +	if (parse_strtoul(buf, 1, &t))
> +		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;
> +	}
> +
> +	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);
> +		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 +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)
> 

RE: [External] Re: [PATCH 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: Monday, February 15, 2021 11:48 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 1/2] platorm/x86: thinkpad_acpi: sysfs
>interface to reduce wlan tx power
>
>Hi Nitin,
>
>On 2/12/21 6:58 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>
>
>This patch as well as patch 2/2 generally look ok to me.
>
>The one thing which stands out is the:
>
>		*wlan_txreduce = -1;
>
>Resp:
>
>		*wwan_antennatype = -1;
>
>	default:
>		return sysfs_emit(buf, "unknown type\n");
>
>Bits, which Barnabás already pointed out.
>
>If you can prepare a v2 of this patch-set addressing all the review remarks
>which you have received so far then that would be great.
>
Thank you for your comment.
I have already prepared patch and will send patch soon.

>Regards,
>
>Hans
Thanks & Regards,
Nitin Joshi 

>
>
>
>
>> ---
>>  .../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;
>> +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))
>> +		*wlan_txreduce = 1;
>> +	else if (output & BIT(8))
>> +		*wlan_txreduce = 0;
>> +	else
>> +		*wlan_txreduce = -1;
>> +
>> +	*has_wlantxreduce = 1;
>> +	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(&has_wlantxreduce, &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;
>> +	unsigned long t;
>> +
>> +	if (parse_strtoul(buf, 1, &t))
>> +		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;
>> +	}
>> +
>> +	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);
>> +		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 +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)
>>

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

[Kevin Locke]

On Fri, 2021-02-12 at 14:58 +0900, 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.

Question from a user: How does wlan_tx_strength_reduce relate to or
interact with ioctl(SIOCSIWTXPOW) (i.e. iwconfig txpower) and
NL80211_ATTR_WIPHY_TX_POWER_LEVEL (i.e. iw dev set txpower)?  If I
request 30 dBm then enable wlan_tx_strength_reduce, what happens?  Same
in the opposite order?  Will ioctl(SIOCGIWTXPOW) show the reduced
txpower?

Thanks,
Kevin

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

[Nitin Joshi1]

Hello,
>-----Original Message-----
>From: Kevin Locke <kevin@kevinlocke.name>
>Sent: Saturday, March 6, 2021 1:43 AM
>To: Nitin Joshi <nitjoshi@gmail.com>
>Cc: hdegoede@redhat.com; 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 1/2] platorm/x86: thinkpad_acpi: sysfs
>interface to reduce wlan tx power
>
>On Fri, 2021-02-12 at 14:58 +0900, 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.
>
>Question from a user: How does wlan_tx_strength_reduce relate to or
>interact with ioctl(SIOCSIWTXPOW) (i.e. iwconfig txpower) and
>NL80211_ATTR_WIPHY_TX_POWER_LEVEL (i .e. iw dev set txpower)?  If I
>request 30 dBm then enable wlan_tx_strength_reduce, what happens?  Same
>in the opposite order?  Will ioctl(SIOCGIWTXPOW) show the reduced
>txpower?

Below comment is just to update current status in this e-mail chain:
As informed in separate e-mail, after testing by changing txpower using "iwconfig tx power  xx" I cannot see any change in wlan_tx_strength_reduce
It seems wlan_tx_strength_reduce is not directly related to iwconfig or iw. So, I am currently trying to find more information regarding this.
Although, this patch is working fine but need some more information for better understanding.
Hence , dropping this patch series and splitting it i.e submitting only "WWAN Antenna patch " now.
 
Thank you !! 
 
Thanks & Regards,
Nitin Joshi  

>
>Thanks,
>Kevin