[PATCH v3 0/6] platform/x86: lenovo-wmi-{capdata,other}: Add HWMON for fan speed

platform-driver-x86.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v3 0/6] platform/x86: lenovo-wmi-{capdata,other}: Add HWMON for fan speed
@ 2025-10-31 15:51 Rong Zhang
  2025-10-31 15:51 ` [PATCH v3 1/6] platform/x86: lenovo-wmi-helpers: Convert returned 4B buffer into u32 Rong Zhang
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Rong Zhang @ 2025-10-31 15:51 UTC (permalink / raw)
  To: Mark Pearson, Derek J. Clark, Armin Wolf, Hans de Goede,
	Ilpo Järvinen
  Cc: Rong Zhang, Guenter Roeck, platform-driver-x86, linux-kernel,
	linux-hwmon

Lenovo WMI Other Mode interface also supports querying or setting fan
speed RPM. This capability is decribed by LENOVO_CAPABILITY_DATA_00.
Besides, LENOVO_FAN_TEST_DATA provides reference data for self-test of
cooling fans, including minimum and maximum fan speed RPM.

This patchset turns lenovo-wmi-capdata01 into a unified driver (now
named lenovo-wmi-capdata) for LENOVO_CAPABILITY_DATA_{00,01} and
LENOVO_FAN_TEST_DATA; then adds HWMON support for lenovo-wmi-other:

 - fanX_enable: enable/disable the fan (tunable)
 - fanX_input: current RPM
 - fanX_max: maximum RPM
 - fanX_min: minimum RPM
 - fanX_target: target RPM (tunable)

This implementation doesn't require all capability data to be available,
and is capable to expose interfaces accordingly:

 - Having LENOVO_CAPABILITY_DATA_00: exposes fanX_{enable,input,target}
 - Having LENOVO_CAPABILITY_DATA_01: exposes firmware_attributes
 - Having LENOVO_FAN_TEST_DATA: exposes fanX_{max,min}

Signed-off-by: Rong Zhang <i@rong.moe>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>

Changes in v2:
- Add a workaround for ACPI methods that return a 4B buffer for u32
  (thanks Armin Wolf)
- Fix function documentation (thanks kernel test bot)
- Reword documentation (thanks Derek J. Clark)
- Squash min/max reporting patch into the initial HWMON one (ditto)
- Query 0x04050000 for interface availability (ditto)
  - New parameter "expose_all_fans" to skip this check
- Enforce min/max RPM constraint on set (ditto)
  - New parameter "relax_fan_constraint" to disable this behavior
  - Drop parameter "ignore_fan_cap", superseded by the next one
  - New parameter "expose_all_fans" to expose fans w/o such data
- Assume auto mode on probe (ditto)
- Do not register HWMON device if no fan can be exposed
- fanX_target: Return -EBUSY instead of raw target value when fan stops
- Link to v1: https://lore.kernel.org/r/20251019210450.88830-1-i@rong.moe/

Changes in v3:
- Fix grammar (thanks Derek J. Clark)
- Link to v2: https://lore.kernel.org/r/20251030193955.107148-1-i@rong.moe/

Rong Zhang (6):
  platform/x86: lenovo-wmi-helpers: Convert returned 4B buffer into u32
  platform/x86: Rename lenovo-wmi-capdata01 to lenovo-wmi-capdata
  platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability
    Data
  platform/x86: lenovo-wmi-capdata: Add support for Capability Data 00
  platform/x86: lenovo-wmi-capdata: Add support for Fan Test Data
  platform/x86: lenovo-wmi-other: Add HWMON for fan speed RPM

 .../wmi/devices/lenovo-wmi-other.rst          |  43 +-
 drivers/platform/x86/lenovo/Kconfig           |   5 +-
 drivers/platform/x86/lenovo/Makefile          |   2 +-
 drivers/platform/x86/lenovo/wmi-capdata.c     | 545 ++++++++++++++++++
 drivers/platform/x86/lenovo/wmi-capdata.h     |  46 ++
 drivers/platform/x86/lenovo/wmi-capdata01.c   | 302 ----------
 drivers/platform/x86/lenovo/wmi-capdata01.h   |  25 -
 drivers/platform/x86/lenovo/wmi-helpers.c     |  21 +-
 drivers/platform/x86/lenovo/wmi-other.c       | 501 +++++++++++++++-
 9 files changed, 1134 insertions(+), 356 deletions(-)
 create mode 100644 drivers/platform/x86/lenovo/wmi-capdata.c
 create mode 100644 drivers/platform/x86/lenovo/wmi-capdata.h
 delete mode 100644 drivers/platform/x86/lenovo/wmi-capdata01.c
 delete mode 100644 drivers/platform/x86/lenovo/wmi-capdata01.h


base-commit: d127176862a93c4b3216bda533d2bee170af5e71
-- 
2.51.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 1/6] platform/x86: lenovo-wmi-helpers: Convert returned 4B buffer into u32
  2025-10-31 15:51 [PATCH v3 0/6] platform/x86: lenovo-wmi-{capdata,other}: Add HWMON for fan speed Rong Zhang
@ 2025-10-31 15:51 ` Rong Zhang
  2025-11-04 20:13   ` Armin Wolf
  2025-10-31 15:51 ` [PATCH v3 2/6] platform/x86: Rename lenovo-wmi-capdata01 to lenovo-wmi-capdata Rong Zhang
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Rong Zhang @ 2025-10-31 15:51 UTC (permalink / raw)
  To: Mark Pearson, Derek J. Clark, Armin Wolf, Hans de Goede,
	Ilpo Järvinen
  Cc: Rong Zhang, Guenter Roeck, platform-driver-x86, linux-kernel,
	linux-hwmon

The Windows WMI-ACPI driver converts all ACPI objects into a common
buffer format, so returning a buffer with four bytes will look like an
integer for WMI consumers under Windows.

Therefore, some devices may simply implement the corresponding ACPI
methods to always return a buffer. While lwmi_dev_evaluate_int() expects
an integer (u32), convert returned 4-byte buffer into u32 to support
these devices.

Suggested-by: Armin Wolf <W_Armin@gmx.de>
Link: https://lore.kernel.org/r/f1787927-b655-4321-b9d9-bc12353c72db@gmx.de/
Signed-off-by: Rong Zhang <i@rong.moe>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
Changes in v2:
- New patch (thanks Armin Wolf)
---
 drivers/platform/x86/lenovo/wmi-helpers.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/lenovo/wmi-helpers.c b/drivers/platform/x86/lenovo/wmi-helpers.c
index f6fef6296251..f3bc92ac505a 100644
--- a/drivers/platform/x86/lenovo/wmi-helpers.c
+++ b/drivers/platform/x86/lenovo/wmi-helpers.c
@@ -59,10 +59,25 @@ int lwmi_dev_evaluate_int(struct wmi_device *wdev, u8 instance, u32 method_id,
 		if (!ret_obj)
 			return -ENODATA;
 
-		if (ret_obj->type != ACPI_TYPE_INTEGER)
-			return -ENXIO;
+		switch (ret_obj->type) {
+		/*
+		 * The ACPI method may simply return a 4-byte buffer when a u32
+		 * integer is expected. This is valid on Windows as its WMI-ACPI
+		 * driver converts everything to a common buffer.
+		 */
+		case ACPI_TYPE_BUFFER: {
+			if (ret_obj->buffer.length != 4)
+				return -ENXIO;
 
-		*retval = (u32)ret_obj->integer.value;
+			*retval = *((u32 *)ret_obj->buffer.pointer);
+			return 0;
+		}
+		case ACPI_TYPE_INTEGER:
+			*retval = (u32)ret_obj->integer.value;
+			return 0;
+		default:
+			return -ENXIO;
+		}
 	}
 
 	return 0;
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 2/6] platform/x86: Rename lenovo-wmi-capdata01 to lenovo-wmi-capdata
  2025-10-31 15:51 [PATCH v3 0/6] platform/x86: lenovo-wmi-{capdata,other}: Add HWMON for fan speed Rong Zhang
  2025-10-31 15:51 ` [PATCH v3 1/6] platform/x86: lenovo-wmi-helpers: Convert returned 4B buffer into u32 Rong Zhang
@ 2025-10-31 15:51 ` Rong Zhang
  2025-10-31 15:51 ` [PATCH v3 3/6] platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability Data Rong Zhang
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Rong Zhang @ 2025-10-31 15:51 UTC (permalink / raw)
  To: Mark Pearson, Derek J. Clark, Armin Wolf, Hans de Goede,
	Ilpo Järvinen
  Cc: Rong Zhang, Guenter Roeck, platform-driver-x86, linux-kernel,
	linux-hwmon

Prepare for the upcoming changes to make it suitable to retrieve
and provide other Capability Data as well.

Signed-off-by: Rong Zhang <i@rong.moe>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
 drivers/platform/x86/lenovo/Kconfig           |   4 +-
 drivers/platform/x86/lenovo/Makefile          |   2 +-
 .../lenovo/{wmi-capdata01.c => wmi-capdata.c} | 124 +++++++++---------
 .../lenovo/{wmi-capdata01.h => wmi-capdata.h} |  10 +-
 drivers/platform/x86/lenovo/wmi-other.c       |  11 +-
 5 files changed, 78 insertions(+), 73 deletions(-)
 rename drivers/platform/x86/lenovo/{wmi-capdata01.c => wmi-capdata.c} (60%)
 rename drivers/platform/x86/lenovo/{wmi-capdata01.h => wmi-capdata.h} (60%)

diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
index d22b774e0236..fb96a0f908f0 100644
--- a/drivers/platform/x86/lenovo/Kconfig
+++ b/drivers/platform/x86/lenovo/Kconfig
@@ -233,7 +233,7 @@ config YT2_1380
 	  To compile this driver as a module, choose M here: the module will
 	  be called lenovo-yogabook.
 
-config LENOVO_WMI_DATA01
+config LENOVO_WMI_DATA
 	tristate
 	depends on ACPI_WMI
 
@@ -264,7 +264,7 @@ config LENOVO_WMI_TUNING
 	tristate "Lenovo Other Mode WMI Driver"
 	depends on ACPI_WMI
 	select FW_ATTR_CLASS
-	select LENOVO_WMI_DATA01
+	select LENOVO_WMI_DATA
 	select LENOVO_WMI_EVENTS
 	select LENOVO_WMI_HELPERS
 	help
diff --git a/drivers/platform/x86/lenovo/Makefile b/drivers/platform/x86/lenovo/Makefile
index 7b2128e3a214..29014d8c1376 100644
--- a/drivers/platform/x86/lenovo/Makefile
+++ b/drivers/platform/x86/lenovo/Makefile
@@ -12,7 +12,7 @@ lenovo-target-$(CONFIG_LENOVO_YMC)	+= ymc.o
 lenovo-target-$(CONFIG_YOGABOOK)	+= yogabook.o
 lenovo-target-$(CONFIG_YT2_1380)	+= yoga-tab2-pro-1380-fastcharger.o
 lenovo-target-$(CONFIG_LENOVO_WMI_CAMERA)	+= wmi-camera.o
-lenovo-target-$(CONFIG_LENOVO_WMI_DATA01)	+= wmi-capdata01.o
+lenovo-target-$(CONFIG_LENOVO_WMI_DATA)		+= wmi-capdata.o
 lenovo-target-$(CONFIG_LENOVO_WMI_EVENTS)	+= wmi-events.o
 lenovo-target-$(CONFIG_LENOVO_WMI_HELPERS)	+= wmi-helpers.o
 lenovo-target-$(CONFIG_LENOVO_WMI_GAMEZONE)	+= wmi-gamezone.o
diff --git a/drivers/platform/x86/lenovo/wmi-capdata01.c b/drivers/platform/x86/lenovo/wmi-capdata.c
similarity index 60%
rename from drivers/platform/x86/lenovo/wmi-capdata01.c
rename to drivers/platform/x86/lenovo/wmi-capdata.c
index fc7e3454e71d..c5e74b2bfeb3 100644
--- a/drivers/platform/x86/lenovo/wmi-capdata01.c
+++ b/drivers/platform/x86/lenovo/wmi-capdata.c
@@ -1,14 +1,17 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * Lenovo Capability Data 01 WMI Data Block driver.
+ * Lenovo Capability Data WMI Data Block driver.
  *
- * Lenovo Capability Data 01 provides information on tunable attributes used by
- * the "Other Mode" WMI interface. The data includes if the attribute is
- * supported by the hardware, the default_value, max_value, min_value, and step
- * increment. Each attribute has multiple pages, one for each of the thermal
- * modes managed by the Gamezone interface.
+ * Lenovo Capability Data provides information on tunable attributes used by
+ * the "Other Mode" WMI interface.
+ *
+ * Capability Data 01 includes if the attribute is supported by the hardware,
+ * and the default_value, max_value, min_value, and step increment. Each
+ * attribute has multiple pages, one for each of the thermal modes managed by
+ * the Gamezone interface.
  *
  * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@gmail.com>
+ *   - Initial implementation (formerly named lenovo-wmi-capdata01)
  */
 
 #include <linux/acpi.h>
@@ -26,55 +29,55 @@
 #include <linux/types.h>
 #include <linux/wmi.h>
 
-#include "wmi-capdata01.h"
+#include "wmi-capdata.h"
 
 #define LENOVO_CAPABILITY_DATA_01_GUID "7A8F5407-CB67-4D6E-B547-39B3BE018154"
 
 #define ACPI_AC_CLASS "ac_adapter"
 #define ACPI_AC_NOTIFY_STATUS 0x80
 
-struct lwmi_cd01_priv {
+struct lwmi_cd_priv {
 	struct notifier_block acpi_nb; /* ACPI events */
 	struct wmi_device *wdev;
-	struct cd01_list *list;
+	struct cd_list *list;
 };
 
-struct cd01_list {
+struct cd_list {
 	struct mutex list_mutex; /* list R/W mutex */
 	u8 count;
 	struct capdata01 data[];
 };
 
 /**
- * lwmi_cd01_component_bind() - Bind component to master device.
- * @cd01_dev: Pointer to the lenovo-wmi-capdata01 driver parent device.
+ * lwmi_cd_component_bind() - Bind component to master device.
+ * @cd_dev: Pointer to the lenovo-wmi-capdata driver parent device.
  * @om_dev: Pointer to the lenovo-wmi-other driver parent device.
- * @data: capdata01_list object pointer used to return the capability data.
+ * @data: cd_list object pointer used to return the capability data.
  *
- * On lenovo-wmi-other's master bind, provide a pointer to the local capdata01
- * list. This is used to call lwmi_cd01_get_data to look up attribute data
+ * On lenovo-wmi-other's master bind, provide a pointer to the local capdata
+ * list. This is used to call lwmi_cd*_get_data to look up attribute data
  * from the lenovo-wmi-other driver.
  *
  * Return: 0
  */
-static int lwmi_cd01_component_bind(struct device *cd01_dev,
-				    struct device *om_dev, void *data)
+static int lwmi_cd_component_bind(struct device *cd_dev,
+				  struct device *om_dev, void *data)
 {
-	struct lwmi_cd01_priv *priv = dev_get_drvdata(cd01_dev);
-	struct cd01_list **cd01_list = data;
+	struct lwmi_cd_priv *priv = dev_get_drvdata(cd_dev);
+	struct cd_list **cd_list = data;
 
-	*cd01_list = priv->list;
+	*cd_list = priv->list;
 
 	return 0;
 }
 
-static const struct component_ops lwmi_cd01_component_ops = {
-	.bind = lwmi_cd01_component_bind,
+static const struct component_ops lwmi_cd_component_ops = {
+	.bind = lwmi_cd_component_bind,
 };
 
 /**
  * lwmi_cd01_get_data - Get the data of the specified attribute
- * @list: The lenovo-wmi-capdata01 pointer to its cd01_list struct.
+ * @list: The lenovo-wmi-capdata pointer to its cd_list struct.
  * @attribute_id: The capdata attribute ID to be found.
  * @output: Pointer to a capdata01 struct to return the data.
  *
@@ -83,7 +86,7 @@ static const struct component_ops lwmi_cd01_component_ops = {
  *
  * Return: 0 on success, or -EINVAL.
  */
-int lwmi_cd01_get_data(struct cd01_list *list, u32 attribute_id, struct capdata01 *output)
+int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output)
 {
 	u8 idx;
 
@@ -97,17 +100,17 @@ int lwmi_cd01_get_data(struct cd01_list *list, u32 attribute_id, struct capdata0
 
 	return -EINVAL;
 }
-EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD01");
+EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
 
 /**
- * lwmi_cd01_cache() - Cache all WMI data block information
- * @priv: lenovo-wmi-capdata01 driver data.
+ * lwmi_cd_cache() - Cache all WMI data block information
+ * @priv: lenovo-wmi-capdata driver data.
  *
  * Loop through each WMI data block and cache the data.
  *
  * Return: 0 on success, or an error.
  */
-static int lwmi_cd01_cache(struct lwmi_cd01_priv *priv)
+static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
 {
 	int idx;
 
@@ -131,17 +134,17 @@ static int lwmi_cd01_cache(struct lwmi_cd01_priv *priv)
 }
 
 /**
- * lwmi_cd01_alloc() - Allocate a cd01_list struct in drvdata
- * @priv: lenovo-wmi-capdata01 driver data.
+ * lwmi_cd_alloc() - Allocate a cd_list struct in drvdata
+ * @priv: lenovo-wmi-capdata driver data.
  *
- * Allocate a cd01_list struct large enough to contain data from all WMI data
+ * Allocate a cd_list struct large enough to contain data from all WMI data
  * blocks provided by the interface.
  *
  * Return: 0 on success, or an error.
  */
-static int lwmi_cd01_alloc(struct lwmi_cd01_priv *priv)
+static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
 {
-	struct cd01_list *list;
+	struct cd_list *list;
 	size_t list_size;
 	int count, ret;
 
@@ -163,28 +166,28 @@ static int lwmi_cd01_alloc(struct lwmi_cd01_priv *priv)
 }
 
 /**
- * lwmi_cd01_setup() - Cache all WMI data block information
- * @priv: lenovo-wmi-capdata01 driver data.
+ * lwmi_cd_setup() - Cache all WMI data block information
+ * @priv: lenovo-wmi-capdata driver data.
  *
- * Allocate a cd01_list struct large enough to contain data from all WMI data
+ * Allocate a cd_list struct large enough to contain data from all WMI data
  * blocks provided by the interface. Then loop through each data block and
  * cache the data.
  *
  * Return: 0 on success, or an error code.
  */
-static int lwmi_cd01_setup(struct lwmi_cd01_priv *priv)
+static int lwmi_cd_setup(struct lwmi_cd_priv *priv)
 {
 	int ret;
 
-	ret = lwmi_cd01_alloc(priv);
+	ret = lwmi_cd_alloc(priv);
 	if (ret)
 		return ret;
 
-	return lwmi_cd01_cache(priv);
+	return lwmi_cd_cache(priv);
 }
 
 /**
- * lwmi_cd01_notifier_call() - Call method for lenovo-wmi-capdata01 driver notifier.
+ * lwmi_cd01_notifier_call() - Call method for cd01 notifier.
  * block call chain.
  * @nb: The notifier_block registered to lenovo-wmi-events driver.
  * @action: Unused.
@@ -199,17 +202,17 @@ static int lwmi_cd01_notifier_call(struct notifier_block *nb, unsigned long acti
 				   void *data)
 {
 	struct acpi_bus_event *event = data;
-	struct lwmi_cd01_priv *priv;
+	struct lwmi_cd_priv *priv;
 	int ret;
 
 	if (strcmp(event->device_class, ACPI_AC_CLASS) != 0)
 		return NOTIFY_DONE;
 
-	priv = container_of(nb, struct lwmi_cd01_priv, acpi_nb);
+	priv = container_of(nb, struct lwmi_cd_priv, acpi_nb);
 
 	switch (event->type) {
 	case ACPI_AC_NOTIFY_STATUS:
-		ret = lwmi_cd01_cache(priv);
+		ret = lwmi_cd_cache(priv);
 		if (ret)
 			return NOTIFY_BAD;
 
@@ -230,10 +233,9 @@ static void lwmi_cd01_unregister(void *data)
 	unregister_acpi_notifier(acpi_nb);
 }
 
-static int lwmi_cd01_probe(struct wmi_device *wdev, const void *context)
-
+static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
 {
-	struct lwmi_cd01_priv *priv;
+	struct lwmi_cd_priv *priv;
 	int ret;
 
 	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -243,7 +245,7 @@ static int lwmi_cd01_probe(struct wmi_device *wdev, const void *context)
 	priv->wdev = wdev;
 	dev_set_drvdata(&wdev->dev, priv);
 
-	ret = lwmi_cd01_setup(priv);
+	ret = lwmi_cd_setup(priv);
 	if (ret)
 		return ret;
 
@@ -257,27 +259,27 @@ static int lwmi_cd01_probe(struct wmi_device *wdev, const void *context)
 	if (ret)
 		return ret;
 
-	return component_add(&wdev->dev, &lwmi_cd01_component_ops);
+	return component_add(&wdev->dev, &lwmi_cd_component_ops);
 }
 
-static void lwmi_cd01_remove(struct wmi_device *wdev)
+static void lwmi_cd_remove(struct wmi_device *wdev)
 {
-	component_del(&wdev->dev, &lwmi_cd01_component_ops);
+	component_del(&wdev->dev, &lwmi_cd_component_ops);
 }
 
-static const struct wmi_device_id lwmi_cd01_id_table[] = {
+static const struct wmi_device_id lwmi_cd_id_table[] = {
 	{ LENOVO_CAPABILITY_DATA_01_GUID, NULL },
 	{}
 };
 
-static struct wmi_driver lwmi_cd01_driver = {
+static struct wmi_driver lwmi_cd_driver = {
 	.driver = {
-		.name = "lenovo_wmi_cd01",
+		.name = "lenovo_wmi_cd",
 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
 	},
-	.id_table = lwmi_cd01_id_table,
-	.probe = lwmi_cd01_probe,
-	.remove = lwmi_cd01_remove,
+	.id_table = lwmi_cd_id_table,
+	.probe = lwmi_cd_probe,
+	.remove = lwmi_cd_remove,
 	.no_singleton = true,
 };
 
@@ -290,13 +292,13 @@ static struct wmi_driver lwmi_cd01_driver = {
  */
 int lwmi_cd01_match(struct device *dev, void *data)
 {
-	return dev->driver == &lwmi_cd01_driver.driver;
+	return dev->driver == &lwmi_cd_driver.driver;
 }
-EXPORT_SYMBOL_NS_GPL(lwmi_cd01_match, "LENOVO_WMI_CD01");
+EXPORT_SYMBOL_NS_GPL(lwmi_cd01_match, "LENOVO_WMI_CD");
 
-module_wmi_driver(lwmi_cd01_driver);
+module_wmi_driver(lwmi_cd_driver);
 
-MODULE_DEVICE_TABLE(wmi, lwmi_cd01_id_table);
+MODULE_DEVICE_TABLE(wmi, lwmi_cd_id_table);
 MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
-MODULE_DESCRIPTION("Lenovo Capability Data 01 WMI Driver");
+MODULE_DESCRIPTION("Lenovo Capability Data WMI Driver");
 MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/lenovo/wmi-capdata01.h b/drivers/platform/x86/lenovo/wmi-capdata.h
similarity index 60%
rename from drivers/platform/x86/lenovo/wmi-capdata01.h
rename to drivers/platform/x86/lenovo/wmi-capdata.h
index bd06c5751f68..2a4746e38ad4 100644
--- a/drivers/platform/x86/lenovo/wmi-capdata01.h
+++ b/drivers/platform/x86/lenovo/wmi-capdata.h
@@ -2,13 +2,13 @@
 
 /* Copyright (C) 2025 Derek J. Clark <derekjohn.clark@gmail.com> */
 
-#ifndef _LENOVO_WMI_CAPDATA01_H_
-#define _LENOVO_WMI_CAPDATA01_H_
+#ifndef _LENOVO_WMI_CAPDATA_H_
+#define _LENOVO_WMI_CAPDATA_H_
 
 #include <linux/types.h>
 
 struct device;
-struct cd01_list;
+struct cd_list;
 
 struct capdata01 {
 	u32 id;
@@ -19,7 +19,7 @@ struct capdata01 {
 	u32 max_value;
 };
 
-int lwmi_cd01_get_data(struct cd01_list *list, u32 attribute_id, struct capdata01 *output);
+int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output);
 int lwmi_cd01_match(struct device *dev, void *data);
 
-#endif /* !_LENOVO_WMI_CAPDATA01_H_ */
+#endif /* !_LENOVO_WMI_CAPDATA_H_ */
diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
index 2a960b278f11..c6dc1b4cff84 100644
--- a/drivers/platform/x86/lenovo/wmi-other.c
+++ b/drivers/platform/x86/lenovo/wmi-other.c
@@ -34,7 +34,7 @@
 #include <linux/types.h>
 #include <linux/wmi.h>
 
-#include "wmi-capdata01.h"
+#include "wmi-capdata.h"
 #include "wmi-events.h"
 #include "wmi-gamezone.h"
 #include "wmi-helpers.h"
@@ -74,7 +74,10 @@ enum attribute_property {
 
 struct lwmi_om_priv {
 	struct component_master_ops *ops;
-	struct cd01_list *cd01_list; /* only valid after capdata01 bind */
+
+	/* only valid after capdata bind */
+	struct cd_list *cd01_list;
+
 	struct device *fw_attr_dev;
 	struct kset *fw_attr_kset;
 	struct notifier_block nb;
@@ -576,7 +579,7 @@ static void lwmi_om_fw_attr_remove(struct lwmi_om_priv *priv)
 static int lwmi_om_master_bind(struct device *dev)
 {
 	struct lwmi_om_priv *priv = dev_get_drvdata(dev);
-	struct cd01_list *tmp_list;
+	struct cd_list *tmp_list;
 	int ret;
 
 	ret = component_bind_all(dev, &tmp_list);
@@ -657,7 +660,7 @@ static struct wmi_driver lwmi_other_driver = {
 
 module_wmi_driver(lwmi_other_driver);
 
-MODULE_IMPORT_NS("LENOVO_WMI_CD01");
+MODULE_IMPORT_NS("LENOVO_WMI_CD");
 MODULE_IMPORT_NS("LENOVO_WMI_HELPERS");
 MODULE_DEVICE_TABLE(wmi, lwmi_other_id_table);
 MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 3/6] platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability Data
  2025-10-31 15:51 [PATCH v3 0/6] platform/x86: lenovo-wmi-{capdata,other}: Add HWMON for fan speed Rong Zhang
  2025-10-31 15:51 ` [PATCH v3 1/6] platform/x86: lenovo-wmi-helpers: Convert returned 4B buffer into u32 Rong Zhang
  2025-10-31 15:51 ` [PATCH v3 2/6] platform/x86: Rename lenovo-wmi-capdata01 to lenovo-wmi-capdata Rong Zhang
@ 2025-10-31 15:51 ` Rong Zhang
  2025-11-04 20:20   ` Armin Wolf
  2025-10-31 15:51 ` [PATCH v3 4/6] platform/x86: lenovo-wmi-capdata: Add support for Capability Data 00 Rong Zhang
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Rong Zhang @ 2025-10-31 15:51 UTC (permalink / raw)
  To: Mark Pearson, Derek J. Clark, Armin Wolf, Hans de Goede,
	Ilpo Järvinen
  Cc: Rong Zhang, Guenter Roeck, platform-driver-x86, linux-kernel,
	linux-hwmon

