[PATCH RFC 0/5] platform/x86: firmware_attributes_class: Add a high level API

[PATCH RFC 0/5] platform/x86: firmware_attributes_class: Add a high level API

[Kurt Borja]

Hi all,

These series adds the _long awaited_ API for the Firmware Attributes
class.

You'll find all the details in the commit messages and kernel-doc.

I think it's easier to understand by example, so I used the
samsung-galaxybook driver for this purpose (last patch). IMO code
readibility, simplicity, maintainability, etc. is greatly improved, but
there is still room for improvement of the API itself. For this reason I
submitted this as an RFC.

As always, your feedback is very appreciated :)

Overview
========

Patch 1-2: New API with docs included.
  Patch 3: New firwmare attributes type
  Patch 4: Misc Maintainers patch
  Patch 5: samsung-galaxybook example

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
Kurt Borja (4):
      platform/x86: firmware_attributes_class: Add a high level API
      platform/x86: firmware_attributes_class: Add a boolean type
      MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry
      platform/x86: samsung-galaxybook: Transition to new firmware_attributes API

Thomas Weißschuh (1):
      platform/x86: firmware_attributes_class: Add device initialization methods

 .../ABI/testing/sysfs-class-firmware-attributes    |   1 +
 MAINTAINERS                                        |   7 +
 drivers/platform/x86/firmware_attributes_class.c   | 424 +++++++++++++++++++++
 drivers/platform/x86/firmware_attributes_class.h   | 283 ++++++++++++++
 drivers/platform/x86/samsung-galaxybook.c          | 299 ++++++---------
 5 files changed, 829 insertions(+), 185 deletions(-)
---
base-commit: 3c415b1df95c06ae4f9bdb166541ab366b862cc2
change-id: 20250326-fw-attrs-api-0eea7c0225b6
-- 
 ~ Kurt

[PATCH RFC 1/5] platform/x86: firmware_attributes_class: Add device initialization methods

[Kurt Borja]

From: Thomas Weißschuh <linux@weissschuh.net>

Currently each user of firmware_attributes_class has to manually set up
kobjects, devices, etc.

Provide this infrastructure out-of-the-box through the newly introduced
fwat_device_register().

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Co-developed-by: Kurt Borja <kuurtb@gmail.com>
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/platform/x86/firmware_attributes_class.c | 165 +++++++++++++++++++++++
 drivers/platform/x86/firmware_attributes_class.h |  44 ++++++
 2 files changed, 209 insertions(+)

diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 736e96c186d9dc6d945517f090e9af903e93bbf4..58ab1495ba3bd449cfe17de2827a57a0c5937788 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -2,7 +2,12 @@
 
 /* Firmware attributes class helper module */
 
