Re: [Qemu-ppc] [Qemu-devel] [PATCH 05/31] dt: add helper for phandle enumeration

[Alexander Graf]

On 09.06.2012, at 03:02, Peter Crosthwaite wrote:

> On Fri, Jun 8, 2012 at 10:46 PM, Alexander Graf <address@hidden> wrote:
>> 
>> On 07.06.2012, at 02:28, Peter Crosthwaite wrote:
>> 
>>> On Thu, Jun 7, 2012 at 1:58 AM, Alexander Graf <address@hidden> 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<address@hidden>
>>>>>> ---
>>>>>>  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