platform-driver-x86.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2 0/1] [x86] BIOS SAR Driver for M.2 Intel Modems
@ 2021-05-10  7:40 Shravan S
  2021-05-10  7:40 ` [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem Shravan S
  0 siblings, 1 reply; 10+ messages in thread
From: Shravan S @ 2021-05-10  7:40 UTC (permalink / raw)
  To: hdegoede, mgross, platform-driver-x86; +Cc: sudhakar.an

SAR (Specific Absorption Rate) driver is a host driver implemented for Linux
or chrome platform to limit the exposure of human body to RF frequency by informing
the Intel M.2 modem to regulate the RF power based on SAR data obtained from the sensors
captured in the BIOS. ACPI interface exposes this data from the BIOS to SAR driver. The
front end application in userspace ( eg: Modem Manager) will interact with SAR driver 
to obtain information like the device mode (Example: tablets, laptops, etx), Antenna index,
baseband index, SAR table index and use available communication like MBIM interface to enable
data communication to modem for RF power regulation.

The BIOS gets notified about device mode changes through Sensor Driver. This information is 
given to a (newly created) WWAN ACPI function driver when there is a device mode change. 
The driver then uses a _DSM method to retrieve the required information from BIOS. 
This information is then forwarded to the User-space using the sysfs_notify interface.
A lookup table is maintained inside the BIOS which suggests the SAR Table index and Antenna 
Tuner Table Index values for individual device modes.

The SAR parameters to be used on the Modem differs for each Regulatory Mode like FCC, CE and ISED.
Hence, the SAR parameters are stored separately in the SMBIOS table in the OEM BIOS, 
for each of the Regulatory mode. Regulatory modes will be different based on the region and network
available in that region.

Hence the entire SAR functionality handling is divided into 2 parts:

•	A ACPI function driver implemented that uses a dedicated ACPI node for WWAN device. 
    sends notifications whenever there is change in Device Mode. (each OEM has different mechanism
    of updating this DEVICE Mode info). This is notified to User-space applications using 
    the sysfs_notify interface.
•	WWAN Device Service listens for sysfs_notify messages and reads the sysfs data and routes them 
	to Modem using MBIM.

Shravan S (1):
  [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem

 drivers/platform/x86/Kconfig                |  12 +
 drivers/platform/x86/Makefile               |   1 +
 drivers/platform/x86/intel-sar.c            | 550 ++++++++++++++++++++
 include/linux/platform_data/x86/intel-sar.h |  91 ++++
 4 files changed, 654 insertions(+)
 create mode 100644 drivers/platform/x86/intel-sar.c
 create mode 100644 include/linux/platform_data/x86/intel-sar.h


base-commit: 0c18f29aae7ce3dadd26d8ee3505d07cc982df75
-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem
  2021-05-10  7:40 [PATCH V2 0/1] [x86] BIOS SAR Driver for M.2 Intel Modems Shravan S
@ 2021-05-10  7:40 ` Shravan S
  2021-06-24 10:24   ` Andy Shevchenko
                     ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Shravan S @ 2021-05-10  7:40 UTC (permalink / raw)
  To: hdegoede, mgross, platform-driver-x86; +Cc: sudhakar.an

V2 :
* Changes to the remove netlink and introduce sysfs to communicate to Userspace via notify
* Handled review comments by changing ioctl calls to sysfs
* "sar" name change for platform_device_id to "intc1092"
* sar_init and sar_exit is modified as per review to minimal initialization
* Inclusion of debug only enabling of device mode change parameters


---
 drivers/platform/x86/Kconfig                |  12 +
 drivers/platform/x86/Makefile               |   1 +
 drivers/platform/x86/intel-sar.c            | 550 ++++++++++++++++++++
 include/linux/platform_data/x86/intel-sar.h |  91 ++++
 4 files changed, 654 insertions(+)
 create mode 100644 drivers/platform/x86/intel-sar.c
 create mode 100644 include/linux/platform_data/x86/intel-sar.h

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 60592fb88e7a..6dfa89310677 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1313,6 +1313,18 @@ config INTEL_TELEMETRY
 	  directly via debugfs files. Various tools may use
 	  this interface for SoC state monitoring.
 
+config INTEL_SAR
+	tristate "Intel Specific Absorption Rate Driver"
+	depends on ACPI
+	help
+	  This driver limit the exposure of human body to RF frequency by informing
+	  the Intel M.2 modem to regulate the RF power based on SAR data obtained
+	  from the sensorscaptured in the BIOS. ACPI interface exposes this data
+	  from the BIOS to SAR driver. The front end application in userspace
+	  will interact with SAR driver to obtain information like the device mode,
+	  Antenna index,baseband index, SAR table index and use available communication
+	  like MBIM interface to enable data communication to modem for RF power regulation.
+
 endif # X86_PLATFORM_DEVICES
 
 config PMC_ATOM
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index dcc8cdb95b4d..548ff663c4af 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -123,6 +123,7 @@ obj-$(CONFIG_INTEL_SMARTCONNECT)		+= intel-smartconnect.o
 obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE)	+= intel_speed_select_if/
 obj-$(CONFIG_INTEL_TURBO_MAX_3)			+= intel_turbo_max_3.o
 obj-$(CONFIG_INTEL_UNCORE_FREQ_CONTROL)		+= intel-uncore-frequency.o
+obj-$(CONFIG_INTEL_SAR)				+= intel-sar.o
 
 # Intel PMIC / PMC / P-Unit devices
 obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU)	+= intel_bxtwc_tmu.o
diff --git a/drivers/platform/x86/intel-sar.c b/drivers/platform/x86/intel-sar.c
new file mode 100644
index 000000000000..fa16ae59851b
--- /dev/null
+++ b/drivers/platform/x86/intel-sar.c
@@ -0,0 +1,550 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel Corporation - ACPI for Specific Absorption Rate
+ * Copyright (c) 2021, Intel Corporation.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/acpi.h>
+#include <uapi/linux/errno.h>
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+#include <linux/platform_data/x86/intel-sar.h>
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Platform device driver for INTEL MODEM BIOS SAR");
+MODULE_AUTHOR("Shravan S <s.shravan@intel.com>");
+
+static struct WWAN_SAR_CONTEXT *context;
+static int sar_add(struct platform_device *device);
+static int sar_remove(struct platform_device *device);
+static void sar_shutdown(struct platform_device *device);
+static void sar_notify(acpi_handle handle, u32 event, void *data);
+
+/**
+ * sar_send_notify - Send SAR data notification to userspace
+ * Send the sar data notification to the userspace and later sent to modem
+ * by the userspace application. The data sent is collected from the BIOS.
+ * This data is used by userspace for further handling of the modem output.
+ */
+static void sar_send_notify(void)
+{
+		if (!context) {
+			pr_alert("%s: context is null\n", __func__);
+			return;
+		}
+		pr_debug("%s: %d, %d, %d, %d\n", __func__, context->sar_data.device_mode,
+			 context->sar_data.bandtable_index, context->sar_data.antennatable_index,
+			 context->sar_data.sartable_index);
+		sysfs_notify(context->sar_kobject, NULL, "intc_data");
+}
+
+/**
+ * clear_sar_dev_mode - Clear Device Mode by freeing the allocated data
+ * If the Device Mode Info is present and allocated, clear it. This is for
+ * dynamic allocated memory of device_mode_info
+ */
+static void clear_sar_dev_mode(void)
+{
+		int reg = 0;
+
+		for (reg = 0; reg < MAX_REGULATORY; reg++) {
+			kfree(context->config_data[reg].device_mode_info);
+			context->config_data[reg].device_mode_info = NULL;
+		}
+}
+
+/**
+ * get_int_value - Retrieve Integer values from ACPI Object
+ * Value of the integer from the object of ACPI is obtained.
+ * @obj: acpi_object pointer which gets the integer value
+ * @out: output pointer for integer
+ * returns 0 on success
+ */
+static int get_int_value(union acpi_object *obj, int *out)
+{
+		if (obj && obj->type == ACPI_TYPE_INTEGER) {
+			*out = (int)obj->integer.value;
+			return 0;
+		} else {
+			return -1;
+		}
+}
+
+/**
+ * update_sar_data - sar data is updated based on reg_value
+ * sar_data is updated based on regulatory value
+ */
+static void update_sar_data(void)
+{
+		pr_debug("%s: Update SAR data\n", __func__);
+		if (context->config_data[context->reg_value].device_mode_info &&
+		    context->sar_data.device_mode <=
+		    context->config_data[context->reg_value].total_dev_mode) {
+			context->sar_data.antennatable_index =
+			context->config_data[context->reg_value]
+			.device_mode_info[context->sar_data.device_mode].antennatable_index;
+			context->sar_data.bandtable_index =
+			context->config_data[context->reg_value]
+			.device_mode_info[context->sar_data.device_mode].bandtable_index;
+			context->sar_data.sartable_index =
+			context->config_data[context->reg_value]
+			.device_mode_info[context->sar_data.device_mode].sartable_index;
+			pr_debug("bandtable_index: %d\n", context->sar_data.bandtable_index);
+			pr_debug("antennatable_index : %d\n", context->sar_data.antennatable_index);
+			pr_debug("sartable_index: %d\n", context->sar_data.sartable_index);
+		} else {
+			pr_err("%s: sar data not assigned! Dev mode: %d, total_dev_mode: %d\n",
+			       __func__, context->sar_data.device_mode,
+			       context->config_data[context->reg_value].total_dev_mode);
+		}
+}
+
+/**
+ * parse_guid - parse guid based on DSM UUID
+ * returns if success or error
+ */
+static acpi_status parse_guid(void)
+{
+		if (guid_parse(SAR_DSM_UUID, &context->guid)) {
+			pr_err("%s: UUID error\n", __func__);
+			return AE_ERROR;
+		}
+		context->parse = true;
+		return AE_OK;
+}
+
+/**
+ * parse_package - parse package for SAR
+ * @item : acpi_object ptr
+ * returns if success or error
+ */
+static acpi_status parse_package(union acpi_object *item)
+{
+		int value = 0, itr = 0, reg = 0;
+		union acpi_object *num;
+		struct WWAN_DEVICE_MODE_CONFIGURATION *data;
+
+		num = &item->package.elements[0];
+		if (get_int_value(num, &value) == 0) {
+			pr_debug("%s: Regulatory value : %d\n", __func__, value);
+			if (value >= 0 && value < MAX_REGULATORY)
+				reg = value;
+			else
+				return AE_ERROR;
+		}
+		data = &context->config_data[reg];
+		if (data->total_dev_mode == 0) {
+			pr_debug("Dev Mode count is zero, return\n");
+			return AE_OK;
+		}
+		data->device_mode_info =
+		kmalloc_array(data->total_dev_mode,
+			      sizeof(struct WWAN_DEVICE_MODE_INFO), GFP_KERNEL);
+		if (!data->device_mode_info) {
+			pr_err("Cannot allocate memory in kernel\n");
+			return AE_ERROR;
+		}
+		for (itr = 0; itr < data->total_dev_mode; itr++) {
+			if (itr + 1 == item->package.count)
+				break;
+			num = &item->package.elements[itr + 1];
+			if (num->type != ACPI_TYPE_PACKAGE)
+				continue;
+			if (get_int_value(&num->package.elements[0], &value) == 0) {
+				pr_debug("%s: Device Mode for mode %d: %d\n", __func__,
+					 itr, value);
+				data->device_mode_info[itr].device_mode = value;
+			}
+			if (get_int_value(&num->package.elements[1], &value) == 0) {
+				pr_debug("%s: band_index mode %d: %d\n", __func__,
+					 itr, value);
+				data->device_mode_info[itr].bandtable_index = value;
+			}
+			if (get_int_value(&num->package.elements[2], &value) == 0) {
+				pr_debug("%s: antenna_index mode %d: %d\n", __func__,
+					 itr, value);
+				data->device_mode_info[itr].antennatable_index = value;
+			}
+			if (get_int_value(&num->package.elements[3], &value) == 0) {
+				pr_debug("%s: sar_index for mode %d: %d\n", __func__,
+					 itr, value);
+				data->device_mode_info[itr].sartable_index = value;
+			}
+		}
+		return AE_OK;
+}
+
+/**
+ * sar_module_probe - Extraction of information from BIOS via DSM calls
+ * Retrieve all values related to device mode and SAR Table index,
+ * Antenna Table index, Band Table index
+ * @device: ACPI device for which to retrieve the data
+ * Returns AE_OK on success
+ */
+static acpi_status sar_module_probe(struct platform_device *device)
+{
+		acpi_status status = AE_OK;
+		union acpi_object *out, *item, req;
+		u32 rev = 0, reg = 0;
+		int value = 0;
+
+		pr_alert("%s Triggered\n", __func__);
+		if (!device) {
+			pr_err("%s: platform driver is null\n", __func__);
+			return AE_ERROR;
+		}
+		if (!context) {
+			pr_err("%s: context is null\n", __func__);
+			return AE_ERROR;
+		}
+		pr_debug("ACPI_HANDLE : %p\n", ACPI_HANDLE(&device->dev));
+		if (!(context->parse)) {
+			status = parse_guid();
+			if (status != AE_OK)
+				return status;
+		}
+		context->handle = ACPI_HANDLE(&device->dev);
+		out = acpi_evaluate_dsm(context->handle, &context->guid, rev,
+					COMMAND_ID_DEV_MODE, NULL);
+		pr_debug("%s: acpi_evaluate_dsm completed %d\n", __func__, out->type);
+		if (get_int_value(out, &value) == 0) {
+			pr_debug("%s: Device Mode is : %d\n", __func__, value);
+			context->sar_data.device_mode = value;
+		} else {
+			pr_err("%s: Cmd:%d Failed\n", __func__, COMMAND_ID_DEV_MODE);
+			return AE_ERROR;
+		}
+		ACPI_FREE(out);
+		if (!(context->data_read)) {
+			for (reg = 0; reg < MAX_REGULATORY; reg++) {
+				req.type = ACPI_TYPE_INTEGER;
+				req.integer.value = reg;
+				out = acpi_evaluate_dsm(context->handle, &context->guid, rev,
+							COMMAND_ID_CONFIG_TABLE, &req);
+				if (!out) {
+					pr_err("%s: Cmd:%d Failed\n", __func__,
+					       COMMAND_ID_CONFIG_TABLE);
+					continue;
+				}
+				pr_debug("%s: acpi_evaluate_dsm  for regulatory %d completed %d\n",
+					 __func__, reg, out->type);
+				if (out->type == ACPI_TYPE_PACKAGE) {
+					pr_debug("%s: ACPI_TYPE_PACKAGE, count: %d, type: %d\n",
+						 __func__, out->package.count, out->package.type);
+					item = &out->package.elements[0];
+					if (get_int_value(item, &value) == 0) {
+						pr_debug("%s: version : %d\n", __func__, value);
+						context->config_data[reg].version = value;
+					}
+					item = &out->package.elements[1];
+					if (get_int_value(item, &value) == 0) {
+						pr_debug("%s: No of Modes: %d\n", __func__, value);
+						context->config_data[reg].total_dev_mode = value;
+					}
+					if (context->config_data[reg].total_dev_mode <= 0 &&
+					    context->config_data[reg].total_dev_mode >
+					    MAX_DEV_MODES) {
+						pr_err("total_dev_mode is not within range : %d\n",
+						       context->config_data[reg].total_dev_mode);
+						ACPI_FREE(out);
+						return AE_ERROR;
+					}
+					item = &out->package.elements[2];
+					if (item)
+						status = parse_package(item);
+					else
+						status = AE_ERROR;
+					if (status != AE_OK) {
+						ACPI_FREE(out);
+						return status;
+					}
+				}
+				ACPI_FREE(out);
+			}
+			update_sar_data();
+			context->data_read = true;
+		}
+		sar_send_notify();
+		return status;
+}
+
+#ifdef DEBUG
+/**
+ * sar_set_device_mode - To set the device mode as BIOS handling test
+ * Test Function call to BIOS for device mode handling of data sent via
+ * DSM calls.
+ * @device: ACPI device for which to retrieve the data
+ * @mode: Device Mode to be set
+ */
+static acpi_status sar_set_device_mode(struct platform_device *device, int mode)
+{
+		union acpi_object *out, req;
+		u32 rev = 0;
+		int value = 0;
+		acpi_status status = AE_OK;
+
+		pr_debug("%s Triggered : mode : %d\n", __func__, mode);
+		if (!device) {
+			pr_err("%s: Device is null\n", __func__);
+			return AE_ERROR;
+		}
+		if (!context->handle) {
+			pr_err("%s: Handle is null\n", __func__);
+			return AE_ERROR;
+		}
+		if (!(context->parse)) {
+			status = parse_guid();
+			if (status != AE_OK)
+				return status;
+		}
+
+		req.type = ACPI_TYPE_INTEGER;
+		req.integer.value = mode;
+		out = acpi_evaluate_dsm(context->handle, &context->guid,
+					rev, COMMAND_TEST_SET, &req);
+		if (get_int_value(out, &value) != 0) {
+			pr_err("%s: Cmd:%d Failed\n", __func__, COMMAND_ID_DEV_MODE);
+			return AE_ERROR;
+		}
+		pr_debug("%s: return value is : %d\n", __func__, value);
+		ACPI_FREE(out);
+		return status;
+}
+#endif
+
+static const struct acpi_device_id sar_device_ids[] = {
+		{ "INTC1092", 0},
+		{ "", 0},
+};
+
+MODULE_DEVICE_TABLE(acpi, sar_device_ids);
+
+static const struct platform_device_id sar_device_table[] = {
+	{"intc1092", 0},
+	{},
+};
+
+static ssize_t sar_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+		unsigned long ret = 0;
+
+		pr_debug("%s triggered\n", __func__);
+		if (!context) {
+			pr_err("%s context is null", __func__);
+			return -EFAULT;
+		}
+		ret = sprintf(buf, "%d %d %d %d\n", context->sar_data.device_mode,
+			      context->sar_data.bandtable_index,
+			      context->sar_data.antennatable_index,
+			      context->sar_data.sartable_index);
+		pr_debug("%s sent: %s\n", __func__, buf);
+		return ret;
+}
+
+static ssize_t sar_reg_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+		unsigned long ret = 0;
+
+		pr_debug("%s triggered\n", __func__);
+		if (!context) {
+			pr_err("%s context is null", __func__);
+			return -EFAULT;
+		}
+		ret = sprintf(buf, "%d\n", context->reg_value);
+		pr_debug("%s sent: %s\n", __func__, buf);
+		return ret;
+}
+
+static ssize_t sar_reg_store(struct kobject *kobj, struct kobj_attribute *attr,
+			     const char *buf, size_t count)
+{
+		int value = 0, read = 0;
+
+		pr_debug("%s triggered\n", __func__);
+		if (!count) {
+			pr_err("%s count = %d", __func__, (int)count);
+			return -EFAULT;
+		}
+		read = sscanf(buf, "%u", &value);
+		pr_debug("%s received %s, integer value : %d", __func__, buf, value);
+		if (read <= 0) {
+			pr_alert("%s Not a integer", __func__);
+			return -EFAULT;
+		}
+		if (value >= 0 && value < MAX_REGULATORY) {
+			context->reg_value = value;
+			update_sar_data();
+			sar_send_notify();
+		}
+		return count;
+}
+
+static struct kobj_attribute sar_attribute = __ATTR(intc_data, 0660, sar_show, NULL);
+static struct kobj_attribute sar_attribute1 = __ATTR(intc_reg, 0660, sar_reg_show, sar_reg_store);
+
+#ifdef DEBUG
+static ssize_t sar_dev_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+		unsigned long ret = 0;
+
+		pr_debug("%s triggered\n", __func__);
+		if (!context) {
+			pr_err("%s context is null\n", __func__);
+			return -EFAULT;
+		}
+		ret = sprintf(buf, "%d\n", context->sar_data.device_mode);
+		pr_debug("%s sent: %s\n", __func__, buf);
+		return ret;
+}
+
+static ssize_t sar_dev_store(struct kobject *kobj, struct kobj_attribute *attr,
+			     const char *buf, size_t count)
+{
+		int value = 0, read = 0;
+		acpi_status status = AE_OK;
+
+		pr_debug("%s triggered\n", __func__);
+		if (!count) {
+			pr_err("%s count = %d", __func__, (int)count);
+			return -EFAULT;
+		}
+		read = sscanf(buf, "%u", &value);
+		pr_debug("%s received %s, integer value : %d", __func__, buf, value);
+		if (read <= 0) {
+			pr_alert("%s Not a integer", __func__);
+			return -EFAULT;
+		}
+		if (value < MAX_DEV_MODES) {
+			status = sar_set_device_mode(context->sar_device, value);
+			if (status != AE_OK) {
+				pr_err("sar_set_device_mode failed\n");
+				return -EINVAL;
+			}
+		}
+		return count;
+}
+
+static struct kobj_attribute sar_attribute2 = __ATTR(intc_dev, 0660, sar_dev_show, sar_dev_store);
+#endif
+
+MODULE_DEVICE_TABLE(platform, sar_device_table);
+
+static struct platform_driver sar_driver = {
+	.probe = sar_add,
+	.remove = sar_remove,
+	.shutdown = sar_shutdown,
+	.driver = {
+			.name = DRVNAME,
+			.owner = THIS_MODULE,
+			/* FOR ACPI HANDLING */
+			.acpi_match_table = ACPI_PTR(sar_device_ids),
+			},
+	.id_table = sar_device_table,
+};
+
+static int sar_add(struct platform_device *device)
+{
+		int result = 0;
+
+		pr_debug("%s Triggered\n", __func__);
+		context = kmalloc(sizeof(*context), GFP_KERNEL);
+		if (!context) {
+			pr_err("Cannot allocate memory in kernel for WWAN_SAR_CONTEXT\n");
+			return -1;
+		}
+		memset(context, 0, sizeof(struct WWAN_SAR_CONTEXT));
+
+		context->sar_kobject = kobject_create_and_add("intc_sar_object", kernel_kobj);
+		if (!context->sar_kobject) {
+			pr_err("Failed to create sar_kobject\n");
+			goto r_free;
+		}
+
+		result = sysfs_create_file(context->sar_kobject, &sar_attribute.attr);
+		if (result) {
+			pr_err("Failed to create the intc_data file in /sys/kernel/intc_sar_object\n");
+			goto r_sys;
+		}
+
+		result = sysfs_create_file(context->sar_kobject, &sar_attribute1.attr);
+		if (result) {
+			pr_err("Failed to create the intc_reg file in /sys/kernel/intc_sar_object\n");
+			goto r_sys;
+		}
+#ifdef DEBUG
+		result = sysfs_create_file(context->sar_kobject, &sar_attribute2.attr);
+		if (result) {
+			pr_err("Failed to create the intc_dev file in /sys/kernel/intc_sar_object\n");
+			goto r_sys;
+		}
+#endif
+		context->sar_device = device;
+		if (sar_module_probe(device) != AE_OK) {
+			pr_err("Failed sar_module_probe\n");
+			goto r_sys;
+		}
+
+		if (acpi_install_notify_handler(ACPI_HANDLE(&device->dev), ACPI_DEVICE_NOTIFY,
+						sar_notify, (void *)device) != AE_OK) {
+			pr_err("Failed acpi_install_notify_handler\n");
+			goto r_sys;
+		}
+		return 0;
+
+r_sys:
+		kobject_put(context->sar_kobject);
+r_free:
+		kfree(context);
+		return -1;
+}
+
+static int sar_remove(struct platform_device *device)
+{
+		pr_debug("%s Triggered\n", __func__);
+		acpi_remove_notify_handler(ACPI_HANDLE(&device->dev),
+					   ACPI_DEVICE_NOTIFY, sar_notify);
+		kobject_put(context->sar_kobject);
+		clear_sar_dev_mode();
+		kfree(context);
+		return 0;
+}
+
+static void sar_shutdown(struct platform_device *device)
+{
+		sar_remove(device);
+		return;
+}
+
+static void sar_notify(acpi_handle handle, u32 event, void *data)
+{
+		struct platform_device *device = data;
+
+		pr_debug("%s Triggered: event: %d\n", __func__, event);
+		if (event == SAR_EVENT) {
+			pr_debug("%s event matched\n", __func__);
+			if (sar_module_probe(device) != AE_OK)
+				pr_err("sar_module_probe error");
+		}
+}
+
+static int sar_init(void)
+{
+		int result = 0;
+
+		pr_alert("SAR Init Triggered\n");
+		result = platform_driver_register(&sar_driver);
+		if (result < 0)
+			pr_err("Error registering driver\n");
+		return result;
+}
+
+static void sar_exit(void)
+{
+		pr_alert("SAR EXIT Triggered\n");
+		platform_driver_unregister(&sar_driver);
+		pr_debug("Kernel Module Removed Successfully.\n");
+}
+
+module_init(sar_init);
+module_exit(sar_exit);
diff --git a/include/linux/platform_data/x86/intel-sar.h b/include/linux/platform_data/x86/intel-sar.h
new file mode 100644
index 000000000000..9ed653284fa5
--- /dev/null
+++ b/include/linux/platform_data/x86/intel-sar.h
@@ -0,0 +1,91 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Intel Corporation Header File for Specific Absorption Rate
+ * Copyright (c) 2021, Intel Corporation.
+ */
+#ifndef INTEL_SAR_H
+#define INTEL_SAR_H
+
+#define DRVNAME "sar"
+#define SAR_DSM_UUID "82737E72-3A33-4C45-A9C7-57C0411A5F13"
+#define COMMAND_ID_DEV_MODE 1
+#define COMMAND_ID_CONFIG_TABLE 2
+#define COMMAND_TEST_SET 31
+#define MAX_REGULATORY 3
+#define SAR_EVENT 0x80
+#define MAX_DEV_MODES 50
+
+/**
+ * Structure WWAN_DEVICE_MODE_INFO - device mode information
+ * Holds the data that needs to be passed to userspace.
+ * The data is updated from the BIOS sensor information.
+ * @device_mode: Specific mode of the device
+ * @bandtable_index: Index of RF band
+ * @antennatable_index: Index of antenna
+ * @sartable_index: Index of SAR
+ */
+struct WWAN_DEVICE_MODE_INFO {
+		int device_mode;
+		int bandtable_index;
+		int antennatable_index;
+		int sartable_index;
+};
+
+/**
+ * Structure WWAN_DEVICE_MODE_CONFIGURATION - device configuration
+ * Holds the data that is configured and obtained on probe event.
+ * The data is updated from the BIOS sensor information.
+ * @version: Mode configuration version
+ * @total_dev_mode: Total number of device modes
+ * @device_mode_info: pointer to structure WWAN_DEVICE_MODE_INFO
+ */
+struct WWAN_DEVICE_MODE_CONFIGURATION {
+		int version;
+		int total_dev_mode;
+		struct WWAN_DEVICE_MODE_INFO *device_mode_info;
+};
+
+/**
+ * Structure WWAN_SUPPORTED_INFO - userspace datastore
+ * Holds the data that is obtained from userspace
+ * The data is updated from the userspace and send value back in the
+ * structure format that is mentioned here.
+ * @reg_mode_needed: regulatory mode set by user for tests
+ * @bios_table_revision: Version of SAR table
+ * @num_supported_modes: Total supported modes based on reg_mode
+ */
+struct WWAN_SUPPORTED_INFO {
+		int reg_mode_needed;
+		int bios_table_revision;
+		int num_supported_modes;
+};
+
+/**
+ * Structure WWAN_SAR_CONTEXT - context of SAR
+ * Holds the complete context as long as the driver is in existence
+ * The context holds instance of the data used for different cases.
+ * @parse: identifies if dsm is parsed
+ * @data_read: identifies if data is already read from BIOS
+ * @guid: Group id
+ * @handle: store acpi handle
+ * @reg_value: regulatory value
+ * Regulatory 0: FCC, 1: CE, 2: ISED
+ * @sar_device: platform_device type
+ * @sar_kobject: kobject for sysfs
+ * @supported_data: WWAN_SUPPORTED_INFO struct
+ * @sar_data: WWAN_DEVICE_MODE_INFO struct
+ * @config_data: WWAN_DEVICE_MODE_CONFIGURATION array struct
+ */
+struct WWAN_SAR_CONTEXT {
+		bool parse;
+		bool data_read;
+		guid_t guid;
+		acpi_handle handle;
+		int reg_value;
+		struct platform_device *sar_device;
+		struct kobject *sar_kobject;
+		struct WWAN_SUPPORTED_INFO supported_data;
+		struct WWAN_DEVICE_MODE_INFO sar_data;
+		struct WWAN_DEVICE_MODE_CONFIGURATION config_data[MAX_REGULATORY];
+};
+#endif /* INTEL_SAR_H */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem
  2021-05-10  7:40 ` [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem Shravan S
@ 2021-06-24 10:24   ` Andy Shevchenko
  2021-06-28 15:12     ` Hans de Goede
  2021-06-28 14:45   ` Enrico Weigelt, metux IT consult
  2021-07-15 16:20   ` Hans de Goede
  2 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2021-06-24 10:24 UTC (permalink / raw)
  To: Shravan S; +Cc: Hans de Goede, Mark Gross, Platform Driver, An, Sudhakar

On Wed, Jun 23, 2021 at 5:04 PM Shravan S <s.shravan@intel.com> wrote:

Besides the fact of missed documentation, this code is far from the
upstream-ready quality.
Send a new version to our internal mailing list for the detailed review.

And as I said, the interface should be provided by WWAN subsystem (or
whatever subsystem(s) for this will be suitable *), while this should
be a driver between actual implementation and the abstract interface.
I believe this kind of feature will be needed (if not used already) by
non-Intel hardware and we should have a more unified interface from
day 1. Is it possible?

*) maybe you need to introduce a layer which will provide a common
interface between subsystems and hardware blocks for this kind of
functionality. Either way, lack of documentation is perceptible.

