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 8:27 PM, andriy.shevchenko@linux.intel.com wrote:
> 
> On Mon, Feb 24, 2025 at 02:32:37PM +0000, Aditya Garg wrote:
>>> 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:
> 
> ...
> 
>>>> +#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
> 
> For the starter I would do the following for all related constants and
> drop that weird and ugly macros at the top (it also has an issue with
> the str4 length as it is 5 bytes long, not 4, btw):
> 
>  #define APPLETBDRM_MSG_CLEAR_DISPLAY cpu_to_le32(0x434c5244) /* CLRD */

Lemme test this.
>  ...
> 
> (assuming we stick with __leXX for now). This will be much less confusing.
> 
> ...
> 
>>>> +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.
> 
> Right. I agree that is better to have something working than something
> good looking, but wrong.
> 
> Can you add a summary to the commit message that since the driver was
> reverse-engineered the actual data types of the protocol might be different
> (including, but not limited to, endianess)?

Ok

> 
> ...
> 
>>>> + /*
>>>> +  * 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.
> 
> But those are seems to me in the correct locations, no? How do we even know
> the DRM device before its creation? So, dev_err_probe() calls in ->probe()
> seem logical to me. Somebody from DRM should clarify this.

Thomas asked me to do this change. Maybe you didn’t see his reply.
> 
>>>> +  */
> 
> ...
> 
>> I’ll send a v5.
> 
> Please, wait a bit. it's too fast to send one version quicker than 24h...
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>