Re: [PATCH v4 00/20] Add support for binding ACPI platform profile to multiple drivers

[Mario Limonciello <mario.limonciello@amd.com>]

On 11/5/2024 16:47, Mark Pearson wrote:
> Hi Mario,
> 
> On Tue, Nov 5, 2024, at 10:32 AM, Mario Limonciello wrote:
>> Currently there are a number of ASUS products on the market that happen to
>> have ACPI objects for amd-pmf to bind to as well as an ACPI platform
>> profile provided by asus-wmi.
>>
>> The ACPI platform profile support created by amd-pmf on these ASUS
>> products is "Function 9" which is specifically for "BIOS or EC
>> notification" of power slider position. This feature is actively used
>> by some designs such as Framework 13 and Framework 16.
>>
>> On these ASUS designs we keep on quirking more and more of them to turn
>> off this notification so that asus-wmi can bind.
>>
>> This however isn't how Windows works.  "Multiple" things are notified for
>> the power slider position. This series adjusts Linux to behave similarly.
>>
>> Multiple drivers can now register an ACPI platform profile and will react
>> to set requests.
>>
>> To avoid chaos, only positions that are common to both drivers are
>> accepted when the legacy /sys/firmware/acpi/platform_profile interface
>> is used.
>>
>> This series also adds a new concept of a "custom" profile.  This allows
>> userspace to discover that there are multiple driver handlers that are
>> configured differently.
>>
>> This series also allows dropping all of the PMF quirks from amd-pmf.
>>
>> v4:
>>   * Drop the list; iterate classes
>>   * Drop patches that didn't make sense without list
>>   * Cover alienware-wmi as well (recently merged to platform-x86/for-next)
>>   * Drop requirement for balanced
>>   * Rename platform-profile class members to 'profile', 'options', 'name'
>>   * Drop the name in /sys/class/platform-profile and just use ida value.
>>     IE platform-profile-0
>>
>> Mario Limonciello (20):
>>    ACPI: platform-profile: Add a name member to handlers
>>    platform/x86/dell: dell-pc: Create platform device
>>    ACPI: platform_profile: Add device pointer into platform profile
>>      handler
>>    ACPI: platform_profile: Add platform handler argument to
>>      platform_profile_remove()
>>    ACPI: platform_profile: Move sanity check out of the mutex
>>    ACPI: platform_profile: Move matching string for new profile out of
>>      mutex
>>    ACPI: platform_profile: Use guard(mutex) for register/unregister
>>    ACPI: platform_profile: Use `scoped_cond_guard`
>>    ACPI: platform_profile: Create class for ACPI platform profile
>>    ACPI: platform_profile: Add name attribute to class interface
>>    ACPI: platform_profile: Add choices attribute for class interface
>>    ACPI: platform_profile: Add profile attribute for class interface
>>    ACPI: platform_profile: Notify change events on register and
>>      unregister
>>    ACPI: platform_profile: Only show profiles common for all handlers
>>    ACPI: platform_profile: Add concept of a "custom" profile
>>    ACPI: platform_profile: Make sure all profile handlers agree on
>>      profile
>>    ACPI: platform_profile: Check all profile handler to calculate next
>>    ACPI: platform_profile: Allow multiple handlers
>>    platform/x86/amd: pmf: Drop all quirks
>>    Documentation: Add documentation about class interface for platform
>>      profiles
>>
>>   .../userspace-api/sysfs-platform_profile.rst  |  28 ++
>>   drivers/acpi/platform_profile.c               | 446 ++++++++++++++----
>>   .../surface/surface_platform_profile.c        |   8 +-
>>   drivers/platform/x86/acer-wmi.c               |  10 +-
>>   drivers/platform/x86/amd/pmf/Makefile         |   2 +-
>>   drivers/platform/x86/amd/pmf/core.c           |   1 -
>>   drivers/platform/x86/amd/pmf/pmf-quirks.c     |  66 ---
>>   drivers/platform/x86/amd/pmf/pmf.h            |   3 -
>>   drivers/platform/x86/amd/pmf/sps.c            |   4 +-
>>   drivers/platform/x86/asus-wmi.c               |   6 +-
>>   drivers/platform/x86/dell/alienware-wmi.c     |   8 +-
>>   drivers/platform/x86/dell/dell-pc.c           |  39 +-
>>   drivers/platform/x86/hp/hp-wmi.c              |   8 +-
>>   drivers/platform/x86/ideapad-laptop.c         |   4 +-
>>   .../platform/x86/inspur_platform_profile.c    |   7 +-
>>   drivers/platform/x86/thinkpad_acpi.c          |   4 +-
>>   include/linux/platform_profile.h              |   7 +-
>>   17 files changed, 456 insertions(+), 195 deletions(-)
>>   delete mode 100644 drivers/platform/x86/amd/pmf/pmf-quirks.c
>>
>>
>> base-commit: d68cb6023356af3bd3193983ad4ec03954a0b3e2
>> -- 
>> 2.43.0
> 
> For the series - I tried it out on my T14s G6 AMD, and it all looks to be working nicely and as expected.
> The thinkpad-acpi and amd-pmf classes show up. I can tweak them individually and 'custom' shows up under firmware/acpi/platform_profile.
> I tried various combo's and didn't see any issues.
> If you have any recommendations of things to try let me know.
> 
> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> 
> Mark

Thanks for testing!  The fact that Lenovo and ASUS are both doing this 
with recent laptops makes it all the stronger of a case to do it rather 
than let them race for who gets the coveted platform profile slot.

If you've already tried mixing and matching combinations that's all I 
was going to suggest.

I'll wait for Armin's review and then I'll respin with the tags and 
small things you and Ilpo found.

BTW if I missed any important feedback of yours from v3 please let me 
know.  It was a lot of overhaul to switch to this way of doing things 
and a lot of it didn't make sense anymore.