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

[Benjamin Tissoires]

On Mar 25 2025, Aditya Garg wrote:
> 
> 
> > On 25 Mar 2025, at 6:43 PM, Benjamin Tissoires <bentiss@xxxxxxxxxx> wrote:
> > 
> > On Mar 10 2025, Aditya Garg wrote:
> >> From: Kerem Karabay <kekrby@xxxxxxxxx>
> >> 
> >> 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@xxxxxxxxx>
> >> Co-developed-by: Aditya Garg <gargaditya08@xxxxxxxx>
> >> Signed-off-by: Aditya Garg <gargaditya08@xxxxxxxx>
> >> ---
> >> 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.

Yes, you need hid_parse() to be able to call hid_find_field(). But you
can put hid_find_field() after hid_parse() at the original location, no?

Cheers,
Benjamin

> 
> > 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
> >>