Re: [PATCH v3 2/3] nvmem: core: add nvmem_cell_write_variable_u32()

[Jennifer Berringer <jberring@redhat.com>]

Thanks for the feedback and for accepting my first patch.

On 12/14/24 10:07, Srinivas Kandagatla wrote:
>> + * nvmem_cell_write_variable_u32() - Write up to 32-bits of data as a host-endian number
> 
>> + *
>> + * @cell: nvmem cell to be written.
>> + * @val: Value to be written which may be truncated.
>> + *
>> + * Return: length of bytes written or negative on failure.
>> + */
>> +int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val)
> 
> This new API is confusing, as writing to cell should in the same order of the data. Doing any data manipulation within the api is confusing to users.
> 
> If the intention is to know the size of cell before writing, then best way to address this is to provide the cell size visibility to the consumer.
> 
> --srini

My intention was to mirror the existing functions in this file, hoping that would make it less confusing rather than more. nvmem_cell_read_variable_le_u32() already similarly has consumers accessing the contents of a cell without knowing its size, silently filling any bytes not read with zero. The function I add doesn't change the order of the bytes. The existing read_variable_le functions (in contrast) byte swap from little-endian to the CPU's endianness and indicate this in the function name. Otherwise, I believe the function I add here is what would be expected from a write equivalent. Its return value also gives the caller the actual cell size upon success.

The only manipulation done to the data here is truncating to ignore the highest-order bytes. This behavior should match the below example unless the size is 3 bytes, which my patch should handle but the below example can't.

if (entry->bytes == sizeof(u32)) {
    return __nvmem_cell_entry_write(entry, &val, sizeof(u32));
} else if (entry->bytes == sizeof(u16)) {
    u16 val_16 = (u16) val;
    return __nvmem_cell_entry_write(entry, &val_16, sizeof(u16));
} else if (entry->bytes == sizeof(u8)) {
    u8 val_8 = (u8) val;
    return __nvmem_cell_entry_write(entry, &val_8, sizeof(u8));
} else {
    return -EINVAL;
}

Still, if you prefer, I can send new patches that add a function to get the cell size and put any truncation behavior needed by nvmem-reboot-mode into that source file instead.