* [PATCH v2] nvmem: core: don't consider subnodes not matching binding
@ 2020-03-23 15:28 Ahmad Fatoum
2020-04-06 11:29 ` Ahmad Fatoum
2020-04-06 12:33 ` Srinivas Kandagatla
0 siblings, 2 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2020-03-23 15:28 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: kernel, Christian Eggers, Ahmad Fatoum, linux-kernel
The nvmem cell binding applies to objects which match "^.*@[0-9a-f]+$",
but so far the driver has matched all objects and failed if they didn't
have the expected properties.
The driver's behavior in this regard precludes future extension of
EEPROMs by child nodes other than nvmem and clashes with the barebox
bootloader binding that extends the fixed-partitions MTD binding to
EEPROMs as it tries to interpret the "fixed-partitions"-compatible
partitions node as a nvmem cell.
Solve this issue by skipping all subnodes that don't contain an @.
This still allows for cell names like `partitions@0,0', but this
is much less likely to cause future collisions.
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
use ->full_name instead of ->name as to not break existing correct
cells (Christian)
---
drivers/nvmem/core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index ef326f243f36..f051051fb1a8 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -278,6 +278,8 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
parent = dev->of_node;
for_each_child_of_node(parent, child) {
+ if (!strchr(kbasename(child->full_name), '@'))
+ continue;
addr = of_get_property(child, "reg", &len);
if (!addr || (len < 2 * sizeof(u32))) {
dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmem: core: don't consider subnodes not matching binding
2020-03-23 15:28 [PATCH v2] nvmem: core: don't consider subnodes not matching binding Ahmad Fatoum
@ 2020-04-06 11:29 ` Ahmad Fatoum
2020-04-06 12:33 ` Srinivas Kandagatla
1 sibling, 0 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2020-04-06 11:29 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: kernel, Christian Eggers, linux-kernel
Hello Srinivas,
On 3/23/20 4:28 PM, Ahmad Fatoum wrote:
> The nvmem cell binding applies to objects which match "^.*@[0-9a-f]+$",
> but so far the driver has matched all objects and failed if they didn't
> have the expected properties.
>
> The driver's behavior in this regard precludes future extension of
> EEPROMs by child nodes other than nvmem and clashes with the barebox
> bootloader binding that extends the fixed-partitions MTD binding to
> EEPROMs as it tries to interpret the "fixed-partitions"-compatible
> partitions node as a nvmem cell.
>
> Solve this issue by skipping all subnodes that don't contain an @.
>
> This still allows for cell names like `partitions@0,0', but this
> is much less likely to cause future collisions.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> v1 -> v2:
> use ->full_name instead of ->name as to not break existing correct
> cells (Christian)
> ---
> drivers/nvmem/core.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index ef326f243f36..f051051fb1a8 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -278,6 +278,8 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
> parent = dev->of_node;
>
> for_each_child_of_node(parent, child) {
> + if (!strchr(kbasename(child->full_name), '@'))
> + continue;
> addr = of_get_property(child, "reg", &len);
> if (!addr || (len < 2 * sizeof(u32))) {
> dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
>
gentle ping. I'd also appreciate pointers if you wish to have this addressed
differently.
Cheers
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmem: core: don't consider subnodes not matching binding
2020-03-23 15:28 [PATCH v2] nvmem: core: don't consider subnodes not matching binding Ahmad Fatoum
2020-04-06 11:29 ` Ahmad Fatoum
@ 2020-04-06 12:33 ` Srinivas Kandagatla
2020-04-06 14:20 ` Ahmad Fatoum
1 sibling, 1 reply; 6+ messages in thread
From: Srinivas Kandagatla @ 2020-04-06 12:33 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: kernel, Christian Eggers, linux-kernel
On 23/03/2020 15:28, Ahmad Fatoum wrote:
> The nvmem cell binding applies to objects which match "^.*@[0-9a-f]+$",
> but so far the driver has matched all objects and failed if they didn't
> have the expected properties.
>
> The driver's behavior in this regard precludes future extension of
> EEPROMs by child nodes other than nvmem and clashes with the barebox
> bootloader binding that extends the fixed-partitions MTD binding to
> EEPROMs as it tries to interpret the "fixed-partitions"-compatible
> partitions node as a nvmem cell.
>
> Solve this issue by skipping all subnodes that don't contain an @.
>
> This still allows for cell names like `partitions@0,0', but this
NACK.. thats nasty hack!
Its standard practice as per device tree specs to have nodes name as
"node-name@unit-address"
Ref:
https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf
> is much less likely to cause future collisions.
There have been discussions on this topic in the past at:
https://patchwork.ozlabs.org/patch/890741/
We need to come up with a better solution!
--srini
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> v1 -> v2:
> use ->full_name instead of ->name as to not break existing correct
> cells (Christian)
> ---
> drivers/nvmem/core.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index ef326f243f36..f051051fb1a8 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -278,6 +278,8 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
> parent = dev->of_node;
>
> for_each_child_of_node(parent, child) {
> + if (!strchr(kbasename(child->full_name), '@'))
> + continue;
> addr = of_get_property(child, "reg", &len);
> if (!addr || (len < 2 * sizeof(u32))) {
> dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmem: core: don't consider subnodes not matching binding
2020-04-06 12:33 ` Srinivas Kandagatla
@ 2020-04-06 14:20 ` Ahmad Fatoum
2020-04-15 8:05 ` Ahmad Fatoum
0 siblings, 1 reply; 6+ messages in thread
From: Ahmad Fatoum @ 2020-04-06 14:20 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: kernel, Christian Eggers, linux-kernel, devicetree
Hello,
On 4/6/20 2:33 PM, Srinivas Kandagatla wrote:
>
>
> On 23/03/2020 15:28, Ahmad Fatoum wrote:
>> The nvmem cell binding applies to objects which match "^.*@[0-9a-f]+$",
>> but so far the driver has matched all objects and failed if they didn't
>> have the expected properties.
>>
>> The driver's behavior in this regard precludes future extension of
>> EEPROMs by child nodes other than nvmem and clashes with the barebox
>> bootloader binding that extends the fixed-partitions MTD binding to
>> EEPROMs as it tries to interpret the "fixed-partitions"-compatible
>> partitions node as a nvmem cell.
>>
>> Solve this issue by skipping all subnodes that don't contain an @.
>>
>> This still allows for cell names like `partitions@0,0', but this
> NACK.. thats nasty hack!
> Its standard practice as per device tree specs to have nodes name as "node-name@unit-address"
>
> Ref: https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf
I didn't say otherwise. I just argued if we verify there's a @ symbol in the
node name, before considering whether it is a NVMEM cell, we will skip fixed-partitions
while adhering to the NVMEM cell binding.
>> is much less likely to cause future collisions.
>
> There have been discussions on this topic in the past at:
>
> https://patchwork.ozlabs.org/patch/890741/
>
> We need to come up with a better solution!
Thanks for the link, I didn't find it when I searched whether this has
come up before.
As you probably noticed, your suggested approach there wouldn't work for me though,
because this time it can't be worked around in the MTD driver.
If the suggestion here is not palatable, how do you think about (in
my preferred order):
- If the cell has a compatible node, it must be "nvmem-cell".
Otherwise, a cell with a compatible node is skipped.
- Adding a nvmem cell container with a compatible and making support for
the old binding configurable
- Skipping cells that are malformed and lack a reg = < > property _without_
error
Cheers
Ahmad
>
> --srini
>
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> v1 -> v2:
>> use ->full_name instead of ->name as to not break existing correct
>> cells (Christian)
>> ---
>> drivers/nvmem/core.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index ef326f243f36..f051051fb1a8 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -278,6 +278,8 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
>> parent = dev->of_node;
>> for_each_child_of_node(parent, child) {
>> + if (!strchr(kbasename(child->full_name), '@'))
>> + continue;
>> addr = of_get_property(child, "reg", &len);
>> if (!addr || (len < 2 * sizeof(u32))) {
>> dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmem: core: don't consider subnodes not matching binding
2020-04-06 14:20 ` Ahmad Fatoum
@ 2020-04-15 8:05 ` Ahmad Fatoum
2020-04-15 8:46 ` Srinivas Kandagatla
0 siblings, 1 reply; 6+ messages in thread
From: Ahmad Fatoum @ 2020-04-15 8:05 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: devicetree, Christian Eggers, linux-kernel, kernel
On 4/6/20 4:20 PM, Ahmad Fatoum wrote:
> Hello,
>
> On 4/6/20 2:33 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 23/03/2020 15:28, Ahmad Fatoum wrote:
>>> The nvmem cell binding applies to objects which match "^.*@[0-9a-f]+$",
>>> but so far the driver has matched all objects and failed if they didn't
>>> have the expected properties.
>>>
>>> The driver's behavior in this regard precludes future extension of
>>> EEPROMs by child nodes other than nvmem and clashes with the barebox
>>> bootloader binding that extends the fixed-partitions MTD binding to
>>> EEPROMs as it tries to interpret the "fixed-partitions"-compatible
>>> partitions node as a nvmem cell.
>>>
>>> Solve this issue by skipping all subnodes that don't contain an @.
>>>
>>> This still allows for cell names like `partitions@0,0', but this
>> NACK.. thats nasty hack!
>> Its standard practice as per device tree specs to have nodes name as "node-name@unit-address"
>>
>> Ref: https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf
>
> I didn't say otherwise. I just argued if we verify there's a @ symbol in the
> node name, before considering whether it is a NVMEM cell, we will skip fixed-partitions
> while adhering to the NVMEM cell binding.
>
>>> is much less likely to cause future collisions.
>>
>> There have been discussions on this topic in the past at:
>>
>> https://patchwork.ozlabs.org/patch/890741/
>>
>> We need to come up with a better solution!
>
> Thanks for the link, I didn't find it when I searched whether this has
> come up before.
>
> As you probably noticed, your suggested approach there wouldn't work for me though,
> because this time it can't be worked around in the MTD driver.
> If the suggestion here is not palatable, how do you think about (in
> my preferred order):
>
> - If the cell has a compatible node, it must be "nvmem-cell".
> Otherwise, a cell with a compatible node is skipped.
>
> - Adding a nvmem cell container with a compatible and making support for
> the old binding configurable
>
> - Skipping cells that are malformed and lack a reg = < > property _without_
> error
gentle ping. I can prepare another patch if you indicate which approach
you'd prefer.
>
> Cheers
> Ahmad
>
>>
>> --srini
>>
>>>
>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>> ---
>>> v1 -> v2:
>>> use ->full_name instead of ->name as to not break existing correct
>>> cells (Christian)
>>> ---
>>> drivers/nvmem/core.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>> index ef326f243f36..f051051fb1a8 100644
>>> --- a/drivers/nvmem/core.c
>>> +++ b/drivers/nvmem/core.c
>>> @@ -278,6 +278,8 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
>>> parent = dev->of_node;
>>> for_each_child_of_node(parent, child) {
>>> + if (!strchr(kbasename(child->full_name), '@'))
>>> + continue;
>>> addr = of_get_property(child, "reg", &len);
>>> if (!addr || (len < 2 * sizeof(u32))) {
>>> dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
>>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] nvmem: core: don't consider subnodes not matching binding
2020-04-15 8:05 ` Ahmad Fatoum
@ 2020-04-15 8:46 ` Srinivas Kandagatla
0 siblings, 0 replies; 6+ messages in thread
From: Srinivas Kandagatla @ 2020-04-15 8:46 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: devicetree, Christian Eggers, linux-kernel, kernel
On 15/04/2020 09:05, Ahmad Fatoum wrote:
>> - If the cell has a compatible node, it must be "nvmem-cell".
>> Otherwise, a cell with a compatible node is skipped.
>>
This one sounds more sensible!
If we go this way, it needs to be properly documented in the bindings too.
>> - Adding a nvmem cell container with a compatible and making support for
>> the old binding configurable
Older bindings should be still supported but we should encourage to use
compatible string for new additions.
>>
>> - Skipping cells that are malformed and lack a reg = < > property_without_
>> error
Currently we only support Offset and Size with address cell and size
cells of 1. Adding an extra check here to see if its malformed makes
sense too!
Thanks,
srini
> gentle ping. I can prepare another patch if you indicate which approach
> you'd prefer.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-15 8:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-23 15:28 [PATCH v2] nvmem: core: don't consider subnodes not matching binding Ahmad Fatoum
2020-04-06 11:29 ` Ahmad Fatoum
2020-04-06 12:33 ` Srinivas Kandagatla
2020-04-06 14:20 ` Ahmad Fatoum
2020-04-15 8:05 ` Ahmad Fatoum
2020-04-15 8:46 ` Srinivas Kandagatla
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.