On 06/10/2023 17:32, Miquel Raynal wrote:
Hi Rafał,
rafal@xxxxxxxxxx wrote on Fri, 06 Oct 2023 13:41:52 +0200:
On 2023-10-05 17:59, Miquel Raynal wrote:
At a first look it might seem that the presence of the of_node pointer
in the nvmem device does not matter much, but in practice, after > looking
deep into the DT core, nvmem_add_cells_from_dt() will simply and always
return NULL if this field is not provided. As most mtd devices don't
populate this field (this could evolve later), it means none of their
children cells will be populated unless no_of_node is explicitly set to
false. In order to clarify the logic, let's add clear check at the
beginning of this helper.
I'm somehow confused by above explanation and code too. I read it
carefully 5 times but I can't see what exactly this change helps with.
At first look at nvmem_add_cells_from_legacy_of() I can see it uses
"of_node" so I don't really agree with "it might seem that the presence
of the of_node pointer in the nvmem device does not matter much".
You really don't need to look deep into DT core (actually you don't have
to look into it at all) to understand that nvmem_add_cells_from_dt()
will return 0 (nitpicking: not NULL) for a NULL pointer. It's all made
of for_each_child_of_node(). Obviously it does nothing if there is
nothing to loop over.
That was not obvious to me as I thought it would start from /, which I
think some other function do when you don't provide a start node.
Given that for_each_child_of_node() is NULL-safe I think code from this
patch is redundant.
I didn't say it was not safe, just not explicit.
Later you mention "no_of_node" which I agree to be a very non-intuitive
config option. As pointed in another thread I already sent:
[PATCH] Revert "nvmem: add new config option"
https://lore.kernel.org/lkml/ba3c419a-6511-480a-b5f2-6c418f9c02e7@xxxxxxxxx/t/
I actually wanted to find again that patch and could not get my hands on
it, but it is probably a much better fix than my other mtd patch, I
agree with you.
Maybe with above patch finally things will get more clear and we don't
need this PATCH after all?
Yes. Srinivas, what are your plans for the above patch?
for_each_child_of_node is null safe, so this patch is really not adding
much value TBH.
--srini
Thanks,
Miquèl
