All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] of: get of_root directly in place of of_find_node_by_path("/")
@ 2018-03-03  1:46 Tyrel Datwyler
  2018-03-07 20:38 ` Rob Herring
  0 siblings, 1 reply; 9+ messages in thread
From: Tyrel Datwyler @ 2018-03-03  1:46 UTC (permalink / raw)
  To: robh+dt; +Cc: frowand.list, devicetree, Tyrel Datwyler

There are a couple places in drivers/of where the root node is found
with a of_find_node_by_path("/") call. This call just returns
of_node_get(of_root) when the the path "/" is passed as a parameter.
So, lets fixup these instances to just do that instead.

Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
 drivers/of/base.c     | 2 +-
 drivers/of/platform.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ad28de9..94c1b4d 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -455,7 +455,7 @@ int of_machine_is_compatible(const char *compat)
 	struct device_node *root;
 	int rc = 0;
 
-	root = of_find_node_by_path("/");
+	root = of_node_get(of_root);
 	if (root) {
 		rc = of_device_is_compatible(root, compat);
 		of_node_put(root);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index c00d81d..1642c11 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -409,7 +409,7 @@ int of_platform_bus_probe(struct device_node *root,
 	struct device_node *child;
 	int rc = 0;
 
-	root = root ? of_node_get(root) : of_find_node_by_path("/");
+	root = root ? of_node_get(root) : of_node_get(of_root);
 	if (!root)
 		return -EINVAL;
 
@@ -461,7 +461,7 @@ int of_platform_populate(struct device_node *root,
 	struct device_node *child;
 	int rc = 0;
 
-	root = root ? of_node_get(root) : of_find_node_by_path("/");
+	root = root ? of_node_get(root) : of_node_get(of_root);
 	if (!root)
 		return -EINVAL;
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] of: get of_root directly in place of of_find_node_by_path("/")
  2018-03-03  1:46 [PATCH] of: get of_root directly in place of of_find_node_by_path("/") Tyrel Datwyler
@ 2018-03-07 20:38 ` Rob Herring
  2018-03-07 22:14   ` Tyrel Datwyler
  0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2018-03-07 20:38 UTC (permalink / raw)
  To: Tyrel Datwyler; +Cc: frowand.list, devicetree

On Fri, Mar 02, 2018 at 08:46:44PM -0500, Tyrel Datwyler wrote:
> There are a couple places in drivers/of where the root node is found
> with a of_find_node_by_path("/") call. This call just returns
> of_node_get(of_root) when the the path "/" is passed as a parameter.
> So, lets fixup these instances to just do that instead.
> 
> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
> ---
>  drivers/of/base.c     | 2 +-
>  drivers/of/platform.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ad28de9..94c1b4d 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -455,7 +455,7 @@ int of_machine_is_compatible(const char *compat)
>  	struct device_node *root;
>  	int rc = 0;
>  
> -	root = of_find_node_by_path("/");
> +	root = of_node_get(of_root);

The former seems more readable to me. I'll wait and see if Frank has any 
comments on this change.

Rob

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of: get of_root directly in place of of_find_node_by_path("/")
  2018-03-07 20:38 ` Rob Herring
@ 2018-03-07 22:14   ` Tyrel Datwyler
  2018-03-07 22:21     ` Rob Herring
  0 siblings, 1 reply; 9+ messages in thread
From: Tyrel Datwyler @ 2018-03-07 22:14 UTC (permalink / raw)
  To: Rob Herring; +Cc: frowand.list, devicetree

