On Thu, Feb 27, 2025 at 05:10:52PM +0000, Aditya Garg wrote:
> > 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:
...
> >> +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.
No, 'network order' / 'host order'. That's where BE comes from, but you may ask
the original author about this. h/r pair makes little sense to me as it
inconsistent.
> >> +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:
Fair enough.
> 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.
I see, thanks for pointing this out.
--
With Best Regards,
Andy Shevchenko
