On Tue, 16 Jul 2024, at 9:50 PM, Hans de Goede wrote: > 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. Ah perfect, this was one problem I ws trying to figure out a better way of doing. I'll have a crack at this after addressing all other concerns mentioned so far. Cheers, Luke. > > Regards, > > Hans > > >