[PATCH v2 0/4] asus-ec-sensors: add support for board families

[PATCH v2 0/4] asus-ec-sensors: add support for board families

[Eugene Shalygin]

Users provided information for boards from AMD-400 and sTRX40 families
and demonstrated that sensor addresses differ from those for the AMD-500
family. Also the AMD-400 family board uses the global ACPI lock instead
of a dedicated mutex to guard access to the hardware.

This patchset implements required changes to support other board
families:
 - per-family sensor definitions
 - options to choose hardware/state guard mutex: an AML mutex or the 
   global ACPI lock.

These changes are used to add support for the PRIME X470-PRO board.

Changes in v2:
 - Removed the case without ACPI mutex where the state was guarded using
	 the normal mutex. After receiving an update from user that case
	 turned out to be non-existent.
 - Removed the __initconst attribute from the board data array.
 - Updated documentation to include the special string for the mutex
	 path module parameters which make the driver use the global ACPI
	 lock.

Eugene Shalygin (4):
  hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data
  hwmon: (asus-ec-sensors) implement locking via the ACPI global lock
  hwmon: (asus-ec-sensors) add support for board families
  hwmon: (asus-ec-sensors) add PRIME X470-PRO board

 Documentation/hwmon/asus_ec_sensors.rst |   2 +
 drivers/hwmon/asus-ec-sensors.c         | 412 +++++++++++++++++-------
 2 files changed, 294 insertions(+), 120 deletions(-)

-- 
2.35.1

[PATCH v2 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Eugene Shalygin]

We need to keep some more information about the current board than just
the sensors set, and with more boards to add the dmi id array grows
quickly. Our probe code is always the same so let's switch to a custom
test code and a custom board info array. That allows us to omit board
vendor string (ASUS uses two strings that differ in case) in the board
info and use case-insensitive comparison, and also do not duplicate
sensor definitions for such board variants as " (WI-FI)" when sensors
are identical to the base variant.

Also saves a quarter of the module size by replacing big dmi_system_id
structs with smaller ones.

Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
---
 drivers/hwmon/asus-ec-sensors.c | 205 ++++++++++++++++++--------------
 1 file changed, 116 insertions(+), 89 deletions(-)

diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index e3d794fb0534..de24dc3d1390 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -54,8 +54,7 @@ static char *mutex_path_override;
 /* ACPI mutex for locking access to the EC for the firmware */
 #define ASUS_HW_ACCESS_MUTEX_ASMX	"\\AMW0.ASMX"
 
