Re: [PATCH v2] platform/x86: hp-wmi: Add fan and thermal profile support for Victus 16-s1000

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

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

On Tue, 14 Jan 2025, Julien ROBIN wrote:

> Thanks for the fast answer,
> 
> On 1/14/25 5:20 PM, Ilpo Järvinen wrote:
> >> Changes since v1:
> >> - More clear description of 0xFF special effect when setting power limits
> >> - Added structs for clearer naming of power limits and GPU power modes settings
> >> - Retrieve and keep current GPU slowdown temp threshold (instead of hard coded)
> >> - Removed platform_profile_victus_s_get(), re-using platform_profile_omen_get()
> >> - Changed char variable types to u8 where it was more relevant
> >> - Moved some comments
> >> - Minor typo / alignment corrections
> > 
> > I wrote a few comments to the v1 thread as the relevant discussion was 
> > there but they'll be relevant to a few places in this v2.
> 
> >> +static int hp_wmi_get_fan_count_userdefine_trigger(void)
> >> +{
> >> +	u8 fan_data[4] = { 0 };
> > 
> > {} is enough to initialize the entire array.
> 
> >> +static int hp_wmi_get_fan_speed_victus_s(int fan)
> >> +{
> >> +	u8 fan_data[128] = { 0 };
> > 
> > Ditto.
> 
> Understood, I just applied that.
> 
> While discussing these array initialisations to zero: I'm addressing 
> what we finished discussing in the v1 thread by defining and using the 
> following self explanatory named literals (instead of comments):
> 
> #define HP_FAN_SPEED_AUTOMATIC	 0x00
> #define HP_POWER_LIMIT_DEFAULT	 0x00
> #define HP_POWER_LIMIT_NO_CHANGE 0xFF

Sound good.

> In this particular case, while technically the {} would work, I suppose it would still be preferable to change from:
> u8 fan_speed[2] = { 0 }; /* Restores automatic speed */
> to:
> u8 fan_speed[2] = { HP_FAN_SPEED_AUTOMATIC }

Do you actually want this?

u8 fan_speed[2]  = { HP_FAN_SPEED_AUTOMATIC, HP_FAN_SPEED_AUTOMATIC };

That is, are those both bytes fans?

> >> +static int hp_wmi_fan_speed_max_reset(void)
> >> +{
> >> +	int ret = hp_wmi_fan_speed_max_set(0);
> >> +
> >> +	if (ret)
> > 
> > To keep the call and its error handling together, please use this form:
> > 
> > 	int ret;
> > 
> > 	ret = hp_wmi_fan_speed_max_set(0);
> > 	if (ret)
> 
> Understood, and applied.
> 
> Before sending the resulting v3, would you like me to apply this form 
> change, alongside with { 0 } becoming {}, to some others already 
> existing functions around? I prefer asking before doing so, because by 
> default I'm not sure changing unrelated already existing lines is 
> expected when sending a patch. 

Yes, please change all similar cases. I always assume/hope people look 
through their own patches for similar things to address. If I'd try to 
mark all of cases, I'd anyway miss a few because my review is not going to 
be perfect (often I only realize these mid-patch, having passed n similar 
cases already; I try to focus on finding logic problems but I catch many 
many style things too while at it).

> If not, I suppose this may be the matter of another future and unrelated 
> patch submission. 
> 
> Thanks again for the review work.

You can always improve the patches beyond what is mentioned by the review 
comments! :-)

-- 
 i.