On Sat, 21 Sep 2024, at 7:13 PM, Christophe JAILLET wrote:
> Le 18/09/2024 à 11:42, Luke D. Jones a écrit :
> > The fw_attributes_class provides a much cleaner interface to all of the
> > attributes introduced to asus-wmi. This patch moves all of these extra
> > attributes over to fw_attributes_class, and shifts the bulk of these
> > definitions to a new kernel module to reduce the clutter of asus-wmi
> > with the intention of deprecating the asus-wmi attributes in future.
> >
> > The work applies only to WMI methods which don't have a clearly defined
> > place within the sysfs and as a result ended up lumped together in
> > /sys/devices/platform/asus-nb-wmi/ with no standard API.
> >
> > Where possible the fw attrs now implement defaults, min, max, scalar,
> > choices, etc. As en example dgpu_disable becomes:
> >
> > /sys/class/firmware-attributes/asus-armoury/attributes/dgpu_disable/
> > ├── current_value
> > ├── display_name
> > ├── possible_values
> > └── type
> >
> > as do other attributes.
> >
> > Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
>
> Hi,
>
> a few nitpick below, should it help.
every review is helpful, thank you :)
> > +/* Boolean style enumeration, base macro. Requires adding show/store */
> > +#define __ATTR_GROUP_ENUM(_attrname, _fsname, _possible, _dispname) \
> > + __ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname); \
> > + __ATTR_SHOW_FMT(possible_values, _attrname, "%s\n", _possible); \
> > + static struct kobj_attribute attr_##_attrname##_type = \
> > + __ASUS_ATTR_RO_AS(type, enum_type_show); \
> > + static struct attribute *_attrname##_attrs[] = { \
> > + &attr_##_attrname##_current_value.attr, \
> > + &attr_##_attrname##_display_name.attr, \
> > + &attr_##_attrname##_possible_values.attr, \
> > + &attr_##_attrname##_type.attr, NULL \
>
> It would be more readable if ", NULL" was on its own line, IMHO.
Ah yes, I didn't see this. Fixed, plus all following.
> > + }; \
> > + static const struct attribute_group _attrname##_attr_group = { \
> > + .name = _fsname, .attrs = _attrname##_attrs \
> > + }
> > +
> > +#define ATTR_GROUP_BOOL_RO(_attrname, _fsname, _wmi, _dispname) \
> > + __ATTR_CURRENT_INT_RO(_attrname, _wmi); \
> > + __ATTR_GROUP_ENUM(_attrname, _fsname, "0;1", _dispname)
> > +
> > +#define ATTR_GROUP_BOOL_RW(_attrname, _fsname, _wmi, _dispname) \
> > + __ATTR_CURRENT_INT_RW(_attrname, 0, 1, _wmi); \
> > + __ATTR_GROUP_ENUM(_attrname, _fsname, "0;1", _dispname)
> > +
> > +/*
> > + * Requires <name>_current_value_show(), <name>_current_value_show()
> > + */
> > +#define ATTR_GROUP_BOOL_CUSTOM(_attrname, _fsname, _dispname) \
> > + static struct kobj_attribute attr_##_attrname##_current_value = \
> > + __ASUS_ATTR_RW(_attrname, current_value); \
> > + __ATTR_GROUP_ENUM(_attrname, _fsname, "0;1", _dispname)
> > +
> > +#define ATTR_GROUP_ENUM_INT_RO(_attrname, _fsname, _wmi, _possible, _dispname) \
> > + __ATTR_CURRENT_INT_RO(_attrname, _wmi); \
> > + __ATTR_GROUP_ENUM(_attrname, _fsname, _possible, _dispname)
> > +
> > +/*
> > + * Requires <name>_current_value_show(), <name>_current_value_show()
> > + * and <name>_possible_values_show()
> > + */
> > +#define ATTR_GROUP_ENUM_CUSTOM(_attrname, _fsname, _dispname) \
> > + __ATTR_SHOW_FMT(display_name, _attrname, "%s\n", _dispname); \
> > + static struct kobj_attribute attr_##_attrname##_current_value = \
> > + __ASUS_ATTR_RW(_attrname, current_value); \
> > + static struct kobj_attribute attr_##_attrname##_possible_values = \
> > + __ASUS_ATTR_RO(_attrname, possible_values); \
> > + static struct kobj_attribute attr_##_attrname##_type = \
> > + __ASUS_ATTR_RO_AS(type, enum_type_show); \
> > + static struct attribute *_attrname##_attrs[] = { \
> > + &attr_##_attrname##_current_value.attr, \
> > + &attr_##_attrname##_display_name.attr, \
> > + &attr_##_attrname##_possible_values.attr, \
> > + &attr_##_attrname##_type.attr, NULL \
>
> It would be more readable if ", NULL" was on its own line, IMHO.
>
> > + }; \
> > + static const struct attribute_group _attrname##_attr_group = { \
> > + .name = _fsname, .attrs = _attrname##_attrs \
> > + }
>
> ...
>
> > +static const struct dmi_system_id asus_rog_ally_device[] = {
> > + {
> > + .matches = {
> > + DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
> > + },
> > + },
> > + {
> > + .matches = {
> > + DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
> > + },
> > + },
> > + { },
>
> I think that an ending comma is not expected after a terminator.
Never really been clear on this myself. A massive amount of other code does include the comma so that's what I followed. And then a massive amount of other code uses the opposite so I could follow that too.. but I work in most of the asus stuff and it looks like that's what is common there so I'll remove the comma.
Many thanks for taking the time to review.
> > +};
> > +
> > #endif /* !_ASUS_WMI_H_ */
>
> ...
>
> CJ
>
>