-/* There are two variants of the vendor spelling */
-#define VENDOR_ASUS_UPPER_CASE	"ASUSTeK COMPUTER INC."
+#define MAX_IDENTICAL_BOARD_VARIATIONS	2
 
 typedef union {
 	u32 value;
@@ -164,74 +163,94 @@ static const struct ec_sensor_info known_ec_sensors[] = {
 	(SENSOR_TEMP_CHIPSET | SENSOR_TEMP_CPU | SENSOR_TEMP_MB)
 #define SENSOR_SET_TEMP_WATER (SENSOR_TEMP_WATER_IN | SENSOR_TEMP_WATER_OUT)
 
-#define DMI_EXACT_MATCH_BOARD(vendor, name, sensors) {                         \
-	.matches = {                                                           \
-		DMI_EXACT_MATCH(DMI_BOARD_VENDOR, vendor),                     \
-		DMI_EXACT_MATCH(DMI_BOARD_NAME, name),                         \
-	},                                                                     \
-	.driver_data = (void *)(sensors), \
-}
+struct ec_board_info {
+	const char *board_names[MAX_IDENTICAL_BOARD_VARIATIONS];
+	unsigned long sensors;
+};
 
-static const struct dmi_system_id asus_ec_dmi_table[] __initconst = {
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "PRIME X570-PRO",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
-		SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
-			      "ProArt X570-CREATOR WIFI",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
-		SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CPU_OPT |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "Pro WS X570-ACE",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
-		SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
-			      "ROG CROSSHAIR VIII DARK HERO",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
-		SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
-			      "ROG CROSSHAIR VIII FORMULA",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG CROSSHAIR VIII HERO",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
-		SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
-		SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
-			      "ROG CROSSHAIR VIII HERO (WI-FI)",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
-		SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
-		SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
-			      "ROG CROSSHAIR VIII IMPACT",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_FAN_CHIPSET |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX B550-E GAMING",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
-		SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_FAN_CPU_OPT),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX B550-I GAMING",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
-		SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_FAN_VRM_HS |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-E GAMING",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
-		SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_FAN_CHIPSET |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-F GAMING",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
-		SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-I GAMING",
-		SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS |
-		SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
+static const struct ec_board_info board_info[] = {
+	{
+		.board_names = {"PRIME X570-PRO"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
+			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
+	},
+	{
+		.board_names = {"ProArt X570-CREATOR WIFI"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
+			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CPU_OPT |
+			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"Pro WS X570-ACE"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
+			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET |
+			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG CROSSHAIR VIII DARK HERO"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR |
+			SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
+			SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW |
+			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG CROSSHAIR VIII FORMULA"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
+			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {
+			"ROG CROSSHAIR VIII HERO",
+			"ROG CROSSHAIR VIII HERO (WI-FI)",
+		},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR |
+			SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
+			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
+			SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU |
+			SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG CROSSHAIR VIII IMPACT"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
+			SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG STRIX B550-E GAMING"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_CPU_OPT,
+	},
+	{
+		.board_names = {"ROG STRIX B550-I GAMING"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_VRM_HS | SENSOR_CURR_CPU |
+			SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG STRIX X570-E GAMING"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
+			SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG STRIX X570-F GAMING"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
+	},
+	{
+		.board_names = {"ROG STRIX X570-I GAMING"},
+		.sensors = SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS |
+			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
+			SENSOR_IN_CPU_CORE,
+	},
 	{}
 };
 
@@ -241,7 +260,7 @@ struct ec_sensor {
 };
 
 struct ec_sensors_data {
-	unsigned long board_sensors;
+	const struct ec_board_info *board_info;
 	struct ec_sensor *sensors;
 	/* EC registers to read from */
 	u16 *registers;
@@ -307,11 +326,6 @@ static int __init bank_compare(const void *a, const void *b)
 	return *((const s8 *)a) - *((const s8 *)b);
 }
 
-static int __init board_sensors_count(unsigned long sensors)
-{
-	return hweight_long(sensors);
-}
-
 static void __init setup_sensor_data(struct ec_sensors_data *ec)
 {
 	struct ec_sensor *s = ec->sensors;
@@ -322,8 +336,8 @@ static void __init setup_sensor_data(struct ec_sensors_data *ec)
 	ec->nr_banks = 0;
 	ec->nr_registers = 0;
 
-	for_each_set_bit(i, &ec->board_sensors,
-			  BITS_PER_TYPE(ec->board_sensors)) {
+	for_each_set_bit(i, &ec->board_info->sensors,
+			 BITS_PER_TYPE(ec->board_info->sensors)) {
 		s->info_index = i;
 		s->cached_value = 0;
 		ec->nr_registers +=
@@ -463,9 +477,10 @@ static inline s32 get_sensor_value(const struct ec_sensor_info *si, u8 *data)
 static void update_sensor_values(struct ec_sensors_data *ec, u8 *data)
 {
 	const struct ec_sensor_info *si;
-	struct ec_sensor *s;
+	struct ec_sensor *s, *sensor_end;
 
-	for (s = ec->sensors; s != ec->sensors + ec->nr_sensors; s++) {
+	sensor_end = ec->sensors + ec->nr_sensors;
+	for (s = ec->sensors; s != sensor_end; s++) {
 		si = &known_ec_sensors[s->info_index];
 		s->cached_value = get_sensor_value(si, data);
 		data += si->addr.components.size;
@@ -603,12 +618,24 @@ static struct hwmon_chip_info asus_ec_chip_info = {
 	.ops = &asus_ec_hwmon_ops,
 };
 
-static unsigned long __init get_board_sensors(void)
+static const struct ec_board_info * __init get_board_info(void)
 {
-	const struct dmi_system_id *dmi_entry =
-		dmi_first_match(asus_ec_dmi_table);
+	const char *dmi_board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
+	const char *dmi_board_name = dmi_get_system_info(DMI_BOARD_NAME);
+	const struct ec_board_info *board;
+
+	if (!dmi_board_vendor || !dmi_board_name ||
+	    strcasecmp(dmi_board_vendor, "ASUSTeK COMPUTER INC."))
+		return NULL;
+
+	for (board = board_info; board->sensors; board++) {
+		if (match_string(board->board_names,
+				 MAX_IDENTICAL_BOARD_VARIATIONS,
+				 dmi_board_name) >= 0)
+			return board;
+	}
 
-	return dmi_entry ? (unsigned long)dmi_entry->driver_data : 0;
+	return NULL;
 }
 
 static int __init asus_ec_probe(struct platform_device *pdev)
@@ -616,17 +643,17 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 	const struct hwmon_channel_info **ptr_asus_ec_ci;
 	int nr_count[hwmon_max] = { 0 }, nr_types = 0;
 	struct hwmon_channel_info *asus_ec_hwmon_chan;
+	const struct ec_board_info *pboard_info;
 	const struct hwmon_chip_info *chip_info;
 	struct device *dev = &pdev->dev;
 	struct ec_sensors_data *ec_data;
 	const struct ec_sensor_info *si;
 	enum hwmon_sensor_types type;
-	unsigned long board_sensors;
 	struct device *hwdev;
 	unsigned int i;
 
-	board_sensors = get_board_sensors();
-	if (!board_sensors)
+	pboard_info = get_board_info();
+	if (!pboard_info)
 		return -ENODEV;
 
 	ec_data = devm_kzalloc(dev, sizeof(struct ec_sensors_data),
@@ -635,8 +662,8 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	dev_set_drvdata(dev, ec_data);
-	ec_data->board_sensors = board_sensors;
-	ec_data->nr_sensors = board_sensors_count(ec_data->board_sensors);
+	ec_data->board_info = pboard_info;
+	ec_data->nr_sensors = hweight_long(ec_data->board_info->sensors);
 	ec_data->sensors = devm_kcalloc(dev, ec_data->nr_sensors,
 					sizeof(struct ec_sensor), GFP_KERNEL);
 
@@ -709,7 +736,7 @@ static struct platform_driver asus_ec_sensors_platform_driver = {
 	},
 };
 
-MODULE_DEVICE_TABLE(dmi, asus_ec_dmi_table);
+MODULE_DEVICE_TABLE(acpi, acpi_ec_ids);
 module_platform_driver_probe(asus_ec_sensors_platform_driver, asus_ec_probe);
 
 module_param_named(mutex_path, mutex_path_override, charp, 0);
-- 
2.35.1

Re: [PATCH v2 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Guenter Roeck]

On Tue, Apr 26, 2022 at 11:23:37AM +0200, Eugene Shalygin wrote:
> We need to keep some more information about the current board than just
> the sensors set, and with more boards to add the dmi id array grows
> quickly. Our probe code is always the same so let's switch to a custom
> test code and a custom board info array. That allows us to omit board
> vendor string (ASUS uses two strings that differ in case) in the board
> info and use case-insensitive comparison, and also do not duplicate
> sensor definitions for such board variants as " (WI-FI)" when sensors
> are identical to the base variant.
> 
> Also saves a quarter of the module size by replacing big dmi_system_id
> structs with smaller ones.
> 
> Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
> ---
...
>  
> -MODULE_DEVICE_TABLE(dmi, asus_ec_dmi_table);
> +MODULE_DEVICE_TABLE(acpi, acpi_ec_ids);
>  module_platform_driver_probe(asus_ec_sensors_platform_driver, asus_ec_probe);

Since this is now tied to MODULE_DEVICE_TABLE(acpi, ...), I think the
probe function should be referenced in asus_ec_sensors_platform_driver,
and it should be module_platform_driver() instead of
module_platform_driver_probe().

Guenter

Re: [PATCH v2 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Eugene Shalygin]

> Since this is now tied to MODULE_DEVICE_TABLE(acpi, ...), I think the
> probe function should be referenced in asus_ec_sensors_platform_driver,
> and it should be module_platform_driver() instead of
> module_platform_driver_probe().

As follows?

static struct platform_driver asus_ec_sensors_platform_driver_probe = {
    .probe = asus_ec_probe,
        .driver = {
        .name = "asus-ec-sensors",
    .acpi_match_table = acpi_ec_ids,
    },
};

MODULE_DEVICE_TABLE(acpi, acpi_ec_ids);
module_platform_driver(asus_ec_sensors_platform_driver_probe);

The "_probe" suffix added to keep the asus_ec_probe() code and its
deps as __init.

Eugene

Re: [PATCH v2 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Guenter Roeck]

On 4/27/22 02:32, Eugene Shalygin wrote:
>> Since this is now tied to MODULE_DEVICE_TABLE(acpi, ...), I think the
>> probe function should be referenced in asus_ec_sensors_platform_driver,
>> and it should be module_platform_driver() instead of
>> module_platform_driver_probe().
> 
> As follows?
> 
> static struct platform_driver asus_ec_sensors_platform_driver_probe = {
>      .probe = asus_ec_probe,
>          .driver = {
>          .name = "asus-ec-sensors",
>      .acpi_match_table = acpi_ec_ids,
>      },
> };
> 
> MODULE_DEVICE_TABLE(acpi, acpi_ec_ids);
> module_platform_driver(asus_ec_sensors_platform_driver_probe);
> 
> The "_probe" suffix added to keep the asus_ec_probe() code and its
> deps as __init.
> 

Sorry, I don't follow that part. One can add "__init" or "__initdata",
as in

static struct platform_driver asus_ec_sensors_platform_driver __initdata = {

to mark a function or data structure as __init. I don't think adding
"_probe" to the struct platform_driver variable name does that.

Guenter

Re: [PATCH v2 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Eugene Shalygin]

> Sorry, I don't follow that part. One can add "__init" or "__initdata",
> as in
>
> static struct platform_driver asus_ec_sensors_platform_driver __initdata = {
>
> to mark a function or data structure as __init. I don't think adding
> "_probe" to the struct platform_driver variable name does that.
>

__initdata leads to modpost warning:
WARNING: modpost: drivers/hwmon/asus-ec-sensors.o(.exit.text+0x3):
Section mismatch in reference from the function cleanup_module() to
the variable .init.data:asus_ec_sensors_platform_driver
The function __exit cleanup_module() references
a variable __initdata asus_ec_sensors_platform_driver.
This is often seen when error handling in the exit function
uses functionality in the init path.
The fix is often to remove the __initdata annotation of
asus_ec_sensors_platform_driver so it may be used outside an init section.

Compiling without attributes resulted in another message:
WARNING: modpost: drivers/hwmon/asus-ec-sensors.o(.data+0x0): Section
mismatch in reference from the variable
asus_ec_sensors_platform_driver to the function
.init.text:asus_ec_probe()
The variable asus_ec_sensors_platform_driver references
the function __init asus_ec_probe()
If the reference is valid then annotate the
variable with __init* or __refdata (see linux/init.h) or name the variable:
*_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console

Here is why I added the "_probe" suffix.

Eugene

Re: [PATCH v2 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Guenter Roeck]

On 4/27/22 05:16, Eugene Shalygin wrote:
>> Sorry, I don't follow that part. One can add "__init" or "__initdata",
>> as in
>>
>> static struct platform_driver asus_ec_sensors_platform_driver __initdata = {
>>
>> to mark a function or data structure as __init. I don't think adding
>> "_probe" to the struct platform_driver variable name does that.
>>
> 
> __initdata leads to modpost warning:
> WARNING: modpost: drivers/hwmon/asus-ec-sensors.o(.exit.text+0x3):
> Section mismatch in reference from the function cleanup_module() to
> the variable .init.data:asus_ec_sensors_platform_driver
> The function __exit cleanup_module() references
> a variable __initdata asus_ec_sensors_platform_driver.
> This is often seen when error handling in the exit function
> uses functionality in the init path.
> The fix is often to remove the __initdata annotation of
> asus_ec_sensors_platform_driver so it may be used outside an init section.
> 
> Compiling without attributes resulted in another message:
> WARNING: modpost: drivers/hwmon/asus-ec-sensors.o(.data+0x0): Section
> mismatch in reference from the variable
> asus_ec_sensors_platform_driver to the function
> .init.text:asus_ec_probe()
> The variable asus_ec_sensors_platform_driver references
> the function __init asus_ec_probe()
> If the reference is valid then annotate the
> variable with __init* or __refdata (see linux/init.h) or name the variable:
> *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console
> 
> Here is why I added the "_probe" suffix.
> 
> Eugene

Ah yes, I forgot about the exit function. It needs a pointer to
the structure, which would be gone if marked __initdata.
Please add a comment to the structure name explaining why
it is named _probe.

Thanks,
Guenter

Re: [PATCH v2 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Eugene Shalygin]

Maybe I'd better leave a comment why module_platform_driver_probe() is
used as opposed to module_platform_driver()? I think that one would be
more straightforward...

Regards,
Eugene

On Wed, 27 Apr 2022 at 15:20, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 4/27/22 05:16, Eugene Shalygin wrote:
> >> Sorry, I don't follow that part. One can add "__init" or "__initdata",
> >> as in
> >>
> >> static struct platform_driver asus_ec_sensors_platform_driver __initdata = {
> >>
> >> to mark a function or data structure as __init. I don't think adding
> >> "_probe" to the struct platform_driver variable name does that.
> >>
> >
> > __initdata leads to modpost warning:
> > WARNING: modpost: drivers/hwmon/asus-ec-sensors.o(.exit.text+0x3):
> > Section mismatch in reference from the function cleanup_module() to
> > the variable .init.data:asus_ec_sensors_platform_driver
> > The function __exit cleanup_module() references
> > a variable __initdata asus_ec_sensors_platform_driver.
> > This is often seen when error handling in the exit function
> > uses functionality in the init path.
> > The fix is often to remove the __initdata annotation of
> > asus_ec_sensors_platform_driver so it may be used outside an init section.
> >
> > Compiling without attributes resulted in another message:
> > WARNING: modpost: drivers/hwmon/asus-ec-sensors.o(.data+0x0): Section
> > mismatch in reference from the variable
> > asus_ec_sensors_platform_driver to the function
> > .init.text:asus_ec_probe()
> > The variable asus_ec_sensors_platform_driver references
> > the function __init asus_ec_probe()
> > If the reference is valid then annotate the
> > variable with __init* or __refdata (see linux/init.h) or name the variable:
> > *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console
> >
> > Here is why I added the "_probe" suffix.
> >
> > Eugene
>
> Ah yes, I forgot about the exit function. It needs a pointer to
> the structure, which would be gone if marked __initdata.
> Please add a comment to the structure name explaining why
> it is named _probe.
>
> Thanks,
> Guenter

Re: [PATCH v2 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Guenter Roeck]

On 4/27/22 06:34, Eugene Shalygin wrote:
> Maybe I'd better leave a comment why module_platform_driver_probe() is
> used as opposed to module_platform_driver()? I think that one would be
> more straightforward...
> 

You are correct, that is easier. I thought that it calls the probe
function directly and bypasses the normal probe handling, but that
is not the case. Sorry for the noise.

Guenter

Re: [PATCH v2 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Eugene Shalygin]

OK, thanks!

Eugene

> You are correct, that is easier. I thought that it calls the probe
> function directly and bypasses the normal probe handling, but that
> is not the case. Sorry for the noise.
>
> Guenter

[PATCH v2 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Eugene Shalygin]

For some board models ASUS uses the global ACPI lock to guard access to
the hardware, so do we.

Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
---
 Documentation/hwmon/asus_ec_sensors.rst |   2 +
 drivers/hwmon/asus-ec-sensors.c         | 125 +++++++++++++++++++-----
 2 files changed, 101 insertions(+), 26 deletions(-)

diff --git a/Documentation/hwmon/asus_ec_sensors.rst b/Documentation/hwmon/asus_ec_sensors.rst
index b3469851ab9a..36ca531d32dd 100644
--- a/Documentation/hwmon/asus_ec_sensors.rst
+++ b/Documentation/hwmon/asus_ec_sensors.rst
@@ -53,3 +53,5 @@ Module Parameters
 		the path is mostly identical for them). If ASUS changes this path
 		in a future BIOS update, this parameter can be used to override
 		the stored in the driver value until it gets updated.
+		A special string ":GLOBAL_LOCK" can be passed to use the ACPI
+		global lock instead of a dedicated mutex.
diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index de24dc3d1390..7267682de191 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -56,6 +56,9 @@ static char *mutex_path_override;
 
 #define MAX_IDENTICAL_BOARD_VARIATIONS	2
 
+/* Moniker for the ACPI global lock (':' is not allowed in ASL identifiers) */
+#define ACPI_GLOBAL_LOCK_PSEUDO_PATH	":GLOBAL_LOCK"
+
 typedef union {
 	u32 value;
 	struct {
@@ -166,6 +169,14 @@ static const struct ec_sensor_info known_ec_sensors[] = {
 struct ec_board_info {
 	const char *board_names[MAX_IDENTICAL_BOARD_VARIATIONS];
 	unsigned long sensors;
+	/*
+	 * Defines which mutex to use for guarding access to the state and the
+	 * hardware. Can be either a full path to an AML mutex or the
+	 * pseudo-path ACPI_GLOBAL_LOCK_PSEUDO_PATH to use the global ACPI lock,
+	 * or left empty to use a regular mutex object, in which case access to
+	 * the hardware is not guarded.
+	 */
+	const char *mutex_path;
 };
 
 static const struct ec_board_info board_info[] = {
@@ -173,6 +184,7 @@ static const struct ec_board_info board_info[] = {
 		.board_names = {"PRIME X570-PRO"},
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
 			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ProArt X570-CREATOR WIFI"},
@@ -185,6 +197,7 @@ static const struct ec_board_info board_info[] = {
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
 			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET |
 			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII DARK HERO"},
@@ -193,6 +206,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
 			SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW |
 			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII FORMULA"},
@@ -200,6 +214,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
 			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {
@@ -212,6 +227,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
 			SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII IMPACT"},
@@ -219,12 +235,14 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG STRIX B550-E GAMING"},
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_CPU_OPT,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG STRIX B550-I GAMING"},
@@ -232,6 +250,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_VRM_HS | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG STRIX X570-E GAMING"},
@@ -239,17 +258,20 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG STRIX X570-F GAMING"},
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
 			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG STRIX X570-I GAMING"},
 		.sensors = SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS |
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{}
 };
@@ -259,6 +281,46 @@ struct ec_sensor {
 	s32 cached_value;
 };
 
+struct lock_data {
+	union {
+		acpi_handle aml;
+		/* global lock handle */
+		u32 glk;
+	} mutex;
+	bool (*lock)(struct lock_data *data);
+	bool (*unlock)(struct lock_data *data);
+};
+
+/*
+ * The next function pairs implement options for locking access to the
+ * state and the EC
+ */
+static bool lock_via_acpi_mutex(struct lock_data *data)
+{
+	/*
+	 * ASUS DSDT does not specify that access to the EC has to be guarded,
+	 * but firmware does access it via ACPI
+	 */
+	return ACPI_SUCCESS(acpi_acquire_mutex(data->mutex.aml,
+					       NULL, ACPI_LOCK_DELAY_MS));
+}
+
+static bool unlock_acpi_mutex(struct lock_data *data)
+{
+	return ACPI_SUCCESS(acpi_release_mutex(data->mutex.aml, NULL));
+}
+
+static bool lock_via_global_acpi_lock(struct lock_data *data)
+{
+	return ACPI_SUCCESS(acpi_acquire_global_lock(ACPI_LOCK_DELAY_MS,
+						     &data->mutex.glk));
+}
+
+static bool unlock_global_acpi_lock(struct lock_data *data)
+{
+	return ACPI_SUCCESS(acpi_release_global_lock(data->mutex.glk));
+}
+
 struct ec_sensors_data {
 	const struct ec_board_info *board_info;
 	struct ec_sensor *sensors;
@@ -269,7 +331,7 @@ struct ec_sensors_data {
 	u8 banks[ASUS_EC_MAX_BANK + 1];
 	/* in jiffies */
 	unsigned long last_updated;
-	acpi_handle aml_mutex;
+	struct lock_data lock_data;
 	/* number of board EC sensors */
 	u8 nr_sensors;
 	/*
@@ -373,23 +435,36 @@ static void __init fill_ec_registers(struct ec_sensors_data *ec)
 	}
 }
 
-static acpi_handle __init asus_hw_access_mutex(struct device *dev)
+static int __init setup_lock_data(struct device *dev)
 {
 	const char *mutex_path;
-	acpi_handle res;
 	int status;
+	struct ec_sensors_data *state = dev_get_drvdata(dev);
 
 	mutex_path = mutex_path_override ?
-		mutex_path_override : ASUS_HW_ACCESS_MUTEX_ASMX;
+		mutex_path_override : state->board_info->mutex_path;
 
-	status = acpi_get_handle(NULL, (acpi_string)mutex_path, &res);
-	if (ACPI_FAILURE(status)) {
-		dev_err(dev,
-			"Could not get hardware access guard mutex '%s': error %d",
-			mutex_path, status);
-		return NULL;
+	if (!mutex_path || !strlen(mutex_path)) {
+		dev_err(dev, "Hardware access guard mutex name is empty");
+		return -EINVAL;
 	}
-	return res;
+	if (!strcmp(mutex_path, ACPI_GLOBAL_LOCK_PSEUDO_PATH)) {
+		state->lock_data.mutex.glk = 0;
+		state->lock_data.lock = lock_via_global_acpi_lock;
+		state->lock_data.unlock = unlock_global_acpi_lock;
+	} else {
+		status = acpi_get_handle(NULL, (acpi_string)mutex_path,
+					 &state->lock_data.mutex.aml);
+		if (ACPI_FAILURE(status)) {
+			dev_err(dev,
+				"Failed to get hardware access guard AML mutex '%s': error %d",
+				mutex_path, status);
+			return -ENOENT;
+		}
+		state->lock_data.lock = lock_via_acpi_mutex;
+		state->lock_data.unlock = unlock_acpi_mutex;
+	}
+	return 0;
 }
 
 static int asus_ec_bank_switch(u8 bank, u8 *old)
@@ -492,15 +567,9 @@ static int update_ec_sensors(const struct device *dev,
 {
 	int status;
 
-	/*
-	 * ASUS DSDT does not specify that access to the EC has to be guarded,
-	 * but firmware does access it via ACPI
-	 */
-	if (ACPI_FAILURE(acpi_acquire_mutex(ec->aml_mutex, NULL,
-					    ACPI_LOCK_DELAY_MS))) {
-		dev_err(dev, "Failed to acquire AML mutex");
-		status = -EBUSY;
-		goto cleanup;
+	if (!ec->lock_data.lock(&ec->lock_data)) {
+		dev_warn(dev, "Failed to acquire mutex");
+		return -EBUSY;
 	}
 
 	status = asus_ec_block_read(dev, ec);
@@ -508,10 +577,10 @@ static int update_ec_sensors(const struct device *dev,
 	if (!status) {
 		update_sensor_values(ec, ec->read_buffer);
 	}
-	if (ACPI_FAILURE(acpi_release_mutex(ec->aml_mutex, NULL))) {
-		dev_err(dev, "Failed to release AML mutex");
-	}
-cleanup:
+
+	if (!ec->lock_data.unlock(&ec->lock_data))
+		dev_err(dev, "Failed to release mutex");
+
 	return status;
 }
 
@@ -651,6 +720,7 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 	enum hwmon_sensor_types type;
 	struct device *hwdev;
 	unsigned int i;
+	int status;
 
 	pboard_info = get_board_info();
 	if (!pboard_info)
@@ -667,6 +737,11 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 	ec_data->sensors = devm_kcalloc(dev, ec_data->nr_sensors,
 					sizeof(struct ec_sensor), GFP_KERNEL);
 
+	status = setup_lock_data(dev);
+	if (status) {
+		dev_err(dev, "Failed to setup state/EC locking: %d", status);
+		return status;
+	}
 	setup_sensor_data(ec_data);
 	ec_data->registers = devm_kcalloc(dev, ec_data->nr_registers,
 					  sizeof(u16), GFP_KERNEL);
@@ -678,8 +753,6 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 
 	fill_ec_registers(ec_data);
 
-	ec_data->aml_mutex = asus_hw_access_mutex(dev);
-
 	for (i = 0; i < ec_data->nr_sensors; ++i) {
 		si = get_sensor_info(ec_data, i);
 		if (!nr_count[si->type])
-- 
2.35.1

[PATCH v2 3/4] hwmon: (asus-ec-sensors) add support for board families

[Eugene Shalygin]

DSDT code for AMD 400-series chipset shows that sensor addresses differ
for this generation from those for the AMD 500-series boards.

Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
---
 drivers/hwmon/asus-ec-sensors.c | 39 ++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index 7267682de191..b52e3679476a 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -135,8 +135,12 @@ enum ec_sensors {
 #define SENSOR_TEMP_WATER_IN BIT(ec_sensor_temp_water_in)
 #define SENSOR_TEMP_WATER_OUT BIT(ec_sensor_temp_water_out)
 
+enum board_family {
+	family_amd_500_series,
+};
+
 /* All the known sensors for ASUS EC controllers */
-static const struct ec_sensor_info known_ec_sensors[] = {
+static const struct ec_sensor_info sensors_family_amd_500[] = {
 	[ec_sensor_temp_chipset] =
 		EC_SENSOR("Chipset", hwmon_temp, 1, 0x00, 0x3a),
 	[ec_sensor_temp_cpu] = EC_SENSOR("CPU", hwmon_temp, 1, 0x00, 0x3b),
@@ -177,6 +181,7 @@ struct ec_board_info {
 	 * the hardware is not guarded.
 	 */
 	const char *mutex_path;
+	enum board_family family;
 };
 
 static const struct ec_board_info board_info[] = {
@@ -185,6 +190,7 @@ static const struct ec_board_info board_info[] = {
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
 			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ProArt X570-CREATOR WIFI"},
@@ -198,6 +204,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET |
 			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII DARK HERO"},
@@ -207,6 +214,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW |
 			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII FORMULA"},
@@ -215,6 +223,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
 			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {
@@ -228,6 +237,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII IMPACT"},
@@ -236,6 +246,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG STRIX B550-E GAMING"},
@@ -243,6 +254,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_CPU_OPT,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG STRIX B550-I GAMING"},
@@ -251,6 +263,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_FAN_VRM_HS | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG STRIX X570-E GAMING"},
@@ -259,12 +272,14 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG STRIX X570-F GAMING"},
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
 			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG STRIX X570-I GAMING"},
@@ -272,6 +287,7 @@ static const struct ec_board_info board_info[] = {
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{}
 };
@@ -323,6 +339,7 @@ static bool unlock_global_acpi_lock(struct lock_data *data)
 
 struct ec_sensors_data {
 	const struct ec_board_info *board_info;
+	const struct ec_sensor_info *sensors_info;
 	struct ec_sensor *sensors;
 	/* EC registers to read from */
 	u16 *registers;
@@ -365,7 +382,7 @@ static bool is_sensor_data_signed(const struct ec_sensor_info *si)
 static const struct ec_sensor_info *
 get_sensor_info(const struct ec_sensors_data *state, int index)
 {
-	return &known_ec_sensors[state->sensors[index].info_index];
+	return state->sensors_info + state->sensors[index].info_index;
 }
 
 static int find_ec_sensor_index(const struct ec_sensors_data *ec,
@@ -403,9 +420,9 @@ static void __init setup_sensor_data(struct ec_sensors_data *ec)
 		s->info_index = i;
 		s->cached_value = 0;
 		ec->nr_registers +=
-			known_ec_sensors[s->info_index].addr.components.size;
+			ec->sensors_info[s->info_index].addr.components.size;
 		bank_found = false;
-		bank = known_ec_sensors[s->info_index].addr.components.bank;
+		bank = ec->sensors_info[s->info_index].addr.components.bank;
 		for (j = 0; j < ec->nr_banks; j++) {
 			if (ec->banks[j] == bank) {
 				bank_found = true;
@@ -556,7 +573,7 @@ static void update_sensor_values(struct ec_sensors_data *ec, u8 *data)
 
 	sensor_end = ec->sensors + ec->nr_sensors;
 	for (s = ec->sensors; s != sensor_end; s++) {
-		si = &known_ec_sensors[s->info_index];
+		si = ec->sensors_info + s->info_index;
 		s->cached_value = get_sensor_value(si, data);
 		data += si->addr.components.size;
 	}
@@ -733,6 +750,17 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(dev, ec_data);
 	ec_data->board_info = pboard_info;
+
+	switch (ec_data->board_info->family) {
+	case family_amd_500_series:
+		ec_data->sensors_info = sensors_family_amd_500;
+		break;
+	default:
+		dev_err(dev, "Unknown board family: %d",
+			ec_data->board_info->family);
+		return -EINVAL;
+	}
+
 	ec_data->nr_sensors = hweight_long(ec_data->board_info->sensors);
 	ec_data->sensors = devm_kcalloc(dev, ec_data->nr_sensors,
 					sizeof(struct ec_sensor), GFP_KERNEL);
@@ -742,6 +770,7 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 		dev_err(dev, "Failed to setup state/EC locking: %d", status);
 		return status;
 	}
+
 	setup_sensor_data(ec_data);
 	ec_data->registers = devm_kcalloc(dev, ec_data->nr_registers,
 					  sizeof(u16), GFP_KERNEL);
-- 
2.35.1

Re: [PATCH v2 3/4] hwmon: (asus-ec-sensors) add support for board families

[Guenter Roeck]

On Tue, Apr 26, 2022 at 11:23:39AM +0200, Eugene Shalygin wrote:
> DSDT code for AMD 400-series chipset shows that sensor addresses differ
> for this generation from those for the AMD 500-series boards.
> 
> Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
> ---
>  drivers/hwmon/asus-ec-sensors.c | 39 ++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
> index 7267682de191..b52e3679476a 100644
> --- a/drivers/hwmon/asus-ec-sensors.c
> +++ b/drivers/hwmon/asus-ec-sensors.c
> @@ -135,8 +135,12 @@ enum ec_sensors {
>  #define SENSOR_TEMP_WATER_IN BIT(ec_sensor_temp_water_in)
>  #define SENSOR_TEMP_WATER_OUT BIT(ec_sensor_temp_water_out)
>  
> +enum board_family {
> +	family_amd_500_series,

The default enum value is 0. This means that specifying nothing
for .family matches in the structure below always matches the first
enum, which doesn't make much sense and might cause trouble in the
future. I would suggest to explicitly select a value != 0 as first
entry, such as 

enum board_family {
	family_amd_500_series = 1,
	...
};

to avoid that problem.

Guenter

Re: [PATCH v2 3/4] hwmon: (asus-ec-sensors) add support for board families

[Eugene Shalygin]

> > +enum board_family {
> > +     family_amd_500_series,
>
> The default enum value is 0. This means that specifying nothing
> for .family matches in the structure below always matches the first
> enum, which doesn't make much sense and might cause trouble in the
> future. I would suggest to explicitly select a value != 0 as first
> entry, such as
>
> enum board_family {
>         family_amd_500_series = 1,
>         ...
> };
>
> to avoid that problem.

Thank you, added family_unknown member ( = 0) instead, to simplify
adding new members (I want this enum to be alphabetically sorted).

Eugene

[PATCH v2 4/4] hwmon: (asus-ec-sensors) add PRIME X470-PRO board

[Eugene Shalygin]

This board is supposed to be handled by the asus-wmi-sensors driver,
but due to a buggy WMI implementation the driver and the official ASUS
software make the BIOS hang together with fan controls [1, 2].

This driver complements values provided by the SIO chip and does not
freeze the BIOS, as tested by a user [2].

[1] https://github.com/electrified/asus-wmi-sensors/blob/master/README.md
[2] https://github.com/zeule/asus-ec-sensors/issues/12

Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
---
 drivers/hwmon/asus-ec-sensors.c | 43 +++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index b52e3679476a..3af4b22fd3c5 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -136,10 +136,41 @@ enum ec_sensors {
 #define SENSOR_TEMP_WATER_OUT BIT(ec_sensor_temp_water_out)
 
 enum board_family {
+	family_amd_400_series,
 	family_amd_500_series,
 };
 
 /* All the known sensors for ASUS EC controllers */
+static const struct ec_sensor_info sensors_family_amd_400[] = {
+	[ec_sensor_temp_chipset] =
+		EC_SENSOR("Chipset", hwmon_temp, 1, 0x00, 0x3a),
+	[ec_sensor_temp_cpu] =
+		EC_SENSOR("CPU", hwmon_temp, 1, 0x00, 0x3b),
+	[ec_sensor_temp_mb] =
+		EC_SENSOR("Motherboard", hwmon_temp, 1, 0x00, 0x3c),
+	[ec_sensor_temp_t_sensor] =
+		EC_SENSOR("T_Sensor", hwmon_temp, 1, 0x00, 0x3d),
+	[ec_sensor_temp_vrm] =
+		EC_SENSOR("VRM", hwmon_temp, 1, 0x00, 0x3e),
+	[ec_sensor_in_cpu_core] =
+		EC_SENSOR("CPU Core", hwmon_in, 2, 0x00, 0xa2),
+	[ec_sensor_fan_cpu_opt] =
+		EC_SENSOR("CPU_Opt", hwmon_fan, 2, 0x00, 0xbc),
+	[ec_sensor_fan_vrm_hs] =
+		EC_SENSOR("VRM HS", hwmon_fan, 2, 0x00, 0xb2),
+	[ec_sensor_fan_chipset] =
+		/* no chipset fans in this generation */
+		EC_SENSOR("Chipset", hwmon_fan, 0, 0x00, 0x00),
+	[ec_sensor_fan_water_flow] =
+		EC_SENSOR("Water_Flow", hwmon_fan, 2, 0x00, 0xb4),
+	[ec_sensor_curr_cpu] =
+		EC_SENSOR("CPU", hwmon_curr, 1, 0x00, 0xf4),
+	[ec_sensor_temp_water_in] =
+		EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x0d),
+	[ec_sensor_temp_water_out] =
+		EC_SENSOR("Water_Out", hwmon_temp, 1, 0x01, 0x0b),
+};
+
 static const struct ec_sensor_info sensors_family_amd_500[] = {
 	[ec_sensor_temp_chipset] =
 		EC_SENSOR("Chipset", hwmon_temp, 1, 0x00, 0x3a),
@@ -185,6 +216,15 @@ struct ec_board_info {
 };
 
 static const struct ec_board_info board_info[] = {
+	{
+		.board_names = {"PRIME X470-PRO"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_CPU_OPT |
+			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+		.mutex_path = ACPI_GLOBAL_LOCK_PSEUDO_PATH,
+		.family = family_amd_400_series,
+	},
 	{
 		.board_names = {"PRIME X570-PRO"},
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
@@ -752,6 +792,9 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 	ec_data->board_info = pboard_info;
 
 	switch (ec_data->board_info->family) {
+	case family_amd_400_series:
+		ec_data->sensors_info = sensors_family_amd_400;
+		break;
 	case family_amd_500_series:
 		ec_data->sensors_info = sensors_family_amd_500;
 		break;
-- 
2.35.1