(Side note, most of the comments you may have avoided if you had read
an internal Wiki page about popular style mistakes)
Another note, comments given even against one occurrence of similar
code should be considered for the others.
On Sat, Aug 7, 2021 at 12:24 AM Shravan S <s.shravan@xxxxxxxxx> wrote:
>
> BIOS SAR driver for RF power regulation on Intel M.2 Modem.
> Uses ACPI to communicate to BIOS to retrieve SAR information data.
> Uses Sysfs to communicate this data to userspace via read and notify.
> Userspace will use MBIM interface to enable data communication to modem.
This is going to be a commit message. The one above is poorly written
to the reader. E.g. what should I get from it "Uses ACPI"? Perhaps
something more elaborative like "It uses ACPI specific methods FOO and
BAR to retrieve this and that information". "The driver provides a
custom sysfs interface, documented in Documentation/...."
...
> drivers/platform/x86/Kconfig | 15 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/intel-sar.c | 317 ++++++++++++++++++
Use intel/ subfolder, please.
...
> + derived based on information like mcc/mnc that is available for
What are the mcc/mnc TLAs?
...
> +INTEL BIOS SAR DRIVER
> +M: Shravan S <s.shravan@xxxxxxxxx>
> +M: Intel Corporation <linuxwwan@xxxxxxxxx>
> +L: platform-driver-x86@xxxxxxxxxxxxxxx
> +S: Maintained
> +F: drivers/platform/x86/intel-sar.c
And nicely misses the file brought in this patch...
...
> +#include <asm/errno.h>
Why asm?
Why at the beginning of the inclusion list?
> +#include <linux/acpi.h>
> +#include <linux/kobject.h>
> +#include <linux/platform_data/x86/intel-sar.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysfs.h>
...
> +/**
> + * get_int_value - Retrieve Integer values from ACPI Object
> + * Value of the integer from the object of ACPI is obtained.
Put this after field descriptions. Same for all kernel doc descriptions.
> + * @obj: acpi_object pointer which has the integer value
> + * @out: output pointer will get integer value
> + * returns 0 on success
and that's it?
Besides that, your style doesn't follow the grammar and punctuation rules.
> + */
> +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 {
Redundant.
> + return -EIO;
> + }
Besides above, use standard pattern
if (foo != expected)
return error;
> +}
...
> +static acpi_status parse_package(struct wwan_sar_context *context, union acpi_object *item)
> +{
> + struct wwan_device_mode_configuration *data;
> + int value = 0, itr = 0, reg = 0, count = 0;
Why redundant assignments? They actually may hide the real errors.
> + union acpi_object *num;
> +
> + num = &item->package.elements[0];
> + if ((get_int_value(num, &value) != 0) || (value < 0 || value >= MAX_REGULATORY))
Too many parentheses. " != 0" is redundant (this applies to many
places in the proposed patch).
> + return AE_ERROR;
> +
> + reg = value;
> +
> + data = &context->config_data[reg];
> + if (data->total_dev_mode > MAX_DEV_MODES || data->total_dev_mode == 0)
> + return AE_ERROR;
> + count = (data->total_dev_mode < item->package.count) ?
> + data->total_dev_mode : item->package.count;
> + data->device_mode_info = kmalloc_array(data->total_dev_mode,
> + sizeof(struct wwan_device_mode_info), GFP_KERNEL);
> + if (!data->device_mode_info)
> + return AE_ERROR;
> + for (itr = 0; itr < count; itr++) {
> + struct wwan_device_mode_info temp = { 0 };
> +
> + num = &item->package.elements[itr + 1];
> + if (num->type != ACPI_TYPE_PACKAGE || num->package.count < TOTAL_DATA)
> + continue;
> + if (get_int_value(&num->package.elements[0], &temp.device_mode) != 0)
> + continue;
> + if (get_int_value(&num->package.elements[1], &temp.bandtable_index) != 0)
> + continue;
> + if (get_int_value(&num->package.elements[2], &temp.antennatable_index) != 0)
> + continue;
> + if (get_int_value(&num->package.elements[3], &temp.sartable_index) != 0)
> + continue;
> + data->device_mode_info[itr] = temp;
> + }
> + return AE_OK;
> +}
...
> +static acpi_status sar_get_device_mode(struct platform_device *device)
> +{
> + struct wwan_sar_context *context = dev_get_drvdata(&device->dev);
> + acpi_status status = AE_OK;
> + union acpi_object *out;
> + int value = 0;
> + u32 rev = 0;
> +
> + out = acpi_evaluate_dsm(context->handle, &context->guid, rev,
> + COMMAND_ID_DEV_MODE, NULL);
> + if (get_int_value(out, &value) != 0) {
> + pr_err("%s: Cmd:%d Failed\n", __func__, COMMAND_ID_DEV_MODE);
Use dev_err() and similar when you have a struct device.
The message itself looks like debugging, to be an error you have to
provide a human readable one and without __func__.
> + status = AE_ERROR;
> + goto dev_mode_error;
> + }
> + context->sar_data.device_mode = value;
> + sysfs_notify(&device->dev.kobj, NULL, SYSFS_DATANAME);
> +
> +dev_mode_error:
> + ACPI_FREE(out);
> + return status;
> +}
> +
> +static const struct acpi_device_id sar_device_ids[] = {
> + { "INTC1092", 0},
> + { "", 0},
No comma for terminator, moreover the { } is enough.
> +};
> +
Redundant blank line.
> +MODULE_DEVICE_TABLE(acpi, sar_device_ids);
...
> +static const struct platform_device_id sar_device_table[] = {
> + {"intc1092", 0},
> + {},
No comma.
> +};
...
> +static ssize_t intc_reg_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct wwan_sar_context *context = dev_get_drvdata(dev);
> + unsigned long value = 0;
> + int read = 0;
> + if (!count) {
> + pr_err("%s count = %d", __func__, (int)count);
> + return -EFAULT;
> + }
Is it ever possible?
> + read = kstrtoul(buf, 0, &value);
> + if (read <= 0) {
> + pr_err("%s Not a integer", __func__);
> + return -EINVAL;
Don't shadow an error code.
> + }
> + if (value >= MAX_REGULATORY)
> + return -EINVAL;
ERANGE?
EOVERFLOW?
> + context->reg_value = (int)value;
Why to use unsigned long in then cast to int? Maybe use int in the first place?
> + update_sar_data(context);
> + dev_set_drvdata(dev, context);
This seems a misuse of the field.
> + sysfs_notify(&dev->kobj, NULL, SYSFS_DATANAME);
> + return count;
> +}
> +
Redundant.
> +static DEVICE_ATTR_RO(intc_data);
> +static DEVICE_ATTR_RW(intc_reg);
Attach each of them to the respective functions.
...
> +static int sar_probe(struct platform_device *device)
> +{
> + union acpi_object *out, *item, req;
> + struct wwan_sar_context *context;
> + acpi_status status = AE_OK;
> + u32 rev = 0, reg = 0;
> + int result = 0;
Why assignments?
> + context = kmalloc(sizeof(*context), GFP_KERNEL);
> + if (!context)
> + return -ENOMEM;
> + memset(context, 0, sizeof(struct wwan_sar_context));
NIH kzalloc().
> + context->sar_device = device;
> + dev_set_drvdata(&device->dev, context);
> + if (guid_parse(SAR_DSM_UUID, &context->guid)) {
Shadowed error code
> + pr_err("%s: UUID error\n", __func__);
> + goto r_free;
> + }
> + context->handle = ACPI_HANDLE(&device->dev);
> + 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;
> + }
> + if (out->type == ACPI_TYPE_PACKAGE && out->package.count >= 3 &&
> + out->package.elements[0].type == ACPI_TYPE_INTEGER &&
> + out->package.elements[1].type == ACPI_TYPE_INTEGER &&
> + out->package.elements[2].type == ACPI_TYPE_PACKAGE) {
> + context->config_data[reg].version = out->package.elements[0].integer.value;
> + context->config_data[reg].total_dev_mode =
> + out->package.elements[1].integer.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);
> + continue;
> + }
> + item = &out->package.elements[2];
> + if (item->package.count > 0)
> + status = parse_package(context, item);
> + else
> + status = AE_ERROR;
> + if (status != AE_OK) {
> + ACPI_FREE(out);
> + continue;
> + }
> + }
> + ACPI_FREE(out);
> + }
Try to refactor this for-loop. Perhaps even move it out to a function or so.
> + update_sar_data(context);
> + result = sysfs_create_group(&device->dev.kobj, &intcsar_group);
> + if (result) {
> + pr_err("sysfs creation failed\n");
> + goto r_free;
> + }
> + if (sar_get_device_mode(device) != AE_OK) {
> + pr_err("Failed sar_get_device_mode\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:
> + sysfs_remove_group(&device->dev.kobj, &intcsar_group);
> +r_free:
> + kfree(context);
> + return -1;
What is this?
> +}
> +
> +static int sar_remove(struct platform_device *device)
> +{
> + struct wwan_sar_context *context = dev_get_drvdata(&device->dev);
> + int reg = 0;
> +
> + acpi_remove_notify_handler(ACPI_HANDLE(&device->dev),
> + ACPI_DEVICE_NOTIFY, sar_notify);
> + sysfs_remove_group(&device->dev.kobj, &intcsar_group);
> + for (reg = 0; reg < MAX_REGULATORY; reg++) {
> + kfree(context->config_data[reg].device_mode_info);
> + context->config_data[reg].device_mode_info = NULL;
> + }
> + kfree(context);
> + return 0;
> +}
...
> +MODULE_DEVICE_TABLE(platform, sar_device_table);
Besides the misplaced line in the code, why do you need this?
...
> + /* FOR ACPI HANDLING */
Useless.
...
> + .id_table = sar_device_table,
Why?
> +};
> +
Redundant blank line.
...
> +/*
> + * Intel Corporation Header File for Specific Absorption Rate
What is this supposed to mean?
> + * Copyright (c) 2021, Intel Corporation.
> + */
It seems like your header lacks the inclusion of the needed headers
and/or type forward declarations.
--
With Best Regards,
Andy Shevchenko
