Patchwork [05/31] dt: add helper for phandle enumeration

login
register
mail settings
Submitter Alexander Graf
Date June 5, 2012, 11:52 p.m.
Message ID <1338940402-28502-6-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/163217/
State New
Headers show

Comments

Alexander Graf - June 5, 2012, 11:52 p.m.
This patch adds a helper to search for a node's phandle by its path. This
is especially useful when the phandle is part of an array, not just a single
cell in which case qemu_devtree_setprop_phandle would be the easy choice.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 device_tree.c |   16 +++++++++++++++-
 device_tree.h |    1 +
 2 files changed, 16 insertions(+), 1 deletions(-)
Peter A. G. Crosthwaite - June 6, 2012, 5:11 a.m.
On Wed, 2012-06-06 at 01:52 +0200, Alexander Graf wrote:
> This patch adds a helper to search for a node's phandle by its path. This
> is especially useful when the phandle is part of an array, not just a single
> cell in which case qemu_devtree_setprop_phandle would be the easy choice.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  device_tree.c |   16 +++++++++++++++-
>  device_tree.h |    1 +
>  2 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/device_tree.c b/device_tree.c
> index 6cbc5af..6745d17 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -162,10 +162,24 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
>      return r;
>  }
>  
> +uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
> +{
> +    uint32_t r;
> +
> +    r = fdt_get_phandle(fdt, findnode_nofail(fdt, path));
> +    if (r <= 0) {
> +        fprintf(stderr, "%s: Couldn't get phandle for %s: %s\n", __func__,
> +                path, fdt_strerror(r));
> +        exit(1);

Is it really this functions job to terminate qemu on fail?  There may be
scenarios where a node does not have a phandle where the client can
handle that. Perhaps return -1 on error and the client has to check?

> +    }
> +
> +    return r;
> +}
> +
>  int qemu_devtree_setprop_phandle(void *fdt, const char *node_path,
>                                   const char *property, const char *string)
>  {
> -    uint32_t phandle = fdt_get_phandle(fdt, findnode_nofail(fdt, string));
> +    uint32_t phandle = qemu_devtree_get_phandle(fdt, string);
>      return qemu_devtree_setprop_cell(fdt, node_path, property, phandle);
>  }
>  
> diff --git a/device_tree.h b/device_tree.h
> index 2e87c58..376287a 100644
> --- a/device_tree.h
> +++ b/device_tree.h
> @@ -33,6 +33,7 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
>                                  const char *property, const char *string);
>  int qemu_devtree_setprop_phandle(void *fdt, const char *node_path,
>                                   const char *property, const char *string);
> +uint32_t qemu_devtree_get_phandle(void *fdt, const char *path);
>  int qemu_devtree_nop_node(void *fdt, const char *node_path);
>  int qemu_devtree_add_subnode(void *fdt, const char *name);
>
Alexander Graf - June 6, 2012, 3:58 p.m.
On 06/06/2012 07:11 AM, Peter Crosthwaite wrote:
> On Wed, 2012-06-06 at 01:52 +0200, Alexander Graf wrote:
>> This patch adds a helper to search for a node's phandle by its path. This
>> is especially useful when the phandle is part of an array, not just a single
>> cell in which case qemu_devtree_setprop_phandle would be the easy choice.
>>
>> Signed-off-by: Alexander Graf<agraf@suse.de>
>> ---
>>   device_tree.c |   16 +++++++++++++++-
>>   device_tree.h |    1 +
>>   2 files changed, 16 insertions(+), 1 deletions(-)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index 6cbc5af..6745d17 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -162,10 +162,24 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path,
>>       return r;
>>   }
>>
>> +uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
>> +{
>> +    uint32_t r;
>> +
>> +    r = fdt_get_phandle(fdt, findnode_nofail(fdt, path));
>> +    if (r<= 0) {
>> +        fprintf(stderr, "%s: Couldn't get phandle for %s: %s\n", __func__,
>> +                path, fdt_strerror(r));
>> +        exit(1);
> Is it really this functions job to terminate qemu on fail?  There may be
> scenarios where a node does not have a phandle where the client can
> handle that. Perhaps return -1 on error and the client has to check?

If it can, what's the point in not calling libfdt directly then?


Alex
Peter A. G. Crosthwaite - June 7, 2012, 12:28 a.m.
On Thu, Jun 7, 2012 at 1:58 AM, Alexander Graf <agraf@suse.de> wrote:
> On 06/06/2012 07:11 AM, Peter Crosthwaite wrote:
>>
>> On Wed, 2012-06-06 at 01:52 +0200, Alexander Graf wrote:
>>>
>>> This patch adds a helper to search for a node's phandle by its path. This
>>> is especially useful when the phandle is part of an array, not just a
>>> single
>>> cell in which case qemu_devtree_setprop_phandle would be the easy choice.
>>>
>>> Signed-off-by: Alexander Graf<agraf@suse.de>
>>> ---
>>>  device_tree.c |   16 +++++++++++++++-
>>>  device_tree.h |    1 +
>>>  2 files changed, 16 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/device_tree.c b/device_tree.c
>>> index 6cbc5af..6745d17 100644
>>> --- a/device_tree.c
>>> +++ b/device_tree.c
>>> @@ -162,10 +162,24 @@ int qemu_devtree_setprop_string(void *fdt, const
>>> char *node_path,
>>>      return r;
>>>  }
>>>
>>> +uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
>>> +{
>>> +    uint32_t r;
>>> +
>>> +    r = fdt_get_phandle(fdt, findnode_nofail(fdt, path));
>>> +    if (r<= 0) {
>>> +        fprintf(stderr, "%s: Couldn't get phandle for %s: %s\n",
>>> __func__,
>>> +                path, fdt_strerror(r));
>>> +        exit(1);
>>
>> Is it really this functions job to terminate qemu on fail?  There may be
>> scenarios where a node does not have a phandle where the client can
>> handle that. Perhaps return -1 on error and the client has to check?
>
>
> If it can, what's the point in not calling libfdt directly then?
>

Its a very good question. If the point of this function is to fail of
error though, perhaps it should have the _nofail suffix for clarity?

>
> Alex
>
Alexander Graf - June 8, 2012, 12:46 p.m.
On 07.06.2012, at 02:28, Peter Crosthwaite wrote:

> On Thu, Jun 7, 2012 at 1:58 AM, Alexander Graf <agraf@suse.de> wrote:
>> On 06/06/2012 07:11 AM, Peter Crosthwaite wrote:
>>> 
>>> On Wed, 2012-06-06 at 01:52 +0200, Alexander Graf wrote:
>>>> 
>>>> This patch adds a helper to search for a node's phandle by its path. This
>>>> is especially useful when the phandle is part of an array, not just a
>>>> single
>>>> cell in which case qemu_devtree_setprop_phandle would be the easy choice.
>>>> 
>>>> Signed-off-by: Alexander Graf<agraf@suse.de>
>>>> ---
>>>>  device_tree.c |   16 +++++++++++++++-
>>>>  device_tree.h |    1 +
>>>>  2 files changed, 16 insertions(+), 1 deletions(-)
>>>> 
>>>> diff --git a/device_tree.c b/device_tree.c
>>>> index 6cbc5af..6745d17 100644
>>>> --- a/device_tree.c
>>>> +++ b/device_tree.c
>>>> @@ -162,10 +162,24 @@ int qemu_devtree_setprop_string(void *fdt, const
>>>> char *node_path,
>>>>      return r;
>>>>  }
>>>> 
>>>> +uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
>>>> +{
>>>> +    uint32_t r;
>>>> +
>>>> +    r = fdt_get_phandle(fdt, findnode_nofail(fdt, path));
>>>> +    if (r<= 0) {
>>>> +        fprintf(stderr, "%s: Couldn't get phandle for %s: %s\n",
>>>> __func__,
>>>> +                path, fdt_strerror(r));
>>>> +        exit(1);
>>> 
>>> Is it really this functions job to terminate qemu on fail?  There may be
>>> scenarios where a node does not have a phandle where the client can
>>> handle that. Perhaps return -1 on error and the client has to check?
>> 
>> 
>> If it can, what's the point in not calling libfdt directly then?
>> 
> 
> Its a very good question. If the point of this function is to fail of
> error though, perhaps it should have the _nofail suffix for clarity?

If we do a global s/qemu_devtree_/qdt/g throughout the code base, I'd be open to add _nofail to all function names at the end :). Otherwise we'll get into even more trouble of staying within 80 characters per line...


Alex
Peter A. G. Crosthwaite - June 9, 2012, 1:02 a.m.
On Fri, Jun 8, 2012 at 10:46 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 07.06.2012, at 02:28, Peter Crosthwaite wrote:
>
>> On Thu, Jun 7, 2012 at 1:58 AM, Alexander Graf <agraf@suse.de> wrote:
>>> On 06/06/2012 07:11 AM, Peter Crosthwaite wrote:
>>>>
>>>> On Wed, 2012-06-06 at 01:52 +0200, Alexander Graf wrote:
>>>>>
>>>>> This patch adds a helper to search for a node's phandle by its path. This
>>>>> is especially useful when the phandle is part of an array, not just a
>>>>> single
>>>>> cell in which case qemu_devtree_setprop_phandle would be the easy choice.
>>>>>
>>>>> Signed-off-by: Alexander Graf<agraf@suse.de>
>>>>> ---
>>>>>  device_tree.c |   16 +++++++++++++++-
>>>>>  device_tree.h |    1 +
>>>>>  2 files changed, 16 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/device_tree.c b/device_tree.c
>>>>> index 6cbc5af..6745d17 100644
>>>>> --- a/device_tree.c
>>>>> +++ b/device_tree.c
>>>>> @@ -162,10 +162,24 @@ int qemu_devtree_setprop_string(void *fdt, const
>>>>> char *node_path,
>>>>>      return r;
>>>>>  }
>>>>>
>>>>> +uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
>>>>> +{
>>>>> +    uint32_t r;
>>>>> +
>>>>> +    r = fdt_get_phandle(fdt, findnode_nofail(fdt, path));
>>>>> +    if (r<= 0) {
>>>>> +        fprintf(stderr, "%s: Couldn't get phandle for %s: %s\n",
>>>>> __func__,
>>>>> +                path, fdt_strerror(r));
>>>>> +        exit(1);
>>>>
>>>> Is it really this functions job to terminate qemu on fail?  There may be
>>>> scenarios where a node does not have a phandle where the client can
>>>> handle that. Perhaps return -1 on error and the client has to check?
>>>
>>>
>>> If it can, what's the point in not calling libfdt directly then?
>>>
>>
>> Its a very good question. If the point of this function is to fail of
>> error though, perhaps it should have the _nofail suffix for clarity?
>
> If we do a global s/qemu_devtree_/qdt/g throughout the code base, I'd be open to add _nofail to all function names at the end :). Otherwise we'll get into even more trouble of staying within 80 characters per line...
>

Since the majority of those functions are wrappers around "fdt_" API
calls, perhaps it should be:

s/qemu_devtree_/qemu_fdt_/g

buys you 4 chars, which should minimise the incidence of 80 char
violations when adding _nofail suffixes. Do we have a large number of
lines already between 78-80 chars?

Regards,
Peter

>
> Alex
>
Alexander Graf - June 19, 2012, 2:03 p.m.
On 09.06.2012, at 03:02, Peter Crosthwaite wrote:

> On Fri, Jun 8, 2012 at 10:46 PM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 07.06.2012, at 02:28, Peter Crosthwaite wrote:
>> 
>>> On Thu, Jun 7, 2012 at 1:58 AM, Alexander Graf <agraf@suse.de> wrote:
>>>> On 06/06/2012 07:11 AM, Peter Crosthwaite wrote:
>>>>> 
>>>>> On Wed, 2012-06-06 at 01:52 +0200, Alexander Graf wrote:
>>>>>> 
>>>>>> This patch adds a helper to search for a node's phandle by its path. This
>>>>>> is especially useful when the phandle is part of an array, not just a
>>>>>> single
>>>>>> cell in which case qemu_devtree_setprop_phandle would be the easy choice.
>>>>>> 
>>>>>> Signed-off-by: Alexander Graf<agraf@suse.de>
>>>>>> ---
>>>>>>  device_tree.c |   16 +++++++++++++++-
>>>>>>  device_tree.h |    1 +
>>>>>>  2 files changed, 16 insertions(+), 1 deletions(-)
>>>>>> 
>>>>>> diff --git a/device_tree.c b/device_tree.c
>>>>>> index 6cbc5af..6745d17 100644
>>>>>> --- a/device_tree.c
>>>>>> +++ b/device_tree.c
>>>>>> @@ -162,10 +162,24 @@ int qemu_devtree_setprop_string(void *fdt, const
>>>>>> char *node_path,
>>>>>>      return r;
>>>>>>  }
>>>>>> 
>>>>>> +uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
>>>>>> +{
>>>>>> +    uint32_t r;
>>>>>> +
>>>>>> +    r = fdt_get_phandle(fdt, findnode_nofail(fdt, path));
>>>>>> +    if (r<= 0) {
>>>>>> +        fprintf(stderr, "%s: Couldn't get phandle for %s: %s\n",
>>>>>> __func__,
>>>>>> +                path, fdt_strerror(r));
>>>>>> +        exit(1);
>>>>> 
>>>>> Is it really this functions job to terminate qemu on fail?  There may be
>>>>> scenarios where a node does not have a phandle where the client can
>>>>> handle that. Perhaps return -1 on error and the client has to check?
>>>> 
>>>> 
>>>> If it can, what's the point in not calling libfdt directly then?
>>>> 
>>> 
>>> Its a very good question. If the point of this function is to fail of
>>> error though, perhaps it should have the _nofail suffix for clarity?
>> 
>> If we do a global s/qemu_devtree_/qdt/g throughout the code base, I'd be open to add _nofail to all function names at the end :). Otherwise we'll get into even more trouble of staying within 80 characters per line...
>> 
> 
> Since the majority of those functions are wrappers around "fdt_" API
> calls, perhaps it should be:
> 
> s/qemu_devtree_/qemu_fdt_/g
> 
> buys you 4 chars, which should minimise the incidence of 80 char
> violations when adding _nofail suffixes. Do we have a large number of
> lines already between 78-80 chars?

Hrm. Let's keep this in mind for a later cleanup series. It certainly is out of scope of this patch set :).


Alex

Patch

diff --git a/device_tree.c b/device_tree.c
index 6cbc5af..6745d17 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -162,10 +162,24 @@  int qemu_devtree_setprop_string(void *fdt, const char *node_path,
     return r;
 }
 
+uint32_t qemu_devtree_get_phandle(void *fdt, const char *path)
+{
+    uint32_t r;
+
+    r = fdt_get_phandle(fdt, findnode_nofail(fdt, path));
+    if (r <= 0) {
+        fprintf(stderr, "%s: Couldn't get phandle for %s: %s\n", __func__,
+                path, fdt_strerror(r));
+        exit(1);
+    }
+
+    return r;
+}
+
 int qemu_devtree_setprop_phandle(void *fdt, const char *node_path,
                                  const char *property, const char *string)
 {
-    uint32_t phandle = fdt_get_phandle(fdt, findnode_nofail(fdt, string));
+    uint32_t phandle = qemu_devtree_get_phandle(fdt, string);
     return qemu_devtree_setprop_cell(fdt, node_path, property, phandle);
 }
 
diff --git a/device_tree.h b/device_tree.h
index 2e87c58..376287a 100644
--- a/device_tree.h
+++ b/device_tree.h
@@ -33,6 +33,7 @@  int qemu_devtree_setprop_string(void *fdt, const char *node_path,
                                 const char *property, const char *string);
 int qemu_devtree_setprop_phandle(void *fdt, const char *node_path,
                                  const char *property, const char *string);
+uint32_t qemu_devtree_get_phandle(void *fdt, const char *path);
 int qemu_devtree_nop_node(void *fdt, const char *node_path);
 int qemu_devtree_add_subnode(void *fdt, const char *name);