Hi Luke,
On 7/16/24 7:16 AM, 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-bioscfg/attributes/dgpu_disable/
> ├── current_value
> ├── display_name
> ├── possible_values
> └── type
>
> as do other attributes.
>
> Signed-off-by: Luke D. Jones <luke@xxxxxxxxxx>
Interesting (also see my reply to the cover-letter).
Note this is not a full review, just a few small remarks
with things which I noticed while taking a quick look.
<snip>
> +static bool asus_bios_requires_reboot(struct kobj_attribute *attr)
> +{
> + return !strcmp(attr->attr.name, "gpu_mux_mode");
> + !strcmp(attr->attr.name, "panel_hd_mode");
> +}
The second statement here is never reached, I believe you want
a || between the 2 strcmp() calls:
static bool asus_bios_requires_reboot(struct kobj_attribute *attr)
{
return !strcmp(attr->attr.name, "gpu_mux_mode") ||
!strcmp(attr->attr.name, "panel_hd_mode");
}
<snip>
> +/* Simple attribute creation */
I think it would be good to do the following for registering
these "simple" attributes ... continued below.
> +ATTR_GROUP_ENUM_INT_RW(thermal_policy, "thermal_policy", ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
> + 0, 3, "0;1;2", "Set the thermal profile: 0=normal, 1=performance, 2=quiet");
> +ATTR_GROUP_PPT_RW(ppt_pl2_sppt, "ppt_pl2_sppt", ASUS_WMI_DEVID_PPT_PL2_SPPT,
> + cpu_default, 5, cpu_max, 1, "Set the CPU fast package limit");
> +ATTR_GROUP_PPT_RW(ppt_apu_sppt, "ppt_apu_sppt", ASUS_WMI_DEVID_PPT_APU_SPPT,
> + platform_default, 5, platform_max, 1, "Set the CPU slow package limit");
> +ATTR_GROUP_PPT_RW(ppt_platform_sppt, "ppt_platform_sppt", ASUS_WMI_DEVID_PPT_PLAT_SPPT,
> + platform_default, 5, platform_max, 1, "Set the CPU slow package limit");
> +ATTR_GROUP_PPT_RW(ppt_fppt, "ppt_fppt", ASUS_WMI_DEVID_PPT_FPPT,
> + cpu_default, 5, cpu_max, 1, "Set the CPU slow package limit");
> +
> +ATTR_GROUP_PPT_RW(nv_dynamic_boost, "nv_dynamic_boost", ASUS_WMI_DEVID_NV_DYN_BOOST,
> + nv_boost_default, 5, nv_boost_max, 1, "Set the Nvidia dynamic boost limit");
> +ATTR_GROUP_PPT_RW(nv_temp_target, "nv_temp_target", ASUS_WMI_DEVID_NV_THERM_TARGET,
> + nv_temp_default, 75, nv_temp_max, 1, "Set the Nvidia max thermal limit");
> +
> +ATTR_GROUP_ENUM_INT_RO(charge_mode, "charge_mode", ASUS_WMI_DEVID_CHARGE_MODE,
> + "0;1;2", "Show the current mode of charging");
> +ATTR_GROUP_BOOL_RW(boot_sound, "boot_sound", ASUS_WMI_DEVID_BOOT_SOUND,
> + "Set the boot POST sound");
> +ATTR_GROUP_BOOL_RW(mcu_powersave, "mcu_powersave", ASUS_WMI_DEVID_MCU_POWERSAVE,
> + "Set MCU powersaving mode");
> +ATTR_GROUP_BOOL_RW(panel_od, "panel_overdrive", ASUS_WMI_DEVID_PANEL_OD,
> + "Set the panel refresh overdrive");
> +ATTR_GROUP_BOOL_RW(panel_hd_mode, "panel_hd_mode", ASUS_WMI_DEVID_PANEL_HD,
> + "Set the panel HD mode to UHD<0> or FHD<1>");
> +ATTR_GROUP_BOOL_RO(egpu_connected, "egpu_connected", ASUS_WMI_DEVID_EGPU_CONNECTED,
> + "Show the eGPU connection status");
Create an array of simple attribute groups for this
entire set of simple attributes:
struct asus_attr_group {
const struct attribute_group *attr_group;
u32 wmi_devid;
};
static const struct asus_attr_group simple_attribute_groups[] = {
{ &thermal_policy_attr_group, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY },
{ &ppt_pl2_sppt_attr_group, ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY },
...
{ &egpu_connected_attr_group, ASUS_WMI_DEVID_EGPU_CONNECTED },
};
And then in asus_fw_attr_add() .. continued below:
> +static int asus_fw_attr_add(void)
> +{
> + int ret;
> +
> + ret = fw_attributes_class_get(&fw_attr_class);
> + if (ret)
> + goto fail_class_created;
> + else
> + asus_bioscfg.fw_attr_dev = device_create(fw_attr_class, NULL,
> + MKDEV(0, 0), NULL, "%s", DRIVER_NAME);
> +
> + if (IS_ERR(asus_bioscfg.fw_attr_dev)) {
> + ret = PTR_ERR(asus_bioscfg.fw_attr_dev);
> + goto fail_class_created;
> + }
> +
> + asus_bioscfg.fw_attr_kset = kset_create_and_add("attributes", NULL,
> + &asus_bioscfg.fw_attr_dev->kobj);
> + if (!asus_bioscfg.fw_attr_dev) {
> + ret = -ENOMEM;
> + pr_debug("Failed to create and add attributes\n");
> + goto err_destroy_classdev;
> + }
> +
> + /* Add any firmware_attributes required */
> + ret = sysfs_create_file(&asus_bioscfg.fw_attr_kset->kobj, &pending_reboot.attr);
> + if (ret) {
> + pr_warn("Failed to create sysfs level attributes\n");
> + goto fail_class_created;
> + }
> +
> + // TODO: logging
> + asus_bioscfg.mini_led_dev_id = 0;
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_MINI_LED_MODE)) {
> + asus_bioscfg.mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE;
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &mini_led_mode_attr_group);
> + } else if (asus_wmi_is_present(ASUS_WMI_DEVID_MINI_LED_MODE2)) {
> + asus_bioscfg.mini_led_dev_id = ASUS_WMI_DEVID_MINI_LED_MODE2;
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &mini_led_mode_attr_group);
> + }
> +
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_GPU_MUX)) {
> + asus_bioscfg.gpu_mux_dev_id = ASUS_WMI_DEVID_GPU_MUX;
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
> + } else if (asus_wmi_is_present(ASUS_WMI_DEVID_GPU_MUX_VIVO)) {
> + asus_bioscfg.gpu_mux_dev_id = ASUS_WMI_DEVID_GPU_MUX_VIVO;
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &gpu_mux_mode_attr_group);
> + }
> +
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_DGPU)) {
> + asus_bioscfg.dgpu_disable_available = true;
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &dgpu_disable_attr_group);
> + }
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_EGPU)) {
> + asus_bioscfg.egpu_enable_available = true;
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &egpu_enable_attr_group);
> + }
Replace the block starting here and ending ...
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_EGPU_CONNECTED))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &egpu_connected_attr_group);
> +
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &thermal_policy_attr_group);
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_PPT_PL1_SPL))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &ppt_pl1_spl_attr_group);
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_PPT_PL2_SPPT))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &ppt_pl2_sppt_attr_group);
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_PPT_APU_SPPT))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &ppt_apu_sppt_attr_group);
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_PPT_PLAT_SPPT))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &ppt_platform_sppt_attr_group);
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_PPT_FPPT))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &ppt_fppt_attr_group);
> +
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_NV_DYN_BOOST))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &nv_dynamic_boost_attr_group);
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_NV_THERM_TARGET))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &nv_temp_target_attr_group);
> +
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_CHARGE_MODE))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &charge_mode_attr_group);
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_BOOT_SOUND))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &boot_sound_attr_group);
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_MCU_POWERSAVE))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &mcu_powersave_attr_group);
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_PANEL_OD))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &panel_od_attr_group);
> + if (asus_wmi_is_present(ASUS_WMI_DEVID_PANEL_HD))
> + sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, &panel_hd_mode_attr_group);
here, with:
for (i = 0; i < ARRAY_SIZE(simple_attribute_groups); i++) {
if (!asus_wmi_is_present(simple_attribute_groups[i].wmi_devid))
continue;
sysfs_create_group(&asus_bioscfg.fw_attr_kset->kobj, simple_attribute_groups[i].attr_group);
pr_dbg("Created sysfs-group for %s\n", simple_attribute_groups[i].attr_group->name);
}
This will make adding new simple attributes a matter of just:
1. Declaring the new attr using one of the macros
2. Adding it to simple_attribute_groups[]
And this also takes care of you logging TODO for simple attributes
without needing to add a ton of pr_debug() calls.
Regards,
Hans
