Re: [PATCH v7 13/22] ACPI: platform_profile: Add profile attribute for class interface

[Armin Wolf <W_Armin@gmx.de>]

Am 21.11.24 um 15:27 schrieb Mark Pearson:

> On Thu, Nov 21, 2024, at 6:10 AM, Ilpo Järvinen wrote:
>> I don't know why you dropped Mario and the list, I reinstanstated those
>> two.
> Unintentional - sorry. Hit the reply button instead of reply-all and didn't notice. Doh.
>
>> On Wed, 20 Nov 2024, Mark Pearson wrote:
>>> On Wed, Nov 20, 2024, at 9:56 AM, Ilpo Järvinen wrote:
>>>> On Tue, 19 Nov 2024, Mario Limonciello wrote:
>>>>
>>>>> Reading and writing the `profile` sysfs file will use the callbacks for
>>>>> the platform profile handler to read or set the given profile.
>>>>>
>>>>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>> ---
>>>>> v7:
>>>>>   * Remove extra handler set
>>>>>   * Remove err variable
>>>>> v6:
>>>>>   * Fix return
>>>>> v5:
>>>>>   * Drop recovery flow
>>>>>   * Don't get profile before setting (not needed)
>>>>>   * Simplify casting for call to _store_class_profile()
>>>>>   * Only notify legacy interface of changes
>>>>>   * Adjust mutex use
>>>>> ---
>>>>>   drivers/acpi/platform_profile.c | 100 ++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 100 insertions(+)
>>>>>
>>>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/platform_profile.c
>>>>> index 9d6ead043994c..1530e6096cd39 100644
>>>>> --- a/drivers/acpi/platform_profile.c
>>>>> +++ b/drivers/acpi/platform_profile.c
>>>>>   static struct attribute *profile_attrs[] = {
>>>>>   	&dev_attr_name.attr,
>>>>>   	&dev_attr_choices.attr,
>>>>> +	&dev_attr_profile.attr,
>>>> I started to wonder if "choices" is good name for the other attribute as
>>>> it is the set of values "profile" accepts? Should they be bound by the
>>>> naming too like "profile_choices" or something along those lines so the
>>>> connection between the two is very evident?
>>>>
>>> Wouldn't it be weird to not have it in sync with the main sysfs entry
>>> (which I don't think we can change at that point without messing up
>>> userspace).
>>>
>>> I think it would be more confusing to have different naming as it would
>>> imply they're different things.
>> Ah, I didn't realize there's a pre-existing convention. Then just
>> disregard what I suggested.
>>
> No idea if it's a convention - I just would think it would be confusing for users.
>
> Thanks
> Mark

I personally would prefer the attribute name "choices", but i would also accept if the attribute was
named "profile_choices". I think adding the "profile" prefix to the attribute when it is already handled
by the platform-profile class is silly.

Either way:
Reviewed-by: Armin Wolf <W_Armin@gmx.de>