On 03/07/2018 12:38 PM, Rob Herring wrote:
> On Fri, Mar 02, 2018 at 08:46:44PM -0500, Tyrel Datwyler wrote:
>> There are a couple places in drivers/of where the root node is found
>> with a of_find_node_by_path("/") call. This call just returns
>> of_node_get(of_root) when the the path "/" is passed as a parameter.
>> So, lets fixup these instances to just do that instead.
>>
>> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
>> ---
>>  drivers/of/base.c     | 2 +-
>>  drivers/of/platform.c | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index ad28de9..94c1b4d 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -455,7 +455,7 @@ int of_machine_is_compatible(const char *compat)
>>  	struct device_node *root;
>>  	int rc = 0;
>>  
>> -	root = of_find_node_by_path("/");
>> +	root = of_node_get(of_root);
> 
> The former seems more readable to me. I'll wait and see if Frank has any 
> comments on this change.

Fair enough. I find them equally readable now that we have a well named/defined global, namely "of_root",  pointing at the device tree root. My primary goal was saving a function call, but I wasn't sure if anybody really cares enough. A quick check shows ~77 uses of "of_find_node_by_path("/")" in the kernel. Figured I'd start here and see what the consensus was.

-Tyrel

> 
> Rob
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of: get of_root directly in place of of_find_node_by_path("/")
  2018-03-07 22:14   ` Tyrel Datwyler
@ 2018-03-07 22:21     ` Rob Herring
  2018-03-07 23:59       ` Frank Rowand
  2018-03-08  1:36       ` Tyrel Datwyler
  0 siblings, 2 replies; 9+ messages in thread
From: Rob Herring @ 2018-03-07 22:21 UTC (permalink / raw)
  To: Tyrel Datwyler; +Cc: Frank Rowand, devicetree

On Wed, Mar 7, 2018 at 4:14 PM, Tyrel Datwyler
<tyreld@linux.vnet.ibm.com> wrote:
> On 03/07/2018 12:38 PM, Rob Herring wrote:
>> On Fri, Mar 02, 2018 at 08:46:44PM -0500, Tyrel Datwyler wrote:
>>> There are a couple places in drivers/of where the root node is found
>>> with a of_find_node_by_path("/") call. This call just returns
>>> of_node_get(of_root) when the the path "/" is passed as a parameter.
>>> So, lets fixup these instances to just do that instead.
>>>
>>> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
>>> ---
>>>  drivers/of/base.c     | 2 +-
>>>  drivers/of/platform.c | 4 ++--
>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>> index ad28de9..94c1b4d 100644
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -455,7 +455,7 @@ int of_machine_is_compatible(const char *compat)
>>>      struct device_node *root;
>>>      int rc = 0;
>>>
>>> -    root = of_find_node_by_path("/");
>>> +    root = of_node_get(of_root);
>>
>> The former seems more readable to me. I'll wait and see if Frank has any
>> comments on this change.
>
> Fair enough. I find them equally readable now that we have a well named/defined global, namely "of_root",  pointing at the device tree root. My primary goal was saving a function call, but I wasn't sure if anybody really cares enough. A quick check shows ~77 uses of "of_find_node_by_path("/")" in the kernel. Figured I'd start here and see what the consensus was.

As far as outside drivers/of/ is concerned, we certainly don't want to
expose global variables. But for most uses, you shouldn't need the
root node pointer because most iterating or searching functions treat
NULL as root.

Rob

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of: get of_root directly in place of of_find_node_by_path("/")
  2018-03-07 22:21     ` Rob Herring
@ 2018-03-07 23:59       ` Frank Rowand
  2018-03-08  1:14         ` Rob Herring
  2018-03-08  1:36       ` Tyrel Datwyler
  1 sibling, 1 reply; 9+ messages in thread
From: Frank Rowand @ 2018-03-07 23:59 UTC (permalink / raw)
  To: Rob Herring, Tyrel Datwyler; +Cc: devicetree

