Hi Peter,
On 12/18/2015 03:23 PM, Peter Maydell wrote:
> On 17 December 2015 at 12:29, Eric Auger <eric.au...@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.au...@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
>
