Hi Rafael, thanks for taking a look! Comments/questions inline below... :) Den tis 18 feb. 2025 kl 17:49 skrev Rafael J. Wysocki <rafael@xxxxxxxxxx>: > > On Tue, Jan 14, 2025 at 2:22 AM Joshua Grisham <josh@xxxxxxxxxxxxxxxxx> wrote: > > > > 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]. > > > > [1]: https://lore.kernel.org/platform-driver-x86/53c5075b-1967-45d0-937f-463912dd966d@xxxxxx > > [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@xxxxxxxxxxxxxxxxx> > > --- > > drivers/acpi/fan.h | 1 + > > drivers/acpi/fan_attr.c | 37 ++++++++++++++++++++++--------------- > > drivers/acpi/fan_core.c | 24 ++++++++++++++++-------- > > drivers/acpi/fan_hwmon.c | 6 ++++++ > > 4 files changed, 45 insertions(+), 23 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; > > I would use has_fst instead of this, that is > > + bool has_fst; > > which would be true when acpi4 is true, but not necessarily the other > way around. > No problem at all with this, I actually struggled a lot with what to call this (or to change the flag to use different bits instead even but did not want to rock the boat tooooo much ;) ) .. I still think naming is probably one of the hardest things in software! > > 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; > > So the above may become > > if (!fan->acpi4) > return 0; > Yes I think this is also better and feels more "self-documenting." > > + > > + 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..b51b1481c 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; > > And here one could do > > if (acpi_fan_has_fst(device)) { > fan->has_fst = true; > fan->acpi4 = acpi_fan_is_acpi4(device); > } > > and if I'm not mistaken, the check for _FST presence could be dropped > from acpi_fan_is_acpi4(). > Yes if you only call acpi_fan_is_acpi4() after you have already gotten positive from acpi_fan_has_fst() then "has _FST" is already baked in. Maybe a bit less obvious if you only look at the code within the function acpi_fan_is_acpi4() but more efficient and less code will be executed overall so if you are ok with this then I will change it like this :) > > + > > + if (fan->acpi4) { > > result = acpi_fan_get_fif(device); > > if (result) > > return result; > > @@ -335,7 +345,7 @@ static int acpi_fan_probe(struct platform_device *pdev) > > result = acpi_fan_get_fps(device); > > if (result) > > return result; > > - > > + } else if (fan->acpi4 || fan->acpi4_only_fst) { > > Then, all of the checks like the above could be replaced with > fan->has_fst checks. > Yes and looks nicer! > > result = devm_acpi_fan_create_hwmon(device); > > if (result) > > return result; > > @@ -343,8 +353,6 @@ static int acpi_fan_probe(struct platform_device *pdev) > > result = acpi_fan_create_attributes(device); > > if (result) > > return result; > > - > > - fan->acpi4 = true; > > } else { > > result = acpi_device_update_power(device, NULL); > > if (result) { > > @@ -391,7 +399,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 +409,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); > > @@ -415,7 +423,7 @@ static void acpi_fan_remove(struct platform_device *pdev) > > static int acpi_fan_suspend(struct device *dev) > > { > > struct acpi_fan *fan = dev_get_drvdata(dev); > > - if (fan->acpi4) > > + if (fan->acpi4 || fan->acpi4_only_fst) > > return 0; > > > > acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D0); > > @@ -428,7 +436,7 @@ static int acpi_fan_resume(struct device *dev) > > int result; > > struct acpi_fan *fan = dev_get_drvdata(dev); > > > > - if (fan->acpi4) > > + if (fan->acpi4 || fan->acpi4_only_fst) > > return 0; > > > > result = acpi_device_update_power(ACPI_COMPANION(dev), NULL); > > diff --git a/drivers/acpi/fan_hwmon.c b/drivers/acpi/fan_hwmon.c > > index bd0d31a39..87bee018c 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. > > + */ > > Nit: One-line comment here, please. > Yes this one is the easiest by far ;) I think it was more that I copy-pasted from other comments within the switch and kept it the same "style" but agree this new one is short enough to be one line. I will probably anyway change the comment a bit to fit better with the change to `(!fan->acpi4)` per your comment below but will see what I can do! I will get these tweaks in as a v4 to this patch shortly. Thanks again! Regards, Joshua > > + if (fan->acpi4_only_fst) > > And this would become > > if (!fan->acpi4) > > > + return 0; > > + > > /* > > * When in fine grain control mode, not every fan control value > > * has an associated fan performance state. > > --
