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. Also as for providing an even higher level interface. There exists a fairly big feature matrix. For example the display_name_language_code: * Is it used at all? * Is it the same for all attributes implemented by the driver and the sysfs attribute can be reused for them all. * Should the same handler logic be reused between different settings which only differ in their language code? The answers seem to differ between each driver and having a uniform high-level interface that can handle all cases would look horrible. So I'd like to stick with the API provided currently for now and we can revisit it if there are more drivers. As mentioned before, the current API should be a decent baseline to build on top of. Thomas