The current implementation are heavily bound to capdata01. Rewrite it so
that it is suitable to utilize other Capability Data as well.

No functional change intended.

Signed-off-by: Rong Zhang <i@rong.moe>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
Changes in v2:
- Fix function parameter 'type' not described in 'lwmi_cd_match' (thanks
  kernel test bot)
---
 drivers/platform/x86/lenovo/wmi-capdata.c | 208 +++++++++++++++++-----
 drivers/platform/x86/lenovo/wmi-capdata.h |   7 +-
 drivers/platform/x86/lenovo/wmi-other.c   |  27 ++-
 3 files changed, 190 insertions(+), 52 deletions(-)

diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
index c5e74b2bfeb3..1f7fc09b7c3f 100644
--- a/drivers/platform/x86/lenovo/wmi-capdata.c
+++ b/drivers/platform/x86/lenovo/wmi-capdata.c
@@ -12,8 +12,13 @@
  *
  * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@gmail.com>
  *   - Initial implementation (formerly named lenovo-wmi-capdata01)
+ *
+ * Copyright (C) 2025 Rong Zhang <i@rong.moe>
+ *   - Unified implementation
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/acpi.h>
 #include <linux/cleanup.h>
 #include <linux/component.h>
@@ -36,6 +41,25 @@
 #define ACPI_AC_CLASS "ac_adapter"
 #define ACPI_AC_NOTIFY_STATUS 0x80
 
+enum lwmi_cd_type {
+	LENOVO_CAPABILITY_DATA_01,
+};
+
+#define LWMI_CD_TABLE_ITEM(_type)		\
+	[_type] = {				\
+		.guid_string = _type##_GUID,	\
+		.name = #_type,			\
+		.type = _type,			\
+	}
+
+static const struct lwmi_cd_info {
+	const char *guid_string;
+	const char *name;
+	enum lwmi_cd_type type;
+} lwmi_cd_table[] = {
+	LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_01),
+};
+
 struct lwmi_cd_priv {
 	struct notifier_block acpi_nb; /* ACPI events */
 	struct wmi_device *wdev;
@@ -44,15 +68,19 @@ struct lwmi_cd_priv {
 
 struct cd_list {
 	struct mutex list_mutex; /* list R/W mutex */
+	enum lwmi_cd_type type;
 	u8 count;
-	struct capdata01 data[];
+
+	union {
+		DECLARE_FLEX_ARRAY(struct capdata01, cd01);
+	};
 };
 
 /**
  * lwmi_cd_component_bind() - Bind component to master device.
  * @cd_dev: Pointer to the lenovo-wmi-capdata driver parent device.
  * @om_dev: Pointer to the lenovo-wmi-other driver parent device.
- * @data: cd_list object pointer used to return the capability data.
+ * @data: lwmi_cd_binder object pointer used to return the capability data.
  *
  * On lenovo-wmi-other's master bind, provide a pointer to the local capdata
  * list. This is used to call lwmi_cd*_get_data to look up attribute data
@@ -64,9 +92,15 @@ static int lwmi_cd_component_bind(struct device *cd_dev,
 				  struct device *om_dev, void *data)
 {
 	struct lwmi_cd_priv *priv = dev_get_drvdata(cd_dev);
-	struct cd_list **cd_list = data;
+	struct lwmi_cd_binder *binder = data;
 
-	*cd_list = priv->list;
+	switch (priv->list->type) {
+	case LENOVO_CAPABILITY_DATA_01:
+		binder->cd01_list = priv->list;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -76,30 +110,33 @@ static const struct component_ops lwmi_cd_component_ops = {
 };
 
 /**
- * lwmi_cd01_get_data - Get the data of the specified attribute
+ * lwmi_cd*_get_data - Get the data of the specified attribute
  * @list: The lenovo-wmi-capdata pointer to its cd_list struct.
  * @attribute_id: The capdata attribute ID to be found.
- * @output: Pointer to a capdata01 struct to return the data.
+ * @output: Pointer to a capdata* struct to return the data.
  *
- * Retrieves the capability data 01 struct pointer for the given
- * attribute for its specified thermal mode.
+ * Retrieves the capability data struct pointer for the given
+ * attribute.
  *
  * Return: 0 on success, or -EINVAL.
  */
-int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output)
-{
-	u8 idx;
-
-	guard(mutex)(&list->list_mutex);
-	for (idx = 0; idx < list->count; idx++) {
-		if (list->data[idx].id != attribute_id)
-			continue;
-		memcpy(output, &list->data[idx], sizeof(list->data[idx]));
-		return 0;
+#define DEF_LWMI_CDXX_GET_DATA(_cdxx, _cd_type, _output_t)					\
+	int lwmi_##_cdxx##_get_data(struct cd_list *list, u32 attribute_id, _output_t *output)	\
+	{											\
+		u8 idx;										\
+		if (WARN_ON(list->type != _cd_type))						\
+			return -EINVAL;								\
+		guard(mutex)(&list->list_mutex);						\
+		for (idx = 0; idx < list->count; idx++) {					\
+			if (list->_cdxx[idx].id != attribute_id)				\
+				continue;							\
+			memcpy(output, &list->_cdxx[idx], sizeof(list->_cdxx[idx]));		\
+			return 0;								\
+		}										\
+		return -EINVAL;									\
 	}
 
-	return -EINVAL;
-}
+DEF_LWMI_CDXX_GET_DATA(cd01, LENOVO_CAPABILITY_DATA_01, struct capdata01);
 EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
 
 /**
@@ -112,10 +149,21 @@ EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
  */
 static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
 {
+	size_t size;
 	int idx;
+	void *p;
+
+	switch (priv->list->type) {
+	case LENOVO_CAPABILITY_DATA_01:
+		p = &priv->list->cd01[0];
+		size = sizeof(priv->list->cd01[0]);
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	guard(mutex)(&priv->list->list_mutex);
-	for (idx = 0; idx < priv->list->count; idx++) {
+	for (idx = 0; idx < priv->list->count; idx++, p += size) {
 		union acpi_object *ret_obj __free(kfree) = NULL;
 
 		ret_obj = wmidev_block_query(priv->wdev, idx);
@@ -123,11 +171,10 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
 			return -ENODEV;
 
 		if (ret_obj->type != ACPI_TYPE_BUFFER ||
-		    ret_obj->buffer.length < sizeof(priv->list->data[idx]))
+		    ret_obj->buffer.length < size)
 			continue;
 
-		memcpy(&priv->list->data[idx], ret_obj->buffer.pointer,
-		       ret_obj->buffer.length);
+		memcpy(p, ret_obj->buffer.pointer, size);
 	}
 
 	return 0;
@@ -136,20 +183,28 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
 /**
  * lwmi_cd_alloc() - Allocate a cd_list struct in drvdata
  * @priv: lenovo-wmi-capdata driver data.
+ * @type: The type of capability data.
  *
  * Allocate a cd_list struct large enough to contain data from all WMI data
  * blocks provided by the interface.
  *
  * Return: 0 on success, or an error.
  */
-static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
+static int lwmi_cd_alloc(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
 {
 	struct cd_list *list;
 	size_t list_size;
 	int count, ret;
 
 	count = wmidev_instance_count(priv->wdev);
-	list_size = struct_size(list, data, count);
+
+	switch (type) {
+	case LENOVO_CAPABILITY_DATA_01:
+		list_size = struct_size(list, cd01, count);
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	list = devm_kzalloc(&priv->wdev->dev, list_size, GFP_KERNEL);
 	if (!list)
@@ -159,6 +214,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
 	if (ret)
 		return ret;
 
+	list->type = type;
 	list->count = count;
 	priv->list = list;
 
@@ -168,6 +224,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
 /**
  * lwmi_cd_setup() - Cache all WMI data block information
  * @priv: lenovo-wmi-capdata driver data.
+ * @type: The type of capability data.
  *
  * Allocate a cd_list struct large enough to contain data from all WMI data
  * blocks provided by the interface. Then loop through each data block and
@@ -175,11 +232,11 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
  *
  * Return: 0 on success, or an error code.
  */
-static int lwmi_cd_setup(struct lwmi_cd_priv *priv)
+static int lwmi_cd_setup(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
 {
 	int ret;
 
-	ret = lwmi_cd_alloc(priv);
+	ret = lwmi_cd_alloc(priv, type);
 	if (ret)
 		return ret;
 
@@ -235,9 +292,13 @@ static void lwmi_cd01_unregister(void *data)
 
 static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
 {
+	const struct lwmi_cd_info *info = context;
 	struct lwmi_cd_priv *priv;
 	int ret;
 
+	if (!info)
+		return -EINVAL;
+
 	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -245,21 +306,34 @@ static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
 	priv->wdev = wdev;
 	dev_set_drvdata(&wdev->dev, priv);
 
-	ret = lwmi_cd_setup(priv);
+	ret = lwmi_cd_setup(priv, info->type);
 	if (ret)
-		return ret;
+		goto out;
 
-	priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
+	if (info->type == LENOVO_CAPABILITY_DATA_01) {
+		priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
 
-	ret = register_acpi_notifier(&priv->acpi_nb);
-	if (ret)
-		return ret;
+		ret = register_acpi_notifier(&priv->acpi_nb);
+		if (ret)
+			goto out;
 
-	ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister, &priv->acpi_nb);
-	if (ret)
-		return ret;
+		ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister,
+					       &priv->acpi_nb);
+		if (ret)
+			goto out;
+	}
+
+	ret = component_add(&wdev->dev, &lwmi_cd_component_ops);
 
-	return component_add(&wdev->dev, &lwmi_cd_component_ops);
+out:
+	if (ret) {
+		dev_err(&wdev->dev, "failed to register %s: %d\n",
+			info->name, ret);
+	} else {
+		dev_info(&wdev->dev, "registered %s with %u items\n",
+			 info->name, priv->list->count);
+	}
+	return ret;
 }
 
 static void lwmi_cd_remove(struct wmi_device *wdev)
@@ -267,8 +341,12 @@ static void lwmi_cd_remove(struct wmi_device *wdev)
 	component_del(&wdev->dev, &lwmi_cd_component_ops);
 }
 
+#define LWMI_CD_WDEV_ID(_type)				\
+	.guid_string = _type##_GUID,			\
+	.context = &lwmi_cd_table[_type]
+
 static const struct wmi_device_id lwmi_cd_id_table[] = {
-	{ LENOVO_CAPABILITY_DATA_01_GUID, NULL },
+	{ LWMI_CD_WDEV_ID(LENOVO_CAPABILITY_DATA_01) },
 	{}
 };
 
@@ -284,21 +362,61 @@ static struct wmi_driver lwmi_cd_driver = {
 };
 
 /**
- * lwmi_cd01_match() - Match rule for the master driver.
- * @dev: Pointer to the capability data 01 parent device.
- * @data: Unused void pointer for passing match criteria.
+ * lwmi_cd_match() - Match rule for the master driver.
+ * @dev: Pointer to the capability data parent device.
+ * @type: Pointer to capability data type (enum lwmi_cd_type *) to match.
  *
  * Return: int.
  */
-int lwmi_cd01_match(struct device *dev, void *data)
+static int lwmi_cd_match(struct device *dev, void *type)
+{
+	struct lwmi_cd_priv *priv;
+
+	if (dev->driver != &lwmi_cd_driver.driver)
+		return false;
+
+	priv = dev_get_drvdata(dev);
+	return priv->list->type == *(enum lwmi_cd_type *)type;
+}
+
+/**
+ * lwmi_cd_match_add_all() - Add all match rule for the master driver.
+ * @master: Pointer to the master device.
+ * @matchptr: Pointer to the returned component_match pointer.
+ *
+ * Adds all component matches to the list stored in @matchptr for the @master
+ * device. @matchptr must be initialized to NULL. This matches all available
+ * capdata types on the machine.
+ */
+void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr)
 {
-	return dev->driver == &lwmi_cd_driver.driver;
+	int i;
+
+	if (WARN_ON(*matchptr))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(lwmi_cd_table); i++) {
+		if (!lwmi_cd_table[i].guid_string ||
+		    !wmi_has_guid(lwmi_cd_table[i].guid_string))
+			continue;
+
+		component_match_add(master, matchptr, lwmi_cd_match,
+				    (void *)&lwmi_cd_table[i].type);
+		if (IS_ERR(matchptr))
+			return;
+	}
+
+	if (!*matchptr) {
+		pr_warn("a master driver requested capability data, but nothing is available\n");
+		*matchptr = ERR_PTR(-ENODEV);
+	}
 }
-EXPORT_SYMBOL_NS_GPL(lwmi_cd01_match, "LENOVO_WMI_CD");
+EXPORT_SYMBOL_NS_GPL(lwmi_cd_match_add_all, "LENOVO_WMI_CD");
 
 module_wmi_driver(lwmi_cd_driver);
 
 MODULE_DEVICE_TABLE(wmi, lwmi_cd_id_table);
 MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
+MODULE_AUTHOR("Rong Zhang <i@rong.moe>");
 MODULE_DESCRIPTION("Lenovo Capability Data WMI Driver");
 MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/lenovo/wmi-capdata.h b/drivers/platform/x86/lenovo/wmi-capdata.h
index 2a4746e38ad4..1e5fce7836cb 100644
--- a/drivers/platform/x86/lenovo/wmi-capdata.h
+++ b/drivers/platform/x86/lenovo/wmi-capdata.h
@@ -7,6 +7,7 @@
 
 #include <linux/types.h>
 
+struct component_match;
 struct device;
 struct cd_list;
 
@@ -19,7 +20,11 @@ struct capdata01 {
 	u32 max_value;
 };
 
+struct lwmi_cd_binder {
+	struct cd_list *cd01_list;
+};
+
 int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output);
-int lwmi_cd01_match(struct device *dev, void *data);
+void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr);
 
 #endif /* !_LENOVO_WMI_CAPDATA_H_ */
diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
index c6dc1b4cff84..20c6ff0be37a 100644
--- a/drivers/platform/x86/lenovo/wmi-other.c
+++ b/drivers/platform/x86/lenovo/wmi-other.c
@@ -579,14 +579,14 @@ static void lwmi_om_fw_attr_remove(struct lwmi_om_priv *priv)
 static int lwmi_om_master_bind(struct device *dev)
 {
 	struct lwmi_om_priv *priv = dev_get_drvdata(dev);
-	struct cd_list *tmp_list;
+	struct lwmi_cd_binder binder = { 0 };
 	int ret;
 
-	ret = component_bind_all(dev, &tmp_list);
+	ret = component_bind_all(dev, &binder);
 	if (ret)
 		return ret;
 
-	priv->cd01_list = tmp_list;
+	priv->cd01_list = binder.cd01_list;
 	if (!priv->cd01_list)
 		return -ENODEV;
 
@@ -618,6 +618,7 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
 {
 	struct component_match *master_match = NULL;
 	struct lwmi_om_priv *priv;
+	int ret;
 
 	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -626,12 +627,26 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
 	priv->wdev = wdev;
 	dev_set_drvdata(&wdev->dev, priv);
 
-	component_match_add(&wdev->dev, &master_match, lwmi_cd01_match, NULL);
+	lwmi_cd_match_add_all(&wdev->dev, &master_match);
 	if (IS_ERR(master_match))
 		return PTR_ERR(master_match);
 
-	return component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
-					       master_match);
+	ret = component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
+					      master_match);
+	if (ret)
+		return ret;
+
+	if (likely(component_master_is_bound(&wdev->dev, &lwmi_om_master_ops)))
+		return 0;
+
+	/*
+	 * The bind callbacks of both master and components were never called in
+	 * this case - this driver won't work at all. Failing...
+	 */
+	dev_err(&wdev->dev, "unbound master; is any component failing to be probed?");
+
+	component_master_del(&wdev->dev, &lwmi_om_master_ops);
+	return -EXDEV;
 }
 
 static void lwmi_other_remove(struct wmi_device *wdev)
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 4/6] platform/x86: lenovo-wmi-capdata: Add support for Capability Data 00
  2025-10-31 15:51 [PATCH v3 0/6] platform/x86: lenovo-wmi-{capdata,other}: Add HWMON for fan speed Rong Zhang
                   ` (2 preceding siblings ...)
  2025-10-31 15:51 ` [PATCH v3 3/6] platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability Data Rong Zhang
@ 2025-10-31 15:51 ` Rong Zhang
  2025-10-31 15:51 ` [PATCH v3 5/6] platform/x86: lenovo-wmi-capdata: Add support for Fan Test Data Rong Zhang
  2025-10-31 15:51 ` [PATCH v3 6/6] platform/x86: lenovo-wmi-other: Add HWMON for fan speed RPM Rong Zhang
  5 siblings, 0 replies; 14+ messages in thread
From: Rong Zhang @ 2025-10-31 15:51 UTC (permalink / raw)
  To: Mark Pearson, Derek J. Clark, Armin Wolf, Hans de Goede,
	Ilpo Järvinen
  Cc: Rong Zhang, Guenter Roeck, platform-driver-x86, linux-kernel,
	linux-hwmon

Add support for LENOVO_CAPABILITY_DATA_00 WMI data block that comes on
"Other Mode" enabled hardware. Provides an interface for querying if a
given attribute is supported by the hardware, as well as its default
value.

Signed-off-by: Rong Zhang <i@rong.moe>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
Changes in v2:
- Reword documentation (thanks Derek J. Clark)
---
 .../wmi/devices/lenovo-wmi-other.rst          | 15 ++++++++++---
 drivers/platform/x86/lenovo/wmi-capdata.c     | 21 +++++++++++++++++++
 drivers/platform/x86/lenovo/wmi-capdata.h     |  8 +++++++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/Documentation/wmi/devices/lenovo-wmi-other.rst b/Documentation/wmi/devices/lenovo-wmi-other.rst
index d7928b8dfb4b..fcad595d49af 100644
--- a/Documentation/wmi/devices/lenovo-wmi-other.rst
+++ b/Documentation/wmi/devices/lenovo-wmi-other.rst
@@ -31,13 +31,22 @@ under the following path:
 
   /sys/class/firmware-attributes/lenovo-wmi-other/attributes/<attribute>/
 
+LENOVO_CAPABILITY_DATA_00
+-------------------------
+
+WMI GUID ``362A3AFE-3D96-4665-8530-96DAD5BB300E``
+
+The LENOVO_CAPABILITY_DATA_00 interface provides various information that
+does not rely on the gamezone thermal mode.
+
 LENOVO_CAPABILITY_DATA_01
 -------------------------
 
 WMI GUID ``7A8F5407-CB67-4D6E-B547-39B3BE018154``
 
-The LENOVO_CAPABILITY_DATA_01 interface provides information on various
-power limits of integrated CPU and GPU components.
+The LENOVO_CAPABILITY_DATA_01 interface provides various information that
+relies on the gamezone thermal mode, including power limits of integrated
+CPU and GPU components.
 
 Each attribute has the following properties:
  - current_value
@@ -48,7 +57,7 @@ Each attribute has the following properties:
  - scalar_increment
  - type
 
-The following attributes are implemented:
+The following firmware-attributes are implemented:
  - ppt_pl1_spl: Platform Profile Tracking Sustained Power Limit
  - ppt_pl2_sppt: Platform Profile Tracking Slow Package Power Tracking
  - ppt_pl3_fppt: Platform Profile Tracking Fast Package Power Tracking
diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
index 1f7fc09b7c3f..e8ec30701d88 100644
--- a/drivers/platform/x86/lenovo/wmi-capdata.c
+++ b/drivers/platform/x86/lenovo/wmi-capdata.c
@@ -5,6 +5,9 @@
  * Lenovo Capability Data provides information on tunable attributes used by
  * the "Other Mode" WMI interface.
  *
+ * Capability Data 00 includes if the attribute is supported by the hardware,
+ * and the default_value. All attributes are independent of thermal modes.
+ *
  * Capability Data 01 includes if the attribute is supported by the hardware,
  * and the default_value, max_value, min_value, and step increment. Each
  * attribute has multiple pages, one for each of the thermal modes managed by
@@ -36,12 +39,14 @@
 
 #include "wmi-capdata.h"
 
+#define LENOVO_CAPABILITY_DATA_00_GUID "362A3AFE-3D96-4665-8530-96DAD5BB300E"
 #define LENOVO_CAPABILITY_DATA_01_GUID "7A8F5407-CB67-4D6E-B547-39B3BE018154"
 
 #define ACPI_AC_CLASS "ac_adapter"
 #define ACPI_AC_NOTIFY_STATUS 0x80
 
 enum lwmi_cd_type {
+	LENOVO_CAPABILITY_DATA_00,
 	LENOVO_CAPABILITY_DATA_01,
 };
 
@@ -57,6 +62,7 @@ static const struct lwmi_cd_info {
 	const char *name;
 	enum lwmi_cd_type type;
 } lwmi_cd_table[] = {
+	LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_00),
 	LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_01),
 };
 
@@ -72,6 +78,7 @@ struct cd_list {
 	u8 count;
 
 	union {
+		DECLARE_FLEX_ARRAY(struct capdata00, cd00);
 		DECLARE_FLEX_ARRAY(struct capdata01, cd01);
 	};
 };
@@ -95,6 +102,9 @@ static int lwmi_cd_component_bind(struct device *cd_dev,
 	struct lwmi_cd_binder *binder = data;
 
 	switch (priv->list->type) {
+	case LENOVO_CAPABILITY_DATA_00:
+		binder->cd00_list = priv->list;
+		break;
 	case LENOVO_CAPABILITY_DATA_01:
 		binder->cd01_list = priv->list;
 		break;
@@ -136,6 +146,9 @@ static const struct component_ops lwmi_cd_component_ops = {
 		return -EINVAL;									\
 	}
 
+DEF_LWMI_CDXX_GET_DATA(cd00, LENOVO_CAPABILITY_DATA_00, struct capdata00);
+EXPORT_SYMBOL_NS_GPL(lwmi_cd00_get_data, "LENOVO_WMI_CD");
+
 DEF_LWMI_CDXX_GET_DATA(cd01, LENOVO_CAPABILITY_DATA_01, struct capdata01);
 EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
 
@@ -154,6 +167,10 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
 	void *p;
 
 	switch (priv->list->type) {
+	case LENOVO_CAPABILITY_DATA_00:
+		p = &priv->list->cd00[0];
+		size = sizeof(priv->list->cd00[0]);
+		break;
 	case LENOVO_CAPABILITY_DATA_01:
 		p = &priv->list->cd01[0];
 		size = sizeof(priv->list->cd01[0]);
@@ -199,6 +216,9 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
 	count = wmidev_instance_count(priv->wdev);
 
 	switch (type) {
+	case LENOVO_CAPABILITY_DATA_00:
+		list_size = struct_size(list, cd00, count);
+		break;
 	case LENOVO_CAPABILITY_DATA_01:
 		list_size = struct_size(list, cd01, count);
 		break;
@@ -346,6 +366,7 @@ static void lwmi_cd_remove(struct wmi_device *wdev)
 	.context = &lwmi_cd_table[_type]
 
 static const struct wmi_device_id lwmi_cd_id_table[] = {
+	{ LWMI_CD_WDEV_ID(LENOVO_CAPABILITY_DATA_00) },
 	{ LWMI_CD_WDEV_ID(LENOVO_CAPABILITY_DATA_01) },
 	{}
 };
diff --git a/drivers/platform/x86/lenovo/wmi-capdata.h b/drivers/platform/x86/lenovo/wmi-capdata.h
index 1e5fce7836cb..a6f0cb006e74 100644
--- a/drivers/platform/x86/lenovo/wmi-capdata.h
+++ b/drivers/platform/x86/lenovo/wmi-capdata.h
@@ -11,6 +11,12 @@ struct component_match;
 struct device;
 struct cd_list;
 
+struct capdata00 {
+	u32 id;
+	u32 supported;
+	u32 default_value;
+};
+
 struct capdata01 {
 	u32 id;
 	u32 supported;
@@ -21,9 +27,11 @@ struct capdata01 {
 };
 
 struct lwmi_cd_binder {
+	struct cd_list *cd00_list;
 	struct cd_list *cd01_list;
 };
 
+int lwmi_cd00_get_data(struct cd_list *list, u32 attribute_id, struct capdata00 *output);
 int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output);
 void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr);
 
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 5/6] platform/x86: lenovo-wmi-capdata: Add support for Fan Test Data
  2025-10-31 15:51 [PATCH v3 0/6] platform/x86: lenovo-wmi-{capdata,other}: Add HWMON for fan speed Rong Zhang
                   ` (3 preceding siblings ...)
  2025-10-31 15:51 ` [PATCH v3 4/6] platform/x86: lenovo-wmi-capdata: Add support for Capability Data 00 Rong Zhang
@ 2025-10-31 15:51 ` Rong Zhang
  2025-10-31 15:51 ` [PATCH v3 6/6] platform/x86: lenovo-wmi-other: Add HWMON for fan speed RPM Rong Zhang
  5 siblings, 0 replies; 14+ messages in thread
From: Rong Zhang @ 2025-10-31 15:51 UTC (permalink / raw)
  To: Mark Pearson, Derek J. Clark, Armin Wolf, Hans de Goede,
	Ilpo Järvinen
  Cc: Rong Zhang, Guenter Roeck, platform-driver-x86, linux-kernel,
	linux-hwmon

Add support for LENOVO_FAN_TEST_DATA WMI data block. Provides an
interface for querying the min/max fan speed RPM (reference data) of a
given fan ID.

Signed-off-by: Rong Zhang <i@rong.moe>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
Changes in v2:
- Reword documentation
---
 .../wmi/devices/lenovo-wmi-other.rst          |  17 +++
 drivers/platform/x86/lenovo/wmi-capdata.c     | 102 ++++++++++++++++++
 drivers/platform/x86/lenovo/wmi-capdata.h     |   8 ++
 3 files changed, 127 insertions(+)

diff --git a/Documentation/wmi/devices/lenovo-wmi-other.rst b/Documentation/wmi/devices/lenovo-wmi-other.rst
index fcad595d49af..821282e07d93 100644
--- a/Documentation/wmi/devices/lenovo-wmi-other.rst
+++ b/Documentation/wmi/devices/lenovo-wmi-other.rst
@@ -62,6 +62,13 @@ The following firmware-attributes are implemented:
  - ppt_pl2_sppt: Platform Profile Tracking Slow Package Power Tracking
  - ppt_pl3_fppt: Platform Profile Tracking Fast Package Power Tracking
 
+LENOVO_FAN_TEST_DATA
+-------------------------
+
+WMI GUID ``B642801B-3D21-45DE-90AE-6E86F164FB21``
+
+The LENOVO_FAN_TEST_DATA interface provides reference data for self-test of
+cooling fans.
 
 WMI interface description
 =========================
@@ -115,3 +122,13 @@ data using the `bmfdec <https://github.com/pali/bmfdec>`_ utility:
     [WmiDataId(3), read, Description("Data Size.")] uint32 DataSize;
     [WmiDataId(4), read, Description("Default Value"), WmiSizeIs("DataSize")] uint8 DefaultValue[];
   };
