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

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

[Aditya Garg]

From: Hector Martin <marcan@marcan.st>

%p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
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
%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).

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
v2 -> 
- Add this patch to appletbdrm patchset

v3 ->
- Make array static

v4 ->
- Fix code error
- Fix sparse warnings
- Make this patch separate from drm

Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++
lib/test_printf.c                         | 39 +++++++++++++++++++----
lib/vsprintf.c                            | 38 ++++++++++++++++++----
scripts/checkpatch.pl                     |  2 +-
4 files changed, 97 insertions(+), 14 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index ecccc0473..9982861fa 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -648,6 +648,38 @@ Examples::
	%p4cc	Y10  little-endian (0x20303159)
	%p4cc	NV12 big-endian (0xb231564e)

+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.
+
+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)
+
Rust
----

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 59dbe4f9a..056929c06 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -776,21 +776,46 @@ static void __init fwnode_pointer(void)
	software_node_unregister_node_group(group);
}

+struct fourcc_struct {
+	u32 code;
+	const char *str;
+};
+
+static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n,
+				       const char *fmt)
+{
+	size_t i;
+
+	for (i = 0; i < n; i++)
+		test(fc[i].str, fmt, &fc[i].code);
+}
+
static void __init fourcc_pointer(void)
{
-	struct {
-		u32 code;
-		char *str;
-	} const try[] = {
+	static const struct fourcc_struct try_cc[] = {
		{ 0x3231564e, "NV12 little-endian (0x3231564e)", },
		{ 0xb231564e, "NV12 big-endian (0xb231564e)", },
		{ 0x10111213, ".... little-endian (0x10111213)", },
		{ 0x20303159, "Y10  little-endian (0x20303159)", },
	};
-	unsigned int i;
+	static const struct fourcc_struct try_ch = {
+		0x41424344, "ABCD (0x41424344)",
+	};
+	static const struct fourcc_struct try_cr = {
+		0x41424344, "DCBA (0x44434241)",
+	};
+	static const struct fourcc_struct try_cl = {
+		le32_to_cpu(0x41424344), "ABCD (0x41424344)",
+	};
+	static const struct fourcc_struct try_cb = {
+		be32_to_cpu(0x41424344), "ABCD (0x41424344)",
+	};

-	for (i = 0; i < ARRAY_SIZE(try); i++)
-		test(try[i].str, "%p4cc", &try[i].code);
+	fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc");
+	fourcc_pointer_test(&try_ch, 1, "%p4ch");
+	fourcc_pointer_test(&try_cr, 1, "%p4cr");
+	fourcc_pointer_test(&try_cl, 1, "%p4cl");
+	fourcc_pointer_test(&try_cb, 1, "%p4cb");
}

static void __init
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 56fe96319..2ac90aba2 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1781,27 +1781,53 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
	char output[sizeof("0123 little-endian (0x01234567)")];
	char *p = output;
	unsigned int i;
+	bool pixel_fmt = false;
	u32 orig, val;

-	if (fmt[1] != 'c' || fmt[2] != 'c')
+	if (fmt[1] != 'c')
		return error_string(buf, end, "(%p4?)", spec);

	if (check_pointer(&buf, end, fourcc, spec))
		return buf;

	orig = get_unaligned(fourcc);
-	val = orig & ~BIT(31);
+	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);
+	}

	for (i = 0; i < sizeof(u32); i++) {
-		unsigned char c = val >> (i * 8);
+		unsigned char c = val >> ((3 - i) * 8);

		/* Print non-control ASCII characters as-is, dot otherwise */
		*p++ = isascii(c) && isprint(c) ? c : '.';
	}

-	*p++ = ' ';
-	strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
-	p += strlen(p);
+	if (pixel_fmt) {
+		*p++ = ' ';
+		strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
+		p += strlen(p);
+	}

	*p++ = ' ';
	*p++ = '(';
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7b28ad331..21516f753 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6904,7 +6904,7 @@ sub process {
					    ($extension eq "f" &&
					     defined $qualifier && $qualifier !~ /^w/) ||
					    ($extension eq "4" &&
-					     defined $qualifier && $qualifier !~ /^cc/)) {
+					     defined $qualifier && $qualifier !~ /^c[chlbr]/)) {
						$bad_specifier = $specifier;
						last;
					}
-- 
2.43.0

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

[andriy.shevchenko]

On Thu, Feb 27, 2025 at 06:30:48AM +0000, Aditya Garg wrote:
> From: Hector Martin <marcan@marcan.st>
> 
> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but

