Hi,
On 2/4/21 1:45 PM, Barnabás Pőcze wrote:
> Hi
>
>
> 2021. február 4., csütörtök 9:58 keltezéssel, Hans de Goede írta:
>
>> Hi,
>>
>> On 2/3/21 10:56 PM, Barnabás Pőcze wrote:
>>
>>> Fix all checkpatch warnings. Reorder variable definitions from
>>> longest to shortest. Add more whitespaces for better readability.
>>> Rename variables named `ret` to `err` where appropriate. Reorder
>>> sysfs attributes show/store callbacks and the `ideapad_attributes`
>>> array in lexicographic order. And other minor formatting changes.
>>> No significant functional changes are intended.
>>> Signed-off-by: Barnabás Pőcze pobrn@xxxxxxxxxxxxxx
>>
>> Ugh, this is a big patch with a lot of things going on.
>>
>> I will take this patch as is this time, but next time please
>> split patches like this up a bit:
>>
>> 1. The reworking of the sysfs show/store functions really belongs in a separate patch
>> 2. That separate rework sysfs show/store functions patch itself should be 2 patches:
>> 2.1 Move the various show/store functions around to their final alphabetical order
>> without any further changes, just move the blocks. And explicitly mention this
>> in the commit message, to make life easier for the reviewer
>> 2.2 And then do the actual reworking in a separate patch, that makes reviewing this
>> much much easier, now I had to jump back and forth between the old and new blocks
>> (which were not in the same place) to make sure that nothing has changed.
>> Note doing things like this (first move without any changes other then moving)
>> also makes checking your own work easier.
>>
>> 3. This change:
>>
>>> @@ -773,7 +801,10 @@ static void dytc_profile_refresh(struct ideapad_private *priv)
>>> return;
>>> perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF;
>>>
>>> - convert_dytc_to_profile(perfmode, &profile);
>>>
>>> -
>>> - if (convert_dytc_to_profile(perfmode, &profile))
>>> - return;
>>>
>>>
>>> - if (profile != priv->dytc->current_profile) {
>>> priv->dytc->current_profile = profile;
>>> platform_profile_notify();
>>>
>>
>> Although a good change, clearly is not just a checkpatch / style change and as
>> such really should have been in its own commit!
>
>
> Yes, I'm sorry, I apologize for the inconvenience, this really should've been split up
> and I failed to do so. If you want, I can still split it up and send the whole
> (or part of the) series again?
I've already merged the entire series (minus patch 12) as is, so no need to resend it,
just something to keep in mind for the next time.
Regards,
Hans
