Re: [PATCH v3] ACPI: fan: Add fan speed reporting for fans with only _FST

[Armin Wolf <W_Armin@gmx.de>]

Am 25.01.25 um 11:07 schrieb Joshua Grisham:

> Add support for ACPI fans with _FST to report their speed even if they do
> not support fan control.
>
> As suggested by Armin Wolf [1] and per the Windows Thermal Management
> Design Guide [2], Samsung Galaxy Book series devices (and possibly many
> more devices where the Windows guide was strictly followed) only implement
> the _FST method and do not support ACPI-based fan control.
>
> Currently, these fans are not supported by the kernel driver but this patch
> will make some very small adjustments to allow them to be supported.
>
> This patch is tested and working for me on a Samsung Galaxy Book2 Pro whose
> DSDT (and several other Samsung Galaxy Book series notebooks which
> currently have the same issue) can be found at [3].

Any updates on this patch? For me it seems ready for mainline.

Thanks,
Armin Wolf

>
> [1]: https://lore.kernel.org/platform-driver-x86/53c5075b-1967-45d0-937f-463912dd966d@gmx.de
> [2]: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/design-guide
> [3]: https://github.com/joshuagrisham/samsung-galaxybook-extras/tree/8e3087a06b8bdcdfdd081367af4b744a56cc4ee9/dsdt
>
> Signed-off-by: Joshua Grisham <josh@joshuagrisham.com>
> ---
>
> v1->v2:
> - Still allow acpi4_only_fst fans to update power state on
>    suspend/resume
> - Fix if / else if logic error
> - Also hide hwmon_power_input for acpi4_only_fst fans
>
> v2->v3:
> - Still allow acpi4_only_fst fans to set initial power state on probe
> ---
>   drivers/acpi/fan.h       |  1 +
>   drivers/acpi/fan_attr.c  | 37 ++++++++++++++++++++++---------------
>   drivers/acpi/fan_core.c  | 22 +++++++++++++++++-----
>   drivers/acpi/fan_hwmon.c | 12 ++++++++++++
>   4 files changed, 52 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/acpi/fan.h b/drivers/acpi/fan.h
> index 488b51e2c..d0aad88a7 100644
> --- a/drivers/acpi/fan.h
> +++ b/drivers/acpi/fan.h
> @@ -49,6 +49,7 @@ struct acpi_fan_fst {
>
>   struct acpi_fan {
>   	bool acpi4;
> +	bool acpi4_only_fst;
>   	struct acpi_fan_fif fif;
>   	struct acpi_fan_fps *fps;
>   	int fps_count;
> diff --git a/drivers/acpi/fan_attr.c b/drivers/acpi/fan_attr.c
> index f4f6e2381..d83f88429 100644
> --- a/drivers/acpi/fan_attr.c
> +++ b/drivers/acpi/fan_attr.c
> @@ -75,15 +75,6 @@ int acpi_fan_create_attributes(struct acpi_device *device)
>   	struct acpi_fan *fan = acpi_driver_data(device);
>   	int i, status;
>
> -	sysfs_attr_init(&fan->fine_grain_control.attr);
> -	fan->fine_grain_control.show = show_fine_grain_control;
> -	fan->fine_grain_control.store = NULL;
> -	fan->fine_grain_control.attr.name = "fine_grain_control";
> -	fan->fine_grain_control.attr.mode = 0444;
> -	status = sysfs_create_file(&device->dev.kobj, &fan->fine_grain_control.attr);
> -	if (status)
> -		return status;
> -
>   	/* _FST is present if we are here */
>   	sysfs_attr_init(&fan->fst_speed.attr);
>   	fan->fst_speed.show = show_fan_speed;
> @@ -92,7 +83,19 @@ int acpi_fan_create_attributes(struct acpi_device *device)
>   	fan->fst_speed.attr.mode = 0444;
>   	status = sysfs_create_file(&device->dev.kobj, &fan->fst_speed.attr);
>   	if (status)
> -		goto rem_fine_grain_attr;
> +		return status;
> +
> +	if (fan->acpi4_only_fst)
> +		return 0;
> +
> +	sysfs_attr_init(&fan->fine_grain_control.attr);
> +	fan->fine_grain_control.show = show_fine_grain_control;
> +	fan->fine_grain_control.store = NULL;
> +	fan->fine_grain_control.attr.name = "fine_grain_control";
> +	fan->fine_grain_control.attr.mode = 0444;
> +	status = sysfs_create_file(&device->dev.kobj, &fan->fine_grain_control.attr);
> +	if (status)
> +		goto rem_fst_attr;
>
>   	for (i = 0; i < fan->fps_count; ++i) {
>   		struct acpi_fan_fps *fps = &fan->fps[i];
> @@ -109,18 +112,18 @@ int acpi_fan_create_attributes(struct acpi_device *device)
>
>   			for (j = 0; j < i; ++j)
>   				sysfs_remove_file(&device->dev.kobj, &fan->fps[j].dev_attr.attr);
> -			goto rem_fst_attr;
> +			goto rem_fine_grain_attr;
>   		}
>   	}
>
>   	return 0;
>
> -rem_fst_attr:
> -	sysfs_remove_file(&device->dev.kobj, &fan->fst_speed.attr);
> -
>   rem_fine_grain_attr:
>   	sysfs_remove_file(&device->dev.kobj, &fan->fine_grain_control.attr);
>
> +rem_fst_attr:
> +	sysfs_remove_file(&device->dev.kobj, &fan->fst_speed.attr);
> +
>   	return status;
>   }
>
> @@ -129,9 +132,13 @@ void acpi_fan_delete_attributes(struct acpi_device *device)
>   	struct acpi_fan *fan = acpi_driver_data(device);
>   	int i;
>
> +	sysfs_remove_file(&device->dev.kobj, &fan->fst_speed.attr);
> +
> +	if (fan->acpi4_only_fst)
> +		return;
> +
>   	for (i = 0; i < fan->fps_count; ++i)
>   		sysfs_remove_file(&device->dev.kobj, &fan->fps[i].dev_attr.attr);
>
> -	sysfs_remove_file(&device->dev.kobj, &fan->fst_speed.attr);
>   	sysfs_remove_file(&device->dev.kobj, &fan->fine_grain_control.attr);
>   }
> diff --git a/drivers/acpi/fan_core.c b/drivers/acpi/fan_core.c
> index 10016f52f..66aa1be64 100644
> --- a/drivers/acpi/fan_core.c
> +++ b/drivers/acpi/fan_core.c
> @@ -211,6 +211,11 @@ static bool acpi_fan_is_acpi4(struct acpi_device *device)
>   	       acpi_has_method(device->handle, "_FST");
>   }
>
> +static bool acpi_fan_has_fst(struct acpi_device *device)
> +{
> +	return acpi_has_method(device->handle, "_FST");
> +}
> +
>   static int acpi_fan_get_fif(struct acpi_device *device)
>   {
>   	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -327,7 +332,12 @@ static int acpi_fan_probe(struct platform_device *pdev)
>   	device->driver_data = fan;
>   	platform_set_drvdata(pdev, fan);
>
> -	if (acpi_fan_is_acpi4(device)) {
> +	if (acpi_fan_is_acpi4(device))
> +		fan->acpi4 = true;
> +	else if (acpi_fan_has_fst(device))
> +		fan->acpi4_only_fst = true;
> +
> +	if (fan->acpi4) {
>   		result = acpi_fan_get_fif(device);
>   		if (result)
>   			return result;
> @@ -335,7 +345,9 @@ static int acpi_fan_probe(struct platform_device *pdev)
>   		result = acpi_fan_get_fps(device);
>   		if (result)
>   			return result;
> +	}
>
> +	if (fan->acpi4 || fan->acpi4_only_fst) {
>   		result = devm_acpi_fan_create_hwmon(device);
>   		if (result)
>   			return result;
> @@ -343,9 +355,9 @@ static int acpi_fan_probe(struct platform_device *pdev)
>   		result = acpi_fan_create_attributes(device);
>   		if (result)
>   			return result;
> +	}
>
> -		fan->acpi4 = true;
> -	} else {
> +	if (!fan->acpi4) {
>   		result = acpi_device_update_power(device, NULL);
>   		if (result) {
>   			dev_err(&device->dev, "Failed to set initial power state\n");
> @@ -391,7 +403,7 @@ static int acpi_fan_probe(struct platform_device *pdev)
>   err_unregister:
>   	thermal_cooling_device_unregister(cdev);
>   err_end:
> -	if (fan->acpi4)
> +	if (fan->acpi4 || fan->acpi4_only_fst)
>   		acpi_fan_delete_attributes(device);
>
>   	return result;
> @@ -401,7 +413,7 @@ static void acpi_fan_remove(struct platform_device *pdev)
>   {
>   	struct acpi_fan *fan = platform_get_drvdata(pdev);
>
> -	if (fan->acpi4) {
> +	if (fan->acpi4 || fan->acpi4_only_fst) {
>   		struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>
>   		acpi_fan_delete_attributes(device);
> diff --git a/drivers/acpi/fan_hwmon.c b/drivers/acpi/fan_hwmon.c
> index bd0d31a39..d0668ecc2 100644
> --- a/drivers/acpi/fan_hwmon.c
> +++ b/drivers/acpi/fan_hwmon.c
> @@ -43,6 +43,12 @@ static umode_t acpi_fan_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_
>   		case hwmon_fan_input:
>   			return 0444;
>   		case hwmon_fan_target:
> +			/*
> +			 * Fans with only _FST do not support fan control.
> +			 */
> +			if (fan->acpi4_only_fst)
> +				return 0;
> +
>   			/*
>   			 * When in fine grain control mode, not every fan control value
>   			 * has an associated fan performance state.
> @@ -57,6 +63,12 @@ static umode_t acpi_fan_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_
>   	case hwmon_power:
>   		switch (attr) {
>   		case hwmon_power_input:
> +			/*
> +			 * Fans with only _FST do not support fan control.
> +			 */
> +			if (fan->acpi4_only_fst)
> +				return 0;
> +
>   			/*
>   			 * When in fine grain control mode, not every fan control value
>   			 * has an associated fan performance state.