Re: [PATCH v2 2/6] platform/x86: asus-armoury: move existing tunings to asus-armoury module

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

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

On Sun, 11 Aug 2024, Luke Jones wrote:

> On Tue, 6 Aug 2024, at 10:16 PM, Ilpo Järvinen wrote:
> > On Tue, 6 Aug 2024, Luke D. Jones wrote:
> > 
> > > 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>

> > > +static ssize_t attr_int_store(struct kobject *kobj, struct kobj_attribute *attr,
> > > +       const char *buf, size_t count, u32 min, u32 max, u32 *store_value,
> > > +       u32 wmi_dev);
> > > +
> > > +static ssize_t int_type_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > 
> > inline missing
> > 
> > > +{
> > > + return sysfs_emit(buf, "integer\n");
> > 
> > Lukas Wunner might have done something to make emitting constant strings 
> > easier, please check out if that's already in mainline.
> 
> I'm not sure what I'm looking for here. Searching my current pdx86 pull 
> isn't returning anything likely and I can't find anything on lore.

I think that was done mostly outside of pdx86, here it is:

https://lore.kernel.org/all/cover.1713608122.git.lukas@wunner.de/

So DEVICE_STRING_ATTR_RO() seems the way to go, I think.

> > > + * On success the return value is 0, and the retval is a valid value returned
> > > + * by the successful WMI function call. An error value is returned only if the
> > > + * WMI function failed, or if it returns "unsupported" which is typically a 0
> > > + * (no return, and no 'supported' bit set), or a 0xFFFFFFFE (~1) which if not
> > > + * caught here can result in unexpected behaviour later.
> > > + */
> > > +int asus_wmi_get_devstate_dsts(u32 dev_id, u32 *retval)
> > > +{
> > > + int err;
> > > +
> > > + err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS, dev_id, 0, retval);
> > > + if (err)
> > > + return err;
> > > + /* Be explicit about retval */
> > > + if (*retval == 0xFFFFFFFE || *retval == 0)
> > 
> > Please name the literals with defines.
> 
> Should have been ASUS_WMI_UNSUPPORTED_METHOD :)

Is one define enough if you have two ways to indicate "unsupported"?

> > > + return -ENODEV;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(asus_wmi_get_devstate_dsts);
> > > +
> > > +/**
> > > + * asus_wmi_set_devstate() - Set the WMI function state.
> > > + * @dev_id: The WMI function to call.
> > > + * @ctrl_param: The argument to be used for this WMI function.
> > > + * @retval: A pointer to where to store the value returned from WMI.
> > > + *
> > > + * The returned WMI function state if not checked here for error as
> > > + * asus_wmi_set_devstate() is not called unless first paired with a call to
> > > + * asus_wmi_get_devstate_dsts() to check that the WMI function is supported.
> > > + *
> > > + * On success the return value is 0, and the retval is a valid value returned
> > > + * by the successful WMI function call. An error value is returned only if the
> > > + * WMI function failed.
> > > + */
> > > +int asus_wmi_set_devstate(u32 dev_id, u32 ctrl_param, u32 *retval)
> > >  {
> > >  return asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS, dev_id,
> > >  ctrl_param, retval);
> > >  }
> > > +EXPORT_SYMBOL_GPL(asus_wmi_set_devstate);
> > 
> > Namespace exports.
> 
> I'm sorry, I don't understand.

Use EXPORT_SYMBOL_NS_GPL(). The driver using the functions, will then do 
MODULE_IMPORT_NS() to pair with the namespace defined by the 
EXPORT_SYMBOL_NS_GPL().

-- 
 i.