On 05/03/2021 14:58, Doug Anderson wrote:
Hi,
On Fri, Mar 5, 2021 at 2:27 AM Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> wrote:
On 27/02/2021 00:26, Douglas Anderson wrote:
The current way that cell "length" is specified for nvmem cells is a
little fuzzy. For instance, let's look at the gpu speed bin currently
in sc7180.dtsi:
gpu_speed_bin: gpu_speed_bin@1d2 {
reg = <0x1d2 0x2>;
bits = <5 8>;
};
This is an 8-bit value (as specified by the "bits" field). However,
it has a "length" of 2 (bytes), presumably because the value spans
across two bytes.
When querying this value right now, it's hard for a client to know if
they should be calling nvmem_cell_read_u16() or nvmem_cell_read_u8().
Today they must call nvmem_cell_read_u16() because the "length" of the
cell was 2 (bytes). However, if a later SoC ever came around and
didn't span across 2 bytes it would be unclear. If a later Soc
specified, for instance:
gpu_speed_bin: gpu_speed_bin@100 {
reg = <0x100 0x1>;
bits = <0 8>;
};
...then the caller would need to change to try calling
nvmem_cell_read_u8() because the u16 version would fail.
If the consumer driver is expecting the sizes to span around byte to
many bytes
I guess in my mind that's outside of the scope of what the consumer
should need to know. The consumer wants a number and they know it's
stored in nvmem. They shouldn't need to consider the bit packing
within nvmem. Imagine that have a structure definition:
struct example {
int num1:6;
int num2:6;
int num3:6;
int num4:6;
};
struct example e;
What I think you're saying is that you should need a different syntax
for accessing "e.num1" and "e.num4" (because they happen not to span
bytes) compared to accessing "e.num2" and "e.num3". As it is, C
abstracts this out and allows you not to care. You can just do:
e.num1 + e.num2 + e.num3 + e.num4
...and it works fine even though some of those span bytes and some
don't. I want the same thing.
, then, Why not just call nvmem_cell_read() which should also
return you how many bytes it has read!
See my response to patch #1. This requires open-coding a small but
still non-trivial bit of code for all consumers. It should be in the
core.
I agree with that this should be in core!
But changing the exiting behavior of the apis is the one am against!
For example if we are reading a fixed size UUID or some cell like that
we would want to validate it, allowing flexible sizes would not catch
errors.
Also if its variable size then which apis should consumer use, should he
use u32 or u16 based, this adds more confusion to this!
Let's solve this by allowing clients to read a "larger" value. We'll
just fill it in with 0.
That is misleading the consumer! If the consumer is expecting a u16 or
u32, cell size should be of that size!!
If you think it's confusing to change the behavior of the existing
functions, would you be opposed to me adding a new function like
nvmem_cell_read_le_u32_or_smaller() (or provide me a better name) that
would be flexible like this?
This should be perfectly okay!
may be something like:
int nvmem_read_variable_cell(struct device *dev, const char *cell_id,
void *buf, size_t sz_min, size_t sz_max);
It should return number of bytes it read and fail if cell size is less
then sz_min!
--srini
-Doug
