[PATCH v1 0/4] ACPI: thermal: Convert ACPI thermal driver to a platform one

[PATCH v1 0/4] ACPI: thermal: Convert ACPI thermal driver to a platform one

[Rafael J. Wysocki]

Hi All,

This is a continuation of

https://lore.kernel.org/linux-acpi/2339822.iZASKD2KPV@rafael.j.wysocki/

specifically concerning the ACPI thermal driver which is kind of a special
case because ACPI thermal zone objects it binds to are formally not device
objects.

Also the series contains one extra modification on top of the driver
conversion.

The general rationale is the same as for the series linked above.

While binding drivers directly to struct acpi_device objects allows
basic functionality to be provided, at least in the majority of cases,
there are some problems with it, related to general consistency, sysfs
layout, power management operation ordering, and code cleanliness.

First of all, struct acpi_device objects represent firmware entities
rather than hardware and in many cases they provide auxiliary information
on devices enumerated independently (like PCI devices or CPUs).  It is
therefore generally questionable to assign resources to them or create
class devices and similar under them because they don't provide
functionality associated with those entities by themselves (for example,
they don't generate wakeup or input events).

As a general rule, a struct acpi_device can only be a parent of another
struct acpi_device.  If that's not the case, the location of the child
device in the device hierarchy is at least confusing and it may not be
straightforward to identify the piece of hardware corresponding to that
device.

Using system suspend and resume callbacks directly with struct acpi_device
objects is questionable either because it may cause ordering problems to
happen.  Namely, struct acpi_device objects are registered before any
devices corresponded to by them and they land on the PM list before all
of those devices.  Consequently, the execution ordering of their PM
callbacks may be different from what is generally expected.  Moreover,
dependencies returned by _DEP objects don't generally affect struct
acpi_device objects themselves, only the "physical" device objects
associated with them, which potentially is one more source of inconsistency.

All of the above means that binding drivers to struct acpi_device "devices"
should generally be avoided and so this series converts three generic ACPI
device drivers, the button driver, the tiny power button driver, and the
battery driver, to platform drivers.

Patches [1-2/4] are preparatory, patch [3/4] is the driver conversion one,
and patch [4/4] updates thermal zone power management to stop attempting
to resume active cooling devices (fans).

Thanks!

[PATCH v1 1/4] ACPI: scan: Register platform devices for thermal zones

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Currently, platform devices are not registered for ACPI thermal zones
because they are not represented as device objects in the ACPI namespace.
Instead, they are represented as thermal zone objects, so in particular
the platform_id flag is not set for them during enumeration because it
is only set for objects of type ACPI_BUS_TYPE_DEVICE, but otherwise they
are handled similarly at the ACPI core level.

To facilitate converting the ACPI thermal zone driver into a platform
one, modify acpi_set_pnp_ids() to set the platform_id flag for thermal
zones in analogy with device objects to cause platform devices to be
registered for them.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |    1 +
 1 file changed, 1 insertion(+)

--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1468,6 +1468,7 @@ static void acpi_set_pnp_ids(acpi_handle
 		break;
 	case ACPI_BUS_TYPE_THERMAL:
 		acpi_add_id(pnp, ACPI_THERMAL_HID);
+		pnp->type.platform_id = 1;
 		break;
 	case ACPI_BUS_TYPE_POWER_BUTTON:
 		acpi_add_id(pnp, ACPI_BUTTON_HID_POWERF);

[PATCH v1 2/4] ACPI: thermal: Adjust event notification routine

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Adjust acpi_thermal_notify() to cast its "data" argument to a struct
acpi_thermal pointer istead of a struct acpi_device one, which allows
the use of acpi_driver_data() to be limited and will facilitate
subsequent changes.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/thermal.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -670,8 +670,7 @@ static void acpi_thermal_unregister_ther
 
 static void acpi_thermal_notify(acpi_handle handle, u32 event, void *data)
 {
-	struct acpi_device *device = data;
-	struct acpi_thermal *tz = acpi_driver_data(device);
+	struct acpi_thermal *tz = data;
 
 	if (!tz)
 		return;
@@ -685,8 +684,8 @@ static void acpi_thermal_notify(acpi_han
 		acpi_thermal_trips_update(tz, event);
 		break;
 	default:
-		acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
-				  event);
+		acpi_handle_debug(tz->device->handle,
+				  "Unsupported event [0x%x]\n", event);
 		break;
 	}
 }
@@ -881,7 +880,7 @@ static int acpi_thermal_add(struct acpi_
 		acpi_device_bid(device), deci_kelvin_to_celsius(tz->temp_dk));
 
 	result = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY,
-						 acpi_thermal_notify, device);
+						 acpi_thermal_notify, tz);
 	if (result)
 		goto flush_wq;
 

