| Message ID | 20231005155907.2701706-1-miquel.raynal@bootlin.com |
|---|---|
| Headers | show |
| Series | NVMEM cells in sysfs | expand |
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
On Thu, 05 Oct 2023 17:59:01 +0200, Miquel Raynal wrote: > This helper is really handy to create unique device names based on their > device tree path, we may need it outside of the OF core (in the NVMEM > subsystem) so let's export it. As this helper has nothing patform > specific, let's move it to of/device.c instead of of/platform.c so we > can add its prototype to of_device.h. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/of/device.c | 41 +++++++++++++++++++++++++++++++++++++++ > drivers/of/platform.c | 40 -------------------------------------- > include/linux/of_device.h | 6 ++++++ > 3 files changed, 47 insertions(+), 40 deletions(-) > Acked-by: Rob Herring <robh@kernel.org>
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