Hi all, Soliciting for opinion on a tiny refactor I've noticed is possible in the hwmgr API, we have functions like int (*dynamic_state_management_enable)( struct pp_hwmgr *hw_mgr); int (*dynamic_state_management_disable)( struct pp_hwmgr *hw_mgr); Which could be merged with a 2nd parameter, or: uint32_t (*get_mclk)(struct pp_hwmgr *hwmgr, bool low); uint32_t (*get_sclk)(struct pp_hwmgr *hwmgr, bool low); Could be merged as well (with a 3rd parameter), and: int (*get_sclk_od)(struct pp_hwmgr *hwmgr); int (*set_sclk_od)(struct pp_hwmgr *hwmgr, uint32_t value); int (*get_mclk_od)(struct pp_hwmgr *hwmgr); int (*set_mclk_od)(struct pp_hwmgr *hwmgr, uint32_t value); Could be merged somewhat, etc. The gain is that we have duplicate blocks like: static uint32_t smu7_dpm_get_mclk(struct pp_hwmgr *hwmgr, bool low) { struct pp_power_state *ps; struct smu7_power_state *smu7_ps; if (hwmgr == NULL) return -EINVAL; ps = hwmgr->request_ps; if (ps == NULL) return -EINVAL; smu7_ps = cast_phw_smu7_power_state(&ps->hardware); if (low) return smu7_ps->performance_levels[0].memory_clock; else return smu7_ps->performance_levels [smu7_ps->performance_level_count-1].memory_clock; } static uint32_t smu7_dpm_get_sclk(struct pp_hwmgr *hwmgr, bool low) { struct pp_power_state *ps; struct smu7_power_state *smu7_ps; if (hwmgr == NULL) return -EINVAL; ps = hwmgr->request_ps; if (ps == NULL) return -EINVAL; smu7_ps = cast_phw_smu7_power_state(&ps->hardware); if (low) return smu7_ps->performance_levels[0].engine_clock; else return smu7_ps->performance_levels [smu7_ps->performance_level_count-1].engine_clock; } Where most of the two functions are the same and could be reduced. Anyways nothing earth shattering but would help reduce some LOC and make the API simpler. I could tackle this for the various hwmgr's all in one series. Thoughts? Tom