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

[Aditya Garg <gargaditya08@live.com>]

> On 24 Feb 2025, at 7:30 PM, andriy.shevchenko@linux.intel.com wrote:
> 
> On Mon, Feb 24, 2025 at 01:40:20PM +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.
> 
> ...
> 
>> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << 24) | (str4[1] << 16) | (str4[2] << 8) | str4[3]))
> 
> As commented previously this is quite strange what's going on with endianess in
> this driver. Especially the above weirdness when get_unaligned_be32() is being
> open coded and force-cast to __le32.

I would assume it was also mimicked from the Windows driver, though I haven't really tried exploring this there.

I’d rather be happy if you give me code change suggestions and let me review and test them

> 
> ...
> 
>> +struct appletbdrm_msg_information {
>> + struct appletbdrm_msg_response_header header;
>> + u8 unk_14[12];
>> + __le32 width;
>> + __le32 height;
>> + u8 bits_per_pixel;
>> + __le32 bytes_per_row;
>> + __le32 orientation;
>> + __le32 bitmap_info;
>> + __le32 pixel_format;
>> + __le32 width_inches; /* floating point */
>> + __le32 height_inches; /* floating point */
>> +} __packed;
> 
> Haven't looked deeply into the protocol, but still makes me think that
> the above (since it's the only __packed data type required) might be simply
> depicted wrongly w.r.t. endianess / data types in use. It might be that
> the data types have something combined and / or different types.
> 
> Do I understand correctly that the protocol was basically reverse-engineered?

Yes. Although it was reverse engineered by the person who wrote the Windows driver. The author has just made a Linux port.
So, as far as how is was reverse engineered, it not really possible for me to explain. I don't even have any contact with the person who wrote the Windows driver. The only point here would be to myself RE the hardware again, which tbh isn't very motivating, considering that we have a working driver.
> 
> ...
> 
>> + /*
>> +  * The coordinate system used by the device is different from the
>> +  * coordinate system of the framebuffer in that the x and y axes are
>> +  * swapped, and that the y axis is inverted; so what the device reports
>> +  * as the height is actually the width of the framebuffer and vice
>> +  * versa
> 
> Missing period.

Alright. For some reason (a mistake on my part), some dev_err_probe were also still left in this version. I’ll send a v5.
> 
>> +  */
> 
> ...
> 
> Otherwise it's nice tiny driver.
> 
> -- 
> With Best Regards,
> Andy Shevchenko