+#include <linux/device.h>
+#include <linux/device/class.h>
+#include <linux/kobject.h>
 #include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/types.h>
 #include "firmware_attributes_class.h"
 
 const struct class firmware_attributes_class = {
@@ -10,6 +15,164 @@ const struct class firmware_attributes_class = {
 };
 EXPORT_SYMBOL_GPL(firmware_attributes_class);
 
+static ssize_t fwat_attrs_kobj_show(struct kobject *kobj, struct attribute *attr,
+				    char *buf)
+{
+	const struct fwat_attribute *fattr = to_fwat_attribute(attr);
+	struct fwat_device *fadev = to_fwat_device(kobj);
+
+	if (!fattr->show)
+		return -ENOENT;
+
+	return fattr->show(fadev->dev, fattr, buf);
+}
+
+static ssize_t fwat_attrs_kobj_store(struct kobject *kobj, struct attribute *attr,
+				     const char *buf, size_t count)
+{
+	const struct fwat_attribute *fattr = to_fwat_attribute(attr);
+	struct fwat_device *fadev = to_fwat_device(kobj);
+
+	if (!fattr->store)
+		return -ENOENT;
+
+	return fattr->store(fadev->dev, fattr, buf, count);
+}
+
+static const struct sysfs_ops fwat_attrs_kobj_ops = {
+	.show	= fwat_attrs_kobj_show,
+	.store	= fwat_attrs_kobj_store,
+};
+
+static void fwat_attrs_kobj_release(struct kobject *kobj)
+{
+	struct fwat_device *fadev = to_fwat_device(kobj);
+
+	kfree(fadev);
+}
+
+static const struct kobj_type fwat_attrs_ktype = {
+	.sysfs_ops	= &fwat_attrs_kobj_ops,
+	.release	= fwat_attrs_kobj_release,
+};
+
+/**
+ * fwat_device_register - Create and register a firmware-attributes class
+ *			  device
+ * @parent: Parent device
+ * @name: Name of the class device
+ * @data: Drvdata of the class device
+ * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
+ *
+ * NOTE: @groups are attached to the .attrs_kobj of the new fwat_device which
+ * has a custom ktype, which makes use of `struct fwat_attribute` to embed
+ * attributes.
+ *
+ * Return: pointer to the new fwat_device on success, ERR_PTR on failure
+ */
+struct fwat_device *
+fwat_device_register(struct device *parent, const char *name, void *data,
+		     const struct attribute_group **groups)
+{
+	struct fwat_device *fadev;
+	struct device *dev;
+	int ret;
+
+	if (!parent || !name)
+		return ERR_PTR(-EINVAL);
+
+	fadev = kzalloc(sizeof(*fadev), GFP_KERNEL);
+	if (!fadev)
+		return ERR_PTR(-ENOMEM);
+
+	dev = device_create(&firmware_attributes_class, parent, MKDEV(0, 0),
+			    data, "%s", name);
+	if (IS_ERR(dev)) {
+		kfree(fadev);
+		return ERR_CAST(dev);
+	}
+
+	ret = kobject_init_and_add(&fadev->attrs_kobj, &fwat_attrs_ktype, &dev->kobj,
+				   "attributes");
+	if (ret)
+		goto out_kobj_put;
+
+	if (groups) {
+		ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
+		if (ret)
+			goto out_kobj_unregister;
+	}
+
+	fadev->dev = dev;
+	fadev->groups = groups;
+
+	kobject_uevent(&fadev->attrs_kobj, KOBJ_ADD);
+
+	return fadev;
+
+out_kobj_unregister:
+	kobject_del(&fadev->attrs_kobj);
+
+out_kobj_put:
+	kobject_put(&fadev->attrs_kobj);
+	device_unregister(dev);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(fwat_device_register);
+
+void fwat_device_unregister(struct fwat_device *fwadev)
+{
+	if (fwadev->groups)
+		sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->groups);
+	kobject_del(&fwadev->attrs_kobj);
+	kobject_put(&fwadev->attrs_kobj);
+	device_unregister(fwadev->dev);
+}
+EXPORT_SYMBOL_GPL(fwat_device_unregister);
+
+static void devm_fwat_device_release(void *data)
+{
+	struct fwat_device *fadev = data;
+
+	fwat_device_unregister(fadev);
+}
+
+/**
+ * devm_fwat_device_register - Create and register a firmware-attributes class
+ *			       device
+ * @parent: Parent device
+ * @name: Name of the class device
+ * @data: Drvdata of the class device
+ * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
+ *
+ * Device managed version of fwat_device_register().
+ *
+ * NOTE: @groups are attached to the .attrs_kobj of the new fwat_device which
+ * has a custom ktype, which makes use of `struct fwat_attribute` to embed
+ * attributes.
+ *
+ * Return: pointer to the new fwat_device on success, ERR_PTR on failure
+ */
+struct fwat_device *
+devm_fwat_device_register(struct device *parent, const char *name, void *data,
+			  const struct attribute_group **groups)
+{
+	struct fwat_device *fadev;
+	int ret;
+
+	fadev = fwat_device_register(parent, name, data, groups);
+	if (IS_ERR(fadev))
+		return fadev;
+
+	ret = devm_add_action_or_reset(parent, devm_fwat_device_release, fadev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return fadev;
+}
+EXPORT_SYMBOL_GPL(devm_fwat_device_register);
+
 static __init int fw_attributes_class_init(void)
 {
 	return class_register(&firmware_attributes_class);
@@ -23,5 +186,7 @@ static __exit void fw_attributes_class_exit(void)
 module_exit(fw_attributes_class_exit);
 
 MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
+MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net>");
+MODULE_AUTHOR("Kurt Borja <kuurtb@gmail.com>");
 MODULE_DESCRIPTION("Firmware attributes class helper module");
 MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
index d27abe54fcf9812a2f0868eec5426bbc8e7eb21c..ad94bf91e5af30a2b8feb9abf224ee6f0d17600a 100644
--- a/drivers/platform/x86/firmware_attributes_class.h
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -5,8 +5,52 @@
 #ifndef FW_ATTR_CLASS_H
 #define FW_ATTR_CLASS_H
 
+#include <linux/container_of.h>
+#include <linux/device.h>
 #include <linux/device/class.h>
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
 
 extern const struct class firmware_attributes_class;
 
+/**
+ * struct fwat_device - The firmware-attributes device
+ * @dev: The class device.
+ * @attrs_kobj: The "attributes" root kobject.
+ * @groups: Sysfs groups attached to the @attrs_kobj.
+ */
+struct fwat_device {
+	struct device *dev;
+	struct kobject attrs_kobj;
+	const struct attribute_group **groups;
+};
+
+#define to_fwat_device(_k)	container_of_const(_k, struct fwat_device, attrs_kobj)
+
+/**
+ * struct fwat_attribute - The firmware-attributes's custom attribute
+ * @attr: Embedded struct attribute.
+ * @show: Show method called by the "attributes" kobject's ktype.
+ * @store: Store method called by the "attributes" kobject's ktype.
+ */
+struct fwat_attribute {
+	struct attribute attr;
+	ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
+			char *buf);
+	ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
+			 const char *buf, size_t count);
+};
+
+#define to_fwat_attribute(_a) container_of_const(_a, struct fwat_attribute, attr)
+
+struct fwat_device * __must_check
+fwat_device_register(struct device *parent, const char *name, void *data,
+		     const struct attribute_group **groups);
+
+void fwat_device_unregister(struct fwat_device *fwadev);
+
+struct fwat_device * __must_check
+devm_fwat_device_register(struct device *parent, const char *name, void *data,
+			  const struct attribute_group **groups);
+
 #endif /* FW_ATTR_CLASS_H */

-- 
2.49.0

[PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API

[Kurt Borja]

Add an attribute configuration mechanism through the newly introduced
`struct fwat_dev_config`, which makes use of other documented structs
and callbacks.

This API aims to be simple, yet flexible. In order to accomplish this,
the following features were taken into account:

* Ability to statically define attributes
* Custom read/write callbacks for each attribute type
* Ability to map attributes to numbers in order to differentiate them in
  callbacks (`aux` number)
* Ability to reuse read/write callbacks in different attributes
* Ability to reuse property selection in different attributes
* Optional visibility callback for dynamic attribute visibility

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/platform/x86/firmware_attributes_class.c | 249 ++++++++++++++++++++++-
 drivers/platform/x86/firmware_attributes_class.h | 228 +++++++++++++++++++++
 2 files changed, 474 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 58ab1495ba3bd449cfe17de2827a57a0c5937788..7cfb0f49f235728c7450a82a7e9d00b8963d3dea 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -7,14 +7,233 @@
 #include <linux/kobject.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/sysfs.h>
 #include <linux/types.h>
 #include "firmware_attributes_class.h"
 
+#define to_fwat_attribute_ext(_a) container_of_const(_a, struct fwat_attribute_ext, attr)
+
+struct fwat_attribute_ext {
+	struct fwat_attribute attr;
+	enum fwat_property prop;
+	const struct fwat_attr_config *config;
+};
+
 const struct class firmware_attributes_class = {
 	.name = "firmware-attributes",
 };
 EXPORT_SYMBOL_GPL(firmware_attributes_class);
 
+static const char * const fwat_type_labels[] = {
+	[fwat_type_integer]		= "integer",
+	[fwat_type_string]		= "string",
+	[fwat_type_enumeration]		= "enumeration",
+};
+
+static const char * const fwat_prop_labels[] = {
+	[FWAT_PROP_DISPLAY_NAME]		= "display_name",
+	[FWAT_PROP_LANGUAGE_CODE]		= "display_name_language_code",
+	[FWAT_PROP_DEFAULT]			= "default",
+
+	[FWAT_INT_PROP_MIN]			= "min_value",
+	[FWAT_INT_PROP_MAX]			= "max_value",
+	[FWAT_INT_PROP_INCREMENT]		= "scalar_increment",
+
+	[FWAT_STR_PROP_MIN]			= "min_length",
+	[FWAT_STR_PROP_MAX]			= "max_length",
+
+	[FWAT_ENUM_PROP_POSSIBLE_VALUES]	= "possible_values",
+};
+
+static ssize_t
+fwat_type_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
+{
+	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+	const struct fwat_attr_config *config = ext->config;
+
+	return sysfs_emit(buf, "%s\n", fwat_type_labels[config->type]);
+}
+
+static ssize_t
+fwat_property_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
+{
+	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+	const struct fwat_attr_config *config = ext->config;
+
+	if (!config->ops->prop_read)
+		return -EOPNOTSUPP;
+
+	return config->ops->prop_read(dev, config->aux, ext->prop, buf);
+}
+
+static ssize_t
+fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
+{
+	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+	const struct fwat_attr_config *config = ext->config;
+	const char *str;
+	long int_val;
+	int ret;
+
+	switch (config->type) {
+	case fwat_type_integer:
+		ret = config->ops->integer_read(dev, config->aux, &int_val);
+		if (ret)
+			return ret;
+
+		return sysfs_emit(buf, "%ld\n", int_val);
+	case fwat_type_string:
+		ret = config->ops->string_read(dev, config->aux, &str);
+		if (ret)
+			return ret;
+
+		return sysfs_emit(buf, "%s\n", str);
+	case fwat_type_enumeration:
+		ret = config->ops->enumeration_read(dev, config->aux, &str);
+		if (ret)
+			return ret;
+
+		return sysfs_emit(buf, "%s\n", str);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static ssize_t
+fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
+			 const char *buf, size_t count)
+{
+	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
+	const struct fwat_attr_config *config = ext->config;
+	long int_val;
+	int ret;
+
+	switch (config->type) {
+	case fwat_type_integer:
+		ret = kstrtol(buf, 0, &int_val);
+		if (ret)
+			return ret;
+
+		ret = config->ops->integer_write(dev, config->aux, int_val);
+		break;
+	case fwat_type_string:
+		ret = config->ops->string_write(dev, config->aux, buf);
+		break;
+	case fwat_type_enumeration:
+		ret = config->ops->enumeration_write(dev, config->aux, buf);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return ret ? ret : count;
+}
+
+static struct attribute *
+fwat_alloc_attr(struct device *dev, const struct fwat_attr_config *config,
+		const char *attr_name, umode_t mode, enum fwat_property prop,
+		ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
+				char *buf),
+		ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
+				 const char *buf, size_t count))
+{
+	struct fwat_attribute_ext *fattr;
+
+	fattr = devm_kzalloc(dev, sizeof(*fattr), GFP_KERNEL);
+	if (!fattr)
+		return NULL;
+
+	fattr->attr.attr.name = attr_name;
+	fattr->attr.attr.mode = mode;
+	fattr->attr.show = show;
+	fattr->attr.store = store;
+	fattr->prop = prop;
+	fattr->config = config;
+	sysfs_attr_init(&fattr->attr.attr);
+
+	return &fattr->attr.attr;
+}
+
+static struct attribute **
+fwat_create_attrs(struct device *dev, const struct fwat_attr_config *config)
+{
+	struct attribute **attrs;
+	enum fwat_property prop;
+	unsigned int index = 0;
+
+	attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL);
+	if (!attrs)
+		return NULL;
+
+	/*
+	 * Create optional attributes
+	 */
+	for (; index < config->num_props; index++) {
+		prop = config->props[index];
+		attrs[index] = fwat_alloc_attr(dev, config, fwat_prop_labels[prop],
+					       0444, prop, fwat_property_show, NULL);
+	}
+
+	/*
+	 * Create mandatory attributes
+	 */
+	attrs[index++] = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL);
+	attrs[index++] = fwat_alloc_attr(dev, config, "current_value", 0644, 0,
+					 fwat_current_value_show, fwat_current_value_store);
+
+	return attrs;
+}
+
+static const struct attribute_group *
+fwat_create_group(struct device *dev, const struct fwat_attr_config *config)
+{
+	struct attribute_group *group;
+	struct attribute **attrs;
+
+	group = devm_kzalloc(dev, sizeof(*group), GFP_KERNEL);
+	if (!group)
+		return NULL;
+
+	attrs = fwat_create_attrs(dev, config);
+	if (!attrs)
+		return NULL;
+
+	group->name = config->name;
+	group->attrs = attrs;
+
+	return group;
+}
+
+static const struct attribute_group **
+fwat_create_auto_groups(struct device *dev, const struct fwat_dev_config *config)
+{
+	const struct attribute_group **groups;
+	const struct attribute_group *grp;
+	unsigned int index = 0;
+	size_t ngroups = 0;
+
+	while (config->attrs_config[ngroups])
+		ngroups++;
+
+	groups = devm_kcalloc(dev, ngroups + 1, sizeof(*groups), GFP_KERNEL);
+	if (!groups)
+		return NULL;
+
+	for (unsigned int i = 0; i < ngroups; i++) {
+		if (config->is_visible &&
+		    !config->is_visible(dev, config->attrs_config[i]))
+			continue;
+
+		grp = fwat_create_group(dev, config->attrs_config[i]);
+		if (!grp)
+			return NULL;
+
+		groups[index++] = grp;
+	}
+
+	return groups;
+}
+
 static ssize_t fwat_attrs_kobj_show(struct kobject *kobj, struct attribute *attr,
 				    char *buf)
 {
@@ -61,6 +280,7 @@ static const struct kobj_type fwat_attrs_ktype = {
  *			  device
  * @parent: Parent device
  * @name: Name of the class device
+ * @config: Device configuration
  * @data: Drvdata of the class device
  * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
  *
@@ -72,8 +292,10 @@ static const struct kobj_type fwat_attrs_ktype = {
  */
 struct fwat_device *
 fwat_device_register(struct device *parent, const char *name, void *data,
+		     const struct fwat_dev_config *config,
 		     const struct attribute_group **groups)
 {
+	const struct attribute_group **auto_groups;
 	struct fwat_device *fadev;
 	struct device *dev;
 	int ret;
@@ -97,19 +319,36 @@ fwat_device_register(struct device *parent, const char *name, void *data,
 	if (ret)
 		goto out_kobj_put;
 
-	if (groups) {
-		ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
+	if (config) {
+		auto_groups = fwat_create_auto_groups(dev, config);
+		if (!auto_groups) {
+			ret = -ENOMEM;
+			goto out_kobj_unregister;
+		}
+
+		ret = sysfs_create_groups(&fadev->attrs_kobj, auto_groups);
 		if (ret)
 			goto out_kobj_unregister;
 	}
 
+	if (groups) {
+		ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
+		if (ret)
+			goto out_remove_auto_groups;
+	}
+
 	fadev->dev = dev;
 	fadev->groups = groups;
+	fadev->auto_groups = groups;
 
 	kobject_uevent(&fadev->attrs_kobj, KOBJ_ADD);
 
 	return fadev;
 
+out_remove_auto_groups:
+	if (config)
+		sysfs_remove_groups(&fadev->attrs_kobj, auto_groups);
+
 out_kobj_unregister:
 	kobject_del(&fadev->attrs_kobj);
 
@@ -125,6 +364,8 @@ void fwat_device_unregister(struct fwat_device *fwadev)
 {
 	if (fwadev->groups)
 		sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->groups);
+	if (fwadev->auto_groups)
+		sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->auto_groups);
 	kobject_del(&fwadev->attrs_kobj);
 	kobject_put(&fwadev->attrs_kobj);
 	device_unregister(fwadev->dev);
@@ -143,6 +384,7 @@ static void devm_fwat_device_release(void *data)
  *			       device
  * @parent: Parent device
  * @name: Name of the class device
+ * @config: Device configuration
  * @data: Drvdata of the class device
  * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
  *
@@ -156,12 +398,13 @@ static void devm_fwat_device_release(void *data)
  */
 struct fwat_device *
 devm_fwat_device_register(struct device *parent, const char *name, void *data,
+			  const struct fwat_dev_config *config,
 			  const struct attribute_group **groups)
 {
 	struct fwat_device *fadev;
 	int ret;
 
-	fadev = fwat_device_register(parent, name, data, groups);
+	fadev = fwat_device_register(parent, name, data, config, groups);
 	if (IS_ERR(fadev))
 		return fadev;
 
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
index ad94bf91e5af30a2b8feb9abf224ee6f0d17600a..f250875d785c3439682c43c693f98e1c20e9ff54 100644
--- a/drivers/platform/x86/firmware_attributes_class.h
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -18,11 +18,14 @@ extern const struct class firmware_attributes_class;
  * @dev: The class device.
  * @attrs_kobj: The "attributes" root kobject.
  * @groups: Sysfs groups attached to the @attrs_kobj.
+ * @auto_groups: Sysgs groups generated from &struct fwat_attr_config attached.
+ * to the @attrs_kobj
  */
 struct fwat_device {
 	struct device *dev;
 	struct kobject attrs_kobj;
 	const struct attribute_group **groups;
+	const struct attribute_group **auto_groups;
 };
 
 #define to_fwat_device(_k)	container_of_const(_k, struct fwat_device, attrs_kobj)
@@ -30,6 +33,7 @@ struct fwat_device {
 /**
  * struct fwat_attribute - The firmware-attributes's custom attribute
  * @attr: Embedded struct attribute.
+ * @aux: Auxiliary number defined by the user.
  * @show: Show method called by the "attributes" kobject's ktype.
  * @store: Store method called by the "attributes" kobject's ktype.
  */
@@ -43,14 +47,238 @@ struct fwat_attribute {
 
 #define to_fwat_attribute(_a) container_of_const(_a, struct fwat_attribute, attr)
 
+enum fwat_attr_type {
+	fwat_type_integer,
+	fwat_type_string,
+	fwat_type_enumeration,
+};
+
+enum fwat_property {
+	FWAT_PROP_DISPLAY_NAME,
+	FWAT_PROP_LANGUAGE_CODE,
+	FWAT_PROP_DEFAULT,
+
+	FWAT_INT_PROP_MIN,
+	FWAT_INT_PROP_MAX,
+	FWAT_INT_PROP_INCREMENT,
+
+	FWAT_STR_PROP_MIN,
+	FWAT_STR_PROP_MAX,
+
+	FWAT_ENUM_PROP_POSSIBLE_VALUES,
+};
+
+struct fwat_attr_config;
+
+/**
+ * struct fwat_attr_ops - Operations for a firmware *attribute*
+ * @prop_read: Callback for retrieving each configured property of an attribute.
+ * @integer_read: Callback for reading the current_value of an attribute of
+ *                type *integer*.
+ * @integer_write: Callback for writing the current_value of an attribute of
+ *                 type *integer*.
+ * @string_read: Callback for reading the current_value of an attribute of type
+ *               *string*.
+ * @string_write: Callback for writing the current_value of an attribute of type
+ *                *string*.
+ * @enumeration_read: Callback for reading the current_value of an attribute of
+ *                    type *enumeration*.
+ * @enumeration_write: Callback for writing the current_value of an attribute
+ *                     of type *enumeration*.
+ */
+struct fwat_attr_ops {
+	ssize_t (*prop_read)(struct device *dev, long aux,
+			     enum fwat_property prop, const char *buf);
+	union {
+		struct {
+			int (*integer_read)(struct device *dev, long aux,
+					    long *val);
+			int (*integer_write)(struct device *dev, long aux,
+					     long val);
+		};
+		struct {
+			int (*string_read)(struct device *dev, long aux,
+					   const char **str);
+			int (*string_write)(struct device *dev, long aux,
+					    const char *str);
+		};
+		struct {
+			int (*enumeration_read)(struct device *dev, long aux,
+						const char **str);
+			int (*enumeration_write)(struct device *dev, long aux,
+						 const char *str);
+		};
+	};
+};
+
+/**
+ * struct fwat_attr_config - Configuration for a single firmware *attribute*
+ * @name: Name of the sysfs group associated with this *attribute*.
+ * @type: Type of this *attribute*.
+ * @aux: Auxiliary number defined by the user, which will be passed to
+ *       read/write callbacks.
+ * @ops: Operations for this *attribute*.
+ * @props: Array of properties of this *attribute*.
+ * @num_props: Size of the props array.
+ */
+struct fwat_attr_config {
+	const char *name;
+	enum fwat_attr_type type;
+	long aux;
+	const struct fwat_attr_ops *ops;
+	const enum fwat_property *props;
+	size_t num_props;
+};
+
+/**
+ * DEFINE_SIMPLE_FWAT_OPS() - Define static &struct fwat_attr_ops for a simple
+ *                            *attribute* with no properties, i.e. No
+ *                            &fwat_attr_ops.read_prop callback
+ * @_name: Prefix of the `read` and `write` callbacks.
+ * @_type: Firmware *attribute* type.
+ *
+ * Example:
+ *
+ * static int example_read(...) {...}
+ * static int example_write(...) {...}
+ *
+ * DEFINE_SIMPLE_FWAT_OPS(example, ...);
+ */
+#define DEFINE_SIMPLE_FWAT_OPS(_name, _type)		\
+	static const struct fwat_attr_ops _name##_ops = { \
+		._type##_read = _name##_read,		\
+		._type##_write = _name##_write,		\
+	}
+
+/**
+ * DEFINE_FWAT_OPS() - Define static &struct fwat_attr_ops with all callbacks.
+ * @_name: Prefix of the `read` and `write` callbacks.
+ * @_type: Firmware *attribute* type.
+ *
+ * Example:
+ *
+ * static int example_read(...) {...}
+ * static int example_write(...) {...}
+ * static int example_prop_read(...) {...}
+ *
+ * DEFINE_FWAT_OPS(example, ...);
+ */
+#define DEFINE_FWAT_OPS(_name, _type)			\
+	static const struct fwat_attr_ops _name##_ops = { \
+		.prop_read = _name##_prop_read,		\
+		._type##_read = _name##_read,		\
+		._type##_write = _name##_write,		\
+	}
+
+/**
+ * FWAT_CONFIG() - Configuration pointer for a single firmware *attribute*.
+ * @_name: String name of this *attribute*.
+ * @_type: Firmware *attribute* type.
+ * @_ops: Pointer to &struct fwat_attr_ops.
+ * @_props: Pointer to a enum fwat_property array.
+ * @_num_props: Size of the @_props array.
+ *
+ * This is a convenience macro to quickly construct a &struct fwat_attr_config
+ * array, which will be passed to &struct fwat_dev_config.
+ *
+ * Example:
+ *
+ * static int example_read(...) {...}
+ * static int example_write(...) {...}
+ * static int example_prop_read(...) {...}
+ *
+ * DEFINE_FWAT_OPS(example, ...);
+ *
+ * static const enum fwat_property props[] = {...};
+ *
+ * static const struct fwat_attr_config * const attrs_config[] = {
+ *	FWAT_CONFIG(example, ..., &example_fwat_ops, props,
+ *		    ARRAY_SIZE(props)),
+ *	...
+ *	NULL
+ * };
+ *
+ * static const struct fwat_dev_config fdev_config = {
+ *	.attrs_config = attrs_config,
+ *	...
+ * }
+ */
+#define FWAT_CONFIG(_name, _type, _ops, _props, _num_props) \
+	(&(const struct fwat_attr_config) {	\
+		.name = _name,				\
+		.type = fwat_type_##_type,		\
+		.ops = _ops,				\
+		.num_props = _num_props,		\
+		.props = _props,			\
+	})
+
+/**
+ * FWAT_CONFIG_AUX() - Configuration pointer for a single firmware *attribute*
+ *                     with an auxiliary number defined by the user
+ * @_name: String name of this *attribute*.
+ * @_type: Firmware *attribute* type.
+ * @_aux: Auxiliary number defined by the user.
+ * @_ops: Pointer to &struct fwat_attr_ops.
+ * @_props: Pointer to a enum fwat_property array.
+ * @_num_props: Size of the @_props array.
+ *
+ * This is a convenience macro to quickly construct a &struct fwat_attr_config
+ * array, which will be passed to &struct fwat_dev_config.
+ *
+ * Example:
+ *
+ * static int example_read(...) {...}
+ * static int example_write(...) {...}
+ * static int example_prop_read(...) {...}
+ *
+ * DEFINE_FWAT_OPS(example, ...);
+ *
+ * static const enum fwat_property props[] = {...};
+ *
+ * static const struct fwat_attr_config * const config[] = {
+ *	FWAT_CONFIG_AUX(example, ..., n, &example_fwat_ops, props,
+ *			ARRAY_SIZE(props)),
+ *	...
+ *	NULL
+ * };
+ *
+ * static const struct fwat_dev_config fdev_config = {
+ *	.attrs_config = attrs_config,
+ *	...
+ * }
+ */
+#define FWAT_CONFIG_AUX(_name, _type, _aux, _ops, _props, _num_props) \
+	(&(const struct fwat_attr_config) {		\
+		.name = _name,				\
+		.type = fwat_type_##_type,		\
+		.aux = _aux,				\
+		.ops = _ops,				\
+		.num_props = _num_props,		\
+		.props = _props,			\
+	})
+
+/**
+ * struct fwat_dev_config - Configuration for this devices's
+ *                          &fwat_device.auto_groups
+ * @attrs_config: NULL terminated &struct fwat_attr_config array.
+ * @is_visible: Optional visibility callback to determine the visibility
+ *              of each auto_group.
+ */
+struct fwat_dev_config {
+	const struct fwat_attr_config *const *attrs_config;
+	bool (*is_visible)(struct device *dev, const struct fwat_attr_config *config);
+};
+
 struct fwat_device * __must_check
 fwat_device_register(struct device *parent, const char *name, void *data,
+		     const struct fwat_dev_config *config,
 		     const struct attribute_group **groups);
 
 void fwat_device_unregister(struct fwat_device *fwadev);
 
 struct fwat_device * __must_check
 devm_fwat_device_register(struct device *parent, const char *name, void *data,
+			  const struct fwat_dev_config *config,
 			  const struct attribute_group **groups);
 
 #endif /* FW_ATTR_CLASS_H */

-- 
2.49.0

[PATCH RFC 3/5] platform/x86: firmware_attributes_class: Add a boolean type

[Kurt Borja]

Add a `boolean` attribute type to prevent the use of the `enumeration`
type for this simple specification.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 .../ABI/testing/sysfs-class-firmware-attributes          |  1 +
 drivers/platform/x86/firmware_attributes_class.c         | 16 ++++++++++++++++
 drivers/platform/x86/firmware_attributes_class.h         | 11 +++++++++++
 3 files changed, 28 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index 2713efa509b465a39bf014180794bf487e5b42d6..dc117e694416aed3f1f7ba0ebb1d0c23337b2298 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -21,6 +21,7 @@ Description:
 			- enumeration: a set of pre-defined valid values
 			- integer: a range of numerical values
 			- string
+			- bool
 
 		HP specific types
 		-----------------
diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
index 7cfb0f49f235728c7450a82a7e9d00b8963d3dea..13b10a1209be726b00fa1ebad6148778e165101e 100644
--- a/drivers/platform/x86/firmware_attributes_class.c
+++ b/drivers/platform/x86/firmware_attributes_class.c
@@ -26,6 +26,7 @@ EXPORT_SYMBOL_GPL(firmware_attributes_class);
 
 static const char * const fwat_type_labels[] = {
 	[fwat_type_integer]		= "integer",
+	[fwat_type_boolean]		= "boolean",
 	[fwat_type_string]		= "string",
 	[fwat_type_enumeration]		= "enumeration",
 };
@@ -72,6 +73,7 @@ fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, c
 	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
 	const struct fwat_attr_config *config = ext->config;
 	const char *str;
+	bool bool_val;
 	long int_val;
 	int ret;
 
@@ -82,6 +84,12 @@ fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, c
 			return ret;
 
 		return sysfs_emit(buf, "%ld\n", int_val);
+	case fwat_type_boolean:
+		ret = config->ops->boolean_read(dev, config->aux, &bool_val);
+		if (ret)
+			return ret;
+
+		return sysfs_emit(buf, "%u\n", bool_val);
 	case fwat_type_string:
 		ret = config->ops->string_read(dev, config->aux, &str);
 		if (ret)
@@ -105,6 +113,7 @@ fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
 {
 	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
 	const struct fwat_attr_config *config = ext->config;
+	bool bool_val;
 	long int_val;
 	int ret;
 
@@ -116,6 +125,13 @@ fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
 
 		ret = config->ops->integer_write(dev, config->aux, int_val);
 		break;
+	case fwat_type_boolean:
+		ret = kstrtobool(buf, &bool_val);
+		if (ret)
+			return ret;
+
+		ret = config->ops->boolean_write(dev, config->aux, bool_val);
+		break;
 	case fwat_type_string:
 		ret = config->ops->string_write(dev, config->aux, buf);
 		break;
diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
index f250875d785c3439682c43c693f98e1c20e9ff54..d95d2024b0d9630ee3750ec26e18874965ec5cff 100644
--- a/drivers/platform/x86/firmware_attributes_class.h
+++ b/drivers/platform/x86/firmware_attributes_class.h
@@ -49,6 +49,7 @@ struct fwat_attribute {
 
 enum fwat_attr_type {
 	fwat_type_integer,
+	fwat_type_boolean,
 	fwat_type_string,
 	fwat_type_enumeration,
 };
@@ -77,6 +78,10 @@ struct fwat_attr_config;
  *                type *integer*.
  * @integer_write: Callback for writing the current_value of an attribute of
  *                 type *integer*.
+ * @boolean_read: Callback for reading the current_value of an attribute of type
+ *                *boolean*.
+ * @boolean_write: Callback for writing the current_value of an attribute of
+ *                 type *boolean*.
  * @string_read: Callback for reading the current_value of an attribute of type
  *               *string*.
  * @string_write: Callback for writing the current_value of an attribute of type
@@ -96,6 +101,12 @@ struct fwat_attr_ops {
 			int (*integer_write)(struct device *dev, long aux,
 					     long val);
 		};
+		struct {
+			int (*boolean_read)(struct device *dev, long aux,
+					    bool *val);
+			int (*boolean_write)(struct device *dev, long aux,
+					     bool val);
+		};
 		struct {
 			int (*string_read)(struct device *dev, long aux,
 					   const char **str);

-- 
2.49.0

[PATCH RFC 4/5] MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry

[Kurt Borja]

Add entry for the FIRMWARE ATTRIBUTES CLASS.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f9417b4e9bafd1a4033bba2fdb4a4e6fb68cf219..c4adad420a8ffee7bc7624d3edc8ba36aec5cb40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9190,6 +9190,13 @@ F:	include/linux/firewire.h
 F:	include/uapi/linux/firewire*.h
 F:	tools/firewire/
 
+FIRMWARE ATTRIBUTES CLASS
+M:	Kurt Borja <kuurtb@gmail.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/x86/firmware_attributes_class.*
+K:	(devm_)?fwat_device_(un)?register
+
 FIRMWARE FRAMEWORK FOR ARMV8-A
 M:	Sudeep Holla <sudeep.holla@arm.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)

-- 
2.49.0

[PATCH RFC 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API

[Kurt Borja]

Transition to new firmware_attributes API and replace `enumeration` types
with the simpler `boolean` type.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/platform/x86/samsung-galaxybook.c | 299 ++++++++++++------------------
 1 file changed, 114 insertions(+), 185 deletions(-)

diff --git a/drivers/platform/x86/samsung-galaxybook.c b/drivers/platform/x86/samsung-galaxybook.c
index 5878a351993eb05a4c5c2c75b4915d972ce9becc..4fe3601fa5d282e7f8ae0a9b7e973da0998a5a72 100644
--- a/drivers/platform/x86/samsung-galaxybook.c
+++ b/drivers/platform/x86/samsung-galaxybook.c
@@ -36,8 +36,6 @@ struct samsung_galaxybook {
 	struct platform_device *platform;
 	struct acpi_device *acpi;
 
-	struct device *fw_attrs_dev;
-	struct kset *fw_attrs_kset;
 	/* block in case firmware attributes are updated in multiple threads */
 	struct mutex fw_attr_lock;
 
@@ -66,13 +64,7 @@ enum galaxybook_fw_attr_id {
 	GB_ATTR_BLOCK_RECORDING,
 };
 
-static const char * const galaxybook_fw_attr_name[] = {
-	[GB_ATTR_POWER_ON_LID_OPEN] = "power_on_lid_open",
-	[GB_ATTR_USB_CHARGING]      = "usb_charging",
-	[GB_ATTR_BLOCK_RECORDING]   = "block_recording",
-};
-
-static const char * const galaxybook_fw_attr_desc[] = {
+static const char * const galaxybook_fwat_desc[] = {
 	[GB_ATTR_POWER_ON_LID_OPEN] = "Power On Lid Open",
 	[GB_ATTR_USB_CHARGING]      = "USB Charging",
 	[GB_ATTR_BLOCK_RECORDING]   = "Block Recording",
@@ -908,209 +900,146 @@ static int galaxybook_block_recording_init(struct samsung_galaxybook *galaxybook
 
 /* Firmware Attributes setup */
 
-static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+static int galaxybook_fwat_read(struct device *dev, long aux, bool *val)
 {
-	return sysfs_emit(buf, "enumeration\n");
-}
-
-static struct kobj_attribute fw_attr_type = __ATTR_RO(type);
-
-static ssize_t default_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
-	return sysfs_emit(buf, "0\n");
-}
-
-static struct kobj_attribute fw_attr_default_value = __ATTR_RO(default_value);
-
-static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
-	return sysfs_emit(buf, "0;1\n");
-}
-
-static struct kobj_attribute fw_attr_possible_values = __ATTR_RO(possible_values);
-
-static ssize_t display_name_language_code_show(struct kobject *kobj, struct kobj_attribute *attr,
-					       char *buf)
-{
-	return sysfs_emit(buf, "%s\n", GB_ATTR_LANGUAGE_CODE);
-}
-
-static struct kobj_attribute fw_attr_display_name_language_code =
-	__ATTR_RO(display_name_language_code);
-
-static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
-	struct galaxybook_fw_attr *fw_attr =
-		container_of(attr, struct galaxybook_fw_attr, display_name);
-
-	return sysfs_emit(buf, "%s\n", galaxybook_fw_attr_desc[fw_attr->fw_attr_id]);
-}
-
-static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
-{
-	struct galaxybook_fw_attr *fw_attr =
-		container_of(attr, struct galaxybook_fw_attr, current_value);
+	struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
 	bool value;
 	int err;
 
-	err = fw_attr->get_value(fw_attr->galaxybook, &value);
+	switch (aux) {
+	case GB_ATTR_POWER_ON_LID_OPEN:
+		err = power_on_lid_open_acpi_get(galaxybook, &value);
+		break;
+	case GB_ATTR_USB_CHARGING:
+		err = usb_charging_acpi_get(galaxybook, &value);
+		break;
+	case GB_ATTR_BLOCK_RECORDING:
+		err = block_recording_acpi_get(galaxybook, &value);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
 	if (err)
 		return err;
 
-	return sysfs_emit(buf, "%u\n", value);
+	*val = value;
+
+	return 0;
 }
 
-static ssize_t current_value_store(struct kobject *kobj, struct kobj_attribute *attr,
-				   const char *buf, size_t count)
+static int galaxybook_fwat_write(struct device *dev, long aux, bool val)
 {
-	struct galaxybook_fw_attr *fw_attr =
-		container_of(attr, struct galaxybook_fw_attr, current_value);
-	struct samsung_galaxybook *galaxybook = fw_attr->galaxybook;
+	struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
+	int err;
+
+	switch (aux) {
+	case GB_ATTR_POWER_ON_LID_OPEN:
+		err = power_on_lid_open_acpi_set(galaxybook, val);
+		break;
+	case GB_ATTR_USB_CHARGING:
+		err = usb_charging_acpi_set(galaxybook, val);
+		break;
+	case GB_ATTR_BLOCK_RECORDING:
+		err = block_recording_acpi_set(galaxybook, val);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return err;
+}
+
+static ssize_t galaxybook_fwat_prop_read(struct device *dev, long aux,
+					 enum fwat_property prop, const char *buf)
+{
+	switch (prop) {
+	case FWAT_PROP_DISPLAY_NAME:
+		return sysfs_emit("%s\n", galaxybook_fwat_desc[aux]);
+	case FWAT_PROP_LANGUAGE_CODE:
+		return sysfs_emit("%s\n", GB_ATTR_LANGUAGE_CODE);
+	case FWAT_PROP_DEFAULT:
+		return sysfs_emit("%d\n", 0);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+DEFINE_FWAT_OPS(galaxybook_fwat, boolean);
+
+static bool galaxybook_fwat_is_visible(struct device *dev,
+				       const struct fwat_attr_config *config)
+{
+	struct samsung_galaxybook *galaxybook = dev_get_drvdata(dev);
 	bool value;
 	int err;
 
-	if (!count)
-		return -EINVAL;
+	switch (config->aux) {
+	case GB_ATTR_POWER_ON_LID_OPEN:
+		err = power_on_lid_open_acpi_get(galaxybook, &value);
+		break;
+	case GB_ATTR_USB_CHARGING:
+		err = usb_charging_acpi_get(galaxybook, &value);
+		break;
+	case GB_ATTR_BLOCK_RECORDING:
+		return galaxybook->has_block_recording;
+	default:
+		return false;
+	}
 
-	err = kstrtobool(buf, &value);
-	if (err)
-		return err;
-
-	guard(mutex)(&galaxybook->fw_attr_lock);
-
-	err = fw_attr->set_value(galaxybook, value);
-	if (err)
-		return err;
-
-	return count;
+	return !err;
 }
 
-#define NUM_FW_ATTR_ENUM_ATTRS  6
+static const enum fwat_property galaxybook_fwat_props[] = {
+	FWAT_PROP_DISPLAY_NAME,
+	FWAT_PROP_LANGUAGE_CODE,
+	FWAT_PROP_DEFAULT,
+};
 
-static int galaxybook_fw_attr_init(struct samsung_galaxybook *galaxybook,
-				   const enum galaxybook_fw_attr_id fw_attr_id,
-				   int (*get_value)(struct samsung_galaxybook *galaxybook,
-						    bool *value),
-				   int (*set_value)(struct samsung_galaxybook *galaxybook,
-						    const bool value))
+static const struct fwat_attr_config * const galaxybook_fwat_config[] = {
+	FWAT_CONFIG_AUX("power_on_lid_open", boolean,
+			GB_ATTR_POWER_ON_LID_OPEN,
+			&galaxybook_fwat_ops,
+			galaxybook_fwat_props,
+			ARRAY_SIZE(galaxybook_fwat_props)),
+	FWAT_CONFIG_AUX("usb_charging", boolean,
+			GB_ATTR_USB_CHARGING,
+			&galaxybook_fwat_ops,
+			galaxybook_fwat_props,
+			ARRAY_SIZE(galaxybook_fwat_props)),
+	FWAT_CONFIG_AUX("block_recording", boolean,
+			GB_ATTR_BLOCK_RECORDING,
+			&galaxybook_fwat_ops,
+			galaxybook_fwat_props,
+			ARRAY_SIZE(galaxybook_fwat_props)),
+	NULL
+};
+
+static const struct fwat_dev_config galaxybook_fwat_dev_config = {
+	.attrs_config = galaxybook_fwat_config,
+	.is_visible = galaxybook_fwat_is_visible,
+};
+
+static int galaxybook_fwat_init(struct samsung_galaxybook *galaxybook)
 {
-	struct galaxybook_fw_attr *fw_attr;
-	struct attribute **attrs;
-
-	fw_attr = devm_kzalloc(&galaxybook->platform->dev, sizeof(*fw_attr), GFP_KERNEL);
-	if (!fw_attr)
-		return -ENOMEM;
-
-	attrs = devm_kcalloc(&galaxybook->platform->dev, NUM_FW_ATTR_ENUM_ATTRS + 1,
-			     sizeof(*attrs), GFP_KERNEL);
-	if (!attrs)
-		return -ENOMEM;
-
-	attrs[0] = &fw_attr_type.attr;
-	attrs[1] = &fw_attr_default_value.attr;
-	attrs[2] = &fw_attr_possible_values.attr;
-	attrs[3] = &fw_attr_display_name_language_code.attr;
-
-	sysfs_attr_init(&fw_attr->display_name.attr);
-	fw_attr->display_name.attr.name = "display_name";
-	fw_attr->display_name.attr.mode = 0444;
-	fw_attr->display_name.show = display_name_show;
-	attrs[4] = &fw_attr->display_name.attr;
-
-	sysfs_attr_init(&fw_attr->current_value.attr);
-	fw_attr->current_value.attr.name = "current_value";
-	fw_attr->current_value.attr.mode = 0644;
-	fw_attr->current_value.show = current_value_show;
-	fw_attr->current_value.store = current_value_store;
-	attrs[5] = &fw_attr->current_value.attr;
-
-	attrs[6] = NULL;
-
-	fw_attr->galaxybook = galaxybook;
-	fw_attr->fw_attr_id = fw_attr_id;
-	fw_attr->attr_group.name = galaxybook_fw_attr_name[fw_attr_id];
-	fw_attr->attr_group.attrs = attrs;
-	fw_attr->get_value = get_value;
-	fw_attr->set_value = set_value;
-
-	return sysfs_create_group(&galaxybook->fw_attrs_kset->kobj, &fw_attr->attr_group);
-}
-
-static void galaxybook_kset_unregister(void *data)
-{
-	struct kset *kset = data;
-
-	kset_unregister(kset);
-}
-
-static void galaxybook_fw_attrs_dev_unregister(void *data)
-{
-	struct device *fw_attrs_dev = data;
-
-	device_unregister(fw_attrs_dev);
-}
-
-static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
-{
-	bool value;
+	struct fwat_device *fwdev;
 	int err;
 
 	err = devm_mutex_init(&galaxybook->platform->dev, &galaxybook->fw_attr_lock);
 	if (err)
 		return err;
 
-	galaxybook->fw_attrs_dev = device_create(&firmware_attributes_class, NULL, MKDEV(0, 0),
-						 NULL, "%s", DRIVER_NAME);
-	if (IS_ERR(galaxybook->fw_attrs_dev))
-		return PTR_ERR(galaxybook->fw_attrs_dev);
-
-	err = devm_add_action_or_reset(&galaxybook->platform->dev,
-				       galaxybook_fw_attrs_dev_unregister,
-				       galaxybook->fw_attrs_dev);
-	if (err)
-		return err;
-
-	galaxybook->fw_attrs_kset = kset_create_and_add("attributes", NULL,
-							&galaxybook->fw_attrs_dev->kobj);
-	if (!galaxybook->fw_attrs_kset)
-		return -ENOMEM;
-	err = devm_add_action_or_reset(&galaxybook->platform->dev,
-				       galaxybook_kset_unregister, galaxybook->fw_attrs_kset);
-	if (err)
-		return err;
-
-	err = power_on_lid_open_acpi_get(galaxybook, &value);
-	if (!err) {
-		err = galaxybook_fw_attr_init(galaxybook,
-					      GB_ATTR_POWER_ON_LID_OPEN,
-					      &power_on_lid_open_acpi_get,
-					      &power_on_lid_open_acpi_set);
-		if (err)
-			return err;
-	}
-
-	err = usb_charging_acpi_get(galaxybook, &value);
-	if (!err) {
-		err = galaxybook_fw_attr_init(galaxybook,
-					      GB_ATTR_USB_CHARGING,
-					      &usb_charging_acpi_get,
-					      &usb_charging_acpi_set);
-		if (err)
-			return err;
-	}
-
 	err = galaxybook_block_recording_init(galaxybook);
-	if (err == GB_NOT_SUPPORTED)
-		return 0;
-	else if (err)
+	if (!err)
+		galaxybook->has_block_recording = true;
+	else if (err != GB_NOT_SUPPORTED)
 		return err;
 
-	galaxybook->has_block_recording = true;
+	fwdev = devm_fwat_device_register(&galaxybook->platform->dev,
+					  "samsung-galaxybook", galaxybook,
+					  &galaxybook_fwat_dev_config, NULL);
 
-	return galaxybook_fw_attr_init(galaxybook,
-				       GB_ATTR_BLOCK_RECORDING,
-				       &block_recording_acpi_get,
-				       &block_recording_acpi_set);
+	return PTR_ERR_OR_ZERO(fwdev);
 }
 
 /*
@@ -1389,7 +1318,7 @@ static int galaxybook_probe(struct platform_device *pdev)
 		return dev_err_probe(&galaxybook->platform->dev, err,
 				     "failed to initialize kbd_backlight\n");
 
-	err = galaxybook_fw_attrs_init(galaxybook);
+	err = galaxybook_fwat_init(galaxybook);
 	if (err)
 		return dev_err_probe(&galaxybook->platform->dev, err,
 				     "failed to initialize firmware-attributes\n");

-- 
2.49.0

Re: [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API

[Mario Limonciello]

On 5/9/2025 2:48 AM, Kurt Borja wrote:
> Add an attribute configuration mechanism through the newly introduced
> `struct fwat_dev_config`, which makes use of other documented structs
> and callbacks.
> 
> This API aims to be simple, yet flexible. In order to accomplish this,
> the following features were taken into account:
> 
> * Ability to statically define attributes
> * Custom read/write callbacks for each attribute type
> * Ability to map attributes to numbers in order to differentiate them in
>    callbacks (`aux` number)
> * Ability to reuse read/write callbacks in different attributes
> * Ability to reuse property selection in different attributes
> * Optional visibility callback for dynamic attribute visibility
> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>   drivers/platform/x86/firmware_attributes_class.c | 249 ++++++++++++++++++++++-
>   drivers/platform/x86/firmware_attributes_class.h | 228 +++++++++++++++++++++
>   2 files changed, 474 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
> index 58ab1495ba3bd449cfe17de2827a57a0c5937788..7cfb0f49f235728c7450a82a7e9d00b8963d3dea 100644
> --- a/drivers/platform/x86/firmware_attributes_class.c
> +++ b/drivers/platform/x86/firmware_attributes_class.c
> @@ -7,14 +7,233 @@
>   #include <linux/kobject.h>
>   #include <linux/module.h>
>   #include <linux/slab.h>
> +#include <linux/sysfs.h>
>   #include <linux/types.h>
>   #include "firmware_attributes_class.h"
>   
> +#define to_fwat_attribute_ext(_a) container_of_const(_a, struct fwat_attribute_ext, attr)
> +
> +struct fwat_attribute_ext {
> +	struct fwat_attribute attr;
> +	enum fwat_property prop;
> +	const struct fwat_attr_config *config;
> +};
> +
>   const struct class firmware_attributes_class = {
>   	.name = "firmware-attributes",
>   };
>   EXPORT_SYMBOL_GPL(firmware_attributes_class);
>   
> +static const char * const fwat_type_labels[] = {
> +	[fwat_type_integer]		= "integer",
> +	[fwat_type_string]		= "string",
> +	[fwat_type_enumeration]		= "enumeration",
> +};
> +
> +static const char * const fwat_prop_labels[] = {
> +	[FWAT_PROP_DISPLAY_NAME]		= "display_name",
> +	[FWAT_PROP_LANGUAGE_CODE]		= "display_name_language_code",
> +	[FWAT_PROP_DEFAULT]			= "default",
> +
> +	[FWAT_INT_PROP_MIN]			= "min_value",
> +	[FWAT_INT_PROP_MAX]			= "max_value",
> +	[FWAT_INT_PROP_INCREMENT]		= "scalar_increment",
> +
> +	[FWAT_STR_PROP_MIN]			= "min_length",
> +	[FWAT_STR_PROP_MAX]			= "max_length",
> +
> +	[FWAT_ENUM_PROP_POSSIBLE_VALUES]	= "possible_values",
> +};
> +
> +static ssize_t
> +fwat_type_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
> +{
> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
> +	const struct fwat_attr_config *config = ext->config;
> +
> +	return sysfs_emit(buf, "%s\n", fwat_type_labels[config->type]);
> +}
> +
> +static ssize_t
> +fwat_property_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
> +{
> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
> +	const struct fwat_attr_config *config = ext->config;
> +
> +	if (!config->ops->prop_read)
> +		return -EOPNOTSUPP;
> +
> +	return config->ops->prop_read(dev, config->aux, ext->prop, buf);
> +}
> +
> +static ssize_t
> +fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
> +{
> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
> +	const struct fwat_attr_config *config = ext->config;
> +	const char *str;
> +	long int_val;
> +	int ret;
> +
> +	switch (config->type) {
> +	case fwat_type_integer:
> +		ret = config->ops->integer_read(dev, config->aux, &int_val);
> +		if (ret)
> +			return ret;
> +
> +		return sysfs_emit(buf, "%ld\n", int_val);
> +	case fwat_type_string:
> +		ret = config->ops->string_read(dev, config->aux, &str);
> +		if (ret)
> +			return ret;
> +
> +		return sysfs_emit(buf, "%s\n", str);
> +	case fwat_type_enumeration:
> +		ret = config->ops->enumeration_read(dev, config->aux, &str);
> +		if (ret)
> +			return ret;
> +
> +		return sysfs_emit(buf, "%s\n", str);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static ssize_t
> +fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
> +	const struct fwat_attr_config *config = ext->config;
> +	long int_val;
> +	int ret;
> +
> +	switch (config->type) {
> +	case fwat_type_integer:
> +		ret = kstrtol(buf, 0, &int_val);
> +		if (ret)
> +			return ret;
> +
> +		ret = config->ops->integer_write(dev, config->aux, int_val);
> +		break;
> +	case fwat_type_string:
> +		ret = config->ops->string_write(dev, config->aux, buf);
> +		break;
> +	case fwat_type_enumeration:
> +		ret = config->ops->enumeration_write(dev, config->aux, buf);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ret ? ret : count;
> +}
> +
> +static struct attribute *
> +fwat_alloc_attr(struct device *dev, const struct fwat_attr_config *config,
> +		const char *attr_name, umode_t mode, enum fwat_property prop,
> +		ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
> +				char *buf),
> +		ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
> +				 const char *buf, size_t count))
> +{
> +	struct fwat_attribute_ext *fattr;
> +
> +	fattr = devm_kzalloc(dev, sizeof(*fattr), GFP_KERNEL);
> +	if (!fattr)
> +		return NULL;
> +
> +	fattr->attr.attr.name = attr_name;
> +	fattr->attr.attr.mode = mode;
> +	fattr->attr.show = show;
> +	fattr->attr.store = store;
> +	fattr->prop = prop;
> +	fattr->config = config;
> +	sysfs_attr_init(&fattr->attr.attr);
> +
> +	return &fattr->attr.attr;
> +}
> +
> +static struct attribute **
> +fwat_create_attrs(struct device *dev, const struct fwat_attr_config *config)
> +{
> +	struct attribute **attrs;
> +	enum fwat_property prop;
> +	unsigned int index = 0;
> +
> +	attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL);
> +	if (!attrs)
> +		return NULL;
> +
> +	/*
> +	 * Create optional attributes
> +	 */
Just a nit here; this probably doesn't need to be a multiline comment. 
a single line like this is fine:

/* optional attributes */

> +	for (; index < config->num_props; index++) {
> +		prop = config->props[index];
> +		attrs[index] = fwat_alloc_attr(dev, config, fwat_prop_labels[prop],
> +					       0444, prop, fwat_property_show, NULL);
> +	}
> +
> +	/*
> +	 * Create mandatory attributes
> +	 */

Same as above

> +	attrs[index++] = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL);
> +	attrs[index++] = fwat_alloc_attr(dev, config, "current_value", 0644, 0,

Is this permission right?  Some attributes could be considered more 
sensitive can't they?

> +					 fwat_current_value_show, fwat_current_value_store);
> +
> +	return attrs;
> +}
> +
> +static const struct attribute_group *
> +fwat_create_group(struct device *dev, const struct fwat_attr_config *config)
> +{
> +	struct attribute_group *group;
> +	struct attribute **attrs;
> +
> +	group = devm_kzalloc(dev, sizeof(*group), GFP_KERNEL);
> +	if (!group)
> +		return NULL;
> +
> +	attrs = fwat_create_attrs(dev, config);
> +	if (!attrs)
> +		return NULL;
> +
> +	group->name = config->name;
> +	group->attrs = attrs;
> +
> +	return group;
> +}
> +
> +static const struct attribute_group **
> +fwat_create_auto_groups(struct device *dev, const struct fwat_dev_config *config)
> +{
> +	const struct attribute_group **groups;
> +	const struct attribute_group *grp;
> +	unsigned int index = 0;
> +	size_t ngroups = 0;
> +
> +	while (config->attrs_config[ngroups])
> +		ngroups++;
> +
> +	groups = devm_kcalloc(dev, ngroups + 1, sizeof(*groups), GFP_KERNEL);
> +	if (!groups)
> +		return NULL;
> +
> +	for (unsigned int i = 0; i < ngroups; i++) {
> +		if (config->is_visible &&
> +		    !config->is_visible(dev, config->attrs_config[i]))
> +			continue;
> +
> +		grp = fwat_create_group(dev, config->attrs_config[i]);
> +		if (!grp)
> +			return NULL;
> +
> +		groups[index++] = grp;
> +	}
> +
> +	return groups;
> +}
> +
>   static ssize_t fwat_attrs_kobj_show(struct kobject *kobj, struct attribute *attr,
>   				    char *buf)
>   {
> @@ -61,6 +280,7 @@ static const struct kobj_type fwat_attrs_ktype = {
>    *			  device
>    * @parent: Parent device
>    * @name: Name of the class device
> + * @config: Device configuration
>    * @data: Drvdata of the class device
>    * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
>    *
> @@ -72,8 +292,10 @@ static const struct kobj_type fwat_attrs_ktype = {
>    */
>   struct fwat_device *
>   fwat_device_register(struct device *parent, const char *name, void *data,
> +		     const struct fwat_dev_config *config,
>   		     const struct attribute_group **groups)
>   {
> +	const struct attribute_group **auto_groups;
>   	struct fwat_device *fadev;
>   	struct device *dev;
>   	int ret;
> @@ -97,19 +319,36 @@ fwat_device_register(struct device *parent, const char *name, void *data,
>   	if (ret)
>   		goto out_kobj_put;
>   
> -	if (groups) {
> -		ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
> +	if (config) {
> +		auto_groups = fwat_create_auto_groups(dev, config);
> +		if (!auto_groups) {
> +			ret = -ENOMEM;
> +			goto out_kobj_unregister;
> +		}
> +
> +		ret = sysfs_create_groups(&fadev->attrs_kobj, auto_groups);
>   		if (ret)
>   			goto out_kobj_unregister;
>   	}
>   
> +	if (groups) {
> +		ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
> +		if (ret)
> +			goto out_remove_auto_groups;
> +	}
> +
>   	fadev->dev = dev;
>   	fadev->groups = groups;
> +	fadev->auto_groups = groups;
>   
>   	kobject_uevent(&fadev->attrs_kobj, KOBJ_ADD);
>   
>   	return fadev;
>   
> +out_remove_auto_groups:
> +	if (config)
> +		sysfs_remove_groups(&fadev->attrs_kobj, auto_groups);
> +
>   out_kobj_unregister:
>   	kobject_del(&fadev->attrs_kobj);
>   
> @@ -125,6 +364,8 @@ void fwat_device_unregister(struct fwat_device *fwadev)
>   {
>   	if (fwadev->groups)
>   		sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->groups);
> +	if (fwadev->auto_groups)
> +		sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->auto_groups);
>   	kobject_del(&fwadev->attrs_kobj);
>   	kobject_put(&fwadev->attrs_kobj);
>   	device_unregister(fwadev->dev);
> @@ -143,6 +384,7 @@ static void devm_fwat_device_release(void *data)
>    *			       device
>    * @parent: Parent device
>    * @name: Name of the class device
> + * @config: Device configuration
>    * @data: Drvdata of the class device
>    * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
>    *
> @@ -156,12 +398,13 @@ static void devm_fwat_device_release(void *data)
>    */
>   struct fwat_device *
>   devm_fwat_device_register(struct device *parent, const char *name, void *data,
> +			  const struct fwat_dev_config *config,
>   			  const struct attribute_group **groups)
>   {
>   	struct fwat_device *fadev;
>   	int ret;
>   
> -	fadev = fwat_device_register(parent, name, data, groups);
> +	fadev = fwat_device_register(parent, name, data, config, groups);
>   	if (IS_ERR(fadev))
>   		return fadev;
>   
> diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
> index ad94bf91e5af30a2b8feb9abf224ee6f0d17600a..f250875d785c3439682c43c693f98e1c20e9ff54 100644
> --- a/drivers/platform/x86/firmware_attributes_class.h
> +++ b/drivers/platform/x86/firmware_attributes_class.h
> @@ -18,11 +18,14 @@ extern const struct class firmware_attributes_class;
>    * @dev: The class device.
>    * @attrs_kobj: The "attributes" root kobject.
>    * @groups: Sysfs groups attached to the @attrs_kobj.
> + * @auto_groups: Sysgs groups generated from &struct fwat_attr_config attached.
> + * to the @attrs_kobj
>    */
>   struct fwat_device {
>   	struct device *dev;
>   	struct kobject attrs_kobj;
>   	const struct attribute_group **groups;
> +	const struct attribute_group **auto_groups;
>   };
>   
>   #define to_fwat_device(_k)	container_of_const(_k, struct fwat_device, attrs_kobj)
> @@ -30,6 +33,7 @@ struct fwat_device {
>   /**
>    * struct fwat_attribute - The firmware-attributes's custom attribute
>    * @attr: Embedded struct attribute.
> + * @aux: Auxiliary number defined by the user.
>    * @show: Show method called by the "attributes" kobject's ktype.
>    * @store: Store method called by the "attributes" kobject's ktype.
>    */
> @@ -43,14 +47,238 @@ struct fwat_attribute {
>   
>   #define to_fwat_attribute(_a) container_of_const(_a, struct fwat_attribute, attr)
>   
> +enum fwat_attr_type {
> +	fwat_type_integer,
> +	fwat_type_string,
> +	fwat_type_enumeration,
> +};
> +
> +enum fwat_property {
> +	FWAT_PROP_DISPLAY_NAME,
> +	FWAT_PROP_LANGUAGE_CODE,
> +	FWAT_PROP_DEFAULT,
> +
> +	FWAT_INT_PROP_MIN,
> +	FWAT_INT_PROP_MAX,
> +	FWAT_INT_PROP_INCREMENT,
> +
> +	FWAT_STR_PROP_MIN,
> +	FWAT_STR_PROP_MAX,
> +
> +	FWAT_ENUM_PROP_POSSIBLE_VALUES,
> +};
> +
> +struct fwat_attr_config;
> +
> +/**
> + * struct fwat_attr_ops - Operations for a firmware *attribute*
> + * @prop_read: Callback for retrieving each configured property of an attribute.
> + * @integer_read: Callback for reading the current_value of an attribute of
> + *                type *integer*.
> + * @integer_write: Callback for writing the current_value of an attribute of
> + *                 type *integer*.
> + * @string_read: Callback for reading the current_value of an attribute of type
> + *               *string*.
> + * @string_write: Callback for writing the current_value of an attribute of type
> + *                *string*.
> + * @enumeration_read: Callback for reading the current_value of an attribute of
> + *                    type *enumeration*.
> + * @enumeration_write: Callback for writing the current_value of an attribute
> + *                     of type *enumeration*.
> + */
> +struct fwat_attr_ops {
> +	ssize_t (*prop_read)(struct device *dev, long aux,
> +			     enum fwat_property prop, const char *buf);
> +	union {
> +		struct {
> +			int (*integer_read)(struct device *dev, long aux,
> +					    long *val);
> +			int (*integer_write)(struct device *dev, long aux,
> +					     long val);
> +		};
> +		struct {
> +			int (*string_read)(struct device *dev, long aux,
> +					   const char **str);
> +			int (*string_write)(struct device *dev, long aux,
> +					    const char *str);
> +		};
> +		struct {
> +			int (*enumeration_read)(struct device *dev, long aux,
> +						const char **str);
> +			int (*enumeration_write)(struct device *dev, long aux,
> +						 const char *str);
> +		};
> +	};
> +};
> +
> +/**
> + * struct fwat_attr_config - Configuration for a single firmware *attribute*
> + * @name: Name of the sysfs group associated with this *attribute*.
> + * @type: Type of this *attribute*.
> + * @aux: Auxiliary number defined by the user, which will be passed to
> + *       read/write callbacks.
> + * @ops: Operations for this *attribute*.
> + * @props: Array of properties of this *attribute*.
> + * @num_props: Size of the props array.
> + */
> +struct fwat_attr_config {
> +	const char *name;
> +	enum fwat_attr_type type;
> +	long aux;
> +	const struct fwat_attr_ops *ops;
> +	const enum fwat_property *props;
> +	size_t num_props;
> +};
> +
> +/**
> + * DEFINE_SIMPLE_FWAT_OPS() - Define static &struct fwat_attr_ops for a simple
> + *                            *attribute* with no properties, i.e. No
> + *                            &fwat_attr_ops.read_prop callback
> + * @_name: Prefix of the `read` and `write` callbacks.
> + * @_type: Firmware *attribute* type.
> + *
> + * Example:
> + *
> + * static int example_read(...) {...}
> + * static int example_write(...) {...}
> + *
> + * DEFINE_SIMPLE_FWAT_OPS(example, ...);
> + */
> +#define DEFINE_SIMPLE_FWAT_OPS(_name, _type)		\
> +	static const struct fwat_attr_ops _name##_ops = { \
> +		._type##_read = _name##_read,		\
> +		._type##_write = _name##_write,		\
> +	}
> +
> +/**
> + * DEFINE_FWAT_OPS() - Define static &struct fwat_attr_ops with all callbacks.
> + * @_name: Prefix of the `read` and `write` callbacks.
> + * @_type: Firmware *attribute* type.
> + *
> + * Example:
> + *
> + * static int example_read(...) {...}
> + * static int example_write(...) {...}
> + * static int example_prop_read(...) {...}
> + *
> + * DEFINE_FWAT_OPS(example, ...);
> + */
> +#define DEFINE_FWAT_OPS(_name, _type)			\
> +	static const struct fwat_attr_ops _name##_ops = { \
> +		.prop_read = _name##_prop_read,		\
> +		._type##_read = _name##_read,		\
> +		._type##_write = _name##_write,		\
> +	}
> +
> +/**
> + * FWAT_CONFIG() - Configuration pointer for a single firmware *attribute*.
> + * @_name: String name of this *attribute*.
> + * @_type: Firmware *attribute* type.
> + * @_ops: Pointer to &struct fwat_attr_ops.
> + * @_props: Pointer to a enum fwat_property array.
> + * @_num_props: Size of the @_props array.
> + *
> + * This is a convenience macro to quickly construct a &struct fwat_attr_config
> + * array, which will be passed to &struct fwat_dev_config.
> + *
> + * Example:
> + *
> + * static int example_read(...) {...}
> + * static int example_write(...) {...}
> + * static int example_prop_read(...) {...}
> + *
> + * DEFINE_FWAT_OPS(example, ...);
> + *
> + * static const enum fwat_property props[] = {...};
> + *
> + * static const struct fwat_attr_config * const attrs_config[] = {
> + *	FWAT_CONFIG(example, ..., &example_fwat_ops, props,
> + *		    ARRAY_SIZE(props)),
> + *	...
> + *	NULL
> + * };
> + *
> + * static const struct fwat_dev_config fdev_config = {
> + *	.attrs_config = attrs_config,
> + *	...
> + * }
> + */
> +#define FWAT_CONFIG(_name, _type, _ops, _props, _num_props) \
> +	(&(const struct fwat_attr_config) {	\
> +		.name = _name,				\
> +		.type = fwat_type_##_type,		\
> +		.ops = _ops,				\
> +		.num_props = _num_props,		\
> +		.props = _props,			\
> +	})
> +
> +/**
> + * FWAT_CONFIG_AUX() - Configuration pointer for a single firmware *attribute*
> + *                     with an auxiliary number defined by the user
> + * @_name: String name of this *attribute*.
> + * @_type: Firmware *attribute* type.
> + * @_aux: Auxiliary number defined by the user.
> + * @_ops: Pointer to &struct fwat_attr_ops.
> + * @_props: Pointer to a enum fwat_property array.
> + * @_num_props: Size of the @_props array.
> + *
> + * This is a convenience macro to quickly construct a &struct fwat_attr_config
> + * array, which will be passed to &struct fwat_dev_config.
> + *
> + * Example:
> + *
> + * static int example_read(...) {...}
> + * static int example_write(...) {...}
> + * static int example_prop_read(...) {...}
> + *
> + * DEFINE_FWAT_OPS(example, ...);
> + *
> + * static const enum fwat_property props[] = {...};
> + *
> + * static const struct fwat_attr_config * const config[] = {
> + *	FWAT_CONFIG_AUX(example, ..., n, &example_fwat_ops, props,
> + *			ARRAY_SIZE(props)),
> + *	...
> + *	NULL
> + * };
> + *
> + * static const struct fwat_dev_config fdev_config = {
> + *	.attrs_config = attrs_config,
> + *	...
> + * }
> + */
> +#define FWAT_CONFIG_AUX(_name, _type, _aux, _ops, _props, _num_props) \
> +	(&(const struct fwat_attr_config) {		\
> +		.name = _name,				\
> +		.type = fwat_type_##_type,		\
> +		.aux = _aux,				\
> +		.ops = _ops,				\
> +		.num_props = _num_props,		\
> +		.props = _props,			\
> +	})
> +
> +/**
> + * struct fwat_dev_config - Configuration for this devices's
> + *                          &fwat_device.auto_groups
> + * @attrs_config: NULL terminated &struct fwat_attr_config array.
> + * @is_visible: Optional visibility callback to determine the visibility
> + *              of each auto_group.
> + */
> +struct fwat_dev_config {
> +	const struct fwat_attr_config *const *attrs_config;
> +	bool (*is_visible)(struct device *dev, const struct fwat_attr_config *config);
> +};
> +
>   struct fwat_device * __must_check
>   fwat_device_register(struct device *parent, const char *name, void *data,
> +		     const struct fwat_dev_config *config,
>   		     const struct attribute_group **groups);
>   
>   void fwat_device_unregister(struct fwat_device *fwadev);
>   
>   struct fwat_device * __must_check
>   devm_fwat_device_register(struct device *parent, const char *name, void *data,
> +			  const struct fwat_dev_config *config,
>   			  const struct attribute_group **groups);
>   
>   #endif /* FW_ATTR_CLASS_H */
> 

Re: [PATCH RFC 1/5] platform/x86: firmware_attributes_class: Add device initialization methods

[Mario Limonciello]

On 5/9/2025 2:48 AM, Kurt Borja wrote:
> From: Thomas Weißschuh <linux@weissschuh.net>
> 
> Currently each user of firmware_attributes_class has to manually set up
> kobjects, devices, etc.
> 
> Provide this infrastructure out-of-the-box through the newly introduced
> fwat_device_register().
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> Co-developed-by: Kurt Borja <kuurtb@gmail.com>
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/platform/x86/firmware_attributes_class.c | 165 +++++++++++++++++++++++
>   drivers/platform/x86/firmware_attributes_class.h |  44 ++++++
>   2 files changed, 209 insertions(+)
> 
> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
> index 736e96c186d9dc6d945517f090e9af903e93bbf4..58ab1495ba3bd449cfe17de2827a57a0c5937788 100644
> --- a/drivers/platform/x86/firmware_attributes_class.c
> +++ b/drivers/platform/x86/firmware_attributes_class.c
> @@ -2,7 +2,12 @@
>   
>   /* Firmware attributes class helper module */
>   
> +#include <linux/device.h>
> +#include <linux/device/class.h>
> +#include <linux/kobject.h>
>   #include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
>   #include "firmware_attributes_class.h"
>   
>   const struct class firmware_attributes_class = {
> @@ -10,6 +15,164 @@ const struct class firmware_attributes_class = {
>   };
>   EXPORT_SYMBOL_GPL(firmware_attributes_class);
>   
> +static ssize_t fwat_attrs_kobj_show(struct kobject *kobj, struct attribute *attr,
> +				    char *buf)
> +{
> +	const struct fwat_attribute *fattr = to_fwat_attribute(attr);
> +	struct fwat_device *fadev = to_fwat_device(kobj);
> +
> +	if (!fattr->show)
> +		return -ENOENT;
> +
> +	return fattr->show(fadev->dev, fattr, buf);
> +}
> +
> +static ssize_t fwat_attrs_kobj_store(struct kobject *kobj, struct attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	const struct fwat_attribute *fattr = to_fwat_attribute(attr);
> +	struct fwat_device *fadev = to_fwat_device(kobj);
> +
> +	if (!fattr->store)
> +		return -ENOENT;
> +
> +	return fattr->store(fadev->dev, fattr, buf, count);
> +}
> +
> +static const struct sysfs_ops fwat_attrs_kobj_ops = {
> +	.show	= fwat_attrs_kobj_show,
> +	.store	= fwat_attrs_kobj_store,
> +};
> +
> +static void fwat_attrs_kobj_release(struct kobject *kobj)
> +{
> +	struct fwat_device *fadev = to_fwat_device(kobj);
> +
> +	kfree(fadev);
> +}
> +
> +static const struct kobj_type fwat_attrs_ktype = {
> +	.sysfs_ops	= &fwat_attrs_kobj_ops,
> +	.release	= fwat_attrs_kobj_release,
> +};
> +
> +/**
> + * fwat_device_register - Create and register a firmware-attributes class
> + *			  device
> + * @parent: Parent device
> + * @name: Name of the class device
> + * @data: Drvdata of the class device
> + * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
> + *
> + * NOTE: @groups are attached to the .attrs_kobj of the new fwat_device which
> + * has a custom ktype, which makes use of `struct fwat_attribute` to embed
> + * attributes.
> + *
> + * Return: pointer to the new fwat_device on success, ERR_PTR on failure
> + */
> +struct fwat_device *
> +fwat_device_register(struct device *parent, const char *name, void *data,
> +		     const struct attribute_group **groups)
> +{
> +	struct fwat_device *fadev;
> +	struct device *dev;
> +	int ret;
> +
> +	if (!parent || !name)
> +		return ERR_PTR(-EINVAL);
> +
> +	fadev = kzalloc(sizeof(*fadev), GFP_KERNEL);
> +	if (!fadev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dev = device_create(&firmware_attributes_class, parent, MKDEV(0, 0),
> +			    data, "%s", name);
> +	if (IS_ERR(dev)) {
> +		kfree(fadev);
> +		return ERR_CAST(dev);
> +	}
> +
> +	ret = kobject_init_and_add(&fadev->attrs_kobj, &fwat_attrs_ktype, &dev->kobj,
> +				   "attributes");
> +	if (ret)
> +		goto out_kobj_put;
> +
> +	if (groups) {
> +		ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
> +		if (ret)
> +			goto out_kobj_unregister;
> +	}
> +
> +	fadev->dev = dev;
> +	fadev->groups = groups;
> +
> +	kobject_uevent(&fadev->attrs_kobj, KOBJ_ADD);
> +
> +	return fadev;
> +
> +out_kobj_unregister:
> +	kobject_del(&fadev->attrs_kobj);
> +
> +out_kobj_put:
> +	kobject_put(&fadev->attrs_kobj);
> +	device_unregister(dev);
> +
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(fwat_device_register);
> +
> +void fwat_device_unregister(struct fwat_device *fwadev)
> +{
> +	if (fwadev->groups)
> +		sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->groups);
> +	kobject_del(&fwadev->attrs_kobj);
> +	kobject_put(&fwadev->attrs_kobj);
> +	device_unregister(fwadev->dev);
> +}
> +EXPORT_SYMBOL_GPL(fwat_device_unregister);
> +
> +static void devm_fwat_device_release(void *data)
> +{
> +	struct fwat_device *fadev = data;
> +
> +	fwat_device_unregister(fadev);
> +}
> +
> +/**
> + * devm_fwat_device_register - Create and register a firmware-attributes class
> + *			       device
> + * @parent: Parent device
> + * @name: Name of the class device
> + * @data: Drvdata of the class device
> + * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
> + *
> + * Device managed version of fwat_device_register().
> + *
> + * NOTE: @groups are attached to the .attrs_kobj of the new fwat_device which
> + * has a custom ktype, which makes use of `struct fwat_attribute` to embed
> + * attributes.
> + *
> + * Return: pointer to the new fwat_device on success, ERR_PTR on failure
> + */
> +struct fwat_device *
> +devm_fwat_device_register(struct device *parent, const char *name, void *data,
> +			  const struct attribute_group **groups)
> +{
> +	struct fwat_device *fadev;
> +	int ret;
> +
> +	fadev = fwat_device_register(parent, name, data, groups);
> +	if (IS_ERR(fadev))
> +		return fadev;
> +
> +	ret = devm_add_action_or_reset(parent, devm_fwat_device_release, fadev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return fadev;
> +}
> +EXPORT_SYMBOL_GPL(devm_fwat_device_register);
> +
>   static __init int fw_attributes_class_init(void)
>   {
>   	return class_register(&firmware_attributes_class);
> @@ -23,5 +186,7 @@ static __exit void fw_attributes_class_exit(void)
>   module_exit(fw_attributes_class_exit);
>   
>   MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>");
> +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net>");
> +MODULE_AUTHOR("Kurt Borja <kuurtb@gmail.com>");
>   MODULE_DESCRIPTION("Firmware attributes class helper module");
>   MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
> index d27abe54fcf9812a2f0868eec5426bbc8e7eb21c..ad94bf91e5af30a2b8feb9abf224ee6f0d17600a 100644
> --- a/drivers/platform/x86/firmware_attributes_class.h
> +++ b/drivers/platform/x86/firmware_attributes_class.h
> @@ -5,8 +5,52 @@
>   #ifndef FW_ATTR_CLASS_H
>   #define FW_ATTR_CLASS_H
>   
> +#include <linux/container_of.h>
> +#include <linux/device.h>
>   #include <linux/device/class.h>
> +#include <linux/kobject.h>
> +#include <linux/sysfs.h>
>   
>   extern const struct class firmware_attributes_class;
>   
> +/**
> + * struct fwat_device - The firmware-attributes device
> + * @dev: The class device.
> + * @attrs_kobj: The "attributes" root kobject.
> + * @groups: Sysfs groups attached to the @attrs_kobj.
> + */
> +struct fwat_device {
> +	struct device *dev;
> +	struct kobject attrs_kobj;
> +	const struct attribute_group **groups;
> +};
> +
> +#define to_fwat_device(_k)	container_of_const(_k, struct fwat_device, attrs_kobj)
> +
> +/**
> + * struct fwat_attribute - The firmware-attributes's custom attribute
> + * @attr: Embedded struct attribute.
> + * @show: Show method called by the "attributes" kobject's ktype.
> + * @store: Store method called by the "attributes" kobject's ktype.
> + */
> +struct fwat_attribute {
> +	struct attribute attr;
> +	ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
> +			char *buf);
> +	ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
> +			 const char *buf, size_t count);
> +};
> +
> +#define to_fwat_attribute(_a) container_of_const(_a, struct fwat_attribute, attr)
> +
> +struct fwat_device * __must_check
> +fwat_device_register(struct device *parent, const char *name, void *data,
> +		     const struct attribute_group **groups);
> +
> +void fwat_device_unregister(struct fwat_device *fwadev);
> +
> +struct fwat_device * __must_check
> +devm_fwat_device_register(struct device *parent, const char *name, void *data,
> +			  const struct attribute_group **groups);
> +
>   #endif /* FW_ATTR_CLASS_H */
> 

Re: [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API

[Kurt Borja]

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

Hi Mario,

On Fri May 9, 2025 at 12:58 PM -03, Mario Limonciello wrote:
> On 5/9/2025 2:48 AM, Kurt Borja wrote:
>> Add an attribute configuration mechanism through the newly introduced
>> `struct fwat_dev_config`, which makes use of other documented structs
>> and callbacks.
>> 
>> This API aims to be simple, yet flexible. In order to accomplish this,
>> the following features were taken into account:
>> 
>> * Ability to statically define attributes
>> * Custom read/write callbacks for each attribute type
>> * Ability to map attributes to numbers in order to differentiate them in
>>    callbacks (`aux` number)
>> * Ability to reuse read/write callbacks in different attributes
>> * Ability to reuse property selection in different attributes
>> * Optional visibility callback for dynamic attribute visibility
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>   drivers/platform/x86/firmware_attributes_class.c | 249 ++++++++++++++++++++++-
>>   drivers/platform/x86/firmware_attributes_class.h | 228 +++++++++++++++++++++
>>   2 files changed, 474 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
>> index 58ab1495ba3bd449cfe17de2827a57a0c5937788..7cfb0f49f235728c7450a82a7e9d00b8963d3dea 100644
>> --- a/drivers/platform/x86/firmware_attributes_class.c
>> +++ b/drivers/platform/x86/firmware_attributes_class.c
>> @@ -7,14 +7,233 @@
>>   #include <linux/kobject.h>
>>   #include <linux/module.h>
>>   #include <linux/slab.h>
>> +#include <linux/sysfs.h>
>>   #include <linux/types.h>
>>   #include "firmware_attributes_class.h"
>>   
>> +#define to_fwat_attribute_ext(_a) container_of_const(_a, struct fwat_attribute_ext, attr)
>> +
>> +struct fwat_attribute_ext {
>> +	struct fwat_attribute attr;
>> +	enum fwat_property prop;
>> +	const struct fwat_attr_config *config;
>> +};
>> +
>>   const struct class firmware_attributes_class = {
>>   	.name = "firmware-attributes",
>>   };
>>   EXPORT_SYMBOL_GPL(firmware_attributes_class);
>>   
>> +static const char * const fwat_type_labels[] = {
>> +	[fwat_type_integer]		= "integer",
>> +	[fwat_type_string]		= "string",
>> +	[fwat_type_enumeration]		= "enumeration",
>> +};
>> +
>> +static const char * const fwat_prop_labels[] = {
>> +	[FWAT_PROP_DISPLAY_NAME]		= "display_name",
>> +	[FWAT_PROP_LANGUAGE_CODE]		= "display_name_language_code",
>> +	[FWAT_PROP_DEFAULT]			= "default",
>> +
>> +	[FWAT_INT_PROP_MIN]			= "min_value",
>> +	[FWAT_INT_PROP_MAX]			= "max_value",
>> +	[FWAT_INT_PROP_INCREMENT]		= "scalar_increment",
>> +
>> +	[FWAT_STR_PROP_MIN]			= "min_length",
>> +	[FWAT_STR_PROP_MAX]			= "max_length",
>> +
>> +	[FWAT_ENUM_PROP_POSSIBLE_VALUES]	= "possible_values",
>> +};
>> +
>> +static ssize_t
>> +fwat_type_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>> +{
>> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>> +	const struct fwat_attr_config *config = ext->config;
>> +
>> +	return sysfs_emit(buf, "%s\n", fwat_type_labels[config->type]);
>> +}
>> +
>> +static ssize_t
>> +fwat_property_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>> +{
>> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>> +	const struct fwat_attr_config *config = ext->config;
>> +
>> +	if (!config->ops->prop_read)
>> +		return -EOPNOTSUPP;
>> +
>> +	return config->ops->prop_read(dev, config->aux, ext->prop, buf);
>> +}
>> +
>> +static ssize_t
>> +fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>> +{
>> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>> +	const struct fwat_attr_config *config = ext->config;
>> +	const char *str;
>> +	long int_val;
>> +	int ret;
>> +
>> +	switch (config->type) {
>> +	case fwat_type_integer:
>> +		ret = config->ops->integer_read(dev, config->aux, &int_val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return sysfs_emit(buf, "%ld\n", int_val);
>> +	case fwat_type_string:
>> +		ret = config->ops->string_read(dev, config->aux, &str);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return sysfs_emit(buf, "%s\n", str);
>> +	case fwat_type_enumeration:
>> +		ret = config->ops->enumeration_read(dev, config->aux, &str);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return sysfs_emit(buf, "%s\n", str);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static ssize_t
>> +fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
>> +			 const char *buf, size_t count)
>> +{
>> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>> +	const struct fwat_attr_config *config = ext->config;
>> +	long int_val;
>> +	int ret;
>> +
>> +	switch (config->type) {
>> +	case fwat_type_integer:
>> +		ret = kstrtol(buf, 0, &int_val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = config->ops->integer_write(dev, config->aux, int_val);
>> +		break;
>> +	case fwat_type_string:
>> +		ret = config->ops->string_write(dev, config->aux, buf);
>> +		break;
>> +	case fwat_type_enumeration:
>> +		ret = config->ops->enumeration_write(dev, config->aux, buf);
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return ret ? ret : count;
>> +}
>> +
>> +static struct attribute *
>> +fwat_alloc_attr(struct device *dev, const struct fwat_attr_config *config,
>> +		const char *attr_name, umode_t mode, enum fwat_property prop,
>> +		ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
>> +				char *buf),
>> +		ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
>> +				 const char *buf, size_t count))
>> +{
>> +	struct fwat_attribute_ext *fattr;
>> +
>> +	fattr = devm_kzalloc(dev, sizeof(*fattr), GFP_KERNEL);
>> +	if (!fattr)
>> +		return NULL;
>> +
>> +	fattr->attr.attr.name = attr_name;
>> +	fattr->attr.attr.mode = mode;
>> +	fattr->attr.show = show;
>> +	fattr->attr.store = store;
>> +	fattr->prop = prop;
>> +	fattr->config = config;
>> +	sysfs_attr_init(&fattr->attr.attr);
>> +
>> +	return &fattr->attr.attr;
>> +}
>> +
>> +static struct attribute **
>> +fwat_create_attrs(struct device *dev, const struct fwat_attr_config *config)
>> +{
>> +	struct attribute **attrs;
>> +	enum fwat_property prop;
>> +	unsigned int index = 0;
>> +
>> +	attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL);
>> +	if (!attrs)
>> +		return NULL;
>> +
>> +	/*
>> +	 * Create optional attributes
>> +	 */
> Just a nit here; this probably doesn't need to be a multiline comment. 
> a single line like this is fine:
>
> /* optional attributes */
>
>> +	for (; index < config->num_props; index++) {
>> +		prop = config->props[index];
>> +		attrs[index] = fwat_alloc_attr(dev, config, fwat_prop_labels[prop],
>> +					       0444, prop, fwat_property_show, NULL);
>> +	}
>> +
>> +	/*
>> +	 * Create mandatory attributes
>> +	 */
>
> Same as above

Ack for these two.

>
>> +	attrs[index++] = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL);
>> +	attrs[index++] = fwat_alloc_attr(dev, config, "current_value", 0644, 0,
>
> Is this permission right?  Some attributes could be considered more 
> sensitive can't they?

You are right. I assumed most drivers would want 0644 but think-lmi and
dell sysman use 0600.

I can add a mode_t mask to fwat_attr_config and use that for
current_value, while other properties would be masked by it, i.e.
`0444 & config->mode`

What do you think?

-- 
 ~ Kurt

>
>> +					 fwat_current_value_show, fwat_current_value_store);
>> +
>> +	return attrs;
>> +}
>> +
>> +static const struct attribute_group *
>> +fwat_create_group(struct device *dev, const struct fwat_attr_config *config)
>> +{
>> +	struct attribute_group *group;
>> +	struct attribute **attrs;
>> +
>> +	group = devm_kzalloc(dev, sizeof(*group), GFP_KERNEL);
>> +	if (!group)
>> +		return NULL;
>> +
>> +	attrs = fwat_create_attrs(dev, config);
>> +	if (!attrs)
>> +		return NULL;
>> +
>> +	group->name = config->name;
>> +	group->attrs = attrs;
>> +
>> +	return group;
>> +}
>> +
>> +static const struct attribute_group **
>> +fwat_create_auto_groups(struct device *dev, const struct fwat_dev_config *config)
>> +{
>> +	const struct attribute_group **groups;
>> +	const struct attribute_group *grp;
>> +	unsigned int index = 0;
>> +	size_t ngroups = 0;
>> +
>> +	while (config->attrs_config[ngroups])
>> +		ngroups++;
>> +
>> +	groups = devm_kcalloc(dev, ngroups + 1, sizeof(*groups), GFP_KERNEL);
>> +	if (!groups)
>> +		return NULL;
>> +
>> +	for (unsigned int i = 0; i < ngroups; i++) {
>> +		if (config->is_visible &&
>> +		    !config->is_visible(dev, config->attrs_config[i]))
>> +			continue;
>> +
>> +		grp = fwat_create_group(dev, config->attrs_config[i]);
>> +		if (!grp)
>> +			return NULL;
>> +
>> +		groups[index++] = grp;
>> +	}
>> +
>> +	return groups;
>> +}
>> +
>>   static ssize_t fwat_attrs_kobj_show(struct kobject *kobj, struct attribute *attr,
>>   				    char *buf)
>>   {
>> @@ -61,6 +280,7 @@ static const struct kobj_type fwat_attrs_ktype = {
>>    *			  device
>>    * @parent: Parent device
>>    * @name: Name of the class device
>> + * @config: Device configuration
>>    * @data: Drvdata of the class device
>>    * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
>>    *
>> @@ -72,8 +292,10 @@ static const struct kobj_type fwat_attrs_ktype = {
>>    */
>>   struct fwat_device *
>>   fwat_device_register(struct device *parent, const char *name, void *data,
>> +		     const struct fwat_dev_config *config,
>>   		     const struct attribute_group **groups)
>>   {
>> +	const struct attribute_group **auto_groups;
>>   	struct fwat_device *fadev;
>>   	struct device *dev;
>>   	int ret;
>> @@ -97,19 +319,36 @@ fwat_device_register(struct device *parent, const char *name, void *data,
>>   	if (ret)
>>   		goto out_kobj_put;
>>   
>> -	if (groups) {
>> -		ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
>> +	if (config) {
>> +		auto_groups = fwat_create_auto_groups(dev, config);
>> +		if (!auto_groups) {
>> +			ret = -ENOMEM;
>> +			goto out_kobj_unregister;
>> +		}
>> +
>> +		ret = sysfs_create_groups(&fadev->attrs_kobj, auto_groups);
>>   		if (ret)
>>   			goto out_kobj_unregister;
>>   	}
>>   
>> +	if (groups) {
>> +		ret = sysfs_create_groups(&fadev->attrs_kobj, groups);
>> +		if (ret)
>> +			goto out_remove_auto_groups;
>> +	}
>> +
>>   	fadev->dev = dev;
>>   	fadev->groups = groups;
>> +	fadev->auto_groups = groups;
>>   
>>   	kobject_uevent(&fadev->attrs_kobj, KOBJ_ADD);
>>   
>>   	return fadev;
>>   
>> +out_remove_auto_groups:
>> +	if (config)
>> +		sysfs_remove_groups(&fadev->attrs_kobj, auto_groups);
>> +
>>   out_kobj_unregister:
>>   	kobject_del(&fadev->attrs_kobj);
>>   
>> @@ -125,6 +364,8 @@ void fwat_device_unregister(struct fwat_device *fwadev)
>>   {
>>   	if (fwadev->groups)
>>   		sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->groups);
>> +	if (fwadev->auto_groups)
>> +		sysfs_remove_groups(&fwadev->attrs_kobj, fwadev->auto_groups);
>>   	kobject_del(&fwadev->attrs_kobj);
>>   	kobject_put(&fwadev->attrs_kobj);
>>   	device_unregister(fwadev->dev);
>> @@ -143,6 +384,7 @@ static void devm_fwat_device_release(void *data)
>>    *			       device
>>    * @parent: Parent device
>>    * @name: Name of the class device
>> + * @config: Device configuration
>>    * @data: Drvdata of the class device
>>    * @groups: Sysfs groups for the custom `fwat_attrs_ktype` kobj_type
>>    *
>> @@ -156,12 +398,13 @@ static void devm_fwat_device_release(void *data)
>>    */
>>   struct fwat_device *
>>   devm_fwat_device_register(struct device *parent, const char *name, void *data,
>> +			  const struct fwat_dev_config *config,
>>   			  const struct attribute_group **groups)
>>   {
>>   	struct fwat_device *fadev;
>>   	int ret;
>>   
>> -	fadev = fwat_device_register(parent, name, data, groups);
>> +	fadev = fwat_device_register(parent, name, data, config, groups);
>>   	if (IS_ERR(fadev))
>>   		return fadev;
>>   
>> diff --git a/drivers/platform/x86/firmware_attributes_class.h b/drivers/platform/x86/firmware_attributes_class.h
>> index ad94bf91e5af30a2b8feb9abf224ee6f0d17600a..f250875d785c3439682c43c693f98e1c20e9ff54 100644
>> --- a/drivers/platform/x86/firmware_attributes_class.h
>> +++ b/drivers/platform/x86/firmware_attributes_class.h
>> @@ -18,11 +18,14 @@ extern const struct class firmware_attributes_class;
>>    * @dev: The class device.
>>    * @attrs_kobj: The "attributes" root kobject.
>>    * @groups: Sysfs groups attached to the @attrs_kobj.
>> + * @auto_groups: Sysgs groups generated from &struct fwat_attr_config attached.
>> + * to the @attrs_kobj
>>    */
>>   struct fwat_device {
>>   	struct device *dev;
>>   	struct kobject attrs_kobj;
>>   	const struct attribute_group **groups;
>> +	const struct attribute_group **auto_groups;
>>   };
>>   
>>   #define to_fwat_device(_k)	container_of_const(_k, struct fwat_device, attrs_kobj)
>> @@ -30,6 +33,7 @@ struct fwat_device {
>>   /**
>>    * struct fwat_attribute - The firmware-attributes's custom attribute
>>    * @attr: Embedded struct attribute.
>> + * @aux: Auxiliary number defined by the user.
>>    * @show: Show method called by the "attributes" kobject's ktype.
>>    * @store: Store method called by the "attributes" kobject's ktype.
>>    */
>> @@ -43,14 +47,238 @@ struct fwat_attribute {
>>   
>>   #define to_fwat_attribute(_a) container_of_const(_a, struct fwat_attribute, attr)
>>   
>> +enum fwat_attr_type {
>> +	fwat_type_integer,
>> +	fwat_type_string,
>> +	fwat_type_enumeration,
>> +};
>> +
>> +enum fwat_property {
>> +	FWAT_PROP_DISPLAY_NAME,
>> +	FWAT_PROP_LANGUAGE_CODE,
>> +	FWAT_PROP_DEFAULT,
>> +
>> +	FWAT_INT_PROP_MIN,
>> +	FWAT_INT_PROP_MAX,
>> +	FWAT_INT_PROP_INCREMENT,
>> +
>> +	FWAT_STR_PROP_MIN,
>> +	FWAT_STR_PROP_MAX,
>> +
>> +	FWAT_ENUM_PROP_POSSIBLE_VALUES,
>> +};
>> +
>> +struct fwat_attr_config;
>> +
>> +/**
>> + * struct fwat_attr_ops - Operations for a firmware *attribute*
>> + * @prop_read: Callback for retrieving each configured property of an attribute.
>> + * @integer_read: Callback for reading the current_value of an attribute of
>> + *                type *integer*.
>> + * @integer_write: Callback for writing the current_value of an attribute of
>> + *                 type *integer*.
>> + * @string_read: Callback for reading the current_value of an attribute of type
>> + *               *string*.
>> + * @string_write: Callback for writing the current_value of an attribute of type
>> + *                *string*.
>> + * @enumeration_read: Callback for reading the current_value of an attribute of
>> + *                    type *enumeration*.
>> + * @enumeration_write: Callback for writing the current_value of an attribute
>> + *                     of type *enumeration*.
>> + */
>> +struct fwat_attr_ops {
>> +	ssize_t (*prop_read)(struct device *dev, long aux,
>> +			     enum fwat_property prop, const char *buf);
>> +	union {
>> +		struct {
>> +			int (*integer_read)(struct device *dev, long aux,
>> +					    long *val);
>> +			int (*integer_write)(struct device *dev, long aux,
>> +					     long val);
>> +		};
>> +		struct {
>> +			int (*string_read)(struct device *dev, long aux,
>> +					   const char **str);
>> +			int (*string_write)(struct device *dev, long aux,
>> +					    const char *str);
>> +		};
>> +		struct {
>> +			int (*enumeration_read)(struct device *dev, long aux,
>> +						const char **str);
>> +			int (*enumeration_write)(struct device *dev, long aux,
>> +						 const char *str);
>> +		};
>> +	};
>> +};
>> +
>> +/**
>> + * struct fwat_attr_config - Configuration for a single firmware *attribute*
>> + * @name: Name of the sysfs group associated with this *attribute*.
>> + * @type: Type of this *attribute*.
>> + * @aux: Auxiliary number defined by the user, which will be passed to
>> + *       read/write callbacks.
>> + * @ops: Operations for this *attribute*.
>> + * @props: Array of properties of this *attribute*.
>> + * @num_props: Size of the props array.
>> + */
>> +struct fwat_attr_config {
>> +	const char *name;
>> +	enum fwat_attr_type type;
>> +	long aux;
>> +	const struct fwat_attr_ops *ops;
>> +	const enum fwat_property *props;
>> +	size_t num_props;
>> +};
>> +
>> +/**
>> + * DEFINE_SIMPLE_FWAT_OPS() - Define static &struct fwat_attr_ops for a simple
>> + *                            *attribute* with no properties, i.e. No
>> + *                            &fwat_attr_ops.read_prop callback
>> + * @_name: Prefix of the `read` and `write` callbacks.
>> + * @_type: Firmware *attribute* type.
>> + *
>> + * Example:
>> + *
>> + * static int example_read(...) {...}
>> + * static int example_write(...) {...}
>> + *
>> + * DEFINE_SIMPLE_FWAT_OPS(example, ...);
>> + */
>> +#define DEFINE_SIMPLE_FWAT_OPS(_name, _type)		\
>> +	static const struct fwat_attr_ops _name##_ops = { \
>> +		._type##_read = _name##_read,		\
>> +		._type##_write = _name##_write,		\
>> +	}
>> +
>> +/**
>> + * DEFINE_FWAT_OPS() - Define static &struct fwat_attr_ops with all callbacks.
>> + * @_name: Prefix of the `read` and `write` callbacks.
>> + * @_type: Firmware *attribute* type.
>> + *
>> + * Example:
>> + *
>> + * static int example_read(...) {...}
>> + * static int example_write(...) {...}
>> + * static int example_prop_read(...) {...}
>> + *
>> + * DEFINE_FWAT_OPS(example, ...);
>> + */
>> +#define DEFINE_FWAT_OPS(_name, _type)			\
>> +	static const struct fwat_attr_ops _name##_ops = { \
>> +		.prop_read = _name##_prop_read,		\
>> +		._type##_read = _name##_read,		\
>> +		._type##_write = _name##_write,		\
>> +	}
>> +
>> +/**
>> + * FWAT_CONFIG() - Configuration pointer for a single firmware *attribute*.
>> + * @_name: String name of this *attribute*.
>> + * @_type: Firmware *attribute* type.
>> + * @_ops: Pointer to &struct fwat_attr_ops.
>> + * @_props: Pointer to a enum fwat_property array.
>> + * @_num_props: Size of the @_props array.
>> + *
>> + * This is a convenience macro to quickly construct a &struct fwat_attr_config
>> + * array, which will be passed to &struct fwat_dev_config.
>> + *
>> + * Example:
>> + *
>> + * static int example_read(...) {...}
>> + * static int example_write(...) {...}
>> + * static int example_prop_read(...) {...}
>> + *
>> + * DEFINE_FWAT_OPS(example, ...);
>> + *
>> + * static const enum fwat_property props[] = {...};
>> + *
>> + * static const struct fwat_attr_config * const attrs_config[] = {
>> + *	FWAT_CONFIG(example, ..., &example_fwat_ops, props,
>> + *		    ARRAY_SIZE(props)),
>> + *	...
>> + *	NULL
>> + * };
>> + *
>> + * static const struct fwat_dev_config fdev_config = {
>> + *	.attrs_config = attrs_config,
>> + *	...
>> + * }
>> + */
>> +#define FWAT_CONFIG(_name, _type, _ops, _props, _num_props) \
>> +	(&(const struct fwat_attr_config) {	\
>> +		.name = _name,				\
>> +		.type = fwat_type_##_type,		\
>> +		.ops = _ops,				\
>> +		.num_props = _num_props,		\
>> +		.props = _props,			\
>> +	})
>> +
>> +/**
>> + * FWAT_CONFIG_AUX() - Configuration pointer for a single firmware *attribute*
>> + *                     with an auxiliary number defined by the user
>> + * @_name: String name of this *attribute*.
>> + * @_type: Firmware *attribute* type.
>> + * @_aux: Auxiliary number defined by the user.
>> + * @_ops: Pointer to &struct fwat_attr_ops.
>> + * @_props: Pointer to a enum fwat_property array.
>> + * @_num_props: Size of the @_props array.
>> + *
>> + * This is a convenience macro to quickly construct a &struct fwat_attr_config
>> + * array, which will be passed to &struct fwat_dev_config.
>> + *
>> + * Example:
>> + *
>> + * static int example_read(...) {...}
>> + * static int example_write(...) {...}
>> + * static int example_prop_read(...) {...}
>> + *
>> + * DEFINE_FWAT_OPS(example, ...);
>> + *
>> + * static const enum fwat_property props[] = {...};
>> + *
>> + * static const struct fwat_attr_config * const config[] = {
>> + *	FWAT_CONFIG_AUX(example, ..., n, &example_fwat_ops, props,
>> + *			ARRAY_SIZE(props)),
>> + *	...
>> + *	NULL
>> + * };
>> + *
>> + * static const struct fwat_dev_config fdev_config = {
>> + *	.attrs_config = attrs_config,
>> + *	...
>> + * }
>> + */
>> +#define FWAT_CONFIG_AUX(_name, _type, _aux, _ops, _props, _num_props) \
>> +	(&(const struct fwat_attr_config) {		\
>> +		.name = _name,				\
>> +		.type = fwat_type_##_type,		\
>> +		.aux = _aux,				\
>> +		.ops = _ops,				\
>> +		.num_props = _num_props,		\
>> +		.props = _props,			\
>> +	})
>> +
>> +/**
>> + * struct fwat_dev_config - Configuration for this devices's
>> + *                          &fwat_device.auto_groups
>> + * @attrs_config: NULL terminated &struct fwat_attr_config array.
>> + * @is_visible: Optional visibility callback to determine the visibility
>> + *              of each auto_group.
>> + */
>> +struct fwat_dev_config {
>> +	const struct fwat_attr_config *const *attrs_config;
>> +	bool (*is_visible)(struct device *dev, const struct fwat_attr_config *config);
>> +};
>> +
>>   struct fwat_device * __must_check
>>   fwat_device_register(struct device *parent, const char *name, void *data,
>> +		     const struct fwat_dev_config *config,
>>   		     const struct attribute_group **groups);
>>   
>>   void fwat_device_unregister(struct fwat_device *fwadev);
>>   
>>   struct fwat_device * __must_check
>>   devm_fwat_device_register(struct device *parent, const char *name, void *data,
>> +			  const struct fwat_dev_config *config,
>>   			  const struct attribute_group **groups);
>>   
>>   #endif /* FW_ATTR_CLASS_H */
>> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a high level API

[Mario Limonciello]

On 5/9/2025 2:56 PM, Kurt Borja wrote:
> Hi Mario,
> 
> On Fri May 9, 2025 at 12:58 PM -03, Mario Limonciello wrote:
>> On 5/9/2025 2:48 AM, Kurt Borja wrote:
>>> Add an attribute configuration mechanism through the newly introduced
>>> `struct fwat_dev_config`, which makes use of other documented structs
>>> and callbacks.
>>>
>>> This API aims to be simple, yet flexible. In order to accomplish this,
>>> the following features were taken into account:
>>>
>>> * Ability to statically define attributes
>>> * Custom read/write callbacks for each attribute type
>>> * Ability to map attributes to numbers in order to differentiate them in
>>>     callbacks (`aux` number)
>>> * Ability to reuse read/write callbacks in different attributes
>>> * Ability to reuse property selection in different attributes
>>> * Optional visibility callback for dynamic attribute visibility
>>>
>>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>>> ---
>>>    drivers/platform/x86/firmware_attributes_class.c | 249 ++++++++++++++++++++++-
>>>    drivers/platform/x86/firmware_attributes_class.h | 228 +++++++++++++++++++++
>>>    2 files changed, 474 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
>>> index 58ab1495ba3bd449cfe17de2827a57a0c5937788..7cfb0f49f235728c7450a82a7e9d00b8963d3dea 100644
>>> --- a/drivers/platform/x86/firmware_attributes_class.c
>>> +++ b/drivers/platform/x86/firmware_attributes_class.c
>>> @@ -7,14 +7,233 @@
>>>    #include <linux/kobject.h>
>>>    #include <linux/module.h>
>>>    #include <linux/slab.h>
>>> +#include <linux/sysfs.h>
>>>    #include <linux/types.h>
>>>    #include "firmware_attributes_class.h"
>>>    
>>> +#define to_fwat_attribute_ext(_a) container_of_const(_a, struct fwat_attribute_ext, attr)
>>> +
>>> +struct fwat_attribute_ext {
>>> +	struct fwat_attribute attr;
>>> +	enum fwat_property prop;
>>> +	const struct fwat_attr_config *config;
>>> +};
>>> +
>>>    const struct class firmware_attributes_class = {
>>>    	.name = "firmware-attributes",
>>>    };
>>>    EXPORT_SYMBOL_GPL(firmware_attributes_class);
>>>    
>>> +static const char * const fwat_type_labels[] = {
>>> +	[fwat_type_integer]		= "integer",
>>> +	[fwat_type_string]		= "string",
>>> +	[fwat_type_enumeration]		= "enumeration",
>>> +};
>>> +
>>> +static const char * const fwat_prop_labels[] = {
>>> +	[FWAT_PROP_DISPLAY_NAME]		= "display_name",
>>> +	[FWAT_PROP_LANGUAGE_CODE]		= "display_name_language_code",
>>> +	[FWAT_PROP_DEFAULT]			= "default",
>>> +
>>> +	[FWAT_INT_PROP_MIN]			= "min_value",
>>> +	[FWAT_INT_PROP_MAX]			= "max_value",
>>> +	[FWAT_INT_PROP_INCREMENT]		= "scalar_increment",
>>> +
>>> +	[FWAT_STR_PROP_MIN]			= "min_length",
>>> +	[FWAT_STR_PROP_MAX]			= "max_length",
>>> +
>>> +	[FWAT_ENUM_PROP_POSSIBLE_VALUES]	= "possible_values",
>>> +};
>>> +
>>> +static ssize_t
>>> +fwat_type_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>>> +{
>>> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> +	const struct fwat_attr_config *config = ext->config;
>>> +
>>> +	return sysfs_emit(buf, "%s\n", fwat_type_labels[config->type]);
>>> +}
>>> +
>>> +static ssize_t
>>> +fwat_property_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>>> +{
>>> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> +	const struct fwat_attr_config *config = ext->config;
>>> +
>>> +	if (!config->ops->prop_read)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	return config->ops->prop_read(dev, config->aux, ext->prop, buf);
>>> +}
>>> +
>>> +static ssize_t
>>> +fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>>> +{
>>> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> +	const struct fwat_attr_config *config = ext->config;
>>> +	const char *str;
>>> +	long int_val;
>>> +	int ret;
>>> +
>>> +	switch (config->type) {
>>> +	case fwat_type_integer:
>>> +		ret = config->ops->integer_read(dev, config->aux, &int_val);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		return sysfs_emit(buf, "%ld\n", int_val);
>>> +	case fwat_type_string:
>>> +		ret = config->ops->string_read(dev, config->aux, &str);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		return sysfs_emit(buf, "%s\n", str);
>>> +	case fwat_type_enumeration:
>>> +		ret = config->ops->enumeration_read(dev, config->aux, &str);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		return sysfs_emit(buf, "%s\n", str);
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +}
>>> +
>>> +static ssize_t
>>> +fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
>>> +			 const char *buf, size_t count)
>>> +{
>>> +	const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> +	const struct fwat_attr_config *config = ext->config;
>>> +	long int_val;
>>> +	int ret;
>>> +
>>> +	switch (config->type) {
>>> +	case fwat_type_integer:
>>> +		ret = kstrtol(buf, 0, &int_val);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		ret = config->ops->integer_write(dev, config->aux, int_val);
>>> +		break;
>>> +	case fwat_type_string:
>>> +		ret = config->ops->string_write(dev, config->aux, buf);
>>> +		break;
>>> +	case fwat_type_enumeration:
>>> +		ret = config->ops->enumeration_write(dev, config->aux, buf);
>>> +		break;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +
>>> +	return ret ? ret : count;
>>> +}
>>> +
>>> +static struct attribute *
>>> +fwat_alloc_attr(struct device *dev, const struct fwat_attr_config *config,
>>> +		const char *attr_name, umode_t mode, enum fwat_property prop,
>>> +		ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
>>> +				char *buf),
>>> +		ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
>>> +				 const char *buf, size_t count))
>>> +{
>>> +	struct fwat_attribute_ext *fattr;
>>> +
>>> +	fattr = devm_kzalloc(dev, sizeof(*fattr), GFP_KERNEL);
>>> +	if (!fattr)
>>> +		return NULL;
>>> +
>>> +	fattr->attr.attr.name = attr_name;
>>> +	fattr->attr.attr.mode = mode;
>>> +	fattr->attr.show = show;
>>> +	fattr->attr.store = store;
>>> +	fattr->prop = prop;
>>> +	fattr->config = config;
>>> +	sysfs_attr_init(&fattr->attr.attr);
>>> +
>>> +	return &fattr->attr.attr;
>>> +}
>>> +
>>> +static struct attribute **
>>> +fwat_create_attrs(struct device *dev, const struct fwat_attr_config *config)
>>> +{
>>> +	struct attribute **attrs;
>>> +	enum fwat_property prop;
>>> +	unsigned int index = 0;
>>> +
>>> +	attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL);
>>> +	if (!attrs)
>>> +		return NULL;
>>> +
>>> +	/*
>>> +	 * Create optional attributes
>>> +	 */
>> Just a nit here; this probably doesn't need to be a multiline comment.
>> a single line like this is fine:
>>
>> /* optional attributes */
>>
>>> +	for (; index < config->num_props; index++) {
>>> +		prop = config->props[index];
>>> +		attrs[index] = fwat_alloc_attr(dev, config, fwat_prop_labels[prop],
>>> +					       0444, prop, fwat_property_show, NULL);
>>> +	}
>>> +
>>> +	/*
>>> +	 * Create mandatory attributes
>>> +	 */
>>
>> Same as above
> 
> Ack for these two.
> 
>>
>>> +	attrs[index++] = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL);
>>> +	attrs[index++] = fwat_alloc_attr(dev, config, "current_value", 0644, 0,
>>
>> Is this permission right?  Some attributes could be considered more
>> sensitive can't they?
> 
> You are right. I assumed most drivers would want 0644 but think-lmi and
> dell sysman use 0600.
> 
> I can add a mode_t mask to fwat_attr_config and use that for
> current_value, while other properties would be masked by it, i.e.
> `0444 & config->mode`
> 
> What do you think?

Yeah; current_value is the only sensitive one.

Re: [PATCH RFC 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API

[Thomas Weißschuh]

On 2025-05-09 04:48:37-0300, Kurt Borja wrote:
> Transition to new firmware_attributes API and replace `enumeration` types
> with the simpler `boolean` type.

This is an ABI change, using a newly introduced ABI.
Some elaboration why it does not break userspace would be good.

> 
> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>  drivers/platform/x86/samsung-galaxybook.c | 299 ++++++++++++------------------
>  1 file changed, 114 insertions(+), 185 deletions(-)

<snip>

Re: [PATCH RFC 5/5] platform/x86: samsung-galaxybook: Transition to new firmware_attributes API

[Kurt Borja]

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

Hi Thomas,

On Mon May 12, 2025 at 4:12 AM -03, Thomas Weißschuh wrote:
> On 2025-05-09 04:48:37-0300, Kurt Borja wrote:
>> Transition to new firmware_attributes API and replace `enumeration` types
>> with the simpler `boolean` type.
>
> This is an ABI change, using a newly introduced ABI.
> Some elaboration why it does not break userspace would be good.

I thought it was not a big deal since the driver was still new. But TIL
fwupd actually interfaces with the firmware-attributes class.

I won't break the ABI of this driver in the next revision.

-- 
 ~ Kurt

>
>> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>  drivers/platform/x86/samsung-galaxybook.c | 299 ++++++++++++------------------
>>  1 file changed, 114 insertions(+), 185 deletions(-)
>
> <snip>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]