On Wed, Jun 23, 2021 at 5:04 PM Shravan S <s.shravan@xxxxxxxxx> 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
