On 15.09.2023 19:59, Jeffrey Hugo wrote:
> Shared memory items are assigned a globally unique ID and almost always
> have a defined structure which is stored in the shared memory. Document
> assigned IDs and corresponding structures.
>
> Signed-off-by: Jeffrey Hugo <quic_jhugo@xxxxxxxxxxx>
> ---
>
> Konrad, before I get too far into this, I was hoping for some early
> feedback since this documentation is a request that you made.
>
> Please let me know if this is aligned with what you were wanting.
This is super nice, I'll just leave you with a few nitty
code-style pointers.
> +/* fixed items - these have a static position in shared memory */
Meaningless but eye-pleasing - comments should start with an
uppercase letter
> +#define SMEM_PROC_COMM 0
In many places like this where we essentially have a firmware
interface, it is customary to go like this:
#define FWNAME_CALL_SOMETHING_FOO (magicval1)
struct fwname_something_foo {
[...]
};
#define FWNAME_CALL_SOMETHING_BAR (magicval2)
struct fwname_something_bar {
[...]
};
This makes matching the call/function/whatev name with what
it expects/returns easier for a typical human
[...]
> +/* Legacy communication protocol between "Apps" and "Modem" processors */
The comments explaining what this does are a great addition, I
think in the spirit of that previous suggestion, they could go
like this:
/* blah blah blah yes yes yes */
#define FWNAME_CALL_SOMETHING_FOO (magicval1)
struct fwname_something_foo {
[...]
};
/* blah blah something something */
#define FWNAME_CALL_SOMETHING_BAR (magicval2)
struct fwname_something_bar {
[...]
};
[...]
> +/* SMEM_ALLOCATION_TABLE is an array of these structures. 512 elements in the array. */
> +struct smem_heap_entry {
> + __le32 allocated;
> + __le32 offset;
> + __le32 size;
> + __le32 reserved; /* bits 1:0 reserved, bits 31:2 aux smem base addr */
If we have an integer split into bitfields or similar, something
like this would make it both readable and usable in code:
struct smem_heap_entry {
__le32 allocated;
__le32 offset;
__le32 size;
__le32 reserved;
#define SMEM_HEAP_ENTRY_BASE_ADDR GENMASK(31, 2)
#define SMEM_HEAP_ENTRY_RESERVED GENMASK(1, 0)
};
[...]
> +#define FLASH_PART_MAGIC1 0x55EE73AA
> +#define FLASH_PART_MAGIC2 0xE35EBDDB
> +#define FLASH_PTABLE_V3 3
> +#define FLASH_PTABLE_V4 4
> +#define FLASH_PTABLE_MAX_PARTS_V3 16
> +#define FLASH_PTABLE_MAX_PARTS_V4 32
> +#define FLASH_PTABLE_ENTRY_NAME_SIZE 16
Similarly having such magic values under the corresponding struct
member would make things more obvious
> +
> +struct flash_partition_entry {
> + char name[FLASH_PTABLE_ENTRY_NAME_SIZE];
> + __le32 offset; /* Offset in blocks from beginning of device */
> + __le32 length; /* Length of the partition in blocks */
> + u8 attr; /* Flags for this partition */
Comments like this are welcome too, particularly where things
are "very not obvious", like here where length is in blocks
and not bytes.
But if we had something like "u32 flags" followed by a bunch
of defines, that's self-explanatory.
Konrad