FourCC (as Four is not an acronym itself).

> 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 :-)

> %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'?

> +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?

-- 
With Best Regards,
Andy Shevchenko

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

[Aditya Garg]

Hi
> 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:
>> From: Hector Martin <marcan@marcan.st>
>> 
>> %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.

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

[Aditya Garg]

> On 27 Feb 2025, at 10:40 PM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> 
> Hi
>>> 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:
>>> From: Hector Martin <marcan@marcan.st>
>>> 
>>> %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:

More easier IMO can be:

val = pixel_fmt ? swab32(orig & ~BIT(31)) : orig;

At the end of the table

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

[andriy.shevchenko]

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

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

[andriy.shevchenko]

On Thu, Feb 27, 2025 at 06:58:24PM +0000, Aditya Garg wrote:
> > On 27 Feb 2025, at 10:40 PM, Aditya Garg <gargaditya08@live.com> 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:

...

> >>> +    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:
> 
> More easier IMO can be:
> 
> val = pixel_fmt ? swab32(orig & ~BIT(31)) : orig;
> 
> At the end of the table

Also will work for me. Just think about it, i.e. how to make code shorter and
still readable.

-- 
With Best Regards,
Andy Shevchenko

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

[Aditya Garg]

> On 27 Feb 2025, at 12:00 PM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> From: Hector Martin <marcan@marcan.st>
> 
> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> 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
> %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).
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
> v2 ->
> - Add this patch to appletbdrm patchset
> 
> v3 ->
> - Make array static
> 
> v4 ->
> - Fix code error
> - Fix sparse warnings
> - Make this patch separate from drm
> 
> Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++
> lib/test_printf.c                         | 39 +++++++++++++++++++----
> lib/vsprintf.c                            | 38 ++++++++++++++++++----
> scripts/checkpatch.pl                     |  2 +-
> 4 files changed, 97 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index ecccc0473..9982861fa 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -648,6 +648,38 @@ Examples::
>    %p4cc    Y10  little-endian (0x20303159)
>    %p4cc    NV12 big-endian (0xb231564e)
> 
> +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.
> +
> +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)
> +
> Rust
> ----
> 
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 59dbe4f9a..056929c06 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void)
>    software_node_unregister_node_group(group);
> }
> 
> +struct fourcc_struct {
> +    u32 code;
> +    const char *str;
> +};
> +
> +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n,
> +                       const char *fmt)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < n; i++)
> +        test(fc[i].str, fmt, &fc[i].code);
> +}
> +
> static void __init fourcc_pointer(void)
> {
> -    struct {
> -        u32 code;
> -        char *str;
> -    } const try[] = {
> +    static const struct fourcc_struct try_cc[] = {
>        { 0x3231564e, "NV12 little-endian (0x3231564e)", },
>        { 0xb231564e, "NV12 big-endian (0xb231564e)", },
>        { 0x10111213, ".... little-endian (0x10111213)", },
>        { 0x20303159, "Y10  little-endian (0x20303159)", },
>    };
> -    unsigned int i;
> +    static const struct fourcc_struct try_ch = {
> +        0x41424344, "ABCD (0x41424344)",
> +    };
> +    static const struct fourcc_struct try_cr = {
> +        0x41424344, "DCBA (0x44434241)",
> +    };
> +    static const struct fourcc_struct try_cl = {
> +        le32_to_cpu(0x41424344), "ABCD (0x41424344)",
> +    };
> +    static const struct fourcc_struct try_cb = {
> +        be32_to_cpu(0x41424344), "ABCD (0x41424344)",
> +    };
> 
> -    for (i = 0; i < ARRAY_SIZE(try); i++)
> -        test(try[i].str, "%p4cc", &try[i].code);
> +    fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc");
> +    fourcc_pointer_test(&try_ch, 1, "%p4ch");
> +    fourcc_pointer_test(&try_cr, 1, "%p4cr");
> +    fourcc_pointer_test(&try_cl, 1, "%p4cl");
> +    fourcc_pointer_test(&try_cb, 1, "%p4cb");
> }
> 
> static void __init
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 56fe96319..2ac90aba2 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1781,27 +1781,53 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
>    char output[sizeof("0123 little-endian (0x01234567)")];
>    char *p = output;
>    unsigned int i;
> +    bool pixel_fmt = false;
>    u32 orig, val;
> 
> -    if (fmt[1] != 'c' || fmt[2] != 'c')
> +    if (fmt[1] != 'c')
>        return error_string(buf, end, "(%p4?)", spec);
> 
>    if (check_pointer(&buf, end, fourcc, spec))
>        return buf;
> 
>    orig = get_unaligned(fourcc);
> -    val = orig & ~BIT(31);
> +    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);
> +    }
> 
>    for (i = 0; i < sizeof(u32); i++) {
> -        unsigned char c = val >> (i * 8);
> +        unsigned char c = val >> ((3 - i) * 8);
> 
>        /* Print non-control ASCII characters as-is, dot otherwise */
>        *p++ = isascii(c) && isprint(c) ? c : '.';
>    }
> 
> -    *p++ = ' ';
> -    strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> -    p += strlen(p);
> +    if (pixel_fmt) {
> +        *p++ = ' ';
> +        strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");

Do we need strscpy here?

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

[Steven Rostedt]

On Fri, 28 Feb 2025 14:09:48 +0000
Aditya Garg <gargaditya08@live.com> wrote:

> > -    *p++ = ' ';
> > -    strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> > -    p += strlen(p);
> > +    if (pixel_fmt) {
> > +        *p++ = ' ';
> > +        strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");  
> 
> Do we need strscpy here?

Why? The size of what p points to has already been calculated to hold this:

	char output[sizeof("0123 little-endian (0x01234567)")];
	char *p = output;

Unless you changed something, it will not overflow.

-- Steve

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

[Petr Mladek]

On Thu 2025-02-27 06:30:48, Aditya Garg wrote:
> From: Hector Martin <marcan@marcan.st>
> 
> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> 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
> %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).
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
>

> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -648,6 +648,38 @@ Examples::
> 	%p4cc	Y10  little-endian (0x20303159)
> 	%p4cc	NV12 big-endian (0xb231564e)
> 
> +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.

I am a bit confused by the description like I was in the past, see
https://lore.kernel.org/r/Y3zhhLoqAOaZ7rMz@alley  ;-)

I wonder if the following sounds better:

<proposa>
Print a generic FourCC code, as both ASCII characters and its numerical
value as hexadecimal.

The generic FourCC code is always printed in the the big-endian format,
the most significant byte first. This is the opposite of V4L/DRM
FOURCCs.

The additional ``h``, ``r``, ``b``, and ``l`` specifiers define what
endianes is used to load the stored bytes. The data might be interpreted
using the host-endian, reverse-host-endian, big-endian, or little endian.
</proposal>

> +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)
> +
> Rust