[PATCH v1 3/4] ACPI: thermal: Convert the driver to a platform one

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

While binding drivers directly to struct acpi_device objects allows
basic functionality to be provided, at least in the majority of cases,
there are some problems with it, related to general consistency, sysfs
layout, power management operation ordering, and code cleanliness.

Overall, it is better to bind drivers to platform devices than to their
ACPI companions, so convert the ACPI thermal zone driver to a platform
one.

While this is not expected to alter functionality, it changes sysfs
layout and so it will be visible to user space.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/thermal.c |   48 +++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -25,6 +25,7 @@
 #include <linux/kmod.h>
 #include <linux/reboot.h>
 #include <linux/device.h>
+#include <linux/platform_device.h>
 #include <linux/thermal.h>
 #include <linux/acpi.h>
 #include <linux/workqueue.h>
@@ -776,9 +777,10 @@ static void acpi_thermal_free_thermal_zo
 	kfree(tz);
 }
 
-static int acpi_thermal_add(struct acpi_device *device)
+static int acpi_thermal_probe(struct platform_device *pdev)
 {
 	struct thermal_trip trip_table[ACPI_THERMAL_MAX_NR_TRIPS] = { 0 };
+	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 	struct acpi_thermal_trip *acpi_trip;
 	struct thermal_trip *trip;
 	struct acpi_thermal *tz;
@@ -794,11 +796,12 @@ static int acpi_thermal_add(struct acpi_
 	if (!tz)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, tz);
+
 	tz->device = device;
 	strscpy(tz->name, device->pnp.bus_id);
 	strscpy(acpi_device_name(device), ACPI_THERMAL_DEVICE_NAME);
 	strscpy(acpi_device_class(device), ACPI_THERMAL_CLASS);
-	device->driver_data = tz;
 
 	acpi_thermal_aml_dependency_fix(tz);
 
@@ -895,16 +898,11 @@ free_memory:
 	return result;
 }
 