> V2 :
> * Changes to the remove netlink and introduce sysfs to communicate to Userspace via notify
> * Handled review comments by changing ioctl calls to sysfs
> * "sar" name change for platform_device_id to "intc1092"
> * sar_init and sar_exit is modified as per review to minimal initialization
> * Inclusion of debug only enabling of device mode change parameters

You mixed up changelog / commit message / cover letter altogether.
...

> +config INTEL_SAR
> +       tristate "Intel Specific Absorption Rate Driver"
> +       depends on ACPI
> +       help
> +         This driver limit the exposure of human body to RF frequency by informing
> +         the Intel M.2 modem to regulate the RF power based on SAR data obtained
> +         from the sensorscaptured in the BIOS. ACPI interface exposes this data

sensors captured

> +         from the BIOS to SAR driver. The front end application in userspace
> +         will interact with SAR driver to obtain information like the device mode,
> +         Antenna index,baseband index, SAR table index and use available communication
> +         like MBIM interface to enable data communication to modem for RF power regulation.

...

> +static void sar_send_notify(void)
> +{
> +               if (!context) {

> +                       pr_alert("%s: context is null\n", __func__);

Is it dead code?

> +                       return;
> +               }
> +               pr_debug("%s: %d, %d, %d, %d\n", __func__, context->sar_data.device_mode,
> +                        context->sar_data.bandtable_index, context->sar_data.antennatable_index,
> +                        context->sar_data.sartable_index);
> +               sysfs_notify(context->sar_kobject, NULL, "intc_data");
> +}

...

> +static void update_sar_data(void)
> +{
> +               pr_debug("%s: Update SAR data\n", __func__);
> +               if (context->config_data[context->reg_value].device_mode_info &&
> +                   context->sar_data.device_mode <=
> +                   context->config_data[context->reg_value].total_dev_mode) {
> +                       context->sar_data.antennatable_index =
> +                       context->config_data[context->reg_value]
> +                       .device_mode_info[context->sar_data.device_mode].antennatable_index;
> +                       context->sar_data.bandtable_index =
> +                       context->config_data[context->reg_value]
> +                       .device_mode_info[context->sar_data.device_mode].bandtable_index;
> +                       context->sar_data.sartable_index =
> +                       context->config_data[context->reg_value]
> +                       .device_mode_info[context->sar_data.device_mode].sartable_index;

Oy vey, this is quite hard to read, use temporary variables.

> +                       pr_debug("bandtable_index: %d\n", context->sar_data.bandtable_index);
> +                       pr_debug("antennatable_index : %d\n", context->sar_data.antennatable_index);
> +                       pr_debug("sartable_index: %d\n", context->sar_data.sartable_index);

No way. Drop debug prints from production code.


> +               } else {
> +                       pr_err("%s: sar data not assigned! Dev mode: %d, total_dev_mode: %d\n",
> +                              __func__, context->sar_data.device_mode,
> +                              context->config_data[context->reg_value].total_dev_mode);
> +               }
> +}


