Re: [PATCH] platform/x86: Add new msi-ec driver

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 2/14/23 14:25, Nikita Kravets wrote:
> Add a new driver to allow various MSI laptops' functionalities to be
> controlled from userspace. This includes such features as power
> profiles (aka shift modes), fan speed, charge thresholds, LEDs, etc.
> 
> This driver contains EC memory configurations for different firmware
> versions and exports battery charge thresholds to userspace (note,
> that start and end thresholds control the same EC parameter
> and are always 10% apart).
> 
> Link: https://github.com/BeardOverflow/msi-ec/
> Discussion: https://github.com/BeardOverflow/msi-ec/pull/13
> Signed-off-by: Nikita Kravets <teackot@gmail.com>

Thanks overall this looks pretty good.

Please address the review-remarks from Barnabás.

I also have 2 small remarks myself:

1. See the remark inline below.

2. I ran checkpatch on the patch and it found several issues,
   I'll copy and paste the output to the end of this email,
   please address these issues.

   And if possible run scripts/checkpatch.pl yourself to
   verify the issues are fixed.


> ---
>  drivers/platform/x86/Kconfig  |   7 +
>  drivers/platform/x86/Makefile |   1 +
>  drivers/platform/x86/msi-ec.c | 528 ++++++++++++++++++++++++++++++++++
>  drivers/platform/x86/msi-ec.h | 119 ++++++++
>  4 files changed, 655 insertions(+)
>  create mode 100644 drivers/platform/x86/msi-ec.c
>  create mode 100644 drivers/platform/x86/msi-ec.h

<snip>

> diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
> new file mode 100644
> index 000000000000..b32106445bf6
> --- /dev/null
> +++ b/drivers/platform/x86/msi-ec.c
> @@ -0,0 +1,528 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +/*
> + * msi-ec: MSI laptops' embedded controller driver.
> + *
> + * This driver allows various MSI laptops' functionalities to be
> + * controlled from userspace.
> + *
> + * It contains EC memory configurations for different firmware versions
> + * and exports battery charge thresholds to userspace.
> + *
> + * Copyright (C) 2023 Jose Angel Pastrana <japp0005@red.ujaen.es>
> + * Copyright (C) 2023 Aakash Singh <mail@singhaakash.dev>
> + * Copyright (C) 2023 Nikita Kravets <teackot@gmail.com>
> + */
> +

Since you are using pr_info() / pr_err() please add:

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

This will automatically prefix all pr_*() calls with

"msi-ec: "

<snip>

> +	pr_err("Your firmware version is not supported!\n");

So this will then now get automatically prefixed.

> +	return -EOPNOTSUPP;
> +}
> +
> +static int __init msi_ec_init(void)
> +{
> +	int result;
> +
> +	if (acpi_disabled) {
> +		pr_err("Unable to init because ACPI needs to be enabled first!\n");

And this too.


> +		return -ENODEV;
> +	}
> +
> +	result = load_configuration();
> +	if (result < 0)
> +		return result;
> +
> +	battery_hook_register(&battery_hook);
> +
> +	pr_info("msi-ec: module_init\n");

And you can drop the manual "msi-ec: " prefixing here then.

> +	return 0;
> +}
> +
> +static void __exit msi_ec_exit(void)
> +{
> +	battery_hook_unregister(&battery_hook);
> +
> +	pr_info("msi-ec: module_exit\n");

And drop it here too.

Regards,

Hans


[hans@shalem platform-drivers-x86]$ git format-patch HEAD~
0001-platform-x86-Add-new-msi-ec-driver.patch
[hans@shalem platform-drivers-x86]$ scripts/checkpatch.pl 0001-platform-x86-Add-new-msi-ec-driver.patch
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#60: 
new file mode 100644

WARNING: Improper SPDX comment style for 'drivers/platform/x86/msi-ec.c', please use '//' instead
#65: FILE: drivers/platform/x86/msi-ec.c:1:
+/* SPDX-License-Identifier: GPL-2.0-or-later */

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#65: FILE: drivers/platform/x86/msi-ec.c:1:
+/* SPDX-License-Identifier: GPL-2.0-or-later */

ERROR: Use of const init definition must use __initconst
#97: FILE: drivers/platform/x86/msi-ec.c:33:
+static const char *ALLOWED_FW_0[] __initdata = {

ERROR: Use of const init definition must use __initconst
#167: FILE: drivers/platform/x86/msi-ec.c:103:
+static const char *ALLOWED_FW_1[] __initdata = {

ERROR: Use of const init definition must use __initconst
#238: FILE: drivers/platform/x86/msi-ec.c:174:
+static const char *ALLOWED_FW_2[] __initdata = {

ERROR: Use of const init definition must use __initconst
#310: FILE: drivers/platform/x86/msi-ec.c:246:
+static const char *ALLOWED_FW_3[] __initdata = {

WARNING: Missing a blank line after declarations
#401: FILE: drivers/platform/x86/msi-ec.c:337:
+	u8 i;
+	for (i = 0; i < len; i++) {

WARNING: Missing a blank line after declarations
#539: FILE: drivers/platform/x86/msi-ec.c:475:
+	u8 fw_version[MSI_EC_FW_VERSION_LENGTH + 1];
+	result = ec_get_firmware_version(fw_version);

WARNING: braces {} are not necessary for single statement blocks
#540: FILE: drivers/platform/x86/msi-ec.c:476:
+	if (result < 0) {
+		return result;
+	}

total: 4 errors, 6 warnings, 667 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0001-platform-x86-Add-new-msi-ec-driver.patch has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.