-static void acpi_thermal_remove(struct acpi_device *device)
+static void acpi_thermal_remove(struct platform_device *pdev)
 {
-	struct acpi_thermal *tz;
-
-	if (!device || !acpi_driver_data(device))
-		return;
+	struct acpi_thermal *tz = platform_get_drvdata(pdev);
 
-	tz = acpi_driver_data(device);
-
-	acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY,
+	acpi_dev_remove_notify_handler(tz->device, ACPI_DEVICE_NOTIFY,
 				       acpi_thermal_notify);
 
 	flush_workqueue(acpi_thermal_pm_queue);
@@ -922,16 +920,9 @@ static int acpi_thermal_suspend(struct d
 
 static int acpi_thermal_resume(struct device *dev)
 {
-	struct acpi_thermal *tz;
+	struct acpi_thermal *tz = dev_get_drvdata(dev);
 	int i, j;
 
-	if (!dev)
-		return -EINVAL;
-
-	tz = acpi_driver_data(to_acpi_device(dev));
-	if (!tz)
-		return -EINVAL;
-
 	for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
 		struct acpi_thermal_trip *acpi_trip = &tz->trips.active[i].trip;
 
@@ -958,15 +949,14 @@ static const struct acpi_device_id  ther
 };
 MODULE_DEVICE_TABLE(acpi, thermal_device_ids);
 
-static struct acpi_driver acpi_thermal_driver = {
-	.name = "thermal",
-	.class = ACPI_THERMAL_CLASS,
-	.ids = thermal_device_ids,
-	.ops = {
-		.add = acpi_thermal_add,
-		.remove = acpi_thermal_remove,
-		},
-	.drv.pm = &acpi_thermal_pm,
+static struct platform_driver acpi_thermal_driver = {
+	.probe = acpi_thermal_probe,
+	.remove = acpi_thermal_remove,
+	.driver = {
+		.name = "acpi-thermal",
+		.acpi_match_table = thermal_device_ids,
+		.pm = &acpi_thermal_pm,
+	},
 };
 
 static int thermal_act(const struct dmi_system_id *d)
@@ -1064,7 +1054,7 @@ static int __init acpi_thermal_init(void
 	if (!acpi_thermal_pm_queue)
 		return -ENODEV;
 
-	result = acpi_bus_register_driver(&acpi_thermal_driver);
+	result = platform_driver_register(&acpi_thermal_driver);
 	if (result < 0) {
 		destroy_workqueue(acpi_thermal_pm_queue);
 		return -ENODEV;
@@ -1075,7 +1065,7 @@ static int __init acpi_thermal_init(void
 
 static void __exit acpi_thermal_exit(void)
 {
-	acpi_bus_unregister_driver(&acpi_thermal_driver);
+	platform_driver_unregister(&acpi_thermal_driver);
 	destroy_workqueue(acpi_thermal_pm_queue);
 }
 

[PATCH v1 4/4] ACPI: thermal: Rework system suspend and resume handling

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

In the process of handling system resume, acpi_thermal_resume() attempts
to power up active cooling devices to guarantee that they will be
operational when the ACPI thermal check queued by it runs.  However,
the only kind of cooling devices that can be bound to ACPI thermal zones
is fans and they are already powered up by the ACPI fan driver resume,
which additionally takes "ACPI 4" fans that don't need to be powered
up into account.

For this reason, remove the part of acpi_thermal_resume() related to
fans and in order to ensure that it will run after powering up all fans,
rename it to acpi_thermal_complete() and point the .complete() driver
callback to it.  Analogously, rename acpi_thermal_suspend() to
acpi_thermal_prepare() and point the .prepare() driver callback to it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/thermal.c |   37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -911,37 +911,26 @@ static void acpi_thermal_remove(struct p
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int acpi_thermal_suspend(struct device *dev)
+static int acpi_thermal_prepare(struct device *dev)
 {
 	/* Make sure the previously queued thermal check work has been done */
 	flush_workqueue(acpi_thermal_pm_queue);
 	return 0;
 }
 
-static int acpi_thermal_resume(struct device *dev)
+static void acpi_thermal_complete(struct device *dev)
 {
-	struct acpi_thermal *tz = dev_get_drvdata(dev);
-	int i, j;
-
-	for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
-		struct acpi_thermal_trip *acpi_trip = &tz->trips.active[i].trip;
-
-		if (!acpi_thermal_trip_valid(acpi_trip))
-			break;
-
-		for (j = 0; j < acpi_trip->devices.count; j++)
-			acpi_bus_update_power(acpi_trip->devices.handles[j], NULL);
-	}
-
-	acpi_queue_thermal_check(tz);
-
-	return AE_OK;
+	acpi_queue_thermal_check(dev_get_drvdata(dev));
 }
-#else
-#define acpi_thermal_suspend	NULL
-#define acpi_thermal_resume	NULL
-#endif
-static SIMPLE_DEV_PM_OPS(acpi_thermal_pm, acpi_thermal_suspend, acpi_thermal_resume);
+
+static const struct dev_pm_ops acpi_thermal_pm_ops = {
+	.prepare = acpi_thermal_prepare,
+	.complete = acpi_thermal_complete,
+};
+#define ACPI_THERMAL_PM	&acpi_thermal_pm_ops
+#else /* !CONFIG_PM_SLEEP */
+#define ACPI_THERMAL_PM	NULL
+#endif /* CONFIG_PM_SLEEP */
 
 static const struct acpi_device_id  thermal_device_ids[] = {
 	{ACPI_THERMAL_HID, 0},
@@ -955,7 +944,7 @@ static struct platform_driver acpi_therm
 	.driver = {
 		.name = "acpi-thermal",
 		.acpi_match_table = thermal_device_ids,
-		.pm = &acpi_thermal_pm,
+		.pm = ACPI_THERMAL_PM,
 	},
 };
 

Re: [PATCH v1 1/4] ACPI: scan: Register platform devices for thermal zones

[lihuisong (C)]

在 2025/12/10 22:42, Rafael J. Wysocki 写道:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Currently, platform devices are not registered for ACPI thermal zones
> because they are not represented as device objects in the ACPI namespace.
> Instead, they are represented as thermal zone objects, so in particular
> the platform_id flag is not set for them during enumeration because it
> is only set for objects of type ACPI_BUS_TYPE_DEVICE, but otherwise they
> are handled similarly at the ACPI core level.
>
> To facilitate converting the ACPI thermal zone driver into a platform
> one, modify acpi_set_pnp_ids() to set the platform_id flag for thermal
> zones in analogy with device objects to cause platform devices to be
> registered for them.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: lihuisong@huawei.com
>
>
>

Re: [PATCH v1 2/4] ACPI: thermal: Adjust event notification routine

[lihuisong (C)]

在 2025/12/10 22:43, Rafael J. Wysocki 写道:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Adjust acpi_thermal_notify() to cast its "data" argument to a struct
> acpi_thermal pointer istead of a struct acpi_device one, which allows
> the use of acpi_driver_data() to be limited and will facilitate
> subsequent changes.
>
> No intentional functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/acpi/thermal.c |    9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
>
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -670,8 +670,7 @@ static void acpi_thermal_unregister_ther
>   
>   static void acpi_thermal_notify(acpi_handle handle, u32 event, void *data)
>   {
> -	struct acpi_device *device = data;
> -	struct acpi_thermal *tz = acpi_driver_data(device);
> +	struct acpi_thermal *tz = data;
>   
>   	if (!tz)
>   		return;
> @@ -685,8 +684,8 @@ static void acpi_thermal_notify(acpi_han
>   		acpi_thermal_trips_update(tz, event);
>   		break;
>   	default:
> -		acpi_handle_debug(device->handle, "Unsupported event [0x%x]\n",
> -				  event);
> +		acpi_handle_debug(tz->device->handle,
> +				  "Unsupported event [0x%x]\n", event);
>   		break;
>   	}
>   }
> @@ -881,7 +880,7 @@ static int acpi_thermal_add(struct acpi_
>   		acpi_device_bid(device), deci_kelvin_to_celsius(tz->temp_dk));
>   
>   	result = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY,
> -						 acpi_thermal_notify, device);
> +						 acpi_thermal_notify, tz);
>   	if (result)
>   		goto flush_wq;
>   
LGTM,
Acked-by: lihuisong@huawei.com
>
>
>

Re: [PATCH v1 3/4] ACPI: thermal: Convert the driver to a platform one

[lihuisong (C)]

在 2025/12/10 22:43, Rafael J. Wysocki 写道:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> While binding drivers directly to struct acpi_device objects allows
> basic functionality to be provided, at least in the majority of cases,
> there are some problems with it, related to general consistency, sysfs
> layout, power management operation ordering, and code cleanliness.
>
> Overall, it is better to bind drivers to platform devices than to their
> ACPI companions, so convert the ACPI thermal zone driver to a platform
> one.
>
> While this is not expected to alter functionality, it changes sysfs
> layout and so it will be visible to user space.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
I tested this series on my platform.
Yeah, there are some changes, such as a 'wakeup' directory.
But the function is ok. And this change is also ok to 'sensors' tool.

Tested-by: lihuisong@huawei.com

>
>
>

Re: [PATCH v1 4/4] ACPI: thermal: Rework system suspend and resume handling

[lihuisong (C)]

在 2025/12/10 22:44, Rafael J. Wysocki 写道:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> In the process of handling system resume, acpi_thermal_resume() attempts
> to power up active cooling devices to guarantee that they will be
> operational when the ACPI thermal check queued by it runs.  However,
> the only kind of cooling devices that can be bound to ACPI thermal zones
> is fans and they are already powered up by the ACPI fan driver resume,
> which additionally takes "ACPI 4" fans that don't need to be powered
> up into account.
>
> For this reason, remove the part of acpi_thermal_resume() related to
> fans and in order to ensure that it will run after powering up all fans,
> rename it to acpi_thermal_complete() and point the .complete() driver
> callback to it.  Analogously, rename acpi_thermal_suspend() to
> acpi_thermal_prepare() and point the .prepare() driver callback to it.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/acpi/thermal.c |   37 +++++++++++++------------------------
>   1 file changed, 13 insertions(+), 24 deletions(-)
>
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -911,37 +911,26 @@ static void acpi_thermal_remove(struct p
>   }
>   
>   #ifdef CONFIG_PM_SLEEP
> -static int acpi_thermal_suspend(struct device *dev)
> +static int acpi_thermal_prepare(struct device *dev)
>   {
>   	/* Make sure the previously queued thermal check work has been done */
>   	flush_workqueue(acpi_thermal_pm_queue);
>   	return 0;
>   }
>   
> -static int acpi_thermal_resume(struct device *dev)
> +static void acpi_thermal_complete(struct device *dev)
>   {
> -	struct acpi_thermal *tz = dev_get_drvdata(dev);
> -	int i, j;
> -
> -	for (i = 0; i < ACPI_THERMAL_MAX_ACTIVE; i++) {
> -		struct acpi_thermal_trip *acpi_trip = &tz->trips.active[i].trip;
> -
> -		if (!acpi_thermal_trip_valid(acpi_trip))
> -			break;
> -
> -		for (j = 0; j < acpi_trip->devices.count; j++)
> -			acpi_bus_update_power(acpi_trip->devices.handles[j], NULL);
> -	}
> -
> -	acpi_queue_thermal_check(tz);
> -
> -	return AE_OK;
> +	acpi_queue_thermal_check(dev_get_drvdata(dev));
>   }
> -#else
> -#define acpi_thermal_suspend	NULL
> -#define acpi_thermal_resume	NULL
> -#endif
> -static SIMPLE_DEV_PM_OPS(acpi_thermal_pm, acpi_thermal_suspend, acpi_thermal_resume);
> +
> +static const struct dev_pm_ops acpi_thermal_pm_ops = {
> +	.prepare = acpi_thermal_prepare,
> +	.complete = acpi_thermal_complete,
> +};
> +#define ACPI_THERMAL_PM	&acpi_thermal_pm_ops
> +#else /* !CONFIG_PM_SLEEP */
> +#define ACPI_THERMAL_PM	NULL
> +#endif /* CONFIG_PM_SLEEP */
>   
>   static const struct acpi_device_id  thermal_device_ids[] = {
>   	{ACPI_THERMAL_HID, 0},
> @@ -955,7 +944,7 @@ static struct platform_driver acpi_therm
>   	.driver = {
>   		.name = "acpi-thermal",
>   		.acpi_match_table = thermal_device_ids,
> -		.pm = &acpi_thermal_pm,
> +		.pm = ACPI_THERMAL_PM,
>   	},
>   };
>   
This rework is good to me. It is more reasonable.
Acked-by: lihuisong@huawei.com
>
>
>
>