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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4404 bytes --]

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.

Hi,

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'?

-- 
 i.

> 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
>