On 10-Feb-2023, at 10:26 AM, Thomas Weißschuh [off-list ref] wrote:

Hi,

some comments inline.

On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:

quoted

From: Ronald Tschalär <redacted>

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 <redacted>
[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 <redacted>
Signed-off-by: Aditya Garg <redacted>
---
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

quoted

+}
+
+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?

quoted

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

quoted

+
+static struct appleib_device *appleib_alloc_device(struct platform_device *pdev)
+{
+ struct appleib_device *ib_dev;
+ acpi_status sts;