+
+  [WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x409"), Description("Definition of Fan Test Data"), guid("{B642801B-3D21-45DE-90AE-6E86F164FB21}")]
+  class LENOVO_FAN_TEST_DATA {
+    [key, read] string InstanceName;
+    [read] boolean Active;
+    [WmiDataId(1), read, Description("Mode.")] uint32 NumOfFans;
+    [WmiDataId(2), read, Description("Fan ID."), WmiSizeIs("NumOfFans")] uint32 FanId[];
+    [WmiDataId(3), read, Description("Maximum Fan Speed."), WmiSizeIs("NumOfFans")] uint32 FanMaxSpeed[];
+    [WmiDataId(4), read, Description("Minumum Fan Speed."), WmiSizeIs("NumOfFans")] uint32 FanMinSpeed[];
+  };
diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
index e8ec30701d88..e456aace87f2 100644
--- a/drivers/platform/x86/lenovo/wmi-capdata.c
+++ b/drivers/platform/x86/lenovo/wmi-capdata.c
@@ -13,6 +13,10 @@
  * attribute has multiple pages, one for each of the thermal modes managed by
  * the Gamezone interface.
  *
+ * Fan Test Data includes the max/min fan speed RPM for each fan. This is
+ * reference data for self-test. If the fan is in good condition, it is capable
+ * to spin faster than max RPM or slower than min RPM.
+ *
  * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@gmail.com>
  *   - Initial implementation (formerly named lenovo-wmi-capdata01)
  *
@@ -41,6 +45,7 @@
 
 #define LENOVO_CAPABILITY_DATA_00_GUID "362A3AFE-3D96-4665-8530-96DAD5BB300E"
 #define LENOVO_CAPABILITY_DATA_01_GUID "7A8F5407-CB67-4D6E-B547-39B3BE018154"
+#define LENOVO_FAN_TEST_DATA_GUID "B642801B-3D21-45DE-90AE-6E86F164FB21"
 
 #define ACPI_AC_CLASS "ac_adapter"
 #define ACPI_AC_NOTIFY_STATUS 0x80
@@ -48,6 +53,7 @@
 enum lwmi_cd_type {
 	LENOVO_CAPABILITY_DATA_00,
 	LENOVO_CAPABILITY_DATA_01,
+	LENOVO_FAN_TEST_DATA,
 };
 
 #define LWMI_CD_TABLE_ITEM(_type)		\
@@ -64,6 +70,7 @@ static const struct lwmi_cd_info {
 } lwmi_cd_table[] = {
 	LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_00),
 	LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_01),
+	LWMI_CD_TABLE_ITEM(LENOVO_FAN_TEST_DATA),
 };
 
 struct lwmi_cd_priv {
@@ -80,6 +87,7 @@ struct cd_list {
 	union {
 		DECLARE_FLEX_ARRAY(struct capdata00, cd00);
 		DECLARE_FLEX_ARRAY(struct capdata01, cd01);
+		DECLARE_FLEX_ARRAY(struct capdata_fan, cd_fan);
 	};
 };
 
@@ -108,6 +116,14 @@ static int lwmi_cd_component_bind(struct device *cd_dev,
 	case LENOVO_CAPABILITY_DATA_01:
 		binder->cd01_list = priv->list;
 		break;
+	case LENOVO_FAN_TEST_DATA:
+		/*
+		 * Do not expose dummy data.
+		 * See also lwmi_cd_fan_list_alloc_cache().
+		 */
+		if (priv->list->count)
+			binder->cd_fan_list = priv->list;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -152,6 +168,9 @@ EXPORT_SYMBOL_NS_GPL(lwmi_cd00_get_data, "LENOVO_WMI_CD");
 DEF_LWMI_CDXX_GET_DATA(cd01, LENOVO_CAPABILITY_DATA_01, struct capdata01);
 EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
 
+DEF_LWMI_CDXX_GET_DATA(cd_fan, LENOVO_FAN_TEST_DATA, struct capdata_fan);
+EXPORT_SYMBOL_NS_GPL(lwmi_cd_fan_get_data, "LENOVO_WMI_CD");
+
 /**
  * lwmi_cd_cache() - Cache all WMI data block information
  * @priv: lenovo-wmi-capdata driver data.
@@ -175,6 +194,9 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
 		p = &priv->list->cd01[0];
 		size = sizeof(priv->list->cd01[0]);
 		break;
+	case LENOVO_FAN_TEST_DATA:
+		/* Done by lwmi_cd_alloc() => lwmi_cd_fan_list_alloc_cache(). */
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -197,6 +219,78 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
 	return 0;
 }
 
+/**
+ * lwmi_cd_fan_list_alloc_cache() - Alloc and cache Fan Test Data list
+ * @priv: lenovo-wmi-capdata driver data.
+ * @listptr: Pointer to returned cd_list pointer.
+ *
+ * Return: count of fans found, or an error.
+ */
+static int lwmi_cd_fan_list_alloc_cache(struct lwmi_cd_priv *priv, struct cd_list **listptr)
+{
+	u32 count, *fan_ids, *fan_min_rpms, *fan_max_rpms;
+	union acpi_object *ret_obj __free(kfree) = NULL;
+	struct block { u32 nr; u32 data[]; } *block;
+	struct cd_list *list;
+	size_t size;
+	int idx;
+
+	ret_obj = wmidev_block_query(priv->wdev, 0);
+	if (!ret_obj)
+		return -ENODEV;
+
+	/*
+	 * This is usually caused by a dummy ACPI method. Do not return an error
+	 * as failing to probe this device will result in master driver being
+	 * unbound - this behavior aligns with lwmi_cd_cache().
+	 */
+	if (ret_obj->type != ACPI_TYPE_BUFFER) {
+		count = 0;
+		goto alloc;
+	}
+
+	size = ret_obj->buffer.length;
+	block = (struct block *)ret_obj->buffer.pointer;
+
+	count = size >= sizeof(*block) ? block->nr : 0;
+	if (size < struct_size(block, data, count * 3)) {
+		dev_warn(&priv->wdev->dev,
+			 "incomplete fan test data block: %zu < %zu, ignoring\n",
+			 size, struct_size(block, data, count * 3));
+		count = 0;
+	}
+
+	if (count == 0)
+		goto alloc;
+
+	if (count > U8_MAX) {
+		dev_warn(&priv->wdev->dev,
+			 "too many fans reported: %u > %u, truncating\n",
+			 count, U8_MAX);
+		count = U8_MAX;
+	}
+
+	fan_ids = &block->data[0];
+	fan_max_rpms = &block->data[count];
+	fan_min_rpms = &block->data[count * 2];
+
+alloc:
+	list = devm_kzalloc(&priv->wdev->dev, struct_size(list, cd_fan, count), GFP_KERNEL);
+	if (!list)
+		return -ENOMEM;
+
+	for (idx = 0; idx < count; idx++) {
+		list->cd_fan[idx] = (struct capdata_fan) {
+			.id = fan_ids[idx],
+			.min_rpm = fan_min_rpms[idx],
+			.max_rpm = fan_max_rpms[idx],
+		};
+	}
+
+	*listptr = list;
+	return count;
+}
+
 /**
  * lwmi_cd_alloc() - Allocate a cd_list struct in drvdata
  * @priv: lenovo-wmi-capdata driver data.
@@ -222,6 +316,12 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
 	case LENOVO_CAPABILITY_DATA_01:
 		list_size = struct_size(list, cd01, count);
 		break;
+	case LENOVO_FAN_TEST_DATA:
+		count = lwmi_cd_fan_list_alloc_cache(priv, &list);
+		if (count < 0)
+			return count;
+
+		goto got_list;
 	default:
 		return -EINVAL;
 	}
@@ -230,6 +330,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
 	if (!list)
 		return -ENOMEM;
 
+got_list:
 	ret = devm_mutex_init(&priv->wdev->dev, &list->list_mutex);
 	if (ret)
 		return ret;
@@ -368,6 +469,7 @@ static void lwmi_cd_remove(struct wmi_device *wdev)
 static const struct wmi_device_id lwmi_cd_id_table[] = {
 	{ LWMI_CD_WDEV_ID(LENOVO_CAPABILITY_DATA_00) },
 	{ LWMI_CD_WDEV_ID(LENOVO_CAPABILITY_DATA_01) },
+	{ LWMI_CD_WDEV_ID(LENOVO_FAN_TEST_DATA) },
 	{}
 };
 
diff --git a/drivers/platform/x86/lenovo/wmi-capdata.h b/drivers/platform/x86/lenovo/wmi-capdata.h
index a6f0cb006e74..52bc215ac43d 100644
--- a/drivers/platform/x86/lenovo/wmi-capdata.h
+++ b/drivers/platform/x86/lenovo/wmi-capdata.h
@@ -26,13 +26,21 @@ struct capdata01 {
 	u32 max_value;
 };
 
+struct capdata_fan {
+	u32 id;
+	u32 min_rpm;
+	u32 max_rpm;
+};
+
 struct lwmi_cd_binder {
 	struct cd_list *cd00_list;
 	struct cd_list *cd01_list;
+	struct cd_list *cd_fan_list;
 };
 
 int lwmi_cd00_get_data(struct cd_list *list, u32 attribute_id, struct capdata00 *output);
 int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output);
+int lwmi_cd_fan_get_data(struct cd_list *list, u32 attribute_id, struct capdata_fan *output);
 void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr);
 
 #endif /* !_LENOVO_WMI_CAPDATA_H_ */
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v3 6/6] platform/x86: lenovo-wmi-other: Add HWMON for fan speed RPM
  2025-10-31 15:51 [PATCH v3 0/6] platform/x86: lenovo-wmi-{capdata,other}: Add HWMON for fan speed Rong Zhang
                   ` (4 preceding siblings ...)
  2025-10-31 15:51 ` [PATCH v3 5/6] platform/x86: lenovo-wmi-capdata: Add support for Fan Test Data Rong Zhang
@ 2025-10-31 15:51 ` Rong Zhang
  5 siblings, 0 replies; 14+ messages in thread
From: Rong Zhang @ 2025-10-31 15:51 UTC (permalink / raw)
  To: Mark Pearson, Derek J. Clark, Armin Wolf, Hans de Goede,
	Ilpo Järvinen
  Cc: Rong Zhang, Guenter Roeck, platform-driver-x86, linux-kernel,
	linux-hwmon

Register an HWMON device for fan spped RPM according to Capability Data
00 and Fan Test Data provided by lenovo-wmi-capdata. The corresponding
HWMON nodes are:

 - fanX_enable: enable/disable the fan (tunable)
 - fanX_input: current RPM
 - fanX_max: maximum RPM
 - fanX_min: minimum RPM
 - fanX_target: target RPM (tunable)

Signed-off-by: Rong Zhang <i@rong.moe>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
---
Changes in v2:
- Define 4 fan channels instead of 2 (thanks Derek J. Clark)
- Squash min/max reporting patch into this one (ditto)
- Query 0x04050000 for interface availability (ditto)
  - New parameter "expose_all_fans" to skip this check
- Enforce min/max RPM constraint on set (ditto)
  - New parameter "relax_fan_constraint" to disable this behavior
  - Drop parameter "ignore_fan_cap", superseded by the next one
  - New parameter "expose_all_fans" to expose fans w/o such data
- Assume auto mode on probe (ditto)
- Reword documentation (ditto)
- Do not register HWMON device if no fan can be exposed
- fanX_target: Return -EBUSY instead of raw target value when fan stops

Changes in v3:
- Fix grammar (thanks Derek J. Clark)
---
 .../wmi/devices/lenovo-wmi-other.rst          |  11 +
 drivers/platform/x86/lenovo/Kconfig           |   1 +
 drivers/platform/x86/lenovo/wmi-other.c       | 467 +++++++++++++++++-
 3 files changed, 467 insertions(+), 12 deletions(-)

diff --git a/Documentation/wmi/devices/lenovo-wmi-other.rst b/Documentation/wmi/devices/lenovo-wmi-other.rst
index 821282e07d93..bd1d733ff286 100644
--- a/Documentation/wmi/devices/lenovo-wmi-other.rst
+++ b/Documentation/wmi/devices/lenovo-wmi-other.rst
@@ -31,6 +31,8 @@ under the following path:
 
   /sys/class/firmware-attributes/lenovo-wmi-other/attributes/<attribute>/
 
+Additionally, this driver also exports attributes to HWMON.
+
 LENOVO_CAPABILITY_DATA_00
 -------------------------
 
@@ -39,6 +41,11 @@ WMI GUID ``362A3AFE-3D96-4665-8530-96DAD5BB300E``
 The LENOVO_CAPABILITY_DATA_00 interface provides various information that
 does not rely on the gamezone thermal mode.
 
+The following HWMON attributes are implemented:
+ - fanX_enable: enable/disable the fan (tunable)
+ - fanX_input: current RPM
+ - fanX_target: target RPM (tunable)
+
 LENOVO_CAPABILITY_DATA_01
 -------------------------
 
@@ -70,6 +77,10 @@ WMI GUID ``B642801B-3D21-45DE-90AE-6E86F164FB21``
 The LENOVO_FAN_TEST_DATA interface provides reference data for self-test of
 cooling fans.
 
+The following HWMON attributes are implemented:
+ - fanX_max: maximum RPM
+ - fanX_min: minimum RPM
+
 WMI interface description
 =========================
 
diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
index fb96a0f908f0..be9af0451146 100644
--- a/drivers/platform/x86/lenovo/Kconfig
+++ b/drivers/platform/x86/lenovo/Kconfig
@@ -263,6 +263,7 @@ config LENOVO_WMI_GAMEZONE
 config LENOVO_WMI_TUNING
 	tristate "Lenovo Other Mode WMI Driver"
 	depends on ACPI_WMI
+	select HWMON
 	select FW_ATTR_CLASS
 	select LENOVO_WMI_DATA
 	select LENOVO_WMI_EVENTS
diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
index 20c6ff0be37a..2a1c06d996bb 100644
--- a/drivers/platform/x86/lenovo/wmi-other.c
+++ b/drivers/platform/x86/lenovo/wmi-other.c
@@ -14,7 +14,16 @@
  * These attributes typically don't fit anywhere else in the sysfs and are set
  * in Windows using one of Lenovo's multiple user applications.
  *
+ * Additionally, this driver also exports tunable fan speed RPM to HWMON.
+ * Min/max RPM are also provided for reference.
+ *
  * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@gmail.com>
+ *   - fw_attributes
+ *   - binding to Capability Data 01
+ *
+ * Copyright (C) 2025 Rong Zhang <i@rong.moe>
+ *   - HWMON
+ *   - binding to Capability Data 00 and Fan
  */
 
 #include <linux/acpi.h>
@@ -25,6 +34,7 @@
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/gfp_types.h>
+#include <linux/hwmon.h>
 #include <linux/idr.h>
 #include <linux/kdev_t.h>
 #include <linux/kobject.h>
@@ -43,12 +53,21 @@
 
 #define LENOVO_OTHER_MODE_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B"
 
+#define LWMI_SUPP_VALID BIT(0)
+#define LWMI_SUPP_MAY_GET (LWMI_SUPP_VALID | BIT(1))
+#define LWMI_SUPP_MAY_SET (LWMI_SUPP_VALID | BIT(2))
+
 #define LWMI_DEVICE_ID_CPU 0x01
 
 #define LWMI_FEATURE_ID_CPU_SPPT 0x01
 #define LWMI_FEATURE_ID_CPU_SPL 0x02
 #define LWMI_FEATURE_ID_CPU_FPPT 0x03
 
+#define LWMI_DEVICE_ID_FAN 0x04
+
+#define LWMI_FEATURE_ID_FAN_RPM 0x03
+#define LWMI_FEATURE_ID_FAN_TEST 0x05
+
 #define LWMI_TYPE_ID_NONE 0x00
 
 #define LWMI_FEATURE_VALUE_GET 17
@@ -59,7 +78,24 @@
 #define LWMI_ATTR_MODE_ID_MASK GENMASK(15, 8)
 #define LWMI_ATTR_TYPE_ID_MASK GENMASK(7, 0)
 
+#define LWMI_FAN_ID_BASE 1
+#define LWMI_FAN_NR 4
+#define LWMI_FAN_ID(x) ((x) + LWMI_FAN_ID_BASE)
+
+#define LWMI_ATTR_ID_FAN_RPM(x)						\
+	(FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, LWMI_DEVICE_ID_FAN) |	\
+	 FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, LWMI_FEATURE_ID_FAN_RPM) |	\
+	 FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, LWMI_FAN_ID(x)))
+
+#define LWMI_ATTR_ID_FAN_TEST							\
+	(FIELD_PREP(LWMI_ATTR_DEV_ID_MASK, LWMI_DEVICE_ID_FAN) |		\
+	 FIELD_PREP(LWMI_ATTR_FEAT_ID_MASK, LWMI_FEATURE_ID_FAN_TEST) |		\
+	 FIELD_PREP(LWMI_ATTR_TYPE_ID_MASK, LWMI_TYPE_ID_NONE))
+
+#define LWMI_FAN_STOP_RPM 1
+
 #define LWMI_OM_FW_ATTR_BASE_PATH "lenovo-wmi-other"
+#define LWMI_OM_HWMON_NAME "lenovo_wmi_other"
 
 static BLOCKING_NOTIFIER_HEAD(om_chain_head);
 static DEFINE_IDA(lwmi_om_ida);
@@ -76,15 +112,391 @@ struct lwmi_om_priv {
 	struct component_master_ops *ops;
 
 	/* only valid after capdata bind */
+	struct cd_list *cd00_list;
 	struct cd_list *cd01_list;
+	struct cd_list *cd_fan_list;
 
+	struct device *hwmon_dev;
 	struct device *fw_attr_dev;
 	struct kset *fw_attr_kset;
 	struct notifier_block nb;
 	struct wmi_device *wdev;
 	int ida_id;
+
+	struct fan_info {
+		u32 supported;
+		u32 last_target;
+		long min_rpm;
+		long max_rpm;
+	} fan_info[LWMI_FAN_NR];
+};
+
+/*
+ * Visibility of fan channels:
+ *
+ * +---------------------+---------+------------------+-----------------------+------------+
+ * |                     | default | +expose_all_fans | +relax_fan_constraint | +both      |
+ * +---------------------+---------+------------------+-----------------------+------------+
+ * | canonical           | RW      | RW               | RW+relaxed            | RW+relaxed |
+ * +---------------------+---------+------------------+-----------------------+------------+
+ * | -capdata_fan        | N       | RO               | N                     | RW+relaxed |
+ * +---------------------+---------+------------------+-----------------------+------------+
+ * | -FAN_TEST.supported | N       | RW               | N                     | RW+relaxed |
+ * +---------------------+---------+------------------+-----------------------+------------+
+ * | -both               | N       | RO               | N                     | RW+relaxed |
+ * +---------------------+---------+------------------+-----------------------+------------+
+ *
+ * Note: LWMI_ATTR_ID_FAN_RPM[idx].supported is always checked before exposing a channel.
+ */
+static bool expose_all_fans;
+module_param(expose_all_fans, bool, 0444);
+MODULE_PARM_DESC(expose_all_fans,
+	"This option skips some capability checks and solely relies on per-channel ones "
+	"to expose fan attributes. Use with caution.");
+
+static bool relax_fan_constraint;
+module_param(relax_fan_constraint, bool, 0444);
+MODULE_PARM_DESC(relax_fan_constraint,
+	"Do not enforce fan RPM constraint (min/max RPM) "
+	"and enables fan tuning when such data is missing. "
+	"Enabling this may results in HWMON attributes being out-of-sync. Use with caution.");
+
+/* ======== HWMON (component: lenovo-wmi-capdata 00 & fan) ======== */
+
+/**
+ * lwmi_om_fan_get_set() - Get or set fan RPM value of specified fan
+ * @priv: Driver private data structure
+ * @channel: Fan channel index (0-based)
+ * @val: Pointer to value (input for set, output for get)
+ * @set: True to set value, false to get value
+ *
+ * Communicates with WMI interface to either retrieve current fan RPM
+ * or set target fan RPM.
+ *
+ * Return: 0 on success, or an error code.
+ */
+static int lwmi_om_fan_get_set(struct lwmi_om_priv *priv, int channel, u32 *val, bool set)
+{
+	struct wmi_method_args_32 args;
+	u32 method_id, retval;
+	int err;
+
+	method_id = set ? LWMI_FEATURE_VALUE_SET : LWMI_FEATURE_VALUE_GET;
+	args.arg0 = LWMI_ATTR_ID_FAN_RPM(channel);
+	args.arg1 = set ? *val : 0;
+
+	err = lwmi_dev_evaluate_int(priv->wdev, 0x0, method_id,
+				    (unsigned char *)&args, sizeof(args), &retval);
+	if (err)
+		return err;
+
+	if (!set)
+		*val = retval;
+	else if (retval != 1)
+		return -EIO;
+
+	return 0;
+}
+
+/**
+ * lwmi_om_hwmon_is_visible() - Determine visibility of HWMON attributes
+ * @drvdata: Driver private data
+ * @type: Sensor type
+ * @attr: Attribute identifier
+ * @channel: Channel index
+ *
+ * Determines whether a HWMON attribute should be visible in sysfs
+ * based on hardware capabilities and current configuration.
+ *
+ * Return: permission mode, or 0 if invisible.
+ */
+static umode_t lwmi_om_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
+					u32 attr, int channel)
+{
+	struct lwmi_om_priv *priv = (struct lwmi_om_priv *)drvdata;
+	bool visible = false;
+
+	if (type == hwmon_fan) {
+		switch (attr) {
+		case hwmon_fan_enable:
+		case hwmon_fan_target:
+			if (!(priv->fan_info[channel].supported & LWMI_SUPP_MAY_SET))
+				return 0;
+
+			if (relax_fan_constraint ||
+			    (priv->fan_info[channel].min_rpm >= 0 &&
+			     priv->fan_info[channel].max_rpm >= 0))
+				return 0644;
+
+			/*
+			 * Reaching here implies expose_all_fans is set.
+			 * See lwmi_om_hwmon_add().
+			 */
+			dev_warn_once(&priv->wdev->dev,
+				      "fan tuning disabled due to missing RPM constraint\n");
+			return 0;
+		case hwmon_fan_input:
+			visible = priv->fan_info[channel].supported & LWMI_SUPP_MAY_GET;
+			break;
+		case hwmon_fan_min:
+			visible = priv->fan_info[channel].min_rpm >= 0;
+			break;
+		case hwmon_fan_max:
+			visible = priv->fan_info[channel].max_rpm >= 0;
+			break;
+		}
+	}
+
+	return visible ? 0444 : 0;
+}
+
+/**
+ * lwmi_om_hwmon_read() - Read HWMON sensor data
+ * @dev: Device pointer
+ * @type: Sensor type
+ * @attr: Attribute identifier
+ * @channel: Channel index
+ * @val: Pointer to store value
+ *
+ * Reads current sensor values from hardware through WMI interface.
+ *
+ * Return: 0 on success, or an error code.
+ */
+static int lwmi_om_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			      u32 attr, int channel, long *val)
+{
+	struct lwmi_om_priv *priv = dev_get_drvdata(dev);
+	u32 retval = 0;
+	int err;
+
+	if (type == hwmon_fan) {
+		switch (attr) {
+		case hwmon_fan_input:
+			err = lwmi_om_fan_get_set(priv, channel, &retval, false);
+			if (err)
+				return err;
+
+			*val = retval;
+			return 0;
+		case hwmon_fan_enable:
+			*val = priv->fan_info[channel].last_target != LWMI_FAN_STOP_RPM;
+			return 0;
+		case hwmon_fan_target:
+			if (priv->fan_info[channel].last_target == LWMI_FAN_STOP_RPM)
+				return -EBUSY;
+
+			*val = priv->fan_info[channel].last_target;
+			return 0;
+		case hwmon_fan_min:
+			*val = priv->fan_info[channel].min_rpm;
+			return 0;
+		case hwmon_fan_max:
+			*val = priv->fan_info[channel].max_rpm;
+			return 0;
+		}
+	}
+
+	return -EOPNOTSUPP;
+}
+
+/**
+ * lwmi_om_hwmon_write() - Write HWMON sensor data
+ * @dev: Device pointer
+ * @type: Sensor type
+ * @attr: Attribute identifier
+ * @channel: Channel index
+ * @val: Value to write
+ *
+ * Writes configuration values to hardware through WMI interface.
+ *
+ * Return: 0 on success, or an error code.
+ */
+static int lwmi_om_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+			       u32 attr, int channel, long val)
+{
+	struct lwmi_om_priv *priv = dev_get_drvdata(dev);
+	u32 raw, min_rpm, max_rpm;
+	int err;
+
+	if (type == hwmon_fan) {
+		switch (attr) {
+		case hwmon_fan_enable:
+			if (val == 0)
+				raw = LWMI_FAN_STOP_RPM;
+			else if (val == 1)
+				raw = 0; /* auto */
+			else
+				return -EINVAL;
+
+			goto fan_set;
+		case hwmon_fan_target:
+			if (val == 0) {
+				raw = 0;
+				goto fan_set;
+			}
+
+			min_rpm = relax_fan_constraint
+					? LWMI_FAN_STOP_RPM + 1
+					: priv->fan_info[channel].min_rpm;
+			max_rpm = relax_fan_constraint
+					? U16_MAX
+					: priv->fan_info[channel].max_rpm;
+
+			if (val < min_rpm || val > max_rpm)
+				return -EDOM;
+
+			raw = val;
+fan_set:
+			err = lwmi_om_fan_get_set(priv, channel, &raw, true);
+			if (err)
+				return err;
+
+			priv->fan_info[channel].last_target = raw;
+			return 0;
+		}
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_channel_info * const lwmi_om_hwmon_info[] = {
+	/* Must match LWMI_FAN_NR. */
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET |
+			   HWMON_F_MIN | HWMON_F_MAX,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET |
+			   HWMON_F_MIN | HWMON_F_MAX,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET |
+			   HWMON_F_MIN | HWMON_F_MAX,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET |
+			   HWMON_F_MIN | HWMON_F_MAX),
+	NULL
 };
 
+static const struct hwmon_ops lwmi_om_hwmon_ops = {
+	.is_visible = lwmi_om_hwmon_is_visible,
+	.read = lwmi_om_hwmon_read,
+	.write = lwmi_om_hwmon_write,
+};
+
+static const struct hwmon_chip_info lwmi_om_hwmon_chip_info = {
+	.ops = &lwmi_om_hwmon_ops,
+	.info = lwmi_om_hwmon_info,
+};
+
+/**
+ * lwmi_om_hwmon_add() - Register HWMON device
+ * @priv: Driver private data
+ *
+ * Initializes capability data and registers the HWMON device.
+ *
+ * Return: 0 on success, or an error code.
+ */
+static int lwmi_om_hwmon_add(struct lwmi_om_priv *priv)
+{
+	struct capdata_fan capdata_fan;
+	struct capdata00 capdata00;
+	int i, err, valid;
+
+	if (expose_all_fans) {
+		dev_warn(&priv->wdev->dev, "all fans exposed. Use with caution\n");
+	} else if (!priv->cd_fan_list) {
+		goto unsupported;
+	} else {
+		err = lwmi_cd00_get_data(priv->cd00_list, LWMI_ATTR_ID_FAN_TEST, &capdata00);
+		if (err || !(capdata00.supported & LWMI_SUPP_VALID))
+			goto unsupported;
+	}
+
+	if (relax_fan_constraint)
+		dev_warn(&priv->wdev->dev, "fan RPM constraint relaxed. Use with caution\n");
+
+	valid = 0;
+	for (i = 0; i < LWMI_FAN_NR; i++) {
+		err = lwmi_cd00_get_data(priv->cd00_list, LWMI_ATTR_ID_FAN_RPM(i), &capdata00);
+
+		priv->fan_info[i] = (struct fan_info) {
+			.supported = err ? 0 : capdata00.supported,
+			/*
+			 * Assume 0 on probe as the EC resets all fans to auto mode on (re)boot.
+			 *
+			 * Note that S0ix (s2idle) preserves the RPM target, so we
+			 * don't need suspend/resume callbacks. This behavior has not
+			 * been tested on S3-capable devices, but I doubt if such devices
+			 * even have this interface.
+			 */
+			.last_target = 0,
+			.min_rpm = -ENODATA,
+			.max_rpm = -ENODATA,
+		};
+
+		if (!(priv->fan_info[i].supported & LWMI_SUPP_VALID))
+			continue;
+
+		valid++;
+
+		if (!priv->cd_fan_list)
+			/*
+			 * Reaching here implies expose_all_fans is set.
+			 * fanX_{target,enable} will be gated by lwmi_om_hwmon_is_visible(),
+			 * unless relax_fan_constraint is also set.
+			 */
+			continue;
+
+		err = lwmi_cd_fan_get_data(priv->cd_fan_list, LWMI_FAN_ID(i), &capdata_fan);
+		if (!err) {
+			priv->fan_info[i].min_rpm = capdata_fan.min_rpm;
+			priv->fan_info[i].max_rpm = capdata_fan.max_rpm;
+			continue;
+		}
+
+		if (!expose_all_fans) {
+			/*
+			 * Fan attribute from capdata00 may be dummy (i.e.,
+			 * get: constant dummy RPM, set: no-op with retval == 0).
+			 *
+			 * If fan capdata is available and a fan is missing from it,
+			 * make the fan invisible.
+			 */
+			dev_dbg(&priv->wdev->dev, "missing RPM constraint for fan%d, hiding\n",
+				LWMI_FAN_ID(i));
+			priv->fan_info[i].supported = 0;
+			valid--;
+		}
+	}
+
+	if (valid == 0)
+		goto unsupported;
+
+	priv->hwmon_dev = hwmon_device_register_with_info(&priv->wdev->dev, LWMI_OM_HWMON_NAME,
+							  priv, &lwmi_om_hwmon_chip_info, NULL);
+	if (IS_ERR(priv->hwmon_dev)) {
+		err = PTR_ERR(priv->hwmon_dev);
+		priv->hwmon_dev = NULL;
+		return err;
+	}
+	return 0;
+
+unsupported:
+	dev_warn(&priv->wdev->dev, "fan reporting/tuning is unsupported on this device\n");
+	return 0;
+}
+
+/**
+ * lwmi_om_hwmon_remove() - Unregister HWMON device
+ * @priv: Driver private data
+ *
+ * Unregisters the HWMON device and resets all fans to automatic mode.
+ * Ensures hardware doesn't remain in manual mode after driver removal.
+ */
+static void lwmi_om_hwmon_remove(struct lwmi_om_priv *priv)
+{
+	if (priv->hwmon_dev)
+		hwmon_device_unregister(priv->hwmon_dev);
+}
+
+/* ======== fw_attributes (component: lenovo-wmi-capdata 01) ======== */
+
 struct tunable_attr_01 {
 	struct capdata01 *capdata;
 	struct device *dev;
@@ -547,6 +959,7 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
 
 err_free_ida:
 	ida_free(&lwmi_om_ida, priv->ida_id);
+	priv->fw_attr_dev = NULL;
 	return err;
 }
 
@@ -556,6 +969,9 @@ static int lwmi_om_fw_attr_add(struct lwmi_om_priv *priv)
  */
 static void lwmi_om_fw_attr_remove(struct lwmi_om_priv *priv)
 {
+	if (!priv->fw_attr_dev)
+		return;
+
 	for (unsigned int i = 0; i < ARRAY_SIZE(cd01_attr_groups) - 1; i++)
 		sysfs_remove_group(&priv->fw_attr_kset->kobj,
 				   cd01_attr_groups[i].attr_group);
@@ -564,15 +980,17 @@ static void lwmi_om_fw_attr_remove(struct lwmi_om_priv *priv)
 	device_unregister(priv->fw_attr_dev);
 }
 
+/* ======== Self (master: lenovo-wmi-other) ======== */
+
 /**
  * lwmi_om_master_bind() - Bind all components of the other mode driver
  * @dev: The lenovo-wmi-other driver basic device.
  *
- * Call component_bind_all to bind the lenovo-wmi-capdata01 driver to the
- * lenovo-wmi-other master driver. On success, assign the capability data 01
- * list pointer to the driver data struct for later access. This pointer
- * is only valid while the capdata01 interface exists. Finally, register all
- * firmware attribute groups.
+ * Call component_bind_all to bind the lenovo-wmi-capdata devices to the
+ * lenovo-wmi-other master driver. On success, assign the capability data
+ * list pointers to the driver data struct for later access. These pointers
+ * are only valid while the capdata interfaces exist. Finally, register the
+ * HWMON device and all firmware attribute groups.
  *
  * Return: 0 on success, or an error code.
  */
@@ -586,26 +1004,45 @@ static int lwmi_om_master_bind(struct device *dev)
 	if (ret)
 		return ret;
 
-	priv->cd01_list = binder.cd01_list;
-	if (!priv->cd01_list)
+	if (!binder.cd00_list && !binder.cd01_list)
 		return -ENODEV;
 
-	return lwmi_om_fw_attr_add(priv);
+	priv->cd00_list = binder.cd00_list;
+	if (priv->cd00_list) {
+		priv->cd_fan_list = binder.cd_fan_list;
+		ret = lwmi_om_hwmon_add(priv);
+		if (ret)
+			return ret;
+	}
+
+	priv->cd01_list = binder.cd01_list;
+	if (priv->cd01_list) {
+		ret = lwmi_om_fw_attr_add(priv);
+		if (ret) {
+			if (priv->cd00_list)
+				lwmi_om_hwmon_remove(priv);
+			return ret;
+		}
+	}
+
+	return 0;
 }
 
 /**
  * lwmi_om_master_unbind() - Unbind all components of the other mode driver
  * @dev: The lenovo-wmi-other driver basic device
  *
- * Unregister all capability data attribute groups. Then call
- * component_unbind_all to unbind the lenovo-wmi-capdata01 driver from the
- * lenovo-wmi-other master driver. Finally, free the IDA for this device.
+ * Unregister the HWMON device and all capability data attribute groups. Then
+ * call component_unbind_all to unbind the lenovo-wmi-capdata driver from the
+ * lenovo-wmi-other master driver.
  */
 static void lwmi_om_master_unbind(struct device *dev)
 {
 	struct lwmi_om_priv *priv = dev_get_drvdata(dev);
 
+	lwmi_om_hwmon_remove(priv);
 	lwmi_om_fw_attr_remove(priv);
+
 	component_unbind_all(dev, NULL);
 }
 
@@ -624,6 +1061,9 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
 	if (!priv)
 		return -ENOMEM;
 
+	/* Sentinel for on-demand ida_free(). */
+	priv->ida_id = -EIDRM;
+
 	priv->wdev = wdev;
 	dev_set_drvdata(&wdev->dev, priv);
 
@@ -654,7 +1094,9 @@ static void lwmi_other_remove(struct wmi_device *wdev)
 	struct lwmi_om_priv *priv = dev_get_drvdata(&wdev->dev);
 
 	component_master_del(&wdev->dev, &lwmi_om_master_ops);
-	ida_free(&lwmi_om_ida, priv->ida_id);
+
+	if (priv->ida_id >= 0)
+		ida_free(&lwmi_om_ida, priv->ida_id);
 }
 
 static const struct wmi_device_id lwmi_other_id_table[] = {
@@ -679,5 +1121,6 @@ MODULE_IMPORT_NS("LENOVO_WMI_CD");
 MODULE_IMPORT_NS("LENOVO_WMI_HELPERS");
 MODULE_DEVICE_TABLE(wmi, lwmi_other_id_table);
 MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
+MODULE_AUTHOR("Rong Zhang <i@rong.moe>");
 MODULE_DESCRIPTION("Lenovo Other Mode WMI Driver");
 MODULE_LICENSE("GPL");
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/6] platform/x86: lenovo-wmi-helpers: Convert returned 4B buffer into u32
  2025-10-31 15:51 ` [PATCH v3 1/6] platform/x86: lenovo-wmi-helpers: Convert returned 4B buffer into u32 Rong Zhang
