| Message ID | 1338940402-28502-6-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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); >
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
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 >
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
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 >
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
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);
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(-)