Re: [PATCH v5 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
From: Thomas Zimmermann
Date: Tue Feb 25 2025 - 07:05:35 EST
Hi
Am 25.02.25 um 11:33 schrieb andriy.shevchenko@xxxxxxxxxxxxxxx:
On Tue, Feb 25, 2025 at 10:09:42AM +0000, Aditya Garg wrote:
From: Kerem Karabay <kekrby@xxxxxxxxx>
The Touch Bars found on x86 Macs support two USB configurations: one
where the device presents itself as a HID keyboard and can display
predefined sets of keys, and one where the operating system has full
control over what is displayed.
This commit adds support for the display functionality of the second
configuration. Functionality for the first configuration has been
merged in the HID tree.
Note that this driver has only been tested on T2 Macs, and only includes
the USB device ID for these devices. Testing on T1 Macs would be
appreciated.
Credit goes to Ben (Bingxing) Wang on GitHub for reverse engineering
most of the protocol.
Also, as requested by Andy, I would like to clarify the use of __packed
structs in this driver:
- All the packed structs are aligned except for appletbdrm_msg_information.
- We have to pack appletbdrm_msg_information since it is requirement of
the protocol.
- We compared binaries compiled by keeping the rest structs __packed and
not __packed using bloat-o-meter, and __packed was not affecting code
generation.
- To maintain consistency, rest structs have been kept __packed.
I would also like to point out that since the driver was reverse-engineered
the actual data types of the protocol might be different, including, but
not limited to, endianness.
...
+static int appletbdrm_probe(struct usb_interface *intf,
+ const struct usb_device_id *id)
+{
+ struct usb_endpoint_descriptor *bulk_in, *bulk_out;
+ struct device *dev = &intf->dev;
+ struct appletbdrm_device *adev;
+ struct drm_device *drm;
+ int ret;
+
+ ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
+ if (ret) {
+ drm_err(drm, "Failed to find bulk endpoints\n");
This is simply wrong (and in this case even lead to crash in some circumstances).
drm_err() may not be used here. That's my point in previous discussions.
Independently on the subsystem the ->probe() for the sake of consistency and
being informative should only rely on struct device *dev,
That's never going to work with DRM. There's so much code in a DRM probe
function that uses the DRM error functions.
This specific instance here is wrong, as the drm pointer hasn't been
initialized. But as soon as it is, it's much better to use drm_err() and
friends. It will do the right thing and give consistent output across
drivers.
Best regards
Thomas
+ return ret;
+ }
+
+ adev = devm_drm_dev_alloc(dev, &appletbdrm_drm_driver, struct appletbdrm_device, drm);
+ if (IS_ERR(adev))
+ return PTR_ERR(adev);
+
+ adev->in_ep = bulk_in->bEndpointAddress;
+ adev->out_ep = bulk_out->bEndpointAddress;
+ adev->dmadev = dev;
+
+ drm = &adev->drm;
+
+ usb_set_intfdata(intf, adev);
+
+ ret = appletbdrm_get_information(adev);
+ if (ret) {
+ drm_err(drm, "Failed to get display information\n");
+ return ret;
+ }
+
+ ret = appletbdrm_signal_readiness(adev);
+ if (ret) {
+ drm_err(drm, "Failed to signal readiness\n");
+ return ret;
+ }
+
+ ret = appletbdrm_setup_mode_config(adev);
+ if (ret) {
+ drm_err(drm, "Failed to setup mode config\n");
+ return ret;
+ }
+
+ ret = drm_dev_register(drm, 0);
+ if (ret) {
+ drm_err(drm, "Failed to register DRM device\n");
+ return ret;
+ }
+
+ ret = appletbdrm_clear_display(adev);
+ if (ret) {
+ drm_err(drm, "Failed to clear display\n");
+ return ret;
+ }
+
+ return 0;
+}
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)