@ 2025-11-04 20:13   ` Armin Wolf
  2025-11-04 20:27     ` Rong Zhang
  0 siblings, 1 reply; 14+ messages in thread
From: Armin Wolf @ 2025-11-04 20:13 UTC (permalink / raw)
  To: Rong Zhang, Mark Pearson, Derek J. Clark, Hans de Goede,
	Ilpo Järvinen
  Cc: Guenter Roeck, platform-driver-x86, linux-kernel, linux-hwmon

Am 31.10.25 um 16:51 schrieb Rong Zhang:

> The Windows WMI-ACPI driver converts all ACPI objects into a common
> buffer format, so returning a buffer with four bytes will look like an
> integer for WMI consumers under Windows.
>
> Therefore, some devices may simply implement the corresponding ACPI
> methods to always return a buffer. While lwmi_dev_evaluate_int() expects
> an integer (u32), convert returned 4-byte buffer into u32 to support
> these devices.
>
> Suggested-by: Armin Wolf <W_Armin@gmx.de>
> Link: https://lore.kernel.org/r/f1787927-b655-4321-b9d9-bc12353c72db@gmx.de/
> Signed-off-by: Rong Zhang <i@rong.moe>
> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
> ---
> Changes in v2:
> - New patch (thanks Armin Wolf)
> ---
>   drivers/platform/x86/lenovo/wmi-helpers.c | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/lenovo/wmi-helpers.c b/drivers/platform/x86/lenovo/wmi-helpers.c
> index f6fef6296251..f3bc92ac505a 100644
> --- a/drivers/platform/x86/lenovo/wmi-helpers.c
> +++ b/drivers/platform/x86/lenovo/wmi-helpers.c
> @@ -59,10 +59,25 @@ int lwmi_dev_evaluate_int(struct wmi_device *wdev, u8 instance, u32 method_id,
>   		if (!ret_obj)
>   			return -ENODATA;
>   
> -		if (ret_obj->type != ACPI_TYPE_INTEGER)
> -			return -ENXIO;
> +		switch (ret_obj->type) {
> +		/*
> +		 * The ACPI method may simply return a 4-byte buffer when a u32
> +		 * integer is expected. This is valid on Windows as its WMI-ACPI
> +		 * driver converts everything to a common buffer.
> +		 */
> +		case ACPI_TYPE_BUFFER: {
> +			if (ret_obj->buffer.length != 4)
> +				return -ENXIO;

The Windows driver also accepts oversized buffers. I suggest that you follow this behavior
for the sake of compatibility.

>   
> -		*retval = (u32)ret_obj->integer.value;
> +			*retval = *((u32 *)ret_obj->buffer.pointer);

The buffer can be unaligned. Better use get_unaligned_le32() from linux/unaligned.h.

Thanks,
Armin Wolf

> +			return 0;
> +		}
> +		case ACPI_TYPE_INTEGER:
> +			*retval = (u32)ret_obj->integer.value;
> +			return 0;
> +		default:
> +			return -ENXIO;
> +		}
>   	}
>   
>   	return 0;

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 3/6] platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability Data
  2025-10-31 15:51 ` [PATCH v3 3/6] platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability Data Rong Zhang
@ 2025-11-04 20:20   ` Armin Wolf
  2025-11-04 20:43     ` Rong Zhang
  2025-11-06 12:36     ` Rong Zhang
  0 siblings, 2 replies; 14+ messages in thread
From: Armin Wolf @ 2025-11-04 20:20 UTC (permalink / raw)
  To: Rong Zhang, Mark Pearson, Derek J. Clark, Hans de Goede,
	Ilpo Järvinen
  Cc: Guenter Roeck, platform-driver-x86, linux-kernel, linux-hwmon

Am 31.10.25 um 16:51 schrieb Rong Zhang:

