Hi Riwen,
Usually it's a good practice to Cc anybody who has commented on previous
versions. It makes it easier to follow your updates.
A couple of comments below.
Riwen Lu <luriwen@xxxxxxxxxxx> writes:
> From: Riwen Lu <luriwen@xxxxxxxxxx>
>
> Commit 239708a3af44 ("ACPI: Split out ACPI PSS from ACPI Processor
> driver"), moves processor thermal registration to acpi_pss_perf_init(),
> which doesn't get executed if ACPI_CPU_FREQ_PSS is not enabled.
>
> As ARM64 supports P-states using CPPC, it should be possible to also
> support processor passive cooling even if PSS is not enabled. Split
> out the processor thermal cooling register from ACPI PSS to support
> this, and move it into a separate function in processor_thermal.c.
>
> Signed-off-by: Riwen Lu <luriwen@xxxxxxxxxx>
> ---
> drivers/acpi/Kconfig | 2 +-
> drivers/acpi/Makefile | 5 +--
> drivers/acpi/processor_driver.c | 72 ++++----------------------------
> drivers/acpi/processor_thermal.c | 69 ++++++++++++++++++++++++++++++
> include/acpi/processor.h | 6 ++-
> 5 files changed, 84 insertions(+), 70 deletions(-)
>
[...]
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
[...]
> @@ -239,7 +183,7 @@ static int __acpi_processor_start(struct acpi_device *device)
> return 0;
>
> result = -ENODEV;
> - acpi_pss_perf_exit(pr, device);
> + acpi_processor_thermal_exit(pr);
>
> err_power_exit:
> acpi_processor_power_exit(pr);
> @@ -277,10 +221,10 @@ static int acpi_processor_stop(struct device *dev)
> return 0;
> acpi_processor_power_exit(pr);
>
> - acpi_pss_perf_exit(pr, device);
> -
> acpi_cppc_processor_exit(pr);
>
> + acpi_processor_thermal_exit(pr);
> +
> return 0;
> }
>
> diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
> index d8b2dfcd59b5..93928db2ae5f 100644
> --- a/drivers/acpi/processor_thermal.c
> +++ b/drivers/acpi/processor_thermal.c
> @@ -266,3 +266,72 @@ const struct thermal_cooling_device_ops processor_cooling_ops = {
> .get_cur_state = processor_get_cur_state,
> .set_cur_state = processor_set_cur_state,
> };
> +
> +int acpi_processor_thermal_init(struct acpi_processor *pr)
> +{
> + struct acpi_device *device;
> + int result = 0;
> +
> + if (!pr)
> + return -ENODEV;
What's the reason for this check? When will "pr" be NULL in this code
path?
> +
> + device = acpi_fetch_acpi_dev(pr->handle);
> + if (!device)
> + return -ENODEV;
Wouldn't it be better to pass the acpi_device into the function as well?
The device is already available in the caller and it'll avoid having to
convert it back.
> +
> + pr->cdev = thermal_cooling_device_register("Processor", device,
> + &processor_cooling_ops);
> + if (IS_ERR(pr->cdev)) {
> + result = PTR_ERR(pr->cdev);
> + return result;
> + }
> +
> + dev_dbg(&device->dev, "registered as cooling_device%d\n",
> + pr->cdev->id);
> +
> + result = sysfs_create_link(&device->dev.kobj,
> + &pr->cdev->device.kobj,
> + "thermal_cooling");
> + if (result) {
> + dev_err(&device->dev,
> + "Failed to create sysfs link 'thermal_cooling'\n");
> + goto err_thermal_unregister;
> + }
> +
> + result = sysfs_create_link(&pr->cdev->device.kobj,
> + &device->dev.kobj,
> + "device");
> + if (result) {
> + dev_err(&pr->cdev->device,
> + "Failed to create sysfs link 'device'\n");
> + goto err_remove_sysfs_thermal;
> + }
> +
> + return 0;
> +
> +err_remove_sysfs_thermal:
> + sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
> +err_thermal_unregister:
> + thermal_cooling_device_unregister(pr->cdev);
> +
> + return result;
> +}
> +
> +void acpi_processor_thermal_exit(struct acpi_processor *pr)
> +{
> + struct acpi_device *device;
> +
> + if (!pr)
> + return;
> +
> + device = acpi_fetch_acpi_dev(pr->handle);
> + if (!device)
> + return;
The same comment about passing the acpi_device structure applies here as
well.
> +
> + if (pr->cdev) {
> + sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
> + sysfs_remove_link(&pr->cdev->device.kobj, "device");
> + thermal_cooling_device_unregister(pr->cdev);
> + pr->cdev = NULL;
> + }
> +}
[...]
