Re: [PATCH 1/3] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip.

[Aditya Garg]

> On 10-Feb-2023, at 2:09 PM, Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote:
> 
> On Fri, Feb 10, 2023 at 9:30 AM Aditya Garg <gargaditya08@xxxxxxxx> wrote:
>> 
>> 
>> 
>>> On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh <thomas@xxxxxxxx> wrote:
>>> 
>>> Hi,
>>> 
>>> some comments inline.
>>> 
>>> On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
>>>> From: Ronald Tschalär <ronald@xxxxxxxxxxxxx>
>>>> 
>>>> The iBridge device provides access to several devices, including:
>>>> - the Touch Bar
>>>> - the iSight webcam
>>>> - the light sensor
>>>> - the fingerprint sensor
>>>> 
>>>> This driver provides the core support for managing the iBridge device
>>>> and the access to the underlying devices. In particular, the
>>>> functionality for the touch bar and light sensor is exposed via USB HID
>>>> interfaces, and on devices with the T1 chip one of the HID devices is
>>>> used for both functions. So this driver creates virtual HID devices, one
>>>> per top-level report collection on each HID device (for a total of 3
>>>> virtual HID devices). The sub-drivers then bind to these virtual HID
>>>> devices.
>>>> 
>>>> This way the Touch Bar and ALS drivers can be kept in their own modules,
>>>> while at the same time making them look very much like as if they were
>>>> connected to the real HID devices. And those drivers then work (mostly)
>>>> without further changes on MacBooks with the T2 chip that don't need
>>>> this driver.
>>>> 
>>>> Signed-off-by: Ronald Tschalär <ronald@xxxxxxxxxxxxx>
>>>> [Kerem Karabay: convert to a platform driver]
>>>> [Kerem Karabay: fix appleib_forward_int_op]
>>>> [Kerem Karabay: rely on HID core's parsing in appleib_add_device]
>>>> Signed-off-by: Kerem Karabay <kekrby@xxxxxxxxx>
>>>> Signed-off-by: Aditya Garg <gargaditya08@xxxxxxxx>
>>>> ---
>>>> drivers/hid/Kconfig         |  15 +
>>>> drivers/hid/Makefile        |   1 +
>>>> drivers/hid/apple-ibridge.c | 610 ++++++++++++++++++++++++++++++++++++
>>>> drivers/hid/apple-ibridge.h |  15 +
>>>> drivers/hid/hid-ids.h       |   1 +
>>>> drivers/hid/hid-quirks.c    |   3 +
>>>> 6 files changed, 645 insertions(+)
>>>> create mode 100644 drivers/hid/apple-ibridge.c
>>>> create mode 100644 drivers/hid/apple-ibridge.h
>>>> 
>>> 
>>>> 
>>>> +}
>>>> +
>>>> +static int appleib_ll_raw_request(struct hid_device *hdev,
>>>> +   unsigned char reportnum, __u8 *buf,
>>>> +   size_t len, unsigned char rtype, int reqtype)
>>>> +{
>>>> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
>>>> +
>>>> + return hid_hw_raw_request(hdev_info->hdev, reportnum, buf, len, rtype,
>>>> +   reqtype);
>>>> +}
>>>> +
>>>> +static int appleib_ll_output_report(struct hid_device *hdev, __u8 *buf,
>>>> +     size_t len)
>>>> +{
>>>> + struct appleib_hid_dev_info *hdev_info = hdev->driver_data;
>>>> +
>>>> + return hid_hw_output_report(hdev_info->hdev, buf, len);
>>>> +}
>>>> +
>>>> +static struct hid_ll_driver appleib_ll_driver = {
>>>> + .start = appleib_ll_start,
>>>> + .stop = appleib_ll_stop,
>>>> + .open = appleib_ll_open,
>>>> + .close = appleib_ll_close,
>>>> + .power = appleib_ll_power,
>>>> + .parse = appleib_ll_parse,
>>>> + .request = appleib_ll_request,
>>>> + .wait = appleib_ll_wait,
>>>> + .raw_request = appleib_ll_raw_request,
>>>> + .output_report = appleib_ll_output_report,
>>>> +};
>>> 
>>> const
>> 
>> Are you sure about const here?
>> 
>>> 
>>>> +
>>>> + if (udev->actconfig->desc.bConfigurationValue != APPLEIB_BASIC_CONFIG) {
>>>> + rc = usb_driver_set_configuration(udev, APPLEIB_BASIC_CONFIG);
>>>> + return rc ? rc : -ENODEV;
>>>> + }
>>>> +
>>>> + rc = hid_parse(hdev);
>>>> + if (rc) {
>>>> + hid_err(hdev, "ib: hid parse failed (%d)\n", rc);
>>>> + goto error;
>>>> + }
>>>> +
>>>> + rc = hid_hw_start(hdev, HID_CONNECT_DRIVER);
>>>> + if (rc) {
>>>> + hid_err(hdev, "ib: hw start failed (%d)\n", rc);
>>>> + goto error;
>>>> + }
>>>> +
>>>> + hdev_info = appleib_add_device(hdev);
>>>> + if (IS_ERR(hdev_info)) {
>>>> + rc = PTR_ERR(hdev_info);
>>>> + goto stop_hw;
>>>> + }
>>>> +
>>>> + hid_set_drvdata(hdev, hdev_info);
>>>> +
>>>> + rc = hid_hw_open(hdev);
>>>> + if (rc) {
>>>> + hid_err(hdev, "ib: failed to open hid: %d\n", rc);
>>>> + goto remove_dev;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +
>>>> +remove_dev:
>>>> + appleib_remove_device(hdev);
>>>> +stop_hw:
>>>> + hid_hw_stop(hdev);
>>>> +error:
>>>> + return rc;
>>>> +}
>>>> +
>>>> +static void appleib_hid_remove(struct hid_device *hdev)
>>>> +{
>>>> + hid_hw_close(hdev);
>>>> + appleib_remove_device(hdev);
>>>> + hid_hw_stop(hdev);
>>>> +}
>>>> +
>>>> +static const struct hid_device_id appleib_hid_ids[] = {
>>>> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
>>>> + { },
>>>> +};
>>>> +
>>>> +static struct hid_driver appleib_hid_driver = {
>>>> + .name = "apple-ibridge-hid",
>>>> + .id_table = appleib_hid_ids,
>>>> + .probe = appleib_hid_probe,
>>>> + .remove = appleib_hid_remove,
>>>> + .raw_event = appleib_hid_raw_event,
>>>> + .report_fixup = appleib_report_fixup,
>>>> +#ifdef CONFIG_PM
>>>> + .suspend = appleib_hid_suspend,
>>>> + .resume = appleib_hid_resume,
>>>> + .reset_resume = appleib_hid_reset_resume,
>>>> +#endif
>>>> +};
>>> 
>>> const
>> 
>> Are you sure you want to do const here, cause other hid-drivers are NOT using const.
> 
> It is scheduled for 6.3:
> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.3/hid-core
> 
> So yes, please make them const.

Makes sense for `hid_ll_driver` as per the tree you send me, but I don’t see such a thing for
`static struct hid_driver`. Is that scheduled as well?

> 
> Cheers,
> Benjamin
> 
>> 
>>>> +
>>>> +static struct appleib_device *appleib_alloc_device(struct platform_device *pdev)
>>>> +{
>>>> + struct appleib_device *ib_dev;
>>>> + acpi_status sts;