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 03:56:36PM +0000, Aditya Garg wrote:
> > On 24 Feb 2025, at 9:23 PM, andriy.shevchenko@linux.intel.com wrote:
> > On Mon, Feb 24, 2025 at 03:40:29PM +0000, Aditya Garg wrote:
> >>>> On 24 Feb 2025, at 9:08 PM, andriy.shevchenko@linux.intel.com wrote:
> >>> On Mon, Feb 24, 2025 at 03:32:56PM +0000, Aditya Garg wrote:
> >>>>> On 24 Feb 2025, at 8:50 PM, Aditya Garg <gargaditya08@live.com> wrote:
> >>>>>> On 24 Feb 2025, at 8:41 PM, andriy.shevchenko@linux.intel.com wrote:
> >>>>>> On Mon, Feb 24, 2025 at 03:03:40PM +0000, Aditya Garg wrote:
> >>>>>>>>> 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.
> >>>>>>> 
> >>>>>>> Just in case it won't work, reverse bytes in the integer. Because I was lost in
> >>>>>>> this conversion.
> >>>>> 
> >>>>> It works. What I understand is that you used the macro to get the final hex and converted it into little endian, which on the x86 macs would technically remain the same.
> >>>> 
> >>>> And now that I oberved again, %p4cc is actually printing these CLRD, REDY etc
> >>>> in reverse order, probably the reason %p4ch was chosen. And I am unable to
> >>>> find what macro upstream can be used.
> >>> 
> >>> %.4s should work as it technically not DRM 4cc, but specifics of the protocol
> >>> (that reminds me about ACPI that uses 4cc a lot).
> >> 
> >> I still get reverse order in that.
> > 
> > Ah, right, it will give you the first letter as LSB, indeed. At the end of the
> > day if it's so important, there are ways how to solve that without using %p4cc.
> > But if others (and esp. PRINTK maintainers) want to have / don't object having
> > that extension, why not?
> 
> Right, but what to do about the case of little endian and host endian? I
> remember the statement "for the sake of completeness" for them. Do you think
> just host endian and reverse endian should be just fine? Or you got any "no
> sparse warning" way to get it done? The macros to convert to le32/be32 expect
> a u32 value, but in those cases we actually are passing a le32/be32 value.

For now I think we better save the energy and wait for PRINTK people to tell
if they are okay in general. Otherwise it makes no sense to develop and review
something that will go to the trash bin.

-- 
With Best Regards,
Andy Shevchenko