Hi Joshua,
I have some comments on the platform profile section. The most important
one is the platform_profile probe one, the rest are suggetions.
On Fri Jan 17, 2025 at 7:43 PM -05, Joshua Grisham wrote:
> Add a new driver for Samsung Galaxy Book series notebook devices with the
> following features:
>
> - Keyboard backlight control
> - Battery extension with charge control end threshold
> - Controller for Samsung's performance modes using the platform profile
> interface
> - Adds firmware-attributes to control various system features
> - Handles various hotkeys and notifications
>
> Signed-off-by: Joshua Grisham <josh@xxxxxxxxxxxxxxxxx>
...
> +/*
> + * Platform Profile / Performance mode
> + */
> +
> +static int performance_mode_acpi_set(struct samsung_galaxybook *galaxybook,
> + const u8 performance_mode)
> +{
> + struct sawb buf = {};
> +
> + buf.safn = GB_SAFN;
> + buf.sasb = GB_SASB_PERFORMANCE_MODE;
> + export_guid(buf.caid, &GB_PERFORMANCE_MODE_GUID);
> + buf.fncn = GB_FNCN_PERFORMANCE_MODE;
> + buf.subn = GB_SUBN_PERFORMANCE_MODE_SET;
> + buf.iob0 = performance_mode;
> +
> + return galaxybook_acpi_method(galaxybook, GB_ACPI_METHOD_PERFORMANCE_MODE,
> + &buf, GB_SAWB_LEN_PERFORMANCE_MODE);
> +}
> +
> +static int performance_mode_acpi_get(struct samsung_galaxybook *galaxybook, u8 *performance_mode)
> +{
> + struct sawb buf = {};
> + int err;
> +
> + buf.safn = GB_SAFN;
> + buf.sasb = GB_SASB_PERFORMANCE_MODE;
> + export_guid(buf.caid, &GB_PERFORMANCE_MODE_GUID);
> + buf.fncn = GB_FNCN_PERFORMANCE_MODE;
> + buf.subn = GB_SUBN_PERFORMANCE_MODE_GET;
> +
> + err = galaxybook_acpi_method(galaxybook, GB_ACPI_METHOD_PERFORMANCE_MODE,
> + &buf, GB_SAWB_LEN_PERFORMANCE_MODE);
> + if (err)
> + return err;
> +
> + *performance_mode = buf.iob0;
> +
> + return 0;
> +}
> +
> +static int get_performance_mode_profile(struct samsung_galaxybook *galaxybook,
> + const u8 performance_mode,
> + enum platform_profile_option *profile)
> +{
> + switch (performance_mode) {
> + case GB_PERFORMANCE_MODE_SILENT:
> + *profile = PLATFORM_PROFILE_LOW_POWER;
> + break;
> + case GB_PERFORMANCE_MODE_QUIET:
> + *profile = PLATFORM_PROFILE_QUIET;
> + break;
> + case GB_PERFORMANCE_MODE_OPTIMIZED:
> + case GB_PERFORMANCE_MODE_OPTIMIZED_LEGACY:
> + *profile = PLATFORM_PROFILE_BALANCED;
> + break;
> + case GB_PERFORMANCE_MODE_PERFORMANCE:
> + case GB_PERFORMANCE_MODE_PERFORMANCE_LEGACY:
> + /* balanced-performance maps to Performance when Ultra exists */
> + if (test_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
> + galaxybook->platform_profile_choices))
> + *profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
> + else
> + *profile = PLATFORM_PROFILE_PERFORMANCE;
> + break;
> + case GB_PERFORMANCE_MODE_ULTRA:
> + *profile = PLATFORM_PROFILE_PERFORMANCE;
> + break;
> + case GB_PERFORMANCE_MODE_IGNORE1:
> + case GB_PERFORMANCE_MODE_IGNORE2:
> + return -EOPNOTSUPP;
> + default:
> + dev_warn(&galaxybook->platform->dev,
> + "unrecognized performance mode 0x%x\n", performance_mode);
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int galaxybook_platform_profile_set(struct device *dev,
> + enum platform_profile_option profile)
> +{
> + struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
> +
> + return performance_mode_acpi_set(galaxybook,
> + galaxybook->profile_performance_modes[profile]);
> +}
> +
> +static int galaxybook_platform_profile_get(struct device *dev,
> + enum platform_profile_option *profile)
> +{
> + struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
> + u8 performance_mode;
> + int err;
> +
> + err = performance_mode_acpi_get(galaxybook, &performance_mode);
> + if (err)
> + return err;
> +
> + return get_performance_mode_profile(galaxybook, performance_mode, profile);
> +}
> +
> +static int galaxybook_platform_profile_probe(void *drvdata, unsigned long *choices)
> +{
> + struct samsung_galaxybook *galaxybook = drvdata;
> + int i;
> +
> + for_each_set_bit(i, galaxybook->platform_profile_choices, PLATFORM_PROFILE_LAST)
> + set_bit(i, choices);
The intended use of this callback is to "probe" for available choices
here. You should move all galaxybook_performance_mode_profile_init()
logic to this method. This would eliminate the need to have a copy of
the choices bitmap.
> +
> + return 0;
> +}
> +
> +static const struct platform_profile_ops galaxybook_platform_profile_ops = {
> + .probe = galaxybook_platform_profile_probe,
> + .profile_get = galaxybook_platform_profile_get,
> + .profile_set = galaxybook_platform_profile_set,
> +};
> +
> +static int galaxybook_performance_mode_init(struct samsung_galaxybook *galaxybook)
> +{
> + enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
> + u8 performance_mode;
> + int err;
> + int i;
> +
> + err = performance_mode_acpi_get(galaxybook, &performance_mode);
> + if (err)
> + return err;
> +
> + err = get_performance_mode_profile(galaxybook, performance_mode, &profile);
> + if (err)
If this method failed we can't safely continue. I think you should
return here, else you may get an out of bounds access bellow.
> + dev_warn(&galaxybook->platform->dev,
> + "initial startup performance mode 0x%x is not mapped\n",
> + performance_mode);
> +
> + for_each_set_bit(i, galaxybook->platform_profile_choices, PLATFORM_PROFILE_LAST)
> + dev_dbg(&galaxybook->platform->dev,
> + "enabled platform profile %d using performance mode 0x%x\n",
> + i, galaxybook->profile_performance_modes[i]);
Maybe we can log this directly in the switch-case block inside
galaxybook_performance_mode_profile_init() instead of having to iterate.
> +
> + /* ensure startup performance_mode matches that mapped to its profile */
> + if (galaxybook->profile_performance_modes[profile] == performance_mode)
> + return 0;
> +
> + /* otherwise, if balanced is enabled, use it as the default */
> + if (test_bit(PLATFORM_PROFILE_BALANCED, galaxybook->platform_profile_choices))
> + return performance_mode_acpi_set(galaxybook,
> + galaxybook->profile_performance_modes[PLATFORM_PROFILE_BALANCED]);
> +
> + /* otherwise, find the first enabled profile and use that instead */
> + profile = find_next_bit_wrap(galaxybook->platform_profile_choices,
> + PLATFORM_PROFILE_LAST,
> + 0);
> +
> + if (profile == PLATFORM_PROFILE_LAST) {
> + dev_err(&galaxybook->platform->dev,
> + "no platform profiles have been enabled\n");
> + return -EOPNOTSUPP;
> + }
> +
> + return performance_mode_acpi_set(galaxybook,
> + galaxybook->profile_performance_modes[profile]);
> +}
> +
> +#define gb_pfmode(profile) galaxybook->profile_performance_modes[profile]
> +
> +static int galaxybook_performance_mode_profile_init(struct samsung_galaxybook *galaxybook)
> +{
> + enum platform_profile_option profile;
> + struct sawb buf = {};
> + unsigned int i;
> + int err;
> +
> + /* fetch supported performance mode values from ACPI method */
> + buf.safn = GB_SAFN;
> + buf.sasb = GB_SASB_PERFORMANCE_MODE;
> + export_guid(buf.caid, &GB_PERFORMANCE_MODE_GUID);
> + buf.fncn = GB_FNCN_PERFORMANCE_MODE;
> + buf.subn = GB_SUBN_PERFORMANCE_MODE_LIST;
> +
> + err = galaxybook_acpi_method(galaxybook, GB_ACPI_METHOD_PERFORMANCE_MODE,
> + &buf, GB_SAWB_LEN_PERFORMANCE_MODE);
> + if (err) {
> + dev_dbg(&galaxybook->platform->dev,
> + "failed to get supported performance modes, error %d\n", err);
> + return err;
> + }
> +
> + /* set initial default profile performance mode mappings */
> + gb_pfmode(PLATFORM_PROFILE_LOW_POWER) = GB_PERFORMANCE_MODE_SILENT;
> + gb_pfmode(PLATFORM_PROFILE_QUIET) = GB_PERFORMANCE_MODE_QUIET;
> + gb_pfmode(PLATFORM_PROFILE_BALANCED) = GB_PERFORMANCE_MODE_OPTIMIZED_LEGACY;
> + gb_pfmode(PLATFORM_PROFILE_PERFORMANCE) = GB_PERFORMANCE_MODE_PERFORMANCE_LEGACY;
> + gb_pfmode(PLATFORM_PROFILE_LAST) = GB_PERFORMANCE_MODE_INVALID;
> +
> + /*
> + * Value returned in iob0 will have the number of supported performance
> + * modes per device. The performance mode values will then be given as a
> + * list after this (iob1-iobX). Loop through the supported values and
> + * enable their mapped platform_profile choice, overriding "legacy"
> + * values along the way if a non-legacy value exists.
> + */
> + for (i = 1; i <= buf.iob0; i++) {
> + dev_dbg(&galaxybook->platform->dev,
> + "device supports performance mode 0x%x\n", buf.iob_values[i]);
> + err = get_performance_mode_profile(galaxybook, buf.iob_values[i], &profile);
Here we pass iob_values[i] through a switch-case block inside
get_performance_mode_profile() and then we do it again bellow. Maybe
this all could be done here, without having to call
get_performance_mode_profile().
> + if (err)
> + continue;
> + switch (buf.iob_values[i]) {
> + case GB_PERFORMANCE_MODE_OPTIMIZED:
> + /* override legacy Optimized value */
> + gb_pfmode(profile) = GB_PERFORMANCE_MODE_OPTIMIZED;
> + break;
> + case GB_PERFORMANCE_MODE_PERFORMANCE:
> + /* override legacy Performance value */
> + gb_pfmode(profile) = GB_PERFORMANCE_MODE_PERFORMANCE;
> + break;
> + case GB_PERFORMANCE_MODE_ULTRA:
> + /*
> + * if Ultra is supported, downgrade performance to
> + * balanced-performance
> + */
I haven't been following the entire discussion, so I don't know if Armin
changed his mind but I agree with him.
I think GB_PERFORMANCE_MODE_PERFORMANCE_LEGACY should be statically
mapped to BALANCED_PERFORMANCE and TURBO should be PERFORMANCE. This
would simplify a lot of the logic here.
> + if (test_bit(PLATFORM_PROFILE_PERFORMANCE,
> + galaxybook->platform_profile_choices)) {
> + gb_pfmode(PLATFORM_PROFILE_BALANCED_PERFORMANCE) =
> + gb_pfmode(PLATFORM_PROFILE_PERFORMANCE);
> + set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
> + galaxybook->platform_profile_choices);
> + }
> + /* override performance profile to use Ultra's value */
> + gb_pfmode(profile) = GB_PERFORMANCE_MODE_ULTRA;
> + break;
> + default:
> + break;
> + }
> + set_bit(profile, galaxybook->platform_profile_choices);
> + }
> +
> + err = galaxybook_performance_mode_init(galaxybook);
If the main goal of this method is to set an initial profile maybe we
can just directly set it after finding GB_PERFORMANCE_MODE_OPTIMIZED?
This would eliminate a bit of indirection.
Do you know if all devices support OPTIMIZED? either legacy or
non-legacy.
> + if (err) {
> + dev_dbg(&galaxybook->platform->dev,
> + "failed to initialize performance mode, error %d\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int galaxybook_platform_profile_init(struct samsung_galaxybook *galaxybook)
> +{
> + struct device *ppdev;
> + int err;
> +
> + err = galaxybook_performance_mode_profile_init(galaxybook);
> + if (err)
> + return 0;
> +
> + galaxybook->has_performance_mode = true;
> +
> + ppdev = devm_platform_profile_register(&galaxybook->platform->dev, DRIVER_NAME,
> + galaxybook, &galaxybook_platform_profile_ops);
> + if (IS_ERR(ppdev))
> + return PTR_ERR(ppdev);
> +
> + platform_profile_notify(ppdev);
No need to notify after registering. You can directly return
PTR_ERR_OR_ZERO().
~ Kurt
> <snip>
