Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

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

On 1/9/2025 09:17, Thomas Weißschuh wrote:
> On 2025-01-08 11:30:12+0200, Ilpo Järvinen wrote:
>> On Tue, 7 Jan 2025, Thomas Weißschuh wrote:
>>
>>> On 2025-01-07 15:18:21-0600, Mario Limonciello wrote:
>>>> On 1/7/2025 14:50, Thomas Weißschuh wrote:
>>>>> On 2025-01-07 13:29:08-0600, Mario Limonciello wrote:
>>>>>> On 1/7/2025 11:05, Thomas Weißschuh wrote:
>>>>>>> The driver showcases the use of the new subsystem API.
>>>>>>>
>>>>>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>>>>>> ---
>>>>>>>     drivers/platform/x86/Kconfig                    | 12 ++++
>>>>>>>     drivers/platform/x86/Makefile                   |  1 +
>>>>>>>     drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
>>>>>>>     3 files changed, 91 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>>>>>> index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
>>>>>>> --- a/drivers/platform/x86/Kconfig
>>>>>>> +++ b/drivers/platform/x86/Kconfig
>>>>>>> @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
>>>>>>>     config FW_ATTR_CLASS
>>>>>>>     	tristate
>>>>>>> +config FW_ATTR_TEST
>>>>>>> +	tristate "Firmware attribute test driver"
>>>>>>> +	select FW_ATTR_CLASS
>>>>>>> +	help
>>>>>>> +	  This driver provides a test user of the firmware attribute subsystem.
>>>>>>> +
>>>>>>> +	  An instance is created at /sys/class/firmware-attributes/test/
>>>>>>> +	  container various example attributes.
>>>>>>> +
>>>>>>> +	  To compile this driver as a module, choose M here: the module
>>>>>>> +	  will be called firmware_attributes_test.
>>>>>>> +
>>>>>>
>>>>>> I think if you're going to be introducing a test driver it should be
>>>>>> compliant to what's in sysfs-class-firmware-attributes so that when it's
>>>>>> inevitably copy/pasted we end up with higher quality drivers.
>>>>>>
>>>>>> That is you need at a minimum 'type', 'current_value', 'default_value',
>>>>>> 'display_name' and 'display_name_language_code'.  Then individual types
>>>>>> employ additional requirements.
>>>>>>
>>>>>> I see 'type', 'current_value', and 'default_value, but I don't see
>>>>>> 'display_name' or 'display_name_language_code' here.
>>>>>>
>>>>>> Furthermore as this is a "string" attribute you're supposed to also have a
>>>>>> "max_length" and "min_length".
>>>>>
>>>>> Agreed that more examples are better.
>>>>>
>>>>> But are these attributes really mandatory?
>>>>> "This attribute is mandatory" is only specified for "type" and>
>>>> "current_value".
>>>>
>>>> Ah wow, I had thought they were, but you're right they're not!
>>>>
>>>>> While "possible_values" certainly looks necessary for "enumeration",
>>>>> "min_length" for "strings" does so much less.
>>>>
>>>> Even if they're not mandatory, I think it's better to include them for the
>>>> same copy/paste reason I mentioned though.
>>>
>>> Thinking about it some more, which attributes should all be included?
>>> Having all of them in there could motivate driver authors to implement
>>> them all even it would mean filling in random values.
>>> The provided examples can already be copied-and-pasted and slightly
>>> adapted to add more attributes.
>>
>> Can't you like add comments to the optional ones to reduce the incentive
>> to fill them with random junk as it's a lot easier to just delete them than
>> generating some random junk. So if a developer is unsure what to do a
>> comment telling something is optional would help to lean towards 'I can
>> safely delete this'?
> 
> That would be possible. But I'm still not convinced.
> If driver authors can't be expected to know how to implement their own
> sysfs attribute groups from the similar provided examples as needed, we
> would have to provide example code for sysfs attributes of all firmware
> attributes. And that would be a lot of them.
> 
> Also the attributes themselves would be highly repetitive. The
> interesting logic would be how to wire it up the the rest of the driver,
> and the example code can't provide copy-paste code for that.

Thinking about it a bit more what do you think about providing a macro 
helper for drivers to use?  Think about how we have macros for pm ops 
for example and drivers can optionally populate all fields with callbacks.

A macro for "enumeration" attributes, another for "string" attributes, 
and another for "integer" attributes.

For string it could have optional values .min_length and .max_length,

For enumeration it can have a callback that gets you a pointer to a 
string of possible options.

For integer attributes it can have a field for scalar value etc.