On 03/07/18 14:21, Rob Herring wrote:
> On Wed, Mar 7, 2018 at 4:14 PM, Tyrel Datwyler
> <tyreld@linux.vnet.ibm.com> wrote:
>> On 03/07/2018 12:38 PM, Rob Herring wrote:
>>> On Fri, Mar 02, 2018 at 08:46:44PM -0500, Tyrel Datwyler wrote:
>>>> There are a couple places in drivers/of where the root node is found
>>>> with a of_find_node_by_path("/") call. This call just returns
>>>> of_node_get(of_root) when the the path "/" is passed as a parameter.
>>>> So, lets fixup these instances to just do that instead.
>>>>
>>>> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
>>>> ---
>>>>  drivers/of/base.c     | 2 +-
>>>>  drivers/of/platform.c | 4 ++--
>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>> index ad28de9..94c1b4d 100644
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>> @@ -455,7 +455,7 @@ int of_machine_is_compatible(const char *compat)
>>>>      struct device_node *root;
>>>>      int rc = 0;
>>>>
>>>> -    root = of_find_node_by_path("/");
>>>> +    root = of_node_get(of_root);
>>>
>>> The former seems more readable to me. I'll wait and see if Frank has any
>>> comments on this change.>>
>> Fair enough. I find them equally readable now that we have a well
>> named/defined global, namely "of_root", pointing at the device tree
>> root. My primary goal was saving a function call, but I wasn't sure
>> if anybody really cares enough. A quick check shows ~77 uses of
>> "of_find_node_by_path("/")" in the kernel.

That is an interesting result.

>> Figured I'd start here
>> and see what the consensus was.
> 
> As far as outside drivers/of/ is concerned, we certainly don't want to
> expose global variables. But for most uses, you shouldn't need the
> root node pointer because most iterating or searching functions treat
> NULL as root.
> 
> Rob

For a simple answer, I'll go along with Rob here.


"Here, hold my beer and watch this!"

Things one should never say.  :-)

Please don't run with this following idea yet.  This could open a can
of worms that I've been trying to leave closed until I have some time
scheduled to handle the fall out.

The core devicetree code was not written with the concept of overlays
in mind.  When the first portions of the overlay code were accepted,
the architectural issues of the extending existing core code were not
first addressed.  I don't even know if they were considered or
discussed because I was not involved at the time.

Some key things that were not updated to account for overlays:

  1) locking (protection of the live devicetree data structures)

  2) pointers to live devicetree data structures being handed out
     to kernel code outside the core devicetree code

One result of this is that after an overlay is applied, we can never
free the memory containing the overlay FDT and the overlay expanded
tree, because we do not know if any other part of the kernel has a
pointer into the overlay data.

I haven't dug into this in any depth yet, but it seems like it should
be possible to create a new set of devicetree APIS, called by drivers
and other parts of the kernel that retrieve information from the
devicetree.  This API can not return pointers into the core devicetree
data structures.  Thus no node pointers, no property pointers, no
property value pointers.

We may find some easy solution for this, but I suspect not.

That's a long winded way of saying that maybe we should not spend
effort on cleaning up non-critical issues with the way the
current API is being used, _if_ the current API is going to
be replaced long term.

