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

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

[Nikita Kravets]

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>
---
 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

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 5692385e2d26..4534d11f9ca5 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -644,6 +644,13 @@ config THINKPAD_LMI
 
 source "drivers/platform/x86/intel/Kconfig"
 
+config MSI_EC
+	tristate "MSI EC Extras"
+	depends on ACPI
+	help
+	  This driver allows various MSI laptops' functionalities to be
+	  controlled from userspace, including battery charge threshold.
+
 config MSI_LAPTOP
 	tristate "MSI Laptop Extras"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 1d3d1b02541b..7cc2beca8208 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -71,6 +71,7 @@ obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
 obj-y				+= intel/
 
 # MSI
+obj-$(CONFIG_MSI_EC)		+= msi-ec.o
 obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
 obj-$(CONFIG_MSI_WMI)		+= msi-wmi.o
 
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>
+ */
+
+#include "msi-ec.h"
+
+#include <acpi/battery.h>
+#include <linux/acpi.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
+
+static const char *const SM_ECO_NAME       = "eco";
+static const char *const SM_COMFORT_NAME   = "comfort";
+static const char *const SM_SPORT_NAME     = "sport";
+static const char *const SM_TURBO_NAME     = "turbo";
+
+static const char *ALLOWED_FW_0[] __initdata = {
+	"14C1EMS1.101",
+	NULL,
+};
+
+static struct msi_ec_conf CONF0 __initdata = {
+	.allowed_fw = ALLOWED_FW_0,
+	.charge_control = {
+		.address      = 0xef,
+		.offset_start = 0x8a,
+		.offset_end   = 0x80,
+		.range_min    = 0x8a,
+		.range_max    = 0xe4,
+	},
+	.webcam = {
+		.address       = 0x2e,
+		.block_address = 0x2f,
+		.bit           = 1,
+	},
+	.fn_super_swap = {
+		.address = 0xbf,
+		.bit     = 4,
+	},
+	.cooler_boost = {
+		.address = 0x98,
+		.bit     = 7,
+	},
+	.shift_mode = {
+		.address = 0xf2,
+		.modes = {
+			{ SM_ECO_NAME,     0xc2 },
+			{ SM_COMFORT_NAME, 0xc1 },
+			{ SM_SPORT_NAME,   0xc0 },
+		},
+		.modes_count = 3,
+	},
+	.super_battery = {
+		.supported = false,
+	},
+	.fan_mode = {
+		.address = 0xf4,
+	},
+	.cpu = {
+		.rt_temp_address       = 0x68,
+		.rt_fan_speed_address  = 0x71,
+		.rt_fan_speed_base_min = 0x19,
+		.rt_fan_speed_base_max = 0x37,
+		.bs_fan_speed_address  = 0x89,
+		.bs_fan_speed_base_min = 0x00,
+		.bs_fan_speed_base_max = 0x0f,
+	},
+	.gpu = {
+		.rt_temp_address      = 0x80,
+		.rt_fan_speed_address = 0x89,
+	},
+	.leds = {
+		.micmute_led_address = 0x2b,
+		.mute_led_address    = 0x2c,
+		.bit                 = 2,
+	},
+	.kbd_bl = {
+		.bl_mode_address  = 0x2c, // ?
+		.bl_modes         = { 0x00, 0x08 }, // ?
+		.max_mode         = 1, // ?
+		.bl_state_address = 0xf3,
+		.state_base_value = 0x80,
+		.max_state        = 3,
+	},
+};
+
+static const char *ALLOWED_FW_1[] __initdata = {
+	"17F2EMS1.106",
+	NULL,
+};
+
+static struct msi_ec_conf CONF1 __initdata = {
+	.allowed_fw = ALLOWED_FW_1,
+	.charge_control = {
+		.address      = 0xef,
+		.offset_start = 0x8a,
+		.offset_end   = 0x80,
+		.range_min    = 0x8a,
+		.range_max    = 0xe4,
+	},
+	.webcam = {
+		.address       = 0x2e,
+		.block_address = 0x2f,
+		.bit           = 1,
+	},
+	.fn_super_swap = {
+		.address = 0xbf,
+		.bit     = 4,
+	},
+	.cooler_boost = {
+		.address = 0x98,
+		.bit     = 7,
+	},
+	.shift_mode = {
+		.address = 0xf2,
+		.modes = {
+			{ SM_ECO_NAME,     0xc2 },
+			{ SM_COMFORT_NAME, 0xc1 },
+			{ SM_SPORT_NAME,   0xc0 },
+			{ SM_TURBO_NAME,   0xc4 },
+		},
+		.modes_count = 4,
+	},
+	.super_battery = {
+		.supported = false,
+	},
+	.fan_mode = {
+		.address = 0xf4,
+	},
+	.cpu = {
+		.rt_temp_address       = 0x68,
+		.rt_fan_speed_address  = 0x71,
+		.rt_fan_speed_base_min = 0x19,
+		.rt_fan_speed_base_max = 0x37,
+		.bs_fan_speed_address  = 0x89,
+		.bs_fan_speed_base_min = 0x00,
+		.bs_fan_speed_base_max = 0x0f,
+	},
+	.gpu = {
+		.rt_temp_address      = 0x80,
+		.rt_fan_speed_address = 0x89,
+	},
+	.leds = {
+		.micmute_led_address = 0x2b,
+		.mute_led_address    = 0x2c,
+		.bit                 = 2,
+	},
+	.kbd_bl = {
+		.bl_mode_address  = 0x2c, // ?
+		.bl_modes         = { 0x00, 0x08 }, // ?
+		.max_mode         = 1, // ?
+		.bl_state_address = 0xf3,
+		.state_base_value = 0x80,
+		.max_state        = 3,
+	},
+};
+
+static const char *ALLOWED_FW_2[] __initdata = {
+	"1552EMS1.118",
+	NULL,
+};
+
+static struct msi_ec_conf CONF2 __initdata = {
+	.allowed_fw = ALLOWED_FW_2,
+	.charge_control = {
+		.address      = 0xd7,
+		.offset_start = 0x8a,
+		.offset_end   = 0x80,
+		.range_min    = 0x8a,
+		.range_max    = 0xe4,
+	},
+	.webcam = {
+		.address       = 0x2e,
+		.block_address = 0x2f,
+		.bit           = 1,
+	},
+	.fn_super_swap = {
+		.address = 0xe8,
+		.bit     = 4,
+	},
+	.cooler_boost = {
+		.address = 0x98,
+		.bit     = 7,
+	},
+	.shift_mode = {
+		.address = 0xf2,
+		.modes = {
+			{ SM_ECO_NAME,     0xc2 },
+			{ SM_COMFORT_NAME, 0xc1 },
+			{ SM_SPORT_NAME,   0xc0 },
+		},
+		.modes_count = 3,
+	},
+	.super_battery = {
+		.supported = true,
+		.address   = 0xeb,
+		.mask      = 0x0f,
+	},
+	.fan_mode = {
+		.address = 0xd4,
+	},
+	.cpu = {
+		.rt_temp_address       = 0x68,
+		.rt_fan_speed_address  = 0x71,
+		.rt_fan_speed_base_min = 0x19,
+		.rt_fan_speed_base_max = 0x37,
+		.bs_fan_speed_address  = 0x89,
+		.bs_fan_speed_base_min = 0x00,
+		.bs_fan_speed_base_max = 0x0f,
+	},
+	.gpu = {
+		.rt_temp_address      = 0x80,
+		.rt_fan_speed_address = 0x89,
+	},
+	.leds = {
+		.micmute_led_address = 0x2c,
+		.mute_led_address    = 0x2d,
+		.bit                 = 1,
+	},
+	.kbd_bl = {
+		.bl_mode_address  = 0x2c, // ?
+		.bl_modes         = { 0x00, 0x08 }, // ?
+		.max_mode         = 1, // ?
+		.bl_state_address = 0xd3,
+		.state_base_value = 0x80,
+		.max_state        = 3,
+	},
+};
+
+static const char *ALLOWED_FW_3[] __initdata = {
+	"1592EMS1.111",
+	"E1592IMS.10C",
+	NULL,
+};
+
+static struct msi_ec_conf CONF3 __initdata = {
+	.allowed_fw = ALLOWED_FW_3,
+	.charge_control = {
+		.address      = 0xef,
+		.offset_start = 0x8a,
+		.offset_end   = 0x80,
+		.range_min    = 0x8a,
+		.range_max    = 0xe4,
+	},
+	.webcam = {
+		.address       = 0x2e,
+		.block_address = 0x2f,
+		.bit           = 1,
+	},
+	.fn_super_swap = {
+		.address = 0xe8,
+		.bit     = 4,
+	},
+	.cooler_boost = {
+		.address = 0x98,
+		.bit     = 7,
+	},
+	.shift_mode = {
+		.address = 0xd2,
+		.modes = {
+			{ SM_ECO_NAME,     0xc2 },
+			{ SM_COMFORT_NAME, 0xc1 },
+			{ SM_SPORT_NAME,   0xc0 },
+		},
+		.modes_count = 3,
+	},
+	.super_battery = {
+		.supported = true,
+		.address   = 0xeb,
+		.mask      = 0x0f,
+	},
+	.fan_mode = {
+		.address = 0xd4,
+	},
+	.cpu = {
+		.rt_temp_address       = 0x68,
+		.rt_fan_speed_address  = 0xc9,
+		.rt_fan_speed_base_min = 0x19,
+		.rt_fan_speed_base_max = 0x37,
+		.bs_fan_speed_address  = 0x89, // ?
+		.bs_fan_speed_base_min = 0x00,
+		.bs_fan_speed_base_max = 0x0f,
+	},
+	.gpu = {
+		.rt_temp_address      = 0x80,
+		.rt_fan_speed_address = 0x89,
+	},
+	.leds = {
+		.micmute_led_address = 0x2b,
+		.mute_led_address    = 0x2c,
+		.bit                 = 1,
+	},
+	.kbd_bl = {
+		.bl_mode_address  = 0x2c, // ?
+		.bl_modes         = { 0x00, 0x08 }, // ?
+		.max_mode         = 1, // ?
+		.bl_state_address = 0xd3,
+		.state_base_value = 0x80,
+		.max_state        = 3,
+	},
+};
+
+static struct msi_ec_conf *CONFIGURATIONS[] __initdata = {
+	&CONF0,
+	&CONF1,
+	&CONF2,
+	&CONF3,
+	NULL,
+};
+
+static struct msi_ec_conf conf; // current configuration
+
+// ============================================================ //
+// Helper functions
+// ============================================================ //
+
+static int ec_read_seq(u8 addr, u8 *buf, int len)
+{
+	int result;
+	u8 i;
+	for (i = 0; i < len; i++) {
+		result = ec_read(addr + i, buf + i);
+		if (result < 0)
+			return result;
+	}
+	return 0;
+}
+
+static int ec_get_firmware_version(u8 buf[MSI_EC_FW_VERSION_LENGTH + 1])
+{
+	int result;
+
+	memset(buf, 0, MSI_EC_FW_VERSION_LENGTH + 1);
+	result = ec_read_seq(MSI_EC_FW_VERSION_ADDRESS,
+			     buf,
+			     MSI_EC_FW_VERSION_LENGTH);
+	if (result < 0)
+		return result;
+
+	return MSI_EC_FW_VERSION_LENGTH + 1;
+}
+
+// ============================================================ //
+// Sysfs power_supply subsystem
+// ============================================================ //
+
+static ssize_t charge_control_threshold_show(u8 offset, struct device *device,
+					     struct device_attribute *attr,
+					     char *buf)
+{
+	u8 rdata;
+	int result;
+
+	result = ec_read(conf.charge_control.address, &rdata);
+	if (result < 0)
+		return result;
+
+	return sprintf(buf, "%i\n", rdata - offset);
+}
+
+static ssize_t charge_control_threshold_store(u8 offset, struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf, size_t count)
+{
+	u8 wdata;
+	int result;
+
+	result = kstrtou8(buf, 10, &wdata);
+	if (result < 0)
+		return result;
+
+	wdata += offset;
+	if (wdata < conf.charge_control.range_min ||
+	    wdata > conf.charge_control.range_max)
+		return -EINVAL;
+
+	result = ec_write(conf.charge_control.address, wdata);
+	if (result < 0)
+		return result;
+
+	return count;
+}
+
+static ssize_t
+charge_control_start_threshold_show(struct device *device,
+				    struct device_attribute *attr, char *buf)
+{
+	return charge_control_threshold_show(conf.charge_control.offset_start,
+					     device, attr, buf);
+}
+
+static ssize_t
+charge_control_start_threshold_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	return charge_control_threshold_store(conf.charge_control.offset_start,
+					      dev, attr, buf, count);
+}
+
+static ssize_t charge_control_end_threshold_show(struct device *device,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	return charge_control_threshold_show(conf.charge_control.offset_end,
+					     device, attr, buf);
+}
+
+static ssize_t charge_control_end_threshold_store(struct device *dev,
+						  struct device_attribute *attr,
+						  const char *buf, size_t count)
+{
+	return charge_control_threshold_store(conf.charge_control.offset_end,
+					      dev, attr, buf, count);
+}
+
+static DEVICE_ATTR_RW(charge_control_start_threshold);
+static DEVICE_ATTR_RW(charge_control_end_threshold);
+
+static struct attribute *msi_battery_attrs[] = {
+	&dev_attr_charge_control_start_threshold.attr,
+	&dev_attr_charge_control_end_threshold.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(msi_battery);
+
+static int msi_battery_add(struct power_supply *battery,
+			   struct acpi_battery_hook *hook)
+{
+	if (device_add_groups(&battery->dev, msi_battery_groups))
+		return -ENODEV;
+	return 0;
+}
+
+static int msi_battery_remove(struct power_supply *battery,
+			      struct acpi_battery_hook *hook)
+{
+	device_remove_groups(&battery->dev, msi_battery_groups);
+	return 0;
+}
+
+static struct acpi_battery_hook battery_hook = {
+	.add_battery = msi_battery_add,
+	.remove_battery = msi_battery_remove,
+	.name = MSI_EC_DRIVER_NAME,
+};
+
+// ============================================================ //
+// Module load/unload
+// ============================================================ //
+
+static int __init load_configuration(void)
+{
+	int result;
+
+	// get firmware version
+	u8 fw_version[MSI_EC_FW_VERSION_LENGTH + 1];
+	result = ec_get_firmware_version(fw_version);
+	if (result < 0) {
+		return result;
+	}
+
+	// load the suitable configuration, if exists
+	for (int i = 0; CONFIGURATIONS[i]; i++) {
+		for (int j = 0; CONFIGURATIONS[i]->allowed_fw[j]; j++) {
+			if (strcmp(CONFIGURATIONS[i]->allowed_fw[j], fw_version) == 0) {
+				memcpy(&conf, CONFIGURATIONS[i], sizeof(struct msi_ec_conf));
+				conf.allowed_fw = NULL;
+				return 0;
+			}
+		}
+	}
+
+	pr_err("Your firmware version is not supported!\n");
+	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");
+		return -ENODEV;
+	}
+
+	result = load_configuration();
+	if (result < 0)
+		return result;
+
+	battery_hook_register(&battery_hook);
+
+	pr_info("msi-ec: module_init\n");
+	return 0;
+}
+
+static void __exit msi_ec_exit(void)
+{
+	battery_hook_unregister(&battery_hook);
+
+	pr_info("msi-ec: module_exit\n");
+}
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jose Angel Pastrana <japp0005@red.ujaen.es>");
+MODULE_AUTHOR("Aakash Singh <mail@singhaakash.dev>");
+MODULE_AUTHOR("Nikita Kravets <teackot@gmail.com>");
+MODULE_DESCRIPTION("MSI Embedded Controller");
+
+module_init(msi_ec_init);
+module_exit(msi_ec_exit);
diff --git a/drivers/platform/x86/msi-ec.h b/drivers/platform/x86/msi-ec.h
new file mode 100644
index 000000000000..4de6bba363ff
--- /dev/null
+++ b/drivers/platform/x86/msi-ec.h
@@ -0,0 +1,119 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+/*
+ * msi-ec: MSI laptops' embedded controller driver.
+ *
+ * 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>
+ */
+
+#ifndef _MSI_EC_H_
+#define _MSI_EC_H_
+
+#include <linux/types.h>
+
+#define MSI_EC_DRIVER_NAME "msi-ec"
+
+// Firmware info addresses are universal
+#define MSI_EC_FW_VERSION_ADDRESS 0xa0
+#define MSI_EC_FW_DATE_ADDRESS    0xac
+#define MSI_EC_FW_TIME_ADDRESS    0xb4
+#define MSI_EC_FW_VERSION_LENGTH  12
+#define MSI_EC_FW_DATE_LENGTH     8
+#define MSI_EC_FW_TIME_LENGTH     8
+
+struct msi_ec_charge_control_conf {
+	int address;
+	int offset_start;
+	int offset_end;
+	int range_min;
+	int range_max;
+};
+
+struct msi_ec_webcam_conf {
+	int address;
+	int block_address;
+	int bit;
+};
+
+struct msi_ec_fn_super_swap_conf {
+	int address;
+	int bit;
+};
+
+struct msi_ec_cooler_boost_conf {
+	int address;
+	int bit;
+};
+
+struct msi_ec_mode {
+	const char *name;
+	int value;
+};
+
+struct msi_ec_shift_mode_conf {
+	int address;
+	struct msi_ec_mode modes[5]; // fixed size for easier hard coding
+	int modes_count;
+};
+
+struct msi_ec_super_battery_conf {
+	bool supported;
+	int address;
+	int mask;
+};
+
+struct msi_ec_fan_mode_conf {
+	int address;
+};
+
+struct msi_ec_cpu_conf {
+	int rt_temp_address;
+	int rt_fan_speed_address; // realtime
+	int rt_fan_speed_base_min;
+	int rt_fan_speed_base_max;
+	int bs_fan_speed_address; // basic
+	int bs_fan_speed_base_min;
+	int bs_fan_speed_base_max;
+};
+
+struct msi_ec_gpu_conf {
+	int rt_temp_address;
+	int rt_fan_speed_address; // realtime
+};
+
+struct msi_ec_led_conf {
+	int micmute_led_address;
+	int mute_led_address;
+	int bit;
+};
+
+#define MSI_EC_KBD_BL_STATE_MASK 0x3
+struct msi_ec_kbd_bl_conf {
+	int bl_mode_address;
+	int bl_modes[2];
+	int max_mode;
+
+	int bl_state_address;
+	int state_base_value;
+	int max_state;
+};
+
+struct msi_ec_conf {
+	const char **allowed_fw;
+
+	struct msi_ec_charge_control_conf charge_control;
+	struct msi_ec_webcam_conf         webcam;
+	struct msi_ec_fn_super_swap_conf  fn_super_swap;
+	struct msi_ec_cooler_boost_conf   cooler_boost;
+	struct msi_ec_shift_mode_conf     shift_mode;
+	struct msi_ec_super_battery_conf  super_battery;
+	struct msi_ec_fan_mode_conf       fan_mode;
+	struct msi_ec_cpu_conf            cpu;
+	struct msi_ec_gpu_conf            gpu;
+	struct msi_ec_led_conf            leds;
+	struct msi_ec_kbd_bl_conf         kbd_bl;
+};
+
+#endif // _MSI_EC_H_
-- 
2.39.1

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

