Re: [PATCH v5 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 25.02.25 um 11:33 schrieb andriy.shevchenko@linux.intel.com:
> On Tue, Feb 25, 2025 at 10:09:42AM +0000, Aditya Garg wrote:
>> From: Kerem Karabay <kekrby@gmail.com>
>>
>> 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)