Re: [PATCH v3 0/6] platform/x86: firmware_attributes_class: Add a high level API

[Derek J. Clark]

On June 21, 2025 8:27:06 PM PDT, Kurt Borja <kuurtb@xxxxxxxxx> wrote:
>On Sun Jun 22, 2025 at 12:01 AM -03, Derek J. Clark wrote:
>>
>>
>> On June 21, 2025 5:04:03 PM PDT, Kurt Borja <kuurtb@xxxxxxxxx> wrote:
>>>Hi all,
>>>
>>>I apologize for taking so long. I've been a bit busy these last few
>>>weeks.
>>>
>>>After my discussion with Joshua on v2, I realized the API I made was not
>>>ergonomic at all and it didn't exactly respond to driver needs. In this
>>>version I tried a completely different approach and IMO it's much much
>>>better now.
>>>
>>>First of all I adopted standard sysfs terminology for everything. A
>>>"firmware attribute" is just an attribute_group under the attributes/
>>>directory so everything related to this concept is just called "group"
>>>now. Everything refered as properties in the previous patch are now just
>>>plain "attributes".
>>>
>>>This new API revolves around the `fwat_{bool,enum,int,str}_data`
>>>structs. These hold all the metadata a "firmware_attribute" of that
>>>given type needs.
>>>
>>>These structs also hold `read` and `write` callbacks for the
>>>current_value attribute, because obviously that value is always dynamic.
>>>However the rest of attributes (default_value, display_name, min, max,
>>>etc) are constant.
>>
>> Hi Kurt,
>>
>> In the lenovo-wmi drivers the min/max for multiple attributes are actually dynamic based on if power is AC connected or on battery. Looking at patch 2 I might be able to do some pointer manipulation with the attribute's "data" member for those events to make this work, but it would be a lot easier if there was a simple way for me to call my own functions here instead. Perhaps a function pointer could be used to override the default method here? 
>
>Hi Derek,
>
>All attributes in a given group have the same show method. Maybe we can
>let users override this with their own show method, i.e. Add a
>
>	ssize_t (*attr_show)(struct device *dev, const struct fwat_attribute *attr, const char *buf)
>
>to struct fwat_group_data. That should be fairly simple to implement.
>
>Did you have another solution in mind?
>
>

That should work, yeah. 
- Derek