| Message ID | 1450355367-14818-4-git-send-email-eric.auger@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote: > This new helper routine returns the node path of a device > referred to by its node name and compat string. > > Signed-off-by: Eric Auger <eric.auger@linaro.org> > > --- > > RFC -> v1: > - improve error handling according to Alex' comments > --- > device_tree.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ > include/sysemu/device_tree.h | 3 +++ > 2 files changed, 48 insertions(+) > > diff --git a/device_tree.c b/device_tree.c > index e556a99..b5d7e0b 100644 > --- a/device_tree.c > +++ b/device_tree.c > @@ -233,6 +233,51 @@ static int findnode_nofail(void *fdt, const char *node_path) > return offset; > } > > +/** > + * qemu_fdt_node_path > + * > + * return the node path of a device, given its node name and its > + * compat string > + * fdt: pointer to the dt blob > + * name: device node name > + * compat: compatibility string of the device > + * > + * upon success, the path is output at node_path address > + * returns 0 on success, < 0 on failure > + */ Can we put the doc comment in the header file, since this is a globally visible function? Also it would be nice to follow the doc-comment syntax standards about marking up arguments with '@' and so on. > +int qemu_fdt_node_path(void *fdt, const char *name, char *compat, > + char **node_path) > +{ > + int offset, len, ret; > + const char *iter_name; > + char path[256]; Rather than a fixed buffer size, we should check whether fdt_get_path returns -FDT_ERR_NOSPACE and if so enlarge the buffer and try again. > + > + *node_path = NULL; > + offset = fdt_node_offset_by_compatible(fdt, -1, compat); > + while (offset != -FDT_ERR_NOTFOUND) { > + if (offset < 0) { > + continue; I don't understand this continue -- if the fdt function returned any error other than -FDT_ERR_NOTFOUND then this will cause us to go into an infinite loop around this while(). Did you mean 'break' ? (Though if you just want to break then fixing the while condition would be better.) > + } > + iter_name = fdt_get_name(fdt, offset, &len); > + if (!iter_name) { > + continue; This also seems like it ought to be a break, except you need to set offset to the error code first (which fdt_get_name() will have returned in 'len'). > + } > + > + if (!strncmp(iter_name, name, len)) { Do we really want strncmp rather than strcmp here ? (ie "find first node whose name has 'name' as a prefix" rather than "find first node whose name matches 'name'). > + goto found; > + } > + offset = fdt_node_offset_by_compatible(fdt, offset, compat); > + } > + return offset; > + > +found: > + ret = fdt_get_path(fdt, offset, path, 256); > + if (!ret) { > + *node_path = g_strdup(path); > + } > + return ret; > +} > + > int qemu_fdt_setprop(void *fdt, const char *node_path, > const char *property, const void *val, int size) > { > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h > index 307e53d..f9e6e6e 100644 > --- a/include/sysemu/device_tree.h > +++ b/include/sysemu/device_tree.h > @@ -18,6 +18,9 @@ void *create_device_tree(int *sizep); > void *load_device_tree(const char *filename_path, int *sizep); > void *load_device_tree_from_sysfs(void); > > +int qemu_fdt_node_path(void *fdt, const char *name, char *compat, > + char **node_path); > + > int qemu_fdt_setprop(void *fdt, const char *node_path, > const char *property, const void *val, int size); > int qemu_fdt_setprop_cell(void *fdt, const char *node_path, > -- > 1.9.1 thanks -- PMM
Hi Peter, On 12/18/2015 03:23 PM, Peter Maydell wrote: > On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote: >> This new helper routine returns the node path of a device >> referred to by its node name and compat string. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> --- >> >> RFC -> v1: >> - improve error handling according to Alex' comments >> --- >> device_tree.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ >> include/sysemu/device_tree.h | 3 +++ >> 2 files changed, 48 insertions(+) >> >> diff --git a/device_tree.c b/device_tree.c >> index e556a99..b5d7e0b 100644 >> --- a/device_tree.c >> +++ b/device_tree.c >> @@ -233,6 +233,51 @@ static int findnode_nofail(void *fdt, const char *node_path) >> return offset; >> } >> >> +/** >> + * qemu_fdt_node_path >> + * >> + * return the node path of a device, given its node name and its >> + * compat string >> + * fdt: pointer to the dt blob >> + * name: device node name >> + * compat: compatibility string of the device >> + * >> + * upon success, the path is output at node_path address >> + * returns 0 on success, < 0 on failure >> + */ > > Can we put the doc comment in the header file, since this is > a globally visible function? Also it would be nice to follow the > doc-comment syntax standards about marking up arguments with '@' > and so on. sure > >> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat, >> + char **node_path) >> +{ >> + int offset, len, ret; >> + const char *iter_name; >> + char path[256]; > > Rather than a fixed buffer size, we should check whether > fdt_get_path returns -FDT_ERR_NOSPACE and if so enlarge the > buffer and try again. OK > >> + >> + *node_path = NULL; >> + offset = fdt_node_offset_by_compatible(fdt, -1, compat); >> + while (offset != -FDT_ERR_NOTFOUND) { >> + if (offset < 0) { >> + continue; > > I don't understand this continue -- if the fdt function returned any > error other than -FDT_ERR_NOTFOUND then this will cause us to go > into an infinite loop around this while(). Did you mean 'break' ? > (Though if you just want to break then fixing the while condition > would be better.) My first understanding of the API was fdt_node_offset_by_compatible would increment the offset even if an error occurred; so I envisioned to continue parsing the tree, looking for another node with same features. But I think it is overkill anyway and I will abort. > >> + } >> + iter_name = fdt_get_name(fdt, offset, &len); >> + if (!iter_name) { >> + continue; > > This also seems like it ought to be a break, except you need to > set offset to the error code first (which fdt_get_name() will > have returned in 'len'). yes will set offset to len and then break; > >> + } >> + >> + if (!strncmp(iter_name, name, len)) { > > Do we really want strncmp rather than strcmp here ? (ie > "find first node whose name has 'name' as a prefix" rather > than "find first node whose name matches 'name'). true strcmp is OK Thanks Eric > >> + goto found; >> + } >> + offset = fdt_node_offset_by_compatible(fdt, offset, compat); >> + } >> + return offset; >> + >> +found: >> + ret = fdt_get_path(fdt, offset, path, 256); >> + if (!ret) { >> + *node_path = g_strdup(path); >> + } >> + return ret; >> +} >> + >> int qemu_fdt_setprop(void *fdt, const char *node_path, >> const char *property, const void *val, int size) >> { >> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h >> index 307e53d..f9e6e6e 100644 >> --- a/include/sysemu/device_tree.h >> +++ b/include/sysemu/device_tree.h >> @@ -18,6 +18,9 @@ void *create_device_tree(int *sizep); >> void *load_device_tree(const char *filename_path, int *sizep); >> void *load_device_tree_from_sysfs(void); >> >> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat, >> + char **node_path); >> + >> int qemu_fdt_setprop(void *fdt, const char *node_path, >> const char *property, const void *val, int size); >> int qemu_fdt_setprop_cell(void *fdt, const char *node_path, >> -- >> 1.9.1 > > thanks > -- PMM >
On 5 January 2016 at 16:20, Eric Auger <eric.auger@linaro.org> wrote: > Hi Peter, > On 12/18/2015 03:23 PM, Peter Maydell wrote: >> On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote: >>> This new helper routine returns the node path of a device >>> referred to by its node name and compat string. >>> >>> Signed-off-by: Eric Auger <eric.auger@linaro.org> >>> + >>> + *node_path = NULL; >>> + offset = fdt_node_offset_by_compatible(fdt, -1, compat); >>> + while (offset != -FDT_ERR_NOTFOUND) { >>> + if (offset < 0) { >>> + continue; >> >> I don't understand this continue -- if the fdt function returned any >> error other than -FDT_ERR_NOTFOUND then this will cause us to go >> into an infinite loop around this while(). Did you mean 'break' ? >> (Though if you just want to break then fixing the while condition >> would be better.) > My first understanding of the API was fdt_node_offset_by_compatible > would increment the offset even if an error occurred; so I envisioned to > continue parsing the tree, looking for another node with same features. Your code doesn't call fdt_node_offset_by_compatible again in the case where it's trying to continue, though... thanks -- PMM
On 01/05/2016 06:55 PM, Peter Maydell wrote: > On 5 January 2016 at 16:20, Eric Auger <eric.auger@linaro.org> wrote: >> Hi Peter, >> On 12/18/2015 03:23 PM, Peter Maydell wrote: >>> On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote: >>>> This new helper routine returns the node path of a device >>>> referred to by its node name and compat string. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@linaro.org> > >>>> + >>>> + *node_path = NULL; >>>> + offset = fdt_node_offset_by_compatible(fdt, -1, compat); >>>> + while (offset != -FDT_ERR_NOTFOUND) { >>>> + if (offset < 0) { >>>> + continue; >>> >>> I don't understand this continue -- if the fdt function returned any >>> error other than -FDT_ERR_NOTFOUND then this will cause us to go >>> into an infinite loop around this while(). Did you mean 'break' ? >>> (Though if you just want to break then fixing the while condition >>> would be better.) >> My first understanding of the API was fdt_node_offset_by_compatible >> would increment the offset even if an error occurred; so I envisioned to >> continue parsing the tree, looking for another node with same features. > > Your code doesn't call fdt_node_offset_by_compatible again > in the case where it's trying to continue, though... I'll be damned, got it now! Thanks Eric > > thanks > -- PMM >
diff --git a/device_tree.c b/device_tree.c index e556a99..b5d7e0b 100644 --- a/device_tree.c +++ b/device_tree.c @@ -233,6 +233,51 @@ static int findnode_nofail(void *fdt, const char *node_path) return offset; } +/** + * qemu_fdt_node_path + * + * return the node path of a device, given its node name and its + * compat string + * fdt: pointer to the dt blob + * name: device node name + * compat: compatibility string of the device + * + * upon success, the path is output at node_path address + * returns 0 on success, < 0 on failure + */ +int qemu_fdt_node_path(void *fdt, const char *name, char *compat, + char **node_path) +{ + int offset, len, ret; + const char *iter_name; + char path[256]; + + *node_path = NULL; + offset = fdt_node_offset_by_compatible(fdt, -1, compat); + while (offset != -FDT_ERR_NOTFOUND) { + if (offset < 0) { + continue; + } + iter_name = fdt_get_name(fdt, offset, &len); + if (!iter_name) { + continue; + } + + if (!strncmp(iter_name, name, len)) { + goto found; + } + offset = fdt_node_offset_by_compatible(fdt, offset, compat); + } + return offset; + +found: + ret = fdt_get_path(fdt, offset, path, 256); + if (!ret) { + *node_path = g_strdup(path); + } + return ret; +} + int qemu_fdt_setprop(void *fdt, const char *node_path, const char *property, const void *val, int size) { diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h index 307e53d..f9e6e6e 100644 --- a/include/sysemu/device_tree.h +++ b/include/sysemu/device_tree.h @@ -18,6 +18,9 @@ void *create_device_tree(int *sizep); void *load_device_tree(const char *filename_path, int *sizep); void *load_device_tree_from_sysfs(void); +int qemu_fdt_node_path(void *fdt, const char *name, char *compat, + char **node_path); + int qemu_fdt_setprop(void *fdt, const char *node_path, const char *property, const void *val, int size); int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
This new helper routine returns the node path of a device referred to by its node name and compat string. Signed-off-by: Eric Auger <eric.auger@linaro.org> --- RFC -> v1: - improve error handling according to Alex' comments --- device_tree.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ include/sysemu/device_tree.h | 3 +++ 2 files changed, 48 insertions(+) -- 1.9.1