> The current implementation are heavily bound to capdata01. Rewrite it so
> that it is suitable to utilize other Capability Data as well.
>
> No functional change intended.
>
> Signed-off-by: Rong Zhang <i@rong.moe>
> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
> ---
> Changes in v2:
> - Fix function parameter 'type' not described in 'lwmi_cd_match' (thanks
>    kernel test bot)
> ---
>   drivers/platform/x86/lenovo/wmi-capdata.c | 208 +++++++++++++++++-----
>   drivers/platform/x86/lenovo/wmi-capdata.h |   7 +-
>   drivers/platform/x86/lenovo/wmi-other.c   |  27 ++-
>   3 files changed, 190 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
> index c5e74b2bfeb3..1f7fc09b7c3f 100644
> --- a/drivers/platform/x86/lenovo/wmi-capdata.c
> +++ b/drivers/platform/x86/lenovo/wmi-capdata.c
> @@ -12,8 +12,13 @@
>    *
>    * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@gmail.com>
>    *   - Initial implementation (formerly named lenovo-wmi-capdata01)
> + *
> + * Copyright (C) 2025 Rong Zhang <i@rong.moe>
> + *   - Unified implementation
>    */
>   
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>   #include <linux/acpi.h>
>   #include <linux/cleanup.h>
>   #include <linux/component.h>
> @@ -36,6 +41,25 @@
>   #define ACPI_AC_CLASS "ac_adapter"
>   #define ACPI_AC_NOTIFY_STATUS 0x80
>   
> +enum lwmi_cd_type {
> +	LENOVO_CAPABILITY_DATA_01,
> +};
> +
> +#define LWMI_CD_TABLE_ITEM(_type)		\
> +	[_type] = {				\
> +		.guid_string = _type##_GUID,	\
> +		.name = #_type,			\
> +		.type = _type,			\
> +	}
> +
> +static const struct lwmi_cd_info {
> +	const char *guid_string;
> +	const char *name;
> +	enum lwmi_cd_type type;
> +} lwmi_cd_table[] = {
> +	LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_01),
> +};
> +
>   struct lwmi_cd_priv {
>   	struct notifier_block acpi_nb; /* ACPI events */
>   	struct wmi_device *wdev;
> @@ -44,15 +68,19 @@ struct lwmi_cd_priv {
>   
>   struct cd_list {
>   	struct mutex list_mutex; /* list R/W mutex */
> +	enum lwmi_cd_type type;
>   	u8 count;
> -	struct capdata01 data[];
> +
> +	union {
> +		DECLARE_FLEX_ARRAY(struct capdata01, cd01);
> +	};
>   };
>   
>   /**
>    * lwmi_cd_component_bind() - Bind component to master device.
>    * @cd_dev: Pointer to the lenovo-wmi-capdata driver parent device.
>    * @om_dev: Pointer to the lenovo-wmi-other driver parent device.
> - * @data: cd_list object pointer used to return the capability data.
> + * @data: lwmi_cd_binder object pointer used to return the capability data.
>    *
>    * On lenovo-wmi-other's master bind, provide a pointer to the local capdata
>    * list. This is used to call lwmi_cd*_get_data to look up attribute data
> @@ -64,9 +92,15 @@ static int lwmi_cd_component_bind(struct device *cd_dev,
>   				  struct device *om_dev, void *data)
>   {
>   	struct lwmi_cd_priv *priv = dev_get_drvdata(cd_dev);
> -	struct cd_list **cd_list = data;
> +	struct lwmi_cd_binder *binder = data;
>   
> -	*cd_list = priv->list;
> +	switch (priv->list->type) {
> +	case LENOVO_CAPABILITY_DATA_01:
> +		binder->cd01_list = priv->list;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>   
>   	return 0;
>   }
> @@ -76,30 +110,33 @@ static const struct component_ops lwmi_cd_component_ops = {
>   };
>   
>   /**
> - * lwmi_cd01_get_data - Get the data of the specified attribute
> + * lwmi_cd*_get_data - Get the data of the specified attribute
>    * @list: The lenovo-wmi-capdata pointer to its cd_list struct.
>    * @attribute_id: The capdata attribute ID to be found.
> - * @output: Pointer to a capdata01 struct to return the data.
> + * @output: Pointer to a capdata* struct to return the data.
>    *
> - * Retrieves the capability data 01 struct pointer for the given
> - * attribute for its specified thermal mode.
> + * Retrieves the capability data struct pointer for the given
> + * attribute.
>    *
>    * Return: 0 on success, or -EINVAL.
>    */
> -int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output)
> -{
> -	u8 idx;
> -
> -	guard(mutex)(&list->list_mutex);
> -	for (idx = 0; idx < list->count; idx++) {
> -		if (list->data[idx].id != attribute_id)
> -			continue;
> -		memcpy(output, &list->data[idx], sizeof(list->data[idx]));
> -		return 0;
> +#define DEF_LWMI_CDXX_GET_DATA(_cdxx, _cd_type, _output_t)					\
> +	int lwmi_##_cdxx##_get_data(struct cd_list *list, u32 attribute_id, _output_t *output)	\
> +	{											\
> +		u8 idx;										\
> +		if (WARN_ON(list->type != _cd_type))						\
> +			return -EINVAL;								\
> +		guard(mutex)(&list->list_mutex);						\
> +		for (idx = 0; idx < list->count; idx++) {					\
> +			if (list->_cdxx[idx].id != attribute_id)				\
> +				continue;							\
> +			memcpy(output, &list->_cdxx[idx], sizeof(list->_cdxx[idx]));		\
> +			return 0;								\
> +		}										\
> +		return -EINVAL;									\
>   	}
>   
> -	return -EINVAL;
> -}
> +DEF_LWMI_CDXX_GET_DATA(cd01, LENOVO_CAPABILITY_DATA_01, struct capdata01);
>   EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
>   
>   /**
> @@ -112,10 +149,21 @@ EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
>    */
>   static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
>   {
> +	size_t size;
>   	int idx;
> +	void *p;
> +
> +	switch (priv->list->type) {
> +	case LENOVO_CAPABILITY_DATA_01:
> +		p = &priv->list->cd01[0];
> +		size = sizeof(priv->list->cd01[0]);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>   
>   	guard(mutex)(&priv->list->list_mutex);
> -	for (idx = 0; idx < priv->list->count; idx++) {
> +	for (idx = 0; idx < priv->list->count; idx++, p += size) {
>   		union acpi_object *ret_obj __free(kfree) = NULL;
>   
>   		ret_obj = wmidev_block_query(priv->wdev, idx);
> @@ -123,11 +171,10 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
>   			return -ENODEV;
>   
>   		if (ret_obj->type != ACPI_TYPE_BUFFER ||
> -		    ret_obj->buffer.length < sizeof(priv->list->data[idx]))
> +		    ret_obj->buffer.length < size)
>   			continue;
>   
> -		memcpy(&priv->list->data[idx], ret_obj->buffer.pointer,
> -		       ret_obj->buffer.length);
> +		memcpy(p, ret_obj->buffer.pointer, size);
>   	}
>   
>   	return 0;
> @@ -136,20 +183,28 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
>   /**
>    * lwmi_cd_alloc() - Allocate a cd_list struct in drvdata
>    * @priv: lenovo-wmi-capdata driver data.
> + * @type: The type of capability data.
>    *
>    * Allocate a cd_list struct large enough to contain data from all WMI data
>    * blocks provided by the interface.
>    *
>    * Return: 0 on success, or an error.
>    */
> -static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> +static int lwmi_cd_alloc(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
>   {
>   	struct cd_list *list;
>   	size_t list_size;
>   	int count, ret;
>   
>   	count = wmidev_instance_count(priv->wdev);
> -	list_size = struct_size(list, data, count);
> +
> +	switch (type) {
> +	case LENOVO_CAPABILITY_DATA_01:
> +		list_size = struct_size(list, cd01, count);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>   
>   	list = devm_kzalloc(&priv->wdev->dev, list_size, GFP_KERNEL);
>   	if (!list)
> @@ -159,6 +214,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
>   	if (ret)
>   		return ret;
>   
> +	list->type = type;
>   	list->count = count;
>   	priv->list = list;
>   
> @@ -168,6 +224,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
>   /**
>    * lwmi_cd_setup() - Cache all WMI data block information
>    * @priv: lenovo-wmi-capdata driver data.
> + * @type: The type of capability data.
>    *
>    * Allocate a cd_list struct large enough to contain data from all WMI data
>    * blocks provided by the interface. Then loop through each data block and
> @@ -175,11 +232,11 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
>    *
>    * Return: 0 on success, or an error code.
>    */
> -static int lwmi_cd_setup(struct lwmi_cd_priv *priv)
> +static int lwmi_cd_setup(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
>   {
>   	int ret;
>   
> -	ret = lwmi_cd_alloc(priv);
> +	ret = lwmi_cd_alloc(priv, type);
>   	if (ret)
>   		return ret;
>   
> @@ -235,9 +292,13 @@ static void lwmi_cd01_unregister(void *data)
>   
>   static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
>   {
> +	const struct lwmi_cd_info *info = context;
>   	struct lwmi_cd_priv *priv;
>   	int ret;
>   
> +	if (!info)
> +		return -EINVAL;
> +
>   	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
>   		return -ENOMEM;
> @@ -245,21 +306,34 @@ static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
>   	priv->wdev = wdev;
>   	dev_set_drvdata(&wdev->dev, priv);
>   
> -	ret = lwmi_cd_setup(priv);
> +	ret = lwmi_cd_setup(priv, info->type);
>   	if (ret)
> -		return ret;
> +		goto out;
>   
> -	priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
> +	if (info->type == LENOVO_CAPABILITY_DATA_01) {
> +		priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
>   
> -	ret = register_acpi_notifier(&priv->acpi_nb);
> -	if (ret)
> -		return ret;
> +		ret = register_acpi_notifier(&priv->acpi_nb);
> +		if (ret)
> +			goto out;
>   
> -	ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister, &priv->acpi_nb);
> -	if (ret)
> -		return ret;
> +		ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister,
> +					       &priv->acpi_nb);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	ret = component_add(&wdev->dev, &lwmi_cd_component_ops);
>   
> -	return component_add(&wdev->dev, &lwmi_cd_component_ops);
> +out:
> +	if (ret) {
> +		dev_err(&wdev->dev, "failed to register %s: %d\n",
> +			info->name, ret);
> +	} else {
> +		dev_info(&wdev->dev, "registered %s with %u items\n",
> +			 info->name, priv->list->count);
> +	}
> +	return ret;
>   }
>   
>   static void lwmi_cd_remove(struct wmi_device *wdev)
> @@ -267,8 +341,12 @@ static void lwmi_cd_remove(struct wmi_device *wdev)
>   	component_del(&wdev->dev, &lwmi_cd_component_ops);
>   }
>   
> +#define LWMI_CD_WDEV_ID(_type)				\
> +	.guid_string = _type##_GUID,			\
> +	.context = &lwmi_cd_table[_type]
> +
>   static const struct wmi_device_id lwmi_cd_id_table[] = {
> -	{ LENOVO_CAPABILITY_DATA_01_GUID, NULL },
> +	{ LWMI_CD_WDEV_ID(LENOVO_CAPABILITY_DATA_01) },
>   	{}
>   };
>   
> @@ -284,21 +362,61 @@ static struct wmi_driver lwmi_cd_driver = {
>   };
>   
>   /**
> - * lwmi_cd01_match() - Match rule for the master driver.
> - * @dev: Pointer to the capability data 01 parent device.
> - * @data: Unused void pointer for passing match criteria.
> + * lwmi_cd_match() - Match rule for the master driver.
> + * @dev: Pointer to the capability data parent device.
> + * @type: Pointer to capability data type (enum lwmi_cd_type *) to match.
>    *
>    * Return: int.
>    */
> -int lwmi_cd01_match(struct device *dev, void *data)
> +static int lwmi_cd_match(struct device *dev, void *type)
> +{
> +	struct lwmi_cd_priv *priv;
> +
> +	if (dev->driver != &lwmi_cd_driver.driver)
> +		return false;
> +
> +	priv = dev_get_drvdata(dev);
> +	return priv->list->type == *(enum lwmi_cd_type *)type;
> +}
> +
> +/**
> + * lwmi_cd_match_add_all() - Add all match rule for the master driver.
> + * @master: Pointer to the master device.
> + * @matchptr: Pointer to the returned component_match pointer.
> + *
> + * Adds all component matches to the list stored in @matchptr for the @master
> + * device. @matchptr must be initialized to NULL. This matches all available
> + * capdata types on the machine.
> + */
> +void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr)
>   {
> -	return dev->driver == &lwmi_cd_driver.driver;
> +	int i;
> +
> +	if (WARN_ON(*matchptr))
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(lwmi_cd_table); i++) {
> +		if (!lwmi_cd_table[i].guid_string ||
> +		    !wmi_has_guid(lwmi_cd_table[i].guid_string))
> +			continue;

I am still not happy about this. AFAIK as soon as the ordinary capdata WMI devices are bound together,
the firmware tells you whether or not the additional fan data WMI device is available. Maybe you can do
something like this:

1. Bind both capdata WMI devices as usual.
2. Check if a fan data WMI device is available (you can use a DMI-based quirk list for devices were
    the firmware reports invalid data).
3. Register an additional component that binds to the fan data WMI device.
4. Bind both the additional component and the component registered by the fan data WMI device.
5. Register the hwmon device with additional fan data.

If the fan data WMI device is not available, you can simply skip steps 3 and 4 and simply
register the hwmon device with any additional fan data.

What do you think?

Thanks,
Armin Wolf

> +
> +		component_match_add(master, matchptr, lwmi_cd_match,
> +				    (void *)&lwmi_cd_table[i].type);
> +		if (IS_ERR(matchptr))
> +			return;
> +	}
> +
> +	if (!*matchptr) {
> +		pr_warn("a master driver requested capability data, but nothing is available\n");
> +		*matchptr = ERR_PTR(-ENODEV);
> +	}
>   }
> -EXPORT_SYMBOL_NS_GPL(lwmi_cd01_match, "LENOVO_WMI_CD");
> +EXPORT_SYMBOL_NS_GPL(lwmi_cd_match_add_all, "LENOVO_WMI_CD");
>   
>   module_wmi_driver(lwmi_cd_driver);
>   
>   MODULE_DEVICE_TABLE(wmi, lwmi_cd_id_table);
>   MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
> +MODULE_AUTHOR("Rong Zhang <i@rong.moe>");
>   MODULE_DESCRIPTION("Lenovo Capability Data WMI Driver");
>   MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/lenovo/wmi-capdata.h b/drivers/platform/x86/lenovo/wmi-capdata.h
> index 2a4746e38ad4..1e5fce7836cb 100644
> --- a/drivers/platform/x86/lenovo/wmi-capdata.h
> +++ b/drivers/platform/x86/lenovo/wmi-capdata.h
> @@ -7,6 +7,7 @@
>   
>   #include <linux/types.h>
>   
> +struct component_match;
>   struct device;
>   struct cd_list;
>   
> @@ -19,7 +20,11 @@ struct capdata01 {
>   	u32 max_value;
>   };
>   
> +struct lwmi_cd_binder {
> +	struct cd_list *cd01_list;
> +};
> +
>   int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output);
> -int lwmi_cd01_match(struct device *dev, void *data);
> +void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr);
>   
>   #endif /* !_LENOVO_WMI_CAPDATA_H_ */
> diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
> index c6dc1b4cff84..20c6ff0be37a 100644
> --- a/drivers/platform/x86/lenovo/wmi-other.c
> +++ b/drivers/platform/x86/lenovo/wmi-other.c
> @@ -579,14 +579,14 @@ static void lwmi_om_fw_attr_remove(struct lwmi_om_priv *priv)
>   static int lwmi_om_master_bind(struct device *dev)
>   {
>   	struct lwmi_om_priv *priv = dev_get_drvdata(dev);
> -	struct cd_list *tmp_list;
> +	struct lwmi_cd_binder binder = { 0 };
>   	int ret;
>   
> -	ret = component_bind_all(dev, &tmp_list);
> +	ret = component_bind_all(dev, &binder);
>   	if (ret)
>   		return ret;
>   
> -	priv->cd01_list = tmp_list;
> +	priv->cd01_list = binder.cd01_list;
>   	if (!priv->cd01_list)
>   		return -ENODEV;
>   
> @@ -618,6 +618,7 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
>   {
>   	struct component_match *master_match = NULL;
>   	struct lwmi_om_priv *priv;
> +	int ret;
>   
>   	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
> @@ -626,12 +627,26 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
>   	priv->wdev = wdev;
>   	dev_set_drvdata(&wdev->dev, priv);
>   
> -	component_match_add(&wdev->dev, &master_match, lwmi_cd01_match, NULL);
> +	lwmi_cd_match_add_all(&wdev->dev, &master_match);
>   	if (IS_ERR(master_match))
>   		return PTR_ERR(master_match);
>   
> -	return component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
> -					       master_match);
> +	ret = component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
> +					      master_match);
> +	if (ret)
> +		return ret;
> +
> +	if (likely(component_master_is_bound(&wdev->dev, &lwmi_om_master_ops)))
> +		return 0;
> +
> +	/*
> +	 * The bind callbacks of both master and components were never called in
> +	 * this case - this driver won't work at all. Failing...
> +	 */
> +	dev_err(&wdev->dev, "unbound master; is any component failing to be probed?");
> +
> +	component_master_del(&wdev->dev, &lwmi_om_master_ops);
> +	return -EXDEV;
>   }
>   
>   static void lwmi_other_remove(struct wmi_device *wdev)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/6] platform/x86: lenovo-wmi-helpers: Convert returned 4B buffer into u32
  2025-11-04 20:13   ` Armin Wolf
@ 2025-11-04 20:27     ` Rong Zhang
  0 siblings, 0 replies; 14+ messages in thread
From: Rong Zhang @ 2025-11-04 20:27 UTC (permalink / raw)
  To: Armin Wolf, Mark Pearson, Derek J. Clark, Hans de Goede,
	Ilpo Järvinen
  Cc: Guenter Roeck, platform-driver-x86, linux-kernel, linux-hwmon

Hi Armin,

On Tue, 2025-11-04 at 21:13 +0100, Armin Wolf wrote:
> Am 31.10.25 um 16:51 schrieb Rong Zhang:
> 
> > The Windows WMI-ACPI driver converts all ACPI objects into a common
> > buffer format, so returning a buffer with four bytes will look like an
> > integer for WMI consumers under Windows.
> > 
> > Therefore, some devices may simply implement the corresponding ACPI
> > methods to always return a buffer. While lwmi_dev_evaluate_int() expects
> > an integer (u32), convert returned 4-byte buffer into u32 to support
> > these devices.
> > 
> > Suggested-by: Armin Wolf <W_Armin@gmx.de>
> > Link: https://lore.kernel.org/r/f1787927-b655-4321-b9d9-bc12353c72db@gmx.de/
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > ---
> > Changes in v2:
> > - New patch (thanks Armin Wolf)
> > ---
> >   drivers/platform/x86/lenovo/wmi-helpers.c | 21 ++++++++++++++++++---
> >   1 file changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/lenovo/wmi-helpers.c b/drivers/platform/x86/lenovo/wmi-helpers.c
> > index f6fef6296251..f3bc92ac505a 100644
> > --- a/drivers/platform/x86/lenovo/wmi-helpers.c
> > +++ b/drivers/platform/x86/lenovo/wmi-helpers.c
> > @@ -59,10 +59,25 @@ int lwmi_dev_evaluate_int(struct wmi_device *wdev, u8 instance, u32 method_id,
> >   		if (!ret_obj)
> >   			return -ENODATA;
> >   
> > -		if (ret_obj->type != ACPI_TYPE_INTEGER)
> > -			return -ENXIO;
> > +		switch (ret_obj->type) {
> > +		/*
> > +		 * The ACPI method may simply return a 4-byte buffer when a u32
> > +		 * integer is expected. This is valid on Windows as its WMI-ACPI
> > +		 * driver converts everything to a common buffer.
> > +		 */
> > +		case ACPI_TYPE_BUFFER: {
> > +			if (ret_obj->buffer.length != 4)
> > +				return -ENXIO;
> 
> The Windows driver also accepts oversized buffers. I suggest that you follow this behavior
> for the sake of compatibility.
> 
> >   
> > -		*retval = (u32)ret_obj->integer.value;
> > +			*retval = *((u32 *)ret_obj->buffer.pointer);
> 
> The buffer can be unaligned. Better use get_unaligned_le32() from linux/unaligned.h.

Thanks for your review and information. Will do in v4.

> Thanks,
> Armin Wolf

Thanks,
Rong

> > +			return 0;
> > +		}
> > +		case ACPI_TYPE_INTEGER:
> > +			*retval = (u32)ret_obj->integer.value;
> > +			return 0;
> > +		default:
> > +			return -ENXIO;
> > +		}
> >   	}
> >   
> >   	return 0;

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 3/6] platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability Data
  2025-11-04 20:20   ` Armin Wolf
@ 2025-11-04 20:43     ` Rong Zhang
  2025-11-06 12:36     ` Rong Zhang
  1 sibling, 0 replies; 14+ messages in thread
From: Rong Zhang @ 2025-11-04 20:43 UTC (permalink / raw)
  To: Armin Wolf, Mark Pearson, Derek J. Clark, Hans de Goede,
	Ilpo Järvinen
  Cc: Guenter Roeck, platform-driver-x86, linux-kernel, linux-hwmon

Hi Armin,

On Tue, 2025-11-04 at 21:20 +0100, Armin Wolf wrote:
> Am 31.10.25 um 16:51 schrieb Rong Zhang:
> 
> > The current implementation are heavily bound to capdata01. Rewrite it so
> > that it is suitable to utilize other Capability Data as well.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > ---
> > Changes in v2:
> > - Fix function parameter 'type' not described in 'lwmi_cd_match' (thanks
> >    kernel test bot)
> > ---
> >   drivers/platform/x86/lenovo/wmi-capdata.c | 208 +++++++++++++++++-----
> >   drivers/platform/x86/lenovo/wmi-capdata.h |   7 +-
> >   drivers/platform/x86/lenovo/wmi-other.c   |  27 ++-
> >   3 files changed, 190 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
> > index c5e74b2bfeb3..1f7fc09b7c3f 100644
> > --- a/drivers/platform/x86/lenovo/wmi-capdata.c
> > +++ b/drivers/platform/x86/lenovo/wmi-capdata.c
> > @@ -12,8 +12,13 @@
> >    *
> >    * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@gmail.com>
> >    *   - Initial implementation (formerly named lenovo-wmi-capdata01)
> > + *
> > + * Copyright (C) 2025 Rong Zhang <i@rong.moe>
> > + *   - Unified implementation
> >    */
> >   
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> >   #include <linux/acpi.h>
> >   #include <linux/cleanup.h>
> >   #include <linux/component.h>
> > @@ -36,6 +41,25 @@
> >   #define ACPI_AC_CLASS "ac_adapter"
> >   #define ACPI_AC_NOTIFY_STATUS 0x80
> >   
> > +enum lwmi_cd_type {
> > +	LENOVO_CAPABILITY_DATA_01,
> > +};
> > +
> > +#define LWMI_CD_TABLE_ITEM(_type)		\
> > +	[_type] = {				\
> > +		.guid_string = _type##_GUID,	\
> > +		.name = #_type,			\
> > +		.type = _type,			\
> > +	}
> > +
> > +static const struct lwmi_cd_info {
> > +	const char *guid_string;
> > +	const char *name;
> > +	enum lwmi_cd_type type;
> > +} lwmi_cd_table[] = {
> > +	LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_01),
> > +};
> > +
> >   struct lwmi_cd_priv {
> >   	struct notifier_block acpi_nb; /* ACPI events */
> >   	struct wmi_device *wdev;
> > @@ -44,15 +68,19 @@ struct lwmi_cd_priv {
> >   
> >   struct cd_list {
> >   	struct mutex list_mutex; /* list R/W mutex */
> > +	enum lwmi_cd_type type;
> >   	u8 count;
> > -	struct capdata01 data[];
> > +
> > +	union {
> > +		DECLARE_FLEX_ARRAY(struct capdata01, cd01);
> > +	};
> >   };
> >   
> >   /**
> >    * lwmi_cd_component_bind() - Bind component to master device.
> >    * @cd_dev: Pointer to the lenovo-wmi-capdata driver parent device.
> >    * @om_dev: Pointer to the lenovo-wmi-other driver parent device.
> > - * @data: cd_list object pointer used to return the capability data.
> > + * @data: lwmi_cd_binder object pointer used to return the capability data.
> >    *
> >    * On lenovo-wmi-other's master bind, provide a pointer to the local capdata
> >    * list. This is used to call lwmi_cd*_get_data to look up attribute data
> > @@ -64,9 +92,15 @@ static int lwmi_cd_component_bind(struct device *cd_dev,
> >   				  struct device *om_dev, void *data)
> >   {
> >   	struct lwmi_cd_priv *priv = dev_get_drvdata(cd_dev);
> > -	struct cd_list **cd_list = data;
> > +	struct lwmi_cd_binder *binder = data;
> >   
> > -	*cd_list = priv->list;
> > +	switch (priv->list->type) {
> > +	case LENOVO_CAPABILITY_DATA_01:
> > +		binder->cd01_list = priv->list;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> >   
> >   	return 0;
> >   }
> > @@ -76,30 +110,33 @@ static const struct component_ops lwmi_cd_component_ops = {
> >   };
> >   
> >   /**
> > - * lwmi_cd01_get_data - Get the data of the specified attribute
> > + * lwmi_cd*_get_data - Get the data of the specified attribute
> >    * @list: The lenovo-wmi-capdata pointer to its cd_list struct.
> >    * @attribute_id: The capdata attribute ID to be found.
> > - * @output: Pointer to a capdata01 struct to return the data.
> > + * @output: Pointer to a capdata* struct to return the data.
> >    *
> > - * Retrieves the capability data 01 struct pointer for the given
> > - * attribute for its specified thermal mode.
> > + * Retrieves the capability data struct pointer for the given
> > + * attribute.
> >    *
> >    * Return: 0 on success, or -EINVAL.
> >    */
> > -int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output)
> > -{
> > -	u8 idx;
> > -
> > -	guard(mutex)(&list->list_mutex);
> > -	for (idx = 0; idx < list->count; idx++) {
> > -		if (list->data[idx].id != attribute_id)
> > -			continue;
> > -		memcpy(output, &list->data[idx], sizeof(list->data[idx]));
> > -		return 0;
> > +#define DEF_LWMI_CDXX_GET_DATA(_cdxx, _cd_type, _output_t)					\
> > +	int lwmi_##_cdxx##_get_data(struct cd_list *list, u32 attribute_id, _output_t *output)	\
> > +	{											\
> > +		u8 idx;										\
> > +		if (WARN_ON(list->type != _cd_type))						\
> > +			return -EINVAL;								\
> > +		guard(mutex)(&list->list_mutex);						\
> > +		for (idx = 0; idx < list->count; idx++) {					\
> > +			if (list->_cdxx[idx].id != attribute_id)				\
> > +				continue;							\
> > +			memcpy(output, &list->_cdxx[idx], sizeof(list->_cdxx[idx]));		\
> > +			return 0;								\
> > +		}										\
> > +		return -EINVAL;									\
> >   	}
> >   
> > -	return -EINVAL;
> > -}
> > +DEF_LWMI_CDXX_GET_DATA(cd01, LENOVO_CAPABILITY_DATA_01, struct capdata01);
> >   EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
> >   
> >   /**
> > @@ -112,10 +149,21 @@ EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
> >    */
> >   static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> >   {
> > +	size_t size;
> >   	int idx;
> > +	void *p;
> > +
> > +	switch (priv->list->type) {
> > +	case LENOVO_CAPABILITY_DATA_01:
> > +		p = &priv->list->cd01[0];
> > +		size = sizeof(priv->list->cd01[0]);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> >   
> >   	guard(mutex)(&priv->list->list_mutex);
> > -	for (idx = 0; idx < priv->list->count; idx++) {
> > +	for (idx = 0; idx < priv->list->count; idx++, p += size) {
> >   		union acpi_object *ret_obj __free(kfree) = NULL;
> >   
> >   		ret_obj = wmidev_block_query(priv->wdev, idx);
> > @@ -123,11 +171,10 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> >   			return -ENODEV;
> >   
> >   		if (ret_obj->type != ACPI_TYPE_BUFFER ||
> > -		    ret_obj->buffer.length < sizeof(priv->list->data[idx]))
> > +		    ret_obj->buffer.length < size)
> >   			continue;
> >   
> > -		memcpy(&priv->list->data[idx], ret_obj->buffer.pointer,
> > -		       ret_obj->buffer.length);
> > +		memcpy(p, ret_obj->buffer.pointer, size);
> >   	}
> >   
> >   	return 0;
> > @@ -136,20 +183,28 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> >   /**
> >    * lwmi_cd_alloc() - Allocate a cd_list struct in drvdata
> >    * @priv: lenovo-wmi-capdata driver data.
> > + * @type: The type of capability data.
> >    *
> >    * Allocate a cd_list struct large enough to contain data from all WMI data
> >    * blocks provided by the interface.
> >    *
> >    * Return: 0 on success, or an error.
> >    */
> > -static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> > +static int lwmi_cd_alloc(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
> >   {
> >   	struct cd_list *list;
> >   	size_t list_size;
> >   	int count, ret;
> >   
> >   	count = wmidev_instance_count(priv->wdev);
> > -	list_size = struct_size(list, data, count);
> > +
> > +	switch (type) {
> > +	case LENOVO_CAPABILITY_DATA_01:
> > +		list_size = struct_size(list, cd01, count);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> >   
> >   	list = devm_kzalloc(&priv->wdev->dev, list_size, GFP_KERNEL);
> >   	if (!list)
> > @@ -159,6 +214,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> >   	if (ret)
> >   		return ret;
> >   
> > +	list->type = type;
> >   	list->count = count;
> >   	priv->list = list;
> >   
> > @@ -168,6 +224,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> >   /**
> >    * lwmi_cd_setup() - Cache all WMI data block information
> >    * @priv: lenovo-wmi-capdata driver data.
> > + * @type: The type of capability data.
> >    *
> >    * Allocate a cd_list struct large enough to contain data from all WMI data
> >    * blocks provided by the interface. Then loop through each data block and
> > @@ -175,11 +232,11 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> >    *
> >    * Return: 0 on success, or an error code.
> >    */
> > -static int lwmi_cd_setup(struct lwmi_cd_priv *priv)
> > +static int lwmi_cd_setup(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
> >   {
> >   	int ret;
> >   
> > -	ret = lwmi_cd_alloc(priv);
> > +	ret = lwmi_cd_alloc(priv, type);
> >   	if (ret)
> >   		return ret;
> >   
> > @@ -235,9 +292,13 @@ static void lwmi_cd01_unregister(void *data)
> >   
> >   static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
> >   {
> > +	const struct lwmi_cd_info *info = context;
> >   	struct lwmi_cd_priv *priv;
> >   	int ret;
> >   
> > +	if (!info)
> > +		return -EINVAL;
> > +
> >   	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> >   	if (!priv)
> >   		return -ENOMEM;
> > @@ -245,21 +306,34 @@ static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
> >   	priv->wdev = wdev;
> >   	dev_set_drvdata(&wdev->dev, priv);
> >   
> > -	ret = lwmi_cd_setup(priv);
> > +	ret = lwmi_cd_setup(priv, info->type);
> >   	if (ret)
> > -		return ret;
> > +		goto out;
> >   
> > -	priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
> > +	if (info->type == LENOVO_CAPABILITY_DATA_01) {
> > +		priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
> >   
> > -	ret = register_acpi_notifier(&priv->acpi_nb);
> > -	if (ret)
> > -		return ret;
> > +		ret = register_acpi_notifier(&priv->acpi_nb);
> > +		if (ret)
> > +			goto out;
> >   
> > -	ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister, &priv->acpi_nb);
> > -	if (ret)
> > -		return ret;
> > +		ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister,
> > +					       &priv->acpi_nb);
> > +		if (ret)
> > +			goto out;
> > +	}
> > +
> > +	ret = component_add(&wdev->dev, &lwmi_cd_component_ops);
> >   
> > -	return component_add(&wdev->dev, &lwmi_cd_component_ops);
> > +out:
> > +	if (ret) {
> > +		dev_err(&wdev->dev, "failed to register %s: %d\n",
> > +			info->name, ret);
> > +	} else {
> > +		dev_info(&wdev->dev, "registered %s with %u items\n",
> > +			 info->name, priv->list->count);
> > +	}
> > +	return ret;
> >   }
> >   
> >   static void lwmi_cd_remove(struct wmi_device *wdev)
> > @@ -267,8 +341,12 @@ static void lwmi_cd_remove(struct wmi_device *wdev)
> >   	component_del(&wdev->dev, &lwmi_cd_component_ops);
> >   }
> >   
> > +#define LWMI_CD_WDEV_ID(_type)				\
> > +	.guid_string = _type##_GUID,			\
> > +	.context = &lwmi_cd_table[_type]
> > +
> >   static const struct wmi_device_id lwmi_cd_id_table[] = {
> > -	{ LENOVO_CAPABILITY_DATA_01_GUID, NULL },
> > +	{ LWMI_CD_WDEV_ID(LENOVO_CAPABILITY_DATA_01) },
> >   	{}
> >   };
> >   
> > @@ -284,21 +362,61 @@ static struct wmi_driver lwmi_cd_driver = {
> >   };
> >   
> >   /**
> > - * lwmi_cd01_match() - Match rule for the master driver.
> > - * @dev: Pointer to the capability data 01 parent device.
> > - * @data: Unused void pointer for passing match criteria.
> > + * lwmi_cd_match() - Match rule for the master driver.
> > + * @dev: Pointer to the capability data parent device.
> > + * @type: Pointer to capability data type (enum lwmi_cd_type *) to match.
> >    *
> >    * Return: int.
> >    */
> > -int lwmi_cd01_match(struct device *dev, void *data)
> > +static int lwmi_cd_match(struct device *dev, void *type)
> > +{
> > +	struct lwmi_cd_priv *priv;
> > +
> > +	if (dev->driver != &lwmi_cd_driver.driver)
> > +		return false;
> > +
> > +	priv = dev_get_drvdata(dev);
> > +	return priv->list->type == *(enum lwmi_cd_type *)type;
> > +}
> > +
> > +/**
> > + * lwmi_cd_match_add_all() - Add all match rule for the master driver.
> > + * @master: Pointer to the master device.
> > + * @matchptr: Pointer to the returned component_match pointer.
> > + *
> > + * Adds all component matches to the list stored in @matchptr for the @master
> > + * device. @matchptr must be initialized to NULL. This matches all available
> > + * capdata types on the machine.
> > + */
> > +void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr)
> >   {
> > -	return dev->driver == &lwmi_cd_driver.driver;
> > +	int i;
> > +
> > +	if (WARN_ON(*matchptr))
> > +		return;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(lwmi_cd_table); i++) {
> > +		if (!lwmi_cd_table[i].guid_string ||
> > +		    !wmi_has_guid(lwmi_cd_table[i].guid_string))
> > +			continue;
> 
> I am still not happy about this. AFAIK as soon as the ordinary capdata WMI devices are bound together,
> the firmware tells you whether or not the additional fan data WMI device is available. Maybe you can do
> something like this:
> 
> 1. Bind both capdata WMI devices as usual.
> 2. Check if a fan data WMI device is available (you can use a DMI-based quirk list for devices were
>     the firmware reports invalid data).
> 3. Register an additional component that binds to the fan data WMI device.
> 4. Bind both the additional component and the component registered by the fan data WMI device.
> 5. Register the hwmon device with additional fan data.
> 
> If the fan data WMI device is not available, you can simply skip steps 3 and 4 and simply
> register the hwmon device with any additional fan data.
> 
> What do you think?

Thanks for your suggestion. Will adopt it in v4.

> Thanks,
> Armin Wolf

Thanks,
Rong

> > +
> > +		component_match_add(master, matchptr, lwmi_cd_match,
> > +				    (void *)&lwmi_cd_table[i].type);
> > +		if (IS_ERR(matchptr))
> > +			return;
> > +	}
> > +
> > +	if (!*matchptr) {
> > +		pr_warn("a master driver requested capability data, but nothing is available\n");
> > +		*matchptr = ERR_PTR(-ENODEV);
> > +	}
> >   }
> > -EXPORT_SYMBOL_NS_GPL(lwmi_cd01_match, "LENOVO_WMI_CD");
> > +EXPORT_SYMBOL_NS_GPL(lwmi_cd_match_add_all, "LENOVO_WMI_CD");
> >   
> >   module_wmi_driver(lwmi_cd_driver);
> >   
> >   MODULE_DEVICE_TABLE(wmi, lwmi_cd_id_table);
> >   MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
> > +MODULE_AUTHOR("Rong Zhang <i@rong.moe>");
> >   MODULE_DESCRIPTION("Lenovo Capability Data WMI Driver");
> >   MODULE_LICENSE("GPL");
> > diff --git a/drivers/platform/x86/lenovo/wmi-capdata.h b/drivers/platform/x86/lenovo/wmi-capdata.h
> > index 2a4746e38ad4..1e5fce7836cb 100644
> > --- a/drivers/platform/x86/lenovo/wmi-capdata.h
> > +++ b/drivers/platform/x86/lenovo/wmi-capdata.h
> > @@ -7,6 +7,7 @@
> >   
> >   #include <linux/types.h>
> >   
> > +struct component_match;
> >   struct device;
> >   struct cd_list;
> >   
> > @@ -19,7 +20,11 @@ struct capdata01 {
> >   	u32 max_value;
> >   };
> >   
> > +struct lwmi_cd_binder {
> > +	struct cd_list *cd01_list;
> > +};
> > +
> >   int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output);
> > -int lwmi_cd01_match(struct device *dev, void *data);
> > +void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr);
> >   
> >   #endif /* !_LENOVO_WMI_CAPDATA_H_ */
> > diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
> > index c6dc1b4cff84..20c6ff0be37a 100644
> > --- a/drivers/platform/x86/lenovo/wmi-other.c
> > +++ b/drivers/platform/x86/lenovo/wmi-other.c
> > @@ -579,14 +579,14 @@ static void lwmi_om_fw_attr_remove(struct lwmi_om_priv *priv)
> >   static int lwmi_om_master_bind(struct device *dev)
> >   {
> >   	struct lwmi_om_priv *priv = dev_get_drvdata(dev);
> > -	struct cd_list *tmp_list;
> > +	struct lwmi_cd_binder binder = { 0 };
> >   	int ret;
> >   
> > -	ret = component_bind_all(dev, &tmp_list);
> > +	ret = component_bind_all(dev, &binder);
> >   	if (ret)
> >   		return ret;
> >   
> > -	priv->cd01_list = tmp_list;
> > +	priv->cd01_list = binder.cd01_list;
> >   	if (!priv->cd01_list)
> >   		return -ENODEV;
> >   
> > @@ -618,6 +618,7 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
> >   {
> >   	struct component_match *master_match = NULL;
> >   	struct lwmi_om_priv *priv;
> > +	int ret;
> >   
> >   	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> >   	if (!priv)
> > @@ -626,12 +627,26 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
> >   	priv->wdev = wdev;
> >   	dev_set_drvdata(&wdev->dev, priv);
> >   
> > -	component_match_add(&wdev->dev, &master_match, lwmi_cd01_match, NULL);
> > +	lwmi_cd_match_add_all(&wdev->dev, &master_match);
> >   	if (IS_ERR(master_match))
> >   		return PTR_ERR(master_match);
> >   
> > -	return component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
> > -					       master_match);
> > +	ret = component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
> > +					      master_match);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (likely(component_master_is_bound(&wdev->dev, &lwmi_om_master_ops)))
> > +		return 0;
> > +
> > +	/*
> > +	 * The bind callbacks of both master and components were never called in
> > +	 * this case - this driver won't work at all. Failing...
> > +	 */
> > +	dev_err(&wdev->dev, "unbound master; is any component failing to be probed?");
> > +
> > +	component_master_del(&wdev->dev, &lwmi_om_master_ops);
> > +	return -EXDEV;
> >   }
> >   
> >   static void lwmi_other_remove(struct wmi_device *wdev)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 3/6] platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability Data
  2025-11-04 20:20   ` Armin Wolf
  2025-11-04 20:43     ` Rong Zhang
@ 2025-11-06 12:36     ` Rong Zhang
  2025-11-10  0:02       ` Armin Wolf
  1 sibling, 1 reply; 14+ messages in thread
From: Rong Zhang @ 2025-11-06 12:36 UTC (permalink / raw)
  To: Armin Wolf, Mark Pearson, Derek J. Clark, Hans de Goede,
	Ilpo Järvinen
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Danilo Krummrich,
	dri-devel, Guenter Roeck, platform-driver-x86, linux-kernel,
	linux-hwmon

Hi Armin,

On Tue, 2025-11-04 at 21:20 +0100, Armin Wolf wrote:
> Am 31.10.25 um 16:51 schrieb Rong Zhang:
> 
> > The current implementation are heavily bound to capdata01. Rewrite it so
> > that it is suitable to utilize other Capability Data as well.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > ---
> > Changes in v2:
> > - Fix function parameter 'type' not described in 'lwmi_cd_match' (thanks
> >    kernel test bot)
> > ---
> >   drivers/platform/x86/lenovo/wmi-capdata.c | 208 +++++++++++++++++-----
> >   drivers/platform/x86/lenovo/wmi-capdata.h |   7 +-
> >   drivers/platform/x86/lenovo/wmi-other.c   |  27 ++-
> >   3 files changed, 190 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
> > index c5e74b2bfeb3..1f7fc09b7c3f 100644
> > --- a/drivers/platform/x86/lenovo/wmi-capdata.c
> > +++ b/drivers/platform/x86/lenovo/wmi-capdata.c
> > @@ -12,8 +12,13 @@
> >    *
> >    * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@gmail.com>
> >    *   - Initial implementation (formerly named lenovo-wmi-capdata01)
> > + *
> > + * Copyright (C) 2025 Rong Zhang <i@rong.moe>
> > + *   - Unified implementation
> >    */
> >   
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> >   #include <linux/acpi.h>
> >   #include <linux/cleanup.h>
> >   #include <linux/component.h>
> > @@ -36,6 +41,25 @@
> >   #define ACPI_AC_CLASS "ac_adapter"
> >   #define ACPI_AC_NOTIFY_STATUS 0x80
> >   
> > +enum lwmi_cd_type {
> > +	LENOVO_CAPABILITY_DATA_01,
> > +};
> > +
> > +#define LWMI_CD_TABLE_ITEM(_type)		\
> > +	[_type] = {				\
> > +		.guid_string = _type##_GUID,	\
> > +		.name = #_type,			\
> > +		.type = _type,			\
> > +	}
> > +
> > +static const struct lwmi_cd_info {
> > +	const char *guid_string;
> > +	const char *name;
> > +	enum lwmi_cd_type type;
> > +} lwmi_cd_table[] = {
> > +	LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_01),
> > +};
> > +
> >   struct lwmi_cd_priv {
> >   	struct notifier_block acpi_nb; /* ACPI events */
> >   	struct wmi_device *wdev;
> > @@ -44,15 +68,19 @@ struct lwmi_cd_priv {
> >   
> >   struct cd_list {
> >   	struct mutex list_mutex; /* list R/W mutex */
> > +	enum lwmi_cd_type type;
> >   	u8 count;
> > -	struct capdata01 data[];
> > +
> > +	union {
> > +		DECLARE_FLEX_ARRAY(struct capdata01, cd01);
> > +	};
> >   };
> >   
> >   /**
> >    * lwmi_cd_component_bind() - Bind component to master device.
> >    * @cd_dev: Pointer to the lenovo-wmi-capdata driver parent device.
> >    * @om_dev: Pointer to the lenovo-wmi-other driver parent device.
> > - * @data: cd_list object pointer used to return the capability data.
> > + * @data: lwmi_cd_binder object pointer used to return the capability data.
> >    *
> >    * On lenovo-wmi-other's master bind, provide a pointer to the local capdata
> >    * list. This is used to call lwmi_cd*_get_data to look up attribute data
> > @@ -64,9 +92,15 @@ static int lwmi_cd_component_bind(struct device *cd_dev,
> >   				  struct device *om_dev, void *data)
> >   {
> >   	struct lwmi_cd_priv *priv = dev_get_drvdata(cd_dev);
> > -	struct cd_list **cd_list = data;
> > +	struct lwmi_cd_binder *binder = data;
> >   
> > -	*cd_list = priv->list;
> > +	switch (priv->list->type) {
> > +	case LENOVO_CAPABILITY_DATA_01:
> > +		binder->cd01_list = priv->list;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> >   
> >   	return 0;
> >   }
> > @@ -76,30 +110,33 @@ static const struct component_ops lwmi_cd_component_ops = {
> >   };
> >   
> >   /**
> > - * lwmi_cd01_get_data - Get the data of the specified attribute
> > + * lwmi_cd*_get_data - Get the data of the specified attribute
> >    * @list: The lenovo-wmi-capdata pointer to its cd_list struct.
> >    * @attribute_id: The capdata attribute ID to be found.
> > - * @output: Pointer to a capdata01 struct to return the data.
> > + * @output: Pointer to a capdata* struct to return the data.
> >    *
> > - * Retrieves the capability data 01 struct pointer for the given
> > - * attribute for its specified thermal mode.
> > + * Retrieves the capability data struct pointer for the given
> > + * attribute.
> >    *
> >    * Return: 0 on success, or -EINVAL.
> >    */
> > -int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output)
> > -{
> > -	u8 idx;
> > -
> > -	guard(mutex)(&list->list_mutex);
> > -	for (idx = 0; idx < list->count; idx++) {
> > -		if (list->data[idx].id != attribute_id)
> > -			continue;
> > -		memcpy(output, &list->data[idx], sizeof(list->data[idx]));
> > -		return 0;
> > +#define DEF_LWMI_CDXX_GET_DATA(_cdxx, _cd_type, _output_t)					\
> > +	int lwmi_##_cdxx##_get_data(struct cd_list *list, u32 attribute_id, _output_t *output)	\
> > +	{											\
> > +		u8 idx;										\
> > +		if (WARN_ON(list->type != _cd_type))						\
> > +			return -EINVAL;								\
> > +		guard(mutex)(&list->list_mutex);						\
> > +		for (idx = 0; idx < list->count; idx++) {					\
> > +			if (list->_cdxx[idx].id != attribute_id)				\
> > +				continue;							\
> > +			memcpy(output, &list->_cdxx[idx], sizeof(list->_cdxx[idx]));		\
> > +			return 0;								\
> > +		}										\
> > +		return -EINVAL;									\
> >   	}
> >   
> > -	return -EINVAL;
> > -}
> > +DEF_LWMI_CDXX_GET_DATA(cd01, LENOVO_CAPABILITY_DATA_01, struct capdata01);
> >   EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
> >   
> >   /**
> > @@ -112,10 +149,21 @@ EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
> >    */
> >   static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> >   {
> > +	size_t size;
> >   	int idx;
> > +	void *p;
> > +
> > +	switch (priv->list->type) {
> > +	case LENOVO_CAPABILITY_DATA_01:
> > +		p = &priv->list->cd01[0];
> > +		size = sizeof(priv->list->cd01[0]);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> >   
> >   	guard(mutex)(&priv->list->list_mutex);
> > -	for (idx = 0; idx < priv->list->count; idx++) {
> > +	for (idx = 0; idx < priv->list->count; idx++, p += size) {
> >   		union acpi_object *ret_obj __free(kfree) = NULL;
> >   
> >   		ret_obj = wmidev_block_query(priv->wdev, idx);
> > @@ -123,11 +171,10 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> >   			return -ENODEV;
> >   
> >   		if (ret_obj->type != ACPI_TYPE_BUFFER ||
> > -		    ret_obj->buffer.length < sizeof(priv->list->data[idx]))
> > +		    ret_obj->buffer.length < size)
> >   			continue;
> >   
> > -		memcpy(&priv->list->data[idx], ret_obj->buffer.pointer,
> > -		       ret_obj->buffer.length);
> > +		memcpy(p, ret_obj->buffer.pointer, size);
> >   	}
> >   
> >   	return 0;
> > @@ -136,20 +183,28 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> >   /**
> >    * lwmi_cd_alloc() - Allocate a cd_list struct in drvdata
> >    * @priv: lenovo-wmi-capdata driver data.
> > + * @type: The type of capability data.
> >    *
> >    * Allocate a cd_list struct large enough to contain data from all WMI data
> >    * blocks provided by the interface.
> >    *
> >    * Return: 0 on success, or an error.
> >    */
> > -static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> > +static int lwmi_cd_alloc(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
> >   {
> >   	struct cd_list *list;
> >   	size_t list_size;
> >   	int count, ret;
> >   
> >   	count = wmidev_instance_count(priv->wdev);
> > -	list_size = struct_size(list, data, count);
> > +
> > +	switch (type) {
> > +	case LENOVO_CAPABILITY_DATA_01:
> > +		list_size = struct_size(list, cd01, count);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> >   
> >   	list = devm_kzalloc(&priv->wdev->dev, list_size, GFP_KERNEL);
> >   	if (!list)
> > @@ -159,6 +214,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> >   	if (ret)
> >   		return ret;
> >   
> > +	list->type = type;
> >   	list->count = count;
> >   	priv->list = list;
> >   
> > @@ -168,6 +224,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> >   /**
> >    * lwmi_cd_setup() - Cache all WMI data block information
> >    * @priv: lenovo-wmi-capdata driver data.
> > + * @type: The type of capability data.
> >    *
> >    * Allocate a cd_list struct large enough to contain data from all WMI data
> >    * blocks provided by the interface. Then loop through each data block and
> > @@ -175,11 +232,11 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> >    *
> >    * Return: 0 on success, or an error code.
> >    */
> > -static int lwmi_cd_setup(struct lwmi_cd_priv *priv)
> > +static int lwmi_cd_setup(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
> >   {
> >   	int ret;
> >   
> > -	ret = lwmi_cd_alloc(priv);
> > +	ret = lwmi_cd_alloc(priv, type);
> >   	if (ret)
> >   		return ret;
> >   
> > @@ -235,9 +292,13 @@ static void lwmi_cd01_unregister(void *data)
> >   
> >   static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
> >   {
> > +	const struct lwmi_cd_info *info = context;
> >   	struct lwmi_cd_priv *priv;
> >   	int ret;
> >   
> > +	if (!info)
> > +		return -EINVAL;
> > +
> >   	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> >   	if (!priv)
> >   		return -ENOMEM;
> > @@ -245,21 +306,34 @@ static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
> >   	priv->wdev = wdev;
> >   	dev_set_drvdata(&wdev->dev, priv);
> >   
> > -	ret = lwmi_cd_setup(priv);
> > +	ret = lwmi_cd_setup(priv, info->type);
> >   	if (ret)
> > -		return ret;
> > +		goto out;
> >   
> > -	priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
> > +	if (info->type == LENOVO_CAPABILITY_DATA_01) {
> > +		priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
> >   
> > -	ret = register_acpi_notifier(&priv->acpi_nb);
> > -	if (ret)
> > -		return ret;
> > +		ret = register_acpi_notifier(&priv->acpi_nb);
> > +		if (ret)
> > +			goto out;
> >   
> > -	ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister, &priv->acpi_nb);
> > -	if (ret)
> > -		return ret;
> > +		ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister,
> > +					       &priv->acpi_nb);
> > +		if (ret)
> > +			goto out;
> > +	}
> > +
> > +	ret = component_add(&wdev->dev, &lwmi_cd_component_ops);
> >   
> > -	return component_add(&wdev->dev, &lwmi_cd_component_ops);
> > +out:
> > +	if (ret) {
> > +		dev_err(&wdev->dev, "failed to register %s: %d\n",
> > +			info->name, ret);
> > +	} else {
> > +		dev_info(&wdev->dev, "registered %s with %u items\n",
> > +			 info->name, priv->list->count);
> > +	}
> > +	return ret;
> >   }
> >   
> >   static void lwmi_cd_remove(struct wmi_device *wdev)
> > @@ -267,8 +341,12 @@ static void lwmi_cd_remove(struct wmi_device *wdev)
> >   	component_del(&wdev->dev, &lwmi_cd_component_ops);
> >   }
> >   
> > +#define LWMI_CD_WDEV_ID(_type)				\
> > +	.guid_string = _type##_GUID,			\
> > +	.context = &lwmi_cd_table[_type]
> > +
> >   static const struct wmi_device_id lwmi_cd_id_table[] = {
> > -	{ LENOVO_CAPABILITY_DATA_01_GUID, NULL },
> > +	{ LWMI_CD_WDEV_ID(LENOVO_CAPABILITY_DATA_01) },
> >   	{}
> >   };
> >   
> > @@ -284,21 +362,61 @@ static struct wmi_driver lwmi_cd_driver = {
> >   };
> >   
> >   /**
> > - * lwmi_cd01_match() - Match rule for the master driver.
> > - * @dev: Pointer to the capability data 01 parent device.
> > - * @data: Unused void pointer for passing match criteria.
> > + * lwmi_cd_match() - Match rule for the master driver.
> > + * @dev: Pointer to the capability data parent device.
> > + * @type: Pointer to capability data type (enum lwmi_cd_type *) to match.
> >    *
> >    * Return: int.
> >    */
> > -int lwmi_cd01_match(struct device *dev, void *data)
> > +static int lwmi_cd_match(struct device *dev, void *type)
> > +{
> > +	struct lwmi_cd_priv *priv;
> > +
> > +	if (dev->driver != &lwmi_cd_driver.driver)
> > +		return false;
> > +
> > +	priv = dev_get_drvdata(dev);
> > +	return priv->list->type == *(enum lwmi_cd_type *)type;
> > +}
> > +
> > +/**
> > + * lwmi_cd_match_add_all() - Add all match rule for the master driver.
> > + * @master: Pointer to the master device.
> > + * @matchptr: Pointer to the returned component_match pointer.
> > + *
> > + * Adds all component matches to the list stored in @matchptr for the @master
> > + * device. @matchptr must be initialized to NULL. This matches all available
> > + * capdata types on the machine.
> > + */
> > +void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr)
> >   {
> > -	return dev->driver == &lwmi_cd_driver.driver;
> > +	int i;
> > +
> > +	if (WARN_ON(*matchptr))
> > +		return;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(lwmi_cd_table); i++) {
> > +		if (!lwmi_cd_table[i].guid_string ||
> > +		    !wmi_has_guid(lwmi_cd_table[i].guid_string))
> > +			continue;
> 
> I am still not happy about this. AFAIK as soon as the ordinary capdata WMI devices are bound together,
> the firmware tells you whether or not the additional fan data WMI device is available. Maybe you can do
> something like this:
> 
> 1. Bind both capdata WMI devices as usual.
> 2. Check if a fan data WMI device is available (you can use a DMI-based quirk list for devices were
>     the firmware reports invalid data).
> 3. Register an additional component that binds to the fan data WMI device.
> 4. Bind both the additional component and the component registered by the fan data WMI device.
> 5. Register the hwmon device with additional fan data.
> 
> If the fan data WMI device is not available, you can simply skip steps 3 and 4 and simply
> register the hwmon device with any additional fan data.
> 
> What do you think?

I tried your approach. I looks pretty well except for step 4:

   debugfs: 'DC2A8805-3A8C-41BA-A6F7-092E0089CD3B-21' already exists in 'device_component'

Moreover, component_[un]bind_all() calls __aggregate_find() with ops ==
NULL, which implies that it can't really distinguish the two aggregate
devices we have. Thus, we are rely on the insertion sequence of
aggregate_devices (see component_master_add_with_match()) to make it
just work. Pseudo code:

   lwmi_other_probe()
   {
   	component_match_add(..., lwmi_cd00_match, ...);
   	component_match_add(..., lwmi_cd01_match, ...);
   	component_master_add_with_match(..., &ops1, ...);
   	component_match_add(..., lwmi_cd_fan_match, ...);
   	component_master_add_with_match(..., &ops2, ...);
   }
   
   lwmi_other_remove()
   {
   	/* This just works (TM). */
   	component_master_del(..., &ops2); /* unbinds cd_fan */
   	component_master_del(..., &ops1); /* unbinds cd00/01 */
   
   	/* WARNING: at drivers/base/component.c:589 */
   	/*
   	component_master_del(..., &ops1); /* unbinds cd_fan!!! */
   	component_master_del(..., &ops2); /* unbinds cd_fan!!! */
   	*/
   }

It seems that the component framework is not prepared to allow multi-
aggregation master device.

Since we are talking about the component framework: all efforts we made
here is to work around one of its limitations -- all components must be
found or else it won't bring up the aggregate device. Do you think
allowing partially bound aggregate device a good idea? E.g., adding a
flag, indicating the master device opts in to such behavior, to the
definition of struct component_master_ops. I can prepare a separate
patch for that.

CC'ing maintainers and lists of driver core and component framework.

> Thanks,
> Armin Wolf

Thanks,
Rong

> > +
> > +		component_match_add(master, matchptr, lwmi_cd_match,
> > +				    (void *)&lwmi_cd_table[i].type);
> > +		if (IS_ERR(matchptr))
> > +			return;
> > +	}
> > +
> > +	if (!*matchptr) {
> > +		pr_warn("a master driver requested capability data, but nothing is available\n");
> > +		*matchptr = ERR_PTR(-ENODEV);
> > +	}
> >   }
> > -EXPORT_SYMBOL_NS_GPL(lwmi_cd01_match, "LENOVO_WMI_CD");
> > +EXPORT_SYMBOL_NS_GPL(lwmi_cd_match_add_all, "LENOVO_WMI_CD");
> >   
> >   module_wmi_driver(lwmi_cd_driver);
> >   
> >   MODULE_DEVICE_TABLE(wmi, lwmi_cd_id_table);
> >   MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
> > +MODULE_AUTHOR("Rong Zhang <i@rong.moe>");
> >   MODULE_DESCRIPTION("Lenovo Capability Data WMI Driver");
> >   MODULE_LICENSE("GPL");
> > diff --git a/drivers/platform/x86/lenovo/wmi-capdata.h b/drivers/platform/x86/lenovo/wmi-capdata.h
> > index 2a4746e38ad4..1e5fce7836cb 100644
> > --- a/drivers/platform/x86/lenovo/wmi-capdata.h
> > +++ b/drivers/platform/x86/lenovo/wmi-capdata.h
> > @@ -7,6 +7,7 @@
> >   
> >   #include <linux/types.h>
> >   
> > +struct component_match;
> >   struct device;
> >   struct cd_list;
> >   
> > @@ -19,7 +20,11 @@ struct capdata01 {
> >   	u32 max_value;
> >   };
> >   
> > +struct lwmi_cd_binder {
> > +	struct cd_list *cd01_list;
> > +};
> > +
> >   int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output);
> > -int lwmi_cd01_match(struct device *dev, void *data);
> > +void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr);
> >   
> >   #endif /* !_LENOVO_WMI_CAPDATA_H_ */
> > diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
> > index c6dc1b4cff84..20c6ff0be37a 100644
> > --- a/drivers/platform/x86/lenovo/wmi-other.c
> > +++ b/drivers/platform/x86/lenovo/wmi-other.c
> > @@ -579,14 +579,14 @@ static void lwmi_om_fw_attr_remove(struct lwmi_om_priv *priv)
> >   static int lwmi_om_master_bind(struct device *dev)
> >   {
> >   	struct lwmi_om_priv *priv = dev_get_drvdata(dev);
> > -	struct cd_list *tmp_list;
> > +	struct lwmi_cd_binder binder = { 0 };
> >   	int ret;
> >   
> > -	ret = component_bind_all(dev, &tmp_list);
> > +	ret = component_bind_all(dev, &binder);
> >   	if (ret)
> >   		return ret;
> >   
> > -	priv->cd01_list = tmp_list;
> > +	priv->cd01_list = binder.cd01_list;
> >   	if (!priv->cd01_list)
> >   		return -ENODEV;
> >   
> > @@ -618,6 +618,7 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
> >   {
> >   	struct component_match *master_match = NULL;
> >   	struct lwmi_om_priv *priv;
> > +	int ret;
> >   
> >   	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> >   	if (!priv)
> > @@ -626,12 +627,26 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
> >   	priv->wdev = wdev;
> >   	dev_set_drvdata(&wdev->dev, priv);
> >   
> > -	component_match_add(&wdev->dev, &master_match, lwmi_cd01_match, NULL);
> > +	lwmi_cd_match_add_all(&wdev->dev, &master_match);
> >   	if (IS_ERR(master_match))
> >   		return PTR_ERR(master_match);
> >   
> > -	return component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
> > -					       master_match);
> > +	ret = component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
> > +					      master_match);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (likely(component_master_is_bound(&wdev->dev, &lwmi_om_master_ops)))
> > +		return 0;
> > +
> > +	/*
> > +	 * The bind callbacks of both master and components were never called in
> > +	 * this case - this driver won't work at all. Failing...
> > +	 */
> > +	dev_err(&wdev->dev, "unbound master; is any component failing to be probed?");
> > +
> > +	component_master_del(&wdev->dev, &lwmi_om_master_ops);
> > +	return -EXDEV;
> >   }
> >   
> >   static void lwmi_other_remove(struct wmi_device *wdev)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 3/6] platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability Data
  2025-11-06 12:36     ` Rong Zhang
