> On 24 Feb 2025, at 8:41 PM, andriy.shevchenko@xxxxxxxxxxxxxxx wrote:
>
> On Mon, Feb 24, 2025 at 03:03:40PM +0000, Aditya Garg wrote:
>>>> On 24 Feb 2025, at 8:27 PM, andriy.shevchenko@xxxxxxxxxxxxxxx wrote:
>>> On Mon, Feb 24, 2025 at 02:32:37PM +0000, Aditya Garg wrote:
>>>>> On 24 Feb 2025, at 7:30 PM, andriy.shevchenko@xxxxxxxxxxxxxxx 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.
>
>>> (assuming we stick with __leXX for now). This will be much less confusing.
>
> ...
>
>>>> 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 saw, maybe I took it wrong, but I really don't understand how on earth
> drm_err() or whatever can be used in real ->probe() of the physical device.
>
> Imagine the hypotetical case
>
> probe(strict device *dev)
> {
> mydrm;
> foo;
> ...
> foo = devm_gpiod_get(dev, ...);
> if (IS_ERR(foo))
> return dev_err_probe(dev, ...); // how?!
> ...
> mydrm = ...DRM alloc...;
> ...
> }
>
> I don't even believe it will be possible to create drm_err_probe() as it most
> likely will require to have an allocation to be always the first op (that may
> fail) in the ->probe() which might be not the case for some device drivers.
>
>>>>>> + */
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
