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.
