platform-driver-x86.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Shravan S <s.shravan@intel.com>,
	mgross@linux.intel.com, platform-driver-x86@vger.kernel.org
Cc: sudhakar.an@intel.com
Subject: Re: [PATCH V2 1/1] [x86]: BIOS Dynamic SAR driver for Intel M.2 Modem
Date: Thu, 15 Jul 2021 18:20:06 +0200	[thread overview]
Message-ID: <f353c576-7960-d211-245f-0e735f95a39f@redhat.com> (raw)
In-Reply-To: <20210510074016.31627-2-s.shravan@intel.com>

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


  parent reply	other threads:[~2021-07-15 16:20 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2021-07-27 14:38     ` Shravan, S

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f353c576-7960-d211-245f-0e735f95a39f@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=mgross@linux.intel.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=s.shravan@intel.com \
    --cc=sudhakar.an@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).