On 2025-01-09 09:37:15-0600, Mario Limonciello wrote: > 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. This is what I tried, but came to the conclusion that it would be very complex. > 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, Then we need a flags field to track if the an attribute has either of these fields, because 0 is a valid value. And macros to initialize a static struct of it, with any combination of optional attributes. And more macros to do the same for dynamically allocated structs. > For enumeration it can have a callback that gets you a pointer to a string > of possible options. Callbacks would be better as there is a clear value when a attribute is missing. > For integer attributes it can have a field for scalar value etc. Same as for .min_length and .max_length. Let me try again with callbacks.