@ 2025-11-10  0:02       ` Armin Wolf
  2025-11-10 17:40         ` Rong Zhang
  0 siblings, 1 reply; 14+ messages in thread
From: Armin Wolf @ 2025-11-10  0:02 UTC (permalink / raw)
  To: Rong Zhang, Mark Pearson, Derek J. Clark, Hans de Goede,
	Ilpo Järvinen
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Danilo Krummrich,
	dri-devel, Guenter Roeck, platform-driver-x86, linux-kernel,
	linux-hwmon

Am 06.11.25 um 13:36 schrieb Rong Zhang:

> Hi Armin,
>
> On Tue, 2025-11-04 at 21:20 +0100, Armin Wolf wrote:
>> Am 31.10.25 um 16:51 schrieb Rong Zhang:
>>
>>> The current implementation are heavily bound to capdata01. Rewrite it so
>>> that it is suitable to utilize other Capability Data as well.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Rong Zhang <i@rong.moe>
>>> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
>>> Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
>>> ---
>>> Changes in v2:
>>> - Fix function parameter 'type' not described in 'lwmi_cd_match' (thanks
>>>     kernel test bot)
>>> ---
>>>    drivers/platform/x86/lenovo/wmi-capdata.c | 208 +++++++++++++++++-----
>>>    drivers/platform/x86/lenovo/wmi-capdata.h |   7 +-
>>>    drivers/platform/x86/lenovo/wmi-other.c   |  27 ++-
>>>    3 files changed, 190 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
>>> index c5e74b2bfeb3..1f7fc09b7c3f 100644
>>> --- a/drivers/platform/x86/lenovo/wmi-capdata.c
>>> +++ b/drivers/platform/x86/lenovo/wmi-capdata.c
>>> @@ -12,8 +12,13 @@
>>>     *
>>>     * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@gmail.com>
>>>     *   - Initial implementation (formerly named lenovo-wmi-capdata01)
>>> + *
>>> + * Copyright (C) 2025 Rong Zhang <i@rong.moe>
>>> + *   - Unified implementation
>>>     */
>>>    
>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>> +
>>>    #include <linux/acpi.h>
>>>    #include <linux/cleanup.h>
>>>    #include <linux/component.h>
>>> @@ -36,6 +41,25 @@
>>>    #define ACPI_AC_CLASS "ac_adapter"
>>>    #define ACPI_AC_NOTIFY_STATUS 0x80
>>>    
>>> +enum lwmi_cd_type {
>>> +	LENOVO_CAPABILITY_DATA_01,
>>> +};
>>> +
>>> +#define LWMI_CD_TABLE_ITEM(_type)		\
>>> +	[_type] = {				\
>>> +		.guid_string = _type##_GUID,	\
>>> +		.name = #_type,			\
>>> +		.type = _type,			\
>>> +	}
>>> +
>>> +static const struct lwmi_cd_info {
>>> +	const char *guid_string;
>>> +	const char *name;
>>> +	enum lwmi_cd_type type;
>>> +} lwmi_cd_table[] = {
>>> +	LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_01),
>>> +};
>>> +
>>>    struct lwmi_cd_priv {
>>>    	struct notifier_block acpi_nb; /* ACPI events */
>>>    	struct wmi_device *wdev;
>>> @@ -44,15 +68,19 @@ struct lwmi_cd_priv {
>>>    
>>>    struct cd_list {
>>>    	struct mutex list_mutex; /* list R/W mutex */
>>> +	enum lwmi_cd_type type;
>>>    	u8 count;
>>> -	struct capdata01 data[];
>>> +
>>> +	union {
>>> +		DECLARE_FLEX_ARRAY(struct capdata01, cd01);
>>> +	};
>>>    };
>>>    
>>>    /**
>>>     * lwmi_cd_component_bind() - Bind component to master device.
>>>     * @cd_dev: Pointer to the lenovo-wmi-capdata driver parent device.
>>>     * @om_dev: Pointer to the lenovo-wmi-other driver parent device.
>>> - * @data: cd_list object pointer used to return the capability data.
>>> + * @data: lwmi_cd_binder object pointer used to return the capability data.
>>>     *
>>>     * On lenovo-wmi-other's master bind, provide a pointer to the local capdata
>>>     * list. This is used to call lwmi_cd*_get_data to look up attribute data
>>> @@ -64,9 +92,15 @@ static int lwmi_cd_component_bind(struct device *cd_dev,
>>>    				  struct device *om_dev, void *data)
>>>    {
>>>    	struct lwmi_cd_priv *priv = dev_get_drvdata(cd_dev);
>>> -	struct cd_list **cd_list = data;
>>> +	struct lwmi_cd_binder *binder = data;
>>>    
>>> -	*cd_list = priv->list;
>>> +	switch (priv->list->type) {
>>> +	case LENOVO_CAPABILITY_DATA_01:
>>> +		binder->cd01_list = priv->list;
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>>    
>>>    	return 0;
>>>    }
>>> @@ -76,30 +110,33 @@ static const struct component_ops lwmi_cd_component_ops = {
>>>    };
>>>    
>>>    /**
>>> - * lwmi_cd01_get_data - Get the data of the specified attribute
>>> + * lwmi_cd*_get_data - Get the data of the specified attribute
>>>     * @list: The lenovo-wmi-capdata pointer to its cd_list struct.
>>>     * @attribute_id: The capdata attribute ID to be found.
>>> - * @output: Pointer to a capdata01 struct to return the data.
>>> + * @output: Pointer to a capdata* struct to return the data.
>>>     *
>>> - * Retrieves the capability data 01 struct pointer for the given
>>> - * attribute for its specified thermal mode.
>>> + * Retrieves the capability data struct pointer for the given
>>> + * attribute.
>>>     *
>>>     * Return: 0 on success, or -EINVAL.
>>>     */
>>> -int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output)
>>> -{
>>> -	u8 idx;
>>> -
>>> -	guard(mutex)(&list->list_mutex);
>>> -	for (idx = 0; idx < list->count; idx++) {
>>> -		if (list->data[idx].id != attribute_id)
>>> -			continue;
>>> -		memcpy(output, &list->data[idx], sizeof(list->data[idx]));
>>> -		return 0;
>>> +#define DEF_LWMI_CDXX_GET_DATA(_cdxx, _cd_type, _output_t)					\
>>> +	int lwmi_##_cdxx##_get_data(struct cd_list *list, u32 attribute_id, _output_t *output)	\
>>> +	{											\
>>> +		u8 idx;										\
>>> +		if (WARN_ON(list->type != _cd_type))						\
>>> +			return -EINVAL;								\
>>> +		guard(mutex)(&list->list_mutex);						\
>>> +		for (idx = 0; idx < list->count; idx++) {					\
>>> +			if (list->_cdxx[idx].id != attribute_id)				\
>>> +				continue;							\
>>> +			memcpy(output, &list->_cdxx[idx], sizeof(list->_cdxx[idx]));		\
>>> +			return 0;								\
>>> +		}										\
>>> +		return -EINVAL;									\
>>>    	}
>>>    
>>> -	return -EINVAL;
>>> -}
>>> +DEF_LWMI_CDXX_GET_DATA(cd01, LENOVO_CAPABILITY_DATA_01, struct capdata01);
>>>    EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
>>>    
>>>    /**
>>> @@ -112,10 +149,21 @@ EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
>>>     */
>>>    static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
>>>    {
>>> +	size_t size;
>>>    	int idx;
>>> +	void *p;
>>> +
>>> +	switch (priv->list->type) {
>>> +	case LENOVO_CAPABILITY_DATA_01:
>>> +		p = &priv->list->cd01[0];
>>> +		size = sizeof(priv->list->cd01[0]);
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>>    
>>>    	guard(mutex)(&priv->list->list_mutex);
>>> -	for (idx = 0; idx < priv->list->count; idx++) {
>>> +	for (idx = 0; idx < priv->list->count; idx++, p += size) {
>>>    		union acpi_object *ret_obj __free(kfree) = NULL;
>>>    
>>>    		ret_obj = wmidev_block_query(priv->wdev, idx);
>>> @@ -123,11 +171,10 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
>>>    			return -ENODEV;
>>>    
>>>    		if (ret_obj->type != ACPI_TYPE_BUFFER ||
>>> -		    ret_obj->buffer.length < sizeof(priv->list->data[idx]))
>>> +		    ret_obj->buffer.length < size)
>>>    			continue;
>>>    
>>> -		memcpy(&priv->list->data[idx], ret_obj->buffer.pointer,
>>> -		       ret_obj->buffer.length);
>>> +		memcpy(p, ret_obj->buffer.pointer, size);
>>>    	}
>>>    
>>>    	return 0;
>>> @@ -136,20 +183,28 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
>>>    /**
>>>     * lwmi_cd_alloc() - Allocate a cd_list struct in drvdata
>>>     * @priv: lenovo-wmi-capdata driver data.
>>> + * @type: The type of capability data.
>>>     *
>>>     * Allocate a cd_list struct large enough to contain data from all WMI data
>>>     * blocks provided by the interface.
>>>     *
>>>     * Return: 0 on success, or an error.
>>>     */
>>> -static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
>>> +static int lwmi_cd_alloc(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
>>>    {
>>>    	struct cd_list *list;
>>>    	size_t list_size;
>>>    	int count, ret;
>>>    
>>>    	count = wmidev_instance_count(priv->wdev);
>>> -	list_size = struct_size(list, data, count);
>>> +
>>> +	switch (type) {
>>> +	case LENOVO_CAPABILITY_DATA_01:
>>> +		list_size = struct_size(list, cd01, count);
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>>    
>>>    	list = devm_kzalloc(&priv->wdev->dev, list_size, GFP_KERNEL);
>>>    	if (!list)
>>> @@ -159,6 +214,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
>>>    	if (ret)
>>>    		return ret;
>>>    
>>> +	list->type = type;
>>>    	list->count = count;
>>>    	priv->list = list;
>>>    
>>> @@ -168,6 +224,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
>>>    /**
>>>     * lwmi_cd_setup() - Cache all WMI data block information
>>>     * @priv: lenovo-wmi-capdata driver data.
>>> + * @type: The type of capability data.
>>>     *
>>>     * Allocate a cd_list struct large enough to contain data from all WMI data
>>>     * blocks provided by the interface. Then loop through each data block and
>>> @@ -175,11 +232,11 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
>>>     *
>>>     * Return: 0 on success, or an error code.
>>>     */
>>> -static int lwmi_cd_setup(struct lwmi_cd_priv *priv)
>>> +static int lwmi_cd_setup(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
>>>    {
>>>    	int ret;
>>>    
>>> -	ret = lwmi_cd_alloc(priv);
>>> +	ret = lwmi_cd_alloc(priv, type);
>>>    	if (ret)
>>>    		return ret;
>>>    
>>> @@ -235,9 +292,13 @@ static void lwmi_cd01_unregister(void *data)
>>>    
>>>    static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
>>>    {
>>> +	const struct lwmi_cd_info *info = context;
>>>    	struct lwmi_cd_priv *priv;
>>>    	int ret;
>>>    
>>> +	if (!info)
>>> +		return -EINVAL;
>>> +
>>>    	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>>>    	if (!priv)
>>>    		return -ENOMEM;
>>> @@ -245,21 +306,34 @@ static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
>>>    	priv->wdev = wdev;
>>>    	dev_set_drvdata(&wdev->dev, priv);
>>>    
>>> -	ret = lwmi_cd_setup(priv);
>>> +	ret = lwmi_cd_setup(priv, info->type);
>>>    	if (ret)
>>> -		return ret;
>>> +		goto out;
>>>    
>>> -	priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
>>> +	if (info->type == LENOVO_CAPABILITY_DATA_01) {
>>> +		priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
>>>    
>>> -	ret = register_acpi_notifier(&priv->acpi_nb);
>>> -	if (ret)
>>> -		return ret;
>>> +		ret = register_acpi_notifier(&priv->acpi_nb);
>>> +		if (ret)
>>> +			goto out;
>>>    
>>> -	ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister, &priv->acpi_nb);
>>> -	if (ret)
>>> -		return ret;
>>> +		ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister,
>>> +					       &priv->acpi_nb);
>>> +		if (ret)
>>> +			goto out;
>>> +	}
>>> +
>>> +	ret = component_add(&wdev->dev, &lwmi_cd_component_ops);
>>>    
>>> -	return component_add(&wdev->dev, &lwmi_cd_component_ops);
>>> +out:
>>> +	if (ret) {
>>> +		dev_err(&wdev->dev, "failed to register %s: %d\n",
>>> +			info->name, ret);
>>> +	} else {
>>> +		dev_info(&wdev->dev, "registered %s with %u items\n",
>>> +			 info->name, priv->list->count);
>>> +	}
>>> +	return ret;
>>>    }
>>>    
>>>    static void lwmi_cd_remove(struct wmi_device *wdev)
>>> @@ -267,8 +341,12 @@ static void lwmi_cd_remove(struct wmi_device *wdev)
>>>    	component_del(&wdev->dev, &lwmi_cd_component_ops);
>>>    }
>>>    
>>> +#define LWMI_CD_WDEV_ID(_type)				\
>>> +	.guid_string = _type##_GUID,			\
>>> +	.context = &lwmi_cd_table[_type]
>>> +
>>>    static const struct wmi_device_id lwmi_cd_id_table[] = {
>>> -	{ LENOVO_CAPABILITY_DATA_01_GUID, NULL },
>>> +	{ LWMI_CD_WDEV_ID(LENOVO_CAPABILITY_DATA_01) },
>>>    	{}
>>>    };
>>>    
>>> @@ -284,21 +362,61 @@ static struct wmi_driver lwmi_cd_driver = {
>>>    };
>>>    
>>>    /**
>>> - * lwmi_cd01_match() - Match rule for the master driver.
>>> - * @dev: Pointer to the capability data 01 parent device.
>>> - * @data: Unused void pointer for passing match criteria.
>>> + * lwmi_cd_match() - Match rule for the master driver.
>>> + * @dev: Pointer to the capability data parent device.
>>> + * @type: Pointer to capability data type (enum lwmi_cd_type *) to match.
>>>     *
>>>     * Return: int.
>>>     */
>>> -int lwmi_cd01_match(struct device *dev, void *data)
>>> +static int lwmi_cd_match(struct device *dev, void *type)
>>> +{
>>> +	struct lwmi_cd_priv *priv;
>>> +
>>> +	if (dev->driver != &lwmi_cd_driver.driver)
>>> +		return false;
>>> +
>>> +	priv = dev_get_drvdata(dev);
>>> +	return priv->list->type == *(enum lwmi_cd_type *)type;
>>> +}
>>> +
>>> +/**
>>> + * lwmi_cd_match_add_all() - Add all match rule for the master driver.
>>> + * @master: Pointer to the master device.
>>> + * @matchptr: Pointer to the returned component_match pointer.
>>> + *
>>> + * Adds all component matches to the list stored in @matchptr for the @master
>>> + * device. @matchptr must be initialized to NULL. This matches all available
>>> + * capdata types on the machine.
>>> + */
>>> +void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr)
>>>    {
>>> -	return dev->driver == &lwmi_cd_driver.driver;
>>> +	int i;
>>> +
>>> +	if (WARN_ON(*matchptr))
>>> +		return;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(lwmi_cd_table); i++) {
>>> +		if (!lwmi_cd_table[i].guid_string ||
>>> +		    !wmi_has_guid(lwmi_cd_table[i].guid_string))
>>> +			continue;
>> I am still not happy about this. AFAIK as soon as the ordinary capdata WMI devices are bound together,
>> the firmware tells you whether or not the additional fan data WMI device is available. Maybe you can do
>> something like this:
>>
>> 1. Bind both capdata WMI devices as usual.
>> 2. Check if a fan data WMI device is available (you can use a DMI-based quirk list for devices were
>>      the firmware reports invalid data).
>> 3. Register an additional component that binds to the fan data WMI device.
>> 4. Bind both the additional component and the component registered by the fan data WMI device.
>> 5. Register the hwmon device with additional fan data.
>>
>> If the fan data WMI device is not available, you can simply skip steps 3 and 4 and simply
>> register the hwmon device with any additional fan data.
>>
>> What do you think?
> I tried your approach. I looks pretty well except for step 4:
>
>     debugfs: 'DC2A8805-3A8C-41BA-A6F7-092E0089CD3B-21' already exists in 'device_component'
>
> Moreover, component_[un]bind_all() calls __aggregate_find() with ops ==
> NULL, which implies that it can't really distinguish the two aggregate
> devices we have. Thus, we are rely on the insertion sequence of
> aggregate_devices (see component_master_add_with_match()) to make it
> just work. Pseudo code:
>
>     lwmi_other_probe()
>     {
>     	component_match_add(..., lwmi_cd00_match, ...);
>     	component_match_add(..., lwmi_cd01_match, ...);
>     	component_master_add_with_match(..., &ops1, ...);
>     	component_match_add(..., lwmi_cd_fan_match, ...);
>     	component_master_add_with_match(..., &ops2, ...);
>     }
>     
>     lwmi_other_remove()
>     {
>     	/* This just works (TM). */
>     	component_master_del(..., &ops2); /* unbinds cd_fan */
>     	component_master_del(..., &ops1); /* unbinds cd00/01 */
>     
>     	/* WARNING: at drivers/base/component.c:589 */
>     	/*
>     	component_master_del(..., &ops1); /* unbinds cd_fan!!! */
>     	component_master_del(..., &ops2); /* unbinds cd_fan!!! */
>     	*/
>     }
>
> It seems that the component framework is not prepared to allow multi-
> aggregation master device.
>
> Since we are talking about the component framework: all efforts we made
> here is to work around one of its limitations -- all components must be
> found or else it won't bring up the aggregate device. Do you think
> allowing partially bound aggregate device a good idea? E.g., adding a
> flag, indicating the master device opts in to such behavior, to the
> definition of struct component_master_ops. I can prepare a separate
> patch for that.
>
> CC'ing maintainers and lists of driver core and component framework.

Oh my, i did not know about this limitation. I think adding support for
optional components will be quite complicated to get right, inside i propose
the following:

The current component master is lenovo-wmi-other, with capdata01 and capdata00
being its components (correct me if i am wrong). Instead of registering an additional
component master on lenovo-wmi-other, how about registering it on capdata00?

Basically when probing, capdata00 will register the component for lenovo-wmi-other
and then check whether attribute 0x04050000 indicates support for LENOVO_FAN_TEST_DATA
(or a DMI check overrides this). Is yes then capdata00 registers an additional component
master, otherwise the hwmon device is created right away.
The driver for LENOVO_FAN_TEST_DATA registers a component for capdata00. As soon as the
component master from capdata00 is bound to the component from LENOVO_FAN_TEST_DATA, a
hwmon device is created.

Do you thing this would be more feasible than extending the component framework itself?

Thanks,
Armin Wolf

>> Thanks,
>> Armin Wolf
> Thanks,
> Rong
>
>>> +
>>> +		component_match_add(master, matchptr, lwmi_cd_match,
>>> +				    (void *)&lwmi_cd_table[i].type);
>>> +		if (IS_ERR(matchptr))
>>> +			return;
>>> +	}
>>> +
>>> +	if (!*matchptr) {
>>> +		pr_warn("a master driver requested capability data, but nothing is available\n");
>>> +		*matchptr = ERR_PTR(-ENODEV);
>>> +	}
>>>    }
>>> -EXPORT_SYMBOL_NS_GPL(lwmi_cd01_match, "LENOVO_WMI_CD");
>>> +EXPORT_SYMBOL_NS_GPL(lwmi_cd_match_add_all, "LENOVO_WMI_CD");
>>>    
>>>    module_wmi_driver(lwmi_cd_driver);
>>>    
>>>    MODULE_DEVICE_TABLE(wmi, lwmi_cd_id_table);
>>>    MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
>>> +MODULE_AUTHOR("Rong Zhang <i@rong.moe>");
>>>    MODULE_DESCRIPTION("Lenovo Capability Data WMI Driver");
>>>    MODULE_LICENSE("GPL");
>>> diff --git a/drivers/platform/x86/lenovo/wmi-capdata.h b/drivers/platform/x86/lenovo/wmi-capdata.h
>>> index 2a4746e38ad4..1e5fce7836cb 100644
>>> --- a/drivers/platform/x86/lenovo/wmi-capdata.h
>>> +++ b/drivers/platform/x86/lenovo/wmi-capdata.h
>>> @@ -7,6 +7,7 @@
>>>    
>>>    #include <linux/types.h>
>>>    
>>> +struct component_match;
>>>    struct device;
>>>    struct cd_list;
>>>    
>>> @@ -19,7 +20,11 @@ struct capdata01 {
>>>    	u32 max_value;
>>>    };
>>>    
>>> +struct lwmi_cd_binder {
>>> +	struct cd_list *cd01_list;
>>> +};
>>> +
>>>    int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output);
>>> -int lwmi_cd01_match(struct device *dev, void *data);
>>> +void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr);
>>>    
>>>    #endif /* !_LENOVO_WMI_CAPDATA_H_ */
>>> diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
>>> index c6dc1b4cff84..20c6ff0be37a 100644
>>> --- a/drivers/platform/x86/lenovo/wmi-other.c
>>> +++ b/drivers/platform/x86/lenovo/wmi-other.c
>>> @@ -579,14 +579,14 @@ static void lwmi_om_fw_attr_remove(struct lwmi_om_priv *priv)
>>>    static int lwmi_om_master_bind(struct device *dev)
>>>    {
>>>    	struct lwmi_om_priv *priv = dev_get_drvdata(dev);
>>> -	struct cd_list *tmp_list;
>>> +	struct lwmi_cd_binder binder = { 0 };
>>>    	int ret;
>>>    
>>> -	ret = component_bind_all(dev, &tmp_list);
>>> +	ret = component_bind_all(dev, &binder);
>>>    	if (ret)
>>>    		return ret;
>>>    
>>> -	priv->cd01_list = tmp_list;
>>> +	priv->cd01_list = binder.cd01_list;
>>>    	if (!priv->cd01_list)
>>>    		return -ENODEV;
>>>    
>>> @@ -618,6 +618,7 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
>>>    {
>>>    	struct component_match *master_match = NULL;
>>>    	struct lwmi_om_priv *priv;
>>> +	int ret;
>>>    
>>>    	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
>>>    	if (!priv)
>>> @@ -626,12 +627,26 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
>>>    	priv->wdev = wdev;
>>>    	dev_set_drvdata(&wdev->dev, priv);
>>>    
>>> -	component_match_add(&wdev->dev, &master_match, lwmi_cd01_match, NULL);
>>> +	lwmi_cd_match_add_all(&wdev->dev, &master_match);
>>>    	if (IS_ERR(master_match))
>>>    		return PTR_ERR(master_match);
>>>    
>>> -	return component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
>>> -					       master_match);
>>> +	ret = component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
>>> +					      master_match);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (likely(component_master_is_bound(&wdev->dev, &lwmi_om_master_ops)))
>>> +		return 0;
>>> +
>>> +	/*
>>> +	 * The bind callbacks of both master and components were never called in
>>> +	 * this case - this driver won't work at all. Failing...
>>> +	 */
>>> +	dev_err(&wdev->dev, "unbound master; is any component failing to be probed?");
>>> +
>>> +	component_master_del(&wdev->dev, &lwmi_om_master_ops);
>>> +	return -EXDEV;
>>>    }
>>>    
>>>    static void lwmi_other_remove(struct wmi_device *wdev)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 3/6] platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability Data
  2025-11-10  0:02       ` Armin Wolf
@ 2025-11-10 17:40         ` Rong Zhang
  0 siblings, 0 replies; 14+ messages in thread
