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@xxxxxxxxxxxxxx>
---
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.
