Re: [PATCH 04/10] common: add barebox TLV support

[Sascha Hauer <s.hauer@pengutronix.de>]

On Fri, Apr 11, 2025 at 09:40:39AM +0200, Ahmad Fatoum wrote:
> barebox TLV is a scheme for storing factory data on non-volatile
> storage. Unlike state, it's meant to be read-only and if content
> is limited to already specified tags, it can be extended later on,
> without modifying the bootloader binary.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> +struct tlv_mapping barebox_tlv_v1_mappings[] = {
> +	{ 0x0002, tlv_format_str, "device-hardware-release" },
> +	{ 0x0003, tlv_format_dec, "factory-timestamp" },

The numbers in the examples suggest that the format is in epoch time,
but shouldn't this be documented?

> +	{ 0x0004, tlv_handle_serial, "device-serial-number"},
> +	{ 0x0005, tlv_format_dec, "modification" },
> +	{ 0x0006, tlv_format_str, "featureset" },

I think things like "device-hardware-release", "modification" and "featureset"
need (better) documentation, at least when they are supposed to be generic.

> +	{ 0x0007, tlv_format_str, "pcba-serial-number"},
> +	{ 0x0008, tlv_format_str, "pcba-hardware-release"},

Assuming 'pcb' is for printed circuit board, what is 'a'?

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |