One comment below
On 2023-10-06 18:32, Miquel Raynal wrote:
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.
What about documenting that function instead of adding redundant code?
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.
--
Rafał Miłecki