--
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem
  2021-05-10  7:40 ` [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem Shravan S
  2021-06-24 10:24   ` Andy Shevchenko
@ 2021-06-28 14:45   ` Enrico Weigelt, metux IT consult
  2021-07-15 16:20   ` Hans de Goede
  2 siblings, 0 replies; 10+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-06-28 14:45 UTC (permalink / raw)
  To: Shravan S, hdegoede, mgross, platform-driver-x86; +Cc: sudhakar.an

On 10.05.21 09:40, Shravan S wrote:

Hi,

> +config INTEL_SAR
> +	tristate "Intel Specific Absorption Rate Driver"
> +	depends on ACPI

Do it really need to be Intel specific ?

By the way: where exactly is Intel specific hardware involved here ?
It doesn't seem to be specific to Intel's modems at all, but instead the
mainboards, which seem to come from somebody else.

> +static struct WWAN_SAR_CONTEXT *context;
> +static int sar_add(struct platform_device *device);
> +static int sar_remove(struct platform_device *device);
> +static void sar_shutdown(struct platform_device *device);
> +static void sar_notify(acpi_handle handle, u32 event, void *data);

Oh, no, please no global singletons here.
Should be per device instance.

> + * sar_send_notify - Send SAR data notification to userspace
> + * Send the sar data notification to the userspace and later sent to modem
> + * by the userspace application. The data sent is collected from the BIOS.
> + * This data is used by userspace for further handling of the modem output.
> + */
> +static void sar_send_notify(void)
> +{
> +		if (!context) {
> +			pr_alert("%s: context is null\n", __func__);

Please no such global pr_alert() calls. And how could such a situation
happen in the first place ?

> +			return;
> +		}
> +		pr_debug("%s: %d, %d, %d, %d\n", __func__, context->sar_data.device_mode,
> +			 context->sar_data.bandtable_index, context->sar_data.antennatable_index,
> +			 context->sar_data.sartable_index);

It's a device specific message, therefore should be dev_debug()

> +		sysfs_notify(context->sar_kobject, NULL, "intc_data");
> +}
> +
> +/**
> + * clear_sar_dev_mode - Clear Device Mode by freeing the allocated data
> + * If the Device Mode Info is present and allocated, clear it. This is for
> + * dynamic allocated memory of device_mode_info
> + */
> +static void clear_sar_dev_mode(void)
> +{
> +		int reg = 0;
> +
> +		for (reg = 0; reg < MAX_REGULATORY; reg++) {
> +			kfree(context->config_data[reg].device_mode_info);
> +			context->config_data[reg].device_mode_info = NULL;
> +		}
> +}

This seems to belong into the device's release callback.

> +/**
> + * get_int_value - Retrieve Integer values from ACPI Object
> + * Value of the integer from the object of ACPI is obtained.
> + * @obj: acpi_object pointer which gets the integer value
> + * @out: output pointer for integer
> + * returns 0 on success
> + */
> +static int get_int_value(union acpi_object *obj, int *out)
> +{
> +		if (obj && obj->type == ACPI_TYPE_INTEGER) {
> +			*out = (int)obj->integer.value;
> +			return 0;
> +		} else {
> +			return -1;
> +		}
> +}

If we really don't have such an helper yet, this belongs into generic
ACPI code.

> +/**
> + * update_sar_data - sar data is updated based on reg_value
> + * sar_data is updated based on regulatory value
> + */
> +static void update_sar_data(void)
> +{

Also should operate on a device instance, not global singleton.

> +		pr_debug("%s: Update SAR data\n", __func__);
> +		if (context->config_data[context->reg_value].device_mode_info &&
> +		    context->sar_data.device_mode <=
> +		    context->config_data[context->reg_value].total_dev_mode) {
> +			context->sar_data.antennatable_index =
> +			context->config_data[context->reg_value]
> +			.device_mode_info[context->sar_data.device_mode].antennatable_index;
> +			context->sar_data.bandtable_index =
> +			context->config_data[context->reg_value]
> +			.device_mode_info[context->sar_data.device_mode].bandtable_index;
> +			context->sar_data.sartable_index =
> +			context->config_data[context->reg_value]
> +			.device_mode_info[context->sar_data.device_mode].sartable_index;
> +			pr_debug("bandtable_index: %d\n", context->sar_data.bandtable_index);
> +			pr_debug("antennatable_index : %d\n", context->sar_data.antennatable_index);
> +			pr_debug("sartable_index: %d\n", context->sar_data.sartable_index);
> +		} else {
> +			pr_err("%s: sar data not assigned! Dev mode: %d, total_dev_mode: %d\n",
> +			       __func__, context->sar_data.device_mode,
> +			       context->config_data[context->reg_value].total_dev_mode);
> +		}
> +}

<snip>