-Frank

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of: get of_root directly in place of of_find_node_by_path("/")
  2018-03-07 23:59       ` Frank Rowand
@ 2018-03-08  1:14         ` Rob Herring
  2018-03-08  1:36           ` Frank Rowand
  0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2018-03-08  1:14 UTC (permalink / raw)
  To: Frank Rowand; +Cc: Tyrel Datwyler, devicetree

On Wed, Mar 7, 2018 at 5:59 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 03/07/18 14:21, Rob Herring wrote:
>> On Wed, Mar 7, 2018 at 4:14 PM, Tyrel Datwyler
>> <tyreld@linux.vnet.ibm.com> wrote:
>>> On 03/07/2018 12:38 PM, Rob Herring wrote:
>>>> On Fri, Mar 02, 2018 at 08:46:44PM -0500, Tyrel Datwyler wrote:
>>>>> There are a couple places in drivers/of where the root node is found
>>>>> with a of_find_node_by_path("/") call. This call just returns
>>>>> of_node_get(of_root) when the the path "/" is passed as a parameter.
>>>>> So, lets fixup these instances to just do that instead.
>>>>>
>>>>> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
>>>>> ---
>>>>>  drivers/of/base.c     | 2 +-
>>>>>  drivers/of/platform.c | 4 ++--
>>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>>> index ad28de9..94c1b4d 100644
>>>>> --- a/drivers/of/base.c
>>>>> +++ b/drivers/of/base.c
>>>>> @@ -455,7 +455,7 @@ int of_machine_is_compatible(const char *compat)
>>>>>      struct device_node *root;
>>>>>      int rc = 0;
>>>>>
>>>>> -    root = of_find_node_by_path("/");
>>>>> +    root = of_node_get(of_root);
>>>>
>>>> The former seems more readable to me. I'll wait and see if Frank has any
>>>> comments on this change.>>
>>> Fair enough. I find them equally readable now that we have a well
>>> named/defined global, namely "of_root", pointing at the device tree
>>> root. My primary goal was saving a function call, but I wasn't sure
>>> if anybody really cares enough. A quick check shows ~77 uses of
>>> "of_find_node_by_path("/")" in the kernel.
>
> That is an interesting result.
>
>>> Figured I'd start here
>>> and see what the consensus was.
>>
>> As far as outside drivers/of/ is concerned, we certainly don't want to
>> expose global variables. But for most uses, you shouldn't need the
>> root node pointer because most iterating or searching functions treat
>> NULL as root.
>>
>> Rob
>
> For a simple answer, I'll go along with Rob here.
>
>
> "Here, hold my beer and watch this!"
>
> Things one should never say.  :-)
>
> Please don't run with this following idea yet.  This could open a can
> of worms that I've been trying to leave closed until I have some time
> scheduled to handle the fall out.

Now there are worms everywhere!

> The core devicetree code was not written with the concept of overlays
> in mind.  When the first portions of the overlay code were accepted,
> the architectural issues of the extending existing core code were not
> first addressed.  I don't even know if they were considered or
> discussed because I was not involved at the time.
>
> Some key things that were not updated to account for overlays:
>
>   1) locking (protection of the live devicetree data structures)
>
>   2) pointers to live devicetree data structures being handed out
>      to kernel code outside the core devicetree code

3) the possibility of multiple independent roots not tied into any
base DT. Think of USB FTDI type chips with things hanging off of
UARTs, I2C, GPIO, etc.

> One result of this is that after an overlay is applied, we can never
> free the memory containing the overlay FDT and the overlay expanded
> tree, because we do not know if any other part of the kernel has a
> pointer into the overlay data.
>
> I haven't dug into this in any depth yet, but it seems like it should
> be possible to create a new set of devicetree APIS, called by drivers
> and other parts of the kernel that retrieve information from the
> devicetree.  This API can not return pointers into the core devicetree
> data structures.  Thus no node pointers, no property pointers, no
> property value pointers.

Or it could be just opaque pointers (aka handles). Or could be split
into public and private structs/pointers. Returning raw property data
and length will still be needed for example.

> We may find some easy solution for this, but I suspect not.
>
> That's a long winded way of saying that maybe we should not spend
> effort on cleaning up non-critical issues with the way the
> current API is being used, _if_ the current API is going to
> be replaced long term.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of: get of_root directly in place of of_find_node_by_path("/")
  2018-03-08  1:14         ` Rob Herring