The patch has been malformed. I guess that your mail client
removed spaces at the beginning of some lines.

> ----
> 
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 59dbe4f9a..056929c06 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void)
> 	software_node_unregister_node_group(group);
> }
> 
> +struct fourcc_struct {
> +	u32 code;
> +	const char *str;
> +};
> +
> +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n,
> +				       const char *fmt)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < n; i++)
> +		test(fc[i].str, fmt, &fc[i].code);
> +}
> +
> static void __init fourcc_pointer(void)
> {
> -	struct {
> -		u32 code;
> -		char *str;
> -	} const try[] = {
> +	static const struct fourcc_struct try_cc[] = {
> 		{ 0x3231564e, "NV12 little-endian (0x3231564e)", },
> 		{ 0xb231564e, "NV12 big-endian (0xb231564e)", },
> 		{ 0x10111213, ".... little-endian (0x10111213)", },
> 		{ 0x20303159, "Y10  little-endian (0x20303159)", },
> 	};
> -	unsigned int i;
> +	static const struct fourcc_struct try_ch = {
> +		0x41424344, "ABCD (0x41424344)",
> +	};
> +	static const struct fourcc_struct try_cr = {
> +		0x41424344, "DCBA (0x44434241)",
> +	};
> +	static const struct fourcc_struct try_cl = {
> +		le32_to_cpu(0x41424344), "ABCD (0x41424344)",
> +	};
> +	static const struct fourcc_struct try_cb = {
> +		be32_to_cpu(0x41424344), "ABCD (0x41424344)",
> +	};
> 
> -	for (i = 0; i < ARRAY_SIZE(try); i++)
> -		test(try[i].str, "%p4cc", &try[i].code);
> +	fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc");
> +	fourcc_pointer_test(&try_ch, 1, "%p4ch");
> +	fourcc_pointer_test(&try_cr, 1, "%p4cr");
> +	fourcc_pointer_test(&try_cl, 1, "%p4cl");
> +	fourcc_pointer_test(&try_cb, 1, "%p4cb");

Nit: I would use ARRAY_SIZE() instead of the hardcoded 1 in all cases.
     But it might be a matter of taste.

> }
> 

Otherwise, it looks good to me.

Best Regards,
Petr

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

[Aditya Garg]

> On 28 Feb 2025, at 9:29 PM, Petr Mladek <pmladek@suse.com> wrote:
> 
> On Thu 2025-02-27 06:30:48, Aditya Garg wrote:
>> From: Hector Martin <marcan@marcan.st>
>> 
>> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
>> 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
>> %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).
>> 
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> Signed-off-by: Aditya Garg <gargaditya08@live.com>
>> 
> 
>> --- a/Documentation/core-api/printk-formats.rst
>> +++ b/Documentation/core-api/printk-formats.rst
>> @@ -648,6 +648,38 @@ Examples::
>>    %p4cc    Y10  little-endian (0x20303159)
>>    %p4cc    NV12 big-endian (0xb231564e)
>> 
>> +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.
> 
> I am a bit confused by the description like I was in the past, see
> https://lore.kernel.org/r/Y3zhhLoqAOaZ7rMz@alley  ;-)
> 
> I wonder if the following sounds better:
> 
> <proposa>
> Print a generic FourCC code, as both ASCII characters and its numerical
> value as hexadecimal.
> 
> The generic FourCC code is always printed in the the big-endian format,
> the most significant byte first. This is the opposite of V4L/DRM
> FOURCCs.
> 
> The additional ``h``, ``r``, ``b``, and ``l`` specifiers define what
> endianes is used to load the stored bytes. The data might be interpreted
> using the host-endian, reverse-host-endian, big-endian, or little endian.
> </proposal>