[Barnabás Pőcze]

Hi


2023. február 14., kedd 14:25 keltezéssel, Nikita Kravets <teackot@gmail.com> írta:

> 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>
> ---
>  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
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 5692385e2d26..4534d11f9ca5 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -644,6 +644,13 @@ config THINKPAD_LMI
> 
>  source "drivers/platform/x86/intel/Kconfig"
> 
> +config MSI_EC
> +	tristate "MSI EC Extras"
> +	depends on ACPI
> +	help
> +	  This driver allows various MSI laptops' functionalities to be
> +	  controlled from userspace, including battery charge threshold.
> +
>  config MSI_LAPTOP
>  	tristate "MSI Laptop Extras"
>  	depends on ACPI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 1d3d1b02541b..7cc2beca8208 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
>  obj-y				+= intel/
> 
>  # MSI
> +obj-$(CONFIG_MSI_EC)		+= msi-ec.o
>  obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
>  obj-$(CONFIG_MSI_WMI)		+= msi-wmi.o
> 
> 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>
> + */
> +
> +#include "msi-ec.h"
> +
> +#include <acpi/battery.h>
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/proc_fs.h>
> +#include <linux/seq_file.h>
> +
> +static const char *const SM_ECO_NAME       = "eco";
> +static const char *const SM_COMFORT_NAME   = "comfort";
> +static const char *const SM_SPORT_NAME     = "sport";
> +static const char *const SM_TURBO_NAME     = "turbo";
> +
> +static const char *ALLOWED_FW_0[] __initdata = {
> +	"14C1EMS1.101",
> +	NULL,

Usually commas are omitted after sentinel entries.


> +};
> +
> +static struct msi_ec_conf CONF0 __initdata = {
> +	.allowed_fw = ALLOWED_FW_0,

Alternatively:

	.allowed_fw = (const char * const []) {
		"...",
		"...",
		NULL
	},

(although this won't inherit the __initdata attribute as far as I can see)


> +	.charge_control = {
> +		.address      = 0xef,
> +		.offset_start = 0x8a,
> +		.offset_end   = 0x80,
> +		.range_min    = 0x8a,
> +		.range_max    = 0xe4,
> +	},
> +	.webcam = {
> +		.address       = 0x2e,
> +		.block_address = 0x2f,
> +		.bit           = 1,
> +	},
> +	.fn_super_swap = {
> +		.address = 0xbf,
> +		.bit     = 4,
> +	},
> +	.cooler_boost = {
> +		.address = 0x98,
> +		.bit     = 7,
> +	},
> +	.shift_mode = {
> +		.address = 0xf2,
> +		.modes = {
> +			{ SM_ECO_NAME,     0xc2 },
> +			{ SM_COMFORT_NAME, 0xc1 },
> +			{ SM_SPORT_NAME,   0xc0 },
> +		},
> +		.modes_count = 3,
> +	},
> +	.super_battery = {
> +		.supported = false,
> +	},
> +	.fan_mode = {
> +		.address = 0xf4,
> +	},
> +	.cpu = {
> +		.rt_temp_address       = 0x68,
> +		.rt_fan_speed_address  = 0x71,
> +		.rt_fan_speed_base_min = 0x19,
> +		.rt_fan_speed_base_max = 0x37,
> +		.bs_fan_speed_address  = 0x89,
> +		.bs_fan_speed_base_min = 0x00,
> +		.bs_fan_speed_base_max = 0x0f,
> +	},
> +	.gpu = {
> +		.rt_temp_address      = 0x80,
> +		.rt_fan_speed_address = 0x89,
> +	},
> +	.leds = {
> +		.micmute_led_address = 0x2b,
> +		.mute_led_address    = 0x2c,
> +		.bit                 = 2,
> +	},
> +	.kbd_bl = {
> +		.bl_mode_address  = 0x2c, // ?
> +		.bl_modes         = { 0x00, 0x08 }, // ?
> +		.max_mode         = 1, // ?
> +		.bl_state_address = 0xf3,
> +		.state_base_value = 0x80,
> +		.max_state        = 3,
> +	},
> +};
> +
> +static const char *ALLOWED_FW_1[] __initdata = {
> +	"17F2EMS1.106",
> +	NULL,
> +};
> +
> +static struct msi_ec_conf CONF1 __initdata = {
> +	.allowed_fw = ALLOWED_FW_1,
> +	.charge_control = {
> +		.address      = 0xef,
> +		.offset_start = 0x8a,
> +		.offset_end   = 0x80,
> +		.range_min    = 0x8a,
> +		.range_max    = 0xe4,
> +	},
> +	.webcam = {
> +		.address       = 0x2e,
> +		.block_address = 0x2f,
> +		.bit           = 1,
> +	},
> +	.fn_super_swap = {
> +		.address = 0xbf,
> +		.bit     = 4,
> +	},
> +	.cooler_boost = {
> +		.address = 0x98,
> +		.bit     = 7,
> +	},
> +	.shift_mode = {
> +		.address = 0xf2,
> +		.modes = {
> +			{ SM_ECO_NAME,     0xc2 },
> +			{ SM_COMFORT_NAME, 0xc1 },
> +			{ SM_SPORT_NAME,   0xc0 },
> +			{ SM_TURBO_NAME,   0xc4 },
> +		},
> +		.modes_count = 4,
> +	},
> +	.super_battery = {
> +		.supported = false,
> +	},
> +	.fan_mode = {
> +		.address = 0xf4,
> +	},
> +	.cpu = {
> +		.rt_temp_address       = 0x68,
> +		.rt_fan_speed_address  = 0x71,
> +		.rt_fan_speed_base_min = 0x19,
> +		.rt_fan_speed_base_max = 0x37,
> +		.bs_fan_speed_address  = 0x89,
> +		.bs_fan_speed_base_min = 0x00,
> +		.bs_fan_speed_base_max = 0x0f,
> +	},
> +	.gpu = {
> +		.rt_temp_address      = 0x80,
> +		.rt_fan_speed_address = 0x89,
> +	},
> +	.leds = {
> +		.micmute_led_address = 0x2b,
> +		.mute_led_address    = 0x2c,
> +		.bit                 = 2,
> +	},
> +	.kbd_bl = {
> +		.bl_mode_address  = 0x2c, // ?
> +		.bl_modes         = { 0x00, 0x08 }, // ?
> +		.max_mode         = 1, // ?
> +		.bl_state_address = 0xf3,
> +		.state_base_value = 0x80,
> +		.max_state        = 3,
> +	},
> +};
> +
> +static const char *ALLOWED_FW_2[] __initdata = {
> +	"1552EMS1.118",
> +	NULL,
> +};
> +
> +static struct msi_ec_conf CONF2 __initdata = {
> +	.allowed_fw = ALLOWED_FW_2,
> +	.charge_control = {
> +		.address      = 0xd7,
> +		.offset_start = 0x8a,
> +		.offset_end   = 0x80,
> +		.range_min    = 0x8a,
> +		.range_max    = 0xe4,
> +	},
> +	.webcam = {
> +		.address       = 0x2e,
> +		.block_address = 0x2f,
> +		.bit           = 1,
> +	},
> +	.fn_super_swap = {
> +		.address = 0xe8,
> +		.bit     = 4,
> +	},
> +	.cooler_boost = {
> +		.address = 0x98,
> +		.bit     = 7,
> +	},
> +	.shift_mode = {
> +		.address = 0xf2,
> +		.modes = {
> +			{ SM_ECO_NAME,     0xc2 },
> +			{ SM_COMFORT_NAME, 0xc1 },
> +			{ SM_SPORT_NAME,   0xc0 },
> +		},
> +		.modes_count = 3,
> +	},
> +	.super_battery = {
> +		.supported = true,
> +		.address   = 0xeb,
> +		.mask      = 0x0f,
> +	},
> +	.fan_mode = {
> +		.address = 0xd4,
> +	},
> +	.cpu = {
> +		.rt_temp_address       = 0x68,
> +		.rt_fan_speed_address  = 0x71,
> +		.rt_fan_speed_base_min = 0x19,
> +		.rt_fan_speed_base_max = 0x37,
> +		.bs_fan_speed_address  = 0x89,
> +		.bs_fan_speed_base_min = 0x00,
> +		.bs_fan_speed_base_max = 0x0f,
> +	},
> +	.gpu = {
> +		.rt_temp_address      = 0x80,
> +		.rt_fan_speed_address = 0x89,
> +	},
> +	.leds = {
> +		.micmute_led_address = 0x2c,
> +		.mute_led_address    = 0x2d,
> +		.bit                 = 1,
> +	},
> +	.kbd_bl = {
> +		.bl_mode_address  = 0x2c, // ?
> +		.bl_modes         = { 0x00, 0x08 }, // ?
> +		.max_mode         = 1, // ?
> +		.bl_state_address = 0xd3,
> +		.state_base_value = 0x80,
> +		.max_state        = 3,
> +	},
> +};
> +
> +static const char *ALLOWED_FW_3[] __initdata = {
> +	"1592EMS1.111",
> +	"E1592IMS.10C",
> +	NULL,
> +};
> +
> +static struct msi_ec_conf CONF3 __initdata = {
> +	.allowed_fw = ALLOWED_FW_3,
> +	.charge_control = {
> +		.address      = 0xef,
> +		.offset_start = 0x8a,
> +		.offset_end   = 0x80,
> +		.range_min    = 0x8a,
> +		.range_max    = 0xe4,
> +	},
> +	.webcam = {
> +		.address       = 0x2e,
> +		.block_address = 0x2f,
> +		.bit           = 1,
> +	},
> +	.fn_super_swap = {
> +		.address = 0xe8,
> +		.bit     = 4,
> +	},
> +	.cooler_boost = {
> +		.address = 0x98,
> +		.bit     = 7,
> +	},
> +	.shift_mode = {
> +		.address = 0xd2,
> +		.modes = {
> +			{ SM_ECO_NAME,     0xc2 },
> +			{ SM_COMFORT_NAME, 0xc1 },
> +			{ SM_SPORT_NAME,   0xc0 },
> +		},
> +		.modes_count = 3,
> +	},
> +	.super_battery = {
> +		.supported = true,
> +		.address   = 0xeb,
> +		.mask      = 0x0f,
> +	},
> +	.fan_mode = {
> +		.address = 0xd4,
> +	},
> +	.cpu = {
> +		.rt_temp_address       = 0x68,
> +		.rt_fan_speed_address  = 0xc9,
> +		.rt_fan_speed_base_min = 0x19,
> +		.rt_fan_speed_base_max = 0x37,
> +		.bs_fan_speed_address  = 0x89, // ?
> +		.bs_fan_speed_base_min = 0x00,
> +		.bs_fan_speed_base_max = 0x0f,
> +	},
> +	.gpu = {
> +		.rt_temp_address      = 0x80,
> +		.rt_fan_speed_address = 0x89,
> +	},
> +	.leds = {
> +		.micmute_led_address = 0x2b,
> +		.mute_led_address    = 0x2c,
> +		.bit                 = 1,
> +	},
> +	.kbd_bl = {
> +		.bl_mode_address  = 0x2c, // ?
> +		.bl_modes         = { 0x00, 0x08 }, // ?
> +		.max_mode         = 1, // ?
> +		.bl_state_address = 0xd3,
> +		.state_base_value = 0x80,
> +		.max_state        = 3,
> +	},
> +};
> +
> +static struct msi_ec_conf *CONFIGURATIONS[] __initdata = {
> +	&CONF0,
> +	&CONF1,
> +	&CONF2,
> +	&CONF3,
> +	NULL,
> +};
> +
> +static struct msi_ec_conf conf; // current configuration
> +
> +// ============================================================ //
> +// Helper functions
> +// ============================================================ //
> +
> +static int ec_read_seq(u8 addr, u8 *buf, int len)
> +{
> +	int result;
> +	u8 i;
> +	for (i = 0; i < len; i++) {

It's a small thing, but I would make `i` and `len` be the same type.


> +		result = ec_read(addr + i, buf + i);
> +		if (result < 0)
> +			return result;
> +	}
> +	return 0;
> +}
> +
> +static int ec_get_firmware_version(u8 buf[MSI_EC_FW_VERSION_LENGTH + 1])
> +{
> +	int result;
> +
> +	memset(buf, 0, MSI_EC_FW_VERSION_LENGTH + 1);
> +	result = ec_read_seq(MSI_EC_FW_VERSION_ADDRESS,
> +			     buf,
> +			     MSI_EC_FW_VERSION_LENGTH);
> +	if (result < 0)
> +		return result;
> +
> +	return MSI_EC_FW_VERSION_LENGTH + 1;
> +}
> +
> +// ============================================================ //
> +// Sysfs power_supply subsystem
> +// ============================================================ //
> +
> +static ssize_t charge_control_threshold_show(u8 offset, struct device *device,
> +					     struct device_attribute *attr,
> +					     char *buf)
> +{
> +	u8 rdata;
> +	int result;
> +
> +	result = ec_read(conf.charge_control.address, &rdata);
> +	if (result < 0)
> +		return result;
> +
> +	return sprintf(buf, "%i\n", rdata - offset);

Please prefer `sysfs_emit()`.


> +}
> +
> +static ssize_t charge_control_threshold_store(u8 offset, struct device *dev,
> +					      struct device_attribute *attr,
> +					      const char *buf, size_t count)
> +{
> +	u8 wdata;
> +	int result;
> +
> +	result = kstrtou8(buf, 10, &wdata);
> +	if (result < 0)
> +		return result;
> +
> +	wdata += offset;
> +	if (wdata < conf.charge_control.range_min ||
> +	    wdata > conf.charge_control.range_max)
> +		return -EINVAL;
> +
> +	result = ec_write(conf.charge_control.address, wdata);
> +	if (result < 0)
> +		return result;
> +
> +	return count;
> +}
> +
> +static ssize_t
> +charge_control_start_threshold_show(struct device *device,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	return charge_control_threshold_show(conf.charge_control.offset_start,
> +					     device, attr, buf);
> +}
> +
> +static ssize_t
> +charge_control_start_threshold_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	return charge_control_threshold_store(conf.charge_control.offset_start,
> +					      dev, attr, buf, count);
> +}
> +
> +static ssize_t charge_control_end_threshold_show(struct device *device,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	return charge_control_threshold_show(conf.charge_control.offset_end,
> +					     device, attr, buf);
> +}
> +
> +static ssize_t charge_control_end_threshold_store(struct device *dev,
> +						  struct device_attribute *attr,
> +						  const char *buf, size_t count)
> +{
> +	return charge_control_threshold_store(conf.charge_control.offset_end,
> +					      dev, attr, buf, count);
> +}
> +
> +static DEVICE_ATTR_RW(charge_control_start_threshold);
> +static DEVICE_ATTR_RW(charge_control_end_threshold);
> +
> +static struct attribute *msi_battery_attrs[] = {
> +	&dev_attr_charge_control_start_threshold.attr,
> +	&dev_attr_charge_control_end_threshold.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(msi_battery);
> +
> +static int msi_battery_add(struct power_supply *battery,
> +			   struct acpi_battery_hook *hook)
> +{
> +	if (device_add_groups(&battery->dev, msi_battery_groups))
> +		return -ENODEV;
> +	return 0;

Why not

  return device_add_groups(...);

?

Furthermore, is it possible that there are two or more batteries?


> +}
> +
> +static int msi_battery_remove(struct power_supply *battery,
> +			      struct acpi_battery_hook *hook)
> +{
> +	device_remove_groups(&battery->dev, msi_battery_groups);
> +	return 0;
> +}
> +
> +static struct acpi_battery_hook battery_hook = {
> +	.add_battery = msi_battery_add,
> +	.remove_battery = msi_battery_remove,
> +	.name = MSI_EC_DRIVER_NAME,
> +};
> +
> +// ============================================================ //
> +// Module load/unload
> +// ============================================================ //
> +
> +static int __init load_configuration(void)
> +{
> +	int result;
> +
> +	// get firmware version
> +	u8 fw_version[MSI_EC_FW_VERSION_LENGTH + 1];
> +	result = ec_get_firmware_version(fw_version);
> +	if (result < 0) {
> +		return result;
> +	}
> +
> +	// load the suitable configuration, if exists
> +	for (int i = 0; CONFIGURATIONS[i]; i++) {
> +		for (int j = 0; CONFIGURATIONS[i]->allowed_fw[j]; j++) {
> +			if (strcmp(CONFIGURATIONS[i]->allowed_fw[j], fw_version) == 0) {
> +				memcpy(&conf, CONFIGURATIONS[i], sizeof(struct msi_ec_conf));
> +				conf.allowed_fw = NULL;
> +				return 0;
> +			}
> +		}
> +	}

Have you checked if `match_string()` from string.h works here?


> +
> +	pr_err("Your firmware version is not supported!\n");
> +	return -EOPNOTSUPP;
> +}
> +
> +static int __init msi_ec_init(void)
> +{
> +	int result;
> +
> +	if (acpi_disabled) {

I am wondering how useful this check really is.


> +		pr_err("Unable to init because ACPI needs to be enabled first!\n");
> +		return -ENODEV;
> +	}
> +
> +	result = load_configuration();

This will start poking the embedded controller when the module is loaded,
regardless of the platform. I am not sure that is desirable.


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

Instead of manually prefixing the messages, I suggest you do

  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

before including any headers and then you can drop the "msi-ec: " from the strings.


> +	return 0;
> +}
> +
> +static void __exit msi_ec_exit(void)
> +{
> +	battery_hook_unregister(&battery_hook);
> +
> +	pr_info("msi-ec: module_exit\n");
> +}
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jose Angel Pastrana <japp0005@red.ujaen.es>");
> +MODULE_AUTHOR("Aakash Singh <mail@singhaakash.dev>");
> +MODULE_AUTHOR("Nikita Kravets <teackot@gmail.com>");
> +MODULE_DESCRIPTION("MSI Embedded Controller");
> +
> +module_init(msi_ec_init);
> +module_exit(msi_ec_exit);
> diff --git a/drivers/platform/x86/msi-ec.h b/drivers/platform/x86/msi-ec.h
> new file mode 100644
> index 000000000000..4de6bba363ff
> --- /dev/null
> +++ b/drivers/platform/x86/msi-ec.h
> @@ -0,0 +1,119 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +/*
> + * msi-ec: MSI laptops' embedded controller driver.
> + *
> + * 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>
> + */
> +
> +#ifndef _MSI_EC_H_
> +#define _MSI_EC_H_
> +
> +#include <linux/types.h>
> +
> +#define MSI_EC_DRIVER_NAME "msi-ec"
> +
> +// Firmware info addresses are universal
> +#define MSI_EC_FW_VERSION_ADDRESS 0xa0
> +#define MSI_EC_FW_DATE_ADDRESS    0xac
> +#define MSI_EC_FW_TIME_ADDRESS    0xb4
> +#define MSI_EC_FW_VERSION_LENGTH  12
> +#define MSI_EC_FW_DATE_LENGTH     8
> +#define MSI_EC_FW_TIME_LENGTH     8
> +
> +struct msi_ec_charge_control_conf {
> +	int address;
> +	int offset_start;
> +	int offset_end;
> +	int range_min;
> +	int range_max;
> +};
> +
> +struct msi_ec_webcam_conf {
> +	int address;
> +	int block_address;
> +	int bit;
> +};
> +
> +struct msi_ec_fn_super_swap_conf {
> +	int address;
> +	int bit;
> +};
> +
> +struct msi_ec_cooler_boost_conf {
> +	int address;
> +	int bit;
> +};
> +
> +struct msi_ec_mode {
> +	const char *name;
> +	int value;
> +};
> +
> +struct msi_ec_shift_mode_conf {
> +	int address;
> +	struct msi_ec_mode modes[5]; // fixed size for easier hard coding
> +	int modes_count;
> +};
> +
> +struct msi_ec_super_battery_conf {
> +	bool supported;
> +	int address;
> +	int mask;
> +};
> +
> +struct msi_ec_fan_mode_conf {
> +	int address;
> +};
> +
> +struct msi_ec_cpu_conf {
> +	int rt_temp_address;
> +	int rt_fan_speed_address; // realtime
> +	int rt_fan_speed_base_min;
> +	int rt_fan_speed_base_max;
> +	int bs_fan_speed_address; // basic
> +	int bs_fan_speed_base_min;
> +	int bs_fan_speed_base_max;
> +};
> +
> +struct msi_ec_gpu_conf {
> +	int rt_temp_address;
> +	int rt_fan_speed_address; // realtime
> +};
> +
> +struct msi_ec_led_conf {
> +	int micmute_led_address;
> +	int mute_led_address;
> +	int bit;
> +};
> +
> +#define MSI_EC_KBD_BL_STATE_MASK 0x3
> +struct msi_ec_kbd_bl_conf {
> +	int bl_mode_address;
> +	int bl_modes[2];
> +	int max_mode;
> +
> +	int bl_state_address;
> +	int state_base_value;
> +	int max_state;
> +};
> +
> +struct msi_ec_conf {
> +	const char **allowed_fw;
> +
> +	struct msi_ec_charge_control_conf charge_control;
> +	struct msi_ec_webcam_conf         webcam;
> +	struct msi_ec_fn_super_swap_conf  fn_super_swap;
> +	struct msi_ec_cooler_boost_conf   cooler_boost;
> +	struct msi_ec_shift_mode_conf     shift_mode;
> +	struct msi_ec_super_battery_conf  super_battery;
> +	struct msi_ec_fan_mode_conf       fan_mode;
> +	struct msi_ec_cpu_conf            cpu;
> +	struct msi_ec_gpu_conf            gpu;
> +	struct msi_ec_led_conf            leds;
> +	struct msi_ec_kbd_bl_conf         kbd_bl;
> +};
> +
> +#endif // _MSI_EC_H_
> --
> 2.39.1
> 


Regards,
Barnabás Pőcze

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

[Nikita Kravets]

Hi,

First of all, sorry if any of you got the message more than once.
Gmail (and I) messed some stuff up.

> Usually commas are omitted after sentinel entries.

I see, makes sense.

> Alternatively:
>
>         .allowed_fw = (const char * const []) {
>                 "...",
>                 "...",
>                 NULL
>         },
>
> (although this won't inherit the __initdata attribute as far as I can see)

This looks nicer so the question is: how important is it to put those
strings into initdata, as they don't take much memory.

> It's a small thing, but I would make `i` and `len` be the same type.

Okay. I should also put the `i` declaration into the for loop header.
(I'm not the original creator of this function so I didn't touch it
yet)

> Why not
>
>   return device_add_groups(...);
>
> ?

Agreed. Didn't look into this one too.

> Furthermore, is it possible that there are two or more batteries?

So far all laptops we've tested only have one, controlled by a single
EC parameter.

> Have you checked if `match_string()` from string.h works here?

Just checked, it does.

> This will start poking the embedded controller when the module is loaded, regardless of the platform. I am not sure that is desirable.

It only reads though, can it cause any harm?

> static int __init load_configuration(void)
> {
>         int result;
>
>         // get firmware version
>         u8 ver[MSI_EC_FW_VERSION_LENGTH + 1];
>         result = ec_get_firmware_version(ver);
>         if (result < 0) {
>                 return result;
>         }

Also a note from myself: I think this should return -EOPNOTSUPP if
ec_get_firmware() returns -ENODEV

-- 
Regards,
Nikita

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

[Hans de Goede]

Hi,

Before diving into a full review of the driver I thought
it would be good to first catch up with the email thread sofar.

I agree with everything discussed so far. One clarifying remark
below:

On 2/15/23 22:27, Nikita Kravets wrote:

<snip>

>> This will start poking the embedded controller when the module is loaded, regardless of the platform. I am not sure that is desirable.
> 
> It only reads though, can it cause any harm?

I have seen cases where some weird (i2c) hw reacts to
reads as if they are writes.

But since this is using the standardized ACPI EC access
routines I believe that doing reads should generally
speaking be safe.

Also it seems that atm the module must always be loaded
manually ?

I don't see any MODULE_ALIAS or MODULE_DEVICE_TABLE entries
in the driver to make it auto-load.

I think this should get a dmi_system_id tables with known
MSI DMI_SYS_VENDOR() matches in there + a
MODULE_DEVICE_TABLE() pointing to the dmi_system_id table
to have the driver auto-load on MSI systems.

I think what we should do here is:

1. Get the module in the mainline kernel
2. Do a blogpost asking MSI laptop users to test
3. Assuming testing does not show any regressions / issues
   (it will likely show lots of unsupported fw versions)
   add the MODULE_DEVICE_TABLE().

Regards,

Hans

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

[Hans de Goede]

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.

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

[Nikita Kravets]

Hi Hans,

We already have changes addressing Barnabás' remarks merged
into the original repo, including the pr_fmt macro, so I only
need to apply them to the kernel.

> I ran checkpatch on the patch and it found several issues

Thanks, I'll address them. Some of them are already fixed
in the original repo.

> Also it seems that atm the module must always be loaded
> manually ?

> I think this should get a dmi_system_id tables with known
> MSI DMI_SYS_VENDOR() matches in there + a
> MODULE_DEVICE_TABLE() pointing to the dmi_system_id table
> to have the driver auto-load on MSI systems.

It loads automatically for me. Though would be better
to only auto-load it on MSI systems.

Regards,

Nikita

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

[Hans de Goede]

Hi Nikita,

On 3/1/23 21:17, Nikita Kravets wrote:
> Hi Hans,
> 
> We already have changes addressing Barnabás' remarks merged
> into the original repo, including the pr_fmt macro, so I only
> need to apply them to the kernel.
> 
>> I ran checkpatch on the patch and it found several issues
> 
> Thanks, I'll address them. Some of them are already fixed
> in the original repo.
> 
>> Also it seems that atm the module must always be loaded
>> manually ?
> 
>> I think this should get a dmi_system_id tables with known
>> MSI DMI_SYS_VENDOR() matches in there + a
>> MODULE_DEVICE_TABLE() pointing to the dmi_system_id table
>> to have the driver auto-load on MSI systems.
> 
> It loads automatically for me. Though would be better
> to only auto-load it on MSI systems.

I don't see any module-aliases in the submitted msi-ec.c,
so AFAICT it should not auto load.

Thus, that it is autoloaded for you is weird. Did you
maybe add it to a config file in /etc/modules-load.d/ ?

What is the output of "modinfo msi-ec" on your system ?

Regards,

Hans

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

[Nikita Kravets]

Here is my `modinfo msi-ec`:

name:           msi_ec
filename:       (builtin)
description:    MSI Embedded Controller
author:         Nikita Kravets <teackot@gmail.com>
author:         Aakash Singh <mail@singhaakash.dev>
author:         Jose Angel Pastrana <japp0005@red.ujaen.es>
license:        GPL
file:           drivers/platform/x86/msi-ec

> Did you maybe add it to a config file in /etc/modules-load.d/ ?

No, I don't have it there

Regards,

Nikita

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

[Hans de Goede]

Hi,

On 3/2/23 13:45, Nikita Kravets wrote:
> Here is my `modinfo msi-ec`:
> 
> name:           msi_ec
> filename:       (builtin)
> description:    MSI Embedded Controller
> author:         Nikita Kravets <teackot@gmail.com>
> author:         Aakash Singh <mail@singhaakash.dev>
> author:         Jose Angel Pastrana <japp0005@red.ujaen.es>
> license:        GPL
> file:           drivers/platform/x86/msi-ec

Ah you have set it to builtin rather then building it as
a module.

Yes then it will work without any module-aliases since then it
is always part of the kernel.

Regards,

Hans