Re: [PATCH RESEND 5/5] HID: multitouch: add device ID for Apple Touch Bar

[Aditya Garg <gargaditya08@live.com>]

> On 25 Mar 2025, at 6:43 PM, Benjamin Tissoires <bentiss@kernel.org> wrote:
> 
> On Mar 10 2025, Aditya Garg wrote:
>> From: Kerem Karabay <kekrby@gmail.com>
>> 
>> This patch adds the device ID of Apple Touch Bar found on x86 MacBook Pros
>> to the hid-multitouch driver.
>> 
>> Note that this is device ID is for T2 Macs. Testing on T1 Macs would be
>> appreciated.
>> 
>> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
>> Co-developed-by: Aditya Garg <gargaditya08@live.com>
>> Signed-off-by: Aditya Garg <gargaditya08@live.com>
>> ---
>> drivers/hid/Kconfig          |  1 +
>> drivers/hid/hid-multitouch.c | 25 +++++++++++++++++++++----
>> 2 files changed, 22 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index dfc245867..727a2ed0d 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -743,6 +743,7 @@ config HID_MULTITOUCH
>>      Say Y here if you have one of the following devices:
>>      - 3M PCT touch screens
>>      - ActionStar dual touch panels
>> +      - Apple Touch Bar on x86 MacBook Pros
>>      - Atmel panels
>>      - Cando dual touch panels
>>      - Chunghwa panels
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 66e33a482..078ceef62 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -221,6 +221,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app);
>> #define MT_CLS_GOOGLE                0x0111
>> #define MT_CLS_RAZER_BLADE_STEALTH        0x0112
>> #define MT_CLS_SMART_TECH            0x0113
>> +#define MT_CLS_APPLE_TOUCHBAR            0x0114
>> #define MT_CLS_SIS                0x0457
>> 
>> #define MT_DEFAULT_MAXCONTACT    10
>> @@ -406,6 +407,12 @@ static const struct mt_class mt_classes[] = {
>>            MT_QUIRK_CONTACT_CNT_ACCURATE |
>>            MT_QUIRK_SEPARATE_APP_REPORT,
>>    },
>> +    { .name = MT_CLS_APPLE_TOUCHBAR,
>> +        .quirks = MT_QUIRK_HOVERING |
>> +            MT_QUIRK_SLOT_IS_CONTACTID_MINUS_ONE |
>> +            MT_QUIRK_APPLE_TOUCHBAR,
>> +        .maxcontacts = 11,
>> +    },
>>    { .name = MT_CLS_SIS,
>>        .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP |
>>            MT_QUIRK_ALWAYS_VALID |
>> @@ -1807,6 +1814,15 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>        }
>>    }
>> 
>> +    ret = hid_parse(hdev);
>> +    if (ret != 0)
>> +        return ret;
>> +
>> +    if (mtclass->name == MT_CLS_APPLE_TOUCHBAR &&
>> +        !hid_find_field(hdev, HID_INPUT_REPORT,
>> +                HID_DG_TOUCHPAD, HID_DG_TRANSDUCER_INDEX))
>> +        return -ENODEV;
>> +
> 
> That hunk and the one below make me very nervous. Is there any reason
> preventing you to keep hid_parse() at the same place?
> 
Wouldn't we need to parse in order to do hid_find_field? Although I haven't tried putting it at the same place tbh.

> The rest of the series looks fine as everything seems properly guarded
> by MT_CLS_APPLE_TOUCHBAR.
> 
> Cheers,
> Benjamin
> 
>>    td = devm_kzalloc(&hdev->dev, sizeof(struct mt_device), GFP_KERNEL);
>>    if (!td) {
>>        dev_err(&hdev->dev, "cannot allocate multitouch data\n");
>> @@ -1854,10 +1870,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> 
>>    timer_setup(&td->release_timer, mt_expired_timeout, 0);
>> 
>> -    ret = hid_parse(hdev);
>> -    if (ret != 0)
>> -        return ret;
>> -
>>    if (mtclass->quirks & MT_QUIRK_FIX_CONST_CONTACT_ID)
>>        mt_fix_const_fields(hdev, HID_DG_CONTACTID);
>> 
>> @@ -2339,6 +2351,11 @@ static const struct hid_device_id mt_devices[] = {
>>        MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
>>            USB_DEVICE_ID_XIROKU_CSR2) },
>> 
>> +    /* Apple Touch Bar */
>> +    { .driver_data = MT_CLS_APPLE_TOUCHBAR,
>> +        HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
>> +            USB_DEVICE_ID_APPLE_TOUCHBAR_DISPLAY) },
>> +
>>    /* Google MT devices */
>>    { .driver_data = MT_CLS_GOOGLE,
>>        HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, USB_VENDOR_ID_GOOGLE,
>> --
>> 2.43.0
>>