Hi
> On 27 Feb 2025, at 8:13 PM, andriy.shevchenko@xxxxxxxxxxxxxxx wrote:
>
> On Thu, Feb 27, 2025 at 06:30:48AM +0000, Aditya Garg wrote:
>> From: Hector Martin <marcan@xxxxxxxxx>
>>
>> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
>
> FourCC (as Four is not an acronym itself).
Ok
>
>> it's useful to be able to print generic 4-character codes formatted as
>> an integer. Extend it to add format specifiers for printing generic
>> 32-bit FOURCCs with various endian semantics:
>>
>> %p4ch Host-endian
>
> Too many spaces :-)
Ok
>
>> %p4cl Little-endian
>> %p4cb Big-endian
>> %p4cr Reverse-endian
>
>> The endianness determines how bytes are interpreted as a u32, and the
>> FOURCC is then always printed MSByte-first (this is the opposite of
>> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
>> allow printing LSByte-first FOURCCs stored in host endian order
>> (other than the hex form being in character order, not the integer
>> value).
>
> ...
>
>> +Generic FourCC code
>> +-------------------
>> +
>> +::
>> + %p4c[hrbl] gP00 (0x67503030)
>> +
>> +Print a generic FourCC code, as both ASCII characters and its numerical
>> +value as hexadecimal.
>> +
>> +The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify
>> +host, reversed, big or little endian order data respectively. Host endian
>> +order means the data is interpreted as a 32-bit integer and the most
>> +significant byte is printed first; that is, the character code as printed
>> +matches the byte order stored in memory on big-endian systems, and is reversed
>> +on little-endian systems.
>
> Btw, this sounds to me that 'h' should be accompanied with 'n', otherwise it's
> confusing why BE is the host order out of the blue.
> so, it needs more information that this mimics htonl() / ntohl() for networking.
>
> Does 'r' actually should be 'n'?
I believe you mean negative endian? Can be done.
>
>> +Passed by reference.
>> +
>> +Examples for a little-endian machine, given &(u32)0x67503030::
>> +
>> + %p4ch gP00 (0x67503030)
>> + %p4cr 00Pg (0x30305067)
>> + %p4cb 00Pg (0x30305067)
>> + %p4cl gP00 (0x67503030)
>> +
>> +Examples for a big-endian machine, given &(u32)0x67503030::
>> +
>> + %p4ch gP00 (0x67503030)
>> + %p4cr 00Pg (0x30305067)
>> + %p4cb gP00 (0x67503030)
>> + %p4cl 00Pg (0x30305067)
>> +
>
> ...
>
>> + switch (fmt[2]) {
>> + case 'h':
>> + val = orig;
>> + break;
>> + case 'r':
>> + orig = swab32(orig);
>> + val = orig;
>> + break;
>> + case 'l':
>> + orig = (__force u32)cpu_to_le32(orig);
>> + val = orig;
>> + break;
>> + case 'b':
>> + orig = (__force u32)cpu_to_be32(orig);
>> + val = orig;
>> + break;
>> + case 'c':
>> + /* Pixel formats are printed LSB-first */
>> + val = swab32(orig & ~BIT(31));
>> + pixel_fmt = true;
>> + break;
>> + default:
>> + return error_string(buf, end, "(%p4?)", spec);
>> + }
>
> Actually you can replace all these orig copies by introducing a new boolean, pixel_be.
>
> Will become
>
> switch (fmt[2]) {
> case 'h':
> val = orig;
> break;
> case 'r': // or 'n' ?
> val = swab32(orig);
> break;
> case 'l':
> val = (__force u32)cpu_to_le32(orig);
> break;
> case 'b':
> val = (__force u32)cpu_to_be32(orig);
> break;
> case 'c':
> pixel_fmt = true;
> pixel_be = orig & BIT(31);
> /* Pixel formats are printed LSB-first */
> val = swab32(orig & ~BIT(31));
> break;
> default:
> return error_string(buf, end, "(%p4?)", spec);
> }
>
> And with this the existence of 'val' now becomes doubtful, we may reuse 'orig',
> just name it 'val' everywhere, no?
In case c, val != orig, in rest it is. We can just use pixel_fmt to check this condition, but places where we use orig, and not val will need an if statement or something similar. Tbh, it's an unecessary complication. You might want to see this part of the code:
if (pixel_fmt) {
*p++ = ' ';
strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
p += strlen(p);
}
*p++ = ' ';
*p++ = '(';
p = special_hex_number(p, output + sizeof(output) - 2, orig, sizeof(u32));
*p++ = ')';
*p = '\0';
Here, special_hex_number is common to all cases.
