Hi,
thanks for reviewing.
On 2/10/21 10:48 PM, Barnabás Pőcze wrote:
> Hi
>
> 2021. február 10., szerda 18:54 keltezéssel, Alexander Kobel írta:
>
>> @@ -174,6 +174,11 @@ enum tpacpi_hkey_event_t {
>> or port replicator */
>> TP_HKEY_EV_HOTPLUG_UNDOCK = 0x4011, /* undocked from hotplug
>> dock or port replicator */
>> + /* Thinkpad X1 Tablet series (and probably other GTOP type 4) emit 0x4012 and 0x4013
>
> The preferred style of multi-line comments is
>
> /* <note the empty line here>
> * Lorem ipsum ...
> */
Corrected.
>> @@ -2166,11 +2173,32 @@ static int hotkey_gmms_get_tablet_mode(int s, int *has_tablet_mode)
>> return !!(mode & TP_ACPI_MULTI_MODE_TABLET_LIKE);
>> }
>>
>> +static int hotkey_gtop_any_type_get_tablet_mode(int s)
>> +{
>> + return !(s & 0x1);
>> +}
>
> I believe it would be preferable to do something like
>
> #define TP_GTOP_TYPE_ANY_ATTACH_STATE BIT(0)
> /* or possibly use an enum */
> ...
> return !(s & TP_GTOP_TYPE_ANY_ATTACH_STATE);
>
> or let `s` be `unsigned long`, and then
>
> #define TP_GTOP_TYPE_ANY_ATTACH_STATE_BIT 0
> ...
> return !test_bit(TP_GTOP_TYPE_ANY_ATTACH_STATE_BIT, &s);
>
> or something along these lines, and I also think the return type could be `bool`.
Indeed, thanks for the suggestion. I used the first variant.
>> +
>> +static int hotkey_gtop_x1_tablet_type_get_tablet_mode(int s)
>> +{
>> + return (!(s & 0x1) /* keyboard NOT attached */
>> + || ((s >> 16) & 0x1) /* or folded onto the back */);
>> +}
>
> Same here, I suggest using the `BIT()` macro or `test_bit()` and preferably
> name the constants.
Ditto.
>> @@ -3213,7 +3241,62 @@ static int hotkey_init_tablet_mode(void)
>> int in_tablet_mode = 0, res;
>> char *type = NULL;
>>
>> - if (acpi_evalf(hkey_handle, &res, "GMMS", "qdd", 0)) {
>> + if (acpi_evalf(hkey_handle, &res, "GTOP", "qdd", 0x0000)
>> + && res >= 0x0103
>> + && acpi_evalf(hkey_handle, &res, "GTOP", "qdd", 0x0100)) {
>
> Minor thing, but checkpatch prefers `&&` at the end of the line. And the rest of
> the code places them at the end, too.
Corrected, thanks.
>> + /*
>> + * GTOP ("Get Tablet OPtion") state ASL method definition:
>> + * - Input: 0x0000: Query version
>> + * Output: 0x0103 (version 1.03)
>> + * - Input: 0x0100: Query interface type
>> + * Output: DWORD But 31-0 Interface type
> ^
> Shouldn't that be "Bit"?
Of course. I effectively turned the entire comment into descriptive constants, at the same time correcting the other issues.
>> + switch (res) {
>> + case 1:
>> + tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_GTOP_ANY_TYPE;
>> + if (acpi_evalf(hkey_handle, &res, "GTOP", "qdd", 0x200))
>
> Please name that `0x200` something like TP_GTOP_CMD_GET_ATTACH_STATE or something
> you like. (Same for the rest of the GTOP calls.) I know it's just above in the comment,
> but I still think it'd be better to have concrete, more or less self-explanatory names
> in the actual command
Indeed. All magic constants are now collected at a single place before the HKEY constants.
Thanks again,
Alex
> Regards,
> Barnabás Pőcze
>
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