@ 2018-03-08  1:36           ` Frank Rowand
  0 siblings, 0 replies; 9+ messages in thread
From: Frank Rowand @ 2018-03-08  1:36 UTC (permalink / raw)
  To: Rob Herring; +Cc: Tyrel Datwyler, devicetree

On 03/07/18 17:14, Rob Herring wrote:
> On Wed, Mar 7, 2018 at 5:59 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 03/07/18 14:21, Rob Herring wrote:
>>> On Wed, Mar 7, 2018 at 4:14 PM, Tyrel Datwyler
>>> <tyreld@linux.vnet.ibm.com> wrote:
>>>> On 03/07/2018 12:38 PM, Rob Herring wrote:
>>>>> On Fri, Mar 02, 2018 at 08:46:44PM -0500, Tyrel Datwyler wrote:
>>>>>> There are a couple places in drivers/of where the root node is found
>>>>>> with a of_find_node_by_path("/") call. This call just returns
>>>>>> of_node_get(of_root) when the the path "/" is passed as a parameter.
>>>>>> So, lets fixup these instances to just do that instead.
>>>>>>
>>>>>> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>  drivers/of/base.c     | 2 +-
>>>>>>  drivers/of/platform.c | 4 ++--
>>>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>>>> index ad28de9..94c1b4d 100644
>>>>>> --- a/drivers/of/base.c
>>>>>> +++ b/drivers/of/base.c
>>>>>> @@ -455,7 +455,7 @@ int of_machine_is_compatible(const char *compat)
>>>>>>      struct device_node *root;
>>>>>>      int rc = 0;
>>>>>>
>>>>>> -    root = of_find_node_by_path("/");
>>>>>> +    root = of_node_get(of_root);
>>>>>
>>>>> The former seems more readable to me. I'll wait and see if Frank has any
>>>>> comments on this change.>>
>>>> Fair enough. I find them equally readable now that we have a well
>>>> named/defined global, namely "of_root", pointing at the device tree
>>>> root. My primary goal was saving a function call, but I wasn't sure
>>>> if anybody really cares enough. A quick check shows ~77 uses of
>>>> "of_find_node_by_path("/")" in the kernel.
>>
>> That is an interesting result.
>>
>>>> Figured I'd start here
>>>> and see what the consensus was.
>>>
>>> As far as outside drivers/of/ is concerned, we certainly don't want to
>>> expose global variables. But for most uses, you shouldn't need the
>>> root node pointer because most iterating or searching functions treat
>>> NULL as root.
>>>
>>> Rob
>>
>> For a simple answer, I'll go along with Rob here.
>>
>>
>> "Here, hold my beer and watch this!"
>>
>> Things one should never say.  :-)
>>
>> Please don't run with this following idea yet.  This could open a can
>> of worms that I've been trying to leave closed until I have some time
>> scheduled to handle the fall out.
> 
> Now there are worms everywhere!
> 
>> The core devicetree code was not written with the concept of overlays
>> in mind.  When the first portions of the overlay code were accepted,
>> the architectural issues of the extending existing core code were not
>> first addressed.  I don't even know if they were considered or
>> discussed because I was not involved at the time.
>>
>> Some key things that were not updated to account for overlays:
>>
>>   1) locking (protection of the live devicetree data structures)
>>
>>   2) pointers to live devicetree data structures being handed out
>>      to kernel code outside the core devicetree code
> 
> 3) the possibility of multiple independent roots not tied into any
> base DT. Think of USB FTDI type chips with things hanging off of
> UARTs, I2C, GPIO, etc.
> 
>> One result of this is that after an overlay is applied, we can never
>> free the memory containing the overlay FDT and the overlay expanded
>> tree, because we do not know if any other part of the kernel has a
>> pointer into the overlay data.
>>
>> I haven't dug into this in any depth yet, but it seems like it should
>> be possible to create a new set of devicetree APIS, called by drivers
>> and other parts of the kernel that retrieve information from the
>> devicetree.  This API can not return pointers into the core devicetree
>> data structures.  Thus no node pointers, no property pointers, no
>> property value pointers.
> 
> Or it could be just opaque pointers (aka handles). Or could be split
> into public and private structs/pointers. Returning raw property data
> and length will still be needed for example.

Yes, my naive assumption, without having thought about things very
hard, is that opaque objects are going to be involved.

For property data, I'm hoping that we can allocate memory to copy
the property into and return the copy to the caller.  Then the
memory returned to the caller  is owned by the caller and they
are responsible for freeing it when they are done with it.


>> We may find some easy solution for this, but I suspect not.
>>
>> That's a long winded way of saying that maybe we should not spend
>> effort on cleaning up non-critical issues with the way the
>> current API is being used, _if_ the current API is going to
>> be replaced long term.
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of: get of_root directly in place of of_find_node_by_path("/")
  2018-03-07 22:21     ` Rob Herring
  2018-03-07 23:59       ` Frank Rowand
@ 2018-03-08  1:36       ` Tyrel Datwyler
  2018-03-08 14:09         ` Rob Herring
  1 sibling, 1 reply; 9+ messages in thread
From: Tyrel Datwyler @ 2018-03-08  1:36 UTC (permalink / raw)
  To: Rob Herring; +Cc: Frank Rowand, devicetree

On 03/07/2018 02:21 PM, Rob Herring wrote:
> On Wed, Mar 7, 2018 at 4:14 PM, Tyrel Datwyler
> <tyreld@linux.vnet.ibm.com> wrote:
>> On 03/07/2018 12:38 PM, Rob Herring wrote:
>>> On Fri, Mar 02, 2018 at 08:46:44PM -0500, Tyrel Datwyler wrote:
>>>> There are a couple places in drivers/of where the root node is found
>>>> with a of_find_node_by_path("/") call. This call just returns
>>>> of_node_get(of_root) when the the path "/" is passed as a parameter.
>>>> So, lets fixup these instances to just do that instead.
>>>>
>>>> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
>>>> ---
>>>>  drivers/of/base.c     | 2 +-
>>>>  drivers/of/platform.c | 4 ++--
>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>> index ad28de9..94c1b4d 100644
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>> @@ -455,7 +455,7 @@ int of_machine_is_compatible(const char *compat)
>>>>      struct device_node *root;
>>>>      int rc = 0;
>>>>
>>>> -    root = of_find_node_by_path("/");
>>>> +    root = of_node_get(of_root);
>>>
>>> The former seems more readable to me. I'll wait and see if Frank has any
>>> comments on this change.
>>
>> Fair enough. I find them equally readable now that we have a well named/defined global, namely "of_root",  pointing at the device tree root. My primary goal was saving a function call, but I wasn't sure if anybody really cares enough. A quick check shows ~77 uses of "of_find_node_by_path("/")" in the kernel. Figured I'd start here and see what the consensus was.
> 
> As far as outside drivers/of/ is concerned, we certainly don't want to
> expose global variables. But for most uses, you shouldn't need the
> root node pointer because most iterating or searching functions treat
> NULL as root.

In the majority of cases I've seen code searching or iterating that wants to start at root just passes NULL. So, agreed. I was more looking at cases where code actually wants the root node for looking up properties of root. Part of my misguidance is likely due to "of_root" being an exported symbol.

-Tyrel

> 
> Rob
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] of: get of_root directly in place of of_find_node_by_path("/")
  2018-03-08  1:36       ` Tyrel Datwyler
