On 16/01/2025 1:50 PM, Ilpo Järvinen wrote:
>> enum hp_thermal_profile {
>> HP_THERMAL_PROFILE_PERFORMANCE = 0x00,
>> HP_THERMAL_PROFILE_DEFAULT = 0x01,
>> @@ -389,7 +423,6 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>>
>> bios_return = (struct bios_return *)obj->buffer.pointer;
>> ret = bios_return->return_code;
>> -
>> if (ret) {
>
> I understand why you made this change (because I told you it okay to
> improve the patch outside of my comments). But it doesn't mean we should
> try to do everything in the same patch.
>
> I'm sorry for the extra confusion.
>
> This change is good, but went you start to make changes entirely unrelated
> to the logical change you're making in this patch, you need to put them
> into a separate patch (and send a patch series). In general, we try to put
> only a single logical change into a single patch. Don't worry if it feels
> making patch series "larger", that is rarely a real problem but
> instead helps review by splitting things into more logical and focused
> chunks, it's easier to see an individual change is "correct".
>
> There's some leeway and fuzziness in the minimal & logical rule. When you
> touch a line, in general, we expect the end result to conform to the
> latest coding style, up-to-date api use, etc. so it might not be required
> to split every single "unrelated" change out of a feature patch but some
> things are allowed to be made "while at it".
>
> I'm personally not against in maintaining full logical splitting in a
> patch series but due to conflicts from touching the same lines over and
> over again, it's a bit pain to manage it. I try to largely do full loglcal
> split myself so I know the constants conflicts that need to be resolved
> even at the tiniests edits may make people prefer do things more "while at
> it". As long as it's not about some lines entirely unrelated to your main
> logical change, it's usually acceptable.
>
> You also don't necessarily have to get every cleanup done in this series
> but can send more patches later if you feel like there's more things you
> want to do, so we can get the feature merged in sooner. But it's up to you
> which way you prefer :-).
Oops, sorry, my mistake, a little misunderstanding about the scope of the expected coding style fixup.
For my 1st patch submission, then, I'll keep the focus on the new feature ;-)
While I'd be happy to submit future helpful contributions when needed, I wouldn't want to become too invasive by touching too many lines outside of ideal occasions of doing so.
>> @@ -429,13 +482,29 @@ static int hp_wmi_get_fan_speed(int fan)
>> return (fsh << 8) | fsl;
>> }
>>
>> +static int hp_wmi_get_fan_speed_victus_s(int fan)
>> +{
>> + u8 fan_data[128] = {};
>> + int ret;
>> +
>> + ret = hp_wmi_perform_query(HPWMI_VICTUS_S_FAN_SPEED_GET_QUERY,
>> + HPWMI_GM, &fan_data, sizeof(u8),
>> + sizeof(fan_data));
>> + if (ret != 0)
>> + return -EINVAL;
>> +
>> + if (fan >= 0 && fan < sizeof(fan_data))
>> + return fan_data[fan] * 100;
>> + else
>> + return -EINVAL;
>
> I think the out-of-bound check should be done before even making the wmi
> query (and -EINVAL returned right away).
>
Yes, let's change that.
>> +/* Note: providing 0x00 as PL1 and PL2 is restoring default values */
>
> This should now refer to the new define, not 0x00.
>
That's right, changing that too.
Thanks again and sorry about the little v3 fixup overshoot.
Preparing the v4 for the next round, I'll send it as soon as it is ready and well checked.
Best regards,
Julien ROBIN