> +/**
> + * sar_module_probe - Extraction of information from BIOS via DSM calls
> + * Retrieve all values related to device mode and SAR Table index,
> + * Antenna Table index, Band Table index
> + * @device: ACPI device for which to retrieve the data
> + * Returns AE_OK on success
> + */
> +static acpi_status sar_module_probe(struct platform_device *device)
> +{
> +		acpi_status status = AE_OK;
> +		union acpi_object *out, *item, req;
> +		u32 rev = 0, reg = 0;
> +		int value = 0;
> +
> +		pr_alert("%s Triggered\n", __func__);

No pr_alert() here. Device specific logging shall be done with dev_*()
functions.

> +		if (!device) {

How could that ever happen ?

> +			pr_err("%s: platform driver is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		if (!context) {

'context' should be per device instance, so we'd allocate it here, as
the device's private data.

> +			pr_err("%s: context is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		pr_debug("ACPI_HANDLE : %p\n", ACPI_HANDLE(&device->dev));
> +		if (!(context->parse)) {
> +			status = parse_guid();
> +			if (status != AE_OK)
> +				return status;
> +		}
> +		context->handle = ACPI_HANDLE(&device->dev);
> +		out = acpi_evaluate_dsm(context->handle, &context->guid, rev,
> +					COMMAND_ID_DEV_MODE, NULL);
> +		pr_debug("%s: acpi_evaluate_dsm completed %d\n", __func__, out->type);
> +		if (get_int_value(out, &value) == 0) {
> +			pr_debug("%s: Device Mode is : %d\n", __func__, value);
> +			context->sar_data.device_mode = value;
> +		} else {
> +			pr_err("%s: Cmd:%d Failed\n", __func__, COMMAND_ID_DEV_MODE);
> +			return AE_ERROR;
> +		}
> +		ACPI_FREE(out);
> +		if (!(context->data_read)) {
> +			for (reg = 0; reg < MAX_REGULATORY; reg++) {
> +				req.type = ACPI_TYPE_INTEGER;
> +				req.integer.value = reg;
> +				out = acpi_evaluate_dsm(context->handle, &context->guid, rev,
> +							COMMAND_ID_CONFIG_TABLE, &req);
> +				if (!out) {
> +					pr_err("%s: Cmd:%d Failed\n", __func__,
> +					       COMMAND_ID_CONFIG_TABLE);
> +					continue;
> +				}
> +				pr_debug("%s: acpi_evaluate_dsm  for regulatory %d completed %d\n",
> +					 __func__, reg, out->type);
> +				if (out->type == ACPI_TYPE_PACKAGE) {
> +					pr_debug("%s: ACPI_TYPE_PACKAGE, count: %d, type: %d\n",
> +						 __func__, out->package.count, out->package.type);
> +					item = &out->package.elements[0];
> +					if (get_int_value(item, &value) == 0) {
> +						pr_debug("%s: version : %d\n", __func__, value);
> +						context->config_data[reg].version = value;
> +					}
> +					item = &out->package.elements[1];
> +					if (get_int_value(item, &value) == 0) {
> +						pr_debug("%s: No of Modes: %d\n", __func__, value);
> +						context->config_data[reg].total_dev_mode = value;
> +					}
> +					if (context->config_data[reg].total_dev_mode <= 0 &&
> +					    context->config_data[reg].total_dev_mode >
> +					    MAX_DEV_MODES) {
> +						pr_err("total_dev_mode is not within range : %d\n",
> +						       context->config_data[reg].total_dev_mode);
> +						ACPI_FREE(out);
> +						return AE_ERROR;
> +					}
> +					item = &out->package.elements[2];
> +					if (item)
> +						status = parse_package(item);
> +					else
> +						status = AE_ERROR;
> +					if (status != AE_OK) {
> +						ACPI_FREE(out);
> +						return status;
> +					}
> +				}
> +				ACPI_FREE(out);
> +			}
> +			update_sar_data();
> +			context->data_read = true;
> +		}
> +		sar_send_notify();
> +		return status;
> +}
> +
> +#ifdef DEBUG

Please no private #ifdef'd debug code. If you really wanna need to extra
debug code in here, look for some suitable generic CONFIG_* or add your
own driver specific one in Kconfig.

> +/**
> + * sar_set_device_mode - To set the device mode as BIOS handling test
> + * Test Function call to BIOS for device mode handling of data sent via
> + * DSM calls.
> + * @device: ACPI device for which to retrieve the data
> + * @mode: Device Mode to be set
> + */
> +static acpi_status sar_set_device_mode(struct platform_device *device, int mode)
> +{
> +		union acpi_object *out, req;
> +		u32 rev = 0;
> +		int value = 0;
> +		acpi_status status = AE_OK;
> +
> +		pr_debug("%s Triggered : mode : %d\n", __func__, mode);

Device specific logging should use dev_*() functions.

> +		if (!device) {

How can that ever happen ?

> +			pr_err("%s: Device is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		if (!context->handle) {

How can that ever happen ?

> +			pr_err("%s: Handle is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		if (!(context->parse)) {
> +			status = parse_guid();
> +			if (status != AE_OK)
> +				return status;
> +		}
> +
> +		req.type = ACPI_TYPE_INTEGER;
> +		req.integer.value = mode;
> +		out = acpi_evaluate_dsm(context->handle, &context->guid,
> +					rev, COMMAND_TEST_SET, &req);
> +		if (get_int_value(out, &value) != 0) {
> +			pr_err("%s: Cmd:%d Failed\n", __func__, COMMAND_ID_DEV_MODE);
> +			return AE_ERROR;
> +		}
> +		pr_debug("%s: return value is : %d\n", __func__, value);
> +		ACPI_FREE(out);
> +		return status;
> +}
> +#endif
> +
> +static const struct acpi_device_id sar_device_ids[] = {
> +		{ "INTC1092", 0},
> +		{ "", 0},
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, sar_device_ids);
> +
> +static const struct platform_device_id sar_device_table[] = {
> +	{"intc1092", 0},
> +	{},
> +};

Is this driver ever instantiated by another platform driver ?

<snip>

> +static struct kobj_attribute sar_attribute = __ATTR(intc_data, 0660, sar_show, NULL);
> +static struct kobj_attribute sar_attribute1 = __ATTR(intc_reg, 0660, sar_reg_show, sar_reg_store);

why not DEVICE_ATTR() ?

> +MODULE_DEVICE_TABLE(platform, sar_device_table);
> +
> +static struct platform_driver sar_driver = {
> +	.probe = sar_add,
> +	.remove = sar_remove,
> +	.shutdown = sar_shutdown,
> +	.driver = {
> +			.name = DRVNAME,
> +			.owner = THIS_MODULE,

is setting .owner still necessary ?

> +			/* FOR ACPI HANDLING */
> +			.acpi_match_table = ACPI_PTR(sar_device_ids),
> +			},
> +	.id_table = sar_device_table,
> +};
> +
> +static int sar_add(struct platform_device *device)

why isn't that called "sar_probe" ?

> +{
> +		int result = 0;
> +
> +		pr_debug("%s Triggered\n", __func__);
> +		context = kmalloc(sizeof(*context), GFP_KERNEL);
> +		if (!context) {
> +			pr_err("Cannot allocate memory in kernel for WWAN_SAR_CONTEXT\n");
> +			return -1;
> +		}
> +		memset(context, 0, sizeof(struct WWAN_SAR_CONTEXT));
> +
> +		context->sar_kobject = kobject_create_and_add("intc_sar_object", kernel_kobj);
> +		if (!context->sar_kobject) {
> +			pr_err("Failed to create sar_kobject\n");
> +			goto r_free;
> +		}

Are these attributes of the device or not ?

Either these belong to the device itself - then these shall be device
attributes - or its something more generic - then we shall have an own
subsys (maybe device class) for that.

> +		result = sysfs_create_file(context->sar_kobject, &sar_attribute.attr);
> +		if (result) {
> +			pr_err("Failed to create the intc_data file in /sys/kernel/intc_sar_object\n");

That's absolutely the wrong place for that. This directly is really just
for kernel *core* stuff, that has nothing to do with any hardware.

It more and more smells like we should have a subsys for this stuff.

> +static int sar_remove(struct platform_device *device)
> +{
> +		pr_debug("%s Triggered\n", __func__);
> +		acpi_remove_notify_handler(ACPI_HANDLE(&device->dev),
> +					   ACPI_DEVICE_NOTIFY, sar_notify);
> +		kobject_put(context->sar_kobject);
> +		clear_sar_dev_mode();
> +		kfree(context);
> +		return 0;
> +}
> +
> +static void sar_shutdown(struct platform_device *device)
> +{
> +		sar_remove(device);
> +		return;
> +}

Are you really sure it really needs the same logic on both remove and
shutdown ?

> +
> +static void sar_notify(acpi_handle handle, u32 event, void *data)
> +{
> +		struct platform_device *device = data;
> +
> +		pr_debug("%s Triggered: event: %d\n", __func__, event);
> +		if (event == SAR_EVENT) {
> +			pr_debug("%s event matched\n", __func__);
> +			if (sar_module_probe(device) != AE_OK)
> +				pr_err("sar_module_probe error");

Doesn't seem that "sar_module_probe()" is an appropriate name.
And does it really need to reload everything on each acpi notify ?

> +static int sar_init(void)
> +{
> +		int result = 0;
> +
> +		pr_alert("SAR Init Triggered\n");
> +		result = platform_driver_register(&sar_driver);
> +		if (result < 0)
> +			pr_err("Error registering driver\n");
> +		return result;
> +}
> +
> +static void sar_exit(void)
> +{
> +		pr_alert("SAR EXIT Triggered\n");
> +		platform_driver_unregister(&sar_driver);
> +		pr_debug("Kernel Module Removed Successfully.\n");
> +}
> +
> +module_init(sar_init);
> +module_exit(sar_exit);

use module_platform_driver() macro instead.


> diff --git a/include/linux/platform_data/x86/intel-sar.h b/include/linux/platform_data/x86/intel-sar.h
> new file mode 100644
> index 000000000000..9ed653284fa5
> --- /dev/null
> +++ b/include/linux/platform_data/x86/intel-sar.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Intel Corporation Header File for Specific Absorption Rate
> + * Copyright (c) 2021, Intel Corporation.
> + */

Do we really need that extra header file ?
I don't see any consumer outside the single-file driver.

> +#ifndef INTEL_SAR_H
> +#define INTEL_SAR_H
> +
> +#define DRVNAME "sar"
> +#define SAR_DSM_UUID "82737E72-3A33-4C45-A9C7-57C0411A5F13"
> +#define COMMAND_ID_DEV_MODE 1
> +#define COMMAND_ID_CONFIG_TABLE 2
> +#define COMMAND_TEST_SET 31
> +#define MAX_REGULATORY 3
> +#define SAR_EVENT 0x80
> +#define MAX_DEV_MODES 50
> +
> +/**
> + * Structure WWAN_DEVICE_MODE_INFO - device mode information
> + * Holds the data that needs to be passed to userspace.
> + * The data is updated from the BIOS sensor information.
> + * @device_mode: Specific mode of the device
> + * @bandtable_index: Index of RF band
> + * @antennatable_index: Index of antenna
> + * @sartable_index: Index of SAR
> + */
> +struct WWAN_DEVICE_MODE_INFO {

I don't think we're using all-caps struct names in the kernel.

> +		int device_mode;

what exactly is the device_mode here ?

> +		int bandtable_index;
> +		int antennatable_index;
> +		int sartable_index;

how many bits to the table indices actually need ?

> +};
> +
> +/**
> + * Structure WWAN_DEVICE_MODE_CONFIGURATION - device configuration
> + * Holds the data that is configured and obtained on probe event.
> + * The data is updated from the BIOS sensor information.
> + * @version: Mode configuration version
> + * @total_dev_mode: Total number of device modes
> + * @device_mode_info: pointer to structure WWAN_DEVICE_MODE_INFO
> + */
> +struct WWAN_DEVICE_MODE_CONFIGURATION {
> +		int version;
> +		int total_dev_mode;

what is exactly shall "total_dev_mode" mean ?
if it's telling the number of entries in the array, name it so.

> +		struct WWAN_DEVICE_MODE_INFO *device_mode_info;
> +};
> +
> +/**
> + * Structure WWAN_SUPPORTED_INFO - userspace datastore
> + * Holds the data that is obtained from userspace
> + * The data is updated from the userspace and send value back in the
> + * structure format that is mentioned here.
> + * @reg_mode_needed: regulatory mode set by user for tests
> + * @bios_table_revision: Version of SAR table
> + * @num_supported_modes: Total supported modes based on reg_mode
> + */
> +struct WWAN_SUPPORTED_INFO {
> +		int reg_mode_needed;
> +		int bios_table_revision;
> +		int num_supported_modes;
> +};
> +
> +/**
> + * Structure WWAN_SAR_CONTEXT - context of SAR
> + * Holds the complete context as long as the driver is in existence
> + * The context holds instance of the data used for different cases.
> + * @parse: identifies if dsm is parsed
> + * @data_read: identifies if data is already read from BIOS
> + * @guid: Group id
> + * @handle: store acpi handle
> + * @reg_value: regulatory value
> + * Regulatory 0: FCC, 1: CE, 2: ISED
> + * @sar_device: platform_device type
> + * @sar_kobject: kobject for sysfs
> + * @supported_data: WWAN_SUPPORTED_INFO struct
> + * @sar_data: WWAN_DEVICE_MODE_INFO struct
> + * @config_data: WWAN_DEVICE_MODE_CONFIGURATION array struct
> + */
> +struct WWAN_SAR_CONTEXT {
> +		bool parse;
> +		bool data_read;
> +		guid_t guid;
> +		acpi_handle handle;
> +		int reg_value;
> +		struct platform_device *sar_device;
> +		struct kobject *sar_kobject;
> +		struct WWAN_SUPPORTED_INFO supported_data;
> +		struct WWAN_DEVICE_MODE_INFO sar_data;
> +		struct WWAN_DEVICE_MODE_CONFIGURATION config_data[MAX_REGULATORY];
> +};

looks like this should be device's private data.


Finally, I still wonder how userland shall cope with that thing in a
sane way. Especially how shall it know which interface to tune ?



--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem
  2021-06-24 10:24   ` Andy Shevchenko
@ 2021-06-28 15:12     ` Hans de Goede
  2021-06-28 17:47       ` Enrico Weigelt, metux IT consult
  0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2021-06-28 15:12 UTC (permalink / raw)
  To: Andy Shevchenko, Shravan S; +Cc: Mark Gross, Platform Driver, An, Sudhakar

Hi,

On 6/24/21 12:24 PM, Andy Shevchenko wrote:
> On Wed, Jun 23, 2021 at 5:04 PM Shravan S <s.shravan@intel.com> wrote:
> 
> Besides the fact of missed documentation, this code is far from the
> upstream-ready quality.

I agree that the code needs more work here.

> Send a new version to our internal mailing list for the detailed review.

I'm not in favor of internal reviews, esp. not when new userspace
API is involved. I would greatly prefer for the discussions surrounding
this to be kept public.

> And as I said, the interface should be provided by WWAN subsystem (or
> whatever subsystem(s) for this will be suitable *), while this should
> be a driver between actual implementation and the abstract interface.
> I believe this kind of feature will be needed (if not used already) by
> non-Intel hardware and we should have a more unified interface from
> day 1. Is it possible?

I agree that ideally we should have some generic userspace API for this,
but I'm afraid that ATM that simply is not possible. This whole notion
of maximum tx power being controlled based on various sensors because
of SAR reasons is pretty new (at least in the PC/laptop space) and
I know of a couple of vendors who are slapping some adhoc firmware
interface on the sensor readings for some modem related userspace
process to consume.

The 2 implementations which I know about for this are wildly different
and I expect we will see more, equally creative firmware interfaces
for this.

I don't think that it is realistic to try and layer a common userspace
interface over this at this point time. Actually I believe that even
trying to do so is a bad idea at this point in time, since this is
all so new that we are bound to get it badly wrong if we try to
design a common userspace API for this now.

I also don't want to wait for this to all crystallize out since that
will likely take years; and we should add support for this under Linux
in a reasonable time-frame. For laptops which ship with Linux
pre-installed it is important that there is feature parity between
Windows and Linux; and support for these new type of modems which need
this "SAR" integration is one of the biggest pain points with this ATM.

So for now I'm ok with having vendor specific userspace API for this
as long as it is somewhat sane and as long as it is very clear from
the sysfs path that this is vendor specific.

> *) maybe you need to introduce a layer which will provide a common
> interface between subsystems and hardware blocks for this kind of
> functionality. Either way, lack of documentation is perceptible.
> 
>> V2 :
>> * Changes to the remove netlink and introduce sysfs to communicate to Userspace via notify
>> * Handled review comments by changing ioctl calls to sysfs

I've not looked at the actual code at al yet, with that set thank you
changing over to sysfs.

I assume the sysfs atttributes for this all sit under

/sys/bus/platform/devices/INTC1092:00  ? 

And thus it is very clear that these are sysfs attributes specifically
for the INTC1092 device ?

If that is the case; and assuming that the new sysfs attributes API
is sane I don't see the API issue as blocking this driver.

But as both Andy and Enrico have pointed out the code needs a lot
of work.

Shravan, please post a new version addressing all remarks made
sofar and then I'll try to make some time to review the sysfs
userspace API for this (and later I'll also review the code).

Regards,

Hans




>> * "sar" name change for platform_device_id to "intc1092"
>> * sar_init and sar_exit is modified as per review to minimal initialization
>> * Inclusion of debug only enabling of device mode change parameters
> 
> You mixed up changelog / commit message / cover letter altogether.
> ...
> 
>> +config INTEL_SAR
>> +       tristate "Intel Specific Absorption Rate Driver"
>> +       depends on ACPI
>> +       help
>> +         This driver limit the exposure of human body to RF frequency by informing
>> +         the Intel M.2 modem to regulate the RF power based on SAR data obtained
>> +         from the sensorscaptured in the BIOS. ACPI interface exposes this data
> 
> sensors captured
> 
>> +         from the BIOS to SAR driver. The front end application in userspace
>> +         will interact with SAR driver to obtain information like the device mode,
>> +         Antenna index,baseband index, SAR table index and use available communication
>> +         like MBIM interface to enable data communication to modem for RF power regulation.
> 
> ...
> 
>> +static void sar_send_notify(void)
>> +{
>> +               if (!context) {
> 
>> +                       pr_alert("%s: context is null\n", __func__);
> 
> Is it dead code?
> 
>> +                       return;
>> +               }
>> +               pr_debug("%s: %d, %d, %d, %d\n", __func__, context->sar_data.device_mode,
>> +                        context->sar_data.bandtable_index, context->sar_data.antennatable_index,
>> +                        context->sar_data.sartable_index);
>> +               sysfs_notify(context->sar_kobject, NULL, "intc_data");
>> +}
> 
> ...
> 
>> +static void update_sar_data(void)
>> +{
>> +               pr_debug("%s: Update SAR data\n", __func__);
>> +               if (context->config_data[context->reg_value].device_mode_info &&
>> +                   context->sar_data.device_mode <=
>> +                   context->config_data[context->reg_value].total_dev_mode) {
>> +                       context->sar_data.antennatable_index =
>> +                       context->config_data[context->reg_value]
>> +                       .device_mode_info[context->sar_data.device_mode].antennatable_index;
>> +                       context->sar_data.bandtable_index =
>> +                       context->config_data[context->reg_value]
>> +                       .device_mode_info[context->sar_data.device_mode].bandtable_index;
>> +                       context->sar_data.sartable_index =
>> +                       context->config_data[context->reg_value]
>> +                       .device_mode_info[context->sar_data.device_mode].sartable_index;
> 
> Oy vey, this is quite hard to read, use temporary variables.
> 
>> +                       pr_debug("bandtable_index: %d\n", context->sar_data.bandtable_index);
>> +                       pr_debug("antennatable_index : %d\n", context->sar_data.antennatable_index);
>> +                       pr_debug("sartable_index: %d\n", context->sar_data.sartable_index);
> 
> No way. Drop debug prints from production code.
> 
> 
>> +               } else {
>> +                       pr_err("%s: sar data not assigned! Dev mode: %d, total_dev_mode: %d\n",
>> +                              __func__, context->sar_data.device_mode,
>> +                              context->config_data[context->reg_value].total_dev_mode);
>> +               }
>> +}
> 
> 
> --
> With Best Regards,
> Andy Shevchenko
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem
  2021-06-28 15:12     ` Hans de Goede
@ 2021-06-28 17:47       ` Enrico Weigelt, metux IT consult
  2021-06-28 18:20         ` Hans de Goede
  0 siblings, 1 reply; 10+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-06-28 17:47 UTC (permalink / raw)
  To: Hans de Goede, Andy Shevchenko, Shravan S
  Cc: Mark Gross, Platform Driver, An, Sudhakar

On 28.06.21 17:12, Hans de Goede wrote:

Hi,

> I'm not in favor of internal reviews, esp. not when new userspace
> API is involved. I would greatly prefer for the discussions surrounding
> this to be kept public.

ACK. Please do that in open public. There're still lots of things to
discuss that should be discussed in wide public, instead of comittee
behind closed doors.

> I agree that ideally we should have some generic userspace API for this,
> but I'm afraid that ATM that simply is not possible. 

Why not ? Lets collect the actual requirements and talk about some
viable solutions (I've already got a few ideas ...)

> This whole notion of maximum tx power being controlled based on various 
> sensors because of SAR reasons is pretty new (at least in the PC/laptop space) 
> and I know of a couple of vendors who are slapping some adhoc firmware
> interface on the sensor readings for some modem related userspace
> process to consume.

We should bring them here onboard, public discussion. And at the same
time we should make it crystally clear to them that weird adhoc hacks
won't be accepted and just give them very bad reputation and
shitstorming. Seriously, I believe we should go that route, in order
to prevent even more damage by insane firmware interfaces.

Such stuff really doesn't belong into firmware, at least not the way its
done now. Instead there just should be a clear description of the actual
hardware.

> I don't think that it is realistic to try and layer a common userspace
> interface over this at this point time. Actually I believe that even
> trying to do so is a bad idea at this point in time, since this is
> all so new that we are bound to get it badly wrong if we try to
> design a common userspace API for this now.

actually, I don't think it should go through userland (except perhaps
a knob for switching it on/off).

> I also don't want to wait for this to all crystallize out since that
> will likely take years; and we should add support for this under Linux
> in a reasonable time-frame. For laptops which ship with Linux
> pre-installed it is important that there is feature parity between
> Windows and Linux; and support for these new type of modems which need
> this "SAR" integration is one of the biggest pain points with this ATM.

I don't think that this should serve as an excuse for slappy and vendor 
specific solutions, especially when userland is involved.

And if we really do that, then just as some intermediate solution,
and lets put it under staging.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem
  2021-06-28 17:47       ` Enrico Weigelt, metux IT consult
@ 2021-06-28 18:20         ` Hans de Goede
  2021-06-29 10:08           ` Enrico Weigelt, metux IT consult
  0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2021-06-28 18:20 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult, Andy Shevchenko, Shravan S
  Cc: Mark Gross, Platform Driver, An, Sudhakar

Hi Enrico,

On 6/28/21 7:47 PM, Enrico Weigelt, metux IT consult wrote:
> On 28.06.21 17:12, Hans de Goede wrote:
> 
> Hi,
> 
>> I'm not in favor of internal reviews, esp. not when new userspace
>> API is involved. I would greatly prefer for the discussions surrounding
>> this to be kept public.
> 
> ACK. Please do that in open public. There're still lots of things to
> discuss that should be discussed in wide public, instead of comittee
> behind closed doors.
> 
>> I agree that ideally we should have some generic userspace API for this,
>> but I'm afraid that ATM that simply is not possible. 
> 
> Why not ? Lets collect the actual requirements and talk about some
> viable solutions (I've already got a few ideas ...)

Because we don't know the actual requirements yet. This is a very
young technology and still evolving fast. Also whether we like it
or not, we don't get to dictate what the involved firmware and
hardware interfaces get to look like. So any API which we come
up with must be capable of working with the existing fw and
hw interfaces as shipped in actual devices.

>> This whole notion of maximum tx power being controlled based on various sensors because of SAR reasons is pretty new (at least in the PC/laptop space) and I know of a couple of vendors who are slapping some adhoc firmware
>> interface on the sensor readings for some modem related userspace
>> process to consume.
> 
> We should bring them here onboard, public discussion. And at the same
> time we should make it crystally clear to them that weird adhoc hacks
> won't be accepted and just give them very bad reputation and
> shitstorming. Seriously, I believe we should go that route, in order
> to prevent even more damage by insane firmware interfaces.

<sigh> we are in no place to make demands here "standard" (non
chrome-os / android) Linux laptop-os usage is a tiny fraction of the
market. So new features like this are primarily developed on other OS-es
and typically we either take the firmware interfaces as is, or we don't
support the feature.

You seem to believe in an utopia where we fully control all the layers
and can design and implement everything to be just perfect, but the
reality is quite different from this.

You also seem to forget that perfect is the enemy of good. This case is
an excellent example of a case where we cannot design anything close to
the "perfect" API in one go because we don't have the necessary
problem-domain information / experience yet.

> Such stuff really doesn't belong into firmware, at least not the way its
> done now. Instead there just should be a clear description of the actual
> hardware.

Yes you've made your opinion on this abundantly clear and I must say
that the whole tone of your emails in this thread and the
"I know better then everyone else" attitude in this thread seriously
rubs me the wrong way.

Don't be surprised if I do not answer any further emails from you in
this thread. That won't be because I don't read them, but that will be
because I deliberately choose to not answer them because IMHO your
strong opinion on how everyone must bow to your vision of how exactly
this all must be implemented adds very little of value to this thread.

Regards,

Hans


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem
  2021-06-28 18:20         ` Hans de Goede
@ 2021-06-29 10:08           ` Enrico Weigelt, metux IT consult
  0 siblings, 0 replies; 10+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-06-29 10:08 UTC (permalink / raw)
  To: Hans de Goede, Andy Shevchenko, Shravan S
  Cc: Mark Gross, Platform Driver, An, Sudhakar

On 28.06.21 20:20, Hans de Goede wrote:

>> Why not ? Lets collect the actual requirements and talk about some
>> viable solutions (I've already got a few ideas ...)
> 
> Because we don't know the actual requirements yet. This is a very
> young technology and still evolving fast. 

Well, with some background in HF engineering, it's not so hard guessing
in which direction it goes. Of course, there're lots of tiny details,
but I don't think they have huge impact on how such subsys could look
like.

Let's recap, what this is actually for: we wanna limit the RF power for
other reasons than just the regulatory ones - in this case biological
compatibility. There can be other reasons like EM interference or
explosion detection. Indeed we already have these in embedded world,
we just didn't invent a generic control mechanism for that.

In the case of bioprotection of these tablets, the decision is *usually*
made by whether a human being is currently holding it. In this
particular case this seems to be done by certain sensors. But there can
be other scenarios, where it always needs to be active, eg. in an ER.

Question A: where exactly do we have to cap the power ? (which RF)
This depends on the actual board (antenna configuration, wires, etc) and
possibly environmental parameters. (think of external antennas, possibly
with extra amps). In general we need a clean association to the actual
devices to be tuned. For out-of-the-box devices there should be a sane
preset from the vendor, but at some point some operator might tune it.

Question B: what are the exact RF power limits to apply ?
This depends on a lot of factors and needs actual lab measurements.
Certainly, it's the duty of the vendor to do these measurements and
provide scientific data. He should also provide sane defaults, but
operator needs to be able to tune this. External antenna with higher
gain is the most obvious case.

Question C: when to apply which "operation mode" (IOW: which set of
limits to apply right now ? This depends on various factors, depending
on the actual use case. One parameter could be whether some user's
holding the device, but there could be others like are we in a specific
building or room.

While distilling this out, we see that:

a) it hasn't much to do with some specific RF module or even some
    specific vendor. therefore designing something that's just for
    the currently discussed Intel Modems, it's conceptionally wrong
    (even though I appreciate that Intel is sponsoring that kind of
    work)

b) the human-user detection is an entirely different field, just like
    ambient or g-sensors. we should split off that topic and look for
    a proper place to do that (does that even need kernel support ?)

Requirements:

1. the solution needs to provide means for capping RF power on certain
    RF devices depending on configurable tables, that can be tuned by
    by operator / system integrator
2. the solution needs to support several "operation modes" that can be
    selected by configurable factors, e.g. whether human is nearby,
    device location, time of day, etc.
3. the vendor shall supply sane defaults (based on actual lab
    measurements) for specific hardware for common scenarios

As things are right now, I don't see that the need to touch the kernel
at all. The most interesting question here is how to get the vendor's
presets. But even if he puts them into acpi tables, we can read them
out from userland by now.

IMHO, the best approach for that would be just collecting the data in
some central place, one package that other tools can just use. We have
similar approach for things like firmware or microcode. Actually, we
already have something similar with tzdata, and we don't let the kernel
try to fetch tz data from all the various authorities.

In summary, I don't see that we have to do anything on kernel side
right now. It's entirely up to userland.

> Also whether we like it
> or not, we don't get to dictate what the involved firmware and
> hardware interfaces get to look like. So any API which we come
> up with must be capable of working with the existing fw and
> hw interfaces as shipped in actual devices.

No, we don't. But we don't need to support all the most weird stuff.
We don't do special hacks for Nvidia's or NI's proprietary crap,
do we ?

What we certainly can do is ignore and blame.

As said, the only new thing that comes in here is extracting the vendors
presets from acpi (if we don't have any better data source). We don't
need any extra kernel support for that.

What Intel can do in this case:
* drop the whole idea of special kernel drivers
* drop the idea of proprietary acpi stuff
* work closely with the board vendors on a public database for the
   sane presets in certain use cases (that I called "operation modes")

> <sigh> we are in no place to make demands here "standard" (non
> chrome-os / android) Linux laptop-os usage is a tiny fraction of the
> market. 

Where did you get this data from ? It might be correct that only few
vendors offer their notebooks specifically for Linux, but that tells
nothing about the actual user numbers. In recent years, the percentage
of Windows users (especialyl on laptops) of my non-tech frieds is
rapidly decreasing - not representative, but shows a strong direction.

But at the beginning of the discussion, especially Chromebooks have been
mentioned as some primary platform (at least for now) - these indeed are
specifically made for Linux. The most likely next category in line are
android smartphones - again Linux. (BTW is there any that runs acpi ?).
And it seems the most push right now coming from Intel, Shravan et al.
He's one of us, Linux kernel folks. So I'm very confident in claming
that we actually do have a great deal of influence on that front now.

> So new features like this are primarily developed on other OS-es
> and typically we either take the firmware interfaces as is, or we don't
> support the feature.

I wouldn't underline that. This particular topic (more precisely some
superset of it) indeed has been implemented on Linux-based embedded
devices long ago. Yet device specific, not standardized. And entirely
in userspace. Nothing that the Kernel community ever needed to care
about.

> You seem to believe in an utopia where we fully control all the layers
> and can design and implement everything to be just perfect, but the
> reality is quite different from this.

In embedded world, we actually are to a very large extent. (at least on
software and custom hardware).

> You also seem to forget that perfect is the enemy of good. This case is
> an excellent example of a case where we cannot design anything close to
> the "perfect" API in one go because we don't have the necessary
> problem-domain information / experience yet.

As mentioned above, we don't need to do that at all. At least for the
kernel. The only thing we *should* do (but that's not a kernel topic) is
finding a common mechanism to retrieve and collect the preset data from
the board vendors. Everything's userland only.

--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem
  2021-05-10  7:40 ` [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem Shravan S
  2021-06-24 10:24   ` Andy Shevchenko
  2021-06-28 14:45   ` Enrico Weigelt, metux IT consult
@ 2021-07-15 16:20   ` Hans de Goede
  2021-07-27 14:38     ` Shravan, S
  2 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2021-07-15 16:20 UTC (permalink / raw)
  To: Shravan S, mgross, platform-driver-x86; +Cc: sudhakar.an

Hi Shravan,

Here is a first set of review remarks, note this is just from
a very quick glance, so these are just some initial remarks.
I will do a more thorough review after you post a v3 addressing these
initial remarks.

First of all the subject of the patch (first line of the commit msg)
should be:

"platform/x86: Add BIOS Dynamic SAR driver for Intel M.2 Modem"

And then after that you should have a proper commit msg describing
what this driver is and why it is necessary.

And you commit msg must end with this line:

Signed-off-by: Shravan S <s.shravan@intel.com>

See: https://elinux.org/Developer_Certificate_Of_Origin

On 5/10/21 9:40 AM, Shravan S wrote:
> V2 :
> * Changes to the remove netlink and introduce sysfs to communicate to Userspace via notify
> * Handled review comments by changing ioctl calls to sysfs
> * "sar" name change for platform_device_id to "intc1092"
> * sar_init and sar_exit is modified as per review to minimal initialization
> * Inclusion of debug only enabling of device mode change parameters

Having a changelog is good, but you need to put it below the
"Signed-off-by: Shravan S <s.shravan@intel.com>" line like this:

Signed-off-by: Shravan S <s.shravan@intel.com
---
V2 :
* Changes to the remove netlink and introduce ...


> ---
>  drivers/platform/x86/Kconfig                |  12 +
>  drivers/platform/x86/Makefile               |   1 +
>  drivers/platform/x86/intel-sar.c            | 550 ++++++++++++++++++++
>  include/linux/platform_data/x86/intel-sar.h |  91 ++++
>  4 files changed, 654 insertions(+)
>  create mode 100644 drivers/platform/x86/intel-sar.c
>  create mode 100644 include/linux/platform_data/x86/intel-sar.h
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 60592fb88e7a..6dfa89310677 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1313,6 +1313,18 @@ config INTEL_TELEMETRY
>  	  directly via debugfs files. Various tools may use
>  	  this interface for SoC state monitoring.
>  
> +config INTEL_SAR
> +	tristate "Intel Specific Absorption Rate Driver"
> +	depends on ACPI
> +	help
> +	  This driver limit the exposure of human body to RF frequency by informing
> +	  the Intel M.2 modem to regulate the RF power based on SAR data obtained
> +	  from the sensorscaptured in the BIOS. ACPI interface exposes this data

missing space between sensors and captured.
> +	  from the BIOS to SAR driver. The front end application in userspace
> +	  will interact with SAR driver to obtain information like the device mode,
> +	  Antenna index,baseband index, SAR table index and use available communication

missing space after the first comma (',')

> +	  like MBIM interface to enable data communication to modem for RF power regulation.

Maybe this would be a good start of the commit msg?


> +
>  endif # X86_PLATFORM_DEVICES
>  
>  config PMC_ATOM
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index dcc8cdb95b4d..548ff663c4af 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -123,6 +123,7 @@ obj-$(CONFIG_INTEL_SMARTCONNECT)		+= intel-smartconnect.o
>  obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE)	+= intel_speed_select_if/
>  obj-$(CONFIG_INTEL_TURBO_MAX_3)			+= intel_turbo_max_3.o
>  obj-$(CONFIG_INTEL_UNCORE_FREQ_CONTROL)		+= intel-uncore-frequency.o
> +obj-$(CONFIG_INTEL_SAR)				+= intel-sar.o
>  
>  # Intel PMIC / PMC / P-Unit devices
>  obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU)	+= intel_bxtwc_tmu.o
> diff --git a/drivers/platform/x86/intel-sar.c b/drivers/platform/x86/intel-sar.c
> new file mode 100644
> index 000000000000..fa16ae59851b
> --- /dev/null
> +++ b/drivers/platform/x86/intel-sar.c
> @@ -0,0 +1,550 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Intel Corporation - ACPI for Specific Absorption Rate
> + * Copyright (c) 2021, Intel Corporation.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
> +#include <uapi/linux/errno.h>

You should never use uapi headers in kernel code

> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +#include <linux/platform_data/x86/intel-sar.h>

please sort all the includes alphabetically.

> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Platform device driver for INTEL MODEM BIOS SAR");
> +MODULE_AUTHOR("Shravan S <s.shravan@intel.com>");

PLease move these 3 MODULE_... lines to the end of the file.

> +
> +static struct WWAN_SAR_CONTEXT *context;

You are binding to a platform_dev, so no global context
pointer please. Instead please use platform_set_drvdata() /
platform_get_drvdata() and the option to pass a data
pointer to other callbacks.


> +static int sar_add(struct platform_device *device);
> +static int sar_remove(struct platform_device *device);
> +static void sar_shutdown(struct platform_device *device);
> +static void sar_notify(acpi_handle handle, u32 event, void *data);

Please move the definition of these functions to above their
first use and drop these forward declarations.

> +
> +/**
> + * sar_send_notify - Send SAR data notification to userspace
> + * Send the sar data notification to the userspace and later sent to modem
> + * by the userspace application. The data sent is collected from the BIOS.
> + * This data is used by userspace for further handling of the modem output.
> + */
> +static void sar_send_notify(void)
> +{


You are using 2 tabs instead of 1 as indentation level here and everywhere
else, please fix this everywhere.

> +		if (!context) {
> +			pr_alert("%s: context is null\n", __func__);
> +			return;
> +		}

context should be passed to this function (see above) and then this check
can go away.

> +		pr_debug("%s: %d, %d, %d, %d\n", __func__, context->sar_data.device_mode,
> +			 context->sar_data.bandtable_index, context->sar_data.antennatable_index,
> +			 context->sar_data.sartable_index);

Use dev_dbg() please, also you have a lot of debugging prints in this
driver, are all these really necessary? Please use less debugging prints
if possible.

> +		sysfs_notify(context->sar_kobject, NULL, "intc_data");

So after removing the useless context check + the over-zealous debuging
this function is reduced to just this one line, please drop this
function and instead directly write this one line.

> +}
> +
> +/**
> + * clear_sar_dev_mode - Clear Device Mode by freeing the allocated data
> + * If the Device Mode Info is present and allocated, clear it. This is for
> + * dynamic allocated memory of device_mode_info
> + */
> +static void clear_sar_dev_mode(void)
> +{
> +		int reg = 0;
> +
> +		for (reg = 0; reg < MAX_REGULATORY; reg++) {
> +			kfree(context->config_data[reg].device_mode_info);
> +			context->config_data[reg].device_mode_info = NULL;
> +		}
> +}

Please just move this code into sar_remove(), and drop this function.

> +
> +/**
> + * get_int_value - Retrieve Integer values from ACPI Object
> + * Value of the integer from the object of ACPI is obtained.
> + * @obj: acpi_object pointer which gets the integer value
> + * @out: output pointer for integer
> + * returns 0 on success
> + */
> +static int get_int_value(union acpi_object *obj, int *out)
> +{
> +		if (obj && obj->type == ACPI_TYPE_INTEGER) {
> +			*out = (int)obj->integer.value;
> +			return 0;
> +		} else {
> +			return -1;

Please use -ESOME_ERRORCODE here.

> +		}
> +}
> +
> +/**
> + * update_sar_data - sar data is updated based on reg_value
> + * sar_data is updated based on regulatory value
> + */
> +static void update_sar_data(void)
> +{
> +		pr_debug("%s: Update SAR data\n", __func__);
> +		if (context->config_data[context->reg_value].device_mode_info &&
> +		    context->sar_data.device_mode <=
> +		    context->config_data[context->reg_value].total_dev_mode) {

Please use:

	struct WWAN_DEVICE_MODE_INFO *info;
	int idx = context->reg_value;

	/* FIXME what if idx > the length of the config_data array? Add check here */

	info = context->config_data[idx].device_mode_info;

	if (!info || context->sar_data.device_mode > context->config_data[idx].total_dev_mode) {
		dev_err(..., "sar data not assigned! ...);
		return;
	}

	// Do all the assigns here using the new idx helper and put one assignment
	// per line, the split up assingment code below is simply not readable at all.



> +			context->sar_data.antennatable_index =
> +			context->config_data[context->reg_value]
> +			.device_mode_info[context->sar_data.device_mode].antennatable_index;
> +			context->sar_data.bandtable_index =
> +			context->config_data[context->reg_value]
> +			.device_mode_info[context->sar_data.device_mode].bandtable_index;
> +			context->sar_data.sartable_index =
> +			context->config_data[context->reg_value]
> +			.device_mode_info[context->sar_data.device_mode].sartable_index;
> +			pr_debug("bandtable_index: %d\n", context->sar_data.bandtable_index);
> +			pr_debug("antennatable_index : %d\n", context->sar_data.antennatable_index);
> +			pr_debug("sartable_index: %d\n", context->sar_data.sartable_index);

Is this debugging really necessary?

> +		} else {
> +			pr_err("%s: sar data not assigned! Dev mode: %d, total_dev_mode: %d\n",
> +			       __func__, context->sar_data.device_mode,
> +			       context->config_data[context->reg_value].total_dev_mode);
> +		}
> +}
> +
> +/**
> + * parse_guid - parse guid based on DSM UUID
> + * returns if success or error
> + */
> +static acpi_status parse_guid(void)
> +{
> +		if (guid_parse(SAR_DSM_UUID, &context->guid)) {
> +			pr_err("%s: UUID error\n", __func__);
> +			return AE_ERROR;
> +		}
> +		context->parse = true;
> +		return AE_OK;
> +}

Please drop this weird helper and just do the parse once from
sar_add() and fail the probe if the parse fails, you can then also
drop the parse boolean value from the context.


> +
> +/**
> + * parse_package - parse package for SAR
> + * @item : acpi_object ptr
> + * returns if success or error
> + */
> +static acpi_status parse_package(union acpi_object *item)
> +{
> +		int value = 0, itr = 0, reg = 0;
> +		union acpi_object *num;
> +		struct WWAN_DEVICE_MODE_CONFIGURATION *data;
> +
> +		num = &item->package.elements[0];
> +		if (get_int_value(num, &value) == 0) {
> +			pr_debug("%s: Regulatory value : %d\n", __func__, value);
> +			if (value >= 0 && value < MAX_REGULATORY)
> +				reg = value;
> +			else
> +				return AE_ERROR;
> +		}
> +		data = &context->config_data[reg];
> +		if (data->total_dev_mode == 0) {
> +			pr_debug("Dev Mode count is zero, return\n");
> +			return AE_OK;
> +		}
> +		data->device_mode_info =
> +		kmalloc_array(data->total_dev_mode,
> +			      sizeof(struct WWAN_DEVICE_MODE_INFO), GFP_KERNEL);
> +		if (!data->device_mode_info) {
> +			pr_err("Cannot allocate memory in kernel\n");
> +			return AE_ERROR;
> +		}
> +		for (itr = 0; itr < data->total_dev_mode; itr++) {
> +			if (itr + 1 == item->package.count)
> +				break;
> +			num = &item->package.elements[itr + 1];
> +			if (num->type != ACPI_TYPE_PACKAGE)
> +				continue;
> +			if (get_int_value(&num->package.elements[0], &value) == 0) {
> +				pr_debug("%s: Device Mode for mode %d: %d\n", __func__,
> +					 itr, value);
> +				data->device_mode_info[itr].device_mode = value;
> +			}
> +			if (get_int_value(&num->package.elements[1], &value) == 0) {
> +				pr_debug("%s: band_index mode %d: %d\n", __func__,
> +					 itr, value);
> +				data->device_mode_info[itr].bandtable_index = value;
> +			}
> +			if (get_int_value(&num->package.elements[2], &value) == 0) {
> +				pr_debug("%s: antenna_index mode %d: %d\n", __func__,
> +					 itr, value);
> +				data->device_mode_info[itr].antennatable_index = value;
> +			}
> +			if (get_int_value(&num->package.elements[3], &value) == 0) {
> +				pr_debug("%s: sar_index for mode %d: %d\n", __func__,
> +					 itr, value);
> +				data->device_mode_info[itr].sartable_index = value;
> +			}
> +		}
> +		return AE_OK;
> +}
> +
> +/**
> + * sar_module_probe - Extraction of information from BIOS via DSM calls
> + * Retrieve all values related to device mode and SAR Table index,
> + * Antenna Table index, Band Table index
> + * @device: ACPI device for which to retrieve the data
> + * Returns AE_OK on success
> + */
> +static acpi_status sar_module_probe(struct platform_device *device)
> +{
> +		acpi_status status = AE_OK;
> +		union acpi_object *out, *item, req;
> +		u32 rev = 0, reg = 0;
> +		int value = 0;
> +
> +		pr_alert("%s Triggered\n", __func__);
> +		if (!device) {
> +			pr_err("%s: platform driver is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		if (!context) {
> +			pr_err("%s: context is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		pr_debug("ACPI_HANDLE : %p\n", ACPI_HANDLE(&device->dev));
> +		if (!(context->parse)) {
> +			status = parse_guid();
> +			if (status != AE_OK)
> +				return status;
> +		}
> +		context->handle = ACPI_HANDLE(&device->dev);
> +		out = acpi_evaluate_dsm(context->handle, &context->guid, rev,
> +					COMMAND_ID_DEV_MODE, NULL);
> +		pr_debug("%s: acpi_evaluate_dsm completed %d\n", __func__, out->type);
> +		if (get_int_value(out, &value) == 0) {
> +			pr_debug("%s: Device Mode is : %d\n", __func__, value);
> +			context->sar_data.device_mode = value;
> +		} else {
> +			pr_err("%s: Cmd:%d Failed\n", __func__, COMMAND_ID_DEV_MODE);
> +			return AE_ERROR;
> +		}
> +		ACPI_FREE(out);
> +		if (!(context->data_read)) {

Do we really need to do this from the ACPI notify callback? Can't this just be done
at sar_add() time once instead ?

> +			for (reg = 0; reg < MAX_REGULATORY; reg++) {
> +				req.type = ACPI_TYPE_INTEGER;
> +				req.integer.value = reg;
> +				out = acpi_evaluate_dsm(context->handle, &context->guid, rev,
> +							COMMAND_ID_CONFIG_TABLE, &req);
> +				if (!out) {
> +					pr_err("%s: Cmd:%d Failed\n", __func__,
> +					       COMMAND_ID_CONFIG_TABLE);
> +					continue;
> +				}
> +				pr_debug("%s: acpi_evaluate_dsm  for regulatory %d completed %d\n",
> +					 __func__, reg, out->type);
> +				if (out->type == ACPI_TYPE_PACKAGE) {
> +					pr_debug("%s: ACPI_TYPE_PACKAGE, count: %d, type: %d\n",
> +						 __func__, out->package.count, out->package.type);
> +					item = &out->package.elements[0];
> +					if (get_int_value(item, &value) == 0) {
> +						pr_debug("%s: version : %d\n", __func__, value);
> +						context->config_data[reg].version = value;
> +					}
> +					item = &out->package.elements[1];
> +					if (get_int_value(item, &value) == 0) {
> +						pr_debug("%s: No of Modes: %d\n", __func__, value);
> +						context->config_data[reg].total_dev_mode = value;
> +					}
> +					if (context->config_data[reg].total_dev_mode <= 0 &&
> +					    context->config_data[reg].total_dev_mode >
> +					    MAX_DEV_MODES) {
> +						pr_err("total_dev_mode is not within range : %d\n",
> +						       context->config_data[reg].total_dev_mode);
> +						ACPI_FREE(out);
> +						return AE_ERROR;
> +					}
> +					item = &out->package.elements[2];
> +					if (item)
> +						status = parse_package(item);
> +					else
> +						status = AE_ERROR;
> +					if (status != AE_OK) {
> +						ACPI_FREE(out);
> +						return status;
> +					}
> +				}
> +				ACPI_FREE(out);
> +			}
> +			update_sar_data();
> +			context->data_read = true;
> +		}
> +		sar_send_notify();
> +		return status;
> +}
> +
> +#ifdef DEBUG
> +/**
> + * sar_set_device_mode - To set the device mode as BIOS handling test
> + * Test Function call to BIOS for device mode handling of data sent via
> + * DSM calls.
> + * @device: ACPI device for which to retrieve the data
> + * @mode: Device Mode to be set
> + */
> +static acpi_status sar_set_device_mode(struct platform_device *device, int mode)
> +{
> +		union acpi_object *out, req;
> +		u32 rev = 0;
> +		int value = 0;
> +		acpi_status status = AE_OK;
> +
> +		pr_debug("%s Triggered : mode : %d\n", __func__, mode);
> +		if (!device) {
> +			pr_err("%s: Device is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		if (!context->handle) {
> +			pr_err("%s: Handle is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		if (!(context->parse)) {
> +			status = parse_guid();
> +			if (status != AE_OK)
> +				return status;
> +		}
> +
> +		req.type = ACPI_TYPE_INTEGER;
> +		req.integer.value = mode;
> +		out = acpi_evaluate_dsm(context->handle, &context->guid,
> +					rev, COMMAND_TEST_SET, &req);
> +		if (get_int_value(out, &value) != 0) {
> +			pr_err("%s: Cmd:%d Failed\n", __func__, COMMAND_ID_DEV_MODE);
> +			return AE_ERROR;
> +		}
> +		pr_debug("%s: return value is : %d\n", __func__, value);
> +		ACPI_FREE(out);
> +		return status;
> +}
> +#endif
> +
> +static const struct acpi_device_id sar_device_ids[] = {
> +		{ "INTC1092", 0},
> +		{ "", 0},
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, sar_device_ids);
> +
> +static const struct platform_device_id sar_device_table[] = {
> +	{"intc1092", 0},
> +	{},
> +};
> +
> +static ssize_t sar_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +		unsigned long ret = 0;
> +
> +		pr_debug("%s triggered\n", __func__);
> +		if (!context) {
> +			pr_err("%s context is null", __func__);
> +			return -EFAULT;
> +		}
> +		ret = sprintf(buf, "%d %d %d %d\n", context->sar_data.device_mode,
> +			      context->sar_data.bandtable_index,
> +			      context->sar_data.antennatable_index,
> +			      context->sar_data.sartable_index);
> +		pr_debug("%s sent: %s\n", __func__, buf);
> +		return ret;
> +}
> +
> +static ssize_t sar_reg_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +		unsigned long ret = 0;
> +
> +		pr_debug("%s triggered\n", __func__);
> +		if (!context) {
> +			pr_err("%s context is null", __func__);
> +			return -EFAULT;
> +		}
> +		ret = sprintf(buf, "%d\n", context->reg_value);
> +		pr_debug("%s sent: %s\n", __func__, buf);
> +		return ret;
> +}
> +
> +static ssize_t sar_reg_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +		int value = 0, read = 0;
> +
> +		pr_debug("%s triggered\n", __func__);
> +		if (!count) {
> +			pr_err("%s count = %d", __func__, (int)count);
> +			return -EFAULT;
> +		}
> +		read = sscanf(buf, "%u", &value);
> +		pr_debug("%s received %s, integer value : %d", __func__, buf, value);
> +		if (read <= 0) {
> +			pr_alert("%s Not a integer", __func__);
> +			return -EFAULT;
> +		}
> +		if (value >= 0 && value < MAX_REGULATORY) {
> +			context->reg_value = value;
> +			update_sar_data();
> +			sar_send_notify();
> +		}
> +		return count;
> +}
> +
> +static struct kobj_attribute sar_attribute = __ATTR(intc_data, 0660, sar_show, NULL);
> +static struct kobj_attribute sar_attribute1 = __ATTR(intc_reg, 0660, sar_reg_show, sar_reg_store);
> +
> +#ifdef DEBUG
> +static ssize_t sar_dev_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +		unsigned long ret = 0;
> +
> +		pr_debug("%s triggered\n", __func__);
> +		if (!context) {
> +			pr_err("%s context is null\n", __func__);
> +			return -EFAULT;
> +		}
> +		ret = sprintf(buf, "%d\n", context->sar_data.device_mode);
> +		pr_debug("%s sent: %s\n", __func__, buf);
> +		return ret;
> +}
> +
> +static ssize_t sar_dev_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +		int value = 0, read = 0;
> +		acpi_status status = AE_OK;
> +
> +		pr_debug("%s triggered\n", __func__);
> +		if (!count) {
> +			pr_err("%s count = %d", __func__, (int)count);
> +			return -EFAULT;
> +		}
> +		read = sscanf(buf, "%u", &value);
> +		pr_debug("%s received %s, integer value : %d", __func__, buf, value);
> +		if (read <= 0) {
> +			pr_alert("%s Not a integer", __func__);
> +			return -EFAULT;
> +		}
> +		if (value < MAX_DEV_MODES) {
> +			status = sar_set_device_mode(context->sar_device, value);
> +			if (status != AE_OK) {
> +				pr_err("sar_set_device_mode failed\n");
> +				return -EINVAL;
> +			}
> +		}
> +		return count;
> +}
> +
> +static struct kobj_attribute sar_attribute2 = __ATTR(intc_dev, 0660, sar_dev_show, sar_dev_store);

You cannot add/remove sysfs attributes based on #define DEBUG, these will become part
of your driver / userspace API and you cannot change that based on #ifdef DEBUG.
Instead please use debugfs for this, or maybe just completely drop it? 

> +#endif
> +
> +MODULE_DEVICE_TABLE(platform, sar_device_table);
> +
> +static struct platform_driver sar_driver = {
> +	.probe = sar_add,
> +	.remove = sar_remove,
> +	.shutdown = sar_shutdown,
> +	.driver = {
> +			.name = DRVNAME,
> +			.owner = THIS_MODULE,
> +			/* FOR ACPI HANDLING */
> +			.acpi_match_table = ACPI_PTR(sar_device_ids),
> +			},
> +	.id_table = sar_device_table,
> +};
> +
> +static int sar_add(struct platform_device *device)
> +{
> +		int result = 0;
> +
> +		pr_debug("%s Triggered\n", __func__);
> +		context = kmalloc(sizeof(*context), GFP_KERNEL);
> +		if (!context) {
> +			pr_err("Cannot allocate memory in kernel for WWAN_SAR_CONTEXT\n");
> +			return -1;
> +		}
> +		memset(context, 0, sizeof(struct WWAN_SAR_CONTEXT));
> +
> +		context->sar_kobject = kobject_create_and_add("intc_sar_object", kernel_kobj);
> +		if (!context->sar_kobject) {
> +			pr_err("Failed to create sar_kobject\n");
> +			goto r_free;
> +		}
> +
> +		result = sysfs_create_file(context->sar_kobject, &sar_attribute.attr);
> +		if (result) {
> +			pr_err("Failed to create the intc_data file in /sys/kernel/intc_sar_object\n");
> +			goto r_sys;
> +		}
> +
> +		result = sysfs_create_file(context->sar_kobject, &sar_attribute1.attr);
> +		if (result) {
> +			pr_err("Failed to create the intc_reg file in /sys/kernel/intc_sar_object\n");
> +			goto r_sys;
> +		}
> +#ifdef DEBUG
> +		result = sysfs_create_file(context->sar_kobject, &sar_attribute2.attr);
> +		if (result) {
> +			pr_err("Failed to create the intc_dev file in /sys/kernel/intc_sar_object\n");
> +			goto r_sys;
> +		}
> +#endif
> +		context->sar_device = device;
> +		if (sar_module_probe(device) != AE_OK) {
> +			pr_err("Failed sar_module_probe\n");
> +			goto r_sys;
> +		}
> +
> +		if (acpi_install_notify_handler(ACPI_HANDLE(&device->dev), ACPI_DEVICE_NOTIFY,
> +						sar_notify, (void *)device) != AE_OK) {
> +			pr_err("Failed acpi_install_notify_handler\n");
> +			goto r_sys;
> +		}
> +		return 0;
> +
> +r_sys:
> +		kobject_put(context->sar_kobject);
> +r_free:
> +		kfree(context);
> +		return -1;
> +}
> +
> +static int sar_remove(struct platform_device *device)
> +{
> +		pr_debug("%s Triggered\n", __func__);
> +		acpi_remove_notify_handler(ACPI_HANDLE(&device->dev),
> +					   ACPI_DEVICE_NOTIFY, sar_notify);
> +		kobject_put(context->sar_kobject);
> +		clear_sar_dev_mode();
> +		kfree(context);
> +		return 0;
> +}
> +
> +static void sar_shutdown(struct platform_device *device)
> +{
> +		sar_remove(device);
> +		return;
> +}
> +
> +static void sar_notify(acpi_handle handle, u32 event, void *data)
> +{
> +		struct platform_device *device = data;
> +
> +		pr_debug("%s Triggered: event: %d\n", __func__, event);
> +		if (event == SAR_EVENT) {
> +			pr_debug("%s event matched\n", __func__);
> +			if (sar_module_probe(device) != AE_OK)
> +				pr_err("sar_module_probe error");
> +		}
> +}
> +
> +static int sar_init(void)
> +{
> +		int result = 0;
> +
> +		pr_alert("SAR Init Triggered\n");
> +		result = platform_driver_register(&sar_driver);
> +		if (result < 0)
> +			pr_err("Error registering driver\n");
> +		return result;
> +}
> +
> +static void sar_exit(void)
> +{
> +		pr_alert("SAR EXIT Triggered\n");
> +		platform_driver_unregister(&sar_driver);
> +		pr_debug("Kernel Module Removed Successfully.\n");
> +}
> +
> +module_init(sar_init);
> +module_exit(sar_exit);
> diff --git a/include/linux/platform_data/x86/intel-sar.h b/include/linux/platform_data/x86/intel-sar.h
> new file mode 100644
> index 000000000000..9ed653284fa5
> --- /dev/null
> +++ b/include/linux/platform_data/x86/intel-sar.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Intel Corporation Header File for Specific Absorption Rate
> + * Copyright (c) 2021, Intel Corporation.
> + */
> +#ifndef INTEL_SAR_H
> +#define INTEL_SAR_H
> +
> +#define DRVNAME "sar"
> +#define SAR_DSM_UUID "82737E72-3A33-4C45-A9C7-57C0411A5F13"
> +#define COMMAND_ID_DEV_MODE 1
> +#define COMMAND_ID_CONFIG_TABLE 2
> +#define COMMAND_TEST_SET 31
> +#define MAX_REGULATORY 3
> +#define SAR_EVENT 0x80
> +#define MAX_DEV_MODES 50
> +
> +/**
> + * Structure WWAN_DEVICE_MODE_INFO - device mode information
> + * Holds the data that needs to be passed to userspace.
> + * The data is updated from the BIOS sensor information.
> + * @device_mode: Specific mode of the device
> + * @bandtable_index: Index of RF band
> + * @antennatable_index: Index of antenna
> + * @sartable_index: Index of SAR
> + */
> +struct WWAN_DEVICE_MODE_INFO {
> +		int device_mode;
> +		int bandtable_index;
> +		int antennatable_index;
> +		int sartable_index;
> +};
> +
> +/**
> + * Structure WWAN_DEVICE_MODE_CONFIGURATION - device configuration
> + * Holds the data that is configured and obtained on probe event.
> + * The data is updated from the BIOS sensor information.
> + * @version: Mode configuration version
> + * @total_dev_mode: Total number of device modes
> + * @device_mode_info: pointer to structure WWAN_DEVICE_MODE_INFO
> + */
> +struct WWAN_DEVICE_MODE_CONFIGURATION {
> +		int version;
> +		int total_dev_mode;
> +		struct WWAN_DEVICE_MODE_INFO *device_mode_info;
> +};
> +
> +/**
> + * Structure WWAN_SUPPORTED_INFO - userspace datastore
> + * Holds the data that is obtained from userspace
> + * The data is updated from the userspace and send value back in the
> + * structure format that is mentioned here.
> + * @reg_mode_needed: regulatory mode set by user for tests
> + * @bios_table_revision: Version of SAR table
> + * @num_supported_modes: Total supported modes based on reg_mode
> + */
> +struct WWAN_SUPPORTED_INFO {
> +		int reg_mode_needed;
> +		int bios_table_revision;
> +		int num_supported_modes;
> +};
> +
> +/**
> + * Structure WWAN_SAR_CONTEXT - context of SAR
> + * Holds the complete context as long as the driver is in existence
> + * The context holds instance of the data used for different cases.
> + * @parse: identifies if dsm is parsed
> + * @data_read: identifies if data is already read from BIOS
> + * @guid: Group id
> + * @handle: store acpi handle
> + * @reg_value: regulatory value
> + * Regulatory 0: FCC, 1: CE, 2: ISED
> + * @sar_device: platform_device type
> + * @sar_kobject: kobject for sysfs
> + * @supported_data: WWAN_SUPPORTED_INFO struct
> + * @sar_data: WWAN_DEVICE_MODE_INFO struct
> + * @config_data: WWAN_DEVICE_MODE_CONFIGURATION array struct
> + */
> +struct WWAN_SAR_CONTEXT {
> +		bool parse;
> +		bool data_read;
> +		guid_t guid;
> +		acpi_handle handle;
> +		int reg_value;
> +		struct platform_device *sar_device;
> +		struct kobject *sar_kobject;
> +		struct WWAN_SUPPORTED_INFO supported_data;
> +		struct WWAN_DEVICE_MODE_INFO sar_data;
> +		struct WWAN_DEVICE_MODE_CONFIGURATION config_data[MAX_REGULATORY];
> +};
> +#endif /* INTEL_SAR_H */
> 


Regards,

Hans


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem
  2021-07-15 16:20   ` Hans de Goede
@ 2021-07-27 14:38     ` Shravan, S
  0 siblings, 0 replies; 10+ messages in thread
From: Shravan, S @ 2021-07-27 14:38 UTC (permalink / raw)
  To: Hans de Goede, mgross, platform-driver-x86; +Cc: An, Sudhakar

Hi Hans,

Thank you for your feedback.
I have taken care of all the feedback provided on my patch for SAR driver on Intel Modem (Version 2) from all the reviewers and submitted a new patch (Version 3) on 16th July 2021.
There is no feedbacks from any reviewers on this latest patch-set V3.
Do let me know on your thoughts on this new patch-set V3 and the next steps that I should take.

Regards,
Shravan S 

-----Original Message-----
From: Hans de Goede <hdegoede@redhat.com> 
Sent: Thursday, July 15, 2021 9:50 PM
To: Shravan, S <s.shravan@intel.com>; mgross@linux.intel.com; platform-driver-x86@vger.kernel.org
Cc: An, Sudhakar <sudhakar.an@intel.com>
Subject: Re: [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem

Hi Shravan,

Here is a first set of review remarks, note this is just from a very quick glance, so these are just some initial remarks.
I will do a more thorough review after you post a v3 addressing these initial remarks.

First of all the subject of the patch (first line of the commit msg) should be:

"platform/x86: Add BIOS Dynamic SAR driver for Intel M.2 Modem"

And then after that you should have a proper commit msg describing what this driver is and why it is necessary.

And you commit msg must end with this line:

Signed-off-by: Shravan S <s.shravan@intel.com>

See: https://elinux.org/Developer_Certificate_Of_Origin

On 5/10/21 9:40 AM, Shravan S wrote:
> V2 :
> * Changes to the remove netlink and introduce sysfs to communicate to 
> Userspace via notify
> * Handled review comments by changing ioctl calls to sysfs
> * "sar" name change for platform_device_id to "intc1092"
> * sar_init and sar_exit is modified as per review to minimal 
> initialization
> * Inclusion of debug only enabling of device mode change parameters

Having a changelog is good, but you need to put it below the
"Signed-off-by: Shravan S <s.shravan@intel.com>" line like this:

Signed-off-by: Shravan S <s.shravan@intel.com
---
V2 :
* Changes to the remove netlink and introduce ...


> ---
>  drivers/platform/x86/Kconfig                |  12 +
>  drivers/platform/x86/Makefile               |   1 +
>  drivers/platform/x86/intel-sar.c            | 550 ++++++++++++++++++++
>  include/linux/platform_data/x86/intel-sar.h |  91 ++++
>  4 files changed, 654 insertions(+)
>  create mode 100644 drivers/platform/x86/intel-sar.c  create mode 
> 100644 include/linux/platform_data/x86/intel-sar.h
> 
> diff --git a/drivers/platform/x86/Kconfig 
> b/drivers/platform/x86/Kconfig index 60592fb88e7a..6dfa89310677 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1313,6 +1313,18 @@ config INTEL_TELEMETRY
>  	  directly via debugfs files. Various tools may use
>  	  this interface for SoC state monitoring.
>  
> +config INTEL_SAR
> +	tristate "Intel Specific Absorption Rate Driver"
> +	depends on ACPI
> +	help
> +	  This driver limit the exposure of human body to RF frequency by informing
> +	  the Intel M.2 modem to regulate the RF power based on SAR data obtained
> +	  from the sensorscaptured in the BIOS. ACPI interface exposes this 
> +data

missing space between sensors and captured.
> +	  from the BIOS to SAR driver. The front end application in userspace
> +	  will interact with SAR driver to obtain information like the device mode,
> +	  Antenna index,baseband index, SAR table index and use available 
> +communication

missing space after the first comma (',')

> +	  like MBIM interface to enable data communication to modem for RF power regulation.

Maybe this would be a good start of the commit msg?


> +
>  endif # X86_PLATFORM_DEVICES
>  
>  config PMC_ATOM
> diff --git a/drivers/platform/x86/Makefile 
> b/drivers/platform/x86/Makefile index dcc8cdb95b4d..548ff663c4af 
> 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -123,6 +123,7 @@ obj-$(CONFIG_INTEL_SMARTCONNECT)		+= intel-smartconnect.o
>  obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE)	+= intel_speed_select_if/
>  obj-$(CONFIG_INTEL_TURBO_MAX_3)			+= intel_turbo_max_3.o
>  obj-$(CONFIG_INTEL_UNCORE_FREQ_CONTROL)		+= intel-uncore-frequency.o
> +obj-$(CONFIG_INTEL_SAR)				+= intel-sar.o
>  
>  # Intel PMIC / PMC / P-Unit devices
>  obj-$(CONFIG_INTEL_BXTWC_PMIC_TMU)	+= intel_bxtwc_tmu.o
> diff --git a/drivers/platform/x86/intel-sar.c 
> b/drivers/platform/x86/intel-sar.c
> new file mode 100644
> index 000000000000..fa16ae59851b
> --- /dev/null
> +++ b/drivers/platform/x86/intel-sar.c
> @@ -0,0 +1,550 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Intel Corporation - ACPI for Specific Absorption Rate
> + * Copyright (c) 2021, Intel Corporation.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
> +#include <uapi/linux/errno.h>

You should never use uapi headers in kernel code

> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
> +#include <linux/platform_data/x86/intel-sar.h>

please sort all the includes alphabetically.

> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Platform device driver for INTEL MODEM BIOS 
> +SAR"); MODULE_AUTHOR("Shravan S <s.shravan@intel.com>");

PLease move these 3 MODULE_... lines to the end of the file.

> +
> +static struct WWAN_SAR_CONTEXT *context;

You are binding to a platform_dev, so no global context pointer please. Instead please use platform_set_drvdata() /
platform_get_drvdata() and the option to pass a data pointer to other callbacks.


> +static int sar_add(struct platform_device *device); static int 
> +sar_remove(struct platform_device *device); static void 
> +sar_shutdown(struct platform_device *device); static void 
> +sar_notify(acpi_handle handle, u32 event, void *data);

Please move the definition of these functions to above their first use and drop these forward declarations.

> +
> +/**
> + * sar_send_notify - Send SAR data notification to userspace
> + * Send the sar data notification to the userspace and later sent to 
> +modem
> + * by the userspace application. The data sent is collected from the BIOS.
> + * This data is used by userspace for further handling of the modem output.
> + */
> +static void sar_send_notify(void)
> +{


You are using 2 tabs instead of 1 as indentation level here and everywhere else, please fix this everywhere.

> +		if (!context) {
> +			pr_alert("%s: context is null\n", __func__);
> +			return;
> +		}

context should be passed to this function (see above) and then this check can go away.

> +		pr_debug("%s: %d, %d, %d, %d\n", __func__, context->sar_data.device_mode,
> +			 context->sar_data.bandtable_index, context->sar_data.antennatable_index,
> +			 context->sar_data.sartable_index);

Use dev_dbg() please, also you have a lot of debugging prints in this driver, are all these really necessary? Please use less debugging prints if possible.

> +		sysfs_notify(context->sar_kobject, NULL, "intc_data");

So after removing the useless context check + the over-zealous debuging this function is reduced to just this one line, please drop this function and instead directly write this one line.

> +}
> +
> +/**
> + * clear_sar_dev_mode - Clear Device Mode by freeing the allocated 
> +data
> + * If the Device Mode Info is present and allocated, clear it. This 
> +is for
> + * dynamic allocated memory of device_mode_info  */ static void 
> +clear_sar_dev_mode(void) {
> +		int reg = 0;
> +
> +		for (reg = 0; reg < MAX_REGULATORY; reg++) {
> +			kfree(context->config_data[reg].device_mode_info);
> +			context->config_data[reg].device_mode_info = NULL;
> +		}
> +}

Please just move this code into sar_remove(), and drop this function.

> +
> +/**
> + * get_int_value - Retrieve Integer values from ACPI Object
> + * Value of the integer from the object of ACPI is obtained.
> + * @obj: acpi_object pointer which gets the integer value
> + * @out: output pointer for integer
> + * returns 0 on success
> + */
> +static int get_int_value(union acpi_object *obj, int *out) {
> +		if (obj && obj->type == ACPI_TYPE_INTEGER) {
> +			*out = (int)obj->integer.value;
> +			return 0;
> +		} else {
> +			return -1;

Please use -ESOME_ERRORCODE here.

> +		}
> +}
> +
> +/**
> + * update_sar_data - sar data is updated based on reg_value
> + * sar_data is updated based on regulatory value  */ static void 
> +update_sar_data(void) {
> +		pr_debug("%s: Update SAR data\n", __func__);
> +		if (context->config_data[context->reg_value].device_mode_info &&
> +		    context->sar_data.device_mode <=
> +		    context->config_data[context->reg_value].total_dev_mode) {

Please use:

	struct WWAN_DEVICE_MODE_INFO *info;
	int idx = context->reg_value;

	/* FIXME what if idx > the length of the config_data array? Add check here */

	info = context->config_data[idx].device_mode_info;

	if (!info || context->sar_data.device_mode > context->config_data[idx].total_dev_mode) {
		dev_err(..., "sar data not assigned! ...);
		return;
	}

	// Do all the assigns here using the new idx helper and put one assignment
	// per line, the split up assingment code below is simply not readable at all.



> +			context->sar_data.antennatable_index =
> +			context->config_data[context->reg_value]
> +			.device_mode_info[context->sar_data.device_mode].antennatable_index;
> +			context->sar_data.bandtable_index =
> +			context->config_data[context->reg_value]
> +			.device_mode_info[context->sar_data.device_mode].bandtable_index;
> +			context->sar_data.sartable_index =
> +			context->config_data[context->reg_value]
> +			.device_mode_info[context->sar_data.device_mode].sartable_index;
> +			pr_debug("bandtable_index: %d\n", context->sar_data.bandtable_index);
> +			pr_debug("antennatable_index : %d\n", context->sar_data.antennatable_index);
> +			pr_debug("sartable_index: %d\n", 
> +context->sar_data.sartable_index);

Is this debugging really necessary?

> +		} else {
> +			pr_err("%s: sar data not assigned! Dev mode: %d, total_dev_mode: %d\n",
> +			       __func__, context->sar_data.device_mode,
> +			       context->config_data[context->reg_value].total_dev_mode);
> +		}
> +}
> +
> +/**
> + * parse_guid - parse guid based on DSM UUID
> + * returns if success or error
> + */
> +static acpi_status parse_guid(void)
> +{
> +		if (guid_parse(SAR_DSM_UUID, &context->guid)) {
> +			pr_err("%s: UUID error\n", __func__);
> +			return AE_ERROR;
> +		}
> +		context->parse = true;
> +		return AE_OK;
> +}

Please drop this weird helper and just do the parse once from
sar_add() and fail the probe if the parse fails, you can then also drop the parse boolean value from the context.


> +
> +/**
> + * parse_package - parse package for SAR
> + * @item : acpi_object ptr
> + * returns if success or error
> + */
> +static acpi_status parse_package(union acpi_object *item) {
> +		int value = 0, itr = 0, reg = 0;
> +		union acpi_object *num;
> +		struct WWAN_DEVICE_MODE_CONFIGURATION *data;
> +
> +		num = &item->package.elements[0];
> +		if (get_int_value(num, &value) == 0) {
> +			pr_debug("%s: Regulatory value : %d\n", __func__, value);
> +			if (value >= 0 && value < MAX_REGULATORY)
> +				reg = value;
> +			else
> +				return AE_ERROR;
> +		}
> +		data = &context->config_data[reg];
> +		if (data->total_dev_mode == 0) {
> +			pr_debug("Dev Mode count is zero, return\n");
> +			return AE_OK;
> +		}
> +		data->device_mode_info =
> +		kmalloc_array(data->total_dev_mode,
> +			      sizeof(struct WWAN_DEVICE_MODE_INFO), GFP_KERNEL);
> +		if (!data->device_mode_info) {
> +			pr_err("Cannot allocate memory in kernel\n");
> +			return AE_ERROR;
> +		}
> +		for (itr = 0; itr < data->total_dev_mode; itr++) {
> +			if (itr + 1 == item->package.count)
> +				break;
> +			num = &item->package.elements[itr + 1];
> +			if (num->type != ACPI_TYPE_PACKAGE)
> +				continue;
> +			if (get_int_value(&num->package.elements[0], &value) == 0) {
> +				pr_debug("%s: Device Mode for mode %d: %d\n", __func__,
> +					 itr, value);
> +				data->device_mode_info[itr].device_mode = value;
> +			}
> +			if (get_int_value(&num->package.elements[1], &value) == 0) {
> +				pr_debug("%s: band_index mode %d: %d\n", __func__,
> +					 itr, value);
> +				data->device_mode_info[itr].bandtable_index = value;
> +			}
> +			if (get_int_value(&num->package.elements[2], &value) == 0) {
> +				pr_debug("%s: antenna_index mode %d: %d\n", __func__,
> +					 itr, value);
> +				data->device_mode_info[itr].antennatable_index = value;
> +			}
> +			if (get_int_value(&num->package.elements[3], &value) == 0) {
> +				pr_debug("%s: sar_index for mode %d: %d\n", __func__,
> +					 itr, value);
> +				data->device_mode_info[itr].sartable_index = value;
> +			}
> +		}
> +		return AE_OK;
> +}
> +
> +/**
> + * sar_module_probe - Extraction of information from BIOS via DSM 
> +calls
> + * Retrieve all values related to device mode and SAR Table index,
> + * Antenna Table index, Band Table index
> + * @device: ACPI device for which to retrieve the data
> + * Returns AE_OK on success
> + */
> +static acpi_status sar_module_probe(struct platform_device *device) {
> +		acpi_status status = AE_OK;
> +		union acpi_object *out, *item, req;
> +		u32 rev = 0, reg = 0;
> +		int value = 0;
> +
> +		pr_alert("%s Triggered\n", __func__);
> +		if (!device) {
> +			pr_err("%s: platform driver is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		if (!context) {
> +			pr_err("%s: context is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		pr_debug("ACPI_HANDLE : %p\n", ACPI_HANDLE(&device->dev));
> +		if (!(context->parse)) {
> +			status = parse_guid();
> +			if (status != AE_OK)
> +				return status;
> +		}
> +		context->handle = ACPI_HANDLE(&device->dev);
> +		out = acpi_evaluate_dsm(context->handle, &context->guid, rev,
> +					COMMAND_ID_DEV_MODE, NULL);
> +		pr_debug("%s: acpi_evaluate_dsm completed %d\n", __func__, out->type);
> +		if (get_int_value(out, &value) == 0) {
> +			pr_debug("%s: Device Mode is : %d\n", __func__, value);
> +			context->sar_data.device_mode = value;
> +		} else {
> +			pr_err("%s: Cmd:%d Failed\n", __func__, COMMAND_ID_DEV_MODE);
> +			return AE_ERROR;
> +		}
> +		ACPI_FREE(out);
> +		if (!(context->data_read)) {

Do we really need to do this from the ACPI notify callback? Can't this just be done at sar_add() time once instead ?

> +			for (reg = 0; reg < MAX_REGULATORY; reg++) {
> +				req.type = ACPI_TYPE_INTEGER;
> +				req.integer.value = reg;
> +				out = acpi_evaluate_dsm(context->handle, &context->guid, rev,
> +							COMMAND_ID_CONFIG_TABLE, &req);
> +				if (!out) {
> +					pr_err("%s: Cmd:%d Failed\n", __func__,
> +					       COMMAND_ID_CONFIG_TABLE);
> +					continue;
> +				}
> +				pr_debug("%s: acpi_evaluate_dsm  for regulatory %d completed %d\n",
> +					 __func__, reg, out->type);
> +				if (out->type == ACPI_TYPE_PACKAGE) {
> +					pr_debug("%s: ACPI_TYPE_PACKAGE, count: %d, type: %d\n",
> +						 __func__, out->package.count, out->package.type);
> +					item = &out->package.elements[0];
> +					if (get_int_value(item, &value) == 0) {
> +						pr_debug("%s: version : %d\n", __func__, value);
> +						context->config_data[reg].version = value;
> +					}
> +					item = &out->package.elements[1];
> +					if (get_int_value(item, &value) == 0) {
> +						pr_debug("%s: No of Modes: %d\n", __func__, value);
> +						context->config_data[reg].total_dev_mode = value;
> +					}
> +					if (context->config_data[reg].total_dev_mode <= 0 &&
> +					    context->config_data[reg].total_dev_mode >
> +					    MAX_DEV_MODES) {
> +						pr_err("total_dev_mode is not within range : %d\n",
> +						       context->config_data[reg].total_dev_mode);
> +						ACPI_FREE(out);
> +						return AE_ERROR;
> +					}
> +					item = &out->package.elements[2];
> +					if (item)
> +						status = parse_package(item);
> +					else
> +						status = AE_ERROR;
> +					if (status != AE_OK) {
> +						ACPI_FREE(out);
> +						return status;
> +					}
> +				}
> +				ACPI_FREE(out);
> +			}
> +			update_sar_data();
> +			context->data_read = true;
> +		}
> +		sar_send_notify();
> +		return status;
> +}
> +
> +#ifdef DEBUG
> +/**
> + * sar_set_device_mode - To set the device mode as BIOS handling test
> + * Test Function call to BIOS for device mode handling of data sent 
> +via
> + * DSM calls.
> + * @device: ACPI device for which to retrieve the data
> + * @mode: Device Mode to be set
> + */
> +static acpi_status sar_set_device_mode(struct platform_device 
> +*device, int mode) {
> +		union acpi_object *out, req;
> +		u32 rev = 0;
> +		int value = 0;
> +		acpi_status status = AE_OK;
> +
> +		pr_debug("%s Triggered : mode : %d\n", __func__, mode);
> +		if (!device) {
> +			pr_err("%s: Device is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		if (!context->handle) {
> +			pr_err("%s: Handle is null\n", __func__);
> +			return AE_ERROR;
> +		}
> +		if (!(context->parse)) {
> +			status = parse_guid();
> +			if (status != AE_OK)
> +				return status;
> +		}
> +
> +		req.type = ACPI_TYPE_INTEGER;
> +		req.integer.value = mode;
> +		out = acpi_evaluate_dsm(context->handle, &context->guid,
> +					rev, COMMAND_TEST_SET, &req);
> +		if (get_int_value(out, &value) != 0) {
> +			pr_err("%s: Cmd:%d Failed\n", __func__, COMMAND_ID_DEV_MODE);
> +			return AE_ERROR;
> +		}
> +		pr_debug("%s: return value is : %d\n", __func__, value);
> +		ACPI_FREE(out);
> +		return status;
> +}
> +#endif
> +
> +static const struct acpi_device_id sar_device_ids[] = {
> +		{ "INTC1092", 0},
> +		{ "", 0},
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, sar_device_ids);
> +
> +static const struct platform_device_id sar_device_table[] = {
> +	{"intc1092", 0},
> +	{},
> +};
> +
> +static ssize_t sar_show(struct kobject *kobj, struct kobj_attribute 
> +*attr, char *buf) {
> +		unsigned long ret = 0;
> +
> +		pr_debug("%s triggered\n", __func__);
> +		if (!context) {
> +			pr_err("%s context is null", __func__);
> +			return -EFAULT;
> +		}
> +		ret = sprintf(buf, "%d %d %d %d\n", context->sar_data.device_mode,
> +			      context->sar_data.bandtable_index,
> +			      context->sar_data.antennatable_index,
> +			      context->sar_data.sartable_index);
> +		pr_debug("%s sent: %s\n", __func__, buf);
> +		return ret;
> +}
> +
> +static ssize_t sar_reg_show(struct kobject *kobj, struct 
> +kobj_attribute *attr, char *buf) {
> +		unsigned long ret = 0;
> +
> +		pr_debug("%s triggered\n", __func__);
> +		if (!context) {
> +			pr_err("%s context is null", __func__);
> +			return -EFAULT;
> +		}
> +		ret = sprintf(buf, "%d\n", context->reg_value);
> +		pr_debug("%s sent: %s\n", __func__, buf);
> +		return ret;
> +}
> +
> +static ssize_t sar_reg_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			     const char *buf, size_t count) {
> +		int value = 0, read = 0;
> +
> +		pr_debug("%s triggered\n", __func__);
> +		if (!count) {
> +			pr_err("%s count = %d", __func__, (int)count);
> +			return -EFAULT;
> +		}
> +		read = sscanf(buf, "%u", &value);
> +		pr_debug("%s received %s, integer value : %d", __func__, buf, value);
> +		if (read <= 0) {
> +			pr_alert("%s Not a integer", __func__);
> +			return -EFAULT;
> +		}
> +		if (value >= 0 && value < MAX_REGULATORY) {
> +			context->reg_value = value;
> +			update_sar_data();
> +			sar_send_notify();
> +		}
> +		return count;
> +}
> +
> +static struct kobj_attribute sar_attribute = __ATTR(intc_data, 0660, 
> +sar_show, NULL); static struct kobj_attribute sar_attribute1 = 
> +__ATTR(intc_reg, 0660, sar_reg_show, sar_reg_store);
> +
> +#ifdef DEBUG
> +static ssize_t sar_dev_show(struct kobject *kobj, struct 
> +kobj_attribute *attr, char *buf) {
> +		unsigned long ret = 0;
> +
> +		pr_debug("%s triggered\n", __func__);
> +		if (!context) {
> +			pr_err("%s context is null\n", __func__);
> +			return -EFAULT;
> +		}
> +		ret = sprintf(buf, "%d\n", context->sar_data.device_mode);
> +		pr_debug("%s sent: %s\n", __func__, buf);
> +		return ret;
> +}
> +
> +static ssize_t sar_dev_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			     const char *buf, size_t count) {
> +		int value = 0, read = 0;
> +		acpi_status status = AE_OK;
> +
> +		pr_debug("%s triggered\n", __func__);
> +		if (!count) {
> +			pr_err("%s count = %d", __func__, (int)count);
> +			return -EFAULT;
> +		}
> +		read = sscanf(buf, "%u", &value);
> +		pr_debug("%s received %s, integer value : %d", __func__, buf, value);
> +		if (read <= 0) {
> +			pr_alert("%s Not a integer", __func__);
> +			return -EFAULT;
> +		}
> +		if (value < MAX_DEV_MODES) {
> +			status = sar_set_device_mode(context->sar_device, value);
> +			if (status != AE_OK) {
> +				pr_err("sar_set_device_mode failed\n");
> +				return -EINVAL;
> +			}
> +		}
> +		return count;
> +}
> +
> +static struct kobj_attribute sar_attribute2 = __ATTR(intc_dev, 0660, 
> +sar_dev_show, sar_dev_store);

You cannot add/remove sysfs attributes based on #define DEBUG, these will become part of your driver / userspace API and you cannot change that based on #ifdef DEBUG.
Instead please use debugfs for this, or maybe just completely drop it? 

> +#endif
> +
> +MODULE_DEVICE_TABLE(platform, sar_device_table);
> +
> +static struct platform_driver sar_driver = {
> +	.probe = sar_add,
> +	.remove = sar_remove,
> +	.shutdown = sar_shutdown,
> +	.driver = {
> +			.name = DRVNAME,
> +			.owner = THIS_MODULE,
> +			/* FOR ACPI HANDLING */
> +			.acpi_match_table = ACPI_PTR(sar_device_ids),
> +			},
> +	.id_table = sar_device_table,
> +};
> +
> +static int sar_add(struct platform_device *device) {
> +		int result = 0;
> +
> +		pr_debug("%s Triggered\n", __func__);
> +		context = kmalloc(sizeof(*context), GFP_KERNEL);
> +		if (!context) {
> +			pr_err("Cannot allocate memory in kernel for WWAN_SAR_CONTEXT\n");
> +			return -1;
> +		}
> +		memset(context, 0, sizeof(struct WWAN_SAR_CONTEXT));
> +
> +		context->sar_kobject = kobject_create_and_add("intc_sar_object", kernel_kobj);
> +		if (!context->sar_kobject) {
> +			pr_err("Failed to create sar_kobject\n");
> +			goto r_free;
> +		}
> +
> +		result = sysfs_create_file(context->sar_kobject, &sar_attribute.attr);
> +		if (result) {
> +			pr_err("Failed to create the intc_data file in /sys/kernel/intc_sar_object\n");
> +			goto r_sys;
> +		}
> +
> +		result = sysfs_create_file(context->sar_kobject, &sar_attribute1.attr);
> +		if (result) {
> +			pr_err("Failed to create the intc_reg file in /sys/kernel/intc_sar_object\n");
> +			goto r_sys;
> +		}
> +#ifdef DEBUG
> +		result = sysfs_create_file(context->sar_kobject, &sar_attribute2.attr);
> +		if (result) {
> +			pr_err("Failed to create the intc_dev file in /sys/kernel/intc_sar_object\n");
> +			goto r_sys;
> +		}
> +#endif
> +		context->sar_device = device;
> +		if (sar_module_probe(device) != AE_OK) {
> +			pr_err("Failed sar_module_probe\n");
> +			goto r_sys;
> +		}
> +
> +		if (acpi_install_notify_handler(ACPI_HANDLE(&device->dev), ACPI_DEVICE_NOTIFY,
> +						sar_notify, (void *)device) != AE_OK) {
> +			pr_err("Failed acpi_install_notify_handler\n");
> +			goto r_sys;
> +		}
> +		return 0;
> +
> +r_sys:
> +		kobject_put(context->sar_kobject);
> +r_free:
> +		kfree(context);
> +		return -1;
> +}
> +
> +static int sar_remove(struct platform_device *device) {
> +		pr_debug("%s Triggered\n", __func__);
> +		acpi_remove_notify_handler(ACPI_HANDLE(&device->dev),
> +					   ACPI_DEVICE_NOTIFY, sar_notify);
> +		kobject_put(context->sar_kobject);
> +		clear_sar_dev_mode();
> +		kfree(context);
> +		return 0;
> +}
> +
> +static void sar_shutdown(struct platform_device *device) {
> +		sar_remove(device);
> +		return;
> +}
> +
> +static void sar_notify(acpi_handle handle, u32 event, void *data) {
> +		struct platform_device *device = data;
> +
> +		pr_debug("%s Triggered: event: %d\n", __func__, event);
> +		if (event == SAR_EVENT) {
> +			pr_debug("%s event matched\n", __func__);
> +			if (sar_module_probe(device) != AE_OK)
> +				pr_err("sar_module_probe error");
> +		}
> +}
> +
> +static int sar_init(void)
> +{
> +		int result = 0;
> +
> +		pr_alert("SAR Init Triggered\n");
> +		result = platform_driver_register(&sar_driver);
> +		if (result < 0)
> +			pr_err("Error registering driver\n");
> +		return result;
> +}
> +
> +static void sar_exit(void)
> +{
> +		pr_alert("SAR EXIT Triggered\n");
> +		platform_driver_unregister(&sar_driver);
> +		pr_debug("Kernel Module Removed Successfully.\n"); }
> +
> +module_init(sar_init);
> +module_exit(sar_exit);
> diff --git a/include/linux/platform_data/x86/intel-sar.h 
> b/include/linux/platform_data/x86/intel-sar.h
> new file mode 100644
> index 000000000000..9ed653284fa5
> --- /dev/null
> +++ b/include/linux/platform_data/x86/intel-sar.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Intel Corporation Header File for Specific Absorption Rate
> + * Copyright (c) 2021, Intel Corporation.
> + */
> +#ifndef INTEL_SAR_H
> +#define INTEL_SAR_H
> +
> +#define DRVNAME "sar"
> +#define SAR_DSM_UUID "82737E72-3A33-4C45-A9C7-57C0411A5F13"
> +#define COMMAND_ID_DEV_MODE 1
> +#define COMMAND_ID_CONFIG_TABLE 2
> +#define COMMAND_TEST_SET 31
> +#define MAX_REGULATORY 3
> +#define SAR_EVENT 0x80
> +#define MAX_DEV_MODES 50
> +
> +/**
> + * Structure WWAN_DEVICE_MODE_INFO - device mode information
> + * Holds the data that needs to be passed to userspace.
> + * The data is updated from the BIOS sensor information.
> + * @device_mode: Specific mode of the device
> + * @bandtable_index: Index of RF band
> + * @antennatable_index: Index of antenna
> + * @sartable_index: Index of SAR
> + */
> +struct WWAN_DEVICE_MODE_INFO {
> +		int device_mode;
> +		int bandtable_index;
> +		int antennatable_index;
> +		int sartable_index;
> +};
> +
> +/**
> + * Structure WWAN_DEVICE_MODE_CONFIGURATION - device configuration
> + * Holds the data that is configured and obtained on probe event.
> + * The data is updated from the BIOS sensor information.
> + * @version: Mode configuration version
> + * @total_dev_mode: Total number of device modes
> + * @device_mode_info: pointer to structure WWAN_DEVICE_MODE_INFO  */ 
> +struct WWAN_DEVICE_MODE_CONFIGURATION {
> +		int version;
> +		int total_dev_mode;
> +		struct WWAN_DEVICE_MODE_INFO *device_mode_info; };
> +
> +/**
> + * Structure WWAN_SUPPORTED_INFO - userspace datastore
> + * Holds the data that is obtained from userspace
> + * The data is updated from the userspace and send value back in the
> + * structure format that is mentioned here.
> + * @reg_mode_needed: regulatory mode set by user for tests
> + * @bios_table_revision: Version of SAR table
> + * @num_supported_modes: Total supported modes based on reg_mode  */ 
> +struct WWAN_SUPPORTED_INFO {
> +		int reg_mode_needed;
> +		int bios_table_revision;
> +		int num_supported_modes;
> +};
> +
> +/**
> + * Structure WWAN_SAR_CONTEXT - context of SAR
> + * Holds the complete context as long as the driver is in existence
> + * The context holds instance of the data used for different cases.
> + * @parse: identifies if dsm is parsed
> + * @data_read: identifies if data is already read from BIOS
> + * @guid: Group id
> + * @handle: store acpi handle
> + * @reg_value: regulatory value
> + * Regulatory 0: FCC, 1: CE, 2: ISED
> + * @sar_device: platform_device type
> + * @sar_kobject: kobject for sysfs
> + * @supported_data: WWAN_SUPPORTED_INFO struct
> + * @sar_data: WWAN_DEVICE_MODE_INFO struct
> + * @config_data: WWAN_DEVICE_MODE_CONFIGURATION array struct  */ 
> +struct WWAN_SAR_CONTEXT {
> +		bool parse;
> +		bool data_read;
> +		guid_t guid;
> +		acpi_handle handle;
> +		int reg_value;
> +		struct platform_device *sar_device;
> +		struct kobject *sar_kobject;
> +		struct WWAN_SUPPORTED_INFO supported_data;
> +		struct WWAN_DEVICE_MODE_INFO sar_data;
> +		struct WWAN_DEVICE_MODE_CONFIGURATION config_data[MAX_REGULATORY]; 
> +}; #endif /* INTEL_SAR_H */
> 


Regards,

Hans


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-07-27 14:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-10  7:40 [PATCH V2 0/1] [x86] BIOS SAR Driver for M.2 Intel Modems Shravan S
2021-05-10  7:40 ` [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem Shravan S
2021-06-24 10:24   ` Andy Shevchenko
2021-06-28 15:12     ` Hans de Goede
2021-06-28 17:47       ` Enrico Weigelt, metux IT consult
2021-06-28 18:20         ` Hans de Goede
2021-06-29 10:08           ` Enrico Weigelt, metux IT consult
2021-06-28 14:45   ` Enrico Weigelt, metux IT consult
2021-07-15 16:20   ` Hans de Goede
2021-07-27 14:38     ` Shravan, S

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).