Re: [PATCH v3 1/5] platform/x86: asus-armoury: move existing tunings to asus-armoury module

["Luke Jones" <luke@ljones.dev>]

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@ljones.dev>
> 
> 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
> 
>