@ 2018-03-08 14:09         ` Rob Herring
  0 siblings, 0 replies; 9+ messages in thread
From: Rob Herring @ 2018-03-08 14:09 UTC (permalink / raw)
  To: Tyrel Datwyler; +Cc: Frank Rowand, devicetree

On Wed, Mar 7, 2018 at 7:36 PM, Tyrel Datwyler
<tyreld@linux.vnet.ibm.com> wrote:
> On 03/07/2018 02:21 PM, Rob Herring wrote:
>> On Wed, Mar 7, 2018 at 4:14 PM, Tyrel Datwyler
>> <tyreld@linux.vnet.ibm.com> wrote:
>>> On 03/07/2018 12:38 PM, Rob Herring wrote:
>>>> On Fri, Mar 02, 2018 at 08:46:44PM -0500, Tyrel Datwyler wrote:
>>>>> There are a couple places in drivers/of where the root node is found
>>>>> with a of_find_node_by_path("/") call. This call just returns
>>>>> of_node_get(of_root) when the the path "/" is passed as a parameter.
>>>>> So, lets fixup these instances to just do that instead.
>>>>>
>>>>> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
>>>>> ---
>>>>>  drivers/of/base.c     | 2 +-
>>>>>  drivers/of/platform.c | 4 ++--
>>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>>> index ad28de9..94c1b4d 100644
>>>>> --- a/drivers/of/base.c
>>>>> +++ b/drivers/of/base.c
>>>>> @@ -455,7 +455,7 @@ int of_machine_is_compatible(const char *compat)
>>>>>      struct device_node *root;
>>>>>      int rc = 0;
>>>>>
>>>>> -    root = of_find_node_by_path("/");
>>>>> +    root = of_node_get(of_root);
>>>>
>>>> The former seems more readable to me. I'll wait and see if Frank has any
>>>> comments on this change.
>>>
>>> Fair enough. I find them equally readable now that we have a well named/defined global, namely "of_root",  pointing at the device tree root. My primary goal was saving a function call, but I wasn't sure if anybody really cares enough. A quick check shows ~77 uses of "of_find_node_by_path("/")" in the kernel. Figured I'd start here and see what the consensus was.
>>
>> As far as outside drivers/of/ is concerned, we certainly don't want to
>> expose global variables. But for most uses, you shouldn't need the
>> root node pointer because most iterating or searching functions treat
>> NULL as root.
>
> In the majority of cases I've seen code searching or iterating that wants to start at root just passes NULL. So, agreed. I was more looking at cases where code actually wants the root node for looking up properties of root. Part of my misguidance is likely due to "of_root" being an exported symbol.

All of these could be replaced with of_machine_is_compatible():

arch/powerpc/platforms/40x/ppc40x_simple.c:     if
(of_device_compatible_match(of_root, board)) {
arch/powerpc/platforms/512x/mpc512x_generic.c:  if
(!of_device_compatible_match(of_root, board))
arch/powerpc/platforms/52xx/efika.c:    const char *model =
of_get_property(of_root, "model", NULL);
arch/powerpc/platforms/52xx/lite5200.c: return
of_device_compatible_match(of_root, board);
arch/powerpc/platforms/52xx/media5200.c:        return
of_device_compatible_match(of_root, board);
arch/powerpc/platforms/52xx/mpc5200_simple.c:   return
of_device_compatible_match(of_root, board);
arch/powerpc/platforms/83xx/mpc830x_rdb.c:      return
of_device_compatible_match(of_root, board);
arch/powerpc/platforms/83xx/mpc831x_rdb.c:      return
of_device_compatible_match(of_root, board);
arch/powerpc/platforms/83xx/mpc837x_rdb.c:      return
of_device_compatible_match(of_root, board);
arch/powerpc/platforms/85xx/corenet_generic.c:  if
(of_device_compatible_match(of_root, boards))
arch/powerpc/platforms/85xx/tqm85xx.c:  return
of_device_compatible_match(of_root, board);


There's lots of open coding to get the model string. Things like this
(in sio_init) could be replaced with either of_machine_is_compatible
(not sure if that would be equivalent) or a helper to get the model:

        root = of_find_node_by_path("/");
        if (!root)
                return;

        model = of_get_property(root, "model", NULL);
        if (model && !strncmp(model, "IBM,LongTrail", 13)) {


Rob

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-03-08 14:09 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-03  1:46 [PATCH] of: get of_root directly in place of of_find_node_by_path("/") Tyrel Datwyler
2018-03-07 20:38 ` Rob Herring
2018-03-07 22:14   ` Tyrel Datwyler
2018-03-07 22:21     ` Rob Herring
2018-03-07 23:59       ` Frank Rowand
2018-03-08  1:14         ` Rob Herring
2018-03-08  1:36           ` Frank Rowand
2018-03-08  1:36       ` Tyrel Datwyler
2018-03-08 14:09         ` Rob Herring

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.