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

["andriy.shevchenko@linux.intel.com" <andriy.shevchenko@linux.intel.com>]

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 */
  ...

(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)?

...

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

> >> +  */

...

> 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