Definitely much clear.
> 
>> +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)
>> +
>> Rust
> 
> The patch has been malformed. I guess that your mail client
> removed spaces at the beginning of some lines.

I dunno what wrong, anyways I'll send a v5 so will make sure things go right.
> 
>> ----
>> 
>> diff --git a/lib/test_printf.c b/lib/test_printf.c
>> index 59dbe4f9a..056929c06 100644
>> --- a/lib/test_printf.c
>> +++ b/lib/test_printf.c
>> @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void)
>>    software_node_unregister_node_group(group);
>> }
>> 
>> +struct fourcc_struct {
>> +    u32 code;
>> +    const char *str;
>> +};
>> +
>> +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n,
>> +                       const char *fmt)
>> +{
>> +    size_t i;
>> +
>> +    for (i = 0; i < n; i++)
>> +        test(fc[i].str, fmt, &fc[i].code);
>> +}
>> +
>> static void __init fourcc_pointer(void)
>> {
>> -    struct {
>> -        u32 code;
>> -        char *str;
>> -    } const try[] = {
>> +    static const struct fourcc_struct try_cc[] = {
>>        { 0x3231564e, "NV12 little-endian (0x3231564e)", },
>>        { 0xb231564e, "NV12 big-endian (0xb231564e)", },
>>        { 0x10111213, ".... little-endian (0x10111213)", },
>>        { 0x20303159, "Y10  little-endian (0x20303159)", },
>>    };
>> -    unsigned int i;
>> +    static const struct fourcc_struct try_ch = {
>> +        0x41424344, "ABCD (0x41424344)",
>> +    };
>> +    static const struct fourcc_struct try_cr = {
>> +        0x41424344, "DCBA (0x44434241)",
>> +    };
>> +    static const struct fourcc_struct try_cl = {
>> +        le32_to_cpu(0x41424344), "ABCD (0x41424344)",
>> +    };
>> +    static const struct fourcc_struct try_cb = {
>> +        be32_to_cpu(0x41424344), "ABCD (0x41424344)",
>> +    };
>> 
>> -    for (i = 0; i < ARRAY_SIZE(try); i++)
>> -        test(try[i].str, "%p4cc", &try[i].code);
>> +    fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc");
>> +    fourcc_pointer_test(&try_ch, 1, "%p4ch");
>> +    fourcc_pointer_test(&try_cr, 1, "%p4cr");
>> +    fourcc_pointer_test(&try_cl, 1, "%p4cl");
>> +    fourcc_pointer_test(&try_cb, 1, "%p4cb");
> 
> Nit: I would use ARRAY_SIZE() instead of the hardcoded 1 in all cases.
>     But it might be a matter of taste.

I'll make that change
> 
>> }
>> 
> 
> Otherwise, it looks good to me.
> 
> Best Regards,
> Petr