[v12,2/7] nvmem: Clarify the situation when there is no DT node available

  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.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/nvmem/core.c | 3 +++
 1 file changed, 3 insertions(+)
  

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.

Given that for_each_child_of_node() is NULL-safe I think code from this
patch is redundant.

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@gmail.com/t/

Maybe with above patch finally things will get more clear and we don't
need this PATCH after all?


> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/nvmem/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index eaf6a3fe8ca6..286efd3f5a31 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -743,6 +743,9 @@ static int nvmem_add_cells_from_dt(struct
> nvmem_device *nvmem, struct device_nod
> 
>  static int nvmem_add_cells_from_legacy_of(struct nvmem_device *nvmem)
>  {
> +	if (!nvmem->dev.of_node)
> +		return 0;
> +
>  	return nvmem_add_cells_from_dt(nvmem, nvmem->dev.of_node);
>  }
  

Hi Rafał,

rafal@milecki.pl 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@gmail.com/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?

Thanks,
Miquèl
  

One comment below

On 2023-10-06 18:32, Miquel Raynal wrote:
> rafal@milecki.pl 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.
  

Hi Rafał,

rafal@milecki.pl wrote on Sat, 07 Oct 2023 18:09:06 +0200:

> One comment below
> 
> On 2023-10-06 18:32, Miquel Raynal wrote:
> > rafal@milecki.pl 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?

Yeah would work as well. But I will just get rid of this, with your
other patch that solves the fact that of_node will be there with mtd
devices, it's no longer relevant.

Thanks,
Miquèl
  

On 06/10/2023 17:32, Miquel Raynal wrote:
> Hi Rafał,
> 
> rafal@milecki.pl 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@gmail.com/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