From: Rong Zhang @ 2025-11-10 17:40 UTC (permalink / raw)
  To: Armin Wolf, Mark Pearson, Derek J. Clark, Hans de Goede,
	Ilpo Järvinen
  Cc: Greg Kroah-Hartman, Rafael J. Wysocki, Danilo Krummrich,
	dri-devel, Guenter Roeck, platform-driver-x86, linux-kernel,
	linux-hwmon

Hi Armin,

On Mon, 2025-11-10 at 01:02 +0100, Armin Wolf wrote:
> Am 06.11.25 um 13:36 schrieb Rong Zhang:
> 
> > Hi Armin,
> > 
> > On Tue, 2025-11-04 at 21:20 +0100, Armin Wolf wrote:
> > > Am 31.10.25 um 16:51 schrieb Rong Zhang:
> > > 
> > > > The current implementation are heavily bound to capdata01. Rewrite it so
> > > > that it is suitable to utilize other Capability Data as well.
> > > > 
> > > > No functional change intended.
> > > > 
> > > > Signed-off-by: Rong Zhang <i@rong.moe>
> > > > Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > > > Tested-by: Derek J. Clark <derekjohn.clark@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > > - Fix function parameter 'type' not described in 'lwmi_cd_match' (thanks
> > > >     kernel test bot)
> > > > ---
> > > >    drivers/platform/x86/lenovo/wmi-capdata.c | 208 +++++++++++++++++-----
> > > >    drivers/platform/x86/lenovo/wmi-capdata.h |   7 +-
> > > >    drivers/platform/x86/lenovo/wmi-other.c   |  27 ++-
> > > >    3 files changed, 190 insertions(+), 52 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/lenovo/wmi-capdata.c b/drivers/platform/x86/lenovo/wmi-capdata.c
> > > > index c5e74b2bfeb3..1f7fc09b7c3f 100644
> > > > --- a/drivers/platform/x86/lenovo/wmi-capdata.c
> > > > +++ b/drivers/platform/x86/lenovo/wmi-capdata.c
> > > > @@ -12,8 +12,13 @@
> > > >     *
> > > >     * Copyright (C) 2025 Derek J. Clark <derekjohn.clark@gmail.com>
> > > >     *   - Initial implementation (formerly named lenovo-wmi-capdata01)
> > > > + *
> > > > + * Copyright (C) 2025 Rong Zhang <i@rong.moe>
> > > > + *   - Unified implementation
> > > >     */
> > > >    
> > > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > +
> > > >    #include <linux/acpi.h>
> > > >    #include <linux/cleanup.h>
> > > >    #include <linux/component.h>
> > > > @@ -36,6 +41,25 @@
> > > >    #define ACPI_AC_CLASS "ac_adapter"
> > > >    #define ACPI_AC_NOTIFY_STATUS 0x80
> > > >    
> > > > +enum lwmi_cd_type {
> > > > +	LENOVO_CAPABILITY_DATA_01,
> > > > +};
> > > > +
> > > > +#define LWMI_CD_TABLE_ITEM(_type)		\
> > > > +	[_type] = {				\
> > > > +		.guid_string = _type##_GUID,	\
> > > > +		.name = #_type,			\
> > > > +		.type = _type,			\
> > > > +	}
> > > > +
> > > > +static const struct lwmi_cd_info {
> > > > +	const char *guid_string;
> > > > +	const char *name;
> > > > +	enum lwmi_cd_type type;
> > > > +} lwmi_cd_table[] = {
> > > > +	LWMI_CD_TABLE_ITEM(LENOVO_CAPABILITY_DATA_01),
> > > > +};
> > > > +
> > > >    struct lwmi_cd_priv {
> > > >    	struct notifier_block acpi_nb; /* ACPI events */
> > > >    	struct wmi_device *wdev;
> > > > @@ -44,15 +68,19 @@ struct lwmi_cd_priv {
> > > >    
> > > >    struct cd_list {
> > > >    	struct mutex list_mutex; /* list R/W mutex */
> > > > +	enum lwmi_cd_type type;
> > > >    	u8 count;
> > > > -	struct capdata01 data[];
> > > > +
> > > > +	union {
> > > > +		DECLARE_FLEX_ARRAY(struct capdata01, cd01);
> > > > +	};
> > > >    };
> > > >    
> > > >    /**
> > > >     * lwmi_cd_component_bind() - Bind component to master device.
> > > >     * @cd_dev: Pointer to the lenovo-wmi-capdata driver parent device.
> > > >     * @om_dev: Pointer to the lenovo-wmi-other driver parent device.
> > > > - * @data: cd_list object pointer used to return the capability data.
> > > > + * @data: lwmi_cd_binder object pointer used to return the capability data.
> > > >     *
> > > >     * On lenovo-wmi-other's master bind, provide a pointer to the local capdata
> > > >     * list. This is used to call lwmi_cd*_get_data to look up attribute data
> > > > @@ -64,9 +92,15 @@ static int lwmi_cd_component_bind(struct device *cd_dev,
> > > >    				  struct device *om_dev, void *data)
> > > >    {
> > > >    	struct lwmi_cd_priv *priv = dev_get_drvdata(cd_dev);
> > > > -	struct cd_list **cd_list = data;
> > > > +	struct lwmi_cd_binder *binder = data;
> > > >    
> > > > -	*cd_list = priv->list;
> > > > +	switch (priv->list->type) {
> > > > +	case LENOVO_CAPABILITY_DATA_01:
> > > > +		binder->cd01_list = priv->list;
> > > > +		break;
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > >    
> > > >    	return 0;
> > > >    }
> > > > @@ -76,30 +110,33 @@ static const struct component_ops lwmi_cd_component_ops = {
> > > >    };
> > > >    
> > > >    /**
> > > > - * lwmi_cd01_get_data - Get the data of the specified attribute
> > > > + * lwmi_cd*_get_data - Get the data of the specified attribute
> > > >     * @list: The lenovo-wmi-capdata pointer to its cd_list struct.
> > > >     * @attribute_id: The capdata attribute ID to be found.
> > > > - * @output: Pointer to a capdata01 struct to return the data.
> > > > + * @output: Pointer to a capdata* struct to return the data.
> > > >     *
> > > > - * Retrieves the capability data 01 struct pointer for the given
> > > > - * attribute for its specified thermal mode.
> > > > + * Retrieves the capability data struct pointer for the given
> > > > + * attribute.
> > > >     *
> > > >     * Return: 0 on success, or -EINVAL.
> > > >     */
> > > > -int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output)
> > > > -{
> > > > -	u8 idx;
> > > > -
> > > > -	guard(mutex)(&list->list_mutex);
> > > > -	for (idx = 0; idx < list->count; idx++) {
> > > > -		if (list->data[idx].id != attribute_id)
> > > > -			continue;
> > > > -		memcpy(output, &list->data[idx], sizeof(list->data[idx]));
> > > > -		return 0;
> > > > +#define DEF_LWMI_CDXX_GET_DATA(_cdxx, _cd_type, _output_t)					\
> > > > +	int lwmi_##_cdxx##_get_data(struct cd_list *list, u32 attribute_id, _output_t *output)	\
> > > > +	{											\
> > > > +		u8 idx;										\
> > > > +		if (WARN_ON(list->type != _cd_type))						\
> > > > +			return -EINVAL;								\
> > > > +		guard(mutex)(&list->list_mutex);						\
> > > > +		for (idx = 0; idx < list->count; idx++) {					\
> > > > +			if (list->_cdxx[idx].id != attribute_id)				\
> > > > +				continue;							\
> > > > +			memcpy(output, &list->_cdxx[idx], sizeof(list->_cdxx[idx]));		\
> > > > +			return 0;								\
> > > > +		}										\
> > > > +		return -EINVAL;									\
> > > >    	}
> > > >    
> > > > -	return -EINVAL;
> > > > -}
> > > > +DEF_LWMI_CDXX_GET_DATA(cd01, LENOVO_CAPABILITY_DATA_01, struct capdata01);
> > > >    EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
> > > >    
> > > >    /**
> > > > @@ -112,10 +149,21 @@ EXPORT_SYMBOL_NS_GPL(lwmi_cd01_get_data, "LENOVO_WMI_CD");
> > > >     */
> > > >    static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> > > >    {
> > > > +	size_t size;
> > > >    	int idx;
> > > > +	void *p;
> > > > +
> > > > +	switch (priv->list->type) {
> > > > +	case LENOVO_CAPABILITY_DATA_01:
> > > > +		p = &priv->list->cd01[0];
> > > > +		size = sizeof(priv->list->cd01[0]);
> > > > +		break;
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > >    
> > > >    	guard(mutex)(&priv->list->list_mutex);
> > > > -	for (idx = 0; idx < priv->list->count; idx++) {
> > > > +	for (idx = 0; idx < priv->list->count; idx++, p += size) {
> > > >    		union acpi_object *ret_obj __free(kfree) = NULL;
> > > >    
> > > >    		ret_obj = wmidev_block_query(priv->wdev, idx);
> > > > @@ -123,11 +171,10 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> > > >    			return -ENODEV;
> > > >    
> > > >    		if (ret_obj->type != ACPI_TYPE_BUFFER ||
> > > > -		    ret_obj->buffer.length < sizeof(priv->list->data[idx]))
> > > > +		    ret_obj->buffer.length < size)
> > > >    			continue;
> > > >    
> > > > -		memcpy(&priv->list->data[idx], ret_obj->buffer.pointer,
> > > > -		       ret_obj->buffer.length);
> > > > +		memcpy(p, ret_obj->buffer.pointer, size);
> > > >    	}
> > > >    
> > > >    	return 0;
> > > > @@ -136,20 +183,28 @@ static int lwmi_cd_cache(struct lwmi_cd_priv *priv)
> > > >    /**
> > > >     * lwmi_cd_alloc() - Allocate a cd_list struct in drvdata
> > > >     * @priv: lenovo-wmi-capdata driver data.
> > > > + * @type: The type of capability data.
> > > >     *
> > > >     * Allocate a cd_list struct large enough to contain data from all WMI data
> > > >     * blocks provided by the interface.
> > > >     *
> > > >     * Return: 0 on success, or an error.
> > > >     */
> > > > -static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> > > > +static int lwmi_cd_alloc(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
> > > >    {
> > > >    	struct cd_list *list;
> > > >    	size_t list_size;
> > > >    	int count, ret;
> > > >    
> > > >    	count = wmidev_instance_count(priv->wdev);
> > > > -	list_size = struct_size(list, data, count);
> > > > +
> > > > +	switch (type) {
> > > > +	case LENOVO_CAPABILITY_DATA_01:
> > > > +		list_size = struct_size(list, cd01, count);
> > > > +		break;
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > >    
> > > >    	list = devm_kzalloc(&priv->wdev->dev, list_size, GFP_KERNEL);
> > > >    	if (!list)
> > > > @@ -159,6 +214,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> > > >    	if (ret)
> > > >    		return ret;
> > > >    
> > > > +	list->type = type;
> > > >    	list->count = count;
> > > >    	priv->list = list;
> > > >    
> > > > @@ -168,6 +224,7 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> > > >    /**
> > > >     * lwmi_cd_setup() - Cache all WMI data block information
> > > >     * @priv: lenovo-wmi-capdata driver data.
> > > > + * @type: The type of capability data.
> > > >     *
> > > >     * Allocate a cd_list struct large enough to contain data from all WMI data
> > > >     * blocks provided by the interface. Then loop through each data block and
> > > > @@ -175,11 +232,11 @@ static int lwmi_cd_alloc(struct lwmi_cd_priv *priv)
> > > >     *
> > > >     * Return: 0 on success, or an error code.
> > > >     */
> > > > -static int lwmi_cd_setup(struct lwmi_cd_priv *priv)
> > > > +static int lwmi_cd_setup(struct lwmi_cd_priv *priv, enum lwmi_cd_type type)
> > > >    {
> > > >    	int ret;
> > > >    
> > > > -	ret = lwmi_cd_alloc(priv);
> > > > +	ret = lwmi_cd_alloc(priv, type);
> > > >    	if (ret)
> > > >    		return ret;
> > > >    
> > > > @@ -235,9 +292,13 @@ static void lwmi_cd01_unregister(void *data)
> > > >    
> > > >    static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
> > > >    {
> > > > +	const struct lwmi_cd_info *info = context;
> > > >    	struct lwmi_cd_priv *priv;
> > > >    	int ret;
> > > >    
> > > > +	if (!info)
> > > > +		return -EINVAL;
> > > > +
> > > >    	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> > > >    	if (!priv)
> > > >    		return -ENOMEM;
> > > > @@ -245,21 +306,34 @@ static int lwmi_cd_probe(struct wmi_device *wdev, const void *context)
> > > >    	priv->wdev = wdev;
> > > >    	dev_set_drvdata(&wdev->dev, priv);
> > > >    
> > > > -	ret = lwmi_cd_setup(priv);
> > > > +	ret = lwmi_cd_setup(priv, info->type);
> > > >    	if (ret)
> > > > -		return ret;
> > > > +		goto out;
> > > >    
> > > > -	priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
> > > > +	if (info->type == LENOVO_CAPABILITY_DATA_01) {
> > > > +		priv->acpi_nb.notifier_call = lwmi_cd01_notifier_call;
> > > >    
> > > > -	ret = register_acpi_notifier(&priv->acpi_nb);
> > > > -	if (ret)
> > > > -		return ret;
> > > > +		ret = register_acpi_notifier(&priv->acpi_nb);
> > > > +		if (ret)
> > > > +			goto out;
> > > >    
> > > > -	ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister, &priv->acpi_nb);
> > > > -	if (ret)
> > > > -		return ret;
> > > > +		ret = devm_add_action_or_reset(&wdev->dev, lwmi_cd01_unregister,
> > > > +					       &priv->acpi_nb);
> > > > +		if (ret)
> > > > +			goto out;
> > > > +	}
> > > > +
> > > > +	ret = component_add(&wdev->dev, &lwmi_cd_component_ops);
> > > >    
> > > > -	return component_add(&wdev->dev, &lwmi_cd_component_ops);
> > > > +out:
> > > > +	if (ret) {
> > > > +		dev_err(&wdev->dev, "failed to register %s: %d\n",
> > > > +			info->name, ret);
> > > > +	} else {
> > > > +		dev_info(&wdev->dev, "registered %s with %u items\n",
> > > > +			 info->name, priv->list->count);
> > > > +	}
> > > > +	return ret;
> > > >    }
> > > >    
> > > >    static void lwmi_cd_remove(struct wmi_device *wdev)
> > > > @@ -267,8 +341,12 @@ static void lwmi_cd_remove(struct wmi_device *wdev)
> > > >    	component_del(&wdev->dev, &lwmi_cd_component_ops);
> > > >    }
> > > >    
> > > > +#define LWMI_CD_WDEV_ID(_type)				\
> > > > +	.guid_string = _type##_GUID,			\
> > > > +	.context = &lwmi_cd_table[_type]
> > > > +
> > > >    static const struct wmi_device_id lwmi_cd_id_table[] = {
> > > > -	{ LENOVO_CAPABILITY_DATA_01_GUID, NULL },
> > > > +	{ LWMI_CD_WDEV_ID(LENOVO_CAPABILITY_DATA_01) },
> > > >    	{}
> > > >    };
> > > >    
> > > > @@ -284,21 +362,61 @@ static struct wmi_driver lwmi_cd_driver = {
> > > >    };
> > > >    
> > > >    /**
> > > > - * lwmi_cd01_match() - Match rule for the master driver.
> > > > - * @dev: Pointer to the capability data 01 parent device.
> > > > - * @data: Unused void pointer for passing match criteria.
> > > > + * lwmi_cd_match() - Match rule for the master driver.
> > > > + * @dev: Pointer to the capability data parent device.
> > > > + * @type: Pointer to capability data type (enum lwmi_cd_type *) to match.
> > > >     *
> > > >     * Return: int.
> > > >     */
> > > > -int lwmi_cd01_match(struct device *dev, void *data)
> > > > +static int lwmi_cd_match(struct device *dev, void *type)
> > > > +{
> > > > +	struct lwmi_cd_priv *priv;
> > > > +
> > > > +	if (dev->driver != &lwmi_cd_driver.driver)
> > > > +		return false;
> > > > +
> > > > +	priv = dev_get_drvdata(dev);
> > > > +	return priv->list->type == *(enum lwmi_cd_type *)type;
> > > > +}
> > > > +
> > > > +/**
> > > > + * lwmi_cd_match_add_all() - Add all match rule for the master driver.
> > > > + * @master: Pointer to the master device.
> > > > + * @matchptr: Pointer to the returned component_match pointer.
> > > > + *
> > > > + * Adds all component matches to the list stored in @matchptr for the @master
> > > > + * device. @matchptr must be initialized to NULL. This matches all available
> > > > + * capdata types on the machine.
> > > > + */
> > > > +void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr)
> > > >    {
> > > > -	return dev->driver == &lwmi_cd_driver.driver;
> > > > +	int i;
> > > > +
> > > > +	if (WARN_ON(*matchptr))
> > > > +		return;
> > > > +
> > > > +	for (i = 0; i < ARRAY_SIZE(lwmi_cd_table); i++) {
> > > > +		if (!lwmi_cd_table[i].guid_string ||
> > > > +		    !wmi_has_guid(lwmi_cd_table[i].guid_string))
> > > > +			continue;
> > > I am still not happy about this. AFAIK as soon as the ordinary capdata WMI devices are bound together,
> > > the firmware tells you whether or not the additional fan data WMI device is available. Maybe you can do
> > > something like this:
> > > 
> > > 1. Bind both capdata WMI devices as usual.
> > > 2. Check if a fan data WMI device is available (you can use a DMI-based quirk list for devices were
> > >      the firmware reports invalid data).
> > > 3. Register an additional component that binds to the fan data WMI device.
> > > 4. Bind both the additional component and the component registered by the fan data WMI device.
> > > 5. Register the hwmon device with additional fan data.
> > > 
> > > If the fan data WMI device is not available, you can simply skip steps 3 and 4 and simply
> > > register the hwmon device with any additional fan data.
> > > 
> > > What do you think?
> > I tried your approach. I looks pretty well except for step 4:
> > 
> >     debugfs: 'DC2A8805-3A8C-41BA-A6F7-092E0089CD3B-21' already exists in 'device_component'
> > 
> > Moreover, component_[un]bind_all() calls __aggregate_find() with ops ==
> > NULL, which implies that it can't really distinguish the two aggregate
> > devices we have. Thus, we are rely on the insertion sequence of
> > aggregate_devices (see component_master_add_with_match()) to make it
> > just work. Pseudo code:
> > 
> >     lwmi_other_probe()
> >     {
> >     	component_match_add(..., lwmi_cd00_match, ...);
> >     	component_match_add(..., lwmi_cd01_match, ...);
> >     	component_master_add_with_match(..., &ops1, ...);
> >     	component_match_add(..., lwmi_cd_fan_match, ...);
> >     	component_master_add_with_match(..., &ops2, ...);
> >     }
> >     
> >     lwmi_other_remove()
> >     {
> >     	/* This just works (TM). */
> >     	component_master_del(..., &ops2); /* unbinds cd_fan */
> >     	component_master_del(..., &ops1); /* unbinds cd00/01 */
> >     
> >     	/* WARNING: at drivers/base/component.c:589 */
> >     	/*
> >     	component_master_del(..., &ops1); /* unbinds cd_fan!!! */
> >     	component_master_del(..., &ops2); /* unbinds cd_fan!!! */
> >     	*/
> >     }
> > 
> > It seems that the component framework is not prepared to allow multi-
> > aggregation master device.
> > 
> > Since we are talking about the component framework: all efforts we made
> > here is to work around one of its limitations -- all components must be
> > found or else it won't bring up the aggregate device. Do you think
> > allowing partially bound aggregate device a good idea? E.g., adding a
> > flag, indicating the master device opts in to such behavior, to the
> > definition of struct component_master_ops. I can prepare a separate
> > patch for that.
> > 
> > CC'ing maintainers and lists of driver core and component framework.
> 
> Oh my, i did not know about this limitation. I think adding support for
> optional components will be quite complicated to get right,

Ahh, yes. This seemed too complicated once I thought it over, as one of
the most important purposes of the component framework is to eliminate
any assumption of the probe sequence of master and its components --
optional components bring the assumption back.

> inside i propose
> the following:
> 
> The current component master is lenovo-wmi-other, with capdata01 and capdata00
> being its components (correct me if i am wrong). Instead of registering an additional
> component master on lenovo-wmi-other, how about registering it on capdata00?
> 
> Basically when probing, capdata00 will register the component for lenovo-wmi-other
> and then check whether attribute 0x04050000 indicates support for LENOVO_FAN_TEST_DATA
> (or a DMI check overrides this). Is yes then capdata00 registers an additional component
> master, otherwise the hwmon device is created right away.
> The driver for LENOVO_FAN_TEST_DATA registers a component for capdata00. As soon as the
> component master from capdata00 is bound to the component from LENOVO_FAN_TEST_DATA, a
> hwmon device is created.

"register the component for lenovo-wmi-other ... and *then* check ...
0x04050000 ... Is yes *then* capdata00 registers an additional
component master ..."

Did you mean capdata00 should check 0x04050000 *right after*
registering itself as a component of lenovo-wmi-other on probe?

Considering both lenovo-wmi-capdata and lenovo-wmi-other declare
probe_type = PROBE_PREFER_ASYNCHRONOUS, the timing of binding
establishment between lenovo-wmi-other and capdata{00,01} is
unpredictable, depending on which function call happens last:
1. component_master_add_with_match(other_dev, ...)
2. component_add(capdata00_dev, ...)
3. component_add(capdata01_dev, ...)

IIUC, there could be a race condition that it's too late to check
0x04050000 and register capdata00 as a master that the binding between
lenovo-wmi-other and capdata{00,01} has been established before. For
example, if 2 happens last, the binding must have been established
right before its return, making all operations afterward (check ... and
regsiter ...) meaningless. Even in other cases, the situation still
heavily depends on how fast the binding between capdata00 and
capdata_fan is established.

Or did you mean:

- lenovo-wmi-other on probe:
  - Registers itself as the master of capdata00/01
- capdata00/capdata01/capdata_fan on probe:
  - Register themselves as components

In the master bind callback of lenovo-wmi-other:
1. Calls component_bind_all()
2. In the component bind callback of capdata00:
   a. Checks 0x04050000 to see if LENOVO_FAN_TEST_DATA is supported
   b. If no, passes cd00_list to lenovo-wmi-other; return
   c. If yes, registers capdata00 as the master of capdata_fan
   d. "As soon as" the master is bound, passes cd00_list and
      cd_fan_list to lenovo-wmi-other
3. In the component bind callback of capdata01:
   a. Passes cd01_list to lenovo-wmi-other
4. Registers fw_attributes and HWMON device accordingly

This causes deadlock IIUC. This is because the component framework
calls 2 (the bind callback) with component_mutex held. Hence, 2.c
(registering capdata00 as a master) acquires component_mutex
recursively.

Or did I misunderstand your proposal or miss anything?

> Do you thing this would be more feasible than extending the component framework itself?

Thanks for you suggestion. I agree that binding capdata_fan to
capdata00 is the right direction to get rid of wmi_has_guid() in the
series.

I think the fundamental idea of your proposal is how to enforce the
following dependency chain while binding all these components/masters:

lenovo-wmi-other <- capdata00 <- capdata_fan
|- master           |- component
                    |- sub-master
                                 |- sub-component

While your proposal is in a top-down (left-to-right) manner, how about
a bottom-up (right-to-left) one? Illustration:

- lenovo-wmi-other on probe:
  - Registers itself as the master of capdata{00,01}
- capdata01/capdata_fan on probe:
  - Register themselves as components
- capdata00 on probe:
  - Checks 0x04050000 to see if LENOVO_FAN_TEST_DATA is supported
  - If no, registers itself as a component
  - If yes, registers itself as the master of capdata_fan

If yes, in the master bind callback of capdata00:
1. Calls component_bind_all()
2. In the component bind callback of capdata_fan:
   a. Passes cd_fan_list to capdata00
3. Schedules a work[1] to register capdata00 as a component

In such an approach, it is guaranteed that the binding between
capdata00 and capdata_fan must have been established before the binding
between lenovo-wmi-other and capdata{00,01} can start establishing,
which enforces the mentioned dependency chain.

Then, in the master bind callback of lenovo-wmi-other:
1. Calls component_bind_all()
2. In the component bind callback of capdata00:
   a. Passes cd00_list and cd_fan_list (if any) to lenovo-wmi-other
3. In the component bind callback of capdata01:
   a. Passes cd01_list to lenovo-wmi-other
4. Registers fw_attributes and HWMON device accordingly

Does my analysis and proposal make sense to you? If so, I will send out
a [PATCH v4] implementing this.

[1]: To break the component_mutex dilemma.

> Thanks,
> Armin Wolf

Thanks,
Rong

> > > Thanks,
> > > Armin Wolf
> > Thanks,
> > Rong
> > 
> > > > +
> > > > +		component_match_add(master, matchptr, lwmi_cd_match,
> > > > +				    (void *)&lwmi_cd_table[i].type);
> > > > +		if (IS_ERR(matchptr))
> > > > +			return;
> > > > +	}
> > > > +
> > > > +	if (!*matchptr) {
> > > > +		pr_warn("a master driver requested capability data, but nothing is available\n");
> > > > +		*matchptr = ERR_PTR(-ENODEV);
> > > > +	}
> > > >    }
> > > > -EXPORT_SYMBOL_NS_GPL(lwmi_cd01_match, "LENOVO_WMI_CD");
> > > > +EXPORT_SYMBOL_NS_GPL(lwmi_cd_match_add_all, "LENOVO_WMI_CD");
> > > >    
> > > >    module_wmi_driver(lwmi_cd_driver);
> > > >    
> > > >    MODULE_DEVICE_TABLE(wmi, lwmi_cd_id_table);
> > > >    MODULE_AUTHOR("Derek J. Clark <derekjohn.clark@gmail.com>");
> > > > +MODULE_AUTHOR("Rong Zhang <i@rong.moe>");
> > > >    MODULE_DESCRIPTION("Lenovo Capability Data WMI Driver");
> > > >    MODULE_LICENSE("GPL");
> > > > diff --git a/drivers/platform/x86/lenovo/wmi-capdata.h b/drivers/platform/x86/lenovo/wmi-capdata.h
> > > > index 2a4746e38ad4..1e5fce7836cb 100644
> > > > --- a/drivers/platform/x86/lenovo/wmi-capdata.h
> > > > +++ b/drivers/platform/x86/lenovo/wmi-capdata.h
> > > > @@ -7,6 +7,7 @@
> > > >    
> > > >    #include <linux/types.h>
> > > >    
> > > > +struct component_match;
> > > >    struct device;
> > > >    struct cd_list;
> > > >    
> > > > @@ -19,7 +20,11 @@ struct capdata01 {
> > > >    	u32 max_value;
> > > >    };
> > > >    
> > > > +struct lwmi_cd_binder {
> > > > +	struct cd_list *cd01_list;
> > > > +};
> > > > +
> > > >    int lwmi_cd01_get_data(struct cd_list *list, u32 attribute_id, struct capdata01 *output);
> > > > -int lwmi_cd01_match(struct device *dev, void *data);
> > > > +void lwmi_cd_match_add_all(struct device *master, struct component_match **matchptr);
> > > >    
> > > >    #endif /* !_LENOVO_WMI_CAPDATA_H_ */
> > > > diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c
> > > > index c6dc1b4cff84..20c6ff0be37a 100644
> > > > --- a/drivers/platform/x86/lenovo/wmi-other.c
> > > > +++ b/drivers/platform/x86/lenovo/wmi-other.c
> > > > @@ -579,14 +579,14 @@ static void lwmi_om_fw_attr_remove(struct lwmi_om_priv *priv)
> > > >    static int lwmi_om_master_bind(struct device *dev)
> > > >    {
> > > >    	struct lwmi_om_priv *priv = dev_get_drvdata(dev);
> > > > -	struct cd_list *tmp_list;
> > > > +	struct lwmi_cd_binder binder = { 0 };
> > > >    	int ret;
> > > >    
> > > > -	ret = component_bind_all(dev, &tmp_list);
> > > > +	ret = component_bind_all(dev, &binder);
> > > >    	if (ret)
> > > >    		return ret;
> > > >    
> > > > -	priv->cd01_list = tmp_list;
> > > > +	priv->cd01_list = binder.cd01_list;
> > > >    	if (!priv->cd01_list)
> > > >    		return -ENODEV;
> > > >    
> > > > @@ -618,6 +618,7 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
> > > >    {
> > > >    	struct component_match *master_match = NULL;
> > > >    	struct lwmi_om_priv *priv;
> > > > +	int ret;
> > > >    
> > > >    	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> > > >    	if (!priv)
> > > > @@ -626,12 +627,26 @@ static int lwmi_other_probe(struct wmi_device *wdev, const void *context)
> > > >    	priv->wdev = wdev;
> > > >    	dev_set_drvdata(&wdev->dev, priv);
> > > >    
> > > > -	component_match_add(&wdev->dev, &master_match, lwmi_cd01_match, NULL);
> > > > +	lwmi_cd_match_add_all(&wdev->dev, &master_match);
> > > >    	if (IS_ERR(master_match))
> > > >    		return PTR_ERR(master_match);
> > > >    
> > > > -	return component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
> > > > -					       master_match);
> > > > +	ret = component_master_add_with_match(&wdev->dev, &lwmi_om_master_ops,
> > > > +					      master_match);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (likely(component_master_is_bound(&wdev->dev, &lwmi_om_master_ops)))
> > > > +		return 0;
> > > > +
> > > > +	/*
> > > > +	 * The bind callbacks of both master and components were never called in
> > > > +	 * this case - this driver won't work at all. Failing...
> > > > +	 */
> > > > +	dev_err(&wdev->dev, "unbound master; is any component failing to be probed?");
> > > > +
> > > > +	component_master_del(&wdev->dev, &lwmi_om_master_ops);
> > > > +	return -EXDEV;
> > > >    }
> > > >    
> > > >    static void lwmi_other_remove(struct wmi_device *wdev)

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2025-11-10 17:45 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-31 15:51 [PATCH v3 0/6] platform/x86: lenovo-wmi-{capdata,other}: Add HWMON for fan speed Rong Zhang
2025-10-31 15:51 ` [PATCH v3 1/6] platform/x86: lenovo-wmi-helpers: Convert returned 4B buffer into u32 Rong Zhang
2025-11-04 20:13   ` Armin Wolf
2025-11-04 20:27     ` Rong Zhang
2025-10-31 15:51 ` [PATCH v3 2/6] platform/x86: Rename lenovo-wmi-capdata01 to lenovo-wmi-capdata Rong Zhang
2025-10-31 15:51 ` [PATCH v3 3/6] platform/x86: lenovo-wmi-{capdata,other}: Support multiple Capability Data Rong Zhang
2025-11-04 20:20   ` Armin Wolf
2025-11-04 20:43     ` Rong Zhang
2025-11-06 12:36     ` Rong Zhang
2025-11-10  0:02       ` Armin Wolf
2025-11-10 17:40         ` Rong Zhang
2025-10-31 15:51 ` [PATCH v3 4/6] platform/x86: lenovo-wmi-capdata: Add support for Capability Data 00 Rong Zhang
2025-10-31 15:51 ` [PATCH v3 5/6] platform/x86: lenovo-wmi-capdata: Add support for Fan Test Data Rong Zhang
2025-10-31 15:51 ` [PATCH v3 6/6] platform/x86: lenovo-wmi-other: Add HWMON for fan speed RPM Rong Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).