Re: [PATCH v4] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

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

On Thu, Feb 27, 2025 at 05:10:52PM +0000, Aditya Garg wrote:
> > On 27 Feb 2025, at 8:13 PM, andriy.shevchenko@linux.intel.com 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