Message-ID: <5bfc6886-b9ac-4bd4-b651-f0c7f90a59f4@linaro.org>
Date: Mon, 30 Dec 2024 13:50:47 +0000
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Jennifer Berringer <jberring@...hat.com>,
 Sebastian Reichel <sre@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Maxime Ripard <mripard@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 2/3] nvmem: core: add nvmem_cell_write_variable_u32()



On 20/12/2024 19:39, Jennifer Berringer wrote:
> 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.
> 

Writes happen in the order of data that is provided, and read in the 
same order that is stored in NVMEM.  Reading from nvmem storage can be 
translated into whatever order that consumer wants which is what the 
read helpers are doing. On the other hand writes should be written in 
the same order as its received.

Note that the order in which EEPROM stores data might not be the same 
order as CPU.

Adding Endianess in writing to a common code is a problem. If you are 
certain that the NVMEM needs particular order(le/be) then either convert 
(cpu_to_le/cpu_to_be) the data before it hits write call.



> 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.

there are two things, one is the variable size u32, other is changing 
the endainess. Am happy with the former one as long as the consumer is 
taking care of order.

But the later one is No, as explained.

--srini
> 

Powered by blists - more mailing lists