On 15/01/2025 2:07 PM, Ilpo Järvinen wrote:
>> 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?
Yes, I did another double check to be sure, with different non zero values, here each byte is clearly setting its own fan speed, they're independent from each other.
So the writing you suggest is more appropriate, resetting both fans to automatic speed, needs the zero value (HP_FAN_SPEED_AUTOMATIC) to be set into both bytes.
>>>> +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).
Fine :-) Thanks for the explanations, no problem I completely understand.
So I looked around for occurrences of the same things to fix into others already existing functions, and I carefully applied the change where appropriate.
>> 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! :-)
Good to know!
Prepared the v3 for the next round (tested it and checked with scripts/checkpatch.pl as always), and described the changes; I'm sending it now.
